Skip to content

Boringssl merge c7a3c46574e7fc32357b2cc68f961c56c72b0ca4#439

Merged
nebeid merged 74 commits intoaws:current-upstream-merge-20220318from
nebeid:boringssl-merge-c7a3c46574e7fc32357b2cc68f961c56c72b0ca4
Mar 28, 2022
Merged

Boringssl merge c7a3c46574e7fc32357b2cc68f961c56c72b0ca4#439
nebeid merged 74 commits intoaws:current-upstream-merge-20220318from
nebeid:boringssl-merge-c7a3c46574e7fc32357b2cc68f961c56c72b0ca4

Conversation

@nebeid
Copy link
Copy Markdown
Contributor

@nebeid nebeid commented Mar 23, 2022

Description of changes:

This change merges in BoringSSL commits up to google/boringssl@c7a3c46 on Mar 15, 2022:

  • The changes to the FIPS self-check test are integrated; the FFDH test with FB parameters was moved within boringssl_self_test_ffdh() so that it's lazily tested with the other test which uses ffdhe2048.

Call-outs:

  • The FFDH test with FB parameter still follows the old design of breaking self-test by re-compiling with BORINGSSL_FIPS_BREAK_FFC_DH defined but was changed to insert a fault in the input. The way to break it needs to be adjusted or removed.
  • Currently the fips breaking test does not run on the FIPS shared build in both AWS-LC and BoringSSL. It runs on BoringSSL FIPS static build with some errors in parsing the algorithm name in the test output on the commit that's at the tip of this merge: google/boringssl@c7a3c46.
  • After commit google/boringssl@c6e8f3e, the test fails on BoringSSL static build due to not finding a module version.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Adam Langley and others added 30 commits January 13, 2022 17:31
This change imports upstream's
openssl/openssl@c045224

Change-Id: Ib50ff9eb8c48d9580aa2ffcae92d3990cc987e30
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50905
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
140-3 says

> the zeroisation of protected and unprotected SSPs
> shall be performed in the following scenarios:
>   ...
>   For temporary value(s) generated during the integrity test of the
>   module’s software or firmware upon completion of the integrity test.

(IG 9.7.B)

Change-Id: I911f294860bf33b13b2c997fc633c9bda777fc48
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50945
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This matches our other free functions.

Fixed: 473
Change-Id: Ie147995c2f5b429f78e95cfc9a08ed54181af94e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51005
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Description:
Creating just a Gerrit password isn't enough.  Before you can push a
change to Gerrit, you must also create/associate a Gerrit account with
the google account used to create the password.

This avoids "git push ..." rejections like this:

  remote: PERMISSION_DENIED: The caller does not have permission
  remote: [type.googleapis.com/google.rpc.LocalizedMessage]
  remote: locale: "en-US"
  remote: message: "\'git push\' requires a Gerrit user account."

Change-Id: Id02c1a69ccb0c2b8bf4c63b77ed3064125966eb3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50985
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is designed to be the minimal infrastructure required to support
using BoringSSL in the Rust ecosystem without fear of ABI drift. Bindgen
is used to generate Rust bindings in lockstep with the rest of the
build. `rust-openssl` can consume these generated bindings with minimal
changes.

Change-Id: I1dacd36a4131e22a930ebb01da00407e8465ad7e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49645
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
See https://fuchsia-review.googlesource.com/c/fuchsia/+/624684. Also
pick up the new, more specific, name for ZX_ARM64_FEATURE_ISA_SHA2.

Update-Note: This CL is written assuming we can just rely on the SDK
changes. Per go/fuchsia-sdk-age, this seems fairly safe. If this file
fails to build due to missing symbols, update your project's Fuchsia
SDK. If this blocks something, let us know.

Change-Id: I28b0c234b577cc0de90e7ef096c15bb75a4ba501
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50926
Reviewed-by: Adam Langley <agl@google.com>
…ndings for the targeted Arch

Change-Id: I8ccd53bce0d73bd9d79f65770e544a75753ce4f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51025
Reviewed-by: David Benjamin <davidben@google.com>
We were fetching the mac-amd64 package even on mac-arm64.

