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.

Files: 
CommentFileSizeAuthor
#57 346285-followup-d7.patch4.51 KBDamien Tournoud
Passed: 8973 passes, 0 fails, 0 exceptions
[ View ]
#54 346285-followup-D5.patch2.1 KBpwolanin
#53 346285-followup-d6.patch2.18 KBDamien Tournoud
#52 346285-empty-http-host-49-D6.patch1.21 KBpwolanin
#52 346285-empty-http-host-49-D5.patch1.17 KBpwolanin
#51 346285-followup-d7.patch2.52 KBDamien Tournoud
Failed: 8896 passes, 3 fails, 14 exceptions
[ View ]
#49 346285-followup-d7.patch697 bytesDamien Tournoud
Failed: 8898 passes, 1 fail, 0 exceptions
[ View ]
#41 346285-empty-http-host_2.patch3.4 KBgrendzy
Failed: Failed to apply patch.
[ View ]
#39 346285-empty-http-host_1.patch3.29 KBgrendzy
Failed: Failed to install HEAD.
[ View ]
#34 346285-empty-http-host.patch3.29 KBDamien Tournoud
Failed: Failed to install HEAD.
[ View ]
#26 346285-empty-http-host.patch3.29 KBDamien Tournoud
Failed: Failed to apply patch.
[ View ]
#19 empty-http-host-346285-D7-19.patch1.85 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#18 drupal-5-empty_http_host_support-2.patch543 bytesthekevinday
Failed: Failed to apply patch.
[ View ]
#18 drupal-6-empty_http_host_support-2.patch539 bytesthekevinday
Failed: Failed to apply patch.
[ View ]
drupal-5-empty_http_host_support-1.patch565 bytesthekevinday
Failed: Failed to apply patch.
[ View ]

Comments

grendzy’s picture

Status:Active» Needs review

Works great for me, thanks. This patch also applies cleanly to Drupal 6.8.

It also fixes #345887: Drush: Bootstrap failed.

thekevinday’s picture

After 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:

'SERVER_NAME'
The name of the server host under which the current script is executing. If the script is running on a virtual host, this will be the value defined for that virtual host.

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:

connect to http://www.example.com/
- HTTP_HOST = www.example.com
- SERVER_NAME = www.example.com

connect to http://www.example.com:81/
- HTTP_HOST = www.example.com:81
- SERVER_NAME = www.example.com

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.

Summit’s picture

SUbscribing,greetings, Martijn

regx’s picture

Works 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!

grendzy’s picture

Status:Needs review» Reviewed & tested by the community
catch’s picture

Version:5.14» 6.x-dev

Since this is valid in Drupal 6, moving to the most recent version first.

catch’s picture

Status:Reviewed & tested by the community» Needs work

Also, please re-roll the patch with curly braces and true capitalised - see http://drupal.org/coding-standards for more.

grendzy’s picture

As an alternative solution, the final + in the regex can be changed to a *:

-  return preg_match('/^\[?(?:[a-z0-9-:\]_]+\.?)+$/', $_SERVER['HTTP_HOST']);
+  return preg_match('/^\[?(?:[a-z0-9-:\]_]+\.?)*$/', $_SERVER['HTTP_HOST']);

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:

-  if (!drupal_valid_http_host()) {
+  if (!drupal_valid_http_host() && isset($_SERVER['HTTP_HOST'])) {
pwolanin’s picture

Sure that change is reasonable, or we could do:

return empty($_SERVER['HTTP_HOST']) || preg_match('/^\[?(?:[a-z0-9-:\]_]+\.?)+$/', $_SERVER['HTTP_HOST']);
Gábor Hojtsy’s picture

Agreed 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.

Damien Tournoud’s picture

Version:6.x-dev» 7.x-dev

An 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.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Added 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.

Gábor Hojtsy’s picture

http://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.

Damien Tournoud’s picture

Apache 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.

Gábor Hojtsy’s picture

Well, 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.

Damien Tournoud’s picture

@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.

thekevinday’s picture

StatusFileSize
new539 bytes
Failed: Failed to apply patch.
[ View ]
new543 bytes
Failed: Failed to apply patch.
[ View ]

In 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)

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new1.85 KB
Failed: Failed to apply patch.
[ View ]

I'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.

Status:Needs review» Needs work

The last submitted patch failed testing.

thekevinday’s picture

In 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?

kbahey’s picture

Just 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.

pwolanin’s picture

@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:

$_SERVER['HTTP_HOST']       = 'default';
grendzy’s picture

thekevinday: 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.

catch’s picture

Status:Needs work» Needs review

Going to send #19 for testing again.

Damien Tournoud’s picture

StatusFileSize
new3.29 KB
Failed: Failed to apply patch.
[ View ]

@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).

pwolanin’s picture

Status:Needs review» Needs work

@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' into HTTP_HOST == ''

abqaria’s picture

