946984 - Miscellaneous code cleanups in libpkix for NSS 3.15.5

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Fix a compiler warning and remove redundant argument checks in pkix_pl_OID_Equals

The attached patch is based on a patch by Thomas Inskip and Ryan Sleevi
of Google.

Comment 1

[Wan-Teh Chang]

Attachment: pkix_CrlChecker_CheckExternal and pkix_CrlChecker_CheckLocal should set their pReasonCode output argument

These two functions receive the reason code from |storeCheckRevocationFn| in
the |reasonCode| local variable, but do not copy the reason code to their
|pReasonCode| output argument:

http://mxr.mozilla.org/nss/ident?i=pkix_CrlChecker_CheckExternal
http://mxr.mozilla.org/nss/ident?i=pkix_CrlChecker_CheckLocal

Comment 2

[Wan-Teh Chang]

Attachment: Callers of pkix_CheckChain should check reasonCode only if pkix_CheckChain fails

This patch is harder to understand, so I added assertions to help
validate my understanding of the code.

The current callers of pkix_CheckChain all use a nonzero reasonCode
as a sign of error, after checking whether pkix_CheckChain fails:
http://mxr.mozilla.org/nss/ident?i=pkix_CheckChain

I believe this is wrong. A reasonCode of 0 means the reason for
revocation is unspecified:

   CRLReason ::= ENUMERATED {
        unspecified             (0),
        keyCompromise           (1),
        cACompromise            (2),
        affiliationChanged      (3),
        superseded              (4),
        cessationOfOperation    (5),
        certificateHold         (6),
             -- value 7 is not used
        removeFromCRL           (8),
        privilegeWithdrawn      (9),
        aACompromise           (10) }

So, the reasonCode should only be inspected when we know the
certificate is revoked.

It is possible that the reasonCode != 0 check is defensive
programming. I assert that in the current NSS code, reasonCode
is always 0 if pkix_CheckChain does not fail, for two reasons:

1. The CRL checkers fail to set their pReasonCode output argument
(see my second patch). So the CRL checkers actually leave
reasonCode unchanged. Note that reasonCode is initialized to 0.

With my second patch, the CRL checkers will receive the reason
code from pkix_pl_Pk11CertStore_CheckRevByCrl, and you can inspect
it and cert_CheckCertRevocationStatus to see that pReasonCode can
only be set to a nonzero value if cert_CheckCertRevocationStatus
returns SECFailure.

2. The OCSP checkers always set their pReasonCode to 0
(crlEntryReasonUnspecified) because OCSP responses do not include
the reason code:
http://mxr.mozilla.org/nss/ident?i=pkix_OcspChecker_CheckExternal
http://mxr.mozilla.org/nss/ident?i=pkix_OcspChecker_CheckLocal

Comment 3

[Ryan Sleevi]

Comment on attachment 8343401 [details] [diff] [review]
Callers of pkix_CheckChain should check reasonCode only if pkix_CheckChain fails

Review of attachment 8343401 [details] [diff] [review]:
-----------------------------------------------------------------

r+ ryan.sleevi

Comment 4

[Ryan Sleevi]

Comment on attachment 8343385 [details] [diff] [review]
pkix_CrlChecker_CheckExternal and pkix_CrlChecker_CheckLocal should set their pReasonCode output argument

Review of attachment 8343385 [details] [diff] [review]:
-----------------------------------------------------------------

r+ ryan.sleevi

Comment 5

[Ryan Sleevi]

Comment on attachment 8343379 [details] [diff] [review]
Fix a compiler warning and remove redundant argument checks in pkix_pl_OID_Equals

Review of attachment 8343379 [details] [diff] [review]:
-----------------------------------------------------------------

r+ ryan.sleevi

Comment 6

[Wan-Teh Chang]

Comment on attachment 8343379 [details] [diff] [review]
Fix a compiler warning and remove redundant argument checks in pkix_pl_OID_Equals

Patch checked in: https://hg.mozilla.org/projects/nss/rev/6bcfca6b2a9a

Comment 7

[Wan-Teh Chang]

Comment on attachment 8343385 [details] [diff] [review]
pkix_CrlChecker_CheckExternal and pkix_CrlChecker_CheckLocal should set their pReasonCode output argument

Patch checked in: https://hg.mozilla.org/projects/nss/rev/4c81bf12335f

Comment 8

[Wan-Teh Chang]

Comment on attachment 8343401 [details] [diff] [review]
Callers of pkix_CheckChain should check reasonCode only if pkix_CheckChain fails

Patch checked in: https://hg.mozilla.org/projects/nss/rev/0e4e58e97971