Closed Bug 1131767 Opened 9 years ago Closed 9 years ago

Prune away paths using unacceptable signature algorithms earlier

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox39 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

(Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(1 file, 4 obsolete files)

See the comments in the patch. This is an optimization that allows us to skip SHA-1 and RSA<2048 certificates during path building earlier on, saving lots of time.

Before this patch, mozilla::pkix only checks that the certificate's signature algorithm is acceptable when it verifies the certificate's signature. However, mozilla::pkix only verifies signatures at the very end of path building. Thus, mozilla::pkix will gladly build a path with one or more unacceptable signature algorithms, which takes time, and then only reject it at the end.

With this optimization, such paths are immediately skipped as soon as we see a certificate that was signed with a too-small RSA key or with an unacceptable digest algorithm.

Note that Gecko currently doesn't have any code for SHA-1 deprecation, so it currently considers all digest algorithms supported by mozilla::pkix to be acceptable. But, this patch lays some of the groundwork for Mozilla's presumed future SHA-1 deprecation plans.

Gecko *does* have code for rejecting too-small RSA signatures, particularly for EV validation. This patch will should immediately improve the performance of EV validation for some websites now. Also, when Mozilla decides to make RSA 2048 the minimum for DV too, this patch will make even more DV validations faster.

Note that this is just an optimization and there will be no compatibility impact.
Attachment #8562309 - Flags: review?(dkeeler)
Blocks: 1130753
No longer blocks: 1130653
Comment on attachment 8562309 [details] [diff] [review]
Prune away paths using unacceptable signature algorithms earlier

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

Looks good - just a couple of nits.

::: security/pkix/include/pkix/pkixtypes.h
@@ +276,5 @@
> +  // Return Success if the algorithm is acceptable,
> +  // Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED if the algorithm is not
> +  // acceptable, or another error code if another error occurred.
> +  virtual Result CheckSignatureDigestAlgorithm(DigestAlgorithm digestAlg) = 0;
> +    

nit: trailing whitespace

@@ +281,4 @@
>    // Check that the RSA public key size is acceptable.
>    //
> +  // This may be called twice for a CA certificate during path building: once
> +  // with an estimate with the public key size, based on the size of the

From the implementation/additional comments there, it looks like the estimate will always be greater than or equal to the actual size - should we mention that here?
Attachment #8562309 - Flags: review?(dkeeler) → review+
Comment on attachment 8562309 [details] [diff] [review]
Prune away paths using unacceptable signature algorithms earlier

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

::: security/pkix/lib/pkixcheck.cpp
@@ +97,5 @@
> +    case der::PublicKeyAlgorithm::RSA_PKCS1:
> +    {
> +      // The signature size is approximately the size of the public key, but it
> +      // may be smaller if the RSA computation gives a result that would have
> +      // leading zero bytes. The odds of their being 128 bits of leading zeros

Drive-by nit: s/their/there/ ?
I will fold these into the previous patch.
Attachment #8564630 - Flags: review?(dkeeler)
Comment on attachment 8564630 [details] [diff] [review]
Updates based on tryserver and review comments

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

::: security/pkix/include/pkix/pkixtypes.h
@@ +277,4 @@
>    // Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED if the algorithm is not
>    // acceptable, or another error code if another error occurred.
>    virtual Result CheckSignatureDigestAlgorithm(DigestAlgorithm digestAlg) = 0;
>      

Looks like there's some trailing whitespace here that would be nice to remove while we're here.

::: security/pkix/lib/pkixcheck.cpp
@@ +103,5 @@
>        // virtually impossible). Also, RSA public keys are almost always
>        // multiples of 256 bits and usually powers of two (1024, 2048, or 4096
>        // bits). With this in mind, we estimate the size of the public key by
>        // rounding up to the nearest multiple of 128 bits.
> +      unsigned int actualSignatureSizeInBits =

Why the change from size_t to unsigned int?
Comment on attachment 8564630 [details] [diff] [review]
Updates based on tryserver and review comments

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

::: security/pkix/include/pkix/pkixtypes.h
@@ +277,4 @@
>    // Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED if the algorithm is not
>    // acceptable, or another error code if another error occurred.
>    virtual Result CheckSignatureDigestAlgorithm(DigestAlgorithm digestAlg) = 0;
>      

I will remove it.

::: security/pkix/lib/pkixcheck.cpp
@@ +103,5 @@
>        // virtually impossible). Also, RSA public keys are almost always
>        // multiples of 256 bits and usually powers of two (1024, 2048, or 4096
>        // bits). With this in mind, we estimate the size of the public key by
>        // rounding up to the nearest multiple of 128 bits.
> +      unsigned int actualSignatureSizeInBits =

1. It isn't a size.

2. CheckRSAPublicKeyModulusSizeInBits takes an unsigned int. When sizeof(size_t) > sizeof(unsigned int), Clang warns about truncation and the build fails.
Comment on attachment 8564630 [details] [diff] [review]
Updates based on tryserver and review comments

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

Sounds good - thanks for explaining.
Attachment #8564630 - Flags: review?(dkeeler) → review+
I merged the two r+d patches together and rebased them on top of bug 1135407.
Attachment #8562309 - Attachment is obsolete: true
Attachment #8564630 - Attachment is obsolete: true
Attachment #8567555 - Flags: review+
Rebased again, getting rid of the use of __func__ that didn't work on anything but MSVC.
Attachment #8567555 - Attachment is obsolete: true
Attachment #8567580 - Flags: review+
1. I rebased this on top of the latest change in bug 1077864.

2. It turns out the rounding up behavior I implemented was not necessary, because RSA signatures are always padded to the RSA key size.

3. I added a test for #2 and updated the tests from bug 1077864.
Attachment #8567580 - Attachment is obsolete: true
Attachment #8567758 - Flags: review?(dkeeler)
Comment on attachment 8567758 [details] [diff] [review]
Prune away paths using unacceptable signature algorithms earlier [v4]

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

Ok - r=me with nits addressed.

::: security/pkix/include/pkix/pkixtypes.h
@@ +276,5 @@
> +  // Return Success if the algorithm is acceptable,
> +  // Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED if the algorithm is not
> +  // acceptable, or another error code if another error occurred.
> +  virtual Result CheckSignatureDigestAlgorithm(DigestAlgorithm digestAlg) = 0;
> +    

nit: trailing whitespace

::: security/pkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
@@ +176,4 @@
>      Success,
>    },
> +  { // Algorithms match, both are supported, key size is not a multile of 128
> +    // bits. This teste verifies that we're not wrongly rounding up the

s/teste/test
Attachment #8567758 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/99f4f2064520

I fixed the nits you pointed out.

Thanks for the quick reviews!
https://hg.mozilla.org/mozilla-central/rev/99f4f2064520
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.