Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent header spoofing via underscore/dash conflation. #80

Closed
wants to merge 3 commits into from

Conversation

@mgrbyte
Copy link
Contributor

mgrbyte commented Jan 13, 2015

This PR prevents the WSGI header attack as documented here (by dropped headers containing underscores from the request)
See https://www.djangoproject.com/weblog/2015/jan/13/security/

@tseaver
Copy link
Member

tseaver commented Jan 14, 2015

The test failures don't look to be related to the changes here: more like timeouts (Travis under heavy load?) I have restarted the test run there.

@tseaver
Copy link
Member

tseaver commented Jan 14, 2015

I was wrong -- this change does break Python 3.x tests: I pushed a trivial change to master and the tests all pass on Travis: re-starting them for this PR shows the same failures for Py3k.

@mmerickel
Copy link
Member

mmerickel commented Jan 14, 2015

@tseaver FWIW you can just hit the refresh button on travis to re-run the tests. You do not need to commit to the repo.

@tseaver
Copy link
Member

tseaver commented Jan 14, 2015

Yup, I did that for the PR run. I've got some superstition related to previous oddball experience that made me want to provoke a build with a fresh commit.

@tseaver tseaver closed this Jan 14, 2015
Matt Russell
@mgrbyte
Copy link
Contributor Author

mgrbyte commented Jan 14, 2015

@tseaver I've fixed the breakage under Python3 (still works under 27) - is this PR desired (can re-issue)?
this change fixes it.

@tseaver tseaver reopened this Jan 14, 2015
@tseaver
Copy link
Member

tseaver commented Jan 14, 2015

Sorry for closing -- that was a fat-fingered late-night miss. Please push your fix.

@tseaver
Copy link
Member

tseaver commented Jan 14, 2015

Note that I'm not convinced that waitress is actually vulnerable here: we don't rely on X-Forwarded-For at all (in spite of what the docs say), although we do allow 'X-Forwarded-Proto' to change the scheme to one of http or https, but only if the source is configured as trusted-proxy (which should not be true for development or where waitress serves direct requests).

I don't think it would hurt to apply this sanitization.

@mgrbyte
Copy link
Contributor Author

mgrbyte commented Jan 14, 2015

@tseaver done

Matt Russell
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.