Miscellaneous code cleanups in libpkix for NSS 3.15.5

RESOLVED FIXED in 3.15.5

Status

NSS
Libraries
P2
minor
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
Created attachment 8343379 [details] [diff] [review]
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.
Attachment #8343379 - Flags: review?(ryan.sleevi)
(Assignee)

Comment 1

5 years ago
Created attachment 8343385 [details] [diff] [review]
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
Attachment #8343385 - Flags: review?(ryan.sleevi)
(Assignee)

Comment 2

5 years ago
Created attachment 8343401 [details] [diff] [review]
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
Attachment #8343401 - Flags: review?(ryan.sleevi)

Comment 3

5 years ago
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
Attachment #8343401 - Flags: review?(ryan.sleevi) → review+

Comment 4

5 years ago
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
Attachment #8343385 - Flags: review?(ryan.sleevi) → review+

Comment 5

5 years ago
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
Attachment #8343379 - Flags: review?(ryan.sleevi) → review+
(Assignee)

Comment 6

5 years ago
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
Attachment #8343379 - Flags: checked-in+
(Assignee)

Comment 7

5 years ago
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
Attachment #8343385 - Flags: checked-in+
(Assignee)

Comment 8

5 years ago
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
Attachment #8343401 - Flags: checked-in+
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Summary: Miscellaneous code cleanups in libpkix for NSS 3.15.4 → Miscellaneous code cleanups in libpkix for NSS 3.15.5
Target Milestone: 3.15.4 → 3.15.5

Updated

4 years ago
Target Milestone: 3.15.5 → 3.16
(Assignee)

Updated

4 years ago
Target Milestone: 3.16 → 3.15.5
You need to log in before you can comment on or make changes to this bug.