While I was simplifiying and optimizing the code in robotstxt_robots(), I found what could potentially be a *very* trivial security flaw in the module. Because the content type is set to 'text/plain' *before* the calls to hook_robotstxt() from additional modules, theoretically a module could alter the content type of the request and add malicious content:
// The current implementation:
function robotstxt_robots() {
$base = variable_get('robotstxt', FALSE);
if ($base === FALSE) {
$base = _robotstxt_get_file_contents();
}
drupal_set_header("Content-type: text/plain");
echo trim(check_plain($base));
// Hook other modules for adding additional lines to robots.txt
$lines = module_invoke_all('robotstxt');
foreach ($lines as $line) {
$line = check_plain($line);
if ($line != '') {
echo "\n" . trim($line);
}
}
exit;
}
// In badmodule.module:
function badmodule_robotstxt() {
drupal_set_header("Content-type: text/html");
return array(
'<script>alert(\'Hello!\');</script>',
);
}
Instead of having to run everything through check_plain(), if we set the content type to 'text/plain' after we get all the content, there can be no problem at all and we save processing time.
Attached patch rewrites the logic of robotstxt_robots() and _robotstxt_get_file_contents() to work more security and efficiently.
function robotstxt_robots() {
$content = array();
$content[] = _robotstxt_get_content();
// Hook other modules for adding additional lines.
if ($additions = module_invoke_all('robotstxt')) {
$content[] = '# Module defined additions';
$content = array_merge($content, $additions);
}
// Trim any extra whitespace and filter out empty strings.
$content = array_map('trim', $content);
$content = array_filter($content);
drupal_set_header("Content-type: text/plain");
echo implode("\n", $content);
exit;
}
Notice all the code sections:
$base = variable_get('robotstxt', FALSE);
if ($base === FALSE) {
$base = _robotstxt_get_file_contents();
}
Now only need to call _robotstxt_get_content().
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 0-robotstxt-optimize-D6.patch | 3.36 KB | dave reid |
| 0-robotstxt-optimize-D6.patch | 3.31 KB | dave reid |
Comments
Comment #1
dave reidComment #2
hass commentedA little bit theoretical... this is no real security issue! I'm not committing this for now as it is a duplicate of #350470: robots.txt content doesn't need to be passed to check_plain() and I'm missing information.
Comment #3
dave reidI'll look into that, but we could easily add
array_map('check_plain', $content);as well, and it would run all the strings using check_plain() but in a more efficient way. This is really something much different than a duplicate of #350470: robots.txt content doesn't need to be passed to check_plain() since the main focus of this patch is the optimization of the functions.What do you think about the changes to _robotstxt_get_file_contents()?
Comment #4
hass commentedThe rest looks good, but you fix the other issue together, what wouldn't be wrong... but I wish to see the docs first... therefore postponed.
Comment #5
dave reidHass, you did see that I revised the patch in #3 to run check_plain() on all the strings right? There's no *actual* change in functionality and it's not removing any check_plain()s that didn't happen.
What docs would you like to see?
Comment #6
hass commentedI'd really like to remove the check_plain... but not before we know 200% for sure that URLs with query params are allowed by the robots.txt spec. I also thought some time about running XSS filter on the content to remove HTML code. Only to be save...
Comment #7
dave reidThat's fine, we can figure out what to do about removing check_plain() in #350470: robots.txt content doesn't need to be passed to check_plain(). But for this issue are there any other concerns or questions you have about the patch in #3? Or does it look good to commit?
Comment #8
dave reidDid some performance benchmarks with
ab -c 2 -n 1000 http://mysql.drupal6.local/robots.txt:CURRENT IMPLEMENTATION (WITHOUT PATCH)
WITH PATCH #3 (with array_map('check_plain', $content))
and just for kicks...
WITH ORIGINAL PATCH (without array_map('check_plain', $content))
Comment #9
hass commentedWhy is the code slower? 0.25 seconds slower is really much.
Comment #10
dave reidWell, the code in the patch is optimized to process an array efficiently as compared to the slower current code (using array_map, only running the check_plain until after all the empty strings have been removed, etc). Also, it runs it's output in one shot (
echo implode("\n", $content);) instead of running multiple echo statements.Comment #11
hass commentedI have removed the line
$content[] = '# Module defined additions';as I think it is not required.Comment #12
dave reidYeah that's no problem. It was just a nice indicator to show where and what was being added by any hook_robotstxt() implementations. Not something critical at all.
Comment #13
hass commentedShould be backported to D5.
Comment #14
hass commentedComment #15
hass commented