After upgrading to drupal 5.14 (from 5.12) I found that under certain circumstances HTTP_HOST is not defined or sent to the drupal website.
This may affect drupal 6.8 as well, but I have not yet gotten around to confirming this.
With the latest version, the drupal_valid_http_host() function was created.
This fails when HTTP_HOST is undefined or blank.
I suspect that this also causes the following issue: http://drupal.org/node/346175
The following threads are relevant:
http://drupal.org/node/251320
http://drupal.org/node/335560
http://drupal.org/node/42876
Most notable is the line: This is in violation of section 14.23 of the HTTP 1.1 protocol
The problem here is that this effectively causes a denial of service to those not following the standard (which makes me feel warm and fuzzy on the inside) but on a production system this kind of action cannot be tolerated.
I have supplied a patch that checks for HTTP_HOST if available, but if undefined acceptance is still granted.
Comment | File | Size | Author |
---|---|---|---|
#57 | 346285-followup-d7.patch | 4.51 KB | Damien Tournoud |
#54 | 346285-followup-D5.patch | 2.1 KB | pwolanin |
#53 | 346285-followup-d6.patch | 2.18 KB | Damien Tournoud |
#52 | 346285-empty-http-host-49-D6.patch | 1.21 KB | pwolanin |
#52 | 346285-empty-http-host-49-D5.patch | 1.17 KB | pwolanin |
Comments
Comment #1
grendzy CreditAttribution: grendzy commentedWorks great for me, thanks. This patch also applies cleanly to Drupal 6.8.
It also fixes #345887: Drush: Bootstrap failed.
Comment #2
thekevinday CreditAttribution: thekevinday commentedAfter some in depth testing and research I came up with the following:
There exist the case where the php function fsockopen is used (defined here: http://php.net/manual/en/function.fsockopen.php).
Using fsockopen requires that the header be manually created.
In this way, it is possible for the HTTP_HEADER to not be sent.
Looking at the examples, I wonder if the HTTP 1.0 spec does not transmit the HTTP_HOST header data.
Looking here: http://php.net/manual/en/reserved.variables.server.php
I found the following:
In general, SERVER_NAME ends up identical to HTTP_HOST.
The main differences in their values is when a non-standard port is used, HTTP_HOST would contain the colon and the port number:
example:
so, HTTP_HOST and SERVER_NAME can contain different values.
HTTP_HOST is defined by the client (which in general cannot be trusted because its from the client)
SERVER_NAME is defined by the server
Perhaps the safer solution is to use
$_SERVER['SERVER_NAME']
over$_SERVER['HTTP_HOST']
.Another option is to use
$_SERVER['SERVER_NAME']
whenever$_SERVER['HTTP_HOST']
is undefined.Comment #3
Summit CreditAttribution: Summit commentedSUbscribing,greetings, Martijn
Comment #4
regx CreditAttribution: regx commentedWorks for me as well.
After upgrading to 5.14 anything running from pp-cli that bootstraped Drupal would die instantly without any error messages.
After applying the patch all is well in Drupal Bootstrap land again!
Thanks for the patch!
Comment #5
grendzy CreditAttribution: grendzy commentedComment #6
catchSince this is valid in Drupal 6, moving to the most recent version first.
Comment #7
catchAlso, please re-roll the patch with curly braces and true capitalised - see http://drupal.org/coding-standards for more.
Comment #8
grendzy CreditAttribution: grendzy commentedAs an alternative solution, the final + in the regex can be changed to a *:
Also, as was pointed out, an empty HTTP_HOST isn't technically valid, so maybe drupal_valid_http_host() shouldn't worry about this, instead:
Comment #9
pwolanin CreditAttribution: pwolanin commentedSure that change is reasonable, or we could do:
Comment #10
Gábor HojtsyAgreed with #8, an empty HTTP_HOST is not valid, so we should not say it is. As far as I know this code is in the 7.x codebase already, so we need to have a fix there as well.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedAn empty HTTP_HOST is valid HTTP/1.0. When Drupal receives a request with an empty HTTP_HOST, it means that the webserver already decided to serve it using its default virtual host. We should serve those requests using the default website.
pwolanin#9 is what makes the most sense to me, and is the only suggested solution that is E_ALL compliant.
Bumping to 7.x to get a fix there first.
Comment #12
Gábor HojtsyAlso reported at #346175: Cron does not run after upgrading 5.12->5.14 (same for 6.6->6.8) which was marked duplicate.
Comment #13
Gábor HojtsyAdded a requirement for a test on this condition in 7.x at #324875: SA-2008-067 conf_path() header injection vulnerability and reopened that one.
Comment #14
Gábor Hojtsyhttp://www.research.att.com/~bala/papers/h0vh1.html says "Clearly, since HTTP/1.0 clients will not send Host headers, HTTP/1.1 servers cannot simply reject all messages without them. However, the HTTP/1.1 specification requires that an HTTP/1.1 server must reject any HTTP/1.1 message that does not contain a Host header."
Not sure we should follow the letter of the spec there though, given these reports on some clients missing the Host header anyway.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedApache already does that for us. It replies
HTTP/1.1 400 Bad Request
to any requests pretending to be in HTTP/1.1 but missing the "Host:" header.
Comment #16
Gábor HojtsyWell, reports around this issue remind us that Drupal is obviously not only run through Apache. It is run on other servers and is run from the command line as part of scripting environments. In that case, Drupal either needs to be forgiven on its own or need to enforce rules in itself.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commented@Gabor: I'm pretty sure that those clients that doesn't transmit the Host headers are in fact HTTP/1.0 clients. Plus, because the webserver already guarantee that HTTP/1.1 requests have an Host header (it's verified on Apache, but other webservers will probably act the same), we just have to accept an empty Host header as valid and serve the default website in that case.
Comment #18
thekevinday CreditAttribution: thekevinday commentedIn regards to #7, #9, and #17, and taking note of #2 and #6
The original patch I applied is more or less a quick fix.
I believe the real problem is that drupal itself should fall back to the default (as mentioned in #17), which I believe might be defined by
$_SERVER['SERVER_NAME']
(and possibly adding the port number???) (as mentioned in #2).Because this was bumped to 7.x-dev (as done in #6) I suggest that the the processing of HTTP_HOST be wrapped by some drupal core function so that when the client does not send the host header, then drupal can force a default for drupal 7.
As far as drupal 5 & 6, I remade the patch using the suggestion from #9 because it is more conservative. (which resolves #7, hopefully)
Comment #19
pwolanin CreditAttribution: pwolanin commentedI'm really not sure we need to fill in a default - one can already force the server name by setting it in settings.php as the global $base_url:
http://api.drupal.org/api/function/conf_init/7
Here's a 7.x patch with added test condition.
Comment #21
thekevinday CreditAttribution: thekevinday commentedIn regards to the drupal 5 & 6 patches, I suspect they failed to apply because I must be using a different style of creating the patch.
Where is the standard patch method defined and what is it?
Should the drupal 5 and 6 be against the drupal 5 & 6 revision or the released sources?
Comment #22
kbahey CreditAttribution: kbahey commentedJust a data point that may be relevant.
I have a few sites upgraded to 6.8 on a multi site install, and cron runs fine.
I use:
wget -O - -q -t 1 http://..../cron.php
The cron job runs from the same host that runs the Drupal sites, if that matter.
This is using Apache2, mod_php (PHP 5.2.4). Not CGI nor FCGI, nor do they use Lighttpd or ngnix.
On two separate servers with the same versions of Apache and PHP, but Drupal 5.14, everything runs fine as well. Cron completes its runs.
The site were upgraded by creating a patch from the 5.12 vs. 5.13 and applying it. Then from 5.13 to 5.14, (and a similar method for 6.x) but that should not matter.
Comment #23
pwolanin CreditAttribution: pwolanin commented@Kbahey - I think the main case for cron runs failing is when using something like drush that does PHP via CLI.
Our drupal.sh script has this:
Comment #24
grendzy CreditAttribution: grendzy commentedthekevinday: your patches were formatted ok. Now that the issue is in the Drupal 7.x queue, the testing is performed against that version. That might be why there were issues with the automated test.
Most often development of patches is focused on one version at a time. After a patch gets committed to the latest version, it can then be backported to D5 and 6.
Regarding cron -- that's exactly it. Some people run
/usr/bin/php $DOCUMENTROOT/cron.php
in their crontab.I'm surprised by the number of exceptions on #19 though. It's possible a bad comment went in, which makes all future tests fail.
Comment #25
catchGoing to send #19 for testing again.
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commented@pwolanin: #19 was not E_ALL safe.
Here is a E_ALL safe version. Also moved the check for drupal_valid_http_host() in drupal_initialize_variables() where it makes a lot more sense (this change will probably not be backported to D6).
Comment #27
pwolanin CreditAttribution: pwolanin commented@Damien Tournoud - there is a typo in one of the code comments. Also, we should really start waith a 7.x patch that can be used as a direct 6.x backport I think, since this seems fairly urgent to fix in a consistent way.
Also, there is a bit of a logic error, perhaps, in that your patch will change the
HTTP_HOST == '0'
intoHTTP_HOST == ''
Comment #28
abqaria CreditAttribution: abqaria commentedI also have the problem
of running cron job manually goes okay
however when using the
crontab it does not fire
i just tested the above patch for drupal 5.14
Comment #29
abqaria CreditAttribution: abqaria commentedi do get this in my cron email
PHP Warning: include_once(): Unable to access ./includes/bootstrap.inc in /var/www/vhosts/i-bloggers.com/httpdocs/cron.php on line 9
Warning: include_once(): Unable to access ./includes/bootstrap.inc in /var/www/vhosts/mysite.com/httpdocs/cron.php on line 9
PHP Warning: include_once(./includes/bootstrap.inc): failed to open stream: No such file or directory in /var/www/vhosts/mysite.com/httpdocs/cron.php on line 9
Warning: include_once(): Failed opening './includes/bootstrap.inc' for inclusion (include_path='.:') in /var/www/vhosts/mysite.com/httpdocs/cron.php on line 9
PHP Fatal error: Call to undefined function drupal_bootstrap() in /var/www/vhosts/mysite.com/httpdocs/cron.php on line 10
Fatal error: Call to undefined function drupal_bootstrap() in /var/www/vhosts/mysite.com/httpdocs/cron.php on line 10
Comment #31
abqaria CreditAttribution: abqaria commentedso is it wring to use this as command
/usr/bin/php /var/www/vhosts/example.com/httpdocs/cron.php
as i do get this by email as a response to the cron
Warning: include_once(): Failed opening './includes/bootstrap.inc' for inclusion (include_path='.:') in /var/www/vhosts/example.com/httpdocs/cron.php on line 9
PHP Fatal error: Call to undefined function drupal_bootstrap() in /var/www/vhosts/example.com/httpdocs/cron.php on line 10
Fatal error: Call to undefined function drupal_bootstrap() in /var/www/vhosts/example.com/httpdocs/cron.php on line 10
Comment #32
kbahey CreditAttribution: kbahey commentedYes, it is wrong. Please review the INSTALL.txt that comes with Drupal on how to properly set it up.
Comment #33
tarvid CreditAttribution: tarvid commentedAny chance of a fix to core so that drush works normally again?
Comment #34
Damien Tournoud CreditAttribution: Damien Tournoud commentedI had this in my sandbox for ages, apparently missed pushing it here.
Comment #35
catchThis is still in HEAD, so needs to be applied there first.
Comment #36
pwolanin CreditAttribution: pwolanin commentedDo we really need to check
'HTTP/1.0'
if HTTP_HOST is empty? Isn't that the job of the web server? We only care that it's safe.Comment #37
grendzy CreditAttribution: grendzy commentedI agree with #36 that checking SERVER_PROTOCOL isn't necessary, though it doesn't hurt anything, since earlier bootstrap.inc will set an empty SERVER_PROTOCOL to HTTP/1.0 anyway.
BTW I'd also like to add a reminder that for a number of us, the issue isn't just old HTTP clients -- it's also making it easy to bootstrap Drupal from a script (like `php cron.php`, Drush, or a custom script like this http://www.lullabot.com/articles/quick_and_dirty_cck_imports ). I think this a really valuable use-case, and it's good DX that you don't have to do anything terribly heroic (like faking a web server CGI environment) in order to bootstrap Drupal.
Also, this check isn't consistent with the test, and causes the test to fail.
empty HTTP_HOST is valid Other bootstrap.test 85 BootstrapIPAddressTestCase->testIPAddressHost()
@kbahey, yes, the fix is likely (IMO) to get backported for D6. In the meantime you can use the first patch (in the original post, not the comments).
Comment #38
kbahey CreditAttribution: kbahey commented@grendzy
I am not the one complaining. I have not seen the problem at all on any of the sites I am involved with.
Comment #39
grendzy CreditAttribution: grendzy commented@kbahey, whoops, that was meant for tarvid. Sorry.
An update to Damien's patch:
Comment #40
Gábor HojtsyLatest patch looks better but this comment from the previous patch would be great to carry over to the place where HTTP_HOST is set to empty if not set.
Also, it would be great to accelerate the work on this patch, so that we can ensure it gets into the next Drupal 6 release.
Comment #41
grendzy CreditAttribution: grendzy commentedThanks for your review. An update with the comment retained:
Comment #42
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, we polished this issue well enough. Time to pull the trigger.
Comment #43
threexk CreditAttribution: threexk commented+1 for backporting this to D5.
Comment #44
catchComment #45
Dries CreditAttribution: Dries commentedI'm happy to pull the trigger, but ... it sounds like this patch allows us to clean-up the CLI scripts in
./scripts
a bit? We can leave that for follow-up patches but I figured I'd bring it up.Comment #46
grendzy CreditAttribution: grendzy commented@Dries, perhaps this would allow
$_SERVER['HTTP_HOST'] = 'default';
to be removed from drupal.sh. This doesn't gain much, but it is one less extra thing a dev needs to worry about to bootstrap Drupal in those little one-off scripts.Comment #47
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. I'm not sure why I did unset that tag.
Comment #48
Gábor HojtsyUpon further review with Damien, we found out that isset($_SERVER['HTTP_HOST']) is not needed in checking the validity, since the patch already ensures that it is set. (I noticed that isset($_SERVER['HTTP_HOST']) && ... != '' would be !empty(), but looks like a better simplification is to just check for != ''). Otherwise should be good for committing to D6.
So expecting a simple patch against D7 to fix this extra cruft and a backport to D6. I'll commit the D6 patch even if the simple cleanup patch is not hitting D7 before, so let's keep on D6.
Comment #49
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is a followup for D7, as #48.
Comment #51
Damien Tournoud CreditAttribution: Damien Tournoud commentedA better version. drupal_valid_http_host() is now an clean helper function.
Comment #52
pwolanin CreditAttribution: pwolanin commentedHere's a simple D6 and D5 fix.
Comment #53
Damien Tournoud CreditAttribution: Damien Tournoud commentedIf we clean-up D7, why not cleaning-up D6 and D5 too?
Comment #54
pwolanin CreditAttribution: pwolanin commentedok, same patch for D5
Comment #55
pwolanin CreditAttribution: pwolanin commentedComment #56
pwolanin CreditAttribution: pwolanin commentedComment #57
Damien Tournoud CreditAttribution: Damien Tournoud commentedSame patch as #51, with fixed tests.
Comment #58
drummCommitted #54 to 5.x.
Comment #59
Gábor HojtsyThanks, committed #53 to Drupal 6.
Comment #60
catchTested #53 on D6 with drush, works great.
Comment #61
grendzy CreditAttribution: grendzy commentednot quite, "0" (0 as a string) is considered empty, even though it could be a valid (though admittedly strange) value for HTTP_HOST (see comment #27).
Comment #62
Gábor HojtsyI was thinking !empty() since the proceeding preg already checked for the validity of the rest. It validated 1, 2, 3, etc. as valid host names, so why would 0 be an exception? :)
Comment #63
grendzy CreditAttribution: grendzy commentedOh yes, I see. In the current D7 version, if you switch to !empty(), "0" won't hit the regex, but the result will still be TRUE and it won't alter HTTP_HOST.
In D6 (current CVS as of a moment ago), the logic looks a little different... An empty string will pass
if (isset($_SERVER['HTTP_HOST'])) {
, then go toif (!drupal_valid_http_host($_SERVER['HTTP_HOST'])) {
. So in D6 NULL is valid, but "" (empty string) is not?Comment #64
catchNo longer critical.
Comment #66
Gábor Hojtsy@grendzy: why would drupal_valid_http_host() allow NULL as valid, when it requires at least one valid char?
Comment #67
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe empty string is not a valid HTTP host, this is by design.
Comment #68
marqpdx CreditAttribution: marqpdx commentedHi,
my cron is still not working in 6.9. is this supposed to be fixed in that version or do we still need the patch?
tried via the admin panel and via cli.
thanks,
m
Comment #69
marqpdx CreditAttribution: marqpdx commentedA follow-up to my post immediately above:
per this post: http://drupal.org/node/146551
i was able to get cron to run again.
it ran only after turning off the search module, and when i re-enabled that, it failed to run again.
thanks,
m
Comment #70
catchIt seems very unlikely that your errors are related to this bug. You should probably reduce the number of search items to index per cron run, and if you don't have tens of thousands of nodes on your site, hit the 're-index' button to start a full index again.
Comment #71
marqpdx CreditAttribution: marqpdx commentedAs i was able to get cron to run, albeit only with the search module disabled, it probably is not relevant to this thread.
i have opened another issue http://drupal.org/node/361171 specifically for this.
thanks!
m
Comment #72
pwolanin CreditAttribution: pwolanin commentedThis went into D6, so shoudl be RTBC for 7.
Comment #73
Tobias Weibel CreditAttribution: Tobias Weibel commentedHi
I still have the issue that cron doesn't work. With 6.6 I had no problems. At the moment (6.9) I've got poormanscron installed to have regular work done. Also manually calling cron.php works.
Does anybody know a good hint how I can debug? The problem is that I can't call wget or similar tools since my provider only allows php scripts for cron tasks. I guess that the HTTP_HOST is then not set properly. So I like to add some logging mechanism in the cron.php file where I can write some environment variables to a file.
Thanks
Tobias
Comment #74
Tobias Weibel CreditAttribution: Tobias Weibel commentedI probably set the status by accident to another state.
Sorry
Tobias
Comment #75
webchick#57 committed to HEAD. Thanks!
Comment #76
Dave ReidComment #77
jonathan1055 CreditAttribution: jonathan1055 commentedsubscribing