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
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:
Comments
Comment #2
mcdruidPatch 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:
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.
Comment #4
mcdruidTest 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.
Comment #5
mcdruidI think the answer is that yes, we should check for no scheme being parsed to avoid errors.
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.
Comment #6
mcdruidhttps://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:
...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.
Comment #7
mcdruidArghh... I think I made a note elsewhere about removing the colon from the "see" here:
...but I keep forgetting. Can be done on commit.
Comment #8
mcdruidLooks like this needs a re-roll.
It'd be good to split out a separate test-only patch too.
Comment #9
mcdruidActually 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.
Comment #10
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI 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:
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
Comment #12
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThanks all!