1029247 - Use mozilla::pkix::der to parse certificates instead of extracting the data of of a CERTCertificate

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Add a new overload of mozilla::pkix::bind

This overload is needed so that we can use mozilla::pkix::OptionalExtensions using a member function like this:

der::Result
BackCert::RememberExtension(der::Input& extnID, const SECItem& extnValue,
                            /*out*/ bool& understood);

I also tested the code using std::bind (MOZILLA_PKIX_USE_REAL_FUNCTIONAL) to make sure std::bind works the same way.

Comment 2

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8448879 [details] [diff] [review]
Part 1: Add a new overload of mozilla::pkix::bind

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

LGTM.

::: security/pkix/include/pkix/bind.h
@@ +173,5 @@
>    return internal::Bind4<R, P1, const B1, const B2, B3, B4>(f, b1, b2, b3, b4);
>  }
>  
> +template <typename R, typename C1, typename P1, typename P2, typename P3>
> +inline internal::BindThat3<R, C1, P1, P2, P3>

Maybe BindToMemberFunction3? (I realize it's verbose...)

::: security/pkix/lib/pkixbind.cpp
@@ +25,1 @@
>  

nit: now there are two blank lines in a row here

@@ +25,4 @@
>  
>  #include "pkix/bind.h"
>  
> +#ifndef MOZILLA_PKIX_USE_REAL_FUNCTIONAL

I'm not sure why this needed to move, but it's probably fine either way.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Add a new overload of mozilla::pkix::bind

I made your suggested changes.

I changed the location of the "#ifndef MOZILLA_PKIX_USE_REAL_FUNCTIONAL" in pkixbind.cpp so that adding "#define MOZILLA_PKIX_USE_REAL_FUNCTIONAL" to the top of pkix/bind.h would result in the correct thing happening. (Defining MOZILLA_PKIX_USE_REAL_FUNCTIONAL is how we check that std::bind accepts the same stuff that our hacky polyfill accepts.)

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: use mozilla::pkix::der to parse certificate

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3: Update CheckValidity, renaming it from CheckTimes

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 4: Update time and validity tests

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 5: Update test_cert_versions.js

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 6: Update everything else

Note to Reviewers: Parts 2-6 will not build independently. I split them up only to make reviewing easier. I will fold them all together before pushing them.

Here is the try run:
https://tbpl.mozilla.org/?tree=Try&rev=3e2a2db08037
The Linux 64 opt (Hf) failure is due to another, unrelated, bug that was on inbound when I rebased my patches; just ignore it. I think the cycle collector transient issue is related to the same problem and can also just be ignored.

Comment 9

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8449172 [details] [diff] [review]
Part 2: use mozilla::pkix::der to parse certificate

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

Great - r=me given how non-critical, not-understood duplicate extensions are handled is ok (see comment).

::: security/pkix/lib/pkixcert.cpp
@@ +37,5 @@
> +
> +  der::Input tbsCertificate;
> +
> +  // The scope of |input| and |certificate| are limited to this block so we
> +  // don't accidentally confuse then for tbsCertificate later.

nit: "them"

@@ +117,5 @@
> +                              subjectPublicKeyInfo) != der::Success) {
> +    return MapSECStatus(SECFailure);
> +  }
> +
> +  static const uint8_t CCS = der::CONTEXT_SPECIFIC | der::CONSTRUCTED;

nit: As written, I would expect this to be called CSC (Context Specific, Constructed)

@@ +201,5 @@
> +  SECItem* out = nullptr;
> +
> +  SECItem dummySubjectAltName = { siBuffer, nullptr, 0 }; // bug 970542
> +
> +  // WE already enforce the maximum possible constraints for policies so we

nit: "We"

@@ +203,5 @@
> +  SECItem dummySubjectAltName = { siBuffer, nullptr, 0 }; // bug 970542
> +
> +  // WE already enforce the maximum possible constraints for policies so we
> +  // can safely ignore even critical policy constraint extensions.
> +  SECItem dummyPolicyConstraints = { siBuffer, nullptr, 0};

