Problem/Motivation

Drupal 7.x has a vulnerability similar to the two recently fixed in Guzzle:

https://github.com/guzzle/guzzle/security/advisories/GHSA-w248-ffj2-4v5q
https://github.com/guzzle/guzzle/security/advisories/GHSA-f2wf-25xc-69c9

drupal_http_request() automatically follows 3xx redirects. RFC 9110 states:

When automatically following a redirected request, the user agent SHOULD resend the original request message with the following modifications:

3. Consider removing header fields that were not automatically generated by the implementation (i.e., those present in the request because they were added by the calling context) where there are security implications; this includes but is not limited to Authorization and Cookie.

See https://www.rfc-editor.org/rfc/rfc9110.html#name-redirection-3xx

This issue has been discussed with the security team, and it has been decided that this can be handled as a security hardening issue in the public issue queue.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3293649

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

mcdruid’s picture

Status: Active » Needs review
Issue tags: +Needs change record
FileSize
3.63 KB

Patch from the private issue.

One thing this does not include is any ability to opt-out of this change.

I think it'd make sense to have two variables that allow opting out of stripping sensitive headers on:

  • https downgrade
  • change of http host

I'd suggest that these are opt-out so the new functionality is on by default (i.e. headers are stripped) for all sites.

This will also need a Change Record.

Setting to NR initially to get a test run, but this is really NW as we want to add the opt-out variables.

Status: Needs review » Needs work

The last submitted patch, 2: 3293649-2.patch, failed testing. View results

mcdruid’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
5.02 KB

Test failure was because of deliberately malformed values being returned in the location header in other tests, so a good catch.

drupal_http_request shouldn't follow those malformed redirects anyway (the tests assert that an error is returned instead), so I believe we're safe to skip the check for stripping headers when no value for host is parsed from the location.

I don't think it'd be possible for a https downgrade to happen without a host being specified in the redirect location; is that a correct assumption?

Should we add a check for no scheme being returned by parse_url($location)? Doesn't look like any of core's tests cause that, but could it happen conceivably?

This patch also adds the opt-out variables, which have descriptive but rather long names. They're well within the 128 char limit though.

mcdruid’s picture

Should we add a check for no scheme being returned by parse_url($location)?

I think the answer is that yes, we should check for no scheme being parsed to avoid errors.

php > var_dump(parse_url('//example.com/foo'));
array(2) {
  ["host"]=>
  string(11) "example.com"
  ["path"]=>
  string(4) "/foo"
}

I don't think we need to worry about https downgrade in the above case but there is no scheme.

Actually, I'm not sure about how drupal_http_request() would treat a redirect to a location like //example.com/foo - I'd assume browsers would preserve the existing scheme, but not sure about drupal_http_request().

Will dig into that a little more, but a check for no scheme being parsed from the location seems sensible.

mcdruid’s picture

FileSize
2.41 KB
7.43 KB

https://stackoverflow.com/questions/12436669/using-protocol-relative-uri...

...seems like perhaps there is some browser support for protocol-relative redirects (e.g. Location: //example.com/foo) and they may be permitted in (draft) specs - which D7 may well predate.

However, adding a test for one reveals that it's not supported by drupal_http_request() which responds with an error when it fails to parse the scheme from the URL:

$ drush ev 'var_dump(drupal_http_request("http://drupal7x.fp/system-test/redirect-protocol-relative"));'
object(stdClass)#277 (4) {
  ["error"]=>
  string(14) "missing schema"
  ["code"]=>
  int(-1002)
  ["redirect_code"]=>
  string(3) "301"
  ["redirect_url"]=>
  string(18) "//example.com/path"
}

...erm, shouldn't that be scheme not schema?

Anyway, that means we don't have to worry about a missing scheme in the redirect location that the new helper function is parsing as that will have been caught earlier.

I'll leave the new test in.

If we wanted to add support for protocol-relative redirects, that'd be a follow up and is not in scope here. Same with fixing that error message if it should say scheme rather than schema.

mcdruid’s picture

Arghh... I think I made a note elsewhere about removing the colon from the "see" here:

        // Check if we need to remove any potentially sensitive headers before
        // following the redirect.
        // @see: https://www.rfc-editor.org/rfc/rfc9110.html#name-redirection-3xx

...but I keep forgetting. Can be done on commit.

mcdruid’s picture

Issue tags: +Needs reroll

Looks like this needs a re-roll.

It'd be good to split out a separate test-only patch too.

mcdruid’s picture

Actually I forgot the reason I didn't do a test-only patch is that most of the tests here are unit tests of a function that doesn't exist without the rest of the patch.

So here's just the re-roll.

I remembered to remove the colon from the comment per #7 this time.

@poker10 if you're happy marking this RTBC, I think you can commit it too.

poker10’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record +RTBM

I think the patch looks good, thanks! Setting as RTBC and ready to commit.

Just one thing I have noticed - but maybe it is an additional layer of security hardening - why are the default values in the commented lines set as TRUE? This means that if a user removes the comment without changing the TRUE to FALSE, nothing will happen:

# $conf['drupal_http_request_strip_sensitive_headers_on_host_change'] = TRUE;

But I have briefly checked settings.php and it seems like there is no pattern for these default values here. Some options are "uncomment to disable" (like $conf['log_user_flood_control'] = FALSE; or $conf['allow_authorize_operations'] = FALSE;) and some are the opposite (like this, or $conf['variable_initialize_wait_for_lock'] = FALSE; or $conf['set_has_js_cookie'] = FALSE;).

I have also drafted a change record for this, but feel free to edit it as needed: https://www.drupal.org/node/3307804

  • poker10 committed e15ccfb on 7.x
    Issue #3293649 by mcdruid: drupal_http_request() fails to strip Cookie...
poker10’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -RTBM

Thanks all!

Status: Fixed » Closed (fixed)

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

fjgarlin made their first commit to this issue’s fork.