Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 14 users
Status: Fixed
Owner:
Closed: Sep 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug-Regression



Sign in to add a comment
Unable to use client certificate with negative modulus
Reported by jaan.mur...@gmail.com, Sep 15 2015 Back to list
UserAgent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.22 Safari/537.36

Steps to reproduce the problem:
1. Chrome 46
2. Estonian ID-card 3.5 version
3. Try to authenticate to testsite for example https://sk.ee/tervitus
4. Authentication fails: certificate selection dialog is displayed but after that it fails without pin entry with error ERR_SSL_PROTOCOL_ERROR

From log
[5904:3352:0910/155650:WARNING:ssl_client_socket_openssl.cc(1804)] Failed to set client certificate
[5904:3352:0910/155650:WARNING:openssl_ssl_util.cc(148)] Unmapped error reason: 269
[5904:3532:0910/155650:VERBOSE1:navigator_impl.cc(168)] Failed Provisional Load: https://sk.ee/id/index2.php, error_code: -107, error_description: SSL protocol error., showing_repost_interstitial: 0, frame_id: 1

What is the expected behavior?
Authentication should be success.

What went wrong?
https://boringssl.googlesource.com/boringssl/+/c0e245a546b15c6b4219d2f3d5455e417cddc782
commit has changes what we belive introduced more strict check on certificate public key. Openssl still has more loose check on same public key.

One such test-certificate attached.

Did this work before? Yes chrome 45

Chrome version: 46.0.2490.22  Channel: beta
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: Shockwave Flash 19.0 r0

In Estonia there are 100 000+ such ID-cards and without any change with chrome 46 those card owners could not use chrome any more for every day usage. So this is urgent issue for them.

Is there possible to make temporary (for 5 years) workaround for such cards in chrome 46 and beyond?
 
47101010033.cer
1.8 KB Download
About workaround for Estonian ID-cards:
There was once issue with old Estonian ID-cards with TLS 1.2 where eventually was possible to add workaround for Estonian ID-cards https://code.google.com/p/chromium/issues/detail?id=278370
Comment 2 by jww@chromium.org, Sep 15 2015
Cc: agl@chromium.org
Labels: Cr-Internals-Network-SSL
Owner: davidben@chromium.org
Status: Assigned
davidben@, can you triage appropriately? Thanks!
Labels: -Restrict-View-SecurityTeam -Type-Bug-Security Type-Bug
Not a security issue. Removing security labels.


Aargh. Supporting incorrect encodings of things would likely mean bleeding this tolerance of broken things all the way into very low-level function RSAPublicKey parsing function. That's kind of ugly.

The other Estonian ID card bug was not in such a low-level function. (And indeed it too later caused us minor problems!)

If we're to work around this, I wonder if we can do it in a relatively targeted way at least. Might be a nuisance with the number of certificate parsing codepaths there are. It would be nice if RSA keys were not forever allowed to be negative, but perhaps this battle is a lost cause.


