1122841 - Security checks for public keys should be done within mozilla::pkix, not by the TrustDomain

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Originally we added CheckPublicKey to TrustDomain, and defined it to be a subset of what VerifySignedData does, out of convenience, because of how the pkixnss.cpp code is structured. However, in bug 1073867, we learned that this structure is error-prone. Also, this makes it harder than it should be to write a new TrustDomain implementation that doesn't depend on pkixnss.cpp.

Instead, let's do all the public key checks within mozilla::pkix proper, to make it easier to write correct TrustDomain implementations.

Comment 1

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

Attachment: PositiveInteger.patch

Comment 2

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

Attachment: CheckSubjectPublicKeyinfo.patch

I reused your code from bug 1077790, but it doesn't look like it, since I moved it to pkixcheck and modified it. The main reason I did that was because I needed to map the curve to its bit length. I also wanted to reduce the number of layers of indirection to make the code more straightforward.

Do you agree that this is doing the same checks that your original code is doing, or a superset of those checks? Is there anything I'm overlooking? Thanks!

Comment 3

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

Attachment: CheckSubjectPublicKeyinfo.patch [v2]

Uploaded the wrong version.

Comment 4

[:Cykesiopka]

Comment on attachment 8552812 [details] [diff] [review]
CheckSubjectPublicKeyinfo.patch [v2]

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

Yes, I agree that this does a superset of the checks done in the original code.
I don't think you're overlooking anything, (but then again, I'm not super familiar with all the mozilla::pkix code).

::: security/pkix/include/pkix/pkixtypes.h
@@ +306,2 @@
>    //
> +  // Return Success if the key size is acceptable,

Hmm, do callers need to know the exact result values that could be returned? I'm fine either way, just curious.

@@ +306,3 @@
>    //
> +  // Return Success if the key size is acceptable,
> +  // Result::ERROR_INADEQUATE_KEY_SIZE if the key size is not acceptable, 

Nit: trailing whitespace.

@@ +313,5 @@
> +
> +  // Check that the given named ECC curve is acceptable for ECDSA signatures.
> +  //
> +  // Return Success if the curve is acceptable,
> +  // Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE if the curve is not acceptable, 

Nit: trailing whitespace.

@@ +323,5 @@
>    //
>    // Most implementations of this function should probably forward the call
>    // directly to mozilla::pkix::VerifySignedData.
>    //
> +  // CheckRSAPublicKeyModulusSizeInBits or CheckECDSACurveIsSupported will

s/CheckECDSACurveIsSupported/CheckECDSACurveIsAcceptable/

::: security/pkix/lib/pkixcheck.cpp
@@ +120,5 @@
> +      return rv;
> +    }
> +
> +    // RFC 5480
> +    // python DottedOIDToCode.py id-secp256r1 1.2.840.10045.3.1.7

Looks like I didn't read RFC 5480 carefully in Bug 1077790 - there's no "id-" prefix for the three curves. Sorry about that!

@@ +205,5 @@
> +        return rv;
> +      }
> +      // XXX: Should we do additional checks of the modulus?
> +      rv = trustDomain.CheckRSAPublicKeyModulusSizeInBits(
> +             endEntityOrCA, modulusSignificantBytes * 8u);

This suffers the same issue described in https://hg.mozilla.org/mozilla-central/file/d8dd513ac780/security/manager/ssl/tests/unit/test_keysize/generate.py#l226 (rounding to next multiple of 8).

On the other hand, it's not a regression. Maybe just file a new bug, and update the comment?

::: security/pkix/lib/pkixnss.cpp
@@ +33,3 @@
>  #include "pkix/pkix.h"
>  #include "pkix/ScopedPtr.h"
>  #include "pkixder.h"

Can this import be removed?

Comment 5

[:Cykesiopka]

Comment on attachment 8552803 [details] [diff] [review]
PositiveInteger.patch

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

::: security/pkix/lib/pkixder.cpp
@@ +569,5 @@
> +    }
> +    assert(firstByte == 0x00 || firstByte == 0xFF);
> +  }
> +
> +  Reader::Mark startOfValue(reader.GetMark());

Is this needed? It doesn't look used.

Comment 6

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

Comment on attachment 8552812 [details] [diff] [review]
CheckSubjectPublicKeyinfo.patch [v2]

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

Thanks for looking at this. I'll fix the patch and upload a new version with everything addressed.

