This aligns with upstream's f72f00d49549c6620d7101f5e9bf7963da6df9ee. In
doing so, I had to fill in a bunch of NULL checks in p_ec_asn1.c, to
account for EVP's needlessly many "empty" states. For now, those cases
return a goofy -2 to align with upstream. Our EVP_PKEY_cmp_parameters
still returns negative values, so this is fine, though ideally we'd
narrow to boolean. That probably depends on some other changes. See
https://crbug.com/boringssl/536#c3.
Bug: 536
Change-Id: I1124c8ad5223ac23953d94ff9ca734fbb714e89c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55185
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
I missed this in
https://boringssl-review.googlesource.com/c/boringssl/+/54905.
Upstream's 2986ecdc08016de978f1134315623778420b51e5 also made copying
into EVP_PKEY_NONE allowed.
For those keeping score, this gives us *even more* layers of empty
states:
- EVP_PKEY with no type
- EVP_PKEY with type but no key
- EVP_PKEY with type and EC_KEY but EC_KEY is empty
- EVP_PKEY with type and EC_KEY and EC_KEY only has a group
To say nothing of the states in https://crbug.com/boringssl/534. This
API is not good.
Bug: b:238920520
Change-Id: I49e85af5b02b16724454999ccb7c61b520d8c99c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55165
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This is a departure from OpenSSL's output (which seems to just append
even more information afterwards), but is a better way to identify the
algorithm.
Change-Id: Iccffdf9297bde5362d902d4de1d99de7b673bed2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54952
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
BIO_hexdump does not really fit here. This matches OpenSSL.
Change-Id: I5c8e2b992c2711fb7986aa549578da9495360536
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54951
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This seems to just have been a bug. OpenSSL partially fixed it in
https://github.com/openssl/openssl/pull/9983, but upstream's fix
duplicated some logic and outputs "Public-Key" in the ptype == 0
(parameters) case.
Change-Id: I2c669c1cb1a4af50858afd5b1179d3550f3c119a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54950
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Aligning the bn_print labels doesn't do anything. They will, almost all
the time, add a newline anyway.
Change-Id: Ib6571eba7508ebd46508c61a68bfbb03d8c52ba6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54949
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
First, stop trying to pre-size the buffer and just have bn_print
allocate the buffer internally. That removes the need for all the
algorithms being two-pass.
While I'm here, stop passing the unused ASN1_PCTX parameters in
everywhere.
As a side effect, this fixes a int vs size_t instance that flagged
-Wshorten-64-32, but it ended up being a much more substantial change.
Bug: 516
Change-Id: Ic210604de85539559b1ed88889ca6a08dfb20bde
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54948
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
These are mostly to ensure they don't crash, and that subsequent changes
don't unintentionally change the output. The current output is a little
weird but, for now, I've just captured the current output, bugs and all.
Change-Id: I9f1a4910ccc717764ef44551de9b3e0f9f2a1b40
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54947
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We shouldn't print different things depending on sizeof(long).
Change-Id: I5f97e17b838f8c9b119421b9ce0e93e95bd33dc0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54946
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Bug: 516
Change-Id: Iba2014da414658c08e42e0993912fa73848832d3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54945
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Some third-party code requires it.
For now, I've just introduced a new hook on the method table. This is
rather goofy though. First, making EVP know about TLS is a layering
violation that OpenSSL introduced. They've since fixed this and added
EVP_PKEY_get1_encoded_public_key in OpenSSL 3.0, but callers expect the
TLS one to exist in OpenSSL 1.1.1, so implement that one.
Along the way, implement EC_KEY_oct2key from upstream, which is slightly
less tedious when you're already working in EC_KEY.
To make this third-party code work (and to write a test without dipping
out of EVP, or using the very tedious EVP_PKEY_paramgen API), we also
need to change EVP_PKEY_copy_parameters to work when the source EVP_PKEY
is empty, per upstream's 2986ecdc08016de978f1134315623778420b51e5.
OpenSSL's API has *multiple* levels of empty states to worry about!
Something to avoid when we get to rethinking this error-prone API.
Bug: b:238920520
Change-Id: I3fd99be560db313c1bf549a4e46ffccc31e746e1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54905
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
We don't support DSA EVP_PKEY_CTXs (trying to create one will just
fail), but to aid building projects that try to create them, add the
functions and make them always fail.
In particular, Node calls these two. It calls
EVP_PKEY_CTX_set_dsa_paramgen_q_bits via EVP_PKEY_CTX_ctrl, but I'll
send them a patch to use the wrapper function.
Change-Id: Ic134c50b6ea0b59dc8f15be77243b9ae9dfa6a61
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54310
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The C11 change has survived for three months now. Let's start freely
using static_assert. In C files, we need to include <assert.h> because
it is a macro. In C++ files, it is a keyword and we can just use it. (In
MSVC C, it is actually also a keyword as in C++, but close enough.)
I moved one assert from ssl3.h to ssl_lib.cc. We haven't yet required
C11 in our public headers, just our internal files.
Change-Id: Ic59978be43b699f2c997858179a9691606784ea5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53665
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This may be okay because of the strict aliasing character type rule, but
easier not to think about it.
Bug: 301
Change-Id: I5eec356a5411a67036425e953e56529bac81ad4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53091
Reviewed-by: Adam Langley <agl@google.com>
This split in methods probably needs rearranging. This means most
callers (i.e. ones that don't use EVP_PKEY_CTX_new_id) will no longer
pick up the HKDF EVP method.
Bug: 497
Change-Id: I72ef255a472c458e8db62461f7b3ac65fa488535
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52830
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL 1.1.1 added HKDF support, but by sticking it into
EVP_PKEY_derive, the API meant for Diffie-Hellman-like primitives.
Implement it for OpenSSL compatibility.
This does unfortunately mean anything using EVP now pulls in HKDF. HKDF
isn't much code, but we should make EVP more static-linker-friendly.
(Filed https://crbug.com/boringssl/497)
Change-Id: I90b9b0d918129829eb36ba9d50ff4d8580346ff0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52829
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
For now, it contains a call to set the service indicator so it should
live in the module. In term it would be nice to move it back out and
have the service indicator set in RSA and ECDSA functions themselves
once the ECDSA functions can take an indicator of the hash function
used.
Change-Id: I2a3c262f66b1881a96ae3e49784a0dc9fc8c4589
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52705
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
We have no EVP_MDs with type NID_ecdsa_with_SHA1 (that's a remnant of
the old signature algorithm EVP_MDs). Also there's no sense in calling
EVP_MD_type or performing the cast five times.
Change-Id: I7ea60d80059420b01341accbadf9854b4c3fd1b8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52685
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is cribbed, with perimssion, from AWS-LC. The FIPS service
indicator[1] signals when an approved service has been completed.
[1] FIPS 140-3 IG 2.4.C
Change-Id: Ib40210d69b3823f4d2a500b23a1606f8d6942f81
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52568
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
GCC has a warning that complains about even more type mismatches in
printf. Some of these are a bit messy and will be fixed in separate CLs.
This covers the easy ones.
The .*s stuff is unfortunate, but printf has no size_t-clean string
printer. ALPN protocol lengths are bound by uint8_t, so it doesn't
really matter.
The IPv6 printing one is obnoxious and arguably a false positive. It's
really a C language flaw: all types smaller than int get converted to
int when you do arithmetic. So something like this first doesn't
overflow the shift because it computes over int, but then the result
overall is stored as an int.
uint8_t a, b;
(a << 8) | b
On the one hand, this fixes a "missing" cast to uint16_t before the
shift. At the same time, the incorrect final type means passing it to
%x, which expects unsigned int. The compiler has forgotten this value
actually fits in uint16_t and flags a warning. Mitigate this by storing
in a uint16_t first.
The story doesn't quite end here. Arguments passed to variadic functions
go through integer promotion[0], so the argument is still passed to
snprintf as an int! But then va_arg allows for a signedness mismatch[1],
provided the value is representable in both types. The combination means
that %x, though actually paired with unsigned, also accept uint8_t and
uint16_t, because those are guaranteed to promote to an int that meets
[1]. GCC recognizes [1] applies here.
(There's also PRI16x, but that's a bit tedious to use and, in glibc, is
defined as plain "x" anyway.)
[0] https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions
[1] https://en.cppreference.com/w/c/variadic/va_arg
Bug: 450
Change-Id: Ic1d41356755a18ab922956dd2e07b560470341f4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50765
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
There are a lot of d2i and i2d functions, and there will be even more
once asn1.h and x509.h are properly documented. We currently replicate
the text in each, but as a result a miss a few points:
- The i2d outp != NULL, *outp == NULL case isn't documented at all.
- We should call out what to do with *inp after d2i.
- Unlike our rewritten functions, object reuse is still quite rampant
with the asn1.h functions. I hope we can get rid of that but, until we
can, it would be nice to describe it in one place.
While I'm here, update a few references to the latest PKCS#1 RFC, and
try to align how we reference ASN.1 structures a bit. The d2i/i2d
functions say "ASN.1, DER-encoded RSA private key" while the CBS/CBB
functions say "DER-encoded RSAPrivateKey structure".
Bug: 426
Change-Id: I8d9a7b0aef3d6d9c8240136053c3b1704b09fd41
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49906
Reviewed-by: Adam Langley <agl@google.com>
We have a ton of per-file rotation functions, often with generic names
that do not tell you whether they are uint32_t vs uint64_t, or rotl vs
rotr.
Additionally, (x >> r) | (x << (32 - r)) is UB at r = 0.
(x >> r) | (x << ((-r) & 31)) works for 0 <= r < 32, which is what
cast.c does. GCC and Clang recognize this pattern as a rotate, but MSVC
doesn't. MSVC does, however, provide functions for this.
We usually rotate by a non-zero constant, which makes this moot, but
rotation comes up often enough that it's worth extracting out. Some
particular changes to call out:
- I've switched sha256.c from rotl to rotr. There was a comment
explaining why it differed from the specification. Now that we have
both functions, it's simpler to just match the specification.
- I've dropped all the inline assembly from sha512.c. Compilers should
be able to recognize rotations in 2021.
Change-Id: Ia1030e8bfe94dad92514ed1c28777447c48b82f9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49765
Reviewed-by: Adam Langley <agl@google.com>
Node.js uses EVP_PKEY_get0, which is present in OpenSSL but which
BoringSSL currently does not export. This CL adds an implementation
for it, which Electron is currently floating as a patch.
See
6a7eb32c5b
from Node.
Change-Id: I2474cacbd22882355a8037e2033739f7496b21f2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47824
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Most asymmetric operations scale superlinearly, which makes them
potential DoS vectors. This (and other problems) are mitigated with
fixed sizes, like RSA-2048, P-256, or curve25519.
In older algorithms like RSA and DSA, these sizes are conventions rather
than well-defined algorithms. "Everyone" uses RSA-2048, but code which
imports an RSA key may see an arbitrary key size, possibly from an
untrusted source. This is commonly a public key, so we bound RSA key
sizes in check_modulus_and_exponent_sizes.
However, some applications import external private keys, and may need
tighter bounds. These typically parse the key then check the result.
However, parsing itself can perform superlinear work (RSA_check_key or
recovering the DSA public key).
This CL does the following:
- Rename check_modulus_and_exponent_sizes to rsa_check_public_key and
additionally call it from RSA_check_key.
- Fix a bug where RSA_check_key, on CRT-less keys, did not bound d, and
bound p and q before multiplying (quadratic).
- Our DSA verifier had stricter checks on q (160-, 224-, and 256-bit
only) than our DSA signer (multiple of 8 bits). Aligner the signer to
the verifier's checks.
- Validate DSA group sizes on parse, as well as priv_key < q, to bound
the running time.
Ideally these invariants would be checked exactly once at construction,
but our RSA and DSA implementations suffer from some OpenSSL's API
mistakes (https://crbug.com/boringssl/316), which means it is hard to
consistently enforce invariants. This CL focuses on the parser, but
later I'd like to better rationalize the freeze_private_key logic.
Performance of parsing RSA and DSA keys, gathered on my laptop.
Did 15130 RSA-2048 parse operations in 5022458us (3012.5 ops/sec)
Did 4888 RSA-4096 parse operations in 5060606us (965.9 ops/sec)
Did 354 RSA-16384 parse operations in 5043565us (70.2 ops/sec)
Did 88 RSA-32768 parse operations in 5038293us (17.5 ops/sec) [rejected by this CL]
Did 35000 DSA-1024/256 parse operations in 5030447us (6957.6 ops/sec)
Did 11316 DSA-2048/256 parse operations in 5094664us (2221.1 ops/sec)
Did 5488 DSA-3072/256 parse operations in 5096032us (1076.9 ops/sec)
Did 3172 DSA-4096/256 parse operations in 5041220us (629.2 ops/sec)
Did 840 DSA-8192/256 parse operations in 5070616us (165.7 ops/sec)
Did 285 DSA-10000/256 parse operations in 5004033us (57.0 ops/sec)
Did 74 DSA-20000/256 parse operations in 5066299us (14.6 ops/sec) [rejected by this CL]
Update-Note: Some invalid or overly large RSA and DSA keys may
previously have been accepted that are now rejected at parse time. For
public keys, this only moves the error from verification to parsing. In
some private key cases, we would previously allow signing with those
keys, but the resulting signatures would not be accepted by BoringSSL
anyway. This CL makes us behave more consistently.
Bug: oss-fuzz:24730
Change-Id: I4ad2003ee61138b693e65d3da4c6aa00bc165251
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42504
Reviewed-by: Adam Langley <agl@google.com>
Prior to 5d7c2f8b1d, these i2d functions would fail, rather than crash,
if passed a NULL argument. While we don't generally have much truck with
the idea that functions should be expected to handle NULL arguments
where not documented, it seems that a fair amount of code is depending
on this.
Change-Id: I928b35533aa2a7beed57d7f58ba44681a8eb05c6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42464
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Code which targets OpenSSL won't use EVP_parse_public_key. X509_PUBKEY
is fairly deeply tied to the old ASN.1 stack, but there's no reason for
i2d_PUBKEY and friends to be. Move them to crypto/evp and reimplement as
wrappers over our functions.
Bug: chromium:1102458
Change-Id: Ic11766acdac797602e4abe1253b0efe33faef298
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42005
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Adam Langley
Shelley Vohr
Daniel McArdle