1006041 - Switch EKU decoding to mozilla::pkix::der from NSS

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

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

Attachment: decode-EKU.patch

This patch:

1. Improves the type safety of EKU parameters in callers to BuildCertChain and internally in mozilla::pkix by using a strongly-typed enum. Thus, for example, it is no longer possible to accidentally a policy OID or algorithm OID as an EKU.
2. Removes dependencies on NSS's CERTOidSequence, CERT_DecodeOidSequence, and CERT_DestroyOidSequence.
3. Eliminates any memory allocation as part of EKU processing, making EKU processing slightly faster and slightly less memory-hungry.

Comment 1

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

Comment on attachment 8417526 [details] [diff] [review]
decode-EKU.patch

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

Looks good - r=me with comments addressed. In particular, I feel fairly strongly about MatchBytes returning Result instead of bool.

::: security/certverifier/CertVerifier.cpp
@@ +299,5 @@
>  
>  static SECStatus
>  BuildCertChainForOneKeyUsage(TrustDomain& trustDomain, CERTCertificate* cert,
>                               PRTime time, KeyUsages ku1, KeyUsages ku2,
> +                             KeyUsages ku3, KeyPurposeId eku,

Do we still want the variable to be called "eku"?

::: security/pkix/include/pkix/bind.h
@@ +26,5 @@
> +// A positive side-effect of this code is improved debugging usability; it is
> +// much more convenient to step through code that uses this polyfill than it is
> +// to step through the many nested layers of a real std::bind implementation.
> +//
> +// Build with MOZILLA_PKIX_USE_REAL_FUNCTIONAL defined in order to use the

Is anyone ever going to actually do this?

@@ +142,5 @@
> +}
> +
> +template <typename R, typename P1, typename B1, typename B2, typename B3,
> +          typename B4>
> +inline internal::Bind4<R, P1, const B1, const B2, B3, B4>

why const for B1 and B2?

::: security/pkix/include/pkix/pkix.h
@@ +86,5 @@
>                           CERTCertificate* cert,
>                           PRTime time,
>                           EndEntityOrCA endEntityOrCA,
>              /*optional*/ KeyUsages requiredKeyUsagesIfPresent,
> +                         KeyPurposeId requiredEKUIfPresent,

Is this no longer optional?

::: security/pkix/lib/pkixcheck.cpp
@@ +349,5 @@
> +  // id-kp-clientAuth      OBJECT IDENTIFIER ::= { id-kp 2 }
> +  // id-kp-codeSigning     OBJECT IDENTIFIER ::= { id-kp 3 }
> +  // id-kp-emailProtection OBJECT IDENTIFIER ::= { id-kp 4 }
> +  // id-kp-OCSPSigning     OBJECT IDENTIFIER ::= { id-kp 9 }
> +  static const uint8_t server[] = { (40*1)+3, 6, 1, 5, 5, 7, 3, 1 };

Include a comment on how OIDs are encoded (base-128 with the first two numbers encoded together for whatever reason).

