Server resonds with incorrect error code when client sends empty list of certificates

[marten-seemann]

When client authentication is requested by setting ClientAuth, the server requests a certificate from the client, but it neither verifies if the client actually sent a certificate (for RequireAnyClientCert and RequireAndVerifyClientCert).

[kriskwiatkowski]

I've to say, I disagree with a statement that "server doesn't verify if client has sent a certificate" in case of TLSv1.3 is used (I'm not sure yet about TLSv1.2). It may not be very clear, but in case client doesn't send certificate, the server will return with an alertHandshakeFailure error. Indeed, returned error is incorrect, but again - it does seem to me that authentication will fail.

In more details, assuming all TLS handshake messages are sent, but Certificate message from client doesn't actually contain any certificate. In case of client authentication on server side:

1. During handshake processing code hits line 13.go:361

   certs := getCertsFromEntries(certMsg.certificates)

2. len(certs) == 0
3. serverHandshakeState.processCertsFromClient is called
4. Function returns with nil,nil

   828:    if len(certs) == 0 {
   829:        return nil, nil
   830:    }

5. verifyPeerHandshakeSignature is called with pubKey = nil
6. pickSignatureAlgorithm is called with pubkey=nil
7. Function returns:

   return 0, 0, 0, errors.New("tls: peer doesn't support any common signature algorithms")

8. alertHandshakeFailure is sent back to the client

[marten-seemann]

Hello @henrydcase,

I should have made this clear in the issue, this issue is kind of a follow-up of #140. Sorry for that. The server is only supposed to read a CertificateVerify message if the client actually sent a certificate. verifyPeerHandshakeSignature is then also only called if the client sent a certificate, i.e. your steps 4-7 wouldn't be executed in that case.

I suggest first merging #140, and then afterwards merging #142. If you prefer, I could also combine these two pull requests into one. Please let me know.

[kriskwiatkowski]

Yep, I'm aware that we will have this issue if #140 is merged but #142 is not. And this would be quite a bug. I would like to avoid having single commit which introduces security issue.

Nevertheless, #140 and #142 should be merged, this is valuable contribution. Could you squash both to one single commit?

[marten-seemann]

Makes sense. #142 it is.