Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge BoringSSL through 27e45c43420125ed293d4646ddf8ff2c321f01b9 #1651

Merged
merged 141 commits into from
Sep 26, 2023

Conversation

briansmith
Copy link
Owner

No description provided.

davidben and others added 30 commits July 25, 2022 15:48
On non-ELF platforms, WEAK_SYMBOL_FUNC expands to a static variable. On
ASan, we don't use sdallocx. Clang then warns about an unused static
variable. Silence the warning.

Change-Id: I3d53519b669d435f3801f45e4b72c6ca4cd27a3b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53565
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This is split out from
https://boringssl-review.googlesource.com/c/boringssl/+/47544 just to
get the bugfixes and tests out of the way of the refactor.

If we trip the SSL_R_BAD_LENGTH check in tls_write_app_data, wnum is set
to zero. But wnum should only be cleared on a successful write. It
tracks the number of input bytes that have been written to the transport
but not yet reported to the caller. Instead, move it to the success
return in that function. All the other error paths already set it to
something else.

Change-Id: Ib22f9cf04454ecdb0062077f183be5070ab7d791
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53545
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Writing application data goes through three steps:

1. Encrypt the data into the write buffer.
2. Flush the write buffer to the network.
3. Report to SSL_write's caller that the write succeeded.

In principle, steps 2 and 3 are done together, but it is possible that
BoringSSL needs to write something, but we are not in the middle of
servicing an SSL_write call. Then we must perform (2) but cannot perform
(3).

TLS 1.3 0-RTT on a client introduces a case like this. Suppose we write
some 0-RTT data, but it is blocked on the network. Meanwhile, the
application tries to read from the socket (protocols like HTTP/2 read
and write concurrently). We discover ServerHello..Finished and must then
respond with EndOfEarlyData..Finished. But to write, we must flush the
current write buffer.

To fix this, https://boringssl-review.googlesource.com/14164 split (2)
and (3) more explicitly. The write buffer may be flushed to the network
at any point, but the wpend_* book-keeping is separate. It represents
whether (3) is done. As part of that, we introduced a wpend_pending
boolean to track whether there was pending data.

This introduces an interesting corner case. We now keep NewSessionTicket
messages buffered until the next SSL_write. (KeyUpdate ACKs are
implemented similarly.) Suppose the caller calls SSL_write(nullptr, 0)
to flush the NewSessionTicket and this hits EWOULDBLOCK. We'll track a
zero-length pending write in wpend_*! A future attempt to write non-zero
data would then violate the moving buffer check. This is strange because
we don't build records for zero-length application writes in the first
place.

Instead, wpend_pending should have been wpend_tot > 0. Remove that and
rearrange the code to check that properly. Also remove wpend_ret as it
has the same data as wpend_tot.

Change-Id: I58c23842cd55e8a8dfbb1854b61278b108b5c7ea
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53546
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
27ffcc6 switched the integrity check to using SHA-256, but the
Aarch64 FIPS build was still passing -sha256 to inject_hash.go.

Change-Id: I641de17d62205c7f127cd2a910d4e98778d492e7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53605
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
p256-armv8-asm.pl defined ecp_nistz256_[to|from]_mont as global
functions, but p256-nistz.h defined them as static inlines.
Additionally, ecp_nistz256_to_mont was never used.

This change drops the assembly versions and drops ecp_nistz256_to_mont
completely.

Change-Id: Ie2cc5bf4adc423f72f61cf227be0e93c9a6e2031
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53606
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
b951243 readded 3DES support in acvptool, but not in
modulewrapper because we don't want it for BoringSSL itself. But without
modulewrapper support, the tests don't work. Support could be backported
into testmodulewrapper but it doesn't seem worthwhile for a few more
months support.

Change-Id: I4e7ace66f9ac1915996db7dfdeeb7e9d4969915f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53607
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: Ie071dcd94d2ae8aa8ee148682f9b0054ed9e3501
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52445
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Follow-up work will add support for TLS 1.2 ticket decryption.

Bug: 504
Change-Id: Ieaee37d94562040f1d51227216359bd63db15198
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53525
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This flag is currently set if DRBG entropy is obtained from RDRAND. It
indicates that we should add kernel entropy when seeding the DRBG. But
this might be true for methods other than RDRAND in the future so this
change renames it accordingly.

Change-Id: I91826178a806e3c6dadebbb844358a7a12e0b09b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52525
Reviewed-by: David Benjamin <davidben@google.com>
When seeding a DRBG for the first time we currently make two reads: one
to start the CRNGT and a second to read the actual seed. These reads can
be merged to save I/O.

Change-Id: I2a83edf7f3c8b9d6cebcde02195845be9fde19b2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52526
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I69aba15ccf57d04c66a98755b98221b8688d291a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52527
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This reverts commit 4259ae8.

Some Android builders perhaps lack getrandom support.

Change-Id: Ic7537c07dacb31a54adb453ddd5f82a789089eaf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53625
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We still have our <= 0 return values because anything with BIOs tries to
preserve BIO_write's error returns. (Maybe we can stop doing this?
BIO_read's error return is a little subtle with EOF vs error, but
BIO_write's is uninteresting.) But the rest of the logic is size_t-clean
and hopefully a little clearer. We still have to support SSL_write's
rather goofy calling convention, however.

