1036107 - Stop using NSS's CERTSignedData in mozilla::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

If we use the CERTSignedData type in mozilla::pkix then we won't be able to use mozilla::pkix without the NSS header files present, using an alternative crypto implementation. Additionally, we can improve the type-safety and const-correctness of mozilla::pkix by using strongly-typed arrays and (eventually) a more secure variant of SECItem.

Comment 1

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

Attachment: Part 1: Replace CERTSignedData

Comment 2

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

Attachment: Part 2: Tests

Comment 3

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

Here's a link to the most recent try push that includes these changes:
https://tbpl.mozilla.org/?tree=Try&rev=e438d7c4f77a

Comment 4

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

Comment on attachment 8454282 [details] [diff] [review]
Part 1: Replace CERTSignedData

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

Just fyi, some B2G builds didn't compile with one of the patches in that push: https://tbpl.mozilla.org/php/getParsedLog.php?id=43576547&tree=Try
Otherwise, looks good. I just had a few questions. r=me with those answered.

::: security/apps/AppTrustDomain.cpp
@@ +187,5 @@
>  }
>  
>  SECStatus
> +AppTrustDomain::VerifySignedData(
> +  const ::mozilla::pkix::SignedDataWithSignature& signedData,

Is fully qualifying the namespace here necessary or is it a stylistic decision?

::: security/pkix/include/pkix/pkixtypes.h
@@ +46,5 @@
> +//   * secp256r1 (OID 1.2.840.10045.3.17, RFC 5480)
> +MOZILLA_PKIX_ENUM_CLASS SignatureAlgorithm
> +{
> +  // ecdsa-with-SHA512 (OID 1.2.840.10045.4.3.4, RFC 5758 Section 3.2)
> +  ecdsa_with_sha512 = 1,

Is there a rationale behind how these enum values are numbered?

@@ +293,5 @@
> +  //
> +  // Most implementations of this function should probably forward the call
> +  // directly to mozilla::pkix::VerifySignedData.
> +  virtual SECStatus VerifySignedData(const SignedDataWithSignature& signedData,
> +                                     const SECItem& subjectPublicKeyInfo) = 0;

Is it important that this declaration be moved from where it was? In any case, let's have a blank line after it.

::: security/pkix/lib/pkixder.cpp
@@ +106,5 @@
>  Result
> +DigestAlgorithmOIDValue(Input& algorithmID, /*out*/ DigestAlgorithm& algorithm)
> +{
> +  // RFC 4055 Section 2.2
> +  // python DottedOIDToCode.py id-sha256 2.16.840.1.101.3.4.2.1

I'm seeing these values in RFC 4055 section 2.1, not 2.2.

@@ +120,5 @@
> +    0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03
> +  };
> +
> +  // RFC 3279 Section 2.2.1
> +  // python DottedOIDToCode.py id-sha1 1.3.14.3.2.26

This appears to be in RFC 4055 section 2.1 as well as RFC 3279.

@@ +136,5 @@
> +    algorithm = DigestAlgorithm::sha384;
> +  } else if (algorithmID.MatchRest(id_sha512)) {
> +    algorithm = DigestAlgorithm::sha512;
> +  } else {
> +    return Fail(SEC_ERROR_INVALID_ALGORITHM); // TODO: better error code?

I think this is a pretty good error code. Did you want something like "unexpected algorithm" or "unsupported algorithm"?

::: security/pkix/lib/pkixkey.cpp
@@ +46,5 @@
>    }
>  
>    // See bug 921585.
> +  if (sd.data.len >
> +        static_cast<unsigned int>(std::numeric_limits<int>::max())) {

This was 80 chars, right? It's probably fine as it was.

Comment 5

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

Comment on attachment 8454283 [details] [diff] [review]
Part 2: Tests

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

LGTM with nits addressed (mainly, some of the testing values aren't documented clearly enough).

::: security/pkix/test/gtest/pkixder_pki_types_tests.cpp
@@ +233,5 @@
> +{
> +};
> +
> +static const AlgorithmIdentifierTestInfo<DigestAlgorithm>
> +  DIGEST_ALGORITHM_TEST_INFO[] = {

nit: maybe don't indent DIGEST_ALGORITHM_TEST_INFO - it makes it hard to identify it as a variable

@@ +259,5 @@
> +
> +TEST_P(pkixder_DigestAlgorithmIdentifier, Valid)
> +{
> +  const AlgorithmIdentifierTestInfo<DigestAlgorithm>& param(GetParam());
> +  

nit: trailing whitespace

@@ +295,5 @@
> +  // Invalid because no MD5-based signatures algorithms are supported by the
> +  // parser.
> +  static const uint8_t DER[] = {
> +    0x30, 0x0a, 0x06, 0x08,
> +    0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x02, 0x05

I'm assuming this is some MD5 OID, but I would appreciate a comment saying what it is.

@@ +307,5 @@
> +}
> +
> +TEST_F(pkixder_DigestAlgorithmIdentifier, Invalid_Digest_RSA_WITH_SHA256)
> +{
> +  // Invalid because SHA-1 is not a signature algorithm.

This test is confusing me - the name says SHA256 but this comment mentions SHA-1. What am I missing here?

@@ +310,5 @@
> +{
> +  // Invalid because SHA-1 is not a signature algorithm.
> +  static const uint8_t DER[] = {
> +    0x30, 0x0a, 0x06, 0x08,
> +    0x2a, 0x86, 0x48, 0xce, 0x3d, 0x04, 0x03, 0x02,

I would appreciate a comment saying what this OID is.

@@ +425,5 @@
> +  // Invalid because no MD5-based signatures algorithms are supported by the
> +  // parser.
> +  static const uint8_t DER[] = {
> +    0x30, 0x0b, 0x06, 0x09,
> +    0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x04

Again, add a comment explaining what this is

@@ +440,5 @@
> +{
> +  // Invalid because SHA-1 is not a signature algorithm.
> +  static const uint8_t DER[] = {
> +    0x30, 0x0b, 0x06, 0x09,
> +    0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01

Same

Comment 6

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

Comment on attachment 8454282 [details] [diff] [review]
Part 1: Replace CERTSignedData

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

::: security/apps/AppTrustDomain.cpp
@@ +187,5 @@
>  }
>  
>  SECStatus
> +AppTrustDomain::VerifySignedData(
> +  const ::mozilla::pkix::SignedDataWithSignature& signedData,

Copy/paste error. fixed.

::: security/pkix/include/pkix/pkixtypes.h
@@ +46,5 @@
> +//   * secp256r1 (OID 1.2.840.10045.3.17, RFC 5480)
> +MOZILLA_PKIX_ENUM_CLASS SignatureAlgorithm
> +{
> +  // ecdsa-with-SHA512 (OID 1.2.840.10045.4.3.4, RFC 5758 Section 3.2)
> +  ecdsa_with_sha512 = 1,

No.

@@ +293,5 @@
> +  //
> +  // Most implementations of this function should probably forward the call
> +  // directly to mozilla::pkix::VerifySignedData.
> +  virtual SECStatus VerifySignedData(const SignedDataWithSignature& signedData,
> +                                     const SECItem& subjectPublicKeyInfo) = 0;

I will add the blank line. I moved this because of bug 1036105, but I think it is OK to do the moving now instead of in bug 1036105.

::: security/pkix/lib/pkixder.cpp
@@ +106,5 @@
>  Result
> +DigestAlgorithmOIDValue(Input& algorithmID, /*out*/ DigestAlgorithm& algorithm)
> +{
> +  // RFC 4055 Section 2.2
> +  // python DottedOIDToCode.py id-sha256 2.16.840.1.101.3.4.2.1

Fixed.

@@ +120,5 @@
> +    0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03
> +  };
> +
> +  // RFC 3279 Section 2.2.1
> +  // python DottedOIDToCode.py id-sha1 1.3.14.3.2.26

OK. I moved it above sha256.

@@ +136,5 @@
> +    algorithm = DigestAlgorithm::sha384;
> +  } else if (algorithmID.MatchRest(id_sha512)) {
> +    algorithm = DigestAlgorithm::sha512;
> +  } else {
> +    return Fail(SEC_ERROR_INVALID_ALGORITHM); // TODO: better error code?

I removed the comment. I agree it is too vague. I was thinking it is strange to have such a general error for bad hash algorithms but then have a much more specific error for disabled signature algorithms, especially when the signature algorithm is disabled because the hash algorithm is disabled (e.g. MD5).

::: security/pkix/lib/pkixkey.cpp
@@ +46,5 @@
>    }
>  
>    // See bug 921585.
> +  if (sd.data.len >
> +        static_cast<unsigned int>(std::numeric_limits<int>::max())) {

82, but I reverted the change.

Comment 7

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

Comment on attachment 8454283 [details] [diff] [review]
Part 2: Tests

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

Thanks for the careful review!

::: security/pkix/test/gtest/pkixder_pki_types_tests.cpp
@@ +233,5 @@
> +{
> +};
> +
> +static const AlgorithmIdentifierTestInfo<DigestAlgorithm>
> +  DIGEST_ALGORITHM_TEST_INFO[] = {

OK, done.

@@ +259,5 @@
> +
> +TEST_P(pkixder_DigestAlgorithmIdentifier, Valid)
> +{
> +  const AlgorithmIdentifierTestInfo<DigestAlgorithm>& param(GetParam());
> +  

nixed.

@@ +295,5 @@
> +  // Invalid because no MD5-based signatures algorithms are supported by the
> +  // parser.
> +  static const uint8_t DER[] = {
> +    0x30, 0x0a, 0x06, 0x08,
> +    0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x02, 0x05

// The OID identifies MD5 (1.2.840.113549.2.5). It is invalid because we
  // don't accept MD5 as a hash algorithm.

@@ +307,5 @@
> +}
> +
> +TEST_F(pkixder_DigestAlgorithmIdentifier, Invalid_Digest_RSA_WITH_SHA256)
> +{
> +  // Invalid because SHA-1 is not a signature algorithm.

I copy/pasted the comment from the Invalid_SignatureAlgorithm_SHA1 test and forgot to modify it.

@@ +310,5 @@
> +{
> +  // Invalid because SHA-1 is not a signature algorithm.
> +  static const uint8_t DER[] = {
> +    0x30, 0x0a, 0x06, 0x08,
> +    0x2a, 0x86, 0x48, 0xce, 0x3d, 0x04, 0x03, 0x02,

// The OID identifies ecdsa-with-SHA256 (1.2.840.10045.4.3.2). It is invalid
  // because ECDSA-with-SHA256 is not a hash algorithm.

@@ +425,5 @@
> +  // Invalid because no MD5-based signatures algorithms are supported by the
> +  // parser.
> +  static const uint8_t DER[] = {
> +    0x30, 0x0b, 0x06, 0x09,
> +    0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x04

// The OID identifies RSA-with-MD5 (1.2.840.113549.1.1.4). It is invalid
  // because no MD5-based signatures algorithms are supported by the parser.

@@ +440,5 @@
> +{
> +  // Invalid because SHA-1 is not a signature algorithm.
> +  static const uint8_t DER[] = {
> +    0x30, 0x0b, 0x06, 0x09,
> +    0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01

// The OID identifies id-sha256 (2.16.840.1.101.3.4.2.1). It is invalid
  // because SHA-256 is not a signature algorithm.

Comment 8

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

Thanks for the quick, careful reviews:

https://hg.mozilla.org/integration/mozilla-inbound/rev/578899c0b819
https://hg.mozilla.org/integration/mozilla-inbound/rev/96d7c79707e5

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/578899c0b819
https://hg.mozilla.org/mozilla-central/rev/96d7c79707e5