Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1690123002:
Reduce Certificate Parsing Strictness (Closed)
DescriptionReduce Certificate Parsing Strictness
In order to allow for compatibility with existing certificates/OCSP
responders in the wild, the strictness on a few structures must be
reduced:
* Allow empty parameters in Signature Algorithms
BUG=
Committed: https://crrev.com/ed6e2ee578343dbc63fd6756f2b37bbaf1b163b8
Cr-Commit-Position: refs/heads/master@{#377722}
Patch Set 1 #Patch Set 2 : Fixing tests. #
Total comments: 12
Patch Set 3 : Updating comments and generalizing CL. #Patch Set 4 : Making VerifySerialNumber public. #Patch Set 5 : Adding note about non-strict parsing. #Patch Set 6 : Fixing unittest. #
Total comments: 12
Patch Set 7 : Updating comments. #Patch Set 8 : Removing 21 octet weakness. #
Dependent Patchsets: Messages
Total messages: 26 (10 generated)
svaldez@chromium.org changed reviewers: + eroman@chromium.org
Currently the top 100k Alexa sites have about a 7% rate of OCSP responses with invalid SignatureAlgorithms due to missing parameters instead of NULL parameters.
(1) You will need to update the unit tests (2) Should we only be lenient in the case when parsing signature algorithms for OCSP responses, rather then allowing this for certificates too? +rsleevi for discussion on the latter
On 2016/02/12 01:12:23, eroman wrote: > (1) You will need to update the unit tests > (2) Should we only be lenient in the case when parsing signature algorithms for > OCSP responses, rather then allowing this for certificates too? > > +rsleevi for discussion on the latter 1) Done 2) There's a worry that if some OCSP Responders encode this incorrectly, the same might be true for generated certificates, but if we've run the top 1M certificates and haven't found that they violate this part of the spec, it might be good to limit the leniency.
https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... File net/cert/internal/signature_algorithm.cc (right): https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm.cc:229: // ("PARAMS TYPE NULL ARE required"), however due to some non-compliance, we Can this be re-phrased without using a pronoun? See: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... File net/cert/internal/signature_algorithm_unittest.cc (right): https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm_unittest.cc:75: ASSERT_TRUE(ParseDer(kData, &algorithm)); Check the values of |algorithm| now that this expectation has changed. https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm_unittest.cc:131: ASSERT_TRUE(ParseDer(kData, &algorithm)); Same here. https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm_unittest.cc:243: ASSERT_TRUE(ParseDer(kData, &algorithm)); Same here. https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm_unittest.cc:280: ASSERT_TRUE(ParseDer(kData, &algorithm)); And here. https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm_unittest.cc:317: ASSERT_TRUE(ParseDer(kData, &algorithm)); ...
Description was changed from ========== Allow empty parameters in Signature Algorithms Around 7% of OCSP responders currently don't include NULL parameters in the signature algorithm. BUG= ========== to ========== Reduce Certificate Parsing Strictness In order to allow for compatibility with existing certificates/OCSP responders in the wild, the strictness on a few structures must be reduced: * Allow empty parameters in Signature Algorithms * Allow 21 octets in a serial number with a leading 0. BUG= ==========
Done, I'm actually going to generalize this CL to include a couple other less strict parsing changes based on the certificates/OCSP responses seen, and we can use this CL to discuss. The current set is: * Empty parameters in Signature Algorithms (Sufficient OCSP Responders do it incorrectly) * Allow 21 octet Serial Number in Certificates (The spec isn't clear whether the 20 octet limit should be the representation of the Integer or the actual Integer value since there needs to be a leading 0 to make it non-negative) https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... File net/cert/internal/signature_algorithm.cc (right): https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm.cc:229: // ("PARAMS TYPE NULL ARE required"), however due to some non-compliance, we On 2016/02/12 21:09:44, eroman wrote: > Can this be re-phrased without using a pronoun? See: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... Done. https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... File net/cert/internal/signature_algorithm_unittest.cc (right): https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm_unittest.cc:75: ASSERT_TRUE(ParseDer(kData, &algorithm)); On 2016/02/12 21:09:44, eroman wrote: > Check the values of |algorithm| now that this expectation has changed. Done. https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm_unittest.cc:131: ASSERT_TRUE(ParseDer(kData, &algorithm)); On 2016/02/12 21:09:44, eroman wrote: > Same here. Done. https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm_unittest.cc:243: ASSERT_TRUE(ParseDer(kData, &algorithm)); On 2016/02/12 21:09:44, eroman wrote: > Same here. Done. https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm_unittest.cc:280: ASSERT_TRUE(ParseDer(kData, &algorithm)); On 2016/02/12 21:09:44, eroman wrote: > And here. Done. https://codereview.chromium.org/1690123002/diff/20001/net/cert/internal/signa... net/cert/internal/signature_algorithm_unittest.cc:317: ASSERT_TRUE(ParseDer(kData, &algorithm)); On 2016/02/12 21:09:44, eroman wrote: > ... Done.
Description was changed from ========== Reduce Certificate Parsing Strictness In order to allow for compatibility with existing certificates/OCSP responders in the wild, the strictness on a few structures must be reduced: * Allow empty parameters in Signature Algorithms * Allow 21 octets in a serial number with a leading 0. BUG= ========== to ========== Reduce Certificate Parsing Strictness In order to allow for compatibility with existing certificates/OCSP responders in the wild, the strictness on a few structures must be reduced: * Allow empty parameters in Signature Algorithms * Allow 21 octets in a serial number with a leading 0. BUG= ==========
Adding TODO to match the ones in the other CL. From the initial runs, these seem to be the only other non-strict behavior we actually see in the top 1M.
Seems fine to me, however Ryan may want to comment as he was opposed to supporting 21 byte serial numbers for certificates (even though our data did turn some up that used it). Also can you provide the evidence for justifying this? - How frequent is your data showing this to be? - What CAs/responders? - What algorithms? Is it for instance specifically RSA SHA1 (in case we can ratchet down further). lgtm after addressing these comments... https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... File net/cert/internal/parse_certificate.cc (right): https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... net/cert/internal/parse_certificate.cc:162: // Allow an extra octet if the first octet is 0, since the serial number may This contradicts the comment in header. https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... net/cert/internal/parse_certificate.cc:163: // not be negative and may require a prefixed 0 octet. The comment should explain why. Maybe more like: RFC 5280 says that serial numbers must be no more than 20 octets long. However a number of certificates have been issued with 21 octets where the leading octet is 0. https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... net/cert/internal/parse_certificate.cc:164: // TODO: Add warning about non-strict parsing. TODO(svaldez): https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... File net/cert/internal/parse_certificate.h (right): https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... net/cert/internal/parse_certificate.h:33: // * No more than 20 octets are used. This is no longer true after your change. https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/sign... File net/cert/internal/signature_algorithm.cc (right): https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/sign... net/cert/internal/signature_algorithm.cc:230: // allowed for compatibility with non-compliant responders: responders --> OCSP responders https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/sign... net/cert/internal/signature_algorithm.cc:265: // TODO: Add warning about non-strict parsing. TODO(svaldez):
svaldez@chromium.org changed reviewers: + rsleevi@chromium.org
For the 21 vs 20 octets, the CAs we've seen that do it badly are Certinomis, Postecom, and TrustTechnologies.it. For the Signature Algorithms, its mostly Sha256, but also some Sha1, and some Sha512. Unless my metrics are off, it looks like this shows up about 7% of the time. https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... File net/cert/internal/parse_certificate.cc (right): https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... net/cert/internal/parse_certificate.cc:162: // Allow an extra octet if the first octet is 0, since the serial number may On 2016/02/23 22:09:52, eroman wrote: > This contradicts the comment in header. Acknowledged. https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... net/cert/internal/parse_certificate.cc:163: // not be negative and may require a prefixed 0 octet. On 2016/02/23 22:09:52, eroman wrote: > The comment should explain why. Maybe more like: > > RFC 5280 says that serial numbers must be no more than 20 octets long. However a > number of certificates have been issued with 21 octets where the leading octet > is 0. Done. https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... net/cert/internal/parse_certificate.cc:164: // TODO: Add warning about non-strict parsing. On 2016/02/23 22:09:52, eroman wrote: > TODO(svaldez): Done. https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... File net/cert/internal/parse_certificate.h (right): https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/pars... net/cert/internal/parse_certificate.h:33: // * No more than 20 octets are used. On 2016/02/23 22:09:52, eroman wrote: > This is no longer true after your change. Done. https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/sign... File net/cert/internal/signature_algorithm.cc (right): https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/sign... net/cert/internal/signature_algorithm.cc:230: // allowed for compatibility with non-compliant responders: On 2016/02/23 22:09:52, eroman wrote: > responders --> OCSP responders Done. https://codereview.chromium.org/1690123002/diff/100001/net/cert/internal/sign... net/cert/internal/signature_algorithm.cc:265: // TODO: Add warning about non-strict parsing. On 2016/02/23 22:09:52, eroman wrote: > TODO(svaldez): Done.
lgtm
Not LGTM for serial We should explicitly reject those. Firefox does as well. I've been haranguing on CAs that do this - we should have minimal to no real compat risk.
Description was changed from ========== Reduce Certificate Parsing Strictness In order to allow for compatibility with existing certificates/OCSP responders in the wild, the strictness on a few structures must be reduced: * Allow empty parameters in Signature Algorithms * Allow 21 octets in a serial number with a leading 0. BUG= ========== to ========== Reduce Certificate Parsing Strictness In order to allow for compatibility with existing certificates/OCSP responders in the wild, the strictness on a few structures must be reduced: * Allow empty parameters in Signature Algorithms BUG= ==========
Removing the 21 octet exception then.
On 2016/02/25 20:58:40, svaldez wrote: > Removing the 21 octet exception then. Sorry bout not seeing it until too late - missed this in the email queue
lgtm
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/1690123002/#ps140001 (title: "Removing 21 octet weakness.")
The CQ bit was checked by svaldez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690123002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690123002/140001
Message was sent while issue was closed.
Description was changed from ========== Reduce Certificate Parsing Strictness In order to allow for compatibility with existing certificates/OCSP responders in the wild, the strictness on a few structures must be reduced: * Allow empty parameters in Signature Algorithms BUG= ========== to ========== Reduce Certificate Parsing Strictness In order to allow for compatibility with existing certificates/OCSP responders in the wild, the strictness on a few structures must be reduced: * Allow empty parameters in Signature Algorithms BUG= ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Reduce Certificate Parsing Strictness In order to allow for compatibility with existing certificates/OCSP responders in the wild, the strictness on a few structures must be reduced: * Allow empty parameters in Signature Algorithms BUG= ========== to ========== Reduce Certificate Parsing Strictness In order to allow for compatibility with existing certificates/OCSP responders in the wild, the strictness on a few structures must be reduced: * Allow empty parameters in Signature Algorithms BUG= Committed: https://crrev.com/ed6e2ee578343dbc63fd6756f2b37bbaf1b163b8 Cr-Commit-Position: refs/heads/master@{#377722} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ed6e2ee578343dbc63fd6756f2b37bbaf1b163b8 Cr-Commit-Position: refs/heads/master@{#377722} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