I haven't pushed Spans down into the low-level record construction logic
yet. We should probably do that, but there are enough offsets tossed
around there that they warrant their own CL.

Bug: 507
Change-Id: Ia0c702d1a2d3713e71b0bbfa8d65649d3b20da9b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47544
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This works correctly, but part of implementing SSL_write_ex will, if not
done correctly, regress this. Specifically, if the read_shutdown check
in SSL_get_error were not conditioned on ret == 0, the last
SSL_get_error in the test would mistakenly classify the write error as
SSL_ERROR_ZERO_RETURN.

Add a regression test in advance.

Bug: 507
Change-Id: I8ddb4606e291977506ee81f4ed11427e5b1636d8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53626
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We only care about dates within years 0000 to 9999 for
RFC5280. timegm() is only semi-standard. Some things require the
setting awkward defines to get libc to give it to you. Other things
let you have it but make it stop working at year 3000. Still other
things have 32 bit time_t.....

Let's just make our own that actually works. all the time, does
everything with an int64_t, and fails if you want to send something
out that would overflow a 32 bit time_t.

In the process of doing this, we get rid of the old Julian date stuff
from OpenSSL, which while functional was a bit awkward dealing only
with days, and using the Julian calendar as the reference point instead of potentially something more useful. Julian seconds since Jan 1 1970
00:00:00 UCT are much more useful to us than Julian days since a
Julian epoch.

The OS implementations of timegm() and gmtime() also can be pretty
complex, due to the nature of needing multiple timezone, daylight
saving, day of week, and other stuff we simply do not need for
doing things with certificate times. A small microbenchmark of
10000000 of each operation comparing this implementation to
the system version on my M1 mac gives:

bbe-macbookpro:tmp bbe$ time ./openssl_gmtime

real    0m0.152s
user    0m0.127s
sys     0m0.018s
bbe-macbookpro:tmp bbe$ time ./gmtime

real    0m0.422s
user    0m0.403s
sys     0m0.014s
bbe-macbookpro:tmp bbe$ time ./openssl_timegm

real    0m0.041s
user    0m0.015s
sys     0m0.019s
bbe-macbookpro:tmp bbe$ time ./timegm

real    0m30.432s
user    0m30.383s
sys     0m0.040s

Similarly On a glinux machine:

bbe@bbe-glinux1:~$ time ./openssl_gmtime

real    0m0.157s
user    0m0.152s
sys     0m0.008s
bbe@bbe-glinux1:~$ time ./gmtime

real    0m0.336s
user    0m0.336s
sys     0m0.002s
bbe@bbe-glinux1:~$ time ./openssl_timegm

real    0m0.018s
user    0m0.019s
sys     0m0.002s
bbe@bbe-glinux1:~$ time ./timegm

real    0m0.680s
user    0m0.671s
sys     0m0.011s
bbe@bbe-glinux1:~$


Bug: 501

Change-Id: If445272d365f2c9673b5f3264d082af1a342e0a1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53245
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: 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>
We need to know which ARM instructions take a condition code because
otherwise the conditions look like symbols. This change includes all
instructions beginning with 'c' from [1] that include a `cond` argument.
Also sort them for easier comparison.

[1]
https://developer.arm.com/documentation/dui0802/a/A64-General-Instructions/CBNZ

Change-Id: Iea07aa4afe171d684135ff6655c52374d86529ce
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53745
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Tests can now be run either in a local build or on an attached
device.  The script tries to infer the correct mode of operation
but it can also be specified on the command line.

Test: Ran break-tests.sh in both modes
Change-Id: I515ac0cede23e2cb775b99e0af8108a3ce0bde37
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53585
Reviewed-by: Adam Langley <agl@google.com>
This syscall is required by generatekey in keystore.

Signed-off-by: Liu Cunyuan <liucunyuan.lcy@linux.alibaba.com>
Signed-off-by: Mao Han <han_mao@linux.alibaba.com>
Change-Id: I4dd0534daa6cfa52429e5bf398679fccb7d67e7f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53765
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
https://boringssl-review.googlesource.com/c/boringssl/+/53007
inadvertently changed the semantics of SSL_load_client_CA_file slightly.
The original implementation, by delaying allocating ret, would fail
rather than return an empty list.

Fix this and add a test. We don't have much support for testing
filesystem-related things yet, so I've just used /dev/null and gated it
to Linux + macOS for now. If we need it later, we can add temporary file
support to the test_support library.

Change-Id: If77dd493a433819a65378d76cf400cce48c0abaa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53785
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This comment refers to something that was removed in
https://boringssl-review.googlesource.com/c/boringssl/+/43889

Change-Id: Icf10ed5eb2ce552f2c1dbcdb89853cddb1183ad1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53786
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I1cf99586d72ee9c01e99ca6baa6479e5dd2aef5d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53787
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Rather than documenting the private sk_new_null, etc., APIs and then
expecting callers to infer the real API, describe a real sample API
under #if 0.

