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

SSL false sense of security #5899

Closed
mcodev31 opened this issue Apr 16, 2022 · 14 comments
Closed

SSL false sense of security #5899

mcodev31 opened this issue Apr 16, 2022 · 14 comments
Assignees
Labels
enhancement team:PS Assigned to OTP team PS

Comments

@mcodev31
Copy link

Is your feature request related to a problem? Please describe.
Some of the ssl default options/behaviour leads to a false sense of security.

Most ssl systems will force explicit deactivation of peer verification in order to prevent accidental mitm exposure (otp default is verify_none).
Using cacerts option for both server can client CAs is a potential security problem where e.g. a client cert can be signed by a cert in the server CA chain.
Both of these problems can be avoided by the user (verify_peer, don't specify server CA, only chain), but typically with a security related library you want to err on the side of caution.

Describe the solution you'd like
Default verify to verify_peer, and clear separation of ca for server/client certificates.

Additional context
You could e.g. imagine a http client (meant for http) being exposed to a user that specifies a https url giving them a false sense of security.

@paulo-ferraz-oliveira
Copy link
Contributor

@mcodev31, not sure you're aware but there's also this reference: https://erlef.github.io/security-wg/secure_coding_and_deployment_hardening/ssl.html, from the Security Working Group of the Erlang Ecosystem Foundation.

@mcodev31
Copy link
Author

Thanks,

I'm not stating that there is a lack of documentation (although that article should probably mention the dangers of adding server and client root ca to cacerts), but rather that, given how prone to error the area of security is, it may be a good idea for OTP to lower the risk of the user removing the S from TLS unknowingly.
E.g. curl could have set the -k option by default and documented it in the man pages, but then the common practice of these types of install commands: curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh would cause some obvious security concerns (more so than currently) and many users would not know that there was a problem at all.

@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Apr 18, 2022
@spencerdcarlson
Copy link

Maybe a good first step is making it more clear that verify_none is the current default.

I don't see any public erlang OTP docs that state that and I always have to jump back into ssl_internal's source code whenever I am trying to show someone erlang's current default SSL behavior.

It seems like something that should be discoverable on the ssl docs but it is not.

@KennethL
Copy link
Contributor

KennethL commented May 5, 2022

I agree that the documentation for the ssl API should clearly state that {verify, verify_none} is the default

@IngelaAndin
Copy link
Contributor

We should improve the documentation, however, we do have this warning (since OTP 24 I think) if you have not specified any CA-certs.

=WARNING REPORT==== 5-May-2022::10:19:07.667363 ===
Description: "Authenticity is not established by certificate path validation"
Reason: "Option {verify, verify_peer} and cacertfile/cacerts is missing"

@rnowak
Copy link
Contributor

rnowak commented May 5, 2022

Worth noting is that while {verify, verify_none} is the default, the warning will be shown unless it is explicitly set to that. Silencing the warning by explicitly setting verify_none was introduced in 24.2, I believe.

@mcodev31
Copy link
Author

mcodev31 commented May 5, 2022

I don't think that this is a big problem in general (or perhaps even a problem at all, if you feel this is how it should be) but simple things like this just feel like they should fail to me (hence the suggestion in the first place):

#!/usr/bin/env escript
%% -*- erlang -*-

main([Url,File]) ->
    inets:start(),
    ssl:start(),
    {ok,{{"HTTP/1.1",200,"OK"}, _Hrds, Rsp}} = httpc:request(Url),
    ok = file:write_file(File,Rsp).

./http_to_file.escript https://www.erlang.org myfile.html > /dev/null

since it may very well be that someone running that command assumes that the cert is verified against a system root (like so many things are), or perhaps doesn't understand TLS at all.

@IngelaAndin
Copy link
Contributor

The default behavior of an HTTP client (such as httpc) and the ssl application (that is a socket/transport API) are different things. The ssl application does not make assumptions and decisions that it does not have enough knowledge to make. We have been working on support for OS-provided CA certs that can be used to improve httpc implementation.

@mcodev31
Copy link
Author

mcodev31 commented May 7, 2022

Of course they are different, but the example problem above also applies to e.g. gun or ibrowse, which I think is telling. These projects have a lot of users and hence a lot of potential security vulnerabilities.

The application doesn't have to make any assumptions. Just like opting into weak ciphers is explicit, opting out of one of the main TLS security measures should be as well.

If it's not clear by now, my suggestion is to not compromise the security of the protocol by default and rely on the application developer having a good enough understanding of the implementation to rectify the problem.

If that can't be done then I would suggest at least adding peer verification options to all examples like these 2 client> {ok, Socket} = ssl:connect("localhost", 9999, [], infinity). in the erlang docs e.g.
Using SSL application API so that when people copy paste the code they don't introduce a MITM vulnerability.

Thanks for all the good work.

@csrl
Copy link

csrl commented May 12, 2022

The side initiating the connection should always verify the peer by default. That said, without ca store to use by default, that doesn't work out. Looking forward to:

We have been working on support for OS-provided CA certs that can be used to improve httpc implementation

and hoping that can be applied to ssl app itself, not just httpc.

@IngelaAndin
Copy link
Contributor

I think that we agree on the basic principle of having secure defaults when possible. But not every user of the ssl application is an HTTPS client. I do not think it should be up to the general library to decide who the user application should trust. For many other applications trusting the hundreds of web CAs out there does not make sense. We will be fixing httpc (as we maintain it). I do understand that from your perspective it would be nice if all HTTPS clients were just automatically fixed by our default values. However, we now made it easy for Web-client developers to use the os provided CA-certs for their applications, so hopefully, they will.

@mcodev31
Copy link
Author

Actually my suggestion is not to default to any os root store, but rather to fail with an error when no ca is supplied (unless no peer verification is set). It would be a breaking change, but the “break” would be to expose vulnerable connections. But great that it’s easier to use os certs.

@IngelaAndin
Copy link
Contributor

We could consider such a breaking change for the OTP-26 time frame. We did add the
warning showed below some time ago as a nonbreaking change but in order to remove the false sense of security.

``=WARNING REPORT==== 12-May-2022::10:20:12.907384 ===
Description: "Authenticity is not established by certificate path validation"
Reason: "Option {verify, verify_peer} and cacertfile/cacerts is missing"

@csrl
Copy link

csrl commented May 16, 2022

Yah, I agree that not all ssl clients would want to trust the OS cert store by default. I do agree it is better to fail by default if no CA is provided. Hopefully OTP 26 can make that change. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement team:PS Assigned to OTP team PS
Projects
None yet
Development

No branches or pull requests

8 participants