Closed Bug 1341229 Opened 4 years ago Closed 4 years ago

tstclnt loses error code when authentication of server cert fails

Categories

(NSS :: Tools, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ueno, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch tstclnt-restore-error.patch (obsolete) — Splinter Review
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.
Attachment #8839399 - Flags: review?(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 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.
Attachment #8839399 - Flags: review?(kaie) → review+
Thank you for the review.  I have updated the patch along those lines.
Attachment #8839399 - Attachment is obsolete: true
Attachment #8840353 - Flags: review+
https://hg.mozilla.org/projects/nss/rev/806c3106536f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.30
You need to log in before you can comment on or make changes to this bug.