The output of ASN1_generate_v3 is *mostly* linear with the input, except
SEQ and SET reference config sections. Sections can be referenced
multiple times, and so the structure grows exponentially.

Cap the total output size to mitigate this. As before, we don't consider
these functions safe to use with untrusted inputs, but unbounded growth
will frustrate fuzzing. This CL sets the limit to 64K, which should be
enough for anyone. (This is the size of a single X.509 extension,
whereas certificates themselves should not get that large.)

While not strictly necessary, this also rearranges the
ASN1_mbstring_copy call to pass in a maximum output. This portion does
scale linearly with the output, so it's fine, but the fuzzer discovered
an input with a 700K-byte input, which, with fuzzer instrumentation and
sanitizers, seems to be a bit slow. This change should help the fuzzer
get past those cases faster.

Update-Note: The stringly-typed API for constructing X.509 extensions
now has a maximum output size. If anyone was constructing an extension
larger than 64K, this will break. This is unlikely and should be caught
by unit tests; if a project hits this outside of tests, that means they
are passing untrusted input into this function, which is a security
vulnerability in itself, and means they especially need this change to
avoid a DoS.

Bug: oss-fuzz:55725
Change-Id: Ibb65854293f44bf48ed5855016ef7cd46d2fae77
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57125
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

This change exposes the necessary APIs for rust-openssl's HKDF wrappers
in [PKeyCtxRef]
(https://docs.rs/openssl/latest/openssl/pkey_ctx/struct.PkeyCtxRef.html#method.set_hkdf_mode).

Diff of the generated bindings: https://gist.github.com/mauricelam/51602bf17d4a5f9023a27cbe5f9ff705

Change-Id: Ic8df3d5308cfee5964ceadd3c2524b77e12512fb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57066
Reviewed-by: Alex Gaynor <alex.gaynor@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

Especially when they were named "2214" instead of "chromium-2214", I've
seen papers and other projects treat them as releases. Add a note to
make it clear they are not releases.

Change-Id: Ie820b800de3d25a31d3083d4ceff75e1d7f74a06
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57145
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

This includes an internal version which allows a flag to specify
the use of system malloc, or OPENSSL_malloc - this in turn allows
us to use this function in the ERR family of functions and allow
for ERR to not call OPENSSL_malloc with a circular dependency.

Bug: 564

Change-Id: Ifd02d062fda9695cddbb0dbef2e1c1db0802a486
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57005
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

This will let us call ERR and thread_local from OPENSSL_malloc
without creating a circular dependency. We also make
ERR_get_error_line_data add ERR_FLAG_MALLOCED to the returned
flags value, since some projects appear to be making
assumptions about it being there.

Bug: 564

Update-Note: Any recent documentation (in all OpenSSL forks) for the ERR functions
cautions against freeing the returned ERR "data" strings, as freeing them is handled
by the error library. This change can make an existing double free bug more
obvious by being more likely to cause a crash with the double free.

Change-Id: Ie30bd3aee0b506473988b90675c48510969db31a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57045
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: Bob Beck <bbe@google.com>

Decoding from decimal takes quadratic time, and BN_dec2bn will happily
decode however large of input you pass in. This frustrates fuzzers.

I've added a cap to the input length in s2i_ASN1_INTEGER for now, rather
than BN_dec2bn, because we've seen people use BN for surprisingly large
calculator operations, and BN generally doesn't cap inputs to quadratic
(or worse) algorithms beyond memory limits. (We generally rely on
cryptography using fixed parameter sizes, though RSA, DSA, and DH were
misstandardized and need ad-hoc limits everywhere.)

Update-Note: The stringly-typed API for constructing X.509 extensions
now has (very generous) maximum input length for decimal integers of
8,192 digits. If anyone was relying on a higher input, this will break.
This is unlikely and should be caught by unit tests; if a project hits
this outside of tests, that means they are passing untrusted input into
this function, which is a security vulnerability in itself, and means
they especially need this change to avoid a DoS.

Bug: chromium:1415108
Change-Id: I138249d23ca6b1996f8437dba98633349bb3042b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57205
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

Remove all the other ERR_R_MALLOC_FAILURES from the
codebase.

Also changes cbb to push to the error stack, to correctly
report cbb failures instead of now only reporting
malloc failures. Previously it turned all cbb failures
into a malloc failure

Bug: 564

Change-Id: Ic13208bf9d9aaa470e83b2f15782fc94946bbc7b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57046
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

The const bool doesn't do anything. While I'm here, make the methods
const.

Change-Id: Id8c31d5fcda6d8bc244c64b02b1d758e4eff6849
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57185
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>

draft-07 to draft-16 is mostly editorial, but there were a few notable
changes:

- Empty DST values are forbidden.

- The sample implementation for map_to_curve_simple_swu has completely
  changed. The new formulation has the same performance (if not a smidge
  faster), and aligning with the spec seems generally useful.

- P-384 is now paired with SHA-384, not SHA-512. As this would be a
  breaking change for the trust tokens code, I've left that in. A
  follow-up CL will add implementations of draft-16, which is expected
  to match the final draft.

Before:
Did 77000 hash-to-curve P384_XMD:SHA-512_SSWU_RO_ operations in 4025677us (19127.2 ops/sec)
Did 7156000 hash-to-scalar P384_XMD:SHA-512 operations in 4000385us (1788827.8 ops/sec)

After:
Did 77000 hash-to-curve P384_XMD:SHA-512_SSWU_RO_ operations in 4009708us (19203.4 ops/sec) [+0.4%]
Did 7327000 hash-to-scalar P384_XMD:SHA-512 operations in 4000477us (1831531.6 ops/sec) [+2.4%]

Bug: 1414562
Change-Id: Ic3c37061e325250d5d8723fd9aa263930c6023cf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57146
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>

Also add public APIs for this, now that the specification is no longer
expected to change, and because a project external to the library wishes
to use it.

For now, I've kept the P-256 version using the generic felem_exp, but we
should update that to use the specialized field arithmetic.

Trust Tokens will presumably move to this later and, in the meantime,
another team wants this.

Bug: chromium:1414562
Change-Id: Ie38203b4439ff55659c4fb2070f45d524c55aa2a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57147
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>

Compilers are fine at inlining functions nowadays. We can hide the
BN_ULLONG vs. manual carry extraction inside an inline function. I've
patterned the type signatures intentionally after Clang's builtins, in
case we want to use them in the future.

(Previously I wrote in
https://boringssl-review.googlesource.com/c/boringssl/+/56966 that the
builtins weren't good on aarch64. This wasn't quite right. Rather, they
were bad on both x86_64 and aarch64 in LLVM 13, but they're fine on both
in LLVM 14. My machine's Xcode was just a little old.)

Change-Id: I666466dce7a146d5e49e94ff372ea018b610ef34
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57245
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

It's unclear to me whether doing it target-by-target is an improvement
in crypto/fipsmodule, but this otherwise does seem a bit tidier. This
aligns with CMake's documentation and "modern CMake" which prefers this
pattern.

Change-Id: I36c81842bff8b36eeaaf5dd3e0695fb45f3376c9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56585
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

Also stick the very verbose default install directory in a variable so
we don't have to repeat it everywhere.

Change-Id: I1a6a85c4d42d3a6e766e52b2d0ecd8e81c6ed4e3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56607
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

This was added with the generated symbol-prefixing header. But it
seems to be sufficient for crypto to have a dependency on the
generated header, along with some of the stray bits of delocate.

It's a little unclear from CMake documentation how these are processed;
normally .o files can be built before libraries are built or linked,
only the link step depends on.

But, empirically, if A links B, and B has a dependency on C, then CMake
seems to run C before building any of A. I tested this by making a small
project where the generation step slept for three seconds and running
with enough parallelism that we'd have tripped.

Interestingly, in the Makefile output, the individual object file
targets didn't have that dependency, but the target itself did. But this
was true on both A and B, so I think that just might not work.

Also fix the dependency in the custom target. The old formulation broke
when using an absolute path to the symbols file.

Change-Id: I2053d44949f907d465da403a5ec69c191740268f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56928
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

Nothing uses this, and the code is somewhat decrepit. Instead of
fixing it and continuing to maintain it as attack surface, we
send this off to the farm where it can run and play all day with
the other unused X.509 extensions.

Update-note: This removes the proxy certificate extension from
the recognized certificate extensions. Previously by default
a certificate with a critical proxy certificate extension would
have been rejected with "proxy certificate not allowed", but
will now be rejected with an unrecognized critical extension
error.

Fixed: 568
Change-Id: I5f838d69c59517254b4fa83f6e2abe6057fa66c7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57265
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Bob Beck <bbe@google.com>

Change-Id: I0b1ba546374aa8b0fe79528f56e19f261536e565
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57305
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

bindgen, by default, will bind every random symbol in the libc, which is
clearly unreasonable. Now that --allowlist-file exists, we can switch to
doing what it should have done from the beginning.

This produces a pretty large diff in the bindgen output, but it's all to
exclude miscellaneous bits of libc.

Change-Id: I9a35fda10ff6f1b82449919f9fcc2ea86ad5b802
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57325
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Alex Gaynor <alex.gaynor@gmail.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

Change-Id: I5682358b5564dbaa734c5910c11a8274e88ca67a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57186
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

Prior to 3.12 (which we won't be requiring until July), OBJECT libraries
cannot be used with target_link_libraries. That means they cannot pick
up INTERFACE_INCLUDE_DIRECTORIES, which makes them pretty unusable in
the "modern CMake" style.

Just switch it to a static library to unbreak the build in CMake 3.10.

For some link ordering reason I don't understand, this also requires
explicitly linking boringssl_gtest to libcxx when we USE_CUSTOM_LIBCXX
is set.

Change-Id: Ia9d8351551f5da060248aa3ca73fe04473bf62aa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57345
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

While hiding 'type' isn't such a huge deal, accessing 'pkey' without a
type check is very dangerous. The accessors are type-checked and avoid
this problem. It also gets us slightly closer to not needing to utter
CRYPTO_refcount_t in public headers, as we're currently not quite
declaring it right. And it allows us to remove another union:
https://boringssl-review.googlesource.com/c/boringssl/+/57106

This matches what upstream did in OpenSSL 1.1.0.

Update-Note: Code that reaches into the EVP_PKEY struct will no longer
compile, like in OpenSSL. I believe I've fixed all the cases. If I
missed any, the fix is to switch code to accessors. EVP_PKEY_id(pkey)
for pkey->type is the most common fix.

Change-Id: Ibe8d6b6cb8fbd141ea1cef0d02dc1ae3703e9469
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57105
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

The union isn't actually providing type-safety: nothing checks that we
access the correct arm of the union, and it has a void* arm anyway.
Instead, it's just adding some strict aliasing risk by relying on
type-punning: we usually write to the pointer as void*, via
EVP_PKEY_assign, but then we read from it as the underlying type.

This is allowed in C, but not in C++. And even in C, while that is
allowed, if we ever wrote &pkey->pkey.rsa, it would suddenly be a strict
aliasing violation. Just use a void*, which means we don't type-pun
pointer types against each other.

While I'm here, I made the free callbacks for EVP_PKEYs also NULL the
pointer. The one caller also NULLs it, so its fine, but some did and
some didn't do it, and this seems prudent.

Bug: 301
Change-Id: I74c76ed3984527df66f64bb2d397af44f63920bd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57106
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

This convention seems to break with some other tooling we have. Until we
figure out how to resolve that, remove the lines.

This partially reverts 54b04fd but
keeps the fixes to the license header comments.

Change-Id: I4f08a9f3daf65d17b4c78ac6f4ac3de234ec3436
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57366
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

Change-Id: I7dea47e73cbfbfa48a8924f3a633215c06730b22
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57425
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

This extends the support for execute-only memory in AArch64 assembly
and uses adrp and add instead of adr.

Change-Id: I388a13ec754e7f179d7a234516f1bb4ff6a5c919
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57446
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Ran go get -u all, followed by go mod tidy. Some tools are flagging
CVE-2021-43565 and CVE-2022-27191 in some of the Go packages. Our uses
of x/crypto are x/net are not impacted by either bug, but update anyway
to silence the tools.

Change-Id: Ia0e2757625b58d964aedd4217f21b72f293b910b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57485
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

It happened that it used to be possible to run `acvptool` with `-json`
and not need a config file. That stopped working as of 02397c7 but
the test runner needs to be updated accordingly.

Change-Id: I54cf0fc7420d18749e93c3d85201ba24d0a59e15
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57465
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

Go has a bzip2 decoder but not an encoder (because I only needed a
decoder when I wrote the package). Thus when updated ACVP tests are
written they aren't compressed. This change removes the `.bz2` suffix
from paths when writing updated tests, which makes it easier to compress
them.

Change-Id: I9de6a1845cdf1cddca2e5d253b97262b023393f6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57466
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

1740ff9 added an "algorithm" key to the output. The test expectations
need to be updated accordingly.

Also drops 3DES tests since they were removed from the regcap in
8241345.

Change-Id: Ibd23f6166111be361511c2f7974348400f7151e2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57467
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Adam Langley <agl@google.com>

After
https://chromium-review.googlesource.com/c/chromium/tools/build/+/4277422,
the old ANDROID_NATIVE_API_LEVEL values are no longer in CMakeCache.txt.

Change-Id: If0c724081ef99bf3bc2e714fe84b6d16925bd116
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57507
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

OpenSSL's APIs are full of empty objects that aren't captured in the
type system. A DSA object may represent any of nothing, a group, a
public key, or a private key.

Since the type system doesn't capture this, better to check which type
we've got and NULL-check the fields we need for the operation.

Change-Id: I32b09ebaad58fcb2a0bfc9ad56d381f95099bf7b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57225
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

We already reject values that are out of bounds. Also we were using the
wrong error, so fix that. Zero should additionally be rejected,
otherwise, when signing an all-zero digest (impossible unless your
system signs untrusted, pre-hashed inputs), ECDSA can infinite loop.
Thanks to Guido Vranken who reported an analogous issue with DSA in
openssl/openssl#20268

When EC_KEYs are obtained through the parser, this CL is a no-op. The
corresponding public key is the point at infinity, which we'll reject at
both parse time and in EC_KEY_check_key. But as EC_KEY runs into
OpenSSL's usual API design flaw (mutable, field-by-field setters over
constructor functions for immutable objects), we should reject this in
EC_KEY_set_private_key too.

Update-Note: Systems that manually construct an EC_KEY (i.e. not from
parsing), and either omit the public key or don't call EC_KEY_check_key
will start rejecting the zero private key. If such a system *also* signs
untrusted digests, this fixes an infinite loop in ECDSA.

Change-Id: I3cc9cd2cc59eb6d16826beab3db71d66b23e83ff
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57226
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

…droid.""

This reverts commit 8ce0e1c.

The original commit didn't work on Android because:

  a) urandom_test didn't handle the fact that Android requires
     getrandom() and will never fall back to /dev/urandom.
  b) Android may open files in /dev/__properties__ which
     confused urandom_test.

