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

Feature request: Do not allow "downgrading" protocol on redirect #226

Closed
vszakats opened this Issue Apr 19, 2015 · 20 comments

Comments

Projects
None yet
5 participants
@vszakats
Member

vszakats commented Apr 19, 2015

Currently these useful curl/libcurl options exist to control which protocol a redirection may be allowed to follow:
http://curl.haxx.se/docs/manpage.html#--proto-redir
http://curl.haxx.se/libcurl/c/CURLOPT_REDIR_PROTOCOLS.html

But, in case the URL is not known in advance (f.e. it is coming from user input), there is no simple way to disable unwanted downgrades of the protocol in a generic way, f.e. (and typically) from HTTPS to HTTP.

For HTTPS URLs, the allowed redirects should be HTTPS (only), while for HTTP URLs, both HTTP and HTTPS would be acceptable.

One way to implement this, would be to allow specifying a special protocol safer (--proto-redir =safer and CURLPROTO_SAFER), which would only allow protocols regarded as equally safe or safer than the one used in the original URL. [I'm not insisting on this particular terminology]

Regarding all supported protocols, this may be a possible categorization of all of them based on a generally accepted amount of safety they provide, ordered by decreasing safety category:

"Safest" (SSL/TLS or SSH):
CURLPROTO_HTTPS
CURLPROTO_SCP
CURLPROTO_SFTP
CURLPROTO_SMTPS
CURLPROTO_IMAPS
CURLPROTO_POP3S
CURLPROTO_RTMPS
CURLPROTO_RTMPTS

"Safe" (plaintext implicitly or explicitly upgraded to SSL/TLS):
CURLPROTO_LDAPS
CURLPROTO_FTPS

"Unsafe" (plaintext):
CURLPROTO_HTTP
CURLPROTO_FTP
CURLPROTO_FILE
CURLPROTO_DICT
CURLPROTO_GOPHER
CURLPROTO_IMAP
CURLPROTO_LDAP
CURLPROTO_POP3
CURLPROTO_RTMP
CURLPROTO_RTMPE
CURLPROTO_RTMPT
CURLPROTO_RTMPTE
CURLPROTO_RTSP
CURLPROTO_SMB
CURLPROTO_SMTP
CURLPROTO_TELNET
CURLPROTO_TFTP

@jay

This comment has been minimized.

Member

jay commented Apr 21, 2015

The URL is at some point known in advance though. Your application passes the URL to libcurl so it would know the URL. My opinion is that seems like a niche to implement in your application. Though I agree maybe some special generic type defines could be helpful.

I'm not interested in taking this issue up really but I just wrote CURLPROTO_STAYSAME to only allow the same protocol on a redirect. In other words you use http only http redirects are allowed, or you use https only https redirects are allowed. I can't imagine a wide use (I'm not filing a PR) it's more poc to help get the ball rolling.

Something like CURLPROTO_ENCRYPTED_ONLY ? maybe would be easier and simpler as a combination of the flags of encrypted protocols. What seems difficult is handling connections that are "upgraded" to encryption. You'd have to allow the unencrypted protocol and then make sure they upgrade.

@jay jay added the feature-request label Apr 21, 2015

@vszakats

This comment has been minimized.

Member

vszakats commented Apr 21, 2015

Hello Jay, many thanks for jumping in. I think your CURLPROTO_STAYSAME patch would solve the problem the best way. It avoids any ambiguity in judging protocols by "safety", while resolving the root concern. ("upgrading" a non-encrypted protocol has no point and can be considered dangerous even.)

I like your second proposal CURLPROTO_ENCRYPTED_ONLY as well, but it keeps part of the ambiguity (not all encryptions are created equal), so I'd personally prefer your first solution.

About it being niche request, I don't agree; if securing a transaction is important, protocol downgrade is a real concern and dealing with this currently needs extra logic on the caller side (parsing the protocol in the source URL and forming a protocol filter based on that). Such extra logic may not always be trivial to implement, especially in scripts. wget features an --https-only option to solve this problem in a simple way. It is analog to your 'staysame' proposal.

For these reasons, I hope to see your patch promoted to a PR and seeing it in a future curl version. In the meantime any opinion and discussion is welcome.

@bagder

This comment has been minimized.

Member

bagder commented Apr 21, 2015

CURLPROTO_STAYSAME sounds good to me too!

@jay

This comment has been minimized.

Member

jay commented Apr 22, 2015

The scope seems pretty narrow to me. What can cause a follow besides an http or https response? Nothing, it appears, which I didn't realize until I was most of the way through implementing CURLPROTO_STAYSAME. In that case it only keeps http as http and https as https. If the user requests an http resource that redirects to https why wouldn't they want to allow that?

Something that stops the downgrade might be better and by making it just another flag that can be used in combination we're back to Viktor's suggestion. If the user doesn't want ambiguity they'd have to know the protocols they're expecting though. Consider:

curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS,
                 CURLPROTO_HTTP | CURLPROTO_HTTPS | CURLPROTO_NO_DOWNGRADE);

And behavior like:
Since version X.X.X there is flag CURLPROTO_NO_DOWNGRADE that can be used to stop a redirect from a secure resource to an unsecure resource. For example, if you request http://foo, which redirects to https://bar, which redirects to http:// baz, the redirect to http://baz would not be allowed.

master...jay:CURLPROTO_NO_DOWNGRADE

@vszakats

This comment has been minimized.

Member

vszakats commented Apr 22, 2015

If the user requests an http resource that redirects to https why wouldn't they want to allow that?

My original proposal was about to allow that, even though it's not something secure to do because the redirection target can be spoofed in the plaintext phase. In such case it's best to switch the origin URL to a secure alternative to keep the complete redirection chain use secure protocols.

'NO_DOWNGRADE' also reintroduces the ambiguity regarding what counts as a "downgrade" (f.e. is it an HTTPS to FTPS redirection considered a downgrade?), this is well mitigated by the optional selection of allowed protocols, it's good idea. Though internally, curl will still have to maintain a protocol list sorted by "upgrade" order.

My preference is still towards 'staysame', but if you guys find it better to allow "upgrades" as well, it's also an alternative - it gives more control, but some of them will be unsafe, it's up to the user.

Another alternative could be called 'STAYSAFE' (or 'STAYENCRYPTED'), and it'd mean that an unencrypted protocol can be changed to anything, while an encrypted protocol can only continue as encrypted - with an optional selection of allowed encrypted protocols. This seems simple, with less ambiguity and it hints no false sense of security about upgrading unencrypted protocols.

@bagder

This comment has been minimized.

Member

bagder commented Apr 22, 2015

It is also a bit tricky to easily figure out what an encrypted protocol is and thus what the level of the redirected-to protocol is. Like FTP, POP3 etc have "explicit" TLS which means they will connect as normal FTP:// etc and only upgrade to TLS if asked to do so with libcurl options.

@vszakats

This comment has been minimized.

Member

vszakats commented Apr 22, 2015

Indeed, the ambiguity is there as well.

@bagder

This comment has been minimized.

Member

bagder commented May 30, 2015

As this is still "just" a feature request and nobody seems to grab it now, I'll close it. This is now mentioned in the TODO: http://curl.haxx.se/docs/todo.html#Refuse_downgrade_redirects

@bagder bagder closed this May 30, 2015

@vszakats

This comment has been minimized.

Member

vszakats commented May 31, 2015

@jay created patches for both the "staysame" and later for the "no-downgrade" implementation. What would it require to get one of them merged? (I'd still prefer "staysame" because it's safer and simpler)

no-downgrade: 409fc44
staysame: bc2a9db

@jay

This comment has been minimized.

Member

jay commented May 31, 2015

Viktor those are drafts that are more concept. To reopen would require a different project collaborator who wants to take this on for assignment and explore the issue. To reiterate I think this is just too niche. Most common scenario I see is user wants to stop https->http but you can use CURLPROTO_HTTPS exclusive to do that or with the curl tool --proto-redir -all,https. As you've pointed out though it's hardly generic.

@jay

This comment has been minimized.

Member

jay commented May 31, 2015

Most common scenario I see is user wants to stop https->http but you can use CURLPROTO_HTTPS exclusive to do that or with the curl tool --proto-redir https.

uhm... I mean --proto-redir -all,https (equivalent to --proto-redir =https). See manual for explanation of +, - and =

@vszakats

This comment has been minimized.

Member

vszakats commented May 31, 2015

Thanks for you reply Jay.

I'd like to reiterate that http->https is also unsafe. (not only https->http)

Hence the recent notion of introducing HSTS in various browsers and creating the protocol for it:
https://developer.mozilla.org/en-US/docs/Web/Security/HTTP_strict_transport_security
https://tools.ietf.org/html/rfc6797

HSTS is a wider topic, but the starting point is that http->https redirection is deemed insecure and something to avoid.

It'd be nice to see curl to follow other HTTP clients and make it easy to avoid this and other security pitfalls. Currently there is no way to enforce secure behavior with a single set of curl/libcurl options, in this regard, yes, probably not too many people will bother. Implementing this feature is lightweight and without any significant future cost (as proven by your patch).

Please note that CURLOPT_REDIR_PROTOCOLS is already a niche feature compared to CURLOPT_FOLLOWLOCATION [1]. Giving it an extra - useful - option would be an opportunity to make this feature utilized or utilized more.

[1] Google returns 379000 hits for the latter and 4610 for the former.

@bagder

This comment has been minimized.

Member

bagder commented May 31, 2015

I certainly wouldn't mind supporting HSTS, but that would need to be an option that specifies where to save the "cache" for that information - it would be tricky to add just transparently and by default - those "other HTTP clients" you speak of are all browsers I take it?. HSTS is however a slightly different topic than the much simpler option this issue is about.

So, this is not shutting down this feature, just that I prefer not to have open issues lingering around if nobody is working on them, especially not for feature requests.

@vszakats

This comment has been minimized.

Member

vszakats commented May 31, 2015

Sorry for bringing in HSTS, it was solely to make a point on http->https "upgrade" being harmful. "other HTTP clients" are all browsers at this moment, yes (and a recent proposal to implement it in wget, here: http://www.burgersoftware.es/gsoc2015.pdf). It's indeed a different topic altogether (being a cache or a hard-wired list pulled from Chrome, Mozilla or wherever else it develops).

Fully on the same boat about lingering Issues. What is unclear to me is what else work needs to be done here, and is there anything I could help to make this happen? We'd need a conclusion on 'staysafe' vs. 'no-downgrade', and certainly one about it being too niche or useful for inclusion, but the patches (to me) look complete, except regression tests.

@bagder

This comment has been minimized.

Member

bagder commented May 31, 2015

I got the impression none of the patches were suggested for real so I've not bothered about them, and @jay seems to have basically the same view. Sorry if that was wrong.

I'll welcome a full patch/pull-request.

@vszakats

This comment has been minimized.

Member

vszakats commented May 31, 2015

It's rather me having being slow understanding the difference between a "real" patch and the ones @jay created. In some spare I'll try finding out what's missing for the staysame proposal.

@bagder

This comment has been minimized.

Member

bagder commented Jun 29, 2015

@vszakats it was no pull request and it was not a posted patch, it was a link to a set of commits. A "real" patch would be a patch sent to the mailing list, a pull-request would appear as one here on github.

@rugk

This comment has been minimized.

Contributor

rugk commented Sep 19, 2016

As for HSTS, I created a new issue: #1026

@NicoHood

This comment has been minimized.

NicoHood commented Jan 19, 2017

Is there any progress for now? I try to detect redirects to http which i want to forbid. It would be nice if this feature would be available and also a special exit code to detect if the error was caused by the redirection downgrade.

@vszakats

This comment has been minimized.

Member

vszakats commented Jan 19, 2017

My solution/workaround for now is to use curl option --proto-redir =https (and the equivalent libcurl option curl_easy_setopt( curl, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTPS )), wherever -L is enabled and the initial URL uses https.

[ When the URL is a variable and/or it happens to use http, the allowed redir protocols can be extended with http. This logic is not practical to implement when using curl from scripts though (as noted in OP). Still not a generic solution overall, but covers most practical use-cases with existing facilities. ]

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.