::: security/pkix/include/pkix/pkixtypes.h
@@ +306,2 @@
>    //
> +  // Return Success if the key size is acceptable,

The callers of BuildCertChain expect Result::ERROR_INADEQUATE_KEY_SIZE to be returned when the key size is too small. Since the result of CheckRSAPublickeyModulusSizeInBits is passed through unmodified, the TrustDomain needs to return Result::ERROR_INADEQUATE_KEY_SIZE in that situation. The ability to return other error codes should very rarely be needed, and is intended mostly for things like "out of memory".

::: security/pkix/lib/pkixcheck.cpp
@@ +120,5 @@
> +      return rv;
> +    }
> +
> +    // RFC 5480
> +    // python DottedOIDToCode.py id-secp256r1 1.2.840.10045.3.1.7

Thanks. I fixed this.

@@ +205,5 @@
> +        return rv;
> +      }
> +      // XXX: Should we do additional checks of the modulus?
> +      rv = trustDomain.CheckRSAPublicKeyModulusSizeInBits(
> +             endEntityOrCA, modulusSignificantBytes * 8u);

I guess the questions we need to figure out the answer to are "Is a 2048-bit modulus where the top bit is 0 still a 2048-bit modulus?" and "Does it matter?"

Comment 7

[:Cykesiopka]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #6)
> I guess the questions we need to figure out the answer to are "Is a 2048-bit
> modulus where the top bit is 0 still a 2048-bit modulus?" and "Does it
> matter?"

Personally I don't think it matters security wise. Really the only reason I put the original comment is to answer "why is the value 2040 instead of 2047?" for anyone who happens to read the code.

Comment 8

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

Attachment: Implement PositiveInteger for syntax checking of RSA modulus and exponent.

Comment 9

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

Attachment: Check syntax of SubjectPublicKeyInfo for every certificate

Comment 10

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

try run looks good so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bf0922f5083

Comment 11

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

Comment on attachment 8558248 [details] [diff] [review]
Implement PositiveInteger for syntax checking of RSA modulus and exponent.

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

Looks good. r=me with comments addressed.

::: security/pkix/lib/pkixder.cpp
@@ +568,5 @@
> +    return rv;
> +  }
> +
> +  // If there is a byte after an initial 0x00/0xFF, then the initial byte
> +  // indicates an positive/negative integer value with its high bit set/unset.

nit: "a positive/negative integer"

@@ +603,5 @@
> +
> +  if (significantBytes) {
> +    *significantBytes = value.GetLength();
> +    if (prefixed) {
> +      assert(*significantBytes > 0);

I think we can have a tighter bound of assert(*significantBytes > 1); here, right?

::: security/pkix/lib/pkixder.h
@@ +295,3 @@
>    }
>    value = valueByte;
> +  return Success;

Seems like it would still be a good idea to return End(valueReader); here (even though IntegralBytes already indirectly checked that this will be the case).

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +904,2 @@
>  
> +class pkixder_universla_types_tests_Integer

typo: "universal"

@@ +953,5 @@
> +  { TLV(2, "\xFF\x7F"), false, INVALID },
> +
> +  // The leading 0xFF is unnecessary.
> +  { TLV(2, "\xFF\x80"), false, INVALID },
> +  { TLV(2, "\xFF\xFF"), false, INVALID },

It would be nice to have tests that differentiate valid/invalid negative numbers.

@@ +967,5 @@
> +
> +  // Misc. larger values
> +  { TLV(2, 4, "\x11\x22\x33\x44"), true, INVALID },
> +  { TLV(2,
> +        "\x01\x02\x03\x04\0x05\x06\x07\\x08\x09\x0a\x0b\x0c\0x0d\x0e\x0f\x00"

What's with the "\\x08"? Doesn't that just come out to the string consisting of the characters "\x08"? (Which isn't invalid, it just doesn't fit the pattern.)

@@ +987,5 @@
> +        "\x01\x02\x03\x04\0x05\x06\x07\\x08\x09\x0a\x0b\x0c\0x0d\x0e\x0f\x00"),
> +    true, INVALID },
> +};
> +
> +TEST_P(pkixder_universla_types_tests_Integer, Integer)

typo

@@ +1007,5 @@
>  }
>  
> +#undef INVALID
> +
> +TEST_P(pkixder_universla_types_tests_Integer, PositiveInteger)

typo