The original change is patchset 1 so the differences build on that.

Change-Id: Ib840ec20d60cb28d126d3d09271b18fbd9ec1371
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53705
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Adam Langley <agl@google.com>

DSA private keys cannot be zero. If they are, trying to sign an all
zeros digest loops forever. Thanks to Guido Vranken who reported this in
openssl/openssl#20268

Along the way, because OpenSSL's bad API design made constructing DSA
objects such a mess, just move all the consistency checks to
dsa_check_parameters (now dsa_check_key) so it is consistently checked
everywhere.

Ideally we'd get a better handle on DSA state, like we hope to do for
RSA state (though not there yet), so checks only happen once. But we
consider DSA deprecated so it's not worth putting much effort into it.

Update-Note: Some invalid DSA keys will be rejected by the parser and at
use. Nothing should be using DSA anymore.

Change-Id: I25d3faf145a85389c47abdd9db8e9b0056b37d8a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57227
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

When the parameters are incorrect, all assumptions of (EC)DSA fly out
the window, including whether the retry loop actually terminates.

While ECDSA is broadly used with fixed, named groups, DSA was
catastrophically mis-specified with arbitrary parameters being the
default and only mode. Cap the number of retries in DSA_do_sign so
invalid DSA groups cannot infinite loop, e.g. if the "generator" is
really nilpotent.

This also caps the iteration count for ECDSA. We do, sadly, support
arbitrary curves via EC_GROUP_new_curve_GFp, to help Conscrypt remain
compatible with a badly-designed Java API. After
https://boringssl-review.googlesource.com/c/boringssl/+/51925, we
documented that untrusted parameters are not supported and may produce
garbage outputs, but we did not document that infinite loops are
possible. I don't have an example where an invalid curve breaks ECDSA,
but as it breaks all preconditions, I cannot be confident it doesn't
exist, so just cap the iterations.

Thanks to Hanno Böck who originally reported an infinite loop on
invalid DSA groups. While that variation did not affect BoringSSL, it
inspired us to find other invalid groups which did.

Thanks also to Guido Vranken who found, in
openssl/openssl#20268, an infinite loop when
the private key is zero. That was fixed in the preceding CL, as it
impacts valid groups too, but the infinite loop is ultimately in the
same place, so this change also would have mitigated the loop.

Update-Note: If signing starts failing with ECDSA_R_INVALID_ITERATIONS,
something went horribly wrong because it should not be possible with
real curves. (Needing even one retry has probability 2^-256 or so.)

Change-Id: If8fb0157055d3d8cb180fe4f27ea7eb349ec2738
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57228
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

Some Android devices in our builder pool are so old that they lack
getrandom. This change attempts to make them happy.

Change-Id: I5eea04f1b1dc599852e3b8448ad829bea05b9fe9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57527
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Adam Langley <agl@google.com>

Change-Id: I8e9a0b83453e44bdecab017144596bd3d61898ac
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57385
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>

Various constants and strings identifying the authors are currently
misplaced in .text. This change allows using execute-only .text on
platforms that enforce it by default, such as OpenBSD.

Modify x86_64-xlate.pl to replace .rodata with __DATA,__const for macs.
Adapt the nasm/masm path to emit an .rdata segment with alignment of 8.
This last change is not strictly needed but makes things explicit.

Change-Id: If716b892c1faabd85c6c70bdd75e145304841f83
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57445
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Change-Id: I736c7dc17efcaa5c3d8bd5fdee36d2dcb86ae627
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57567
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

At least on the devices we have on CI, PTRACE_SETREGS seems to break
things when passing in a slightly smaller structure. I'm guessing the
other registers get set to zero, so we SIGILL.

Cq-Include-Trybots: luci.boringssl.try:android_aarch64
Change-Id: I09da595dba2b6b70805c9a79c71c797c0f6635c0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57585
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>

Setting the first argument to -1 works, but changing the syscall number
is more straightforward. It's doable on aarch64 too, if we use a
different regset.

Cq-Include-Trybots: luci.boringssl.try:android_aarch64
Change-Id: I6c3c2d3dc67c06a44b181f9086cb5c9d343d51bc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57587
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

We used to have a tower of fallbacks to support older Androids that were
missing getauxval. The comments say getauxval is available in Android
API level 20 or higher, but this wasn't right. It's actually API level
18 or higher per the NDK headers and
https://developer.android.com/ndk/guides/cpu-features

Android API level 18 is Android 4.3, or Jelly Bean MR2. Recent versions
of the NDK (starting r24, March 2022) don't even support Jelly Bean,
i.e. the minimum API level is 19, and the usage statistics in the latest
Android Studio stop at KitKat. As far as I know, nothing needs us to
support API levels 17 and below anymore.

Update-Note: BoringSSL now requires API level 18 or later. Projects
needing to support API level of 17 or below will fail to build due to
the use of getauxval. If any such projects exist, please contact
BoringSSL maintainers.

Change-Id: Iedc4836ffd701428ab6d11253d4ebd5a9121e667
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57506
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

We call Accept as a TLS 1.2 client and a TLS 1.3 server. In the latter,
we create an SSLKeyShare object, Accept, and immediately destroy it. In
the former, we create the SSLKeyShare object a couple steps before
actually using it.

It's equivalent to create the object just before Accept, so switch to
that. This change means that hs->key_shares now only ever contains
objects in between Offer and Finish. Or, in KEM terms, it only ever
contains KEM private keys. (SSLKeyShare objects are currently a little
confused about what kind of state they contain.)

Change-Id: Idec62ac298785f784485bc9065f7647034d2a607
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57605
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>

We've got two layers of serialization. There's the lower-level
SerializePrivateKey/DeserializePrivateKey functions that just encode a
private key assuming you already know the group, and then there's
Serialize/Create which output an INTEGER and OCTET STRING pair.

The latter is only used by handoff.cc, so move them there. This trims
the SSLKeyShare abstraction slightly.

Change-Id: I1c901d7c16b082bfe1b6acd0a1711575e7f95c05
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Since this was written, we've tidied up the EC code a bit:

1. While not quite yet infallible (but we should get there), the output
   of EC_GROUP_new_by_curve_name no longer needs to be freed.

2. BN_CTX no longer does anything in EC code, so just pass in NULL.

We really should build a real ECDH API, but for now just improve our use
of the current thing.

Change-Id: I44f5429afec06c28372ae70148eb8de263d716f3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57626
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

This was added in https://crbug.com/1414198. Extracting
it to the same place as before to avoid needing to
update the recipe.

Change-Id: I02dd8bc2a9c0fc54236e10e13a43e716f1a34247
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57647
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

Nothing uses it yet, but this makes it available for any CI builders
that set vs_version to 2022.

Bug: 569
Change-Id: I7058157182f3e7762a35f8138977f37d56d65094
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57648
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

This doesn't run them as part of CI yet, but I'll follow-up with a
recipe change to read the same util/go_tests.txt file and run Go tests
in there.

Bug: 506
Change-Id: Ifd527b1d00ec30896582360132249230fab7e7c0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57645
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

It seems empty assembly files generate object files without a symbol
section at all. Just skip them.

Change-Id: I1bf06050dbde793e57faaeb2e40bcc4b6de497a6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57665
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

If another project includes us as a subproject, as gRPC does,
CMAKE_SOURCE_DIR points to the top-level source directory, not ours.
PROJECT_SOURCE_DIR points to ours. Likewise, CMAKE_BINARY_DIR will
point to the top-level one.

gRPC doesn't consume this CMake build, but in preparation for
eventually unifying the two CMake builds, replace CMAKE_SOURCE_DIR and
CMAKE_BINARY_DIR with a combination of CMAKE_CURRENT_{SOURCE,BINARY}_DIR
and PROJECT_SOURCE_DIR.

There's one more CMAKE_SOURCE_DIR which controls some default install
directory. I've left that one alone for now as I'm not sure what to do
with it. Probably the answer is to, like in gRPC, disable the install
target by default when we're not the top-level source directory.

Bug: 542
Change-Id: Iea26bbef8a4637671fd0e7476101512e871e7e18
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57686
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

This fixes two issues introduced in
0e68520. First, libssl never had a
dependency on libcrypto, so the include directory did not get passed
along. Second, although this build (unlike the other CMake build)
doesn't define an install target, gRPC includes it and then pulls it
into its own install target. That then runs afoul of CMake's check
against include directories in install targets.

To avoid this, condition the src/include directory on
$<BUILD_INTERFACE:...> but omit a corresponding
$<INSTALL_INTERFACE:...>. Since we're not the ones providing an install
target, we don't actually know the value to use.

Per [0], using the generator expression means we need to manually make
it absolute ourselves.

[0] https://cmake.org/cmake/help/latest/command/target_include_directories.html

Change-Id: I2a25cc8382116c5957d94f65641220559b7af87d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57685
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

No new tests because they're actually caught by our own tests. I just
forgot to put UBSan on CI! Will fix this shortly.

For BLAKE2, fix this by checking for zero. For c2i_ASN1_INTEGER,
rewrite it with CBS, which has the side effect of avoiding this. (It's
effectively maintaining in->data + start as a temporary, rather than
start itself.)

Bug: fuchsia:46910
Change-Id: I9366f1ba4fd0b0140d64c56e0534d7b060ab90e5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57687
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

https://boringssl-review.googlesource.com/c/boringssl/+/57645 wasn't
quite right. The cd to run ssl/test/runner affects the subsequent
commands. Fix this by running the Go tests first. They're very fast
compared to the others anyway.

Change-Id: Id5ea54a9787173eb3ed80e9db2c9ecfe064a93b0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57688
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

Follow-up to
https://chromium-review.googlesource.com/c/chromium/tools/build/+/4296194/

But mostly I've since added a lot more configurations to CI and want to
make sure I haven't broken anything.

Change-Id: I627711356004bf2244bd729b6055e9e0e619724e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57691
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

We do not expect to support this combination, but other consumers of
BoringSSL may choose to.

Change-Id: Ifdafa6a0032af078343bb9ecd80eea89eee582be
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57705
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

TLS 1.2 ECDHE and TLS 1.3 key shares were originally designed around
Diffie-Hellman-like primitives and use language based on that.
Post-quantum replacements do not look like Diffie-Hellman, where each
part exchanges a public key, but schemes that work differently can still
slot in without protocol changes.

We previously came up with our own Offer/Accept/Finish abstraction for
early post-quantum experiments, but the NIST constructions are all
expressed as KEMs: First, the recipient generates a keypair and sends
the public key. Then the sender encapsulates a symmetric secret and
sends the ciphertext. Finally, the recipient decapsulates the ciphertext
to get the secret.

Align our C++ and Go abstractions to this terminology. The functions are
now called Generate/Encap/Decap, and the output of Encap is called
"ciphertext", which seems to align with what most folks use. (RFC 9180
uses "enc" for "encapsulated key", but they staple a KEM to an AEAD, so
"ciphertext" would be ambiguous.)

Where variable names refer to parts of the protocol, rather than the
the underlying KEM-like construction, I've kept variable names matching
the protocol mechanism, so we still talk about "curves" and "key
shares", but, when using the post-quantum replacements, the terminology
is no longer quite accurate.

I've also not yet renamed SSLKeyShare yet, though the name is now
inaccurate. Also ideally we'd touch that up so the stateful object is
just a KEM private key, for SSLKEMKey. Though at that point, we maybe
should just add EVP_KEM and EVP_KEM_KEY APIs to libcrypto.

Change-Id: Icbcc1840c5d2dfad210ef4caad2a7c4bf8146553
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57726
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

This test could simulate the lack of MADV_WIPEONFORK on systems with it,
but couldn't simulate having it if the kernel didn't support it.

This change makes the presence / absence of
BORINGSSL_IGNORE_MADV_WIPEONFORK control whether MADV_WIPEONFORK is
"supported" in the test environment or not, and corrects the test for
the case where it's missing.

Change-Id: I23876788a0e0a4fd2a148f98b6b94e40880b6fc9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57745
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>

This made fido2's fuzzer angry: https://buganizer.corp.google.com/issues/271220905

Change-Id: Ib1b909be10f230df2daea3942f35cba0a81dcedb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57765
Commit-Queue: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

Also turn assertions into static_assert where we can.

These should be no-ops with existing assertions. The int assertion is
tighter, but we already assert this in constant_time_declassify_int. We
cannot support 64-bit int because it messes up integer promotion rules.

Change-Id: I628d2d7decdfa8bc01d8c6013bc7c20f927d63b1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57785
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>

ASN1_item_ex_i2d() does not take ownership of the memory pointed at by
*out, so it's the caller's responsibility to free it on error.

Change-Id: Id8cb70e50f280944418629a32b53fd4ca248b0bd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57805
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

I did not have "find a bug in the assembler" on my bingo card today, but
here we are.

NASM 2.15, prior to 2.15.04, has a bug where, if a section that already
exists is referenced again with alignment qualifiers, it incorrect adds
padding and mangles the output. See
https://bugzilla.nasm.us/show_bug.cgi?id=3392701.

Work around this by suppressing the perlasm-emitted qualifiers the
second time a section is emitted. We likely don't need these qualifiers
because, for all sections we care about, NASM's defaults are fine, but
perlasm tries to align .text more aggressively than the default, so let
it do that.

Bug: chromium:1422018
Change-Id: Iade5702c139b70772d4957a83c8f9be86c8af97c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57825
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

When running on a device via `adb shell`, stdout will be a pipe
and so is block buffered, leading to lost output if abort() is
called before flushing.

Change-Id: Ica67132fb8b2b1e7967df89fa3d0a9a793d8cbbf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54025
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

With EVP_PKEY and EVP_PKEY_CTX opaque, these symbols don't appear in any
public APIs anymore. Make them internal, which also opens the door to
renaming them:

- EVP_PKEY_METHOD is more accurately EVP_PKEY_CTX_METHOD
- EVP_PKEY_ASN1_METHOD is more accurately EVP_PKEY_METHOD

Or perhaps the split doesn't mean much and we should fold them together.

Change-Id: I8a0f7c2e07445dc981c7cef697263e59dba7784e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57885
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

Similar idea to https://boringssl-review.googlesource.com/c/boringssl/+/55466

Results are pretty close to the current state,
e.g. tool speed goes from
Did 74000 SHA-1 (16384 bytes) operations in 1004094us (73698.3 ops/sec): 1207.5 MB/s
to
Did 75000 SHA-1 (16384 bytes) operations in 1004022us (74699.6 ops/sec): 1223.9 MB/s

But on AMD with prefetchers disabled and large enough data size,
to force cache misses this gives ~3x improvement:
name              old time/op  new time/op  delta
BM_SHA1Hash/2      141ns ± 1%   143ns ± 2%     ~     (p=0.421 n=5+5)
BM_SHA1Hash/4      143ns ± 2%   143ns ± 3%     ~     (p=0.841 n=5+5)
BM_SHA1Hash/8      141ns ± 1%   141ns ± 2%     ~     (p=1.000 n=5+5)
BM_SHA1Hash/16     141ns ± 1%   141ns ± 1%     ~     (p=0.841 n=5+5)
BM_SHA1Hash/32     143ns ± 2%   143ns ± 1%     ~     (p=0.690 n=5+5)
BM_SHA1Hash/64     178ns ± 1%   179ns ± 1%     ~     (p=0.151 n=5+5)
BM_SHA1Hash/512    454ns ± 1%   454ns ± 1%     ~     (p=0.841 n=5+5)
BM_SHA1Hash/4k    2.66µs ± 1%  2.65µs ± 1%     ~     (p=1.000 n=5+5)
BM_SHA1Hash/32k   20.3µs ± 1%  20.3µs ± 2%     ~     (p=1.000 n=5+5)
BM_SHA1Hash/256k   162µs ± 1%   161µs ± 1%     ~     (p=0.548 n=5+5)
BM_SHA1Hash/1M     644µs ± 1%   645µs ± 1%     ~     (p=0.841 n=5+5)
BM_SHA1Hash/2M    1.29ms ± 1%  1.29ms ± 2%     ~     (p=0.690 n=5+5)
BM_SHA1Hash/4M    2.58ms ± 1%  2.58ms ± 1%     ~     (p=0.841 n=5+5)
BM_SHA1Hash/8M    5.14ms ± 0%  5.15ms ± 1%     ~     (p=0.286 n=4+5)
BM_SHA1Hash/16M   11.4ms ± 3%  10.3ms ± 1%   -9.04%  (p=0.016 n=4+5)
BM_SHA1Hash/128M   249ms ± 0%    83ms ± 1%  -66.73%  (p=0.008 n=5+5)

Change-Id: I7cae746b6d8a705d6bf2d5c5df6a2dca6d44791a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57826
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>

Change-Id: Ie7543047c6f489ae849b3c27703948f0977c33fd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57905
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

- update rust folder to split into `bssl` and `bssl-sys`
- add initial bindings for hmac and a subset of sha2

Change-Id: I09e0e778c1590de6818a49e19529ceb011e4d9f6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57285
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>

Add Cargo.lock to .gitignore for rust/bssl-crypto and remove generated file
Clean up cargo deny warnings and set reasonable starting point in deny.toml

Change-Id: I0b925408ab6b7947b1b01282bac803feb769421a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57985
Reviewed-by: Nabil Wadih <nwadih@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

Change-Id: Idbf0be16fbda2f0cb2444837021d4f95c29a1857
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57405
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

Guarding the OPENSSL_memset was unnecessary since OPENSSL_memset with
zero length works fine. Also OPENSSL_memset, to workaround a C bug,
internally does the same check anyway. (But also this wasn't a context
where the C bug applied.)

Also don't bother calling EVP_MD_block_size a second time when we
already saved it into block_size.

Finally, don't bother filling key_block up to the whole
EVP_MAX_MD_BLOCK_SIZE size. I could believe the fixed size is easier for
the compiler to optimize, but the XORs in setting up an HMAC cannot
possibly be performance-sensitive, and using the actual length is
clearer.

Also add an assert that the hash's output size fits in the block size.
We're implicitly relying on this when hashing the key down.

Change-Id: I6ce37d41ea5bdbc8890bde7910e1b5651bc35709
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58027
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>

RDNs are a SET OF attributes which means they should be sorted by
DER encoding length, then lexicographically. We didn't have any test
coverage for this.

Bug: 548
Change-Id: I542196aae26984aeee4f1c6774878b121675b0dc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58025
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

Change-Id: I59bcacf10a59ffdf9709785727f5f8b73c992f6e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58026
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

HMAC hashes long keys internally using the provided hash function.
This removes the arbitrary limit of EVP_MAX_MD_SIZE on the length
of the key.

Change-Id: I56d68cbd2ddaf1f5f1fa4ecc40105b00b2ccd87c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58006
Reviewed-by: Nabil Wadih <nwadih@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

Change-Id: I18596751776262be09d8ba09ed258e1f66d90654
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58046
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

An in-progress rewrite of tasn_dec.c accidentally broke this, so add a
regression test.

Bug: 548
Change-Id: Iac6a23acbc08459187c96a2f6471f0aa97d445a1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58125
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

ASN1_TYPE is a union of a bunch of pointer types. Often it takes a
shorthand and accesses a->value.ptr directly. This is allowed in C, but
not C++. Writing the switch/case barely takes more code, so just write
it that way.

Along the way, extract the code for cleaning up an ASN1_TYPE from
tasn_fre.c. This is a small step towards being able to use crypto/asn1's
types without depending on the templates. ASN1_TYPE_free still, for now,
calls into the templates. That will be fixable once tasn_*.c are
rewritten a bit further.

This also removes the weird hack here ASN1_primitive_free (an internal
function) with NULL ASN1_ITEM secretly meant to partially clean up the
ASN1_TYPE. We can just call the function directly now.

Bug: 574
Change-Id: Ie2ba41418801a366ab2f12eccc01e8dadf82c33e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58126
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Mark the two existing dependencies which should be made to go away.
This is a bindings library to boringssl. it should really *not*
need extra dependencies on external crates.

Change-Id: Ia687510247154634e6d8b2be4c2f5840dabc1c89
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58107
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

It appears to no longer be needed.

Change-Id: Idd9aa128192eb0f1bdff52e190fd032090411d43
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58165
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Nabil Wadih <nwadih@google.com>

Change-Id: Ic0149a69cc27727c2302bc2a90d03839fd5637b5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57545
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

These functions already don't go through tasn_*.c. Rewrite them to use
CBS and CBB. This removes some dependencies on ASN1_get_object and
ASN1_put_object.

Update-Note: d2i_ASN1_OBJECT and d2i_ASN1_BOOLEAN will no longer accept
non-minimal length prefixes (forbidden in DER). d2i_ASN1_BOOLEAN will
also no longer accept non-canonical representations of TRUE (also
forbidden in DER). This does not affect certificate parsing, as that
still goes through the old template system, though we will make a
similar change to those functions later.

Bug: 354, 548
Change-Id: I0b7aa96f47aca5c31ec4f702e27108b4106311f2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58145
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

Change-Id: If7eb9daf65f6c9480a9bffb5f1e1ea3143c5faf7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58146
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

Removing object reuse makes it dramatically simpler. Along the way, lift
the OID validity checker into crypto/bytestring, so we can use it more
generally. (Although the difference between invalid OID and unknown OID
is pretty academic, so this check isn't that important.)

For now I've preserved the existing behavior, where the OID validity
checker accepts arbitrarily large OID components. Though this results in
an oddity where the OID to string functions reject inputs that the
parser accepts. (There we only allow up to 2^64-1.)

Update-Note: When we removed object-reuse from all the d2i functions, we
missed one d2i_ASN1_OBJECT. See
https://boringssl-review.googlesource.com/c/boringssl/+/56647.
Otherwise, this CL is not expected to change behavior.

Change-Id: If4d2d83d9f3c96abfdc268e156f2cf3a9a903b0c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58147
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

- add helper for converting between hex strings and bytes

Change-Id: I073b597cfb1b2687dd7d1743441bdfaaf601810d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58225
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

OpenSSL's ASN1_STRING representation has many cases. There's a grab-bag
V_ASN1_OTHER cases that can represent any element. But it is currently
only used for non-universal tags. Unknown universal tags go into the
type field directly.

This has a few problems:

- Certain high values, V_ASN1_NEG_INTEGER and V_ASN1_NEG_ENUMERATED,
  are treated special. This was one of the two causes behind
  CVE-2016-2108 and had to be worked around with V_ASN1_MAX_UNIVERSAL.

- OpenSSL can never compatibly support a new universal type in a
  non-ASN1_STRING form. Otherwise ASN1_TYPE's union changes its
  in-memory representation.

- It is a bit ambiguous when OpenSSL does or doesn't know the type.

- This is broadly implemented by having a default in all the
  switch/cases, which is a little awkward.

- It's yet another "unknown tag" case when V_ASN1_OTHER covers such
  cases just fine.

Remove this representation and use V_ASN1_OTHER. This more unambiguously
resolves CVE-2016-2108. ASN1_STRING's and ASN1_TYPE's respective type
fields are now a closed set. Update the documenthation accordingly.

Formally allowing universal types in ASN1_STRING also opens the door to
clearing the ASN1_PRINTABLE mess (https://crbug.com/boringssl/412).
BoringSSL currently rejects X.509 names that are actually valid, because
the OpenSSL X509_NAME representation cannot represent them. This allows
us to introduce an ASN1_STRING-based ANY representation, which just
represents all non-ASN1_STRING types in an V_ASN1_OTHER.

The implementation is a little clumsy (the way things tasn_dec.c is
written, I had to introduce yet another check), but I'm hoping that,
when the parser is rewritten with CBS, this can be integrated into a
single type dispatch.

Update-Note: This does not change the set of inputs accepted or rejected
by the ASN.1 parser. It does, however, change the in-memory
representation in edge cases. Unless the application was specifically
inspecting the in-memory representation for these unknown types, we
expect this to have no impact.

Fixed: 561
Change-Id: Ibf9550e285ce50b11c7609d28b139354b9dd41dc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58148
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Change-Id: I9ec56ae8d4ca0a28dccbf2a04c4c675791c053b8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58265
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

This is a cursory conversion and is, currently, very tedious because it
needs to bridge calling conventions. After tasn_*.c and all the
underlying primitives have CBS/CBB-based calling conventions, this
should be a lot cleaner.

This is to break a dependency cycle:

- We'd like to rewrite d2i_X509 with CBS

- To do that, we need to rewrite its underlying types with CBS

- Those parsers are tied up in tasn_dec.c, so we effectively need to
  rewrite tasn_dec.c with CBS.

- CBS is designed for DER, not BER, so such a change would most
  naturally switch the TLV parser to require DER.

- We've *almost* done that already except
  https://boringssl-review.googlesource.com/c/boringssl/+/51626 had to
  stop at non-minimal definite lengths, which are allowed in BER but
  forbidden in DER. See b/18228011 for a bunch of certificates which
  have a non-minimal definite length at *just* the signature field.

- So, to do that, we'd ideally special case just that field, or BIT
  STRINGs in general, to tolerate minimal lengths. That's easiest done
  when d2i_X509 is CBS, so we can just do what we want in imperative
  code. And thus we're back full circle.

So, detach X509 from the templates now. It's a bit tedious because we
need to switch calling conventions for now, but it breaks the cycle.
Later, we can revisit this and get all the benefits of a fully CBS-based
path.

For now, I haven't added an ASN1_ITEM. If it comes up, we can make an
EXTERN ASN1_ITEM.

Update-Note: The ASN1_ITEM removal means custom ASN.1 templates (which
are discouraged in favor of our much simpler CBS and CBB types) using
X509 will fail to compile. We don't believe anyone is relying on this,
but this can be restored if we find something.

Update-Note: Certificate parsing is slightly stricter: the outermost
TLVs, except for the signature field, no longer accept non-minimal
lengths, as mandated by DER. This strictness was broadly already applied
by the libssl parser.

Bug: 547
Change-Id: Ie5ad8ba4bb39f54fdd3dd45c53965b72a3850709
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58185
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Now that the preceding CL has isolated the X.509 signature hack, we can
apply the strictness across the legacy parser. This is particularly
important for the TBSCertificate parser, where it is ambiguous which
value one checks the signature over. (Officially, you're supposed to
re-encode as DER. In practice, people don't do this.)

This change means many of our primitive types are actually parsed as
DER. I've removed the bug references in the comment in the documentation
where I believe they're finally correct.

Update-Note: Non-minimal lengths in certificates are no longer accepted,
as required for standards compliance. The one exception is the signature
field, where we still carry an exception. Some of this was already
enforced by libssl's parser.

Bug: 354
Change-Id: I57cfa7df9e1ec5707390e9b32fe1ec6b5d8172f9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58186
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

I mistakenly thought no one needed X509 as an ASN1_ITEM, but that wasn't
true. wpa_supplicant relies on this. Restore this and add a test for it.

As with the rest of the rewrite, this is currently a little tedious. I'm
hoping that, as the internals are rewritten with CBS and CBB, we can
establish some cleaner patterns and abstractions.

Bug: 547
Change-Id: I761ee058f8ec916b2ec7f4730a764d46d72f1f10
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58285
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

alg was a copy-paste error.

Change-Id: I4c591bb98f7da853c79679727d49a00c7c65ad91
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58305
Reviewed-by: Alex Gaynor <alex.gaynor@gmail.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

This function is probably not what you want. I think the
X509V3_EXT_METHOD and ASN1_STRING_TABLE_add stuff are the only cases
where you actually need an OID. Those, themselves, have problems with
conflicting registrations and aren't really necessary.

ASN1_STRING_TABLE_add has a similar warning on it. X509V3_EXT_METHOD is
not yet documented, but should get a similar warning when we document
it.

Change-Id: I173918a3f7db25059ed078b2f67b23acfb43033d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58365
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>

These functions need a lot more work, documentation, warnings
that using them isn't a good idea, and really we should just remove them
entirely.

But, for now, this is a minimal fix to the most egregious of issues: not
only are the functions themselves not thread-safe (i.e. you must call it
in some program-global initialization), but using them puts you in a
state where future uses of the X.509 library are not thread-safe! Fix
the latter by sorting the list at the point we're already mutating
things.

Re-sorting a list after every addition is not a particularly sensible
implementation, but we can assume these lists will only ever contain
O(1) entries.

(The sort calls date to
https://boringssl-review.googlesource.com/c/boringssl/+/27304, but the
issue was there before. Prior to that CL, sk_FOO_find implicitly sorted
the list. That CL made sk_FOO_find itself a const operation, necessary
for this, and just added explicit sk_FOO_sort calls to preserve the
existing behavior, initially.)

Change-Id: I063b8e708eaf17dfe66c5a3e8d33733adb3297e9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58385
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

This is a double-pointer and both layers should be const. This matches
OpenSSL 1.1.1, so in addition to being more const-correct, we're more
OpenSSL-compatible.

Update-Note: Anything that defines a comparison function would need to
fix the type signature. I found only one external caller, Envoy, that
defines it. envoyproxy/envoy#25051 fixes it.
(That we hadn't run into the upstream incompatibility suggests this is
just not a feature folks use outside the library much.)

Bumping BORINGSSL_API_VERSION, in case I missed any, and there's some
caller where we can't just use C++14 generic lambdas to smooth it over.

Fixed: 498
Change-Id: I8f07ff42215172aa65ad8819acf69b63d6d8e54c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56190
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

This was not thread-safe and, until the previous CL, egregiously so. No
one uses this API, so remove it.

Update-Note: Various unused functions for registering named
X509_VERIFY_PARAMs were removed. These functions only exist to make
X509_VERIFY_PARAM_lookup return a custom value. Instead, applications
that want a particular X509_VERIFY_PARAM can just configure it directly,
rather than stashing it in library-global state and then looking it back
up with X509_VERIFY_PARAM_lookup.

Change-Id: I8d532a1a137c7abbc131f2cb5d12ba94e5728e2d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58386
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

OpenSSL interprets NULL and empty lists as {anyPolicy}. I intended to
implement this, but didn't quite get the NULL case right. Fix this and
add a test.

Change-Id: I50dbf02695f424697e28a6e0ec4fd50b2822f44f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58425
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

If we see a critical policy constraints extension, we have two options:
We can either process it, which requires running policy validation, or
reject the certificate. We and OpenSSL do neither by default, which
means we may accept certificate chains that we weren't supposed to.

This fixes it by enabling X.509 policy validation unconditionally and
makes X509_V_FLAG_POLICY_CHECK moot. As a side effect, callers no longer
need to do anything for CVE-2023-0466.

This is the opposite of [0]'s advice, which instead recommends skipping
the feature and rejecting critical policy contraints. That would be a
good move for a new X.509 implementation. Policy validation is
badly-designed, even by X.509's standards. But we have OpenSSL's history
of previously accepting critical policy constraints (even though it
didn't check it). I also found at least one caller that tests a chain
with policy constraints, albeit a non-critical one.

We now have an efficient policy validation implementation, so just
enable it.

Of course, fixing this bug in either direction has compatibility risks:
either we take on the compat risk of being newly incompatible with
policyConstraints-using PKIs, or we take on the compat risk of newly
rejecting certificates that were invalid due to a policy validation
error, but no one noticed. The latter case seems safer because the chain
is unambiguously invalid.

Update-Note: X.509 certificate verification (not parsing) will now
notice policy-validation-related errors in the certificate chain. These
include syntax errors in policy-related extensions, and chains with a
requireExplicitPolicy policy constraint that are valid for no
certificate policies. Such chains are unambiguously invalid. We just did
not check it before by default. This is an obscure corner of X.509 and
not expected to come up in most PKIs.

[0] https://www.ietf.org/archive/id/draft-davidben-x509-policy-graph-01.html#section-3.4.4

Fixed: 557
Change-Id: Icc00c7797bb95fd3b14570eb068543fd83cda7b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58426
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Passes test vectors, and should be constant time, but is currently
not optimized and neither the API nor the standard is stable.

Change-Id: I89b90877e023a43ee7238e11b86065444ab3bdec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57845
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>

Change-Id: I0d1b4ebb291b19bf549f863a1aaaa07e770328a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58505
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

This updated value is not in the private use area and can hopefully be
more stable over time.

Change-Id: Ieda34ec0f356cfd03365973f611a6edc23431e29
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58525
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>