1341229 - tstclnt loses error code when authentication of server cert fails

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[Daiki Ueno [:ueno]]

Attachment: tstclnt-restore-error.patch

If certificate authentication fails with tstclnt and if a non-AEAD ciphersuite is used, tstclnt doesn't print the actual error code, but "error 0: Success":

$ tstclnt -d certs/ -4 -h localhost -p 4433 -V tls1:
tstclnt: authentication of server cert failed: SSL_ERROR_BAD_CERT_DOMAIN: Unable to communicate securely with peer: requested domain name does not match the server's certificate.

$ tstclnt -d certs/ -4 -h localhost -p 4433 -V tls1: -c :002F
tstclnt: authentication of server cert failed: error 0: Success

This is because SSL_AuthCertificateComplete could reset the error code previously set by PORT_SetError, when it sends an alert.  The application should record the error code and restore it after the call.

Comment 1

[Kai Engert [:KaiE:]]

I was asking myself, should NSS itself reset the original error code in SSL_AuthCertificateComplete? No, I think that at the time SSL_AuthCertificateComplete returns with a failure, the error code should report that failure.

I agree that the application is responsible to remember the original failure, and if necessary, use the original failure after SSL_AuthCertificateComplete returns.

So, I think the patch correctly fixes the original problem.

However, I think the patch could be enhanced.

The following could happen:
- SSL_AuthCertificate was successful, error == 0
- SSL_AuthCertificateComplete fails and sets an error code
- the call to PORT_SetError(0) hides the error code from SSL_AuthCertificateComplete

If you agree, then I suggest the following logic:

    if (SSL_AuthCertificateComplete(fd, error) != SECSuccess) {
        rv = SECFailure;
    } else {
        /* restore the original error code, which could be reset by
         * SSL_AuthCertificateComplete */
        PORT_SetError(error);
    }

Alternatively, it might be easier to read:

    rv = SSL_AuthCertificateComplete(fd, error);
    if (rv == SECSuccess) {
        /* restore the original error code, which could be reset by
         * SSL_AuthCertificateComplete */
        PORT_SetError(error);
    }

Comment 2

[Kai Engert [:KaiE:]]

Comment on attachment 8839399 [details] [diff] [review]
tstclnt-restore-error.patch

r=kaie because the fix is correct, but I'd like to see an improvement, if my comments are correct.

Daiki, please confirm if the suggestion makes sense, then I can check it in.

Comment 3

[Daiki Ueno [:ueno]]

Attachment: tstclnt-restore-error-v2.patch

Thank you for the review.  I have updated the patch along those lines.

Comment 4

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/806c3106536f