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: improve invalid client certificate error message #35190

Closed
dpifke opened this issue Oct 26, 2019 · 14 comments
Closed

crypto/tls: improve invalid client certificate error message #35190

dpifke opened this issue Oct 26, 2019 · 14 comments
Milestone

Comments

@dpifke
Copy link

@dpifke dpifke commented Oct 26, 2019

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

go version 1.13.1 linux/amd64

Does this issue reproduce with the latest release?

It should, based on reading the source code.

What did you do?

I have a web server which handles a mix of API and interactive requests. API requests are authenticated using TLS client certificates, so tls.Config.ClientAuth was set to tls.RequestClientCert.

A customer who uses the Brave browser was consistently seeing ERR_SSL_DECRYPT_ERROR_ALERT when trying to connect. This appears to correspond to the server aborting the TLS connection. When this happened, I saw the following message in the server logs:

tls: invalid certificate signature

This error is emitted by crypto/tls/handshake_server_tls13.go:821. Reading that routine, it appears that any error with the client cert causes the connection to be aborted, rather than the certificate being ignored.

Switching to NoClientCert allowed them to connect. Unfortunately, I didn't have a way to capture the client certificate they were sending.

What did you expect to see?

I'd like to be able to treat invalid client certificates as "not present." The documentation for ClientAuthType is sparse, but given these options:

const (
    NoClientCert ClientAuthType = iota
    RequestClientCert
    RequireAnyClientCert
    VerifyClientCertIfGiven
    RequireAndVerifyClientCert
)

... it seemed reasonable to assume that Request != Require or Verify. I thus think this is probably a bug.

If this is actually working as intended, then please consider a feature request for some setting below RequestClientCert which allows the use of client certificates but doesn't abort in the case of a bogus cert.

Expanding the ClientAuthType documentation would be a definite plus as well.

@wade-welles

This comment has been minimized.

Copy link

@wade-welles wade-welles commented Oct 26, 2019

I'm setting up a server now to try run a fuzzer on this package and see if I can uncover anything. I'll report back if I discover anything related to this bug.

@dpifke

This comment has been minimized.

Copy link
Author

@dpifke dpifke commented Oct 27, 2019

FYI, you can reproduce this without fuzzing just by sending a cert that's not valid for some reason (e.g. use SHA1 for the signature algorithm).

I think the question is what the behavior should be when an invalid certificate is received. I'm sure the client certificate in this case was being rejected for a good reason.

@FiloSottile FiloSottile added this to the Go1.14 milestone Oct 27, 2019
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Oct 27, 2019

Hmm. Normally, we try not to be lenient on peers sending invalid data. Why would a client legitimately send a broken certificate? Is there a compatibility case to be made for it? (Like, if we consider invalid certificates that clients have to serve to certain legacy servers, or something like that.)

There's also the fact that if we ignore it, we have no way to report that, so it would make for pretty hard to debug scenarios.

BTW, I feel like if we ignore invalid certificates we should do so also for VerifyClientCertIfGiven.

@dpifke

This comment has been minimized.

Copy link
Author

@dpifke dpifke commented Oct 28, 2019

