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

http2: verify :athority in push promise requests #3581

Closed
wants to merge 2 commits into from

Conversation

@bagder
Copy link
Member

@bagder bagder commented Feb 18, 2019

RFC 7540 says we should verify that the push is for an "authoritative"
server. We make sure of this by only allowing push with an :athority
header that matches the host that was asked for in the URL.

Fixes #3577
Reported-by: Nicolas Grekas
Bug: https://curl.haxx.se/mail/lib-2019-02/0057.html

I've asked @tatsuhiro-t separately for advice on what the best suitable return code is from the callback in case the authority isn't deemed fine, but in the mean time here's a take.

RFC 7540 says we should verify that the push is for an "authoritative"
server. We make sure of this by only allowing push with an :athority
header that matches the host that was asked for in the URL.

Fixes #3577
Reported-by: Nicolas Grekas
Bug: https://curl.haxx.se/mail/lib-2019-02/0057.html
@bagder bagder added the HTTP/2 label Feb 18, 2019
@bagder bagder closed this in aa5a28b Feb 20, 2019
@bagder bagder deleted the bagder/http2-authority-check branch Feb 20, 2019
@nicolas-grekas
Copy link

@nicolas-grekas nicolas-grekas commented Feb 20, 2019

Thanks!
Should we keep track somewhere that this could be improved by using the certificate's alt-names to allow for more domains that the one of the origin URL?

@bagder
Copy link
Member Author

@bagder bagder commented Feb 20, 2019

I'll add a note in the TODO about it, then at least it's not forgotten.

@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants