294428 - CERT_VerifyCertificate does not handle usages correctly

Status: RESOLVED INVALID

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

This bug was reported by Hanfei.
In CERT_VerifyCertificate, there is a local variable declared as follows :

SECCertUsage certUsage = 0;

The value of that variable never changes throughout the function .
It gets used in a loop as follows :

        switch ( certUsage ) {
          case certUsageSSLClient:
          case certUsageSSLServer:
          case certUsageSSLServerWithStepUp:
          case certUsageSSLCA:
          case certUsageEmailSigner:
          case certUsageEmailRecipient:
          case certUsageObjectSigner:
          case certUsageStatusResponder:
            rv = CERT_KeyUsageAndTypeForCertUsage(certUsage, PR_FALSE,
                                                  &requiredKeyUsage,
                                                  &requiredCertType);
            if ( rv != SECSuccess ) {
                PORT_Assert(0);
                /* EXIT_IF_NOT_LOGGING(log); XXX ??? */
                requiredKeyUsage = 0;
                requiredCertType = 0;
                INVALID_USAGE();
            }
            break;

          case certUsageAnyCA:
          case certUsageProtectedObjectSigner:
          case certUsageUserCertImport:
          case certUsageVerifyCA:
              /* these usages cannot be verified */
              NEXT_ITERATION();

          default:
            PORT_Assert(0);
            requiredKeyUsage = 0;
            requiredCertType = 0;
            INVALID_USAGE();
        }
        if ( CERT_CheckKeyUsage(cert, requiredKeyUsage) != SECSuccess ) {
            if (PR_TRUE == requiredUsage) {
                PORT_SetError(SEC_ERROR_INADEQUATE_KEY_USAGE);
            }
            LOG_ERROR(log,cert,0,requiredKeyUsage);
            INVALID_USAGE();
        }
        if ( !( certType & requiredCertType ) ) {
            if (PR_TRUE == requiredUsage) {
                PORT_SetError(SEC_ERROR_INADEQUATE_CERT_TYPE);
            }
            LOG_ERROR(log,cert,0,requiredCertType);
            INVALID_USAGE();
        }

The net effect is that CERT_VerifyCertificate always assumes the usage has a
zero value, which is certUsageSSLClient, and checks the key usage only for that
application usage.

Also, further in the code, this usage is always passed in to
cert_VerifyCertChain . Even though that function is called multiple times - once
for each bit in requiredUsages - it is always passed the same arguments.

The problem also affects trust checking .

That's a rather damning bug that makes the whole function invalid.
Unfortunately, it's been there since I wrote it . See bug
https://bugzilla.mozilla.org/show_bug.cgi?id=149832 and
https://bugzilla.mozilla.org/attachment.cgi?id=90741 .

The fix is to do a conversion from the SECCertificateUsage in the loop, which is
a single usage bit, to a SECCertusage enum.

Comment 1

[Julien Pierre]

Fortunately, this bug is invalid.  The macro "NEXT_ITERATION" handles the
changing of value of certUsage . It wasn't obvious because the macro modifies a
local variable.

#define NEXT_ITERATION() { \
    i*=2; \
    certUsage++; \
    continue; \
}

Thus, I'm clearing the security sensitive bit.

Comment 2

[Julien Pierre]

Attachment: rename NEXT_ITERATION macro to NEXT_USAGE to clarify code

As checked in to the tip .

Checking in certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v	<--  certvfy.c
new revision: 1.44; previous revision: 1.43
done