nit: "0 };"

@@ +231,5 @@
> +  }
> +
> +  if (out) {
> +    // Don't allow an empty value for any extension we understand. This way, we
> +    // can test out->len to check for duplicates.

I don't think this will catch duplicate PolicyConstraints or SubjectAltName extensions (i.e. any of the "dummy" extensions declared in this function). Do we care?
In fact, this won't catch any duplicate non-critical extensions that we don't parse. If they're not critical and we don't handle them anyway, it doesn't seem like that would matter, though.
Maybe we should just document this aspect of how we handle duplicates here.

::: security/pkix/lib/pkixutil.h
@@ +94,5 @@
>    // the subject CN as a possible dNSName. IncludeCN::Yes means that name
>    // constraint enforcement will consider the subject CN as a possible dNSName.
>    MOZILLA_PKIX_ENUM_CLASS IncludeCN { No = 0, Yes = 1 };
>  
>    // nssCert and childCert must be valid for the lifetime of BackCert

nit: looks like this comment has already needed updating

@@ +155,5 @@
> +  // empty extensions and extensions that weren't included. However, when
> +  // *processing* extensions, we distinguish between whether an extension was
> +  // included or not based on whetehr the GetXXX function for the extension
> +  // returns nullptr.
> +  static inline const SECItem* MaybeSECItem(const SECItem& item) 

nit: trailing space

@@ +163,5 @@
>  
> +  // Helper classes to zero-initialize these fields on construction and to
> +  // document that they contain non-owning pointers to the data they point
> +  // to.
> +  struct NonOwningSECItem : public SECItemStr {

Any interest in implementing this as a generalized template type?

Comment 10

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8449178 [details] [diff] [review]
Part 6: Update everything else

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

Looks good. r=me with nits addressed.

::: security/pkix/lib/pkixbuild.cpp
@@ +92,5 @@
>    }
>  
> +  SECStatus srv = trustDomain.VerifySignedData(
> +                                  subject.GetSignedData(),
> +                                  potentialIssuer.GetSubjectPublicKeyInfo());

I would probably indent these two spaces from the start of "trustDomain" or two spaces from the start of "VerifySignedData", but it's not a big deal.

@@ +208,5 @@
>  
>        CertID certID(subject.GetIssuer(), n->cert->derPublicKey,
>                      subject.GetSerialNumber());
> +      SECStatus
> +        srv = trustDomain.CheckRevocation(endEntityOrCA, certID, time,

I'm not sure this line break is necessary (the arguments can go below the call like on line 94).

::: security/pkix/lib/pkixocsp.cpp
@@ +156,5 @@
>    }
>  
>    // TODO(bug 926260): check name constraints
> +
> +  if (trustDomain.VerifySignedData(potentialSigner.GetSignedData(),

Probably better to do:

SECStatus srv = trustDomain.Verify...
if (srv != SECSuccess) {
  return MapSECStatus(srv);
}

return Success;

(like pkixbuild.cpp:94)

Comment 11

[Camilo Viecco (:cviecco)]

Comment on attachment 8449173 [details] [diff] [review]
Part 3: Update CheckValidity, renaming it from CheckTimes

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

With the rename an r+ (or an comment here why keeping NSS lousy name convention is a good idea)

::: security/pkix/lib/pkixutil.h
@@ +109,5 @@
>    const SECItem& GetDER() const { return der; }
>    const der::Version GetVersion() const { return version; }
>    const CERTSignedData& GetSignedData() const { return signedData; }
>    const SECItem& GetIssuer() const { return issuer; }
> +  const SECItem& GetValidity() const { return validity; }

I would rename this to getTimeValidity (validity is not really descriptive)

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Camilo Viecco (:cviecco) from comment #11)
> With the rename an r+ (or an comment here why keeping NSS lousy name
> convention is a good idea)
> 
> > +  const SECItem& GetValidity() const { return validity; }
> 
> I would rename this to getTimeValidity (validity is not really descriptive)

I will check it in with this comment:

// XXX: "validity" is a horrible name for the structure that holds
// notBefore/notAfter, but that is the name used in RFC 5280 and
// we use the RFC 5280 names everywhere else.

Basically, I agree with you, but I don't think it makes sense for us to invent our own names for stuff when there are standard names. I wish the standard names were better. (The inconsistent capitalization and inconsistent use of hyphens and prefixes in OID names also sucks.)

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8449172 [details] [diff] [review]
Part 2: use mozilla::pkix::der to parse certificate

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

::: security/pkix/lib/pkixcert.cpp
@@ +37,5 @@
> +
> +  der::Input tbsCertificate;
> +
> +  // The scope of |input| and |certificate| are limited to this block so we
> +  // don't accidentally confuse then for tbsCertificate later.

Fixed.

@@ +117,5 @@
> +                              subjectPublicKeyInfo) != der::Success) {
> +    return MapSECStatus(SECFailure);
> +  }
> +
> +  static const uint8_t CCS = der::CONTEXT_SPECIFIC | der::CONSTRUCTED;

I agree. I looked at the existing code and saw that it was inconsistent: sometimes we use (der::CONTEXT_SPECIFIC | der::CONSTRUCTED | x) and sometimes we use (der::CONSTRUCTED | der::CONSTRUCTED | x). In pkixocsp.cpp, I found a reason for putting der::CONTEXT_SPECIFIC first: it is possible to have a CHOICE where all the choices are CONTEXT_SPECIFIC but not all of them are CONSTRUCTED. So, I changed all the code to use the order (der::CONTEXT_SPECIFIC | der::CONSTRUCTED) and I also renamed all "CCS" variables to "CSC" as you suggested.

@@ +201,5 @@
> +  SECItem* out = nullptr;
> +
> +  SECItem dummySubjectAltName = { siBuffer, nullptr, 0 }; // bug 970542
> +
> +  // WE already enforce the maximum possible constraints for policies so we

Fixed.

@@ +203,5 @@
> +  SECItem dummySubjectAltName = { siBuffer, nullptr, 0 }; // bug 970542
> +
> +  // WE already enforce the maximum possible constraints for policies so we
> +  // can safely ignore even critical policy constraint extensions.
> +  SECItem dummyPolicyConstraints = { siBuffer, nullptr, 0};

Fixed.

@@ +231,5 @@
> +  }
> +
> +  if (out) {
> +    // Don't allow an empty value for any extension we understand. This way, we
> +    // can test out->len to check for duplicates.

I see. You mean that, because the dummy variables are local variables, and this function is called once for each extension, we'll forget any changes we made to the dummy variables. Good catch. I will upload a new version of the patch that fixes this.

::: security/pkix/lib/pkixutil.h
@@ +94,5 @@
>    // the subject CN as a possible dNSName. IncludeCN::Yes means that name
>    // constraint enforcement will consider the subject CN as a possible dNSName.
>    MOZILLA_PKIX_ENUM_CLASS IncludeCN { No = 0, Yes = 1 };
>  
>    // nssCert and childCert must be valid for the lifetime of BackCert

Fixed.

@@ +155,5 @@
> +  // empty extensions and extensions that weren't included. However, when
> +  // *processing* extensions, we distinguish between whether an extension was
> +  // included or not based on whetehr the GetXXX function for the extension
> +  // returns nullptr.
> +  static inline const SECItem* MaybeSECItem(const SECItem& item) 

Fixed.

@@ +163,5 @@
>  
> +  // Helper classes to zero-initialize these fields on construction and to
> +  // document that they contain non-owning pointers to the data they point
> +  // to.
> +  struct NonOwningSECItem : public SECItemStr {

Yes, we can do that if you think it would be helpful. Let's do it in another bug.

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8449178 [details] [diff] [review]
Part 6: Update everything else

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

::: security/pkix/lib/pkixbuild.cpp
@@ +92,5 @@
>    }
>  
> +  SECStatus srv = trustDomain.VerifySignedData(
> +                                  subject.GetSignedData(),
> +                                  potentialIssuer.GetSubjectPublicKeyInfo());

Fixed (the latter way).

@@ +208,5 @@
>  
>        CertID certID(subject.GetIssuer(), n->cert->derPublicKey,
>                      subject.GetSerialNumber());
> +      SECStatus
> +        srv = trustDomain.CheckRevocation(endEntityOrCA, certID, time,

OK, changed.

::: security/pkix/lib/pkixocsp.cpp
@@ +156,5 @@
>    }
>  
>    // TODO(bug 926260): check name constraints
> +
> +  if (trustDomain.VerifySignedData(potentialSigner.GetSignedData(),

OK, changed.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2b: Fix duplicate detection bug in Part 2 found by keeler

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #15)
> Created attachment 8450609 [details] [diff] [review]
> Part 2b: Fix duplicate detection bug in Part 2 found by keeler

Note: I just tried to write a test for this, bug the test doesn't work without the fix for bug 1033953, because CERT_NewTempCertificate fails when there are duplicate extensions! So, I'll have to add the test in a separate bug.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Parse certificates using mozilla::pkix::der

The new "Part 2" is the old parts 1-6 folded together, with review comments addressed except for stuff about duplicate extensions, which will be handled in another patch.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3: Remove CERTCertificate usage from BuildCertChain

We need to remove uses of CERTCertificate from BuildCertChain and from the pkix_cert_extensions test in order to be sure that our new test in pkix_cert_extensions is testing what mozilla::pkix is doing instead of what NSS's CERT_NewTempCertificate is doing.

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 4: Add a test for the case of multiple subjectAltName extensions

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 5: Detect multiple subjectAltName extensions and document the behavior for policyConstraints extensions

I verified that that test added in part 4 fails without this patch and passes with this patch.

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8449141 [details] [diff] [review]
Part 1: Add a new overload of mozilla::pkix::bind

https://hg.mozilla.org/integration/mozilla-inbound/rev/6afeb8211a65

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6afeb8211a65

Comment 24

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8450704 [details] [diff] [review]
Part 3: Remove CERTCertificate usage from BuildCertChain

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

LGTM.

::: security/pkix/test/gtest/pkix_cert_extension_tests.cpp
@@ +203,5 @@
>    };
>    const char* certCN = "CN=Cert With Critical Wrong OID Extension";
>    ScopedSECKEYPrivateKey key;
> +  // cert is owned by the arena
> +  const SECItem* cert(CreateCert(arena.get(), certCN, 

nit: trailing space

Comment 25

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8450705 [details] [diff] [review]
Part 4: Add a test for the case of multiple subjectAltName extensions

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

Looks good.

::: security/pkix/test/gtest/pkix_cert_extension_tests.cpp
@@ +329,5 @@
>                                     results));
>  }
> +
> +// Two subjectAltNames must result in an error.
> +TEST_F(pkix_cert_extensions, DuplicateSubjectAltName)

Looks like this will have to be changed for the new naming conventions.

Comment 26

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8450708 [details] [diff] [review]
Part 5: Detect multiple subjectAltName extensions and document the behavior for policyConstraints extensions

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

Great.

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/911d02f2c02a
(In reply to David Keeler (:keeler) [use needinfo?] from comment #24)
> > +  const SECItem* cert(CreateCert(arena.get(), certCN, 
> 
> nit: trailing space

Sorry, I missed this nit. I added the removal of the trailing space to the patch for bug 1035008.

> > +// Two subjectAltNames must result in an error.
> > +TEST_F(pkix_cert_extensions, DuplicateSubjectAltName)
> 
> Looks like this will have to be changed for the new naming conventions.

The patch for bug 1035008 already changes this.

Thanks for the reviews!

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #27)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/911d02f2c02a

Forgot to mention: Part 1 landed previously. Parts 2, 2b, 3, 4, and 5 were all folded together into changeset 911d02f2c02a.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/911d02f2c02a