I wish I knew the story of how this fellow came to install this cert. The problem was explained by him to me as "some websites block Brave" and he had no idea why that was. (I don't think the fact that he was using Brave has anything to do with it.)

I can understand not wanting to follow Postel's Law in the default case, but the alternative is going to be that I have to set up a completely separate HTTPS endpoint for API requests, so that I can accept client certificates without locking out important users. Or patch the stdlib, which is probably less operational overhead than the former.

If we set NoClientAuth, we're already ignoring client certificates. So I fail to see that doing so is somehow dangerous. For reporting warnings, http.Server contains a Logger, maybe tls.Config should as well?

At a minimum, I feel the consequences of setting RequestClientCert (some users will be unable to connect) should be documented somewhere. I would have never figured out the problem from the error message without grepping for it in the source; the words "client certificate" appear in the pre-TLS1.3 error, but not in the newer.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Oct 28, 2019

Is there any reason to believe this will happen again? It's the first time it's reported here, as far as I know.

Also, if a client sends an invalid client certificate, I suspect they would rather learn that there is an issue rather than have it be silently ignored. Most clients don't send certificates, and the ones that do should be presumed to know what they are doing.

BTW, if you are setting ClientCAs, clients should only send certificates that chain to acceptable CAs. It's kind of surprising Brave would send a random certificate.

We should 100% make the error message better, marking this NeedsFix for improving the error message.

@FiloSottile FiloSottile changed the title crypto/tls: RequestClientCert + invalid client certificate = failed connection crypto/tls: improve invalid client certificate error message Oct 28, 2019
@dpifke

This comment has been minimized.

Copy link
Author

@dpifke dpifke commented Oct 28, 2019

I just checked, and Nginx ignores client certificate errors when ssl_verify_client is set to optional. Apache appears to do likewise. Go's web server appears to be the outlier here.

I understand the client may want to know that their certificate isn't being used, but "why isn't my certificate being accepted?" is a very different debugging problem than "why can't I connect?" In this case, I can't tell the user "sorry, your browser is broken," since they're able to use it to connect to most other sites. From their perspective, my web site is "always down."

@dpifke

This comment has been minimized.

Copy link
Author

@dpifke dpifke commented Oct 28, 2019

It occurs to me that maybe Github isn't the best place to debate this; should I cross-post this somewhere for broader input?

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Oct 28, 2019

An important detail here is whether the certificate is completely invalid or not. If not, then we might have a compatibility problem of the shape "I need to send this client certificate for these legacy servers but Go thinks it's invalid", and we might have to accommodate it. Otherwise, why not tell the user to drop the broken client certificate?

(GitHub issues is where we have most technical discussions.)

@dpifke

This comment has been minimized.

Copy link
Author

@dpifke dpifke commented Oct 28, 2019

I don't have access to the original certificate, but let me see if I can come up with a plausible test case. The returned error message indicates verifyHandshakeSignature failed, but doesn't bubble up the error message from that function. That function can fail with a valid signature because an unsupported algorithm was specified.

Telling the user to "drop the broken client certificate" would probably work in cases where one wasn't providing phone support to a very important non-technical customer in a different organization, i.e. if I had any access to their computer. As it stands, they seem unlikely to be convinced that this isn't a vast conspiracy against Brave, since their browser only fails on some small subset of sites.

Re: forum, was just making sure. :)

@dpifke

This comment has been minimized.

Copy link
Author

@dpifke dpifke commented Oct 28, 2019

Mostly for my own edification, I looked up what RFC 8446 has to say about this:

Also, if some aspect of the certificate chain was unacceptable (e.g., it was not signed by a known, trusted CA), the server MAY at its discretion either continue the handshake (considering the client unauthenticated) or abort the handshake.

Any endpoint receiving any certificate which it would need to validate using any signature algorithm using an MD5 hash MUST abort the handshake with a "bad_certificate" alert. SHA-1 is deprecated, and it is RECOMMENDED that any endpoint receiving any certificate which it would need to validate using any signature algorithm using a SHA-1 hash abort the handshake with a "bad_certificate" alert. For clarity, this means that endpoints can accept these algorithms for certificates that are self-signed or are trust anchors.

Thus either Go's current behavior (reject the handshake) or Apache/Nginx/etc.'s (consider the client unauthenticated) is acceptable. Even if it's not the default, I would really like a configuration parameter for the latter. I would be more than happy to submit a patch to this effect, if it has any chance of being accepted.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Oct 29, 2019

Sorry, but the existence of a single client with an unspecified broken configuration is not enough grounds to extend the API surface or make the stack less strict. The cases in which I can see us making a different decision are A) if the client turns out to be sending that certificate for a legitimate (even if incompatible) reason or B) if this proves a much more common issue than we've seen so far. The part of the spec you quoted suggests that even A) is not absolute.

@dpifke

This comment has been minimized.

Copy link
Author

@dpifke dpifke commented Oct 29, 2019

No worries, I'll just fork crypto/tls on my end to mimic the Nginx behavior. Adding RequestClientCertIgnoreInvalid to ClientAuthType turned out to be pretty easy.

Appreciate your looking into this, thanks!

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Oct 29, 2019

As I got into the code to improve the error messages, I realized that this is not an invalid certificate, but an invalid signature by that certificate on the handshake transcript.

This should not be possible if both stacks are behaving correctly (as by then the signature algorithms have been negotiated successfully), so there is a bug either in our stack or in theirs. Maybe even in the Brave or OS code that generates the signatures.

If you can get more details (exact browser version, OS, certificate type, a PCAP with a keylog...) please open a new issue (this one will be fixed by my error message CL) and we should investigate.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 29, 2019

Change https://golang.org/cl/204157 mentions this issue: crypto/tls: improve error messages for invalid certificates and signatures

@gopherbot gopherbot closed this in cd18da4 Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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