OpenSSL added a similar helper function. It's very, very common for us
to malloc something an then zero it. This saves some effort. Also
replace some more malloc + memcpy pairs with memdup.
Change-Id: I1e765c8774a0d15742827c39a1f16df9748ef247
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63345
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Also remove unnecessary EC_GROUP_free calls. EC_GROUP_free is only
necessary in codepaths where arbitrary groups are possible.
Bug: 20
Change-Id: I3dfb7f07b890ab002ba8a302724d8bc671590cfe
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60932
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
They end up in uint64_t frequently right now just because the CBS APIs
use uint64_t, but we don't actually accept that large of an iteration
count.
Also use uint32_t instead of unsigned. This type should be sized based
on how large of an iteration count we think is reasonable, not something
platform-dependent.
Bug: 516
Change-Id: Ie5ff379af6bc65c5e4d25f4d10774bd819f08a50
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61426
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
BN_MONT_CTX already has the modulus, so just use it. This is one less
value to initialize statically.
Bug: 20
Change-Id: I78f73994ab595b795e99d67851bdff3b73fc3dd6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60926
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We really should remove these (we only support them in private keys)
but, in the meantime, add some tests for all the curves, not just P-256.
Change-Id: I9c4c0660f082fa1701afe11f51bb157b06befd3c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60925
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
RSA CRT is tiny bit messier when p < q.
https://boringssl-review.googlesource.com/25263 solved this by
normalizing to p > q. The cost was we sometimes had to compute a new
iqmp.
Modular inversion is expensive. We did it only once per key, but it's
still a performance cliff in per-key costs. When later work moves
freeze_private_key into RSA private key parsing, it will be a
performance cliff in the private key parser.
Instead, just handle p < q in the CRT function. The only difference is
needing one extra reduction before the modular subtraction. Even using
the fully general mod_montgomery function (as opposed to checking p < q,
or using bn_reduce_once when num_bits(p) == num_bits(q)) was not
measurable.
In doing so, I noticed we didn't actually have tests that exercise the
reduction step. I added one to evp_tests.txt, but it is only meaningful
when blinding is disabled. (Another cost of blinding.) When blinding is
enabled, the answers mod p and q are randomized and we hit this case
with about 1.8% probability. See comment in evp_test.txt.
I kept the optimization where we store iqmp in Montgomery form, not
because the optimization matters, but because we need to store a
corrected, fixed-width version of the value anyway, so we may as well
store it in a more convenient form.
M1 Max
Before:
Did 9048 RSA 2048 signing operations in 5033403us (1797.6 ops/sec)
Did 1500 RSA 4096 signing operations in 5009288us (299.4 ops/sec)
After:
Did 9116 RSA 2048 signing operations in 5053802us (1803.8 ops/sec) [+0.3%]
Did 1500 RSA 4096 signing operations in 5008283us (299.5 ops/sec) [+0.0%]
Intel(R) Xeon(R) Gold 6154 CPU @ 3.00GHz
Before:
Did 9282 RSA 2048 signing operations in 5019395us (1849.2 ops/sec)
Did 1302 RSA 4096 signing operations in 5055011us (257.6 ops/sec)
After:
Did 9240 RSA 2048 signing operations in 5024845us (1838.9 ops/sec) [-0.6%]
Did 1302 RSA 4096 signing operations in 5046157us (258.0 ops/sec) [+0.2%]
Bug: 316
Change-Id: Icb90c7d5f5188f9b69a6d7bcc63db13d92ec26d5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60705
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
While EVP_PKEY_RSA, EVP_PKEY_DSA, and EVP_PKEY_EC have publicly-exposed
internaly representations, other EVP_PKEY types to not. EVP_PKEY_assign
should not allow callers to manipulate those representations.
As part of this, teach EVP_PKEY_assign_RSA, etc. to find their method
tables directly, rather than indirecting through an integer. This makes
those EVP APIs static-linker-friendly.
Bug: 618, 497
Change-Id: Ic45a7514e9a3adc505759f2327129f13faf03a65
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60645
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Update-Note: Accessing the DSA struct directly is no longer supported.
Use accessors instead.
Bug: 325
Change-Id: I73dc20f2e275d48648ca84d2db7806fe155c567d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60425
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@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>
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
https://github.com/openssl/openssl/issues/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>
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
https://github.com/openssl/openssl/issues/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>
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>
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>
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>
Caught by running malloc failure tests on unit tests.
Bug: 563
Change-Id: Ic0167ef346a282dc8b5a26a1cedafced7fef9ed0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56927
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
RSA state management is generally a mess right now, which causes thread
contention issues in highly threaded servers. We need to do a lot
of work within the library to fix it, but in the end state,
RSA_check_key (called by the parser), BN_MONT_CTX_set_locked, and
freeze_private_key should all be unified.
This means that anything which can causes the latter two steps to fail
will be lifted up into the parser, currently RSA_check_key. We've
broadly done that, but odd moduli (n, p, and q) are currently not
covered by RSA_check_key. Fix that. We only need to check for odd n,
because odd p and q are then implied by p * q == n.
Update-Note: RSA keys with even moduli already do not work. (In addition
to being nonsensical, all operations will fail with them because we
cannot do Montgomery reduction on even moduli.) This CL shifts the error
from when you use the key, to when you parse the key, like our other
validation steps. Also after this lands, the check for odd modulus in
cl/447099278 can be removed.
Bug: 316
Change-Id: Ifa4af610316a8f717a026128078a5d38d046bff9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56885
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
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
Bob Beck
Adam Langley
Shelley Vohr
Daniel McArdle