New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 621645 link

Starred by 2 users
Status: Fixed
Owner:
eroman@chromium.org
Closed: Jun 2016
Cc:
█ ryanchung@chromium.org
mfo...@chromium.org
taku...@chromium.org
powerb@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Add an option for parsing certificates with invalid serial numbers

Project Member Reported by eroman@chromium.org, Jun 20 2016 
Currently the certificate verification code in //net/cert/internal is strict in what sorts of serial number it accepts, adhering to RFC 5280's restriction that they be no longer than 20 bytes:

    Certificate users MUST be able to
    handle serialNumber values up to 20 octets.  Conforming CAs MUST NOT
    use serialNumber values longer than 20 octets.

There are some certificates which have serial numbers 21 octets long (these tend to be 20 octets of numerical data, and then a leading 0 to force its interpretation as being non-negative).

For compatibility reasons we would like to temporarily allow such certificates in some circumstances (notably, verifying cast device certs for which some ICAs have such serial numbers).

This can be worked around by adding an option for allowing this during parsing, and enabling it only for these scenarios but not in general.

Comment 1 by eroman@chromium.org, Jun 20 2016

Status: Started (was: Assigned)
https://codereview.chromium.org/2079273004/

Comment 2 by eroman@chromium.org, Jun 20 2016

Labels: -Type-Bug -Pri-3 M-52 Pri-1 Type-Bug-Regression
We want this fix in M52.

The regression happened starting with M51.
Project Member

Comment 3 by sheriffbot@chromium.org, Jun 21 2016

Labels: -M-52 M-53 MovedFrom-52
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 4 by eroman@chromium.org, Jun 21 2016

Components: -Internals>Network>SSL Internals>Network>Certificate
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 21 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c15c91eed960673eff22f0bec50cdab4fd950824

commit c15c91eed960673eff22f0bec50cdab4fd950824
Author: eroman <eroman@chromium.org>
Date: Tue Jun 21 17:10:21 2016

Allow Cast certificates to have serial numbers greater than 20 bytes, as well as non-minimal INTEGER encodings (invalid DER).

Previously such certificates were rejected, however there are some intermediate device certificates with serial numbers 21 bytes long, so make a temporary allowance.

BUG= 621645 

Review-Url: https://codereview.chromium.org/2079273004
Cr-Commit-Position: refs/heads/master@{#401019}

[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/components/cast_certificate/cast_cert_validator.cc
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/components/cast_certificate/cast_cert_validator_unittest.cc
[add] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/components/test/data/cast_certificate/certificates/intermediate_serialnumber_toolong.pem
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/cert_issuer_source_aia.cc
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/cert_issuer_source_aia_unittest.cc
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/cert_issuer_source_static_unittest.cc
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/parse_certificate.cc
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/parse_certificate.h
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/parse_certificate_fuzzer.cc
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/parse_certificate_unittest.cc
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/parse_ocsp.cc
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/parsed_certificate.cc
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/parsed_certificate.h
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/verify_certificate_chain_pkits_unittest.cc
[modify] https://crrev.com/c15c91eed960673eff22f0bec50cdab4fd950824/net/cert/internal/verify_certificate_chain_unittest.cc

Comment 6 by eroman@chromium.org, Jun 21 2016

Status: Fixed (was: Started)

Comment 7 by eroman@chromium.org, Jun 21 2016

Labels: -M-53 -MovedFrom-52 M-52
I will let this bake for at least 1 Canary release before requesting a merge to M52.

Comment 8 by eroman@chromium.org, Jun 23 2016

Labels: Merge-Request-52
Would like to merge to M52.

This fixes a regression whereby certain Cast devices could not be used in Chrome (due to using an intermediate with an invalid serial number).

Comment 9 by dimu@google.com, Jun 23 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 24 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/36b7414ef935eb3a60ff3d934d722e5eedf055ca

commit 36b7414ef935eb3a60ff3d934d722e5eedf055ca
Author: Eric Roman <eroman@chromium.org>
Date: Fri Jun 24 21:00:20 2016

Allow Cast certificates to have serial numbers greater than 20 bytes, as well as non-minimal INTEGER encodings (invalid DER).

Previously such certificates were rejected, however there are some intermediate device certificates with serial numbers 21 bytes long, so make a temporary allowance.

BUG= 621645 
Review-Url: https://codereview.chromium.org/2079273004
Cr-Commit-Position: refs/heads/master@{#401019}
(cherry picked from commit c15c91eed960673eff22f0bec50cdab4fd950824 -- with many merge conflicts resolved)

R=mattm@chromium.org

Review URL: https://codereview.chromium.org/2093223002 .

Cr-Commit-Position: refs/branch-heads/2743@{#469}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/components/cast_certificate/cast_cert_validator.cc
[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/components/cast_certificate/cast_cert_validator_unittest.cc
[add] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/components/test/data/cast_certificate/certificates/intermediate_serialnumber_toolong.pem
[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/net/cert/internal/parse_certificate.cc
[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/net/cert/internal/parse_certificate.h
[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/net/cert/internal/parse_certificate_fuzzer.cc
[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/net/cert/internal/parse_certificate_unittest.cc
[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/net/cert/internal/parse_ocsp.cc
[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/net/cert/internal/verify_certificate_chain.cc
[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/net/cert/internal/verify_certificate_chain.h
[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/net/cert/internal/verify_certificate_chain_pkits_unittest.cc
[modify] https://crrev.com/36b7414ef935eb3a60ff3d934d722e5eedf055ca/net/cert/internal/verify_certificate_chain_unittest.cc
►

Sign in to add a comment