They're the only two half-finished ports left, so we may as well finish
that up and trim them down a little.
Change-Id: Ic058124a44086161ab5d2d6fa24448492c3ba219
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55506
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Disable blinding for boringssl_self_test_rsa() to avoid an entropy draw like
the 'k' value for ECDSA is fixed to avoid an entropy draw in boringssl_self_test_ecc().
The boringssl_self_test_rsa() use entropy to generate the blinding factor and
the inverse of blinding factor. Running boringssl_self_test_rsa() from init stage of OS
on some devices as the kernel's entropy pool is not yet initialized, causing the process
to block for seconds.
Bug: None
Change-Id: I4c1119c9950553eec030bedf36ec22ab41088f20
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55545
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
For the C files, rather than force the caller to juggle
crypto_linux_sources, etc., we just wrap the whole file in ifdefs and
ask the callers to link everything together.
Assembly is typically built by a different tool, so we have less room
here. However, there are really only two families of tools we care
about: gas (which runs the C preprocessor) and nasm (which has its own
preprocessor). Callers should be able to limit themselves to
special-casing Windows x86(_64) for NASM and then pass all the remaining
assembly files to their gas-like tool. File-wide ifdefs can take care of
the rest.
We're almost set up to allow this, except the files condition on
architecture, but not OS. Add __ELF__, __APPLE__, and _WIN32 conditions
as appropriate.
One subtlety: the semantics of .note.GNU-stack are that *any* unmarked
object file makes the stack executable. (In current GNU ld. lld doesn't
have this issue, and GNU ld claims they'll remove it in a later
release.) Empirically, this doesn't seem to apply to empty object files
but, to be safe, we should ensure all object files have the marking.
That leads to a second subtlety: on targets where @ is a comment,
@progbits is spelled %progbits, per [0]. If we want all .S files to work
in all targets, that includes these markers. Fortunately, %progbits
appears to work universally (see [1], [2], [3], [4]), so I've just
switched us to that spelling.
I've also tightened up the __arm__ and __aarch64__ checks to __ARMEL__
and __AARCH64EL__. We don't support big-endian Arm (or any other
platform) and, even if we did, the conditions in the assembly files
should match the conditions in the C files that pull them in.
This CL doesn't change our build to take advantage of this (though I'll
give it a go later), just makes it possible for builds to do it.
[0] https://sourceware.org/binutils/docs/as/Section.html
[1] https://patchwork.kernel.org/project/linux-crypto/patch/20170119212805.18049-1-dvlasenk@redhat.com/#20050285
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92820#c11
[3] https://sourceware.org/legacy-ml/gdb-patches/2016-01/msg00319.html
[4] de990b270d
Bug: 542
Change-Id: I0a8ded24423087c0da13bd0335cbd757d4eee65a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55626
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Bug: 516
Change-Id: Ifd381d1a2ed30aed6ffe84eb83d8fb4d93ec02ba
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55451
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Less code, and internally handles overflows. (Although this one cannot
overflow.)
Bug: 516
Change-Id: I3c2718075689d2815a43534a578a323c52787223
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55452
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
BIO_ctrl is one of OpenSSL's ioctl-style patterns, where they jam many
different function signatures into one type. BIO_ctrl returns long for
the sake of other operations, but many of them are only allowed to
return int.
Bug: 516
Change-Id: Ieffad1da89c60a538f142b12bdebdb950efd5c6a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55454
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Along the way, this fixes some size_t truncations.
Bug: 516
Change-Id: Iff0cf6ced0b7deb4c48c268e051a4da433088056
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55453
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Some of them were flagging -Wshorten-64-to-32 warnings. None of these
values are long, so just remove them. (I suspect this assumes unsigned
int is at least 32-bit, but we already assume this rather than wrap all
32-bit constants in UINT32_C(x).)
Ideally the c2l and l2c macros could be replaced with the load/store
functions but, like with the ciphers in decrepit, this is probably not
worth the effort for DES.
Bug: 516
Change-Id: I19e8cd4a321c20b9a22e4c007d943185c10755bb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55450
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
tag and utype are always accessed as int, so make the structs match.
Boolean ASN1_ITEMs put an ASN1_BOOLEAN in it->size, so add a cast. Also
fix the time set_string functions to call the underlying CBS parser
directly, so they don't need to put a strlen into an int.
Bug: 516
Change-Id: Ie10e7eaf58ec0b0dec59813a0ddcb0197fce1fd1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55449
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
We currently shift between unsigned long and int.
Bug: 516
Change-Id: I9e3fcc9393e24a352a2c08b9df0650a508d7a60b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55448
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
It would be nice to have a single-shot EVP_CIPHER_CTX API. This function
is not it.
EVP_Cipher is absurd. It's actually just exposing the internal
EVP_CIPHER 'cipher' callback, whose calling convention is extremely
complex. We've currently documented it as a "single-shot" API, but it's
not single-shot either, as it does update cipher state. It just can't
update across block boundaries.
It is particularly bizarre for "custom ciphers", which include AEADs,
which completely changes the return value convention from
bytes_written/-1 to 1/0, but also adds a bunch of magic NULL behaviors:
- out == NULL, in != NULL: supply AAD
- out != NULL, in != NULL: bulk encrypt/decrypt
- out == NULL, in == NULL: compute/check the tag
Moreover, existing code, like OpenSSH, relies on this behavior. To
ensure we don't break it when refactoring EVP_CIPHER internals, capture
the current behavior in tests. But also, no one should be using this in
new code, so deprecate it.
Upstream hasn't quite deprecated it, they now say "Due to the
constraints of the API contract of this function it shouldn't be used in
applications, please consider using EVP_CipherUpdate() and
EVP_CipherFinal_ex() instead."
Bug: 494
Change-Id: Icfe39a8fbbc860b03c9861f4164b7ee8da340216
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55391
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This API supports that too, so we should test it.
Bug: 494
Change-Id: If978705b3120697cc18fc91c06ca950e0e93bcf3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55390
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
See upstream's 70d8b304d01b9e0c4ec182db20c33aa0698cda51. Also import
upstream's AES-192-GCM tests, now that we support that in EVP_CIPHER
too.
Bug: 494
Change-Id: I157ebe23f32147214641aeb664ce7db2e21bf86a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55389
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The most likely use of EVP_CIPHER_CTX copy is to work around
EVP_CIPHER_CTX's lack of separation between "key" and "pending
operation". That is, you'd probably first configure the key, save that
as the precomputed AES key schedule, and then mint copies for each
operation to avoid mutating your key-only EVP_CIPHER_CTX. Rearrange
things to test that point.
But, while I'm here, we can just sprinkle copies throughout the whole
operation and ensure it still works.
Bug: 494
Change-Id: I464aa265cee317020d6b3ae3fd2ebfa92d7e12ae
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55388
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The main FileTest callback didn't support invalid ciphertexts, while the
Wycheproof one did but reimplemented a lot of logic to discover
variations.
Rework this a bit so we have a single TestCipher function that's
agnostic to the FileTest setup. The Operation enum gains a
kInvalidDecrypt mode to specify that the ciphertext is invalid. As part
of this, we tighten up the test
This is to make the later changes easier:
1. We don't have any negative tests for EVP_CIPHER's tag check at all!
We only test it for EVP_AEAD right now. Now TestCipher can express
this.
2. The weird EVP_Cipher API has no tests. It has a weird calling
convention that was easier to test if calling code knew whether to
expect success or failure. Previously, kInvalidDecrypt was
implemented by checking if DoCipher failed, which makes it harder to
embed success/failure-specific assertions.
Along the way, I've made each function responsible for running through
the input once, with separate TestCipherAPI and TestLowLevelAPI helpers.
Otherwise we have to reset the spans and keep track of the state of
intermediate values.
Bug: 494
Change-Id: Ieab87c0c76586aefeb596cc90edd4910ff1b962f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55387
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
EVP_CIPH_NO_PADDING is a no-op when block_size is one, yet we sized the
output expecting it to always add a byte of padding. (I don't think this
makes a difference because most call sites of DoCipher set
EVP_CIPH_NO_PADDING.)
Bug: 494
Change-Id: Ic75e48a60e669270a093416b862ec03706e1d6ef
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55386
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
It's weird to have both "in" and "input" in the same function. Also the
vector const refs can be spans. This is a bit of preparatory cleanup
along the way to a larger refactor so we can do negative tests and test
the weird EVP_Cipher API more coherently.
Bug: 494
Change-Id: I5cddf1b3ab88b3419bd88ce15bee56a2016bcd57
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55385
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We have two places where the current cap on BIGNUM sizes (64 MiB) is too
large, both involving Montgomery reduction: bn_mul_mont allocates a
spare value on the stack, and BN_mod_exp_mont_constime needs to allocate
a buffer of up to 64 contiguous values, which may overflow an int.
Make BN_MONT_CTX reject any BIGNUM larger than 8 KiB. This is 65,536
bits which is well above our maximum RSA key size, 16,384 bits. Ideally
we'd just apply this in bn_wexpand, to all BIGNUMs across the board, but
we found one caller that depends on creating an 8 MiB BIGNUM.
Update-Note: This will not affect any cryptography implemented by
BoringSSL, such as RSA, but other callers may run into this limit. If
necessary, we can raise this a bit, but the stack allocation means we
don't want to go *significantly* beyond what's in this CL.
Fixed: 541
Change-Id: Ia00f3ea6714a5042434f446943db55a533752dc5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55266
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
powerbuf's layout is:
- num_powers values mod m, stored transposed
- one value mod m, tmp
- one value mod m, am
- (mont5-only) an extra copy of m
powerbuf_len broadly computed this, but where tmp + am would be
sizeof(BN_ULONG) * top * 2, it used
sizeof(BN_ULONG) * max(top * 2, num_powers).
(In the actual code it's written as a ternary op and some
multiplications are factored out.)
That is, it allocated enough room for tmp + am OR an extra row in the
num_powers table, as if each entry were top + 1 words long instead of
top, with the space overlapping. This expression dates to upstream's
361512da0d,
though the exact layout has shifted over the years as mont5 evolved.
(Originally, it only contained one extra value mod m.)
At the time, this was necessary because bn_mul_mont_gather5 actually
overreads the table by one row! Although it only uses top * 32 words, it
requires the table to have (top + 1) * 32 words. This is because the
computation was scheduled so that the .Louter4x loop would read and mask
off the next iteration's value while incorporating the previous
iteration:
There were masked reads from $bp into XMM registers at the start of the
loop:
361512da0d/crypto/bn/asm/x86_64-mont5.pl (L541)
The XMM logic is interleaved throughout and does not move to a
general-purpose register, $m0, until much later. $m is not read again
until after the jump.
361512da0d/crypto/bn/asm/x86_64-mont5.pl (L700)
Meanwhile, the loop is already reading $m0 at the start of the
iteration.
361512da0d/crypto/bn/asm/x86_64-mont5.pl (L551)
The whole thing is bootstrapped by similar code just above it:
361512da0d/crypto/bn/asm/x86_64-mont5.pl (L531)
In the final iteration, we read one extra row into $m0 but never use it.
That is the overread.
I also confirmed this by rewinding our x86_64-mont5.pl to this state,
hacking things up until it built, and then hacking up
BN_mod_exp_mont_consttime to place the table in its own allocation, with
no extra slop using C11 aligned_alloc. This was so valgrind could
accurately instrument the bounds. When I did that, valgrind was clean if
I allocated (top + 1) * num_powers, but flagged an out-of-bounds read at
top * num_powers.
This no longer applies. After
25d14c6c29,
bn_mul_mont_gather5's scheduling is far less complicated. .Louter4x now
begins with a masked read, setting up $m0, and then it incorporates $m0
into the product. The same valgrind strategy confirmed this. Thus, I
don't believe this extra row is needed and we can allocate the buffer
straightforwardly.
Change-Id: I6c1ee8d5ebdb66eb4e5fec63d2140814c13ae146
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55231
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Since 31dcfcd080, delocate can drive cpp
itself to preprocess assembly inputs. This change switches the CMake
build to doing that and does it on all platforms in order to be more
uniform.
Change-Id: Ie28228fb1a4c63a2d43ab8a97f09cfe890ef39a1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55326
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Change-Id: I57a65d26c2a69f7084ea80b1a565ed7cb89b2a72
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55228
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
FileTests run sequentially and cannot be filtered. Split them up so it's
easier to, say, just run the ModExp ones. Also our test sharding
machinery will do a slightly better job parallelizing them when split up
like this. (This is one of our slower tests.)
Change-Id: Ie69864982f043655f68e592440b1f36e971b033a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55230
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
GTest likes to dump the underlying bytes for parameters which, in its
fallback paths, tends to hit uninitialized memory. See
https://github.com/google/googletest/issues/3805
Work around this. Use the NID, rather than the whole EC_builtin_curve
for ECCurveTest, and then don't use TEST_P for one of the BIO tests at
all.
Change-Id: Ic578d1a1b08294b0cd2f13b3bd17f23f6e5f996d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55229
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
BN_mod_exp_mont_consttime originally assumed accesses within a cache
line were indistinguishable and indexed into a cache line with secret
values. As a result, it required all of its tables, etc., to be
cache-line-aligned. Nowadays, the standard constant time memory model is
to assume the whole address leaks and not make these assumptions.
In particular, CacheBleed (CVE-2016-0702) showed this assumption was
false and which cache bank you accessed as leaked. OpenSSL's fix for the
assembly (mont5 and rsaz) appears to match the standard constant-time
model. However, its fix to the C code narrowed the assumption to cache
banks, so the alignment was still necessary.
After https://boringssl-review.googlesource.com/c/boringssl/+/33268, we
dropped this and use the standard model. All together, it should mean we
no longer make assumptions about cache lines. Update all the comments
and variable names accordingly.
Change-Id: I7bcb828eb2751a0167c3a3c8242b1b3971efc708
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55227
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
I have a use for these in the chrome verifier conversions, we
could choose to make them hidden again after a future move to
boringssl..
Change-Id: If059debbdf482d64577ad04c1ec4f9c82724de1e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55305
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Back in https://boringssl-review.googlesource.com/c/boringssl/+/33268, I
wrote that I had no idea what the mont5 assembly was doing. In
preparation for fixing up some comments around
BN_mod_exp_mont_consttime, I wanted to understand whether we were still
making assumptions about cache lines.
Happily, for the mont5 code, the answer is no, we are not. We just make
a bunch of masks and apply them in the natural way. But we do require
16-byte alignment on the table, because we use movdqa to read out of it.
I didn't look as closely at RSAZ, but I believe it too is fine. It
fairly quickly tosses $power into an XMM register and builds up masks,
rather than incorporating it into address computations.
(Both scatter5 functions incorporate it into the address, but that's
part of table building, where the index is public. I've updated the
comments to note when the index is secret or public.)
There is one reference to cache lines in the comments of mont5.pl, in
computing $N. However, $N has been unused since
https://boringssl-review.googlesource.com/c/boringssl/+/7244. (There are
references to $N[0] and friends, but those refer to @N, which is a
completely unrelated variable.) Remove it.
Change-Id: I1fac0660dffcd1380572029de2e5baece60cddf6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55225
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
ioutil has been deprecated since Go 1.16. The functions were moved to
some combination of io and os. See https://pkg.go.dev/io/ioutil.
(File-related functions went to os. Generic things went to io. Names
were kept the same except TempDir and TempFile are os.MkdirTemp and
os.CreateTemp, respectively.)
Change-Id: I031306f69e70424841df08f64fa9d90f31780928
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55186
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
The indices do fit in unsigned, but we're not taking any advantage of
this because of struct padding, and the RSA structure is not that
memory-sensitive.
Bug: 516
Change-Id: I678e20fcd6f6fa8f69eaef1f4108fa94194b6ee7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55270
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
One less function to make size_t-clean.
Update-Note: All callers of this function since been removed.
Change-Id: I4cd77ede5f58cdbc3cf65365a8fd23967545ecfa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55269
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
ASN1_ENCODING has a 'modified' bit, but every time it is set, the
contents are both ignored and never filled in again (we don't fill in
the encoding except on parse). That means keeping the underlying buffer
around is just wasting memory. Remove the bit and use the len != 0 to
determine if there's a saved encoding. Replace all the modified bits
with a helper function that drops the encoding.
I don't think we need a separate "present" boolean and can just treat
empty as not saved; a cached value always has a tag and length, so it
cannot be empty. (Even if it could be empty, that would imply the
value's encoding is trivial enough that we probably don't need the saved
encoding to preserve the value.)
Change-Id: I6beda94d33f3799daf85f1397818b9a41e7dd18a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55267
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We use unsigned, but we actually assume it is 32-bit for the bit-packing
strategy. But also introduce a typedef to hint that callers shouldn't
treat it as an arbitrary 32-bit integer. A typedef would also allow us
to extend to uint64_t in the future, if we ever need to.
Update-Note: Some APIs switch from unsigned * to uint32_t * out
pointers. This is only source-compatible if unsigned and uint32_t are
the exact same type. The CQ suggests this is indeed true. If they are
not, replace unsigned with CBS_ASN1_TAG to fix the build.
Bug: 525
Change-Id: I45cbe127c1aa252f5f6a169dca2e44d1e6e1d669
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54986
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
I had a rewrite of the decrepit ciphers (CAST and Blowfish) to use
CRYPTO_{load,store}_u32_be and drop the old macros, but this is probably
not worth the effort to review. Instead, just fix the type in the macro.
Bug: 516
Change-Id: I1cdecc16f6108a6235f90cf9c2198bc797c6716e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54985
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We have a number of APIs that cannot migrate to size_t because OpenSSL
used negative numbers as some special indicator. This makes it hard to
become size_t-clean.
However, in reality, the largest buffer size is SSIZE_MAX, or, more
accurately PTRDIFF_MAX. But every platform I've ever seen make ptrdiff_t
and size_t the same size. malloc is just obligated to fail allocations
that don't fit in ssize_t. ssize_t itself is not portable (Windows
doesn't have it), but we can define ossl_ssize_t to be ptrdiff_t.
OpenSSL also has an ossl_ssize_t (though they don't use it much), so
we're also improving compatibility.
Start this out with ASN1_STRING_set. It still internally refuses to
construct a string bigger than INT_MAX; the struct can't hold this and
even if we fix the struct, no other code, inside or outside the library,
can tolerate it. But now code which passes in a size_t (including our
own) can do so without overflow.
Bug: 428, 516
Change-Id: I17aa6971733f34dfda7d971882d0f062e92340e9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54953
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@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>
The counter is accessed as x2, not w2, so this is a uint64_t parameter.
Change-Id: I97a5dabc521fc00fc366a67712bc4932b256532f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55145
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: 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>
OpenSSL uses integer parameters for this function, and the
multiplication here ends up being done as an integer. Since we
support values up to year 9999, it is possible for someone to pass
in a number of days to the "adj" function to adjust a base time far
enough to overflow a 32 bit integer.
Change-Id: Iedfc33d8bf90d70049f99897df1d193fb29805d0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55125
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Should the string be INT_MAX, we cannot actually represent the output
length. i2c_ASN1_INTEGER and ASN1_object_size have checks this, but this
was missing it.
Change-Id: I7cf5debb87568b876f3799308ef4ad6d2b1ff7e6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55085
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
OpenSSL added a bunch of these. oct2priv is a little weird (see
https://crbug.com/boringssl/534), but I've made it match OpenSSL and
set_private_key for now. But I think we should reduce the state-space a
bit.
EC_KEY_oct2priv behaves slightly differently from upstream OpenSSL in
one way: we reject inputs that aren't exactly the right size. This
matches the OpenSSL documentation (the OCTET STRING inside an
ECPrivateKey, per spec, is fixed-width), but not OpenSSL's behavior.
Update-note: see go/xshow when incorporating this change internally.
Change-Id: I33863d773ac4c7f3eabf4ffda157e8250c7fdbd9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55066
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
https://boringssl-review.googlesource.com/c/boringssl/+/41084
inadvertently added a somewhat expensive operation (field inversion) in
the path of EC_POINT_point2oct when passed with buf == NULL. The result
is a caller that calls the function twice, first to measure and then to
serialize, actually ends up doing the field inversion twice.
Fix this by removing the dual-use calling convention from the internal
function and just have a separate function to measure the output size
separately. It's slightly subtle because EC_POINT_point2oct would check
for the point at infinity by way of converting to affine coordinates, so
we do need to repeat that check.
As part of this, add a unit test for
https://boringssl-review.googlesource.com/6488, which rejected the point
at infinity way back.
Change-Id: I3b6c0f95cced9c00489386f064a2c3f0bb1776f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55065
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@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>
Also build with -Wtype-limits to catch future instances.
Bug: 529
Change-Id: I2d84dc1824ffc7cd92411f49c9f953bcd3c74331
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55045
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: 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>
David Benjamin
hehe.li
Adam Langley
Bob Beck