Which Estonian ID cards are broken like this? Is it all of them or just ones from a particular vendor or time period? Are at least the new ones going forward functional? How upgrade-able is an Estonian card? What's the lifetime of one?
Labels: -Type-Bug Type-Bug-Regression M-46
(Adding M-46 and regression labels so we don't forget about this.)
It is far from nice, indeed. Answers to your questions:

>Which Estonian ID cards are broken like this? 
actually it is not a “broken card”, it is the certificate(s) on the card

>Is it all of them or just ones from a particular vendor or time period? 
the ones issued from Sep 2014 till yesterday

>Are at least the new ones going forward functional? 
yes

> What's the lifetime of one?
5 years


The sneaky OpenSSL reports the zero in the certificat dump (openssl x509 -text) while not being there in the actual ASN.1 representation (dumpasn1 -v), so I suspect that the issue (not triggered on other platforms or browsers, at least not until now) might be present in other setups as well.


Alright, good to know it's fixed going forward!

I expect we'll have quite a lot of fallout if we don't fix this. 46 has already branched, so I think the first thing to do is to make the RSAPublicKey parsing just uniformly lenient (if not overall, at least in d2i_X509) and merge that to 46. We weren't really enforcing things before and our certificate parsing bits are very far from unified right now, so things aren't really worse than they were in 45.

Going forward, I actually don't really care all that much about strictness of client certificates as we're not really interpreting them. It's the server certificates that I wouldn't like to keep this around for five years for. mozilla::pkix, by inspection, DOES enforce that RSA moduli are positive, which suggests there won't be compatibility issues there.

Once Chromium's certificate handling code is somewhat more uniform, we'll be in a better position to parse client certificates differently from others. In so far as we need to parse them at all. We need to get them into a net::X509Certificate structure and then into whatever BoringSSL accepts as input, eventually to be decoupled from crypto/x509. And then we can, if we want, remove the laxness from d2i_X509 for whatever's still using it, and limit it to just client certificates from Chromium as nothing else is likely to touch an Estonian ID.
So what's the plan, anything that we could do from this side? I absolutely agree on having the code in place for doing the right thing - strict checking - but enforcing this on client certificates at this point of time can disrupt many others as well. 

Unfortunately my current capability does not include deeper discussions of Chromium/BoringSSL code paths or codebase, but that can be achieved if necessary.

I guess https://www.chromium.org/developers/calendar is out of sync?


Cc: tinazh@chromium.org
Status: Started
martin.paljak: This CL will around this.
https://boringssl-review.googlesource.com/#/c/5894/

I'll probably ask you to verify the workaround in canary later this week and then I'll request a merge for M46. On your end, if there's anything you can do to get the offending certificates out of circulation sooner than 2019, that would be great. We should be able to route some quirk into just client cert code eventually, but this is obviously not ideal.


+tinazh: FYI, we'll probably want to merge this workaround to M46 branch. (The original change is going to be difficult to revert at this point.)
Project Member Comment 9 by bugdroid1@chromium.org, Sep 15 2015
The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl.git/+/231cb8214511ea5784dd94a59f3e5f5fb7d39f8e

commit 231cb8214511ea5784dd94a59f3e5f5fb7d39f8e
Author: David Benjamin <davidben@chromium.org>
Date: Tue Sep 15 20:47:35 2015

Work around broken Estonian smart cards. Again.

Estonian IDs issued between September 2014 to September 2015 are broken and use
negative moduli. They last five years and are common enough that we need to
work around this bug.

Add parallel "buggy" versions of BN_cbs2unsigned and RSA_parse_public_key which
tolerate this mistake, to align with OpenSSL's previous behavior. This code is
currently hooked up to rsa_pub_decode in RSA_ASN1_METHOD so that d2i_X509 is
tolerant. (This isn't a huge deal as the rest of that stack still uses the
legacy ASN.1 code which is overly lenient in many other ways.)

In future, when Chromium isn't using crypto/x509 and has more unified
certificate handling code, we can put client certificates under a slightly
different codepath, so this needn't hold for all certificates forever. Then in
September 2019, when the broken Estonian certificates all expire, we can purge
this codepath altogether.

BUG= 532048 

Change-Id: Iadb245048c71dba2eec45dd066c4a6e077140751
Reviewed-on: https://boringssl-review.googlesource.com/5894
Reviewed-by: Adam Langley <agl@google.com>

[modify] http://crrev.com/231cb8214511ea5784dd94a59f3e5f5fb7d39f8e/crypto/bn/bn_asn1.c
[modify] http://crrev.com/231cb8214511ea5784dd94a59f3e5f5fb7d39f8e/crypto/bn/bn_test.cc
[modify] http://crrev.com/231cb8214511ea5784dd94a59f3e5f5fb7d39f8e/crypto/evp/p_rsa_asn1.c
[modify] http://crrev.com/231cb8214511ea5784dd94a59f3e5f5fb7d39f8e/crypto/rsa/rsa_asn1.c
[modify] http://crrev.com/231cb8214511ea5784dd94a59f3e5f5fb7d39f8e/crypto/rsa/rsa_test.cc
[modify] http://crrev.com/231cb8214511ea5784dd94a59f3e5f5fb7d39f8e/include/openssl/bn.h
[modify] http://crrev.com/231cb8214511ea5784dd94a59f3e5f5fb7d39f8e/include/openssl/rsa.h

> so I think the first thing to do is to make the RSAPublicKey parsing just uniformly lenient (if not overall, at least in d2i_X509)

To clarify, I did NOT make it uniformly lenient. There's a separate RSA_parse_public_key_buggy which is only called from the certificate-based parsing code. That code is still using the legacy OpenSSL ASN.1 stack which means it's horribly lenient in many other ways anyway. Getting off that code is much further out (issue #499653).
Great! I'll be watching this place for the heads-up about the canary build. And maybe switch permanently to the beta channel...

FYI: crrev.com links give 404 to me.
crrev.com: Yeah, bugdroid gets the URLs wrong for BoringSSL. I should see about fixing that one of these days...
Comment 13 by tin...@google.com, Sep 15 2015
FYI: Once the fix is ready and safe to merge, pls label it with Merge-Request-46, then it'll show in my M46 merge review and approval queue.
Labels: -Arch-x86_64 Needs-Feedback
martin, jaan: While I realize you may not represent sk.ee, I'd like to get your help in understanding something.

The public keys in the certificates encode a negative moduli.
At face value, a negative moduli is not a valid in an RSA public key.
At best, these are improperly-encoded DER.
RFC 3280 requires properly encoded DER, as does ITU-T X.509 & X.690
RFC 5280 requires properly encoded DER, as does ITU-T X.509 & X.690
ETSI TS 102 042 requires conformance to RFC 5280 and ITU-T X.509 (Section 7.3.3 of ETSI TS 102 042 v2.4.1), which is the most recently conducted audit and audit criteria for sk.ee, pursuant to https://www.sk.ee/en/repository/audit/
The following audit, https://www.sk.ee/upload/files/Audit%20Conclusion%20-%20Audit%20according%20to%20ETSI%20TS%20102%20042%20standard%20and%20CAB%20Guidelines%202015.pdf , covers the period in question where these certificates were issued.
The CPS governing sk.ee for this time period is https://sk.ee/upload/files/SK_CPS_en_v2_5.pdf . Section 7.1 requires that all certificate profiles conforming to this CPS must be composed in accordance with the requirements of RFC 3280.

It would seem each of these certificates fails to conform to the ETSI TS 102 042 policies (for which sk.ee was audited), which would invalidate them for use as QCP-SSD/QCP/NCP, nor would they conform to the sk.ee CPS in force at this time.

Do you think the above is a fair and factual summary of the requirements and policies in force?

If so, wouldn't all of these certificates need to be revoked, per sk.ee's CPS?

I'm asking these questions because I believe five years to be an unreasonable request to continue to accept invalid encodings. I'd like to better understand the organizational environment surrounding the issuance of these certificates, as well as find a more reasonable timeframe that we might be able to remove the acceptance of malformed certificates. If these conclusions hold, and the certificates are required to be revoked by the policies the CA has set out, then it seems that it may be more reasonable to remove this workaround in several months to half a year, allowing the time necessary for the replacement of these certificates.
rsleevi: not representing sk.ee and IANAL, but let me answer some of your questions.

> Do you think the above is a fair and factual summary of the requirements and policies in force?

Looks logical and accurate, yes.

> If so, wouldn't all of these certificates need to be revoked, per sk.ee's CPS?

Reading the mentioned CPS, section 4.6.1 ("Authority to Revoke Certificates"): revocation can basically be either initiated by the cardholder or by someone representing the law, which in this case means acting upon Identity Documents Act.

The Act, in turn, does talk about revocation (https://www.riigiteataja.ee/en/eli/ee/513042015004/consolide/current#para13lg1b1): that it is possible if there is a security concern. Which is not the case currently. Encoding error is a nuisance, yes, but not a security issue.

So basically: in case of a security issue certificates are revoked; in case of other issues they can be recalled for renewal.

> I'm asking these questions because I believe five years to be an unreasonable request to continue to accept invalid encodings.

Absolutely agree on this, the workaround should be removed as soon as possible. It seems that there is a lot of code out there that depends (or at least tolerates) on the lenient interpretation as found in OpenSSL - none of the real life applications I know of has had issues before.

AFAIK, no more certificates with incorrect encoding are being generated and the renewal of the issued ones is being planned. It shall require time, less than 5 years but obviously not a month or two, due to the sheer number of the cards our there.

The conclusion is: it's a bug, it's being dealt with, the fix takes some time to propagate, after what the strict coding check workaround should be removed.

> It seems that there is a lot of code out there that depends (or at least tolerates) on the lenient interpretation as found in OpenSSL - none of the real life applications I know of has had issues before.

While a lot of ASN.1 parsers are of low quality and absurdly lenient, newly written ones tend not to be. By inspection neither Go's implementation and mozilla::pkix (already used in Firefox for server certificates) are not tolerant of this mistake. (And our new one too, until the bug had to be reintroduced on behalf of sk.ee.)

The latter especially suggests that this is not required in practice for server certificates. To the best of my knowledge, this bug is unique to sk.ee.

But of course, as you note, OpenSSL is one of those lenient ones and thus it is especially important that we get rid of (or at least limit the scope of) this as soon as possible, so that future organizations do not follow your path. Any time that the workaround stays active for is risk that problems continue to grow undetected.


> It shall require time, less than 5 years but obviously not a month or two, due to the sheer number of the cards our there.

A month or two to five years is a rather wide range. Can you please give us an estimate for how long you expect to take to correct the misissuance?
Good question. Given the timescale in rsleevi's post it would be more likely on the half year side. I'll update once I know more.
davidben: But please let me know when I could try something. Right now I have google-chrome-* version 45.0.2454.93-1, 46.0.2490.33-1 and 47.0.2508.0-1 and only 45 works, as expected.
The change is in BoringSSL, but we haven't incorporated it into Chromium yet. Do you have a confirmation on when you can reissue the certificates by?
6 months seems like a realistic target.

Alright, thanks! Can you make sure to get appropriate confirmation of this deadline? I'll move forward for now assuming 6 months (plus a tad, depending on how Chrome's releases line up) is the target for removal.
Project Member Comment 23 by bugdroid1@chromium.org, Sep 18 2015
The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl.git/+/c71567dd50b1915ce3a72be0d63ad598d06f4879

commit c71567dd50b1915ce3a72be0d63ad598d06f4879
Author: David Benjamin <davidben@chromium.org>
Date: Thu Sep 17 16:02:24 2015

Update the Estonian workaround comments.

Target date for removal of the workaround is 6 months.

BUG= 532048 

Change-Id: I402f75e46736936725575559cd8eb194115ab0df
Reviewed-on: https://boringssl-review.googlesource.com/5910
Reviewed-by: Adam Langley <agl@google.com>

[modify] http://crrev.com/c71567dd50b1915ce3a72be0d63ad598d06f4879/crypto/bn/bn_asn1.c
[modify] http://crrev.com/c71567dd50b1915ce3a72be0d63ad598d06f4879/crypto/evp/p_rsa_asn1.c
[modify] http://crrev.com/c71567dd50b1915ce3a72be0d63ad598d06f4879/crypto/rsa/rsa_asn1.c

martin, jaan: The workaround is in Chrome Canary now. Could you confirm it behaves as expected? Thanks!

https://www.google.com/chrome/browser/canary.html
martin, jaan: Can you please confirm that Chrome Canary is working now, so that I may request a merge before 46 hits stable?
davidben: Okay, it seems the preliminary tests by our software testing partner on this front are OK. Please do add the Merge-request-46 flag. I'll let you know as we progress on the process front!
Labels: Merge-Request-46
Hey TPMs,

Could we merge https://boringssl.googlesource.com/boringssl.git/+/231cb8214511ea5784dd94a59f3e5f5fb7d39f8e and https://boringssl.googlesource.com/boringssl.git/+/c71567dd50b1915ce3a72be0d63ad598d06f4879 into M46? (The second one's just changing comments.)

Estonian IDs issued in the past year were broken and encoded incorrectly. Some stricter code in M46 detected this misissuance, but we'll need to work around their bug for six months while they reissue things. The new code is somewhat messy to revert, so I think it's better to just merge the workaround. (The reporters have confirmed the workaround resolves it.)

Note: this is somewhat of a funny merge as it's in a separate repository. I've gone ahead and cherry-picked them onto branch 2490 of BoringSSL. We need to edit M46's DEPS file and change boringssl_revision from 12fe1b25ead258858309d22ffa9e1f9a316358d7 to a7a4063c10a3b92dee3fd91787757de5cf4dd127.

David
Comment 28 by tin...@google.com, Sep 21 2015
Labels: -Merge-Request-46 Merge-Review-46 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
Labels: -Merge-Review-46 Merge-Approved-46
Merge approved for M46 (branch 2490).
tinazh: This is going to require updating DEPS in M46. I'm not familiar with how one does this---I believe just modifying DEPS on the branch doesn't work. Do you mind giving me some pointers or making the change? (boringssl_revision needs to change from 12fe1b25ead258858309d22ffa9e1f9a316358d7 to a7a4063c10a3b92dee3fd91787757de5cf4dd127.)

Thanks!
Comment 31 Deleted
Comment 32 Deleted
Labels: -Restrict-View-Google
Deleted your comments, removing RVG. We shouldn't RVG this bug.
Project Member Comment 34 by bugdroid1@chromium.org, Sep 21 2015
Labels: -Merge-Approved-46 merge-merged-2490
The following revision refers to this bug:
  http://goto.ext.google.com/viewvc/chrome-internal?view=rev&revision=78735

------------------------------------------------------------------
r78735 | davidben@google.com | 2015-09-21T23:32:26.567673Z

-----------------------------------------------------------------
Status: Fixed
Project Member Comment 37 by bugdroid1@chromium.org, May 10
The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl/+/1d59f6e3e2f3acef7498635adad4de0540433f72

commit 1d59f6e3e2f3acef7498635adad4de0540433f72
Author: David Benjamin <davidben@google.com>
Date: Wed May 10 15:49:33 2017

Add a flag to toggle the buggy RSA parser.

It's about time we got rid of this. As a first step, introduce a flag,
so that some consumers may stage this change in appropriately.

BUG= chromium:534766 , chromium:532048 

Change-Id: Id53f0bacf5bdbf85dd71d1262d9f3a9ce3c4111f
Reviewed-on: https://boringssl-review.googlesource.com/16104
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

[modify] https://crrev.com/1d59f6e3e2f3acef7498635adad4de0540433f72/include/openssl/evp.h
[modify] https://crrev.com/1d59f6e3e2f3acef7498635adad4de0540433f72/crypto/evp/p_rsa_asn1.c

Sign in to add a comment