1130753 - Pass more details of the public key to the TrustDomain

Status: NEW

(Core :: Security: PSM, defect, P3)

Description

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

In order to collect detailed telemetry for algorithms used for certificate verification, we need to pass more detailed information to the TrustDomain during signature verification, so that the TrustDomain does not need to re-decode the SubjectPublickeyInfo.

Comment 1

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

Attachment: Part 1: Move CheckSubjectPublicKeyInfo to pkixkey.cpp

Comment 2

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

Attachment: Part 2(a): Pass parsed key and signature to TrustDomain

Comment 3

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

Attachment: Part 2(b): Update tests

Comment 4

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

Attachment: Part 2(c): Update PSM

Comment 5

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

Attachment: Part 2(d): Add initial tests for RSA key parsing

I'm still working on the tests. I'm hoping you can give me some feedback on this before I go too much further.

Ultimately, I think it would be good if Gecko constructed the SECKEYPublicKey structure and did other signature verification parsing itself using mozilla::pkix's pkixder.cpp. This is one big step towards that.

Before that, though, this pre-parsing ahead of NSS's parser is helpful for protecting NSS from any malformed inputs. With this change, all the DER that NSS sees will be syntactically valid, except for the DER that is encrypted within an RSA PKCS#1 signature.

Also, this parsing is useful for the purposes of collecting some telemetry regarding combinations of public key algorithms, public key parameters, and digest algorithms.

Finally, I think this should make it easier to support PSS signatures in mozilla::pkix.

Comment 6

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

Comment on attachment 8592519 [details] [diff] [review]
Part 2(a): Pass parsed key and signature to TrustDomain

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

LGTM.

::: security/pkix/lib/pkixkey.cpp
@@ +253,5 @@
>  
>  Result
> +PublicKey::VerifySignedDigest(TrustDomain& trustDomain,
> +                             der::PublicKeyAlgorithm signtaurePublicKeyAlg,
> +                             const SignedDigest& signedDigest) const

nit: indentation on these two lines

@@ +259,5 @@
> +  switch (type) {
> +    case Type::ECC:
> +    {
> +      if (signtaurePublicKeyAlg != der::PublicKeyAlgorithm::ECDSA) {
> +        return Result::ERROR_BAD_SIGNATURE; // TODO: better error code?

I think this error code makes sense.

Comment 7

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

Comment on attachment 8592520 [details] [diff] [review]
Part 2(b): Update tests

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

LGTM - just a couple of nits.

::: security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp
@@ +94,5 @@
>    ASSERT_EQ(Success, spki.Init(issuerSPKI.data(), issuerSPKI.length()));
>  
> +  PublicKey publicKey;
> +  ASSERT_EQ(Success, publicKey.Init(EndEntityOrCA::MustBeCA, spki));
> +  MockCertIDCreationTrustDomain mochCERTIDTrustDomain;

"mockCERTIDTrustDomain"?

::: security/pkix/test/lib/pkixtestutil.h
@@ +436,5 @@
> +  {
> +    return Success;
> +  }
> +
> +  Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override {

nit: { on the next line

Comment 8

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

Comment on attachment 8592522 [details] [diff] [review]
Part 2(c): Update PSM

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

Looks good - just one clarification question.

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +88,5 @@
> +  {
> +    return nullptr;
> +  }
> +  PublicKey issuerPublicKey;
> +  if (issuerPublicKey.Init(EndEntityOrCA::MustBeEndEntity,

Shouldn't the issuerPublicKey be a CA? Or am I misunderstanding this field?

Comment 9

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

Comment on attachment 8592525 [details] [diff] [review]
Part 2(d): Add initial tests for RSA key parsing

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

This seems like a good approach. I suggested another test case, but other than that I don't have any additional ideas.

::: security/pkix/test/gtest/pkixkey_tests.cpp
@@ +6,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +/* Copyright 2014 Mozilla Contributors

Should we use 2015 for new files? Or does the 2014 apply to everything that ends up in these directories?

@@ +65,5 @@
> +    BS("\x03"),
> +    2048,
> +  },
> +  { // A realistically-sized modulus and a common exponent (65537)
> +    BS("\x7F"),

Copy/paste error here? (this modulus doesn't look realistic)

@@ +190,5 @@
> +      TLV(der::INTEGER, BS("\x80")) +
> +      TLV(der::INTEGER, BS("\x03"))),
> +
> +  // Negative exponent is not accepted (note that it is odd, so that the test
> +  // makes sense even if we start rejecting even exponents in the future).

Maybe add a syntactically-valid testcase where the exponent is even in anticipation that it should fail if/when we start rejecting even exponents?

Comment 10

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

Attachment: Part 1 [v2]: Move CheckSubjectPublicKeyInfo to pkixkey.cpp

Comment 11

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

Attachment: Part 2: Pass more details of the public key to the TrustDomain

Review comments addressed.

The try build is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d042436be408

Comment 12

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

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #11)
> The try build is at
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d042436be408

Now https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3bb8d503ed7.

Comment 13

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

This is a bug worth fixing but I don't have the time to finish this, so unassigning myself.