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

Upgrade SSL Certificate to use modern cipher #5256

Closed
Jtasiu opened this Issue Oct 11, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@Jtasiu
Copy link

Jtasiu commented Oct 11, 2018

If you use install the SSL certificate to use HTTPS for the web GUI it uses an old cipher method.

Please upgrade SSL Certificate to support modern TLS cipher method like ChaCha-Poly1305 or at least AES GCM.

@calmh

This comment has been minimized.

Copy link
Member

calmh commented Oct 11, 2018

This looks OK to me. For example, ECDHE-RSA-AES128-GCM-SHA256 is supported.

jb@kvin:~ $ ./test
tls1:	ECDHE-RSA-AES256-SHA
tls1:	AES256-SHA
tls1:	ECDHE-RSA-AES128-SHA
tls1:	AES128-SHA
tls1:	ECDHE-RSA-DES-CBC3-SHA
tls1:	DES-CBC3-SHA
tls1_1:	ECDHE-RSA-AES256-SHA
tls1_1:	AES256-SHA
tls1_1:	ECDHE-RSA-AES128-SHA
tls1_1:	AES128-SHA
tls1_1:	ECDHE-RSA-DES-CBC3-SHA
tls1_1:	DES-CBC3-SHA
tls1_2:	ECDHE-RSA-AES256-SHA
tls1_2:	AES256-SHA
tls1_2:	ECDHE-RSA-AES128-GCM-SHA256
tls1_2:	ECDHE-RSA-AES128-SHA
tls1_2:	AES128-SHA
tls1_2:	ECDHE-RSA-DES-CBC3-SHA
tls1_2:	DES-CBC3-SHA
[1]
jb@kvin:~ $ cat test
#!/bin/sh

for v in tls1 tls1_1 tls1_2; do
 for c in $(openssl ciphers 'ALL:eNULL' | tr ':' ' '); do
  openssl s_client -connect localhost:8384 -verify 0 -cipher $c -$v </dev/null 2>&1 | grep -q 'BEGIN CERTIFICATE' && echo "$v:\t$c"
 done
done
jb@kvin:~ $

(Also limited by what my openssl supports, which for example doesn't include the chacha stuff)

Generally speaking, we don't want or need to be bleeding edge on the GUI. We want something that's reasonably secure enough and supports a lot of browsers.

The sync connection is different.

@Ferroin

This comment has been minimized.

Copy link

Ferroin commented Oct 11, 2018

A couple of points here:

  • GCM modes aren't bleeding edge, they've been around for more than a decade, are reasonably well supported, and are generally considered to be one of the best options right now for non-AEAD connections. You've also got one in that list already.
  • Triple-DES (all those DES-CBC3 modes) is bad. It's just as known broken as plain DES, and the only things being supported by that are systems that should not be on the internet to begin with.
  • The various AEAD modes (including ChaCha20 modes) may be new, but they're actually pretty well supported, and have undergone a reasonable amount of cryptanalysis without any issues being found.
  • Adding 'new' modes without removing older ones can only ever expand what you support, not reduce it. The only exception is if there's a broken implementation of a particular mode, but that's highly unlikely.
  • It looks kind of bad when your main website provides better security (at least according to SSLLabs) than the GUI HTTPS connection does.
@calmh

This comment has been minimized.

Copy link
Member

calmh commented Oct 11, 2018

I misremembered; I though we ran with the Go defaults which should be good enough, but we already have a hardcoded list to git rid of old RC4 stuff. So we should update that or, better, see if the defaults are now acceptable.

While this is somewhat a matter of basic hygiene, I doubt any relevant practical attack against the GUI is going to be based on attacking the cipher suite.

@calmh calmh added the enhancement label Oct 11, 2018

calmh added a commit to calmh/syncthing that referenced this issue Oct 18, 2018

cmd/*, lib/tlsutil: Refactor TLS stuff (fixes syncthing#5256)
This changes the TLS and certificate handling in a few ways:

- We always use TLS 1.2, both for sync connections (as previously) and
  the GUI/REST/discovery stuff. This is a tightening of the requirements
  on the GUI. AS far as I can tell from caniusethis.com every browser from
  2013 and forward supports TLS 1.2, so I think we should be fine.

- We always greate ECDSA certificates. Previously we'd create
  ECDSA-with-RSA certificates for sync connections and pure RSA
  certificates for the web stuff. The new default is more modern and the
  same everywhere. These certificates are OK in TLS 1.2.

- We use the Go CPU detection stuff to choose the cipher suites to use,
  indirectly. The TLS package uses CPU capabilities probing to select
  either AES-GCM (fast if we have AES-NI) or ChaCha20 (faster if we
  don't). These CPU detection things aren't exported though, so the tlsutil
  package now does a quick TLS handshake with itself as part of init().
  If the chosen cipher suite was AES-GCM we prioritize that, otherwise we
  prefer ChaCha20. Some might call this ugly. I think it's awesome.

@calmh calmh closed this in 8519a24 Oct 21, 2018

@calmh calmh added this to the v0.14.53 milestone Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.