@@ +402,5 @@
> +    }
> +  }
> +
> +  if (match) {
> +    if (value.AtEnd()) {

maybe "if (match && value.AtEnd()) {" ?

::: security/pkix/lib/pkixder.h
@@ +131,5 @@
>      return Success;
>    }
>  
> +  template <uint16_t N>
> +  bool MatchBytes(const uint8_t (&toMatch)[N])

I think this should return Result - all of these other functions that aren't predicates (i.e. all except AtEnd()) return Result. That way, it will be more clear that the internal state of the Input is modified upon Success.

Comment 2

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

(In reply to David Keeler (:keeler) from comment #1)
> >  static SECStatus
> >  BuildCertChainForOneKeyUsage(TrustDomain& trustDomain, CERTCertificate* cert,
> >                               PRTime time, KeyUsages ku1, KeyUsages ku2,
> > +                             KeyUsages ku3, KeyPurposeId eku,
> 
> Do we still want the variable to be called "eku"?

I am not sure what would be a better name and I think "eku" is fine. (I still wonder if calling the type KeyPurposeId (to match the spec) is too pendantic.) Suggestions?

> > +// Build with MOZILLA_PKIX_USE_REAL_FUNCTIONAL defined in order to use the
> 
> Is anyone ever going to actually do this?

I do this when I have to add more overloads of bind, because the compilation errors are maddening and I want to make sure that my code works with std::bind. Otherwise, probably not.

> 
> @@ +142,5 @@
> > +}
> > +
> > +template <typename R, typename P1, typename B1, typename B2, typename B3,
> > +          typename B4>
> > +inline internal::Bind4<R, P1, const B1, const B2, B3, B4>
> 
> why const for B1 and B2?

In C++, you can make a const reference to a temporary value (e.g. a literal value) but you can't take a non-const reference. Thus, if we pass temporary/constant values as parameters to bind(), we need the references in the BindX class to be const. In C++11 one can use rvalue references (T&&) to work around this. This is called the "perfect forwarding" problem in C++. See http://thbecker.net/articles/rvalue_references/section_07.html for a good explanation.

FWIW, I think everything in bind.h is ugly as hell but it's the only way to cope with the ancient C++ support on Android and B2G (where even with modern compilers we use STLPort that doesn't support std::bind).

The consequence of this is that we'll often have to add new overloads of mozilla::pkix::bind like this. (I have another patch that adds another one already.)

> >                           CERTCertificate* cert,
> >                           PRTime time,
> >                           EndEntityOrCA endEntityOrCA,
> >              /*optional*/ KeyUsages requiredKeyUsagesIfPresent,
> > +                         KeyPurposeId requiredEKUIfPresent,
> 
> Is this no longer optional?

If one doesn't want to restrict the EKU then one can pass anyExtendedKeyUsage. I thought that was different enough from null or zero that the argument couldn't be considered optional any more.

> Include a comment on how OIDs are encoded (base-128 with the first two
> numbers encoded together for whatever reason).

OK. I will add a link to the Layman's Guide to ASN.1.

> > +
> > +  if (match) {
> > +    if (value.AtEnd()) {
> 
> maybe "if (match && value.AtEnd()) {" ?

I won't make this change because the "else if" would have a different (wrong) meaning if I did.

> > +  template <uint16_t N>
> > +  bool MatchBytes(const uint8_t (&toMatch)[N])
> 
> I think this should return Result - all of these other functions that aren't
> predicates (i.e. all except AtEnd()) return Result. That way, it will be
> more clear that the internal state of the Input is modified upon Success.

I disagree, because Result (Success vs. Failure) should be used to indicate succsss or failure. For the Match methods, there is no failure case--a mismatch is a success condition and so is a match. Compare that to Expect* where a mismatch is an error condition. So, I'd like to leave it as it is, though perhaps I should add some documentation to that effect. What do you think?

Comment 3

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #2)
> > Do we still want the variable to be called "eku"?
> 
> I am not sure what would be a better name and I think "eku" is fine. (I
> still wonder if calling the type KeyPurposeId (to match the spec) is too
> pendantic.) Suggestions?

I think it's probably fine. Maybe add a comment somewhere saying that KeyPurposeId is what is sometimes called "extended key usage".

> > @@ +142,5 @@
> > why const for B1 and B2?
> 
> In C++, you can make a const reference to a temporary value (e.g. a literal
> value) but you can't take a non-const reference. Thus, if we pass
> temporary/constant values as parameters to bind(), we need the references in
> the BindX class to be const. In C++11 one can use rvalue references (T&&) to
> work around this. This is called the "perfect forwarding" problem in C++.
> See http://thbecker.net/articles/rvalue_references/section_07.html for a
> good explanation.

Interesting - I think I understand.

> > >                           CERTCertificate* cert,
> > >                           PRTime time,
> > >                           EndEntityOrCA endEntityOrCA,
> > >              /*optional*/ KeyUsages requiredKeyUsagesIfPresent,
> > > +                         KeyPurposeId requiredEKUIfPresent,
> > 
> > Is this no longer optional?
> 
> If one doesn't want to restrict the EKU then one can pass
> anyExtendedKeyUsage. I thought that was different enough from null or zero
> that the argument couldn't be considered optional any more.

Sounds good.

> > maybe "if (match && value.AtEnd()) {" ?
> 
> I won't make this change because the "else if" would have a different
> (wrong) meaning if I did.

Oh, I see - this is me getting tripped up by that MatchBytes can change the internal state of the Input.

> > > +  template <uint16_t N>
> > > +  bool MatchBytes(const uint8_t (&toMatch)[N])
> > 
> > I think this should return Result - all of these other functions that aren't
> > predicates (i.e. all except AtEnd()) return Result. That way, it will be
> > more clear that the internal state of the Input is modified upon Success.
> 
> I disagree, because Result (Success vs. Failure) should be used to indicate
> succsss or failure. For the Match methods, there is no failure case--a
> mismatch is a success condition and so is a match. Compare that to Expect*
> where a mismatch is an error condition. So, I'd like to leave it as it is,
> though perhaps I should add some documentation to that effect. What do you
> think?

Well, as we just saw, I forgot that MatchBytes can change the internal state. However we communicate that is fine by me, as long as it's sufficient. Maybe a more descriptive name would help - "ConsumeBytesIfMatch"? A comment is probably fine, anyway.

Comment 4

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

(In reply to David Keeler (:keeler) from comment #3)
> Well, as we just saw, I forgot that MatchBytes can change the internal
> state. However we communicate that is fine by me, as long as it's
> sufficient. Maybe a more descriptive name would help -
> "ConsumeBytesIfMatch"? A comment is probably fine, anyway.

OK, that name seems reasonable. But, I also think Read/Skip/Expect have the same naming problem and it seems like making one super-explicit just makes the it more confusing about whether Expect & friends consume the input. Perhaps SkipIfMatch would be more consistent with the other names. I'll think about it.

Comment 5

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

(In reply to David Keeler (:keeler) [needinfo? is a good way to get my attention] from comment #3)
> Well, as we just saw, I forgot that MatchBytes can change the internal
> state. However we communicate that is fine by me, as long as it's
> sufficient. Maybe a more descriptive name would help -
> "ConsumeBytesIfMatch"? A comment is probably fine, anyway.

I didn't come up with a good solution beyond adding a comment and making the naming more consistent. See bug 1010581 for that.

Comment 6

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

Attachment: fix-EKU.patch

This patch fixes two tiny issues with the original patch.

Comment 7

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

Comment on attachment 8423496 [details] [diff] [review]
fix-EKU.patch

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

Looks good. I guess it would be good to double-check that the entire input is consumed in similar situations in the other patches like this you've written recently.

::: security/pkix/lib/pkixcheck.cpp
@@ +431,5 @@
>  
>      der::Input input;
>      if (input.Init(encodedExtendedKeyUsage->data,
>                     encodedExtendedKeyUsage->len) != der::Success) {
> +      return Fail(RecoverableError, SEC_ERROR_INADEQUATE_CERT_TYPE);

I'm assuming this is in the philosophy of only returning one type of error from each Check... function?

Comment 8

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

(In reply to David Keeler (:keeler) [needinfo? is a good way to get my attention] 
> Looks good. I guess it would be good to double-check that the entire input
> is consumed in similar situations in the other patches like this you've
> written recently.

I did double-check that already. (I actually noticed this when double-checking a later patch.)

> I'm assuming this is in the philosophy of only returning one type of error
> from each Check... function?

Right. That way we can know more quickly which part of the certificate is bad, when the user reports only the error code.

Thanks for the quick review!

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9eff37173e1

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9eff37173e1