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

curl: warn when --insecure is used #1821

Closed
wants to merge 1 commit into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Aug 23, 2017

As mentioned in #1810, the --insecure (and its short version -k) command line option is used very widely by programs and scripts all over the world (hundreds of thousands of times in github hosted code alone).

Presumably, not all users of this option fully aware of the implications. One way to try to make more people aware, is to add a warning message in the output when this option is used.

This PR adds such a warning and I figured it could be used as a sort of discussion-starter of what we can do to raise people's awareness and encourage that users rather make secure SSL connections.

@tlhackque
Copy link

tlhackque commented Aug 23, 2017

As noted on curl-users, I would rather see a warning only if --insecure actually results in an insecure connection.

That is, curl should always attempt to connect with verification, even when --insecure is specified. curl should retry with verification disabled only if verification fails on the first attempt and --insecure is specified. If the second attempt succeeds, then issue the 'This TLS connection is insecure because the server certificate is not trusted` warning.

While this causes an 'extra' connection attempt in the case where the server certificate can not be verified, it prevents warning fatigue -- issuing a warning when in fact nothing is wrong. I don't mind penalizing connections to an untrusted server to avoid further habituating users to ignore warnings.

See the full discussion on curl-users and #1822 for a more complete discussion of the underlying issue.

@coveralls
Copy link

coveralls commented Aug 23, 2017

Coverage Status

Changes Unknown when pulling d82708e on bagder/insecure-warning into ** on master**.

@jay
Copy link
Member

jay commented Aug 24, 2017

That is, curl should always attempt to connect with verification, even when --insecure is specified.

Disagree. If someone uses --insecure they get what they ask for.

@jzakrzewski
Copy link
Contributor

I'm with @jay here.
An we want to discourage using the --insecure switch altogether. The way @tlhackque proposes actually encourages to use it because you'd get secure connection by default and if not, there would only be a warning but stuff would work which in this case is undesirable.

@tlhackque
Copy link

If someone uses --insecure they get what they ask for.

The question is what they asked for. And what this PR is actually trying to accomplish.

Previously, what they got was a connection to a site, even if the certificate could not be verified. No warning, no error - just connect. So that's what anyone who has used -k up to now is asking for.

This PR changes that. Now you get a warning whether or not the certificate can be verified. So any current user is NOT asking for a warning that (s)he is doing something dangerous. The PR is adding a warning - whether or not the user is in fact doing something dangerous. Substituting your judgement for theirs, based on the stated belief that many users don't know what they are doing.

The motivation is good - educate users and try to improve security by having --insecure used judiciously. But the implementation causes a new problem: you still assume that the user is uneducated/following random instructions when the site certificate may in fact be verifiable, yet you provide a warning that the connection IS insecure without even trying to see if the certificate can be verified.

The result is that you do not attain your goal, which is that --insecure is employed only when necessary. Instead, your PR:

  • warns about sites that don't need --insecure
  • provides yet another "warning" for users to ignore
  • misses an opportunity to notify the user whether --insecure is necessary for a site.

An alternative to only warning when the certificate fails to verify is to also warn when --insecure is used but the certificate DOES verify. This would address the last point. I didn't suggest this initially because it seemed better to meet the current user expectation of silent success when things are OK.

I believe that either of my suggestions would advance your goals more effectively than the current PR.

@jay
Copy link
Member

jay commented Aug 25, 2017

If someone uses --insecure they get what they ask for.

The question is what they asked for.

That's what they asked for. (Cue Abbott and Costello). I think the warning is unnecessary.

@bagder
Copy link
Member Author

bagder commented Aug 25, 2017

I'm not pushing forward on this idea, as I fear it won't help much and add to our general warning-fatigue. I'm instead trying out a larger take, see Trust On First Use.

@bagder bagder closed this Aug 25, 2017
@vszakats
Copy link
Member

vszakats commented Aug 25, 2017

The warning can easily be avoided by just using http:// protocol, instead of using --insecure. This way the lack of security is obvious.

With --insecure, and especially with the -k flag that's been added there in the past and forgotten since (or its meaning being misunderstood), there is very likely a false sense/appearance of security. Meaning, the logs/script suggest transfer is secure while in reality it's just a theatre.

