Well crafted urls such as /node?destination[]=node can reveal the full path of the directory where Drupal is installed:
warning: urlencode() expects parameter 1 to be string, array given in /Applications/MAMP/htdocs/d616/includes/common.inc on line 258.
See more details at http://blog.zerodayscan.com/2010/04/full-path-disclosure-bug-in-drupal-6... and discussion at http://drupal.org/node/783430. This was reported to the Security team which decided this could be fixed publicly. Production servers should set PHP.ini's display_error to OFF and Drupal's error reporting to 'log only' instead of 'log and display'.
To reproduce, browse to /node?destination[]=node on a Drupal 6 site as anonymous user. I'm not able to reproduce this on Drupal 7.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | drupal.display-errors.10.patch | 2.64 KB | sun |
| #3 | 783618-narcissistic-vulnerability-pimp-D7.patch | 669 bytes | dave reid |
Comments
Comment #1
heine commentedThe point of logging errors to the screen is that you can see what goes on.
By design IMO, but I'll leave it open for other opinions.
Comment #2
damien tournoud commentedWe could strip the full path in the error handler. I don't really see the point (this information is readily available everywhere in the system), but if that can make some self-proclaimed "security researchers" sleep better at night...
Bumping to D7 for consideration.
Comment #3
dave reidBleh. Here's the patch if anyone wants it although it was pretty easy to do.
Comment #4
heine commentedWhy not strip *ALL* useful info...
Really, why bother with this?
Comment #5
scor commentedI first thought we could just fix drupal_get_destination() and ensure $_REQUEST['destination'] is not an array but I suppose there might be other ways to force similar notices in D6 (in theory not in D7). Logging warnings to the screen is useful for developers, but should be turned off on production sites so I would agree with Heine that this is by design. @damz's idea is good, though again, the full path is great when debugging to find out what file in which Drupal code base is causing a warning etc.: having just the relative path won't tell you if you're looking at the wrong Drupal install for example. leaving this issue open to get more opinions.
Comment #6
scor commentedcross posted, setting by design
Comment #7
entendu commentedI won't change the status of the bug, although it should be noted that something may be both "by design" and wrong.
I think the point here is that Drupal reveals its path to all users (including anon) in errors by default. Should that be the case? Sane defaults are obviously something we should be reaching for, so the question is: is this sane?
Perhaps a saner set of selections for Drupal error reporting would be:
-Log to database only (recommended for production)
-Log to screen and database (recommended for development)
-Log to screen only for User 1, and always to database (default)
I'll whip up a patch if there's any interest.
Comment #8
heine commentedHaving errors display only the UID 1 is quite a pain when you encounter an error that leaves you logged out. It's a development setting you need to change before going to production.
Comment #9
entendu commentedThese points are well-taken, but irrelevant. A condition being "quite a pain" isn't enough (in this case would we not assume a properly configured php install and take a look at php's error log?). I realize it's a development setting, but that's not the issue. The question at hand is: is this a sane default?
If revealing the location of the webapp isn't a security risk, then I'd say it is sane. Is this what you're saying?
Comment #10
sunPlease forgive me for re-purposing, but after having read this thread and related posts, I think the starting scenario and presumption is wrong.
On production sites, you want and should usually disable the visible output of error messages.
Attached patch is what I applied via settings.php since Drupal 5 to all production sites. The consideration is:
If Drupal's error reporting ('error_level' system variable) is configured to not output errors, then PHP should not output errors either. Drupal mutes its own errors, but whenever PHP throws something, Drupal's settings do not take effect and the output appears.
Note that I have limited experience with the entirely revamped error handling in Drupal 7. Perhaps this patch is moot due to those changes, but I wanted to raise attention for this edge-case + adjustment I had to manually apply to Drupal 6 and below.
Lastly, if PHP's "display_errors" ini setting is properly handled in D7 already, then I think we still want to move the error level constants into bootstrap.inc.
Comment #11
damien tournoud commented@sun: this is hardly necessary: we set the error handler in _drupal_bootstrap_configuration() ie. before calling drupal_settings_initialize().
Moreover you don't have full access to variables at this stage of the bootstrap, because the database hasn't been initialized yet.
Reading the code, there is one thing we might want to change in _drupal_log_error(): don't call drupal_set_message() when Drupal is not fully bootstrapped. Currently, if an error is raised before DRUPAL_BOOTSTRAP_VARIABLES (for example: some broken code in settings.php),
variable_get('error_level', ERROR_REPORTING_DISPLAY_ALL)will likely return ERROR_REPORTING_DISPLAY_ALL and the error will be reported to the user via drupal_set_message().