BUG: default ciphersSuites for 1.3

[kriskwiatkowski]

Tris has a weird logic, if no TLSv1.3 ciphers are provided than default (v1.3 specific) are added, resulting in adding ciphers which users do not explicitly request. The logic should follow the rule - either user specifies ciphers to be used or it specifies nothing and then default ones are used.

func (c *Config) cipherSuites() []uint16 {
    s := c.CipherSuites
    if s == nil {
        s = defaultCipherSuites()
    } else if c.maxVersion() >= VersionTLS13 {
        // Ensure that TLS 1.3 suites are always present, but respect
        // the application cipher suite preferences.
        s13 := defaultTLS13CipherSuites()
        if !hasOverlappingCipherSuites(s, s13) {
            allSuites := make([]uint16, len(s13)+len(s))
            allSuites = append(allSuites, s13...)
            s = append(allSuites, s...)
        }
    }
    return s
}

Related bugs:

1. It can be assumed that caller has a good reason for not using default set of ciphers. For example, caller may specify not to use AES-128. Because of this bug - tris will allow it and it will allow it only for TLS 1.3. Contract with the caller is violated.
2. Code (above) is creating an array full of zeros of length equal to len(s13)+len(s) and appends IDs from s13 and s. Returned array is equal to 2*(len(s13)+len(s)) - it means cipherSuites() may allow to use cipher suite with ID=0 (TLS_NULL_WITH_NULL_NULL) !? What for? Luckly it is not supported by tris... luckly.

[Lekensteyn]

This is not a bug, it is done by design to ensure that TLS 1.3 support is possible for a broader range of applications.

Consider an application that sets some cipher suites to use modern ciphers with PFS:

config := &tls.Config{
    CipherSuites: []uint16{
        tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
        tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
        tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
        tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
    },
}

The intention is to disable ciphers such as RC4 and AES-CBC as well as those using an RSA kex. Perhaps the developer has set MinVersion: tls.VersionTLS12 while at it.

Now, if this application switches to tris, it should use the more secure TLS 1.3 if possible. If the TLS 1.3 cipher suites configuration is mixed with TLS 1.2 (as is done now) and your suggestion is applied, then the application would not be able to negotiate TLS 1.3 even if both peers have support for it. That would be a shame.

If you really do not want to negotiate TLS 1.3, set MaxVersion: tls.VersionTLS12.

Notes:

• In the past, a separate TLS13CipherSuites field was present. This was removed in #22.
• boringssl and upstream Go crypto/tls have a hard-coded preference list for cipher suites. This makes sense because there are few TLS 1.3 cipher suites and these are all deemed secure. Offering less ways for a developer to shoot themselves in the foot is reasonable. However for testing purposes, it is useful to explicitly override the TLS 1.2/1.3 cipher suite which is why I prefer the current approach over the one in upstream crypto/tls.

[kriskwiatkowski]

I'm not convinced.

[Lekensteyn]

Code (above) is creating an array full of zeros of length equal to len(s13)+len(s) and appends IDs from s13 and s

Good catch, it should have been make([]uint16, 0, len(s)+len(s13)).

It can be assumed that caller has a good reason for not using default set of ciphers. For example, caller may specify not to use AES-128. Because of this bug - tris will allow it and it will allow it only for TLS 1.3. Contract with the caller is violated.

I have a different viewpoint, CipherSuites configures TLS <= 1.2 cipher suites. The fact that it can currently be used to override TLS 1.3 cipher suites is an extension to crypto/tls. Unless there is a good reason to deviate from crypto/tls, I would not do that.

If you do want to disable AES, you can explicitly list the cipher suites that you do want to keep. This should usually not be necessary as the defaults for both TLS 1.2 and 1.3 in recent Go versions are sane (AES-GCM or ChaCha20-Poly1305 with ECDHE). (In other software using OpenSSL, the default cipher list might be worse and changing the cipher suite list is a more common practice. That does not apply here though.)

I'm not convinced.

Why? In my opinion it is unacceptable to fail the handshake when the cipher list is overridden with TLS 1.2-only ciphers (as is shown in the previous configuration example).