[D7] HTTP_HOST header cannot be trusted

Adding a last D7 patch from the parent issue, but let's convert it to the MR and add a missing tests.

PHPUnit tests failed for a "temporary glitch." Now they all pass.

Thanks for adding the tests @akalata! The tests looks good.

I am just not sure about the added check: !empty($host) (see here). I am not sure about this check - if the host will be empty, are we supposed to return TRUE? When could that happen? Or should we check for this situation sooner in drupal_valid_http_host(), when the condition for CLI was also added?

All tests are green, as mentioned by @avpaderno - https://git.drupalcode.org/issue/drupal-3444017/-/pipelines/298319 .

As the original backport was created by me, adding a tag for the review from another D7 maintainer.

I think this looks good, and would be an excellent thing to add to D7 (even this late in the day).

However it has to be an opt-in change.

Just sanity checking that the code only enforces the host pattern checking if the variable is set:

  // Check trusted HTTP Host headers to protect against header attacks.
  if (PHP_SAPI !== 'cli') {
    $host_patterns = variable_get('trusted_host_patterns', array());
    if (!empty($host_patterns)) {
      if (!drupal_check_trusted_hosts($_SERVER['HTTP_HOST'], $host_patterns)) {
        header($_SERVER['SERVER_PROTOCOL'] . ' 400 Bad Request');
...snip...

So if a site doesn't define any patterns, there's no change.

Site's can add patterns (e.g. in settings.php) and they will then start to be enforced.

If that's all true, +1 from me; I'll double check this and then am happy to commit.

One small problem - the test data uses short array syntax (D7 uses the old array() syntax).

If we can fix that, I'm happy to commit this.

Well spotted, thanks! I have updated the array syntax to standard. Tests are still green (https://git.drupalcode.org/project/drupal/-/pipelines/358081/), but yeah, we are no longer testing PHP 5.4 to reveal such issues.

Great, thanks!

Leaving the "Needs change record" tag in place.

Created a draft CR: https://www.drupal.org/node/3491619 →

Text is based on the D10 CR, with some tweaks.

Hi Guys,

how do I remove this function?

This messed up my four Drupal 7.102 websites and none of the fixes I found, will make the error on the status page go away.

"This new security improvement is an opt-in feature - if you do not want to use this, there is no action needed."

Not true, after upgrading to 7.103 all my websites had errors. Neither adding:

$conf['trusted_host_patterns'] = array(
  '^www\.example\.com$',
);

Nor setting a precise base domain in settings.php made the errors go away.

Kind regards
Paul

@paul_constantine so to be clear the problem you're having is the message on the admin status report?

This really should have been a warning rather than an error, and I missed that in review:

  // See if trusted hostnames have been configured, and warn the user if they   
  // are not set.                                                                   
  if ($phase == 'runtime') {                                                        
    $trusted_host_patterns = variable_get('trusted_host_patterns', array());    
    if (empty($trusted_host_patterns)) {                                            
      $requirements['trusted_host_patterns'] = array(                               
        'title' => $t('Trusted Host Settings'),                                     
        'value' => $t('Not enabled'),                                               
        'description' => $t('The trusted_host_patterns setting is not configured in settings.php. This can lead to security vulnerabilities. It is <strong>highly recommended</strong> that you configure this. See <a href="@url">Protecting against HTTP HOST Header attacks</a> for more information.', array('@url' => 'https://www.drupal.org/node/1992030')),
        'severity' => REQUIREMENT_ERROR,                                            
      );                                                                            
    }                                                                               
    else {                                                                          
      $requirements['trusted_host_patterns'] = array(                               
        'title' => $t('Trusted Host Settings'),                                     
        'value' => $t('Enabled'), 

...snip...

One workaround for this which comes to mind is to set a very permissive trusted host pattern such as:

$conf['trusted_host_patterns'] = array('.*');

So that should match anything.

It'd be better to set it up properly if possible.

I'm curious as to how the status report message would not go away if you did set a value for trusted_host_patterns in settings.php - and also how it would not break your site(s) quite badly if you set an example value which doesn't actually match the host / domain in use.

Could you please double check that?

In the meantime, we'll have to decide whether this warrants a follow up / hotfix release or something similar.

Apologies for the disruption; as mentioned I think a warning would have been more appropriate.

Hello mcdruid,

that did the trick. I added the "domain.tld" to that little line of code and it worked. The errors are gone.

$conf['trusted_host_patterns'] = array('.domain.tld');

Thank you very much for the fast reply.

Paul

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