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

Dropped unnecessary curl option #2204

Closed
wants to merge 1 commit into from

Conversation

marcobellaccini
Copy link

The curl proto option is not necessary (since target URL is https), hence it should be dropped.

The curl proto option is not necessary (since target URL is https), hence it should be dropped.
@kinnison
Copy link
Contributor

the --proto =https argument means that a MITM cannot force the protocol to downgrade to http so it isn't unnecessary as I see it. Why do you think it's unnecessary?

@marcobellaccini
Copy link
Author

marcobellaccini commented Jan 20, 2020

What kind of HTTPS->HTTP downgrade attacks are you afraid of?
Can you provide a practical example?
curl (and https clients in general) won't accept http-downgrade that easily.

Also, that proto option isn't protecting you from TLS/SSL downgrade attacks like POODLE (but that --tlsv1.2 option will do the job).

Moreover, if your threat model requires a very high degree of security, I think you should consider many other issues first (e.g.: neither rustup.rs nor rust-lang.org seem to be included in HSTS preload lists, they also don't support TLS 1.3).

@kinnison
Copy link
Contributor

In this instance I'm more interested in belt and braces. The particular combination of arguments was recommended to us as part of #1716 and I'm reluctant to remove arguments without the author of that change (@sanmai-NL) confirming how you believe cURL works wrt. redirects.

@marcobellaccini
Copy link
Author

Ok, I understand your point.
However please consider that, in order to perform a redirect, the TLS handshake will have to complete successfully (hence you should be protected).
I read in #1716 that @sanmai-NL was concerned about future unsafe redirects to HTTP.
That's right, but it's somewhat fancy (you're trying to partially protect yourselves from some unlikely, future, critical security issue).
And it's quite uncommon too (see what Node.js, Docker, NGINX are doing for packages and keys when using curl).

www/index.html Show resolved Hide resolved
@sanmai-NL
Copy link

sanmai-NL commented Jan 21, 2020

@marcobellaccini: why should we remove security measures when other security measures haven't been implemented yet? Do you have a risk assessment that supports your idea such an attack is highly unlikely?

Please help by implementing the missing security measures.

@marcobellaccini
Copy link
Author

marcobellaccini commented Jan 22, 2020

I have made my comments.
If you disagree, you can just keep that option.
Nothing personal when it comes to software.
Just for the record: I can't add the website to the HSTS preload list, nor I can enable TLS 1.3 - because those actions would require administrative access to the webservers.
I'm closing this PR.

@sanmai-NL
Copy link

I also don't have administrative access for rustup. You may raise the concerns though. TLS 1.3 is limited by Amazon I'm afraid.

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