Also rename the function pointers to sk_FOO_whatever, which both matches
OpenSSL and reduces the namespaces we squat. The generic callback types
I've renamed to OPENSSL_sk_whatever, to similarly match OpenSSL. We
should also rename plain sk_whatever, but that'll require fixing some
downstream code.

Bug: 499
Change-Id: I49d250958d40858cb49eeee2aad38a17a63add87
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53009
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This runs through much the same code as the TLS 1.3 bits, though we use
a different hint field to avoid mixups between the fields. (Otherwise
the receiver may misinterpret a decryptPSK hint as the result of
decrypting the session_ticket extension, or vice versa. This could
happen if a ClientHello contains both a PSK and a session ticket.)

Bug: 504
Change-Id: I968bafe12120938e6e46e52536efd552b12c66a0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53805
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
More OpenSSL compatibility functions.

Change-Id: I8e9429fcbc3e285f4c4ad9bdf4c1d9d3c73c3064
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53925
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This was added in OpenSSL 1.1.0. cryptography.io binds it. They don't
actually use it, but this is a useful feature to have anyway. Projects
like Envoy currently implement such a mode with
X509_STORE_set_verify_cb, which is a very problematic API to support.
Add this so we can move them to something more sustainable.

Change-Id: Iaff2d08daa743e0b5f4be261cb785fdcd26a8281
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53965
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL renamed X509_STORE_CTX_trusted_stack to
X509_STORE_CTX_set0_trusted_stack. This name is a partially an
improvement as this is a setter, and partially a setback. The "set0"
name is a bit misleading.

set0 is narrowly correct, in that this function does not adjust
refcounts. But usually set0 functions don't adjust refcounts because
they take ownership of the input. This function does not. It simply
borrows the pointer and assumes it will remain valid for the duration of
X509_STORE_CTX.

OpenSSL also renamed X509_STORE_CTX_set_chain to
X509_STORE_CTX_set0_untrusted. I've declined to add that one for now, in
hopes that we can remove both functions. From what I can tell, there's
no point in ever using either function. It's redundant with the last
parameter to X509_STORE_CTX_init.

Change-Id: I0ef37ba56a2feece6f927f033bdcb4671225dc6f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53966
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
A later CL will tighten up SSL_ERROR_ZERO_RETURN handling. In
preparation for this, test that SSL_CTX_set_quiet_shutdown can trigger
SSL_ERROR_ZERO_RETURN.

Bug: 507
Change-Id: Ib50a02c514673ad4b73540934480d54b372d9505
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53945
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
CPython uses this function.

Change-Id: I03ead7f54ad19e2a0b2ea3b142298cc1e55c3c90
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53967
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Matching Chromium, Go, and TLS 1.3, only allow SHA-256, SHA-384, and
SHA-512 RSA-PSS signatures, where MGF-1 and message hash match and salt
length is hash length. Sadly, we are stuck tolerating an explicit
trailerField for now. See the certificates in cl/362617931.

This also fixes an overflow bug in handling the salt length. On
platforms with 64-bit long and 32-bit int, we would misinterpret, e.g,
2^62 + 32 as 32. Also clean up the error-handling of maskHash. It was
previously handled in a very confusing way; syntax errors in maskHash
would succeed and only be noticed later, in rsa_mgf1_decode.

I haven't done it in this change, but as a followup, we can, like
Chromium, reduce X.509 signature algorithms down to a single enum.

Update-Note: Unusual RSA-PSS combinations in X.509 are no longer
accepted. This same change (actually a slightly stricter version) has
already landed in Chrome.

Bug: 489
Change-Id: I85ca3a4e14f76358cac13e66163887f6dade1ace
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53865
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
davidben and others added 27 commits November 21, 2022 20:17
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>
CMake 3.10 was released November 20, 2017, which is now more than five
years ago.

Change-Id: Ic939fd137983914ce1041740f58d98a56433e739
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55271
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
The clang script needed to be tweaked slightly because they've since
changed the URL. Also libc++ now needs to be built as C++20. (The
bundled libc++ is only built in some of our test configs, so this
doesn't imply a C++20 dependency across the board.)

Change-Id: I0a9e3aed71268bcd37059af8549a23cfc0270b05
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55272
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Change-Id: Ia176cf8d03452e96ae8103fae40c9617a9dd71e1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55273
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: 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>
In the CMake build we did this with
https://boringssl-review.googlesource.com/c/boringssl/+/44847. But in
other environments delocate may need to run cpp itself.

Change-Id: I429e849f6d7c566aa14e63be6c8e93f9dd6847ed
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55306
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@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>
Change-Id: I26f90a3a9f81d71e4cc2bf13777492552227140d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55325
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Change-Id: Iea1cf557acc85e9bab7ddd50a15376ce77b1c65d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55226
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@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>
GTest likes to dump the underlying bytes for parameters which, in its
fallback paths, tends to hit uninitialized memory. See
google/googletest#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>
@briansmith briansmith self-assigned this Sep 25, 2023
@briansmith briansmith merged commit b04bed1 into main Sep 26, 2023
265 checks passed
@briansmith briansmith deleted the b/merge-boringssl-11 branch September 26, 2023 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.