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

Disable authentications on redirections #207

Merged
merged 2 commits into from
Apr 1, 2020

Conversation

yahavi
Copy link
Contributor

@yahavi yahavi commented Mar 31, 2020

This PR fixes a security issue.

Current behavior

  1. Send any request with BasicCredentialHandler, BearerCredentialHandler or PersonalAccessTokenCredentialHandler.
  2. The target host may return a redirection (3xx), with a link to a second host.
  3. The next request will use the credentials to authenticate with the second host, by setting the Authorization header.

Expected behavior
3. The next request will NOT set the Authorization header.

See the same issue for CURL:

  1. HTTP authentication leak in redirects - I used the same solution as CURL did.
  2. CVE-2018-1000007.

@msftclas
Copy link

msftclas commented Mar 31, 2020

CLA assistant check
All CLA requirements met.

@damccorm
Copy link

I don't think we should introduce this change without a way to override it since this is a breaking change, but I generally agree with the change.

Could you add another parameter to PrepareRequest to control this (and propogate it back where it gets called in httpclient)? It can probably just be an extra requestOption in the constructor

@yahavi
Copy link
Contributor Author

yahavi commented Mar 31, 2020

Thanks for the response.
I agree this can be configurable.
What do you think on Access-Control-Allow-Origin request header?
3 Options:
1. Access-Control-Allow-Origin is falsy - Don't allow authorization with other hosts. (This is the default)
2. Access-Control-Allow-Origin=* - Allow authorization with all hosts.
3. Access-Control-Allow-Origin=<host-name> - Allow authorization with the the first host AND with <host-name>.

Edit: See my next comment.

@yahavi
Copy link
Contributor Author

yahavi commented Apr 1, 2020

After more thoughts, I concluded that from user's perspective it would be much simpler to add an optional boolean allowCrossOriginAuthentication to the constructor.

Changes in the last commit:

  • Add allowCrossOriginAuthentication?: boolean to the constructor.
  • Change host -> origin to be more specific.

Copy link

@damccorm damccorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks for doing this!

@damccorm damccorm merged commit f9ff755 into microsoft:master Apr 1, 2020
@yahavi yahavi deleted the auth-redirect branch April 2, 2020 05:35
@JLLeitschuh
Copy link
Contributor

Was a CVE ever issued for this vulnerability?

@JLLeitschuh
Copy link
Contributor

I've reported this upstream to MSRC to get a CVE issued.
https://msrc.microsoft.com/report-detail/VULN-047585

@JLLeitschuh
Copy link
Contributor

I've requested a CVE number from MITRE as I received the following from Microsoft:

CVEs are generally only released from Microsoft for very specific issues. Specifically they are used for items that require patching through Windows Update. They are used to explain why a certain patch is needed and are tied into that download. If a fix is to an online service or online repo like GitHub locations, these would not receive a CVE. This item does not look like it was tied into the Windows Update release and would not be eligible for a CVE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants