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

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

Closed
robpike opened this issue Jan 17, 2019 · 12 comments
Closed

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

robpike opened this issue Jan 17, 2019 · 12 comments

Comments

@robpike
Copy link
Contributor

@robpike robpike commented Jan 17, 2019

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

@robpike robpike added this to the Go1.12 milestone Jan 17, 2019
@robpike 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
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 17, 2019

/cc @FiloSottile

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

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jan 17, 2019

(*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

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

This comment has been minimized.

Copy link
Member

@dsymonds dsymonds commented Jan 17, 2019

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

@robpike

This comment has been minimized.

Copy link
Contributor Author

@robpike robpike commented Jan 17, 2019

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

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jan 17, 2019

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

This comment has been minimized.

Copy link
Contributor

@mikioh mikioh commented Jan 17, 2019

@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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 17, 2019

@dsymonds, two questions.

  1. The bad certificate in the server config: was it in the tls.Config.Certificates list or was it returned by tls.Config.GetCertificate?
  2. 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

This comment has been minimized.

Copy link
Member

@dsymonds dsymonds commented Jan 17, 2019

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

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jan 17, 2019

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 18, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 18, 2019

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

gopherbot pushed a commit that referenced this issue Jan 18, 2019
…fail

Updates #29779

Change-Id: I9becaba41ab4cd0bac25b4bedf3f8b19761d8158
Reviewed-on: https://go-review.googlesource.com/c/158638
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot gopherbot closed this in 6718bb2 Jan 20, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 5, 2019

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

gopherbot pushed a commit that referenced this issue Feb 7, 2019
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>
nebulabox added a commit to nebulabox/go that referenced this issue Feb 18, 2019
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>
nebulabox added a commit to nebulabox/go that referenced this issue Feb 20, 2019
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>
@golang golang locked and limited conversation to collaborators Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.