Upstream already have a fix for this. Cherry-picked in https://boringssl-review.googlesource.com/#/c/7199/.

Could we be missing more of such security fixes ? Is it time to update to latest trunk ?

This wasn't marked as a security issue by upstream. We try to keep up to date with changes in any case, but we didn't have this one. (Sadly, even changes that upstream backport are often flawed.)

The long-term fix is not to use the ASN.1 code and the change for that was already in BoringSSL trunk but hadn't been rolled into Chromium yet.

Hrm. I remember I saw this one and was confused how one even got a negative zero to begin with. I fixed the places I found. We ought to import this patch (or maybe make it error), but I want to see how we even get a negative zero since that makes no sense.

Marking as fixed based on #3.

The fix isn't in yet. It will be shortly, but I'm trying to understand where the negative zero comes from first. And then it needs to roll into Chromium.

(Even that will be weird, because the next BoringSSL roll will get rid of the particular codepath hit by the fuzzer anyway. So we'll also want to see whether it's reachable in other ways. And then I'll need to figure out which releases we need to cherry-pick this to.)

The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl.git/+/c4eec0c16b02c97a62a95b6a08656c3a9ddb6baa

commit c4eec0c16b02c97a62a95b6a08656c3a9ddb6baa
Author: Adam Langley <agl@google.com>
Date: Mon Feb 29 16:01:05 2016

Fix encoding bug in i2c_ASN1_INTEGER

(Imported from upstream's 3661bb4e7934668bd99ca777ea8b30eedfafa871.)

Fix bug where i2c_ASN1_INTEGER mishandles zero if it is marked as
negative.

Thanks to Huzaifa Sidhpurwala <huzaifas@redhat.com> and Hanno Böck
<hanno@hboeck.de> for reporting this issue.

BUG= 590615 

Change-Id: I8959e8ae01510a5924862a3f353be23130eee554
Reviewed-on: https://boringssl-review.googlesource.com/7199
Reviewed-by: David Benjamin <davidben@google.com>

[modify] https://crrev.com/c4eec0c16b02c97a62a95b6a08656c3a9ddb6baa/crypto/asn1/a_int.c

From further invesitgation, the root problem as to how we're getting a negative zero (which *should* be nonsense), is that OpenSSL has another bug. (Sigh.) They put the negative flag in the tag, so V_ASN1_NEG_INTEGER is actually a tag of V_ASN1_INTEGER | V_ASN1_NEG = 2 | 0x100 = 258.

No one uses tags larger than 31, so this is "fine". Except that OpenSSL parses high tag number form, so it'll happily parse (in der-ascii syntax):

  [UNIVERSAL 258 PRIMITIVE] { `00` }

as negative zero. (ClusterFuzz actually found a case where the tag number was encoded with too many leading zeros and it was actually a:

  [UNIVERSAL 258 CONSTRUCTED] { OCTET_STRING { `00` } }

which is super-nonsense, but that's a red herring.) I believe, but am not confident, that this only affects structures with ASN1_ANY in them (which PKCS#8 PrivateKeyInfo shouldn't be, but that's another matter...). Decoding and then immediately re-encoding a PKCS8_PRIV_KEY_INFO is pretty weird and only something we did sparingly.

But X509_ALGOR has an ASN1_ANY in there, and we'll encode/decode that one a ton. I'll go merge Adam's fix to handle negative zero gracefully, and then also fix ASN1_get_object to never emit that tag because that's nonsense. This bug also affects upstream, but I'm not sure if one can do any mischief here beyond this issue which upstream has already fixed.

The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl.git/+/fb2c6f8c8565e1e2d85c24408050c96521acbcdc

commit fb2c6f8c8565e1e2d85c24408050c96521acbcdc
Author: David Benjamin <davidben@google.com>
Date: Mon Feb 29 20:42:59 2016

ASN1_get_object should not accept large universal tags.

The high bits of the type get used for the V_ASN1_NEG bit, so when used with
ASN1_ANY/ASN1_TYPE, universal tags become ambiguous. This allows one to create
a negative zero, which should be impossible. Impose an upper bound on universal
tags accepted by crypto/asn1 and add a test.

BUG= 590615 

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

[modify] https://crrev.com/fb2c6f8c8565e1e2d85c24408050c96521acbcdc/crypto/asn1/CMakeLists.txt
[modify] https://crrev.com/fb2c6f8c8565e1e2d85c24408050c96521acbcdc/crypto/asn1/asn1_lib.c
[add] https://crrev.com/fb2c6f8c8565e1e2d85c24408050c96521acbcdc/crypto/asn1/asn1_test.cc
[modify] https://crrev.com/fb2c6f8c8565e1e2d85c24408050c96521acbcdc/crypto/test/scoped_types.h
[modify] https://crrev.com/fb2c6f8c8565e1e2d85c24408050c96521acbcdc/include/openssl/asn1.h
[modify] https://crrev.com/fb2c6f8c8565e1e2d85c24408050c96521acbcdc/util/all_tests.json

This should be fixed on trunk now, as of https://chromium.googlesource.com/chromium/src/+/060ab3d05722f54e62fa8e7de6aedd6076d968ae

I believe this is reachable anywhere we deserialize and reserialize a structure in OpenSSL's legacy ASN.1 stack which contains an ASN1_TYPE. Unfortunately, there's one of those in X509_ALGOR, which is in certificates. (Yay OpenSSL ASN.1 code. Concerned folks may star issue #499653 to follow the long and grueling process to unship the worst code in BoringSSL other than the assembly.)

Severity-wise, we'll read off a malloc'd buffer backwards so long as we find zeros. We'll also *write* zeros backwards into another buffer so long as we can do that. I don't know how malloc implementations look and whether we know that reading *backwards* off a buffer will always hit a non-zero byte in malloc book-keeping somewhere. (It stops at the first non-zero value.)

If we are guaranteed it will terminate quickly, there will always be at least two bytes of scratch (tag + length) before the start of the buffer, and likely more. It needs to be large enough to successfully parse as whatever comes before the AlgorithmIdentifier. For a certificate, this is decently large. It's not quite so large for PKCS#8, but this is only reachable in the PKCS#8 codepath in M50. (And we'll only process untrusted PKCS#8 data in the sandbox.)

Anyway, both fixes are rather low risk, so I think I'll definitely request a merge to 50 tomorrow. Maybe 49 too? (Security folks?)

According to the severity guidelines (https://www.chromium.org/developers/severity-guidelines) it should be merged to a current stable (M-49?), but since we consider this as a low risk, might be fine to M-50.

Eh, it's a small enough change. May as well merge to both.

Hey TPMs, can we merge this change to M50 and M49? I would like to merge both of these changes:
https://boringssl.googlesource.com/boringssl.git/+/c4eec0c16b02c97a62a95b6a08656c3a9ddb6baa
https://boringssl.googlesource.com/boringssl.git/+/fb2c6f8c8565e1e2d85c24408050c96521acbcdc (most of these is a test, the meaningful diff is just the asn1_lib.c bit)

[Automated comment] Less than 2 weeks to go before stable on M49, manual review required.

Your change meets the bar and is auto-approved for M50 (branch: 2661)

Merged to branch 2661 with https://chromegw.corp.google.com/viewvc/chrome-internal?view=rev&revision=84725

(Am I using the right merge-merged label? Last time I had to do one of these merges, bugdroid didn't do it right, probably because it's funny.)

Merge approved for M49 (branch 2623). Stable candidate cut on Friday, 5PM PST. Please merge in your change ASAP.

sshrthi: Done.
https://chromegw.corp.google.com/viewvc/chrome-internal?view=rev&revision=84738

The following revision refers to this bug:
  http://goto.ext.google.com/viewvc/chrome-internal?view=rev&revision=84725

------------------------------------------------------------------
r84725 | davidben@google.com | 2016-03-03T20:18:40.139655Z

-----------------------------------------------------------------

The following revision refers to this bug:
  http://goto.ext.google.com/viewvc/chrome-internal?view=rev&revision=84738

------------------------------------------------------------------
r84738 | davidben@google.com | 2016-03-03T23:14:39.111674Z

-----------------------------------------------------------------

This bug has been closed for more than 14 weeks. Removing security view restrictions.

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

ClusterFuzz has detected this issue as fixed in range 378538:378726.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5188284487041024

Fuzzer: libfuzzer_boringssl_d2i_autoprivatekey_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Heap-buffer-overflow READ 1
Crash Address: 0x60300000ebcf
Crash State:
  i2c_ASN1_INTEGER
  asn1_ex_i2c
  asn1_i2d_ex_primitive
  
Recommended Security Severity: Medium

Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan&range=376310:376342
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan&range=378538:378726

Minimized Testcase (0.06 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94LUJli7uF5mlnOwi3FF7CPD2pqXFfrBQaVNpaN4cX9Y-L1_FcVHNCEHPZOR7MnnwkEFVMU2kPvMJJ7FYNCJ7dF2tlqwgL5rKwtejr28wqmfe1mRLS3S_aGbON-8UOELC_0NBzyf_VghFV61YRmCYyfIKsuow

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Just a few months late to the party there, ClusterFuzz. :-P

(It picked up a BoringSSL roll that disconnected the PKCS#8 parser from this function, which is what's expected. The function itself has, of course, also since been fixed.)

This bug has been closed for more than 14 weeks. Removing security view restrictions.

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

This bug has been closed for more than 14 weeks. Removing security view restrictions.

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