989564 - Clean up implementation of DecodeBasicConstraints (mozilla::pkix)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

+++ This bug was initially created as a clone of Bug #985021 +++

The current implementation of DecodeBasicConstraints has the following pattern:

  ...
  
  if (der::ExpectTagAndIgnoreLength(input, der::SEQUENCE) != der::Success) {
    return der::Fail(SEC_ERROR_EXTENSION_VALUE_INVALID);
  }
   
  ...
   
  if (der::End(input) != der::Success) {
    return der::Fail(SEC_ERROR_EXTENSION_VALUE_INVALID);
  }

This pattern is exactly what der::Nested is for, and we should change the code so we use der::Nested. I didn't catch this during review of the patch in bug 985021.

Comment 1

[Avik Pal [:avikpal]]

Hi Brian,

I would like to work on this. Can you assign this to me?

Comment 2

[Mike Hoye [:mhoye]]

Hi, Avik -

You're welcome to get started on it. Here are some links to help you get your development environment going. Once you've got your own version of Firefox building, we'd be happy to assign the bug to you.

https://wiki.mozilla.org/Mobile/Get_Involved
https://developer.mozilla.org/docs/Introduction

Comment 3

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

Hi, I needed a patch for this for something else so I wrote one.

Avik, thanks for your offer to help. I recommend checking out some other open bugs.

Comment 4

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

Attachment: decode-basic-constraints.patch

This patch:

1. Uses der::Nested
2. Factors out the handling of optional integers into a testable function and adds tests for it.
3. Refactors the implementation of der::Enumeration and der::CertificateSerialNumber. Note that the der::CertificateSerialNumber implementation is a literal copy/paste from the old der::Integer implementation.
4. Removes the dependency on the NSS DER_GetInteger function.

Comment 5

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

Comment on attachment 8417496 [details] [diff] [review]
decode-basic-constraints.patch

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

I think this looks good. Just a few general comments:
1. Is there a particularly compelling reason to use long without specifying the type size? The implementation basically limits it to int8_t, so why don't we just use that?
2. DER_INTEGER, not DER_INTEGUR
3. Would ASSERT_SECFailure, etc. be useful in the tests you added?
4. Maybe a few more comments explaining the tests would be good.

::: security/pkix/lib/pkixcheck.cpp
@@ +22,5 @@
>  #include "pkixcheck.h"
>  #include "pkixder.h"
>  #include "pkixutil.h"
> +
> +using namespace std;

If I understand what's going on, this isn't necessary.

