Server resonds with incorrect error code when client sends empty list of certificates
Closed this issue · 4 comments
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
).
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:
- During handshake processing code hits line 13.go:361
certs := getCertsFromEntries(certMsg.certificates)
len(certs) == 0
serverHandshakeState.processCertsFromClient
is called- Function returns with
nil,nil
828: if len(certs) == 0 {
829: return nil, nil
830: }
verifyPeerHandshakeSignature
is called withpubKey = nil
pickSignatureAlgorithm
is called withpubkey=nil
- Function returns:
return 0, 0, 0, errors.New("tls: peer doesn't support any common signature algorithms")
alertHandshakeFailure
is sent back to the client
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.
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?
Makes sense. #142 it is.