Change-Id: Iad842ebd46d467c0def9bdbd14c77698a03f58d3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51045
Reviewed-by: Adam Langley <agl@google.com>
The check finds implicit conversions of integer literals to bools:
  bool b1 = 1;
  bool b2 = static_cast<bool>(1);
and transforms them to:
  bool b1 = true;
  bool b2 = true;

Bug: chromium:1290142
Change-Id: I15579e28f544d07b331a230b70a8278e0651150d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51085
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This hash table, in applications that use pooling, can dedup received
certificates in memory and thus should use a keyed hash.

Change-Id: Idc40dc8f7463025183121642b30ea0de43ebac0e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51125
Reviewed-by: Adam Langley <agl@google.com>
u8 strings in C++20 are char8_t instead of char; in order to compile on
both C++17 and C++20 we need to remove the prefix.

Change-Id: I85d1a9d72d24e8fa96ca22b1d99be9982fee8fb5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51065
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
POSIX feature macros are a mess. Reportedly, FreeBSD also breaks with
_XOPEN_SOURCE, so try leaving it unset by default.

Update-Note: It's possible this will break yet another obscure UNIX.
Hopefully we can eventually find a combination that works?

Bug: 471
Change-Id: I103f8093110d343789b9c5a22eb056ab78d9cd14
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51145
Reviewed-by: Adam Langley <agl@google.com>
Upstream used the macro originally, but later added a bespoke
implementation to transparently support X9.42 DH parameter serialization
(DHXPARAMS in OpenSSL) in 2ca873e8d898e8a232ea707227400213980059a4.

We don't support DHXPARAMS, so revert to the macro.

Change-Id: Ib17902c6c640bb88ee55881945ce57a032f7061b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51105
Reviewed-by: Adam Langley <agl@google.com>
This matches the source, which only builds support for these tests on
Linux. Note Android sets CMAKE_SYSTEM_NAME to "Android", so this covers
the previous ANDROID check.

Bug: 476
Change-Id: I41ca408706d0d0c5bb22006f4c31d51fc1267f69
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51165
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Description:
Mark Wooden and Franck Rondepierre noted that the square-root-mod-p
operations used in the EdDSA RFC (RFC 8032) can be simplified.  For
Ed25519, instead of computing u*v^3 * (u * v^7)^((p-5)/8), we can
compute u * (u*v)^((p-5)/8).  This saves 3 multiplications and 2
squarings.  For more details (including a proof), see the following
message from the CFRG mailing list:

  https://mailarchive.ietf.org/arch/msg/cfrg/qlKpMBqxXZYmDpXXIx6LO3Oznv4/

Testing:
Build and run the Ed25519 tests:

  mkdir build
  cd build
  cmake -GNinja ..
  ninja && ./crypto/crypto_test --gtest_filter="Ed25519Test*"

Numerical testing of the square-root computation can be done using the
following sage script:

  def legendre(x,p):
      return kronecker(x,p)

  # Ed25519
  p = 2**255-19
  # -1 is a square
  if legendre(-1,p)==1:
      print("-1 is a square")
  # 2 is a non-square
  if legendre(2,p)==-1:
      print("2 is a non-square")

  # 2 is a generator
  # this can be checked by factoring p-1
  # and then showing 2**((p-1)/q) != 1 (mod p)
  # for all primes q dividing p-1.

  # suppose u/v is a square.
  # to compute one of its square roots, find x such that
  #    x**4 == (u/v)**2 .
  # this implies
  #    x**2 ==  u/v, or
  #    x**2 == -(u/v) ,
  # which implies either x or i*x is a square-root of u/v (where i is a square root of -1).
  # we can take x equal to u * (u*v)**((p-5)/8).

  g = 2
  s = p>>2  # s = (p-1)/4
  i = power_mod(g, s, p)

  t = p>>3  # t = (p-5)/8
  COUNT = 1<<18
  while COUNT > 0:
      COUNT -= 1

      r = randint(0,p-1)   # r = u/v
      v = randint(1,p-1)
      u = mod(r*v,p)

      # compute x = u * (u*v)**((p-5)/8)
      w = mod(u*v,p)
      x = mod(u*power_mod(w, t, p), p)

      # check that x**2 == r, or (i*x)**2 == r, or r is not a square
      rr = power_mod(x, 2, p)
      if rr==r:
          continue

      rr = power_mod(mod(i*x,p), 2, p)
      if rr==r:
          continue

      if legendre(r,p) != 1:
          continue

      print("failure!")
      exit()

  print("passed!")

Change-Id: Iaa284d3365dd8c9fa18a4584121013f05a3f4cc6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50965
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
The ECH extension is not covered in the AAD and so should not be
referenced in ech_outer_extensions. We end up rejecting this anyway when
checking for valid ClientHelloInners, but better to reject this
explicitly, as the spec suggests.

As part of this, use the more specific error in the various tests, so we
can distinguish the two cases. (DECODE_ERROR is coming from an extra,
probably unnecessary, error in ssl_decode_client_hello_inner's caller.)

Bug: 275
Change-Id: Ibeff55e5e1b7646ce9c68c5847cd1b40a47e6480
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51185
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Both call sites end up calling them in succession. This saves a little
bit of code.

Bug: 275
Change-Id: Ib87bd9be446c368f77beb3b329deaa84ef43ac95
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51186
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
In testing out the ECH bits on the Chromium side, it is much harder to
tell what's going on without some indication that we sent a
ClientHelloInner. This CL routes it into the callback. A corresponding
CL in Chromium will add it to NetLog.

Bug: 275
Change-Id: I945ab2679614583e875a0ba90d6cf1481ed315d9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51205
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is the recommended way for detecting platforms:
https://docs.bazel.build/versions/main/platforms.html

Fixes cross-compilation with bazel-zig-cc.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Change-Id: Ifc7d2b502e01feb9cbad51127ca3ec90a54c6e90
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51265
Reviewed-by: Adam Langley <agl@google.com>
https://boringssl-review.googlesource.com/c/boringssl/+/51265 broke
Linux and macOS platforms on architectures where we don't build
assembly. OPENSSL_NO_ASM needs to match the assembly selector.

While I'm here, remove the OPENSSL_C11_ATOMIC comment. We autodetect
atomics these days, so the flag is only useful if you want to force it.

Change-Id: I1f20f7577b5ca7e208dc90fb46a93b20da864ec5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51305
Reviewed-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I8bc146a951b77d2111b971e6472bfd7f010f8585
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51306
Reviewed-by: Adam Langley <agl@google.com>
The current names are confusing because (ios, aarch64) is also used on
macOS. The various Apple platforms all use the same ABI, and aarch64 is
no longer limited to iOS. (For that matter, the iOS simulator on x86
Macs is x86 iOS.) Just call it 'apple'.

Update-Note: References to 'mac' or 'ios' source lists in downstream
builds should be renamed to 'apple'.

Change-Id: Id1a0627f8ce3241f34ffa6bb245ee5783adc7c15
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51307
Reviewed-by: Adam Langley <agl@google.com>
Now that the naming for Apple platforms is aligned, we can select the
variable uniformly.

Change-Id: Id547d1a4ba0585d5f9e4ea0b5d8b255b2ab2ec38
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51345
Reviewed-by: Adam Langley <agl@google.com>
Bazel can shard tests, but runner runs a lot of tests inside a single
“test”, as Go sees it. In order to shard within runner we implement
support for Bazel's environment variables[1] directly. This does mean
that the handful of other tests in runner are run in every shard.

[1]
https://docs.bazel.build/versions/1.1.0/test-encyclopedia.html#initial-conditions

Change-Id: Idaa5c6ae5225cd86951cd40f47b5f86f31664e04
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51245
Reviewed-by: David Benjamin <davidben@google.com>
ACVP authorisation tokens expire and, once expired, need to be renewed
by sending a new TOTP code. We almost never hit this but some FIPS
modules are slow enough that they can't compute the response within the
token lifetime.

But the ACVP code was putting an Authorization header on the renewal
message because it put that header on every message. But doing so breaks
the renewal because the server rejects the request because the token has
expired before noticing that it's a renewal request.

Also, put a 10 second buffer on deciding if a token has expired to
account for the transmission delay.

Change-Id: I50643a223cdb313d07dd7b2c559ad160cbe608ff
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51385
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
FIPS no longer likes it.

Change-Id: I32a4ba93a5849927ff75aa72b816cdc669e8a0af
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51325
Reviewed-by: David Benjamin <davidben@google.com>
The provision of FIPS that allowed the tests to be skipped based on a
flag-file has been removed in 140-3. Therefore we expect to run the fast
KATs on start-up, but to defer to slower ones until the functionality in
question is first used. So this change splits off the fast KATs and
removes support for skipping KATs based on a flag-file.

Change-Id: Ib24cb1739cfef93e4a1349d786a0257ee1083cfb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51326
Reviewed-by: David Benjamin <davidben@google.com>
The word “calculated” is two letters longer than “expected” and it's
nice to line up the ouptuts.

Change-Id: Idac70e62d98fbe26c430f03f4643ba295e40853d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51327
Reviewed-by: David Benjamin <davidben@google.com>
AS10.20 requires that the self-test for the integrity algorithm pass
before the integrity check itself. IG 10.3.A requires an HMAC self-test
now. Therefore run these tests before the integrity check.

Since we also need the ability to run all self-tests, both SHA
self-tests and the HMAC test are run again when running self-tests.
I'm assuming that they're so fast that it doesn't matter.

Change-Id: I6b23b6fd3cb6107edd7420bc8680780719bd41d2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51328
Reviewed-by: David Benjamin <davidben@google.com>
Builds that compile the FIPS stuff separately don't get this header from
other files.

Change-Id: I8a1b30ae360b08d4f4b9f804cd234998889477bc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51405
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
0-wiz-0 and others added 27 commits March 7, 2022 20:12
Fixes build on NetBSD.

Fixed: 483
Change-Id: I329eb327b67590828a3891f77a2cbbee5ec7affc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51705
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
…ent.

Having to check for header_len == len and a last byte of 0x80 is
actually unambiguous, but not obvious. Before we supported multi-byte
tags, a two-byte header was always {tag, 0x80}, but now a three-byte
header could be {tag1, tag2, 0x80}. But a 0x80 suffix could also be
{tag, 0x81, 0x80} for a 128-byte definite-length element.

This is unambiguous because header_len == len implies either zero length
or indefinite-length, and it is not possible to encode a definite length
of zero, in BER or DER, with a header that ends in 0x80. Still, rather
than go through all this, we can just report indefinite lengths to the
caller directly.

Update-Note: This is a breaking change to CBS_get_any_ber_asn1_element.
There is only one external caller of this function, and it should be
possible to fix them atomically with this change, so I haven't bothered
introducing another name, etc. (See cl/429632075 for the fix.)

Change-Id: Ic94dab562724fd0b388bc8d2a7a223f21a8da413
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Now we only have one BER/DER TLV parser. Annoyingly, this uses the CBS
BER function, not the DER one. This is because Android sometimes needs
allow a non-minimal length in certificate signature fields (see
b/18228011).

For now, this CL calls CBS_get_any_ber_asn1_element. This is still an
improvement over the old parser because we'll reject non-minimal tags
(which are actually even forbidden in BER). Later, we should move the
special case to just the signature field, and ultimately to a
preprocessing step specific to that part of Android.

Update-Note: Invalid certificates (and the few external structures using
asn1t.h) with incorrectly-encoded tags will now be rejected.

Bug: 354
Change-Id: I56a7faa1ffd51ee38cc315ebaddaef98079fd90e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51626
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I0a27b98ea1b8fd28ec415cb6c9ca789d438dd545
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51627
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Although the preceding CL fixes x509v3_bytes_to_hex to work with the
empty string, it's not really a good representation for zero. Zero as an
ASN1_INTEGER is sometimes the empty string (default-constructed) and
sometimes a single zero byte (parsed). bytes_to_hex also doesn't capture
the sign bit.

Instead, use X509V3_add_value_int, matching most of the other i2v, etc.,
functions in crypto/x509v3. X509V3_add_value_int calls i2s_ASN1_INTEGER,
which prints small values in decimal and large values in hexadecimal
with a 0x prefix.

It is unclear to me whether i2v and v2i are generally expected to be
inverses. i2v (or i2s or i2r) is used when printing an extension, while
v2i is used when using the stringly-typed config file APIs. However,
i2v_AUTHORITY_KEYID does not consume the "serial" key at all. It
computes the serial from the issuer cert.

Oddly, there is one ASN1_INTEGER,
PROXY_CERT_INFO_EXTENSION.pcPathLengthConstraint, which uses
i2a_ASN1_INTEGER instead. That one uses hexadecimal without the "0x"
prefix, and with newlines. Interestingly, its r2i function is not the
reverse of i2r and parses the s2i_ASN1_INTEGER format.

Between those, I'm assuming they're not necessarily invertible.

Change-Id: I6d813d1a93c5cd94a2bd06b22bcf1b80bc9d937b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51628
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This extends the old ASN1_INTEGER_set tests to cover all integers. There
are a whole bunch of ways to construct and convert ASN1_INTEGERs (DER,
BIGNUM, uint64_t, long, etc.). Rather than maintain one set of test
vectors for small numbers and another for BIGNUMs, this CL makes a
single set of BIGNUM-based test vectors.

Notably, this test now covers:

- Serialization and deserialization

- ASN1_INTEGER_get, not just ASN1_INTEGER_set

- BIGNUM conversions

- ASN1_INTEGER_cmp

Later CLs will add to this or change code covered by it.

Change-Id: I05bd6bc9e70c392927937c2f727cee25092802a1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51629
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Most libcs seem to be reasonable. Bionic (Android) always makes
pthread_rwlock_t. Reportedly, NetBSD does too. Default to assuming libcs
are reasonable and treat glibc as the exception.

Update-Note: If there are non-glibc libcs with similarly problematic
headers, this may break the build. Let us know if it does.

Fixed: 482
Change-Id: I63052cad7693d9e28d98775205fe7688e262d88c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51685
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I9aac529f181068746c5099ad08b6471887184202
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51725
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
While unlikely, ASN1_STRING_cmp is allowed to return INT_MIN (by way of
memcmp), in which case negating would overflow.

Change-Id: Iec63a6acfad2c662493d22a0acea39ca630881c8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51630
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These functions need some work, but first avoid the duplicate versions.
See also upstream's 6c5b6cb035666d46495ccbe4a4f3d5e3a659cd40.

Update-Note: ASN1_INTEGER_to_BN and ASN1_ENUMERATED_to_BN will now fail
when called on an ASN1_STRING/ASN1_INTEGER/ASN1_ENUMERATED (they're all
the same type) with the wrong runtime type value. Previously, callers
that mixed them up would get the right answer on positive values and
silently misinterpret the input on negative values. This change matches
OpenSSL's 1.1.0's behavior.

Change-Id: Ie01366003f7b2e49477cb73eaf7eaac26d86675d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51631
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This fixes several issues around ASN1_INTEGER handling. First, invalid
INTEGERs (not allowed in BER or DER) will no longer be accepted by
d2i_ASN1_INTEGER. This aligns with upstream OpenSSL, which became strict
in 6c5b6cb035666d46495ccbe4a4f3d5e3a659cd40, part of OpenSSL 1.1.0.

In addition to matching the standard, this is needed to avoid
round-tripping issues: ASN1_INTEGER uses a sign-and-magnitude
representation, different from the DER two's complement representation.
That means we cannot represent invalid DER INTEGERs. Attempting to do so
messes up some invariants and causes values to not round-trip correctly
when re-encoded. Thanks to Tavis Ormandy for catching this.

Next, this CL tidies the story around invalid ASN1_INTEGERs (non-minimal
and negative zero). Although we will never produce them in parsing, it
is still possible to manually construct them with ASN1_STRING APIs.
Historically (CVE-2016-2108), it was possible to get them out of the
parser, due to a different bug, *and* i2d_ASN1_INTEGER had a memory
error in doing so. That different bug has since been fixed, but we
should still handle them correctly and test this. (To that end, this CL
adds a test we ought to have added importing upstream's
3661bb4e7934668bd99ca777ea8b30eedfafa871 back in
c4eec0c.)

As the two's complement invariants are subtle as it is, I've opted to
just fix the invalid values before encoding. However, invalid
ASN1_INTEGERs still do not quite work right because ASN1_INTEGER_get,
ASN1_INTEGER_cmp, and ASN1_STRING_cmp will all return surprising values
with them. I've left those alone.

Finally, that leads to the zero value. Almost every function believes
the representation of 0 is a "\0" rather than "". However, a
default-constructed INTEGER, like any other string type, is "". Those do
not compare as equal. crypto/asn1 treats ASN1_INTEGER generically as
ASN1_STRING enough that I think changing the other functions to match is
cleaner than changing default-constructed ASN1_INTEGERs. Thus this CL
removes all the special cases around zero.

Update-Note: Invalid INTEGERs will no longer parse, but they already
would not have parsed in OpenSSL. Additionally, zero is now internally
represented as "" rather than "\0".

Bug: 354
Change-Id: Id4d51a18f32afe90fd4df7455b21e0c8bdbc5389
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51632
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
It's a little simpler to avoid messing around with malloc. It also
allows ASN1_STRING to internally reuse its buffer or realloc.

Change-Id: I12c9f8f7c1a22f3bcc919f5fcc8b00d442cf10f9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51633
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Along the way, add ASN1_INTEGER_get_uint64 from upstream, which has much
better error-handling. Also fold the IntegerSetting test into the main
integer test as the test data is largely redundant.

Change-Id: I7ec84629264ebf405bea4bce59a13c4495d81ed7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51634
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This gives clearer, less platform-dependent behavior.

Change-Id: Ib935bf861108ec010d8d409d840f94b52a3b3ae0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51635
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Nothing uses it, and I've never seen an ASN.1 spec use ANY DEFINED BY
with an integer selector. (Although X.680 1997 does seem to allow it.)

Change-Id: Ie1076f58838e4b889c5e6e12e9ca6dd1012472e7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51636
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
When a rust crate uses the boringssl rust bindings like:

```Cargo.toml
[dependencies]
bssl-sys = { path = "./third_party/boringssl/build/rust" }
```

The working directory of `build.rs` is set to the the crate's
working directory so "." and ".." aren't relative to the bindings'
directory. Use CARGO_MANIFEST_DIR to specify link search paths.

Change-Id: Ieb49f4ab479f47390388dc5ace70561f593dc238
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51645
Reviewed-by: David Drysdale <drysdale@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I5ef2db63a009bc1d69daff4449f5aa5973d6ad89
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51865
Reviewed-by: Bob Beck <beck@obtuse.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
BN_mod_sqrt implements the Tonelli–Shanks algorithm, which requires a
prime modulus. It was written such that, given a composite modulus, it
would sometimes loop forever. This change fixes the algorithm to always
terminate. However, callers must still pass a prime modulus for the
function to have a defined output.

In OpenSSL, this loop resulted in a DoS vulnerability, CVE-2022-0778.
BoringSSL is mostly unaffected by this. In particular, this case is not
reachable in BoringSSL from certificate and other ASN.1 elliptic curve
parsing code. Any impact in BoringSSL is limited to:

- Callers of EC_GROUP_new_curve_GFp that take untrusted curve parameters
- Callers of BN_mod_sqrt that take untrusted moduli

This CL updates documentation of those functions to clarify that callers
should not pass attacker-controlled values. Even with the infinite loop
fixed, doing so breaks preconditions and will give undefined output.

Change-Id: I64dc1220aaaaafedba02d2ac0e4232a3a0648160
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51925
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Commit-Queue: Adam Langley <agl@google.com>
…l-merge-c7a3c46574e7fc32357b2cc68f961c56c72b0ca4
bssl@4d955d2 added CPU cap checks based on the macro defined by compiler. The checks
are performed during compiler time not runtime because today most x86 platforms support a
much wider range of capabilities.

Some aws-lc customers may move the built binary(libcrypto) to another platforms. For safety,
aws-lc prefers to keep runtime CPU cap check only.

This code commit can be reverted in future when we think compile time checks are good enough.

google/boringssl@4d955d2
The reason is to have it lazily performed with the
other FFDH test.
This test was added in aws@282b528
- Uninitialised variable complained about by gcc-5.
- A file without a license header; added to exclude list.
This is instead of having it in the output.
@nebeid
Copy link
Copy Markdown
Contributor Author

nebeid commented Mar 28, 2022

At this point, the only tests failing are formal verification tests which are being looked into. Merging to the staging branch

@nebeid nebeid merged commit aca6347 into aws:current-upstream-merge-20220318 Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.