1130754 - Calculate the tbsCertificate digest at most once per path building step

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Right now, TrustDomain::VerifySignedData is responsible for calculating the digest on the given data and verifying the public key signature on that digest. But, if there are multiple candidate certificates, then there's no reason to calculate the digest multiple times.

In reality, it is probably very rare that there are multiple candidate certificates with matching subject names and that are otherwise acceptable, except that they have different public keys, so this isn't much of an optimization.

However, passing the digest to CheckPublicKey makes TrustDomain implementation simpler and also enables other upcoming optimizations.

Comment 1

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

Attachment: avoid-recalculating-tbsCertificate-digest.patch

Comment 2

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

Comment on attachment 8560934 [details] [diff] [review]
avoid-recalculating-tbsCertificate-digest.patch

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

::: security/pkix/include/pkix/pkixtypes.h
@@ +288,5 @@
> +  // VerifyRSAPKCS1SignedDigest *is* responsible for doing the mathematical
> +  // verification of the public key validity as specified in NIST SP 800-56A.
> +  virtual Result VerifyRSAPKCS1SignedDigest(
> +                   const SignedDigest& signedDigest,
> +                   Input subjectPublicKeyInfo) = 0;

Note: It isn't strictly necessary to split VerifySignedData into the RSA-specific and ECDSA-specific methods in this patch. However, my next patch, for bug 1130753, requires this split, because there will be an RSA-specific parameter and an ECDSA-parameter, respectively, to each method. To make things simpler, I just did it in this patch. Also, if I kept them as a single method, I'd have to expose the der::PublicKeyAlgorithm type in pkixtypes.h, which I'd rather not do, because I'd have to change that back to the way it is in this patch after the patch for bug 1130753.

Comment 3

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

Comment on attachment 8560934 [details] [diff] [review]
avoid-recalculating-tbsCertificate-digest.patch

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

LGTM. I found one particularly important typo (see the 5th comment).

::: config/external/nss/nss.def
@@ -676,5 @@
>  VFY_DestroyContext
>  VFY_End
>  VFY_Update
>  VFY_VerifyData
>  VFY_VerifyDataDirect

Looks like we can remove VFY_VerifyDataDirect now.

::: security/pkix/lib/pkixder.cpp
@@ +254,5 @@
> +  Result rv = AlgorithmIdentifierValue(input, oidValue);
> +  if (rv != Success) {
> +    return rv;
> +  }
> +  return SignatureAlgorithmOIDValue(oidValue, publicKeyAlgorithm,

I think we should check End(oidValue) after calling this, even though it will implicitly be guaranteed by SignatureAlgorithmOIDValue using MatchRest.

@@ +268,5 @@
> +    Result rv = AlgorithmIdentifierValue(r, oidValue);
> +    if (rv != Success) {
> +      return rv;
> +    }
> +    return DigestAlgorithmOIDValue(oidValue, algorithm);

Similarly, even though DigestAlgorithmOIDValue implicitly will guarantee that oidValue has been read to the end with MatchRest, I think it would be good to check that with End(oidValue).

::: security/pkix/lib/pkixnss.cpp
@@ -100,5 @@
> -  // The static_cast is safe as long as the length of the data in sd.data can
> -  // fit in an int. Right now that length is stored as a uint16_t, so this
> -  // works. In the future this may change, hence the assertion.
> -  // See also bug 921585.
> -  static_assert(sizeof(decltype(sd.data.GetLength())) < sizeof(int),

It occurs to me we should maybe be doing a check like this in UnsafeMapInputToSECItem.

@@ +107,5 @@
> +{
> +  SECOidTag oid;
> +  size_t bits;
> +  switch (digestAlg) {
> +    case DigestAlgorithm::sha512: oid = SEC_OID_SHA384; bits = 512; break;

SEC_OID_SHA512, right?

::: security/pkix/lib/pkixverify.cpp
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This code is made available to you under your choice of the following sets
> +* of licensing terms:

Looks like the '*' in the license blocks are all aligned in other files.

::: security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp
@@ +74,5 @@
>  
> +  Result VerifyRSAPKCS1SignedDigest(const SignedDigest& signedDigest,
> +                                    Input subjectPublicKeyInfo) override
> +  {
> +    return TestVerifyRSAPKCS1SignedDigest(signedDigest, subjectPublicKeyInfo);

Looks like we originally ensured that the signature verification code never ran for these tests - should we keep that behavior?

Comment 4

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

Comment on attachment 8560934 [details] [diff] [review]
avoid-recalculating-tbsCertificate-digest.patch

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e39cbc525ad

::: config/external/nss/nss.def
@@ -676,5 @@
>  VFY_DestroyContext
>  VFY_End
>  VFY_Update
>  VFY_VerifyData
>  VFY_VerifyDataDirect

Done.

::: security/pkix/lib/pkixder.cpp
@@ +254,5 @@
> +  Result rv = AlgorithmIdentifierValue(input, oidValue);
> +  if (rv != Success) {
> +    return rv;
> +  }
> +  return SignatureAlgorithmOIDValue(oidValue, publicKeyAlgorithm,

I refactored the code so that the redundant check won't be necessary, by inlining SignatureAlgorithmOIDValue into the function. I originally wanted to do the inlining anyway, but I thought the approach I took would be easier to review.

@@ +268,5 @@
> +    Result rv = AlgorithmIdentifierValue(r, oidValue);
> +    if (rv != Success) {
> +      return rv;
> +    }
> +    return DigestAlgorithmOIDValue(oidValue, algorithm);

Ditto, I inlined DigestAlgorithmOIDValue into the function.

::: security/pkix/lib/pkixnss.cpp
@@ -100,5 @@
> -  // The static_cast is safe as long as the length of the data in sd.data can
> -  // fit in an int. Right now that length is stored as a uint16_t, so this
> -  // works. In the future this may change, hence the assertion.
> -  // See also bug 921585.
> -  static_assert(sizeof(decltype(sd.data.GetLength())) < sizeof(int),

I added it:

  static_assert(sizeof(decltype(input.GetLength())) <= sizeof(result.len),
                "input.GetLength() must fit in a SECItem");

@@ +107,5 @@
> +{
> +  SECOidTag oid;
> +  size_t bits;
> +  switch (digestAlg) {
> +    case DigestAlgorithm::sha512: oid = SEC_OID_SHA384; bits = 512; break;

Oops! Thanks! Fixed.

::: security/pkix/lib/pkixverify.cpp
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This code is made available to you under your choice of the following sets
> +* of licensing terms:

Fixed.

::: security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp
@@ +74,5 @@
>  
> +  Result VerifyRSAPKCS1SignedDigest(const SignedDigest& signedDigest,
> +                                    Input subjectPublicKeyInfo) override
> +  {
> +    return TestVerifyRSAPKCS1SignedDigest(signedDigest, subjectPublicKeyInfo);

I reverted back to the previous behavior. Good catch.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e39cbc525ad

Comment 6

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

Attachment: Follow-up: Make der::PublicKeyAlgorithm an enum class

Comment 7

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

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #6)
> Created attachment 8565121 [details] [diff] [review]
> Follow-up: Make der::PublicKeyAlgorithm an enum class

The first part was already checked in. Only this follow-up needs to be checked in.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05b27953c865

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05b27953c865