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

US server uses insecure ciphers #23

Open
triska opened this issue Jun 26, 2017 · 20 comments
Open

US server uses insecure ciphers #23

triska opened this issue Jun 26, 2017 · 20 comments

Comments

@triska
Copy link
Member

triska commented Jun 26, 2017

The security of us.swi-prolog.org is currently graded with the worst possible mark F.

For more information, please see:

https://www.ssllabs.com/ssltest/analyze.html?d=us.swi-prolog.org

To mitigate many of the issues that are reported in this assessment, please start the server with the following option:

--cipherlist='EECDH+AESGCM:EDH+AESGCM:EECDH+AES256:EDH+AES256:EECDH+CHACHA20:EDH+CHACHA20'

This restricts the set of acceptable ciphers to a much more secure subset. You can see in the above assessment which clients are ruled out by these restrictions. Only very old software is affected by this. Any site that wants to receive a grading of A or higher needs to use only a subset of these secure ciphers.

Note that the CHACHA20 ciphers are only available with OpenSSL 1.1.0 or greater. It is OK to use them in the setting above. It only means that they are not actually available when negotiating TLS connections.

@triska
Copy link
Member Author

triska commented Jun 26, 2017

One useful command to test which ciphers are currently supported by a server is nmap.

For example, for eu.swi-prolog.org:

$ nmap --script ssl-enum-ciphers eu.swi-prolog.org -p 443

This yields:

Starting Nmap 6.47 ( http://nmap.org ) at 2017-06-26 22:41 CEST
Nmap scan report for eu.swi-prolog.org (130.37.193.190)
Host is up (0.012s latency).
rDNS record for 130.37.193.190: ops.few.vu.nl
PORT    STATE SERVICE
443/tcp open  https
| ssl-enum-ciphers:
|   SSLv3: No supported ciphers found
|   TLSv1.0:
|     ciphers:
|       TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA - strong
|       TLS_DHE_RSA_WITH_AES_128_CBC_SHA - strong
|       TLS_DHE_RSA_WITH_AES_256_CBC_SHA - strong
|       TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA - strong
|       TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA - strong
|       TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA - strong
|       TLS_RSA_WITH_3DES_EDE_CBC_SHA - strong
|       TLS_RSA_WITH_AES_128_CBC_SHA - strong
|       TLS_RSA_WITH_AES_256_CBC_SHA - strong
|     compressors:
|       NULL
|   TLSv1.1:
|     ciphers:
|       TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA - strong
|       TLS_DHE_RSA_WITH_AES_128_CBC_SHA - strong
|       TLS_DHE_RSA_WITH_AES_256_CBC_SHA - strong
|       TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA - strong
|       TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA - strong
|       TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA - strong
|       TLS_RSA_WITH_3DES_EDE_CBC_SHA - strong
|       TLS_RSA_WITH_AES_128_CBC_SHA - strong
|       TLS_RSA_WITH_AES_256_CBC_SHA - strong
|     compressors:
|       NULL
|   TLSv1.2:
|     ciphers:
|       TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA - strong
|       TLS_DHE_RSA_WITH_AES_128_CBC_SHA - strong
|       TLS_DHE_RSA_WITH_AES_128_CBC_SHA256 - strong
|       TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 - strong
|       TLS_DHE_RSA_WITH_AES_256_CBC_SHA - strong
|       TLS_DHE_RSA_WITH_AES_256_CBC_SHA256 - strong
|       TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 - strong
|       TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA - strong
|       TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA - strong
|       TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 - strong
|       TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 - strong
|       TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA - strong
|       TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 - strong
|       TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 - strong
|       TLS_RSA_WITH_3DES_EDE_CBC_SHA - strong
|       TLS_RSA_WITH_AES_128_CBC_SHA - strong
|       TLS_RSA_WITH_AES_128_CBC_SHA256 - strong
|       TLS_RSA_WITH_AES_128_GCM_SHA256 - strong
|       TLS_RSA_WITH_AES_256_CBC_SHA - strong
|       TLS_RSA_WITH_AES_256_CBC_SHA256 - strong
|       TLS_RSA_WITH_AES_256_GCM_SHA384 - strong
|     compressors:
|       NULL
|_  least strength: strong

Nmap done: 1 IP address (1 host up) scanned in 0.95 seconds

In this case, all accepted ciphers are graded strong by nmap.

@JanWielemaker
Copy link
Member

Should we care though? We do not need to protect anything, especially not on the US server that has no editing capabilities but simply mirrors from the EU server using SSH. It is the user that may want more or less protection. The server offers plenty secure connection options and, as I understand it, modern clients will choose one of those.

@triska
Copy link
Member Author

triska commented Jun 27, 2017

You must disable obsolete ciphers to reliably mitigate protocol downgrade attacks. This is a situation where the client does want to use a more secure cipher, but is tricked by a malicious third party to think that the server does not support it, and so uses a weaker cipher to negotiate the connection which the server then accepts.

I have one general comment in response to the "we have nothing to protect" argument, which I have now seen a few times already to seemingly justify a situation where no or only insufficient encryption is used for critical services:

The attack vector for a simple mirroring server is not that the SWI-Prolog homepage is hosted as you intend it.

Rather, a major risk when using obsolete ciphers in this case is that a malicious third party poses as you to serve content that you did not intend, using the popularity of SWI-Prolog for their own purposes in manipulated sessions that users think take place with the SWI-Prolog site (which they trust), but in fact occur with an attacker that is acting as man in the middle.

So, yes, I definitely think this is worth caring about. Using the --cipherlist option as shown above guarantees authenticity that is deemed sufficiently secure by today's standards, and therefore reliably prevents third parties from posing as you.

@JanWielemaker
Copy link
Member

Of course this is not about the work of adding an option. It is more that, as a humble programmer, I want to enable HTTPS in a simple way and have something that adheres to the current practices. I think the default selection of acceptable protocols and ciphers should be done elsewhere. Of course, there are projects with particularly strong or unconventionally weak security demands (due to interfacing needs with old software).

So, my proposal would be to add a setting, either to the HTTPS plugin or the core SSL layer. I'm not sure about that. Most users probably want HTTPS and in a sense we could assume that people who go down to SSL are supposed to know what they are doing and may often have non-standard requirements. I guess the HTTPS community has some best practice.

Does that make sense? And, as the person knowing most about this, can you propose something?

@triska
Copy link
Member Author

triska commented Jun 28, 2017

OK, I have filed SWI-Prolog/packages-http#97 which makes the HTTP Unix daemon use the much more secure ciphers as the new default. I think the Unix daemon is a good place to ensure this as the most convenient way to run an HTTPS server with SWI-Prolog.

I have been running SWI-Prolog HTTPS servers with this setting for over a year and they are all reachable from a large variety of devices (including Android, iOS and Blackberry). Please try it it out and merge as applicable.

These ciphers are also recommended by:

https://cipherli.st/

and to those I have added the CHACHA20 ciphers which are available as of OpenSSL 1.1.0 and deemed to be very secure while at the same time providing better performance than AES-GCM on systems that lack the necessary processor instructions or hardware (such as smartphones) for utmost efficiency.

@JanWielemaker
Copy link
Member

Better. Now get a C rating due to SSLv3. I guess that is because this machine is running a pretty old OS that ships with an old SSL library. Not sure what to do: disable SSLv3 from our binding or update the OS and accept that you do not have up-to-date security if you use old versions of OpenSSL.

@triska
Copy link
Member Author

triska commented Jun 30, 2017

To be completely honest I can only recommend to upgrade OpenSSL in this case as the only viable option, for the following reasons:

SSLv3 is disabled as of OpenSSL 1.0.1, which was released in March 2012 and is itself now no longer supported. You must upgrade to at least version 1.0.2 to use a supported version of OpenSSL.

I see you have now manually disabled SSLv3 in SWI-Prolog/packages-ssl@04a118e. In my opinion, this falls short for the following reasons:

First, quoting from the manpage of SSL_set_options:

       SSL_OP_NO_SSLv3, SSL_OP_NO_TLSv1, SSL_OP_NO_TLSv1_1, SSL_OP_NO_TLSv1_2,
       SSL_OP_NO_DTLSv1, SSL_OP_NO_DTLSv1_2
           These options turn off the SSLv3, TLSv1, TLSv1.1 or TLSv1.2
           protocol versions with TLS or the DTLSv1, DTLSv1.2 versions with
           DTLS, respectively.  As of OpenSSL 1.1.0, these options are
           deprecated, use SSL_CTX_set_min_proto_version(3) and
           SSL_CTX_set_max_proto_version(3) instead.

This means that we will want to remove the disable_ssl_methods/1 option in the medium term, to support only the min_protocol_version/1 option that maps to the now recommended way in OpenSSL. At that point in the future, specifying disable_ssl_methods/1 will no longer do anything, since it will likely not even be available in future OpenSSL versions and thus be disabled already at compilation time of library(ssl)!

Second, using the min_protocol_version/1 in this patch is redundant: This method is only available starting with OpenSSL 1.1.0, and thus only in versions where SSLv3 is already disabled.

In my view, this all only complicates the code and creates more work for future upgrades, in addition to the two most problematic consequences:

  • Using min_protocol_version/1 as in the patch is a security risk in itself: When a problem with TLS 1.1 is discovered in the future, new OpenSSL versions will disable it by default, but the current patch will re-enable it for SWI-Prolog HTTPS servers!

  • This patch gives a false sense of security to users who are using unsupported OpenSSL versions!

I would much prefer to make affected users think about how to upgrade their OpenSSL version which by now is older than 5 years. Thus, I can only strongly recommend to:

  1. revert the patch and
  2. upgrade the OpenSSL version on the server instead.

This would ensure that outdated OpenSSL versions could at most obtain grade C with the default settings, and thus highlights an urgent problem with the OpenSSL installation for affected users, whereas the current approach unfortunately only overshadows it.

triska referenced this issue in SWI-Prolog/packages-ssl Jun 30, 2017
@JanWielemaker
Copy link
Member

The problem is that LTS versions of Ubuntu and other Linux distros that are rather conservative and generally provide long term support are precisely those that are often deployed in servers. Centos
is a good example. Our US server runs Ubuntu 14.04, which ships with OpenSSL 1.0.1f and has SSLv3 enabled. I assume that although the OpenSSL project may not support this version, Ubuntu (and most likely most other vendors providing long term support versions) will backport critical security issues. At the same time, given their LTS promise, they cannot simply drop protocols as this may stop services to work. It is of course a bit odd to support a protocol that is proven to be insecure. Still, that is simply how the IT landscape evolves. And no, sysadmins are typically way more reluctant to compile their own version precisely because they need to take care of them themselves instead of relying on the vendor security updates.

As is we have a shorter release cycle and it makes sense for us to comply with the security practice at the moment of our releases. If possible we should do that on the basis of, in this case, older OpenSSL releases. This means we must reconsider this patch as well as the deemed secure ciphers regularly. As a result, users that keep up with the SWI-Prolog releases by default get reasonable current practice security out of the box. If you really care about security, you need to study the subject and make your choices. That is already explained in the manual. I'm ok being even more explicit here and, for example, point at ssllabs or similar services.

@triska
Copy link
Member Author

triska commented Jul 1, 2017

Please consider the following two points:

First, the min_protocol_version/1 option is only available starting with OpenSSL 1.1.0, and these versions already disable SSLv3 by default. So, I see no point in adding min_protocol_version(tlsv1) in this patch. In fact, adding this option per default is an additional security risk since it may re-enable TLSv1 on those systems where it is disabled by default.

Second, to fine-tune SSL parameters, I have added the hook predicates to the HTTP Unix daemon. Users can easily extend them to add additional restrictions. However, disabling a protocol version from a given context is currently not possible via the library(ssl) API. This is one of the missing features I still need to implement. It will be a special case of a more general predicate that copies and modifies a given context according to given parameters. ssl_set_sni_hook/3 will also be subsumed by this more general predicate.

Given such a predicate, tentatively called ssl_set_context/3, users who are stuck with outdated OpenSSL versions could disable SSLv3 as follows:

http:ssl_server_create_hook(SSL0, SSL, _) :-
        ssl_set_context(SSL0, SSL, [disable_ssl_methods([sslv3,sslv23])]).

This works without weakening or complicating the code that ships with SWI-Prolog, and lets us stay close to those parameters that OpenSSL itself will use by default in all new versions.

I have not yet implemented ssl_set_context/3 because it is not clear how to handle some cases where the specified options conflict with those that were given at the time the context was created. I mean not only conceptually, but even the behaviour of OpenSSL is not clear in such cases. For this reason, I have filed the following issue:

openssl/openssl#2147

I could still implement such a predicate, and for now restrict the use cases to those where it is clear what OpenSSL does, such as disabling protocol versions and setting hooks. This predicate will be an important building block to users who want more control over SSL settings.

Please consider whether it would work for you as a solution of this issue.

@JanWielemaker
Copy link
Member

My initial intend was to just pass [disable_ssl_methods([sslv3,sslv23])], which be quite harmless. Those versions that understand these options disable these outdated methods and those that don't just ignore the option. I haven't investigated why, but Trusty's OpenSSL 1.0.1f ignored this option according to ssllabs. It did work after setting min_protocol_version. Deprecating disable_ssl_methods with min_protocol_version seems rather dubious to me, exactly for the reason you point out: if you disable an unsafe method you can be sure you made it safer. If you set a minimum version you don't know.

Anyway, I'm not against a new hook to have better control over the context. As far as I'm concerned this should be part of http_ssl_plugin.pl as that is the core HTTP server stuff. The http_unix_daemon.pl is just there for unix daemons. If anything, I'd propose to move non-OS specific stuff such as certificate handling to http_ssl_plugin.pl where it (1) is part of the SSL package were it logically belongs and (2) can be reused in non-Unix settings. Of course, http_unix_daemon.pl should use the moved functionality and remain compatible.

The other strong wish I have is that you get a reasonably sane setup if you just load the default code on all maintained OSes. This notably includes releases such as redhat/centos and the Ubuntu LTS versions. It seems we do not need to maintain OpenSSL < 1.0 any more. That simplifies things. A big warning that you should evaluate the security of your server might be a good idea ...

I do not see the need to hurry though. In particularly, think first about what you want the new hook to be capable of.

@triska
Copy link
Member Author

triska commented Jul 1, 2017

Only for clarification, in case anyone is interested:

  • It is OpenSSL that has deprecated the API that we map to disable_ssl_methods! In future versions, this will likely no longer be available. The reason for this is that using disable_ssl_methods (or rather, the OpenSSL API that it is mapped to) is prone to leaving "holes" in the supported protocols, where lower versions are supported but higher ones only partially.
  • Said hook is already implemented in SWI-Prolog, exactly where you say (http_ssl_plugin.pl). The Unix daemon benefits from this too.

@JanWielemaker
Copy link
Member

I know it is OpenSSL deprecating disable_ssl_methods. I don't know why it is a problem to leave "holes". This seems to assume that if v1 and v3 are safe, v2 is so by definition. I doubt this is true, unless a new version is only added after the previous one was proven to be unsafe. I think that is not how it works. Anyway, we just have to deal with OpenSSL 1.0 or later and their quirks for now.

Said hook is already implemented in SWI-Prolog, exactly where you say (http_ssl_plugin.pl).

? I see no such hook in thread_httpd:make_socket_hook/3, just the hacks to set the ciphers and disable sslv3. I refer to them as hacks as IMO OpenSSL should have provided facilities that make such kludges at the application level unneeded. For example by providing a /etc/ssl/ssl.conf file or something similar that specifies methods and ciphers and is maintained by the OS to remain up-to-date while allowing specific hosts and applications to do things differently. Just as the OS provides the set of trusted root certificate as a default for all SSL applications.

@triska
Copy link
Member Author

triska commented Jul 1, 2017

I haven't investigated why, but Trusty's OpenSSL 1.0.1f ignored this option according to ssllabs.

This is an additional security issue in itself!

@JanWielemaker
Copy link
Member

I had a brief look. Processing disable_ssl_methods was a little shaky, but not wrong. I do not see a deprecation on https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_options(3) though. For now I leave this.

@triska
Copy link
Member Author

triska commented Jul 1, 2017

The description you link to is outdated. Please use the following version:

https://www.openssl.org/docs/man1.1.0/ssl/SSL_CTX_set_options.html

I quote (again):

SSL_OP_NO_SSLv3, SSL_OP_NO_TLSv1, SSL_OP_NO_TLSv1_1, SSL_OP_NO_TLSv1_2, SSL_OP_NO_DTLSv1, SSL_OP_NO_DTLSv1_2

These options turn off the SSLv3, TLSv1, TLSv1.1 or TLSv1.2 protocol versions with TLS or the DTLSv1, DTLSv1.2 versions with DTLS, respectively. As of OpenSSL 1.1.0, these options are deprecated, use SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version instead.

You can expect that this way to disable SSL versions will be removed in future versions of OpenSSL.

@triska
Copy link
Member Author

triska commented Jul 1, 2017

I see no such hook in thread_httpd:make_socket_hook/3, just the hacks to set the ciphers and disable sslv3.

Seeing this question, I think I need to explain this all in more detail. Please do not take the following as arguing against the concrete hacks you have now added (although I can still only recommend to revert them and solve the issues differently, since you have probably also discovered a security problem in the likely botched OpenSSL version you are using). Rather, in this post, I want you to understand the overall architecture and how you can best tweak SSL parameters in user code.

The hook I mentioned is already available. I have added it in:

SWI-Prolog/packages-ssl@27e8bd1

Since this commit, http:ssl_server_create_hook/3 is automatically called when the server is started. This hook is the right place for users to tweak any SSL parameters, overriding the system settings if necessary! As the first argument, they are given the SSL context that would normally be used to create the server, and they can compute a new context that is used instead.

In your case for example, you need a way to disable SSLv3 for a given SSL context. For this, let us briefly suppose that a predicate ssl_set_options/3 were available, with the following meaning:

ssl_set_options(+SSL0, -SSL, +Options)

Create a new context SSL that has the same settings as SSL0, expect for those that are
specified in Options.

Once this predicate is available, every user can disable SSLv3 if (still) necessary by adding the following definition to their server code:

http:ssl_server_create_hook(SSL0, SSL, _) :-
        ssl_set_options(SSL0, SSL, [disable_ssl_methods([sslv3,sslv23])]).

This requires no hacks in the plugin, only a simple extension in user code for affected users. This is easy to document, and affected users only need to include this snippet in their code to disable SSLv3 with OpenSSL 1.0.1.

Also, a bit of history: It was always clear that ssl_set_options/3 or equivalent predicates are needed to get all the flexibility we need from such hooks. However, I first wanted to have a stable basis via documented behaviour of OpenSSL (see the mentioned OpenSSL issue I filed for this).

Since ssl_set_options/3 is currently not available, users have to use more specialized predicates like ssl_set_sni_hook/3 to tweak existing SSL contexts. This goes in the wrong direction in my view: I want to deprecate and ideally even remove ssl_set_sni_hook/3 and subsume it with the more general ssl_set_options/3. You can then easily get ssl_set_sni_hook/3 as:

ssl_set_sni_hook(SSL0, Goal, SSL) :- ssl_set_options(SSL0, SSL, [sni_hook(Goal)]).

Also, I do not want to add a new predicate for each possible setting that users could specify (even though SSL bindings of other language libraries have done this!), because it unnecessarily increases the number of interface predicates of the library. I rather want it all subsumed by a few well-defined predicates that are easy to understand.

I think ssl_set_options/3, in combination with the already available hooks is the best place to tune SSL parameters for affected users without complicating and/or weakening the main logic, or using features that should rather be deprecated.

I will work on ssl_set_options/3 in the coming days and file a pull request. Please, if possible, read the documentation of these hooks and see how they fit in the overall architecture to give users an opportunity to tweak SSL parameters whether or not they are using the Unix daemon.

@JanWielemaker
Copy link
Member

An ssl_set_options to simplify the public API is certainly a good idea. The hook is however not dealing with what I like dealing with: make sure that the system is somewhat reasonably configured if the user doesn't do anything. That should have been what OpenSSL does, but it doesn't. So, the flow should be

  1. Apply user hooks and configuration
  2. If nothing is said about ciphers and methods, restrict them to a somewhat sensible set.

In that sense the current thread_httpd:make_socket_hook is not ok as the hacks should be after
the http:ssl_server_create_hook hook to check the user didn't specify anything. That is probably tricky as this hook manages the context itself, so it gets hard to figure out whether the context is still in default mode or something was explicitly specified by the user.

Makes we wonder why we create the context first and modify it afterwards instead of modifying the creation options and just create it?

@triska
Copy link
Member Author

triska commented Jul 2, 2017

I have filed the pull request for ssl_set_options/3, please have a look.

To answer your important question "Why create the context first and modify it afterwards", please consider how for example the Unix daemon works:

First, the key, password and certificates are read. These are passed around in plain text!

After the context is created, they are all safely contained in the context.

Now the point: At the time the user-supplied hook is called, it is no longer necessary to pass around sensitive information as arguments! Therefore, the SSL options are decoupled when the hook is invoked, and cannot be inspected. Also, they cannot appear in backtraces!

Also, there is more: The hooks should ideally be so general that even dropping privileges before they are invoked would work: With the Unix daemon, the hooks themselves could be executed purely with user privileges, because they no longer need to read the actual keys and certificates which may require privileged access.

Please consider how this all fits together: ssl_set_options/3 is in any case a very important building block in such hooks, and I wanted to add it for months already (it was delayed due to the mentioned OpenSSL issues, and I have therefore now slightly limited the options to those that can be set safely already).

Other than that, please disable SSLv3 in any way you see fit: To recap, keeping the existing one would have the main drawback that it adds a security risk for more recent OpenSSL versions. Personally, I would recommend to simply show affected users how they can themselves disable SSLv3 with the new predicate ssl_set_options/3 and using the existing hooks, if they are still using unsupported OpenSSL versions and want to continue using them (I cannot recommend relying on distributions for backports in this concrete case). The fact that disable_ssl_methods/1 does not work in your setup could be a security issue in the (modified) OpenSSL version you are using...

@triska
Copy link
Member Author

triska commented Jul 2, 2017

As to your other observation, to reliably disable SSLv3, you can now simply add, somewhere in the plugin code, one additional call of ssl_set_options/3 on the final SSL context that was created by users! This completely eliminates any checking. For example, assuming your version of OpenSSL works correctly, you only have to add:

ssl_set_options(SSL1, SSL, [disable_ssl_methods([sslv3])])

where SSL1 is the result of the hook (initial context SSL0 → hook resultSSL1 → final contextSSL)

For completeness, please also note that the generality of the hooks allows users to create a completely new SSL context from scratch and still benefit from the infrastructure provided by the HTTP Unix daemon in other respects!

@JanWielemaker
Copy link
Member

The idea behind the checking is that if the user says [disable_methods([])] you do not disable any methods and assume the user know what s/he is doing. Similar for the ciphers: if the user specifies ciphers he probably has a reason. If not, the default should be something sensible.

Not sure how to deal with that in the current design. One option might be to have an ssl_property/2
or something similar that lets us query whether the user has modified the ciphers or methods. For now, I'm happy (enough) as it stands.

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

No branches or pull requests

2 participants