CMake versions newer than ~3.1x automatically determine the subdirectory under CMAKE_INSTALL_PREFIX using the type of the installed target. Older versions need this to be manually computed using the GNUInstallDirs library.
Since we override the CMAKE_INSTALL_PREFIX default, this just controls
the internal layout of the install/ directory generated underneath the
boringssl checkout.
Bug: 488
Change-Id: I97b02006301e463bb0cfd54acb2b27656484cc85
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52345
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We no longer use a weak hash for certificate comparisons. There
is no need to do extra work when certificates are the same.
Change-Id: I3b4b295122b289ae389bce2245b8348562700855
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52346
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
(Tweaking pending CLs now upsets the tooling so doing this as a
follow-up CL instead.)
Change-Id: I78efc5cb72de7b41d57769bb3958c20167de69b3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52325
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This work continues on top of the CL opened by Vlad Krasnov
(https://boringssl-review.googlesource.com/c/boringssl/+/44364). The
CL was thoroughly reviewed by David Benjamin but not merged due to
some outstanding comments which this work addresses:
- The flag check when doing the final reduction in poly1305 was
changed from `eq` to `cs`
- The CFI prologues and epilogues of open/seal were modified as
recommended by David.
- Added Pointer Authentication instruction to the functions that are
exported from the assembly code as pointed out by David.
Testing:
- The current tests against ChaCha20-Poly1305 continue to pass.
- More test vectors were produced using a Python script to try and
prove that having `eq` instead of `cs` was a bug. They passed as
well, but didn't result in the most significant word being
non-zero after the reduction, which would have highlighted the
bug. An argument about why it's unlikely to find the vector is
detailed below.
- `objdump -W|Wf|WF` was used to confirm the value of the CFA and the
locations of the registers relative to the CFA were as expected. See
https://www.imperialviolet.org/2017/01/18/cfi.html.
Performance:
| Size | Before (MB/s) | After (MB/s) | Improvement |
| 16 bytes | 30.5 | 43.3 | 1.42x |
| 256 bytes | 220.7 | 361.5 | 1.64x |
| 1350 bytes | 285.9 | 639.4 | 2.24x |
| 8192 bytes | 329.6 | 798.3 | 2.42x |
| 16384 bytes | 331.9 | 814.9 | 2.46x |
Explanation of the unlikelihood of finding a test vector:
* the modulus is in t2:t1:t0 = 3 : FF..FF : FF..FB, each being a 64 bit
word; i.e. t2 = 3, t1 = all 1s.
* acc2 <= 4 after the previous reduction.
* It is highly likely to have borrow = 1 from acc1 - t1 since t1 is
all FFs.
* So for almost all test vectors we have acc2 <= 4 and borrow = 1,
thus (t2 = acc2 - t2 - borrow) will be 0 whenever acc >
modulus. **It would be highly unlikely to find such a test vector
with t2 > 0 after that final reduction:** Trying to craft that
vector requires having acc and r of high values before their
multiplication, yet ensuring that after the reduction (see Note) of
their product, the resulting value of the accumulator has t2 = 4,
all 1s in t1 and most of t0 so that no borrow occurs from acc1:acc0
- t1:t0.
* Note: the reduction is basically carried by folding over the top
64+62 bits once, then folding them again shifted left by 2,
resulting in adding 5 times those bits.
Change-Id: If7d86b7a9b74ec3615ac2d7a97f80100dbfaee7f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51885
Reviewed-by: Adam Langley <alangley@gmail.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
strcasecmp is locale-sensitive, which can cause some mishaps.
This CL should be a no-op, because this call is only used on Android,
and bionic's strcasecmp seems to be ASCII-only. But using
OPENSSL_strcasecmp everywhere is easier to reason about.
Change-Id: Iecf9bc4da1bb3a4ab87b1e8b1d7f6f6c6e44aceb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52305
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
The HEAD version of Clang fails to build boringssl due to the following
warning triggered at -Werror:
/usr/local/google/home/dthorn/curl/boringssl/tool/digest.cc:229:12:
error: variable 'bad_hash_lines' set but not used
This change removes the variable.
Change-Id: I25009925d9a2dfc5b1783accab19e4b861db4ec2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52265
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
directly. This removes some temporary variables that would only be
used to hold ctx->verify_cb.
Change-Id: Id8f61c30bc74bb49757085c68baa4e8eec06b1d0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52285
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
MSVC is a little behind, but otherwise we should be able to assume C11
support in all our compilers. The only C99 builds should just be stale
build files. Such consumers are leaving performance on the table, by
using the worse refcounting implementation.
For now, don't require it in public headers. Android's build is still
defaulting to C99, which means requiring C11 will be disruptive. We can
try the public headers after that's fixed.
Update-Note: If the build fails with an error about C11, remove -std=c99
or -std=gnu99 from your build. Refcounting will get faster.
Change-Id: I2ec6f7d7acc026a451851d0c38f60c14bae6b00f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52247
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Now that we've dropped MSVC 2015, I believe we can rely on C++14 (which
is now seven years old). This switches the build to require C++14. I've
gone ahead and switched code in both public headers and within the
library, but if the public headers are a problem, we can revert those
separately.
C++14 doesn't get us quite as much as C++17, but see if we can get to
C++14 first. Still, std::enable_if_t and the less restricted constexpr
are nice wins.
Update-Note: C++14 is now required to build BoringSSL. If the build
breaks, make sure your compiler is C++14-capable and is not passing
-std=c++11. If this is causing problems for your project, let us know.
Change-Id: If03a88e3f8a11980180781f95b806e7f3c3cb6c3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52246
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
[UNIVERSAL 0] is reserved by X.680 for the encoding to use. BER uses
this to encode indefinite-length EOCs, but it is possible to encode it
in a definite-length element or in a non-EOC form (non-zero length, or
constructed).
Whether we accept such encodings is normally moot: parsers will reject
the tag as unsuitable for the type. However, the ANY type matches all
tags. Previously, we would allow this, but crypto/asn1 has some ad-hoc
checks for unexpected EOCs, in some contexts, but not others.
Generalize this check to simply rejecting [UNIVERSAL 0] in all forms.
This avoids a weird hole in the abstraction where tags are sometimes
representable in BER and sometimes not. It also means we'll preserve
this check when migrating parsers from crypto/asn1.
Update-Note: There are two kinds of impacts I might expect from this
change. The first is BER parsers might be relying on the CBS DER/BER
element parser to pick up EOCs, as our ber.c does. This should be caught
by the most basic unit test and can be fixed by detecting EOCs
externally.
The second is code might be trying to parse "actual" elements with tag
[UNIVERSAL 0]. No actual types use this tag, so any non-ANY field is
already rejecting such inputs. However, it is possible some input has
this tag in a field with type ANY. This CL will cause us to reject that
input. Note, however, that crypto/asn1 already rejects unexpected EOCs
inside sequences, so many cases were already rejected anyway. Such
inputs are also invalid as the ANY should match some actual, unknown
ASN.1 type, and that type cannot use the reserved tag.
Fixed: 455
Change-Id: If42cacc01840439059baa0e67179d0f198234fc4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52245
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This creates an install directory under the top level source directory.
The install contains a CMake config file that produces variables and
targets compatible with FindOpenSSL, or the directory can be scanned by
FindOpenSSL via -DOPEN_SSL_ROOT. This allows using BoringSSL with
third-party dependencies that find an SSL implementation via CMake.
Change-Id: Iffeac64b9cced027d549486c98a6cd9721415454
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52205
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
The ARMv8 assembly code in this commit is mostly taken from OpenSSL's `ecp_nistz256-armv8.pl` at 19e277dd19/crypto/ec/asm/ecp_nistz256-armv8.pl (see Note 1), adapting it to the implementation in p256-x86_64.c.
Most of the assembly functions found in `crypto/fipsmodule/ec/asm/p256-x86_64-asm.pl` required to support that code have their analogous functions in the imported OpenSSL ARMv8 Perl assembly implementation with the exception of the functions:
- ecp_nistz256_select_w5
- ecp_nistz256_select_w7
An implementation for these functions was added.
Summary of modifications to the imported code:
* Renamed to `p256-armv8-asm.pl`
* Modified the location of `arm-xlate.pl` and `arm_arch.h`
* Replaced the `scatter-gather subroutines` with `select subroutines`. The `select subroutines` are implemented for ARMv8 similarly to their x86_64 counterparts, `ecp_nistz256_select_w5` and `ecp_nistz256_select_w7`.
* `ecp_nistz256_add` is removed because it was conflicting during the static build with the function of the same name in p256-nistz.c. The latter calls another assembly function, `ecp_nistz256_point_add`.
* `__ecp_nistz256_add` renamed to `__ecp_nistz256_add_to` to avoid the conflict with the function `ecp_nistz256_add` during the static build.
* l. 924 `add sp,sp,#256` the calculation of the constant, 32*(12-4), is not left for the assembler to perform.
Other modifications:
* `beeu_mod_inverse_vartime()` was implemented for AArch64 in `p256_beeu-armv8-asm.pl` similarly to its implementation in `p256_beeu-x86_64-asm.pl`.
* The files containing `p256-x86_64` in their name were renamed to, `p256-nistz` since the functions and tests defined in them are hereby running on ARMv8 as well, if enabled.
* Updated `delocate.go` and `delocate.peg` to handle the offset calculation in the assembly instructions.
* Regenerated `delocate.peg.go`.
Notes:
1- The last commit in the history of the file is in master only, the previous commits are in OpenSSL 3.0.1
2- This change focuses on AArch64 (64-bit architecture of ARMv8). It does not support ARMv4 or ARMv7.
Testing the performance on Armv8 platform using -DCMAKE_BUILD_TYPE=Release:
Before:
```
Did 2596 ECDH P-256 operations in 1093956us (2373.0 ops/sec)
Did 6996 ECDSA P-256 signing operations in 1044630us (6697.1 ops/sec)
Did 2970 ECDSA P-256 verify operations in 1084848us (2737.7 ops/sec)
```
After:
```
Did 6699 ECDH P-256 operations in 1091684us (6136.4 ops/sec)
Did 20000 ECDSA P-256 signing operations in 1012944us (19744.4 ops/sec)
Did 7051 ECDSA P-256 verify operations in 1060000us (6651.9 ops/sec)
```
Change-Id: I9fdef12db365967a9264b5b32c07967b55ea48bd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51805
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Polynomials have more elements than strictly needed in order to better
align them for when vector instructions are available. The
|poly_mul_vec| path is sensitive to this and so zeros out the extra
elements before starting work. That means that it'll write to a const
pointer, even if it'll always write the same value. However, while this
code is intended for ephemeral uses, we have a case where this is a data
race that upsets tools.
Therefore this change always normalises the polynomials, even if it's
running in a configuration that doesn't care.
Change-Id: I83eb04c5ce7c4e7ca959f2dd7fbd3efbe306d989
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52186
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
There are paperwork reasons why it's useful to use the same hash
function in all cases. Thus unify on SHA-256 because contexts where
SHA-512 is faster, are faster overall and thus less sensitive.
Change-Id: I7a782a3adba4ace3257313a24dc8bc213b9d64ec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52165
Reviewed-by: David Benjamin <davidben@google.com>
This is causing -Werror failures with recent Clang builds.
Change-Id: I0f82d99ae51d02037f5f614a8fadfb3efc0da2da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52145
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Should not run jemalloc with ASAN simultaneously.
Change-Id: I2ea3107178c11fe34978bb093737564e1222c0d5
Signed-off-by: niewei <niewei@xiaomi.com>
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51945
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The files no longer need to be patched because fiat-crypto now has its
own copy of our value barrier. It does, however, require syncing our
NO_ASM define with fiat's.
fiat-crypto is now licensed under any of MIT, BSD 1-clause, or Apache 2.
I've stuck with the MIT one as that's what we were previously importing.
No measurable perf difference before/after this CL, with GCC or Clang on
x86_64.
Change-Id: I2939fd517de37aabdea3ead49150135200a1b112
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52045
Reviewed-by: Adam Langley <agl@google.com>
VS 2017 was released in March 2017, five years ago now. This means VS
2015 is now past our support window.
This will make the unmarked and "vs2017" configs in CI/CQ do the same
thing. I'll follow up with a separate CL in infra/config to switch the
test VS 2019 instead.
Update-Note: BoringSSL may no longer build with VS 2015. Consumers
should upgrade to the latest Visual Studio release. VS 2017 or later is
required.
Change-Id: I477759deb95a27efe132de76d9ed103826110df0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52085
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It appears to be unused. It has global effects and is not thread-safe.
Rather than try to make the double-function-pointer declaration
readable, remove it.
Change-Id: If58ecd0c9367bbb27cf8c5e27ac9997fe4c1225d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51965
Reviewed-by: Alex Gaynor <alex.gaynor@gmail.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ifdb2fe5952930c33dfa9ea5bbdb9d1ce699952a4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52027
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Our FIPS module only claims support for RSA signing/verification, and
|RSA_generate_key_fips| already performs a sign/verify pair-wise
consistency test (PCT). For ECDSA, |EC_KEY_generate_fips| performs a
sign/verify PCT too. But when |EC_KEY_generate_fips| is used for key
agreement a sign/verify PCT may not be correct.
The FIPS IG[1], page 60, says:
> Though not a CAST, a pairwise consistency test (PCT) shall be
> conducted for every generated public and private key pair for the
> applicable approved algorithm (per ISO/IEC 19790:2012 Section
> 7.10.3.3). To further clarify, at minimum, the PCT that is required by
> the underlying algorithm standard (e.g. SP 800- 56Arev3 or SP
> 800-56Brev2) shall be performed.
SP 800-56Ar3, page 36, says:
> For an ECC key pair (d, Q): Use the private key, d, along with the
> generator G and other domain parameters associated with the key pair,
> to compute dG (according to the rules of elliptic-curve arithmetic).
> Compare the result to the public key, Q. If dG is not equal to Q, then
> the pair-wise consistency test fails
But |EC_KEY_generate_fips| has always done that via
|EC_KEY_check_key|. So I believe that |EC_KEY_generate_fips| works for
either case.
This change documents that.
[1] FIPS 140-3 IG dated 2022-03-14 and with SHA-256
2f232f7f5839e3263284d71c35771c9fdf2e505b02813be999377030c56b37e4
Change-Id: I4b4e2ed92ae3d59e2f2404c41694abeb3eb283f4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51988
Reviewed-by: David Benjamin <davidben@google.com>
We need a function that returns a version that links to a certificate.
Previously we have used the git hash as the version of our modules but
the source cannot contain its own hash. Thus this change defines a new
format for FIPS module versions which will be filled in once we're ready
to define a version.
Change-Id: Ie4641945119106bc47e8da94ed8a45a86abb6f92
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51986
Reviewed-by: David Benjamin <davidben@google.com>
VS2015 has finally dropped off our support window. As part of dropping
it from the bots, I'm thinking of using the current vs2017 builders to
test vs2019. In preparation for that, add a vs2019 hash to
vs_toolchain.py.
Change-Id: I4c3dde2825f57c39a8da0e155e96d08550d39893
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52005
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This type is opaque, with no accessors or setters, and there is no way
to get a hold of one except by parsing it. It's only used indirectly via
X509 functions.
The 'other' field is unused and appears to be impossible to set or
query, in either us or upstream.
Change-Id: I4aca665872792f75e9d92e5af68da597b849d4b6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51746
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The getters would leave the length uninitialized when empty, which is
dangerous if the caller does not check. Instead, always fill it in.
This opens a can of worms around whether empty alias and missing alias
are meaningfully different. Treating {NULL, 0} differently from
{non-NULL, 0} has typically caused problems. At the PKCS#12 level,
neither friendlyName, nor localKeyId are allowed to be empty, which
suggests we should not distinguish. However, X509_CERT_AUX, which is
serialized in i2d_X509_AUX, does distinguish the two states. The getters
try to, but an empty ASN1_STRING can have NULL data pointer. (Although,
when parsed, they usually do not because OpenSSL helpfully
NUL-terminates it for you.)
For now, I've just written the documentation to suggest the empty string
is the same as the missing state. I'm thinking we can make the PKCS#12
functions not bother distinguishing the two and see how it goes. I've
also gone ahead and grouped them with d2i_X509_AUX, although the rest of
the headers has not yet been grouped into sections.
Bug: 426, 481
Change-Id: Ic9c21bc2b5ef3b012c2f812b0474f04d5232db06
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51745
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@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>
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>
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>
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>
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>
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>
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
c4eec0c16b02c97a62a95b6a08656c3a9ddb6baa.)
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>
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>
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>
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>
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>
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>
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>
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>
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>
Also don't linkify pipe words in the table of contents. Those are
already inside a link.
Change-Id: Ib984e914bcfe7a8e0216a0553b92100fd034bf36
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51605
Reviewed-by: Adam Langley <agl@google.com>
Daniel Thornburgh
Bob Beck
Adam Langley
Nevine Ebeid
David Benjamin
Kent Ross
Alex Gaynor
niewei
Adam Langley
Bob Beck
Kenichi Ishibashi
Thomas Klausner