Remove CERTCertificate uses from mozilla::pkix revocation checking API

RESOLVED FIXED in mozilla33

Status

()

Core
Security: PSM
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

unspecified
mozilla33
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Comment hidden (empty)
Summary: Remove CERTCertificate uses from mozilla::pkix OCSP API → Remove CERTCertificate uses from mozilla::pkix revocation checking API
Created attachment 8441020 [details] [diff] [review]
Part 1: Remove CERTCertificate from the mozilla::pkix revocation checking API and all users
Attachment #8441020 - Flags: review?(dkeeler)
Created attachment 8441021 [details] [diff] [review]
Part 2: Remove CERTCertificate usage from OCSPCache.cpp
Attachment #8441021 - Flags: review?(dkeeler)
Comment on attachment 8441021 [details] [diff] [review]
Part 2: Remove CERTCertificate usage from OCSPCache.cpp

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

::: security/certverifier/OCSPCache.cpp
@@ +143,2 @@
>  {
> +  PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, (aMessage, &aCertID));

I admit that this is not as readable/useful as before, but I'm not sure what else is reasonable to do. We do things this way in Necko and PSM and it usually turns out OK when we're actually debugging an issue. Let me know if you have a suggestion for a better way of doing things.
Created attachment 8441022 [details] [diff] [review]
Part 3: Update pkixtestutil.cpp for removal of CERTCertificate from revocation checking API
Attachment #8441022 - Flags: review?(dkeeler)
Created attachment 8441023 [details] [diff] [review]
Part 4: Remove CERTCertificate from OCSPCacheTest.cpp
Attachment #8441023 - Flags: review?(dkeeler)
Comment on attachment 8441023 [details] [diff] [review]
Part 4: Remove CERTCertificate from OCSPCacheTest.cpp

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