@@ +211,5 @@
> +                    bind(DecodeBasicConstraints, _1, ref(isCA),
> +                         ref(pathLenConstraint))) != der::Success) {
> +      return Fail(RecoverableError, SEC_ERROR_EXTENSION_VALUE_INVALID);
> +    }
> +    if (der::End(input) != der::Success) {

Nested checks End, right? So this shouldn't be necessary.

::: security/pkix/lib/pkixder.h
@@ +364,5 @@
>  }
>  
>  // Universal types
>  
> +namespace {

my understanding is static is preferred over unnamed namespaces

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +264,3 @@
>  {
> +  for (uint8_t i = 0; i <= 127; ++i) {
> +    uint8_t DER_INTEGUR[] = {

DER_INTEGER

@@ +265,5 @@
> +  for (uint8_t i = 0; i <= 127; ++i) {
> +    uint8_t DER_INTEGUR[] = {
> +      0x02, // INTEGER
> +      0x01, // length
> +      i,    // 0

This comment is unclear - maybe "// value"?

@@ +271,5 @@
> +
> +    Input input;
> +    ASSERT_EQ(Success, input.Init(DER_INTEGUR, sizeof DER_INTEGUR));
> +
> +    uint8_t value = i + 1;

Maybe add a comment that says we're initializing value with a number we're expecting it to not be? (Or just don't initialize value?)

@@ +282,5 @@
> +{
> +  // This is a valid integer value but our integer parser cannot parse
> +  // negative values.
> +
> +  const uint8_t DER_INTEGUR[] = {

DER_INTEGER

@@ +292,5 @@
> +  Input input;
> +  ASSERT_EQ(Success, input.Init(DER_INTEGUR, sizeof DER_INTEGUR));
> +
> +  uint8_t value;
> +  ASSERT_EQ(Failure, Integer(input, value));

Use ASSERT_SECFailure et. al.?

@@ +438,4 @@
>  
> +TEST_F(pkixder_universal_types_tests, OptionalIntegerUnsupportedDefault)
> +{
> +  const uint8_t DER_BOOLEAN_TRUE[] = {

It would be good to have a comment explaining why you're using a BOOLEAN for these tests (my understanding is that the INTEGER, being optional, is omitted).

Comment 6

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

Comment on attachment 8417496 [details] [diff] [review]
decode-basic-constraints.patch

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

Thanks for the quick review.

::: security/pkix/lib/pkixcheck.cpp
@@ +211,5 @@
> +                    bind(DecodeBasicConstraints, _1, ref(isCA),
> +                         ref(pathLenConstraint))) != der::Success) {
> +      return Fail(RecoverableError, SEC_ERROR_EXTENSION_VALUE_INVALID);
> +    }
> +    if (der::End(input) != der::Success) {

This check to End is necessary. See https://bugzilla.mozilla.org/show_bug.cgi?id=989833#c1. der::Nested checks that all the input WITHIN the sequence is consumed, but it doesn't check if there is unconsumed input AFTER the sequence. And, it doesn't make sense to do so because you may have more stuff to parse after the sequence.

::: security/pkix/lib/pkixder.h
@@ +364,5 @@
>  }
>  
>  // Universal types
>  
> +namespace {

static wouldn't help anything because this is a header file. I could name it "internal" if you think that would be better.

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +264,3 @@
>  {
> +  for (uint8_t i = 0; i <= 127; ++i) {
> +    uint8_t DER_INTEGUR[] = {

This mispelling is intentional and is carried over from the existing tests. Apparently using DER_INTEGER fails because some platforms (Windows, I think) define a symbol DER_INTEGER. So, I suggest we just leave it.

@@ +265,5 @@
> +  for (uint8_t i = 0; i <= 127; ++i) {
> +    uint8_t DER_INTEGUR[] = {
> +      0x02, // INTEGER
> +      0x01, // length
> +      i,    // 0

Thanks; this is a typo.

@@ +271,5 @@
> +
> +    Input input;
> +    ASSERT_EQ(Success, input.Init(DER_INTEGUR, sizeof DER_INTEGUR));
> +
> +    uint8_t value = i + 1;

I will add the comment. Note that in these tests, we explicitly initial the values of out parameters like this so that we can verify that the function being tested didn't just leave it uninitialized; if we didn't explicitly initialize the variable then the test might pass in that circumstance if the uninitialized memory happened to have the right value.

@@ +292,5 @@
> +  Input input;
> +  ASSERT_EQ(Success, input.Init(DER_INTEGUR, sizeof DER_INTEGUR));
> +
> +  uint8_t value;
> +  ASSERT_EQ(Failure, Integer(input, value));

I think it is a good idea to define ASSERT_Failure and ASSERT_Success macros analogous to ASSERT_SECFailure and ASSERT_SECSuccess but (a) all the existing tests use the style I used, and (b) it would be better to do that after we merge mozilla::pkix::der::Result and mozilla::pkix::Result into one type, IMO.

@@ +438,4 @@
>  
> +TEST_F(pkixder_universal_types_tests, OptionalIntegerUnsupportedDefault)
> +{
> +  const uint8_t DER_BOOLEAN_TRUE[] = {

I will add the comment. Your understanding is correct. Additionally, I wanted to have a valid DER encoding of something.

Comment 7

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

(In reply to David Keeler (:keeler) from comment #5)
> Comment on attachment 8417496 [details] [diff] [review]
> decode-basic-constraints.patch
> 
> Review of attachment 8417496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks good. Just a few general comments:
> 1. Is there a particularly compelling reason to use long without specifying
> the type size? The implementation basically limits it to int8_t, so why
> don't we just use that?

We have to cast the subCACount and pathLenConstraint to some type that isn't going to result in truncation/overflow. I just chose "long" because that was the simplest to do. Let me know if you have a suggestion for how to proceed.

> 4. Maybe a few more comments explaining the tests would be good.

Are there other comments that you think I should add besides the ones you specifically requested?

Comment 8

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

Comment on attachment 8417496 [details] [diff] [review]
decode-basic-constraints.patch

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

Looks good - I don't think more comments are necessary other than one for DER_INTEGUR (below).

::: security/pkix/lib/pkixcheck.cpp
@@ +211,5 @@
> +                    bind(DecodeBasicConstraints, _1, ref(isCA),
> +                         ref(pathLenConstraint))) != der::Success) {
> +      return Fail(RecoverableError, SEC_ERROR_EXTENSION_VALUE_INVALID);
> +    }
> +    if (der::End(input) != der::Success) {

Oh, right.

::: security/pkix/lib/pkixder.h
@@ +364,5 @@
>  }
>  
>  // Universal types
>  
> +namespace {

Oh, right. Nevermind, then.

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +264,3 @@
>  {
> +  for (uint8_t i = 0; i <= 127; ++i) {
> +    uint8_t DER_INTEGUR[] = {

Ok - a comment would be appreciated.

Comment 9

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

Attachment: RemoveCERTCertificateFromCheckBasicConstraints.patch

This removes the dependency on CERTCertificate from CheckBasicConstraints. Another effect of this patch is that CheckBasicConstraints will be easier to unit test.

Comment 10

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

Comment on attachment 8419088 [details] [diff] [review]
RemoveCERTCertificateFromCheckBasicConstraints.patch

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

Looks good. On an unrelated note, it would be nice if all of these bugs you've been working on had some blocks/depends-on relationships set up, so we can track the work.

Comment 11

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #6)
> static wouldn't help anything because this is a header file. I could name it
> "internal" if you think that would be better.

I did in fact name the namespace "internal" because unnamed namespaces in header files don't make much sense.

> > +    uint8_t DER_INTEGUR[] = {
> 
> This mispelling is intentional and is carried over from the existing tests.
> Apparently using DER_INTEGER fails because some platforms (Windows, I think)
> define a symbol DER_INTEGER. So, I suggest we just leave it.

I renamed these variables to DER and also made them "static const" or "const" as appropriate.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c93eb56c4f06
https://hg.mozilla.org/integration/mozilla-inbound/rev/3714913ab8c4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c93eb56c4f06
https://hg.mozilla.org/mozilla-central/rev/3714913ab8c4