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

Forbid Redirect Downgrade from HTTPS to HTTP #165

Closed
JLLeitschuh opened this issue Sep 25, 2019 · 3 comments · Fixed by #173
Closed

Forbid Redirect Downgrade from HTTPS to HTTP #165

JLLeitschuh opened this issue Sep 25, 2019 · 3 comments · Fixed by #173
Assignees

Comments

@JLLeitschuh
Copy link
Contributor

Feature Request

If an HTTPS client request gets downgraded by a redirect to HTTP by some host in the redirect chain, users should have the option to forbid this downgrade in the configuration of the HttpClient.

Impacted Code

let redirectsRemaining: number = this._maxRedirects;
while (HttpRedirectCodes.indexOf(response.message.statusCode) != -1
&& this._allowRedirects
&& redirectsRemaining > 0) {
const redirectUrl: any = response.message.headers["location"];
if (!redirectUrl) {
// if there's no location to redirect to, we won't
break;
}
// we need to finish reading the response before reassigning response
// which will leak the open socket.
await response.readBody();
// let's make the request with the new redirectUrl
info = this._prepareRequest(verb, redirectUrl, headers);
response = await this.requestRaw(info, data);

There is no check to see if the new redirect location is a downgrade from HTTPS to HTTP.

While not strictly a security vulnerability, this does have security implications.

Use Case

The use case for this is the GitHub Actions toolkit API. See: actions/toolkit#162

The GitHub actions toolkit is used to download other tools using the downloadTool API. We'd like to require that users provide SHA-256 checksums for their artifacts if they end up using HTTP instead of HTTPS to prevent MITM attacks against the GH Actions supply chain.

This verification should also be required if somewhere in the redirect chain in downloading the tool, the request is downgraded from HTTPS to HTTP. Currently, the HttpClient doesn't support this sort of check.

@bryanmacfarlane
Copy link
Contributor

This is a good fix

@JLLeitschuh
Copy link
Contributor Author

JLLeitschuh commented Oct 10, 2019

@bryanmacfarlane Do you have the cycles to implement this change and roll it downstream to the actions repository?

@JLLeitschuh
Copy link
Contributor Author

Would it be appropriate for this to be opt-in or opt-out?
I feel like for security reasons, this should be an opt-out thing. You can opt-out of this protection, but that has to be intentional.

JLLeitschuh added a commit to JLLeitschuh/typed-rest-client that referenced this issue Oct 10, 2019
damccorm pushed a commit that referenced this issue Oct 11, 2019
* Forbid redirect from HTTPS to HTTP by default

Closes #165

* Apply suggestions from code review
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 a pull request may close this issue.

2 participants