::: security/manager/ssl/tests/gtest/OCSPCacheTest.cpp
@@ +259,3 @@
>    ASSERT_TRUE(errorOut == 0 && timeOut == timeIn);
> +  // Test that we don't match a different issuer DN
> +  ASSERT_FALSE(cache.Get(CertID(fakeIssuer2, fakeKey000, fakeSerial001),

I'm not sure what this test is actually testing, so I took a guess, which is explained in the two comments I added here.
Try build:
https://tbpl.mozilla.org/?tree=Try&rev=bab5644aca14

I had to add some #includes and "using namespace" declarations to get these patches to pass tryserver.
Just letting you know: it's going to take me a few days to review these.
Comment on attachment 8441021 [details] [diff] [review]
Part 2: Remove CERTCertificate usage from OCSPCache.cpp

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

::: security/certverifier/OCSPCache.cpp
@@ +143,2 @@
>  {
> +  PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, (aMessage, &aCertID));

I guess this is fine.
Attachment #8441021 - Flags: review?(dkeeler) → review+
Comment on attachment 8441022 [details] [diff] [review]
Part 3: Update pkixtestutil.cpp for removal of CERTCertificate from revocation checking API

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

r- for now since I'm a little confused as to what's going on with OCSPKeyHash and KeyHashHelper - see comments. Everything else looks good.

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +42,5 @@
> +namespace mozilla { namespace pkix {
> +
> +// defined in pkixocsp.cpp
> +Result OCSPKeyHash(const SECItem& subjectPublicKeyInfo,
> +                   /*out*/ uint8_t *hashBuf, size_t hashBufSize);

nit: I prefer aligning the types like so:

Result OCSPKeyHash(const SECItem& subjectPublicKeyInfo,
           /*out*/ uint8_t *hashBuf, size_t hashBufSize);

I see we're not very consistent about which way we do this, though, so I guess either is fine.

@@ +266,5 @@
>    return EncodeNested(arena, der::OCTET_STRING, hashBuf);
>  }
>  
>  static SECItem*
>  KeyHashHelper(PLArenaPool* arena, const CERTSubjectPublicKeyInfo* spki)

Hmmm - we're probably going to eventually want to remove this usage of CERTSubjectPublicKeyInfo as well, right?

@@ +1272,5 @@
> +
> +  SECItem* issuerKeyHash;
> +  {
> +    uint8_t hash[SHA1_LENGTH];
> +    // XXX: Don't use OCSPKeyHash here; we're using the function we're testing

Er, yeah, this seems bad. Can we avoid this? Why can't we just use/modify KeyHashHelper? (I guess they're basically the same function.) In fact, I'm a little confused as to why we're using OCSPKeyHash here while we still have and use KeyHashHelper elsewhere. Do we need both?
Attachment #8441022 - Flags: review?(dkeeler) → review-
Created attachment 8442529 [details] [diff] [review]
Part 3: Update pkixtestutil.cpp for removal of CERTCertificate from revocation checking API [v2]

Thanks for the review, David!

Regarding removing the dependencies on CERTSubjectPublicKeyInfo from the *tests*: That would be nice, but it is outside the scope of the project I'm currently working on. First let's remove the CERTCertificate badness from the mozilla::pkix code that is actually used in Gecko, and then we can figure out what to do about the tests. In fact, to resolve your concern with my patch, I ended up reverting back to using KeyHashHelper, which makes a lot more sense. I'm not sure what I was thinking when I wrote the original version of the patch!
Attachment #8441022 - Attachment is obsolete: true
Attachment #8442529 - Flags: review?(dkeeler)
Attachment #8441023 - Attachment description: remove-OCSP-CERTCertificate-p4.patch → Part 4: Remove CERTCertificate from OCSPCacheTest.cpp
Comment on attachment 8441023 [details] [diff] [review]
Part 4: Remove CERTCertificate from OCSPCacheTest.cpp

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

Ok - this looks correct.
Attachment #8441023 - Flags: review?(dkeeler) → review+
Comment on attachment 8442529 [details] [diff] [review]
Part 3: Update pkixtestutil.cpp for removal of CERTCertificate from revocation checking API [v2]

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

Cool - looks good.
Attachment #8442529 - Flags: review?(dkeeler) → review+
Created attachment 8443618 [details] [diff] [review]
Part 1: Remove CERTCertificate from the mozilla::pkix revocation checking API and all users [v2]

Here's Part 1 rebased on top of changes that landed today. I didn't mark the previous version of the Part 1 patch obsolete in case you had stored review comments in it.
Attachment #8443618 - Flags: review?(dkeeler)
Comment on attachment 8441020 [details] [diff] [review]
Part 1: Remove CERTCertificate from the mozilla::pkix revocation checking API and all users

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

Looks great - no major concerns. I'll try interdiff now...

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +180,5 @@
> +// with url == nullptr when an OCSP URI was not found, and SECSuccess with
> +// url != nullptr when an OCSP URI was found. The output url will be owned
> +// by the arena.
> +static SECStatus
> +GetOCSPAuthorityInfoAccessLocation(PLArenaPool* arena,

This looks good, but it's something it would be good to test. Since we're replacing one untested implementation with another that's based on it, I don't think we need to block on that, but it would be good to file a bug and throw it on the testing backlog pile.

@@ +182,5 @@
> +// by the arena.
> +static SECStatus
> +GetOCSPAuthorityInfoAccessLocation(PLArenaPool* arena,
> +                                   const SECItem& aiaExtension,
> +                                   /*out*/ char const*& url)

nit: I prefer to align to the type names instead of comments, but we don't have a consistent style for this.

@@ +373,5 @@
> +  if (!arena) {
> +    return SECFailure;
> +  }
> +
> +  const char* url = nullptr;

Comment that the memory for url is owned by the arena?

::: security/pkix/include/pkix/pkixtypes.h
@@ +76,5 @@
>    ActivelyDistrusted = 2, // certificate is known to be bad
>    InheritsTrust = 3       // certificate must chain to a trust anchor
>  };
>  
> +// CertID references the information needed to do revocation checking for the

If I understand correctly, the lifetime of the memory being referenced must outlive that of the CertID object. Do we want to explicitly mention this here?

@@ +82,5 @@
> +//
> +// issuer must be the DER-encoded issuer field from the certificate for which
> +// revocation checking is being done, **NOT** the subject field of the issuer
> +// certificate. (Those two fields must be equal to each other, but they may not
> +// be encoded exactly the same, and the encoding matters for OCSP.)

Documentation for issuerSubjectPublicKeyInfo and serialNumber? I'm assuming they're both the DER encoding as they appear in the certificate.

::: security/pkix/lib/pkixkey.cpp
@@ +27,2 @@
>  #include <limits>
>  #include <stdint.h>

nit: these #includes seem like they're in a strange order to me. Shouldn't limits and stdint.h go first separated by a blank line, and then the rest, alphabetized?

::: security/pkix/lib/pkixocsp.cpp
@@ +51,5 @@
>  {
>  public:
> +  Context(TrustDomain& trustDomain, const CertID& certID, PRTime time,
> +          uint16_t maxLifetimeInDays, /*optional*/ PRTime* thisUpdate,
> +          /*optional*/ PRTime* validThrough)

nit: /*optional out*/ for these two times?

@@ +183,5 @@
>  static Result MatchKeyHash(const SECItem& issuerKeyHash,
>                             const SECItem& issuerSubjectPublicKeyInfo,
>                             /*out*/ bool& match);
> +Result OCSPKeyHash(const SECItem& subjectPublicKeyInfo,
> +                   /*out*/ uint8_t *hashBuf, size_t hashBufSize);

nit: uint8_t* hashBuf

@@ +764,4 @@
>  
> +// TODO(bug 966856): support SHA-2 hashes
> +Result
> +OCSPKeyHash(const SECItem& subjectPublicKeyInfo, /*out*/ uint8_t *hashBuf,

We're always going to know the output size, right? Can this be either templateized or given a reference to an array of a fixed size?
Also, nit: uint8_t* hashBuf
Attachment #8441020 - Flags: review?(dkeeler) → review+
Comment on attachment 8443618 [details] [diff] [review]
Part 1: Remove CERTCertificate from the mozilla::pkix revocation checking API and all users [v2]

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

Well, interdiff wasn't working, but vimdiff worked well enough. Looks like you rebased this just fine - r=me with comments from the review of the previous version addressed.
Attachment #8443618 - Flags: review?(dkeeler) → review+
Comment on attachment 8441020 [details] [diff] [review]
Part 1: Remove CERTCertificate from the mozilla::pkix revocation checking API and all users

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

Thanks for the review!

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +180,5 @@
> +// with url == nullptr when an OCSP URI was not found, and SECSuccess with
> +// url != nullptr when an OCSP URI was found. The output url will be owned
> +// by the arena.
> +static SECStatus
> +GetOCSPAuthorityInfoAccessLocation(PLArenaPool* arena,

bug 1028368.

@@ +182,5 @@
> +// by the arena.
> +static SECStatus
> +GetOCSPAuthorityInfoAccessLocation(PLArenaPool* arena,
> +                                   const SECItem& aiaExtension,
> +                                   /*out*/ char const*& url)

Let's start an email thread about what to do about this in general, and then change all the code at once to be consistent.

@@ +373,5 @@
> +  if (!arena) {
> +    return SECFailure;
> +  }
> +
> +  const char* url = nullptr;

done.

::: security/pkix/include/pkix/pkixtypes.h
@@ +76,5 @@
>    ActivelyDistrusted = 2, // certificate is known to be bad
>    InheritsTrust = 3       // certificate must chain to a trust anchor
>  };
>  
> +// CertID references the information needed to do revocation checking for the

It can't reasonably be any other way and be correct, can it? Maybe we should add some comments about being careful about lifetimes somewhere, but I'm not sure where. Like I said on dev-platform, I'm looking forward to some tools that can help us document and enforce these invariants in an automated way.

@@ +82,5 @@
> +//
> +// issuer must be the DER-encoded issuer field from the certificate for which
> +// revocation checking is being done, **NOT** the subject field of the issuer
> +// certificate. (Those two fields must be equal to each other, but they may not
> +// be encoded exactly the same, and the encoding matters for OCSP.)

I added:

// issuerSubjectPublicKeyInfo is the entire DER-encoded subjectPublicKeyInfo
// field from the issuer's certificate. serialNumber is the entire DER-encoded
// serial number from the subject certificate (the certificate for which we are
// checking the revocation status).

::: security/pkix/lib/pkixkey.cpp
@@ +27,2 @@
>  #include <limits>
>  #include <stdint.h>

I changed this to be in the order you asked for.

::: security/pkix/lib/pkixocsp.cpp
@@ +51,5 @@
>  {
>  public:
> +  Context(TrustDomain& trustDomain, const CertID& certID, PRTime time,
> +          uint16_t maxLifetimeInDays, /*optional*/ PRTime* thisUpdate,
> +          /*optional*/ PRTime* validThrough)

done.

@@ +183,5 @@
>  static Result MatchKeyHash(const SECItem& issuerKeyHash,
>                             const SECItem& issuerSubjectPublicKeyInfo,
>                             /*out*/ bool& match);
> +Result OCSPKeyHash(const SECItem& subjectPublicKeyInfo,
> +                   /*out*/ uint8_t *hashBuf, size_t hashBufSize);

Fixed.

@@ +764,4 @@
>  
> +// TODO(bug 966856): support SHA-2 hashes
> +Result
> +OCSPKeyHash(const SECItem& subjectPublicKeyInfo, /*out*/ uint8_t *hashBuf,

I fixed the whitespace issue.

We can't use the "reference to an array of fixed size" because of the call to this function from CreateEncodedOCSPRequest.

I don't think we should make hashBufSize a template parameter, because that would just make this bloat up once we fix bug 966856.
Thanks for the reviews! I folded all the patches together before landing since they don't build individually:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d54fd14fb9c
https://hg.mozilla.org/mozilla-central/rev/3d54fd14fb9c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.