@@ +1018,5 @@
> +  Result expectedResult = params.isValidPositiveInteger
> +                        ? Success
> +                        : Result::ERROR_BAD_DER;
> +  Input value;
> +  ASSERT_EQ(expectedResult, der::PositiveInteger(reader, value));

I think we should also verify the significantBytes functionality of PositiveInteger.

@@ +1031,3 @@
>  }
>  
> +INSTANTIATE_TEST_CASE_P(pkixder_universla_types_tests_Integer,

I'm not seeing the INSTANTIATE_TEST_CASE_P for PositiveInteger.

Comment 12

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

Comment on attachment 8558249 [details] [diff] [review]
Check syntax of SubjectPublicKeyInfo for every certificate

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

LGTM.

Comment 13

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

Comment on attachment 8558248 [details] [diff] [review]
Implement PositiveInteger for syntax checking of RSA modulus and exponent.

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

::: security/pkix/lib/pkixder.cpp
@@ +603,5 @@
> +
> +  if (significantBytes) {
> +    *significantBytes = value.GetLength();
> +    if (prefixed) {
> +      assert(*significantBytes > 0);

Yes.

::: security/pkix/lib/pkixder.h
@@ +295,3 @@
>    }
>    value = valueByte;
> +  return Success;

I updated this part to be this:

  value = valueByte;
  rv = End(valueReader);
  assert(rv == Success); // guaranteed by IntegralBytes's range checks.
  return rv;

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +953,5 @@
> +  { TLV(2, "\xFF\x7F"), false, INVALID },
> +
> +  // The leading 0xFF is unnecessary.
> +  { TLV(2, "\xFF\x80"), false, INVALID },
> +  { TLV(2, "\xFF\xFF"), false, INVALID },

valid/invalid negative numbers can never happen in these tests, which test PositiveInteger and IntegerValue.

Only CertificateSerialNumber supports negative values, right? And, that's tested in the other file that tests PKI-specific stuff like CertificateSerialNumber.

@@ +967,5 @@
> +
> +  // Misc. larger values
> +  { TLV(2, 4, "\x11\x22\x33\x44"), true, INVALID },
> +  { TLV(2,
> +        "\x01\x02\x03\x04\0x05\x06\x07\\x08\x09\x0a\x0b\x0c\0x0d\x0e\x0f\x00"

Yes, it in fact was a typo. Fixed.

@@ +1018,5 @@
> +  Result expectedResult = params.isValidPositiveInteger
> +                        ? Success
> +                        : Result::ERROR_BAD_DER;
> +  Input value;
> +  ASSERT_EQ(expectedResult, der::PositiveInteger(reader, value));

I will upload a new patch that adds this.

@@ +1031,3 @@
>  }
>  
> +INSTANTIATE_TEST_CASE_P(pkixder_universla_types_tests_Integer,

The INSTANTIATE_TEST_CASE_P is for the class, not for the test, so this INSTANTIATE_TEST_CASE_P will cause all the tests that declare their fixture to be pkixder_universla_types_tests_Integer to run. I verified this when running the tests.

Comment 14

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

Attachment: Interdiff which addresses review comments

Comment 15

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

Comment on attachment 8560807 [details] [diff] [review]
Interdiff which addresses review comments

I will merge this interdiff with Part 1 when I check this in.

Comment 16

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

Comment on attachment 8558248 [details] [diff] [review]
Implement PositiveInteger for syntax checking of RSA modulus and exponent.

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

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +967,5 @@
> +
> +  // Misc. larger values
> +  { TLV(2, 4, "\x11\x22\x33\x44"), true, INVALID },
> +  { TLV(2,
> +        "\x01\x02\x03\x04\0x05\x06\x07\\x08\x09\x0a\x0b\x0c\0x0d\x0e\x0f\x00"

In fact, there were other typos, like "\0x05" instead of "\x05" and "\0x0d" instead of "\x0d", caught by the additional significantBytes test.

Comment 17

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

Comment on attachment 8560807 [details] [diff] [review]
Interdiff which addresses review comments

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

LGTM

Comment 18

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

Thanks for the reviews!:

https://hg.mozilla.org/integration/mozilla-inbound/rev/75c440d6b2ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe8d7d7f9f7

I also removed the unnecessary #include of pkixder.h as mentioned by Cykesiopka earlier and the include of cert.h which has long been unnecessary.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/75c440d6b2ff
https://hg.mozilla.org/mozilla-central/rev/3fe8d7d7f9f7