Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#3065 closed feature-request (Fixed)

Enhance TLS security

Reported by: Jay-C Owned by: DjLegolas
Priority: minor Milestone: 2.0.0
Component: Web UI Version: develop
Keywords: TLS security Cc:

Description

Following a consideration on ticket 3064.

To meet industry standards on SSL/TLS security and to increase security headroom when exposing the Web UI to the Internet, I believe that some changes are required on the transport side.

  1. Disable TLS v1.0. Only allow TLS >= v1.1 (link). All reasonably modern browsers should have no problem with this.
  1. Respect certificate x509 extensions, especially key usage

For example: Currently, if keyUsage is set to "critical, digitalSignature", which should forbid plain RSA since that is a keyEncipherment operation, the server will happily still use plain RSA. That is... bad.

  1. Enable (only) (elliptic curve or regular) Diffie-Hellman epidermal key exchange cipher suites.

This is necessary to provide Perfect Forward Secrecy. This requires adding DH parameters, but it shouldn't be to hard to generate with dhparams and include it with the distribution or generate it dynamically. Currently there's only plain RSA cipher suites enabled.

  1. Tweak cipher list to prefer AES GCM modes and disable MD5/DES/RC4.

More resistant to certain types of attacks. Current list seems to prefer AES CBC for some reason.

(1) and (4) can be trivially done now but might break compatibility with some outdated clients. (2) I don't know about if Twisted even supports. (3) requires a newer version of Twisted than currently in the Trusty repositories.

Attachments (2)

testssl-result.xhtml (13.4 KB) - added by Jay-C 4 years ago.
Test results from testssl.sh. Redacted for privacy.
deluge-testssl.sh-report.zip (5.9 KB) - added by DjLegolas 3 years ago.
deluge-web and deluged testssl.sh report

Download all attachments as: .zip

Change History (10)

Changed 4 years ago by Jay-C

Test results from testssl.sh. Redacted for privacy.

comment:1 Changed 4 years ago by Cas

  • Milestone changed from needs verified to 2.0

comment:2 Changed 3 years ago by DjLegolas

Started working on this in this branch.
Things I changed:

  • Set TLS minimal version to 1.1
  • Added specific cipher suite list (details bellow)
    • Added support for ECDHE key exchange algorithm
    • Added support for ECDSA authentication keys

All changes applied for both WEBUI and DAEMON.
I've attach 2 testssl.sh results, one for each.
Note: I'm running with twisted version is 18.4.0.

Current issues and things to do:

  • Secure Client-Initiated Renegotiation is still a vulnerability.
  • Daemon does not have a cipher order.
  • Still using ciphers with CBC support because TLSv1.1 does not support something else.
  • Check usage with both RSA and ECDSA authentication keys.

Please reply for any issues or suggestions.


List Of ciphers:

Cipher Suite Version Key Ex Auth Encryption Hash
ECDHE-ECDSA-AES256-GCM-SHA384TLSv1.2ECDHECDSAAESGCM(256)AEAD
ECDHE-RSA-AES256-GCM-SHA384TLSv1.2ECDHRSAAESGCM(256)AEAD
ECDHE-ECDSA-CHACHA20-POLY1305TLSv1.2ECDHECDSACHACHA20/POLY1305(256)AEAD
ECDHE-RSA-CHACHA20-POLY1305TLSv1.2ECDHRSACHACHA20/POLY1305(256)AEAD
ECDHE-ECDSA-AES128-GCM-SHA256TLSv1.2ECDHECDSAAESGCM(128)AEAD
ECDHE-RSA-AES128-GCM-SHA256TLSv1.2ECDHRSAAESGCM(128)AEAD
ECDHE-ECDSA-AES256-SHA384TLSv1.2ECDHECDSAAES(256)SHA384
ECDHE-RSA-AES256-SHA384TLSv1.2ECDHRSAAES(256)SHA384
ECDHE-ECDSA-AES128-SHA256TLSv1.2ECDHECDSAAES(128)SHA256
ECDHE-RSA-AES128-SHA256TLSv1.2ECDHRSAAES(128)SHA256
ECDHE-ECDSA-AES256-SHATLSv1ECDHECDSAAES(256)SHA1
ECDHE-RSA-AES256-SHATLSv1ECDHRSAAES(256)SHA1
ECDHE-ECDSA-AES128-SHATLSv1ECDHECDSAAES(128)SHA1
ECDHE-RSA-AES128-SHATLSv1ECDHRSAAES(128)SHA1
AES256-GCM-SHA384TLSv1.2RSARSAAESGCM(256)AEAD
AES128-GCM-SHA256TLSv1.2RSARSAAESGCM(128)AEAD
AES256-SHA256TLSv1.2RSARSAAES(256)SHA256
AES128-SHA256TLSv1.2RSARSAAES(128)SHA256
AES256-SHASSLv3RSARSAAES(256)SHA1
AES128-SHASSLv3RSARSAAES(128)SHA1

Changed 3 years ago by DjLegolas

deluge-web and deluged testssl.sh report

comment:3 Changed 3 years ago by Cas

I think that setting the minimum to TLS 1.2 for RPCServer would be better. In fact since TLS 1.2 is supported in nearly every browser now, the web server too. Thoughts?

For the cipher list use this format with comments (also add ref link): https://github.com/urllib3/urllib3/blob/master/urllib3/util/ssl_.py#L79

How did you generate the report, I feel that we should have a unit test that verifies the security. I see that testssh can output in json format and filter by severity.

Last edited 3 years ago by Cas (previous) (diff)

comment:4 Changed 3 years ago by DjLegolas

Web support for only TLS 1.2 will force all users to start using modern browsers which is always a good thing.
Therefor, I removed all CBC ciphers (everything without GCM or ChaCha20 in its name).

In addition, I noticed that Twisted sets a server HTTP-header with the version of Twisted. This can help attackers to exploit some known vulnerabilities within Twisted.
To prevent it, I added a function which sets the header with the server type only (TwistedWeb).

Regarding the test units, I'll start write something.
And more of this topic, I think we need to add support for codecov.
This way we will know what part of deluge is being covered by the test units.

comment:5 Changed 3 years ago by Cas

I created a PR so I can easily add comments: https://github.com/deluge-torrent/deluge/pull/196

We have and can use trial or pytest to get coverage and include it in Travis run, although coverage is not something I want to worry about just now as it's really not good...

comment:6 Changed 3 years ago by DjLegolas

  • Owner set to DjLegolas
  • Status changed from new to accepted

comment:7 Changed 3 years ago by Cas

  • Resolution set to Fixed
  • Status changed from accepted to closed

Fixed in develop, thanks!

comment:8 Changed 3 years ago by Cas

  • Milestone changed from 2.0 to 2.0.0

Milestone renamed

Note: See TracTickets for help on using tickets.