curl isn't known to display unnecessary warnings/messages, so "warning fatigue" isn't a strong argument in this particular case. Also the fix is easy: remove the switch if the URLs correctly implement HTTPS, and use HTTP where they don't. Using HTTPS with --insecure can then be left for debugging/troubleshooting/temporary purposes only, where such a warning message is fine.

Having seen the confusion over this flag (also in wget), I think the warning is a good idea to help bringing the issue to the surface (at least when using the command-line tool.)

(To ease on "warning-fatigue", the warning may be omitted when used with HTTP protocol. I've seen projects where the flag is used with HTTP - yet another sign of getting the meaning of this flag wrong.)

@tlhackque
Copy link

The warning can easily be avoided by just using http:// protocol, instead of using --insecure. This way the lack of security is obvious.

No. There many sites that do not respond to http, or if they do, simply redirect to https. This is increasingly common with the push for TLS everywhere. If the certificate is self-signed/not locally trusted, the site is inaccessible without some means of adding trust (or ignoring the error.)

TOFU seems like the best path forward.

@vszakats
Copy link
Member

If a site unconditionally redirects HTTP to HTTPS and have a broken (self-signed/untrusted/etc) certificate, it's a site that should be fixed. Such scenario also don't seem to be very widespread, at least not on the public internet and such site is already broken when used from a browser.

So, if the curl command-line behaviour is optimized around the above use-case, it's optimizing around the exception of seriously/unrealistically misconfigured web servers.

@vszakats
Copy link
Member

vszakats commented Aug 25, 2017

As for TOFU: To me this seems to address a different problem and won't help with the hundreds of thousands of cases where -k/--insecure is already added and silently nilling the (positive) effect of HTTPS.

It also inherits the issues of known_hosts in SSH, where for the first connection an interactive human actor needs to be present to manually verify and confirm a site being secure (if I'm reading the proposal correctly.)
TOFU seems to be inventing and adding a complex protocol to allow using the 'insecure' option on a per-site basis. It also stores state in a volatile, local config file, so its effect is impossible to track/replicate just by looking at scripts/curl command-lines.

(It'd be nice to add a few words about what problem TOFU is exactly designed to solve though. Maybe I'm misunderstanding it completely. It'd be also nice to verify how its functionality may or may not overlap with HSTS.)

@bagder
Copy link
Member Author

bagder commented Aug 25, 2017

The TOFU concept could help people avoid --insecure for most common cases and thus make the consecutive transfers to that site done securely. Today we tell the users "don't use -k, get the cacert, put it somewhere and then use ---cacert in the future when you use that site". This concept could do that job for them.

HSTS support would be lovely but seems to be somewhat orthogonal as that automatically upgrades HTTP to HTTPS for us - but doesn't really address the trust part.

@vszakats
Copy link
Member

vszakats commented Aug 25, 2017

Not sure what the common case may be for --insecure. (What I've seen is: There was/is one old machine where HTTPS didn't seem to work, or a site with a temporarily expired cert where an URL didn't work, so security got disabled for all users from that point on to solve it. IOW as a "solve-it-all" switch. The self-signed/custom certificate issue could already be addressed by passing the correct custom CA cert. UPDATE: On Windows systems it's probably common that the CA bundle is left unused.)

I think inherently TOFU encourages to create manual "custom trusts" (by accepting whatever cert the server is offering), instead of fixing the underlying TLS/server configuration or tapping on existing curl features like passing a trusted custom CA certificate. It's also a question how/if people with --insecure enabled will transition to this new system. It will also likely require the introduction of a --batch option, and get this added to all existing non-interactive uses, if the TOFU mode will become the default and users want to avoid a stall waiting for user input.

@jay
Copy link
Member

jay commented Aug 29, 2017

I think inherently TOFU encourages to create manual "custom trusts" (by accepting whatever cert the server is offering), instead of fixing the underlying TLS/server configuration or tapping on existing curl features like passing a trusted custom CA certificate.

Agree. I don't think it will make educated users any safer, they have already established a secure method and if they choose to use --insecure so be it. They could also use pubkey pinning. Just because there are a lot of people using --insecure does not mean they don't know what they're doing. Maybe they need a public resource and don't care to deal with some server's shenanigans. Uneducated though I can see it now, echo y | curl --tofu ...

@vszakats
Copy link
Member

I'd only add that I'd guesstimate that a significant portion of --insecure/-k usage isn't necessarily intentional. Reasons for this may be blind copy-pasting of command-line, not understanding what the drawback of the switch is or even what the switch does (besides magically making all HTTPS issues disappear.)

If someone has chosen this road intentionally: fine. The issue starts to become a bigger problem when downstream/end users are lead to believe they are using security when seeing/specifying an HTTPS:// address, while in reality security is invisibly disabled.

This is even more hidden with direct libcurl usage - and turning off security is even more widespread there (at least according to GitHub code search numbers.)

Speaking of the curl command-line, another possible cure might be to show the certification verification error even with the --insecure option enabled - then let the transfer continue. This may be more informative and positive than a mere warning. A further nudge may be to deprecate the short version (-k), and only leave the long form. This would help to make the issue greppable in existing code. (The short form is very hard to detect with regular text search, and practically impossible when combined with other short options, as in -fsSkL.)

@bagder
Copy link
Member Author

bagder commented Aug 30, 2017

In my view, the discussion around tofu is a discussion on how to make curl enable users to do the right thing easier. Not necessarily make people automatically become perfect netizens without knowing a thing about it. Like "Sure, --insecure is bad, but what's the easy step to make the connection secure?"

A further nudge may be to deprecate the short version ( -k)

True, but I'm firmly against deliberately breaking an unimaginable large number of scripts and programs "out there" for this reason alone. It also goes orthogonality against our efforts to maintain backwards compatibility.

@vszakats
Copy link
Member

vszakats commented Aug 30, 2017

The idea of TOFU is based on the assumption that the majority reason for --insecure use is self-signed certificates. I think this assumption vastly overestimates the ratio of this particular use-case. Unless TOFU is limited to self-signed certs (which doesn't seem easily possible), it's likely to be misused in those numerous cases when a proper CA bundle would have been the correct solution.

I'm still in favour of a warning, at least by showing the cert problems at all times, and possibly toning it down by omitting it for IP addresses and localhost, where a correctly configured certificate is less likely.

I also think that giving new tools for curl users to shoot themselves (and their end-users) in the foot is perhaps not the best solution and curl being as widely used as it is, has some "responsibility" to encourage a better practice, if one exists. This is not altogether different from web-browsers where there was a strong focus in recent years to make it apparent which page is securely loaded and which isn't.

@kdudka
Copy link
Contributor

kdudka commented Sep 7, 2017

I am forwarding a reply on this from Nikos Mavrogiannopoulos (sorry for the delay):

----------  Forwarded Message  ----------
Subject: Re: Trust-On-First-Use implementation for TLS in libcurl
Date: Friday, August 25, 2017, 3:20:22 PM CEST

On Fri, 2017-08-25 at 12:30 +0200, Kamil Dudka wrote:
> curl upstream is working on a Trust-On-First-Use implementation for
> TLS:
> 
> https://github.com/curl/curl/wiki/Trust-On-First-Use
> 
> If you have any comments on the proposed implementation, please speak
> up.

That's quite interesting idea. It would be nice if the trust file is
documented and not very curl-specific. There is nothing specific for
curl there, and wget for example could re-use that.

gnutls has an API which can store such server PIN with form
|g0|hostname|protoname or port|expiration time in time_t|server pubkey

(e.g., to see output run 'gnutls-cli --tofu www.amazon.com' and check
./gnutls/known_hosts)

so if there can be a common file form apps can read it would be a good
starting point to having tofu more widespread. I'm open to revise that
format if they would be interested in something generic and not curl
specific.


One comment on their proposal; I see they plan to store the certificate
for pinning. Storing the certificate for tofu is not very useful. For
example, when one has 1-month validity cert which is renewed each month
but the same private key is kept. 

The way to PIN such certificates is either by storing the public key of
the certificate, or a hash of the public key (just like HPKP does).


The other thing is, that on internet HTTPS servers keys change quite
often, so PINs may not work as well as with ssh servers.

regards,
Nikos
-----------------------------------------

@bagder
Copy link
Member Author

bagder commented Sep 7, 2017

Good points. I've updated the wiki page slightly to make it more clearly prefer pinning.

@bagder bagder deleted the bagder/insecure-warning branch November 11, 2017 23:23
@lock lock bot locked as resolved and limited conversation to collaborators May 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

7 participants