I 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

abqaria’s picture

i 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

abqaria’s picture

so 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

kbahey’s picture

Yes, it is wrong. Please review the INSTALL.txt that comes with Drupal on how to properly set it up.

tarvid’s picture

Version:7.x-dev» 6.8

Any chance of a fix to core so that drush works normally again?

Damien Tournoud’s picture

Status:Needs work» Needs review
StatusFileSize
new3.29 KB
Failed: Failed to install HEAD.
[ View ]

I had this in my sandbox for ages, apparently missed pushing it here.

catch’s picture

Version:6.8» 7.x-dev

This is still in HEAD, so needs to be applied there first.

pwolanin’s picture

Do 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.

grendzy’s picture

Status:Needs review» Needs work

I 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).

kbahey’s picture

@grendzy

I am not the one complaining. I have not seen the problem at all on any of the sites I am involved with.

grendzy’s picture

Status:Needs work» Needs review
StatusFileSize
new3.29 KB
Failed: Failed to install HEAD.
[ View ]

@kbahey, whoops, that was meant for tarvid. Sorry.

An update to Damien's patch:

Gábor Hojtsy’s picture

Latest 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.

+    // Some pre-HTTP/1.1 clients will not send a Host header. Ensure the key is
+    // defined for E_ALL compliance.

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.

grendzy’s picture

StatusFileSize
new3.4 KB
Failed: Failed to apply patch.
[ View ]

Thanks for your review. An update with the comment retained:

Damien Tournoud’s picture

Status:Needs review» Reviewed & tested by the community

Ok, we polished this issue well enough. Time to pull the trigger.

threexk’s picture

+1 for backporting this to D5.

catch’s picture

Issue tags:+needs backport to D6
Dries’s picture

Issue tags:-needs backport to D6

I'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.

grendzy’s picture

@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.

Dries’s picture

Version:7.x-dev» 6.x-dev
Status:Reviewed & tested by the community» Needs work
Issue tags:+needs backport to D6

Committed to CVS HEAD. I'm not sure why I did unset that tag.

Gábor Hojtsy’s picture

Status:Needs work» Patch (to be ported)

Upon 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.

Damien Tournoud’s picture

StatusFileSize
new697 bytes
Failed: 8898 passes, 1 fail, 0 exceptions
[ View ]

Here is a followup for D7, as #48.

Damien Tournoud’s picture

StatusFileSize
new2.52 KB
Failed: 8896 passes, 3 fails, 14 exceptions
[ View ]

A better version. drupal_valid_http_host() is now an clean helper function.

pwolanin’s picture

Here's a simple D6 and D5 fix.

Damien Tournoud’s picture

StatusFileSize
new2.18 KB

If we clean-up D7, why not cleaning-up D6 and D5 too?

pwolanin’s picture

StatusFileSize
new2.1 KB

ok, same patch for D5

pwolanin’s picture

Status:Patch (to be ported)» Needs review
pwolanin’s picture

Version:6.x-dev» 7.x-dev
Damien Tournoud’s picture

StatusFileSize
new4.51 KB
Passed: 8973 passes, 0 fails, 0 exceptions
[ View ]

Same patch as #51, with fixed tests.

drumm’s picture

Committed #54 to 5.x.

Gábor Hojtsy’s picture

Thanks, committed #53 to Drupal 6.

catch’s picture

Tested #53 on D6 with drush, works great.

grendzy’s picture

I noticed that isset($_SERVER['HTTP_HOST']) && ... != '' would be !empty()

not 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).

Gábor Hojtsy’s picture

I 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? :)

grendzy’s picture

Oh 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 to if (!drupal_valid_http_host($_SERVER['HTTP_HOST'])) {. So in D6 NULL is valid, but "" (empty string) is not?

catch’s picture

Priority:Critical» Normal

No longer critical.

Gábor Hojtsy’s picture

@grendzy: why would drupal_valid_http_host() allow NULL as valid, when it requires at least one valid char?

Damien Tournoud’s picture

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 to if (!drupal_valid_http_host($_SERVER['HTTP_HOST'])) {. So in D6 NULL is valid, but "" (empty string) is not?

The empty string is not a valid HTTP host, this is by design.

marqpdx’s picture

Hi,

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

marqpdx’s picture

A 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

catch’s picture

It 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.

marqpdx’s picture

As 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

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

This went into D6, so shoudl be RTBC for 7.

Tobias Weibel’s picture

Status:Reviewed & tested by the community» Needs review

Hi

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

Tobias Weibel’s picture

Status:Needs review» Reviewed & tested by the community

I probably set the status by accident to another state.

Sorry
Tobias

webchick’s picture

Status:Reviewed & tested by the community» Fixed

#57 committed to HEAD. Thanks!

Dave Reid’s picture

Issue tags:-needs backport to D6
jonathan1055’s picture

subscribing

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.