811909 - Reverse the sense of the ss->getClientAuthData test in ssl3_HandleCertificateRequest

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Patch

Please see the Chromium code review
http://codereview.chromium.org/11359197/
for the context.

I'd like to change the test of ss->getClientAuthData in ssl3_HandleCertificateRequest
from

    if (ss->getClientAuthData == NULL) {
        rv = SECFailure;
    } else {
        rv = (SECStatus)(*ss->getClientAuthData)(...);
    }

to

    if (ss->getClientAuthData != NULL) {
        rv = (SECStatus)(*ss->getClientAuthData)(...);
    } else {
        rv = SECFailure;
    }

This allows us to add the check for an alternative getClientAuthData
hook in a more natural way, like this:

    if (ss->getClientAuthDataV2 != NULL) {
        rv = (SECStatus)(*ss->getClientAuthDataV2)(...);
    } else if (ss->getClientAuthData != NULL) {
        rv = (SECStatus)(*ss->getClientAuthData)(...);
    } else {
        rv = SECFailure;
    }

NOTE: this example uses either-or semantics. It is also possible
to use chaining semantics, where we go on to try ss->getClientAuthData
if ss->getClientAuthDataV2 returns no data, like this:

    rv = SECFailure;

    if (rv == SECFailure && ss->getClientAuthDataV2 != NULL) {
        rv = (SECStatus)(*ss->getClientAuthDataV2)(...);
    }

    if (rv == SECFailure && ss->getClientAuthData != NULL) {
        rv = (SECStatus)(*ss->getClientAuthData)(...);
    }

Comment 1

[Ryan Sleevi]

I believe the alternate example you give (checking for rv==SECFailure) is not equivalent, as presently, SECFailure indicates "don't send a cert". This may in fact be a positive signal from the callback (eg: the caller does not wish to send a cert), so trying another callback would *not* be intended.

I believe the semantics of your patch are correct.

Comment 2

[Wan-Teh Chang]

Thank you for the review. The alternative example is not meant to
be equivalent. It is intended to support multiple disjoint sources
of client certs that are consulted in the order of preference. If
a source of client certs has no suitable client cert, it can tell
NSS to try the next source of client certs. But I just realized
that prevents an application from collecting the client certs from
all sources and ask the user to select one only once. So I now
think it is not a good idea.

Patch checked in on the NSS trunk (NSS 3.14.1).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.195; previous revision: 1.194
done