crypto/tls: reported "internal errors" are just user errors misdiagnosed #29779

robpike · 2019-01-17T04:15:42Z

What version of Go are you using (`go version`)?

$ go version
go version devel +006a5e7d00 Thu Jan 17 01:28:22 2019 +0000 darwin/amd64

Just spent too much time tracking down a bug in our code that was reported by the servers as "tls: internal error" but was actually not an internal error at all.

The offending line is handshake_server_tls13.go:636:

	sig, err := hs.cert.PrivateKey.(crypto.Signer).Sign(c.config.rand(), h.Sum(nil), signOpts)
	if err != nil {
		c.sendAlert(alertInternalError)
		return errors.New("tls: failed to sign handshake: " + err.Error())
	}

The real error is this:

crypto/rsa: key size too small for PSS signature

It is a 1.3-specific problem; our code worked with TLS 1.2 and we spent a long time trying to figure out what the actual problem was. I ended up hacking crypto/tls to report the actual errors when reporting an internal one just to find out where this was happening (there are about 50 appearances of alertInternalError in the code). Once I found where the problem was, it was easy to fix - just increase our key size - but it was certainly not an internal error, and calling it one made it much harder to figure out our problem.

I think that instead of alerting with the special alert type, it should be alerting with the error. To do this would require minor changes to the alert mechanism so that c.sendAlert accepts the error type rather than the custom alert type.

Given that this particular problem was triggered by a change from 1.2 to 1.3 (I surmise), others might have similar problems. It might be prudent to address this as part of 1.12, which introduces 1.3 as the default.

@dsymonds @rsc

The text was updated successfully, but these errors were encountered:

rsc · 2019-01-17T04:41:40Z

/cc @FiloSottile

(I agree this should be fixed for the release since it is a mysterious new failure caused by enabling 1.3.)

FiloSottile · 2019-01-17T04:59:18Z

(*serverHandshakeStateTLS13).sendServerCertificate returns tls: failed to sign handshake: crypto/rsa: key size too small for PSS signature (since c009433 which was added specifically for this case) to (*serverHandshakeStateTLS13).handshake to (*Conn).serverHandshake() to (*Conn).Handshake() to the application.

It sounds like the error you might have been seeing is the error surfaced on the client side, remote error: tls: internal error. If that's the case, there is nothing we can do, as TLS alerts are not free-form, but specified and numbered

go/src/crypto/tls/alert.go

Lines 17 to 44 in 006a5e7

    
           const ( 
        
           	alertCloseNotify            alert = 0 
        
           	alertUnexpectedMessage      alert = 10 
        
           	alertBadRecordMAC           alert = 20 
        
           	alertDecryptionFailed       alert = 21 
        
           	alertRecordOverflow         alert = 22 
        
           	alertDecompressionFailure   alert = 30 
        
           	alertHandshakeFailure       alert = 40 
        
           	alertBadCertificate         alert = 42 
        
           	alertUnsupportedCertificate alert = 43 
        
           	alertCertificateRevoked     alert = 44 
        
           	alertCertificateExpired     alert = 45 
        
           	alertCertificateUnknown     alert = 46 
        
           	alertIllegalParameter       alert = 47 
        
           	alertUnknownCA              alert = 48 
        
           	alertAccessDenied           alert = 49 
        
           	alertDecodeError            alert = 50 
        
           	alertDecryptError           alert = 51 
        
           	alertProtocolVersion        alert = 70 
        
           	alertInsufficientSecurity   alert = 71 
        
           	alertInternalError          alert = 80 
        
           	alertInappropriateFallback  alert = 86 
        
           	alertUserCanceled           alert = 90 
        
           	alertNoRenegotiation        alert = 100 
        
           	alertMissingExtension       alert = 109 
        
           	alertUnsupportedExtension   alert = 110 
        
           	alertNoApplicationProtocol  alert = 120 
        
           )

and I believe "internal error" is the correct alert to send to the client, as being configured with the wrong key is an internal error of the server from the client point of view. This is why (*Conn).sendAlert takes the custom type instead of error.

By the way, this error is due to the new support of RSA-PSS keys in both TLS 1.2 and 1.3.

dsymonds · 2019-01-17T05:04:03Z

If the server has a bad configuration, why could it even start serving instead of bailing out earlier?

robpike · 2019-01-17T05:13:36Z

I would think "handshake failure" is a much better error in this case. The error you're returning even has the word "handshake".

FiloSottile · 2019-01-17T05:28:55Z

If the server has a bad configuration, why could it even start serving instead of bailing out earlier?

Sadly, tls.Client and tls.Server don't let us surface errors earlier consistently. There are many things I'd like to check before a connection comes in, like #29349, but I could only cheat and do it in tls.Listen and tls.Dial, which are optional helpers.

I would think "handshake failure" is a much better error in this case. The error you're returning even has the word "handshake".

Sure, "handshake failure" is specified to be about a failure to negotiate parameters, but it's not like this case is in the spec anyway, and I don't have a strong opinion. I'll send a CL tomorrow morning.

mikioh · 2019-01-17T05:56:34Z

@FiloSottile,

Not for Go 1.12: it might be better to have a new error type for alert protocol, the same as RecordHeaderError for record layer protocol, to avoid losing protocol-layer information and to be useful for both API users and infrastructure people. IIRC, there are a few neglected issues/CLs like "io.EOF on TLS connection setup does not provide any useful information for troubleshooting."

rsc · 2019-01-17T13:47:37Z

@dsymonds, two questions.

The bad certificate in the server config: was it in the tls.Config.Certificates list or was it returned by tls.Config.GetCertificate?
Were you using tls.Listen?

If tls.Config.Certificates contains any invalid certificates, it seems reasonable to me for tls.Listen to return an error, as it currently does for completely missing certificates. If the bad cert came from tls.Config.GetCertificates, that's more difficult to report well since the server is humming along at that point - there's no obvious way to report the misconfiguration (Filippo's point as I understand it).

If the answers to the questions are "tls.Config.Certificates" and "yes", then I think we should adjust Listen to check for this new failure mode in code. It seems likely to trip others too. (And no matter what else we do, also add it to the release notes.)

Thanks.

dsymonds · 2019-01-17T19:35:09Z

On the server side it is a certificate stuffed in the Certificates field of tls.Config (loaded from a file, sent through tls.X509KeyPair and x509.ParseCertificate).

The actual networking is via net.Listen, then going via grpc-go with grpc.Server and its Serve method, providing the certs with grpc.Creds. So it is possible there's a grpc-go defect instead.

FiloSottile · 2019-01-17T19:53:22Z

I tried a couple approaches at tls.Listen validation, and I don't think we should do it. The obvious advantage is that an error is raised before the connection is opened, so there is no opaque client-side error. The issues are multiple:

it's inconsistent, as it would only work with the rarely used tls.Listen, not with tls.NewListener nor tls.Server nor net/http (and for example wouldn't work for @dsymonds)
it's incomplete, as it would have to be disabled if either GetCertificate or GetConfigForClient is set (and I don't like diverging the behavior of GetConfigForClient)
it depends on client behavior, as a client that does not support RSA-PSS can still connect successfully (using TLS 1.2) so we can't make a universal decision at tls.Listen time
it's extremely late in the cycle for making things that used to work not work (that is, servers with small keys serving old TLS 1.2 clients)

I will change the alert number for 1.12, and add a note to the release notes.

For 1.13 I will make also a small change to the negotiation logic, because as things are right now a (weird, non-Go) client that prefers PSSWithSHA512 (minimum key size 1040 bits) but supports PSSWithSHA256 (minimum key size 528 bits) would fail to connect to a server with a 1024 bit key (still insecure), which is suboptimal. This is #29793.

gopherbot · 2019-01-18T22:36:28Z

Change https://golang.org/cl/158639 mentions this issue: crypto/tls: send a "handshake failure" alert if the RSA key is too small

gopherbot · 2019-01-18T22:36:29Z

Change https://golang.org/cl/158638 mentions this issue: doc/go1.12: mention small RSA keys will cause some TLS handshakes to fail

…fail Updates #29779 Change-Id: I9becaba41ab4cd0bac25b4bedf3f8b19761d8158 Reviewed-on: https://go-review.googlesource.com/c/158638 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

gopherbot · 2019-02-05T21:05:45Z

Change https://golang.org/cl/160998 mentions this issue: crypto/tls: disable RSA-PSS in TLS 1.2

Most of the issues that led to the decision on #30055 were related to incompatibility with or faulty support for RSA-PSS (#29831, #29779, v1.5 signatures). RSA-PSS is required by TLS 1.3, but is also available to be negotiated in TLS 1.2. Altering TLS 1.2 behavior based on GODEBUG=tls13=1 feels surprising, so just disable RSA-PSS entirely in TLS 1.2 until TLS 1.3 is on by default, so breakage happens all at once. Updates #30055 Change-Id: Iee90454a20ded8895e5302e8bcbcd32e4e3031c2 Reviewed-on: https://go-review.googlesource.com/c/160998 Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Adam Langley <agl@golang.org>

Most of the issues that led to the decision on golang#30055 were related to incompatibility with or faulty support for RSA-PSS (golang#29831, golang#29779, v1.5 signatures). RSA-PSS is required by TLS 1.3, but is also available to be negotiated in TLS 1.2. Altering TLS 1.2 behavior based on GODEBUG=tls13=1 feels surprising, so just disable RSA-PSS entirely in TLS 1.2 until TLS 1.3 is on by default, so breakage happens all at once. Updates golang#30055 Change-Id: Iee90454a20ded8895e5302e8bcbcd32e4e3031c2 Reviewed-on: https://go-review.googlesource.com/c/160998 Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Adam Langley <agl@golang.org>

robpike added the release-blocker label Jan 17, 2019

robpike added this to the Go1.12 milestone Jan 17, 2019

robpike changed the title ~~crypto/tls: reported "internal errors" are just user errors diagnosed~~ crypto/tls: reported "internal errors" are just user errors misdiagnosed Jan 17, 2019

FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 17, 2019

This was referenced Jan 17, 2019

crypto/tls: don't negotiate RSA-PSS algorithms that are too big for our key #29793

Closed

crypto/tls: TLS 1.3 ciphers are not configurable #29349

Closed

gopherbot closed this as completed in 6718bb2 Jan 20, 2019

michaeljs1990 mentioned this issue Jan 31, 2020

Go mod support Cloud-Foundations/Dominator#40

Open

golang locked and limited conversation to collaborators Feb 5, 2020

gopherbot added the FrozenDueToAge label Feb 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto/tls: reported "internal errors" are just user errors misdiagnosed #29779

crypto/tls: reported "internal errors" are just user errors misdiagnosed #29779

robpike commented Jan 17, 2019 •

edited

rsc commented Jan 17, 2019

FiloSottile commented Jan 17, 2019

dsymonds commented Jan 17, 2019

robpike commented Jan 17, 2019 •

edited

FiloSottile commented Jan 17, 2019 •

edited

mikioh commented Jan 17, 2019

rsc commented Jan 17, 2019

dsymonds commented Jan 17, 2019

FiloSottile commented Jan 17, 2019 •

edited

gopherbot commented Jan 18, 2019

gopherbot commented Jan 18, 2019

gopherbot commented Feb 5, 2019

crypto/tls: reported "internal errors" are just user errors misdiagnosed #29779

crypto/tls: reported "internal errors" are just user errors misdiagnosed #29779

Comments

robpike commented Jan 17, 2019 • edited

What version of Go are you using (go version)?

rsc commented Jan 17, 2019

FiloSottile commented Jan 17, 2019

dsymonds commented Jan 17, 2019

robpike commented Jan 17, 2019 • edited

FiloSottile commented Jan 17, 2019 • edited

mikioh commented Jan 17, 2019

rsc commented Jan 17, 2019

dsymonds commented Jan 17, 2019

FiloSottile commented Jan 17, 2019 • edited

gopherbot commented Jan 18, 2019

gopherbot commented Jan 18, 2019

gopherbot commented Feb 5, 2019

robpike commented Jan 17, 2019 •

edited

What version of Go are you using (`go version`)?

robpike commented Jan 17, 2019 •

edited

FiloSottile commented Jan 17, 2019 •

edited

FiloSottile commented Jan 17, 2019 •

edited