Closed
Bug 1131767
Opened 6 years ago
Closed 6 years ago
Prune away paths using unacceptable signature algorithms earlier
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
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)
21.25 KB,
patch

keeler
:
review+

Details  Diff  Splinter Review 
See the comments in the patch. This is an optimization that allows us to skip SHA1 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 toosmall RSA key or with an unacceptable digest algorithm. Note that Gecko currently doesn't have any code for SHA1 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 SHA1 deprecation plans. Gecko *does* have code for rejecting toosmall 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)
Assignee  
Updated•6 years ago

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 2•6 years ago


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 Driveby nit: s/their/there/ ?
Assignee  
Comment 3•6 years ago


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?
Assignee  
Comment 5•6 years ago


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+
Assignee  
Comment 7•6 years ago


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+
Assignee  
Comment 8•6 years ago


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+
Assignee  
Comment 9•6 years ago


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+
Assignee  
Comment 11•6 years ago


https://hg.mozilla.org/integration/mozillainbound/rev/99f4f2064520 I fixed the nits you pointed out. Thanks for the quick reviews!
Blocks: 1137484
You need to log in
before you can comment on or make changes to this bug.
Description
•