Commit Graph

5608 Commits

Author SHA1 Message Date
David Benjamin
f77c8a38be Be less clever with CHECK_ABI.
Unwind testing will make CHECK_ABI much slower. The original
ptrace-based design is some 10,000x slower. I've found an alternate
design that's a mere 1,000x slower, but this probably warrants being
more straightforward. It also removes the weirdness where NDEBUG
controlled which tests were run.

While it does mean we need to write some extra tests for p256-x86_64.pl,
we otherwise do not directly unit test our assembly anyway. Usually we
test the public crypto APIs themselves. So, for most files, this isn't
actually extra work.

Bug: 181
Change-Id: I7cbb7f930c2ea6ae32a201da503dcd36844704f0
Reviewed-on: https://boringssl-review.googlesource.com/c/33965
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2019-01-03 21:02:24 +00:00
David Benjamin
cc5a888fe5 Update SDE and add the Windows version.
Windows is sufficiently different from Linux that running tests under
SDE for Windows, particularly with the new ABI tests, is worthwhile.

Change-Id: I32c4f6de06b2e732ebb2c1492eb1766cae73c0e0
Reviewed-on: https://boringssl-review.googlesource.com/c/34064
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2019-01-03 21:01:33 +00:00
Adam Langley
e6bf9065af Remove pooling of PRNG state.
Prior to 82639e6f we used thread-local data for the PRNG state. That
change switched to using a mutex-protected pool instead in order to save
memory in heavily-threaded applications.

However, the pool mutex can get extremely hot in cases where the PRNG is
heavily used. 8e8f2504 was a short-term work around, but supporting both
modes is overly complex.

This change moves back to the state of the prior to 82639e6f. The best
way to review this is to diff the changed files against '82639e6f^' and
note that the only difference is a comment added in rand.c:
https://paste.googleplex.com/4997991748337664

Change-Id: I8febce089696fa6bc39f94f4a1e268127a8f78db
Reviewed-on: https://boringssl-review.googlesource.com/c/34024
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2019-01-03 20:19:44 +00:00
Jeremy Apthorp
7177c1d29f Add EC_KEY_key2buf for OpenSSL compatibility
Change-Id: If45ef3a9bb757bd0c7f592f40ececaf4aa2f607d
Reviewed-on: https://boringssl-review.googlesource.com/c/33824
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2019-01-03 16:32:21 +00:00
David Benjamin
43e636a2e4 Remove bundled copy of android-cmake.
I don't believe we use this anymore. People using it should upgrade to a newer
NDK (or, worst case, download android-cmake themselves).

Change-Id: Ia99d7b19d6f2ec3f4ffe90795813b00480dc2d60
Reviewed-on: https://boringssl-review.googlesource.com/c/34004
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2019-01-03 16:28:10 +00:00
David Benjamin
6f9f4cc443 Clarify build requirements.
The minimum versions are largely bogus, since we do not continuously test them.
Instead, we've been using Abseil's five year guidelines to decide when to rely
on tooling improvements. Document this.

Remove the note on how to build Ninja as that'll just get out of date. For
instance, they appear to support Python 3 when building now.

Explicitly call out that CMake 3.0 will be required next year (released June
2014). 3.0 is the minimum needed to distinguish Clang from AppleClang, without
which version checks on Clang don't work.

Also document that we require a C++11 compiler for more than just tests these
days.

Change-Id: I4e5766934edc1d69f7be01f48e855d400adfb5f2
Reviewed-on: https://boringssl-review.googlesource.com/c/33845
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2019-01-02 23:57:14 +00:00
Jeremy Apthorp
79c7ec06f6 Add EC_GROUP_order_bits for OpenSSL compatibility
Change-Id: I37149fa4274357d84befff85728ce2337131afa7
Reviewed-on: https://boringssl-review.googlesource.com/c/33804
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2019-01-02 23:51:14 +00:00
David Benjamin
0eaf783fbf Annotate leaf functions with .cfi_{startproc,endproc}
While gdb can figure it out, libunwind requires CFI directives to
unwind a leaf function, even though the directives are trivial.
Adding them matches what GCC outputs, and likely gdb has many
heuristics that less complex tools (e.g. profilers) may not.

Bug: 181
Change-Id: I25c72152de33109a29710a828aeb99c608dd0470
Reviewed-on: https://boringssl-review.googlesource.com/c/33964
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2019-01-02 23:49:24 +00:00
David Benjamin
c2e8d016f5 Fix beeu_mod_inverse_vartime CFI annotations and preamble.
This was also caught by the in-progress unwind tester. There are two
issues here.  First, .cfi_endproc must come after ret to fully cover the
function.  More importantly, this function is confused about whether it
has a frame pointer or not.

It looks like it does (movq %rsp, %rbp), and annotates accordingly, but
it does not actually use the frame pointer. It cannot. $y4 is rbp and
gets clobbered immediately after the preamble!

Remove this instruction and align the CFI annotations with a
frame-pointer-less function.

Bug: 181
Change-Id: I47b5f9798b3bcee1748e537b21c173d312a14b42
Reviewed-on: https://boringssl-review.googlesource.com/c/33947
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2019-01-02 23:47:34 +00:00
David Benjamin
a306b1b908 Fix CFI annotations in p256-x86_64-asm.pl.
This was caught by in-progress work to test unwind information. It was
incorrect at two instructions: immediately before we jump to
.Lpoint_double_shortcut$x. This is needed because
ecp_nistz256_point_add$x tries to be clever about not unwinding the
stack frame in its tail call.

It's also unlikely that the SEH handlers in this file are correct at
this point, but that will be handled separately while overhauling
everything else here. (For Win64, probably the only ABI-compliant option
is to just properly unwind the stack frame. Without a custom handler,
Win64 unwind codes are very restrictive.)

Bug: 181
Change-Id: I9f576d868850312d6c14d1386f8fbfa85021b347
Reviewed-on: https://boringssl-review.googlesource.com/c/33946
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2019-01-02 23:39:21 +00:00
David Benjamin
6ef1b64558 Add a comment about ecp_nistz256_point_add_affine's limitations.
ecp_nistz256_point_add_affine does not support the doubling case and,
unlike ecp_nistz256_point_add which does a tail call, computes the wrong
answer. Note TestPointAdd in the unit tests skips this case.

This works fine because we only use ecp_nistz256_point_add_affine for
the g_scalar term, which is fully computed before the p_scalar term.
(Additionally it requires that the windowing pattern never hit the
doubling case for single multiplication.)

But this is not obvious from reading the multiplication functions, so
leave a comment at the call site to point this out.

Change-Id: I08882466d98030cdc882a5be9e702ee404e80cce
Reviewed-on: https://boringssl-review.googlesource.com/c/33945
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2019-01-02 23:33:31 +00:00
David Benjamin
1c55e54eda Refresh p256-x86_64_tests.txt.
The old points weren't even on the curve. I probably had no clue what I
was doing at the time when I generated them. Refresh them with a
checked-in generate script.

Change-Id: Ib4613fe922edcf45fc4ea49fc4c2cc23a9a2a9bd
Reviewed-on: https://boringssl-review.googlesource.com/c/33944
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2019-01-02 23:29:31 +00:00
David Benjamin
fb3f0638ba Fix some indentation nits.
perlasm's bizarre mix of asm and perl indentation and clever editors always
mess me up.

Change-Id: Iac906a636207867939cc327b4c21b8a982abce29
Reviewed-on: https://boringssl-review.googlesource.com/c/33844
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2019-01-02 19:26:54 +00:00
Adam Langley
8e8f250422 Use thread-local storage for PRNG states if fork-unsafe buffering is enabled.
We switched from thread-local storage to a mutex-pool in 82639e6f53
because, for highly-threaded processes, the memory used by all the
states could be quite large. I had judged that a mutex-pool should be
fine, but had underestimated the PRNG requirements of some of our jobs.

This change makes rand.c support using either thread-locals or a
mutex-pool. Thread-locals are used if fork-unsafe buffering is enabled.
While not strictly related to fork-safety, we already have the
fork-unsafe control, and it's already set by jobs that care a lot about
PRNG performance, so fits quite nicely here.

Change-Id: Iaf1e0171c70d4c8dbe1e42283ea13df5b613cb2d
Reviewed-on: https://boringssl-review.googlesource.com/c/31564
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-28 18:05:18 +00:00
David Benjamin
74944287e1 Add Win64 SEH unwind codes for the ABI test trampoline.
This is all manual right now. Once we've added SEH tests, we can add support
for emitting these in x86_64-xlate.pl, probably based on MASM and Yasm's unwind
directives, and unify with CFI. (Sadly, NASM does not support these
directives.) Then we can push that upstream to replace the error-prone and
non-standard custom handlers.

Change-Id: I5a734fd494b7eaafab24a00e6df624bd03b37d43
Reviewed-on: https://boringssl-review.googlesource.com/c/33785
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
2018-12-21 16:38:03 +00:00
David Benjamin
5edf8957b5 Translate .L directives inside .byte too.
Win64 unwind tables place distances from the start of a function in
byte-wide values.

Change-Id: Ie2aad7f6f5b702a60933bd52d872a83cba4e73a9
Reviewed-on: https://boringssl-review.googlesource.com/c/33784
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
2018-12-21 16:35:32 +00:00
David Benjamin
54efa1afc0 Add an ABI testing framework.
Dear reader, I must apologize in advance. This CL contains the following:

- A new 256-line perlasm file with non-trivial perl bits and a dual-ABI
  variadic function caller.

- C preprocessor gymnastics, with variadic macros and fun facts about
  __VA_ARGS__'s behavior on empty argument lists.

- C++ template gymnastics, including variadic arguments, template
  specialization, std::enable_if, and machinery to control template argument
  deduction.

Enjoy.

This tests that our assembly functions correctly honor platform ABI
conventions. Right now this only tests callee-saved registers, but it should be
extendable to SEH/CFI unwind testing with single-step debugging APIs.
Register-checking does not involve anything funny and should be compatible with
SDE. (The future unwind testing is unlikely to be compatible.)

This CL adds support for x86_64 SysV and Win64 ABIs. ARM, AArch64, and x86 can
be added in the future. The testing is injected in two places. First, all the
assembly tests in p256-x86_64-test.cc are now instrumented. This is the
intended workflow and should capture all registers.

However, we currently do not unit-test our assembly much directly. We should do
that as follow-up work[0] but, in the meantime, I've also wrapped all of the GTest
main function in an ABI test. This is imperfect as ABI failures may be masked
by other stack frames, but it costs nothing[1] and is pretty reliable at
catching Win64 xmm register failures.

[0] An alternate strategy would be, in debug builds, unconditionally instrument
every assembly call in libcrypto. But the CHECK_ABI macro would be difficult to
replicate in pure C, and unwind testing may be too invasive for this. Still,
something to consider when we C++ libcrypto.

[1] When single-stepped unwind testing exists, it won't cost nothing. The
gtest_main.cc call will turn unwind testing off.

Change-Id: I6643b26445891fd46abfacac52bc024024c8d7f6
Reviewed-on: https://boringssl-review.googlesource.com/c/33764
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
2018-12-21 16:09:32 +00:00
Alessandro Ghedini
2cc6f449d7 Use same HKDF label as TLS 1.3 for QUIC as per draft-ietf-quic-tls-17
Change-Id: Ie9825634f0f290aa3af0e88477013f62e2e0c246
Reviewed-on: https://boringssl-review.googlesource.com/c/33724
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-19 20:25:34 +00:00
Adam Langley
ba9ad6628c Add |SSL_key_update|.
This function allows a client to send a TLS 1.3 KeyUpdate message.

Change-Id: I69935253795a79d65a8c85b652378bf04b7058e2
Reviewed-on: https://boringssl-review.googlesource.com/c/33706
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-19 20:15:24 +00:00
Adam Langley
9700b44ff5 HRSS: omit reconstruction of ciphertext.
In [1], section 5.1, an optimised re-encryption process is given. In the
code, this simplifies to not needing to rebuild the ciphertext at all.

Thanks to John Schanck for pointing this out.

[1] https://eprint.iacr.org/2018/1174.pdf

Change-Id: I807bd509e936b7e82a43e8656444431546e9bbdf
Reviewed-on: https://boringssl-review.googlesource.com/c/33705
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-19 20:09:34 +00:00
Adam Langley
a6a049a6fb Add start of infrastructure for checking constant-time properties.
Valgrind's checking of uninitialised memory behaves very much like a
check for constant-time code: branches and memory indexes based on
uninitialised memory trigger warnings. Therefore, if we can tell
Valgrind that some secret is “uninitialised”, it'll give us a warning if
we do something non-constant-time with it.

This was the idea behind https://github.com/agl/ctgrind. But tricks like
that are no longer needed because Valgrind now comes with support for
marking regions of memory as defined or not. Therefore we can use that
API to check constant-time code.

This CL defines |CONSTTIME_SECRET| and |CONSTTIME_DECLASSIFY|, which are
no-ops unless the code is built with
|BORINGSSL_CONSTANT_TIME_VALIDATION| defined, which it isn't by default.
So this CL is a no-op itself so far. But it does show that a couple of
bits of constant-time time are, in fact, constant-time—seemingly even
when compiled with optimisations, which is nice.

The annotations in the RSA code are a) probably not marking all the
secrets as secret, and b) triggers warnings that are a little
interesting:

The anti-glitch check calls |BN_mod_exp_mont| which checks that the
input is less than the modulus. Of course, it is because the input is
the RSA plaintext that we just decrypted, but the plaintext is supposed
to be secret and so branching based on its contents isn't allows by
Valgrind. The answer isn't totally clear, but I've run out of time on
this for now.

Change-Id: I1608ed0b22d201e97595fafe46127159e02d5b1b
Reviewed-on: https://boringssl-review.googlesource.com/c/33504
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2018-12-18 22:43:02 +00:00
Adam Langley
c2897a158a Don't enable intrinsics on x86 without ABI support.
At some point after GCC 7.3, but before 8.2, GCC enabled the SSE ABI by
default. However, if it isn't enabled, the vector intrinsics in HRSS
cannot be used. (See https://github.com/grpc/grpc/issues/17540.)

Note that the intrinsics used are SSE2, but that should be ok because
they are guarded by a run-time check. The compile-time check for __SSE__
just ensures that GCC will build the code at all. (SDE does not simulate
anything that doesn't have SSE2, however.)

Change-Id: If092a06a441ed9d38576ea30351b3b40693a3399
Reviewed-on: https://boringssl-review.googlesource.com/c/33744
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2018-12-18 17:06:48 +00:00
Adam Langley
f8068ce885 HRSS: be strict about unused bits being zero.
It's excessively complex to worry about leaving these few bits for
extensions. If we need to change things, we can spin a new curve ID in
TLS. We don't need to support two versions during the transition because
a fallback to X25519 is still fine.

Change-Id: I0a4019d5693db0f0f3a5379909d99c2e2c762560
Reviewed-on: https://boringssl-review.googlesource.com/c/33704
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-17 21:02:58 +00:00
David Benjamin
41c10e2b5f Disable AES-GCM-SIV assembly on Windows.
I'm working on a test harness to check our assembly correctly restores
callee-saved registers. It caught this.

While perlasm tries to smooth over the differences between Windows and SysV
ABIs, it does not capture the difference in xmm registers. All xmm registers
are volatile in SysV, while Windows makes xmm6 through xmm15 callee-saved.

Change-Id: Ia549b0f126885768f7fb330271a590174c483a3d
Reviewed-on: https://boringssl-review.googlesource.com/c/33685
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2018-12-17 17:54:07 +00:00
David Benjamin
e1b2a65e7f Fix typo in AES-GCM-SIV comments.
Change-Id: I73bd495cf99bbc8a993a726b009d68e74c893420
Reviewed-on: https://boringssl-review.googlesource.com/c/33684
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-12-17 17:39:46 +00:00
Alessandro Ghedini
9b0970f1b0 Fix HRSS build error on ARM
Seeing the following errors with GCC 6 on ARM:

  crypto/hrss/hrss.c:212:12: error: function declaration isn't a prototype [-Werror=strict-prototypes]
   static int vec_capable() { return CRYPTO_is_NEON_capable(); }
              ^~~~~~~~~~~
  crypto/hrss/hrss.c: In function 'vec_capable':
  crypto/hrss/hrss.c:212:12: error: old-style function definition [-Werror=old-style-definition]

Change-Id: Ice540e6d436b8ada1dbc494f1feca10efff11687
Reviewed-on: https://boringssl-review.googlesource.com/c/33624
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2018-12-14 17:12:49 +00:00
David Benjamin
4cce955d14 Fix thread-safety bug in SSL_get_peer_cert_chain.
https://boringssl-review.googlesource.com/12704 pushed it just too far
to the edge. Once we have an established SSL_SESSION, any modifications
need to either be locked or done ahead of time. Do it ahead of time.
session->is_server gives a suitable place to check and X509s are
ref-counted so this should be cheap.

Add a regression test via TSan. Confirmed that TSan indeed catches this.

Change-Id: I30ce7b757d3a44465b318af3c98961ff3667483e
Reviewed-on: https://boringssl-review.googlesource.com/c/33606
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-12-13 19:30:49 +00:00
Adam Langley
200fe6786b Remove HRSS confirmation hash.
Since the underlying operation is deterministic the confirmation hash
isn't needed and SXY didn't use it in their proof.

Change-Id: I3a03c20ee79645cf94b10dbfe654c1b88d9aa416
Reviewed-on: https://boringssl-review.googlesource.com/c/33605
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2018-12-13 18:42:02 +00:00
Adam Langley
35a66d4aae Drop NEON assembly for HRSS.
Since we build Chrome with -mfpu=neon anyway, this isn't currently
needed. Additionally, I had included poly3_invert_vec in the assembly
but hadn't gotten around to wiring it up yet. That assembly referenced a
couple of functions in the C code that had been renamed. Surprisingly,
the NDK linker didn't have a problem with the undefined symbols since it
could statically find them to be unreachable.

But that isn't true everywhere. Some builds did fail because of the
undefined symbols although we're not sure what's different about them.
(Different NDK version perhaps?)

Change-Id: Ibac4724d24df05d6f6007499e1cd884e59889101
Reviewed-on: https://boringssl-review.googlesource.com/c/33604
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-13 17:43:07 +00:00
Adam Langley
d6e1f230b3 Add |SSL_export_traffic_secrets|.
This allows an application to obtain the current TLS 1.3 traffic secrets
for a connection.

Change-Id: I8ad8d0559caba266f74081441dea54b22da3db20
Reviewed-on: https://boringssl-review.googlesource.com/c/33590
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-12 22:57:33 +00:00
David Benjamin
3adb1e5a37 Patch out the XTS implementation in bsaes.
We don't call it, so ship less code and reduce the number of places
where we must think about the bsaes -> aes_nohw fallback.

Bug: 256
Change-Id: I10ac2d70e18ec81e679631a9532c36d9edab1c6e
Reviewed-on: https://boringssl-review.googlesource.com/c/33586
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2018-12-12 22:27:13 +00:00
Adam Langley
fc30467f28 Remove .file and .loc directives from HRSS ARM asm.
This is a workaround for https://bugs.llvm.org/show_bug.cgi?id=38740.

Change-Id: I74d5066c4c782745e003a608b3ccc002599bf6b4
Reviewed-on: https://boringssl-review.googlesource.com/c/33587
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-12 22:26:53 +00:00
David Benjamin
43cc9c6e86 Do not allow AES_128_GCM_SHA256 with CECPQ2.
Just forbid it altogether, so we don't need to worry about a mess of
equipreferences.

Change-Id: I4921ff326c6047e50c075d4311dd42219bf8318e
Reviewed-on: https://boringssl-review.googlesource.com/c/33585
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-12-12 20:05:52 +00:00
Adam Langley
1ea083d8b2 Always 16-byte align |poly| elements.
Even if the vector code isn't used in hrss.c, it might call external
assembly that still requires alignment.

Change-Id: I11ceb88f96deec6b20883872030ca090506ca150
Reviewed-on: https://boringssl-review.googlesource.com/c/33584
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2018-12-12 18:58:50 +00:00
Adam Langley
2526c66b72 Fix bug in HRSS tests.
I moved the |poly3_rand| code into a function and omitted to update a
|sizeof|.

Change-Id: I861fac4fe26ee3b5e5116d5cee71e64d9af9d175
Reviewed-on: https://boringssl-review.googlesource.com/c/33564
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2018-12-12 18:04:25 +00:00
Adam Langley
7b935937b1 Add initial HRSS support.
This change includes support for a variant of [HRSS], a post-quantum KEM
based on NTRU. It includes changes suggested in [SXY]. This is not yet
ready for any deployment: some breaking changes, like removing the
confirmation hash, are still planned.

(CLA for HRSS's assembly code noted in b/119426559.)

[HRSS] https://eprint.iacr.org/2017/667.pdf
[SXY] https://eprint.iacr.org/2017/1005.pdf

Change-Id: I85d813733b066d5c578484bdd248de3f764194db
Reviewed-on: https://boringssl-review.googlesource.com/c/33105
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-12 17:35:02 +00:00
David Benjamin
602f4669ab Forbid empty CertificateRequestsupported_signature_algorithms in TLS 1.2.
See the IETF thread here:
https://www.ietf.org/mail-archive/web/tls/current/msg27292.html

In particular, although the original publication of RFC 5246 had a
syntax error in the field (the minimum length was unspecified), there is
an errata from 2012 to fix it to be non-empty.
https://www.rfc-editor.org/errata/eid2864

Currently, when empty, we implicitly interpret it as SHA1/*, matching
the server behavior in missing extension in ClientHellos. However that
text does not support doing it for CertificateRequests, and there is not
much reason to. That default (which is in itself confusing and caused
problems such as older OpenSSL only signing SHA-1 given SNI) was
because, at the time, there were concerns over making any ClientHello
extensions mandatory. This isn't applicable for CertificateRequest,
which can freely advertise their true preferences.

Change-Id: I113494d8f66769fde1362795fb08ff2f471ef31d
Reviewed-on: https://boringssl-review.googlesource.com/c/33524
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-12-11 20:08:12 +00:00
Adam Langley
bf5021a6b8 Eliminate |OPENSSL_ia32cap_P| in C code in the FIPS module.
This can break delocate with certain compiler settings.

Change-Id: I76cf0f780d0e967390feed754e39b0ab25068f42
Reviewed-on: https://boringssl-review.googlesource.com/c/33485
Commit-Queue: Adam Langley <alangley@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-06 00:58:14 +00:00
David Benjamin
750fea158a Fix d2i_*_bio on partial reads.
If BIO_read returns partial reads, d2i_*_bio currently fails. This is a
partial (hah) regression from 419144adce.
The old a_d2i_fp.c code did *not* tolerate partial reads in the ASN.1
header, but it *did* tolerate them in the ASN.1 body. Since partial
reads are more likely to land in the body than the header, I think we
can say d2i_*_bio was "supposed to" tolerate this but had a bug in the
first few bytes.

Fix it for both cases. Add a regression test for this and the partial
write case (which works fine).

See also https://github.com/google/conscrypt/pull/587.

Change-Id: I886f6388f0b80621960e196cf2a56f5c02a14a04
Reviewed-on: https://boringssl-review.googlesource.com/c/33484
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-12-05 22:05:28 +00:00
Adam Langley
ff433815b5 Fix |BN_HEX_FMT2|.
It appears to be only used in p256-x86_64_test.cc, which is obviously
64-bit only and do not affected by this. Internal code search doesn't
find any uses and GitHub just finds several thousand copies of bn.h.

Change-Id: If8185bf6275d90efa172c95cb67c62c86a17e394
Reviewed-on: https://boringssl-review.googlesource.com/c/33464
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-04 20:35:05 +00:00
Brian Smith
90247be1d9 Remove XOP code from sha512-x86_64.pl.
Other XOP code was removed already.

Change-Id: I0c457effebd22f89e722653b93905a0b2e3eb5c0
Reviewed-on: https://boringssl-review.googlesource.com/c/33424
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-12-04 01:10:32 +00:00
Brian Smith
36ee9a5a0d Pretend AMD XOP was never a thing.
It's not clear that any AMD XOP code paths are being properly tested.
AMD dropped XOP starting in Zen.

Here's the one place I found (without looking too hard) where it seems
there is a XOP code path in BoringSSL, in sha512-x86_64.pl. Most of the
other XOP code was removed.

```
$code.=<<___ if ($avx && $SZ==8);
	test	\$`1<<11`,%r10d		# check for XOP
	jnz	.Lxop_shortcut
```

Change-Id: Id3301b2c84648790d010dae546b8e21ece1c528d
Reviewed-on: https://boringssl-review.googlesource.com/c/33405
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2018-12-03 22:59:55 +00:00
Adam Langley
e6ad7a027f Drop some explicit SSLKeyShare destructors.
We zero out memory in |OPENSSL_free| already.

Change-Id: I84a0f3cdfadd4544c0fade1d3d727baa6496ffe5
Reviewed-on: https://boringssl-review.googlesource.com/c/33446
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-12-03 22:51:05 +00:00
Brian Smith
96b05ed487 Assume hyper-threading-like vulnerabilities are always present.
It's not clear that CPUID will always report the correct value here,
especially for hyper-threading environments. It also isn't clear that
the assumptions made by AMD processors are correct and will always be
correct. It also seems likely that, if a code path is
security-sensitive w.r.t. SMT, it is probably also security-sensitive
w.r.t. other processor (mis)features. Finally, it isn't clear that all
dynamic analysis (fuzzing, SDE, etc.) is done separately for the cross
product of all CPU feature combinations * the value of this bit.

With all that in mind, instruct code sensitive to this bit to always
choose the more conservative path.

I only found one place that's sensitive to this bit, though I didn't
look too hard:

```
aes_nohw_cbc_encrypt:
    [...]
    leaq	OPENSSL_ia32cap_P(%rip),%r10
    mov	(%r10), %r10d
    [...]
    bt	\$28,%r10d
    jc	.Lcbc_slow_prologue
```

I didn't verify that the code in the HTT-enabled paths is any better
than the code in the HTT-disabled paths.

Change-Id: Ifd643e6a1301e5ca2174b84c344eb933d49e0067
Reviewed-on: https://boringssl-review.googlesource.com/c/33404
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2018-12-03 22:32:24 +00:00
David Benjamin
eb7d5b69e9 Replace the last CRITICAL_SECTION with SRWLOCK.
We don't support Windows XP, so we can rely on SRWLOCK. Per
https://crbug.com/592752, SRWLOCKs are more efficient and less of a
hassle to use. We'd previously converted CRYPTO_MUTEX to SRWLOCK, but I
missed this one. Not that this one lock matters much, may as well. It's
less initialization code.

Change-Id: I7ae435be5202b0a19f42015c9abff932dc04dbc7
Reviewed-on: https://boringssl-review.googlesource.com/c/33445
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-12-03 20:37:35 +00:00
David Benjamin
278b3120ee Validate ClientHellos in tests some more.
This way we'll notice if we ever generate a bad padding extension or
duplicate an extension. This did require fixing one of the JDK11 test
vectors. When I manually added a padding extension, I forgot the
contents were all zeros and incorrectly put in "padding" instead.

Change-Id: Ifec5bb01a739014ed0fdf5b49b82a6b514646e9a
Reviewed-on: https://boringssl-review.googlesource.com/c/33444
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-12-03 20:31:55 +00:00
Brian Smith
0f5ecd3a85 Re-enable AES-NI on 32-bit x86 too.
commit 05750f23ae disabled AES-NI for
32-bit x86, perhaps unintentionally.

Change-Id: Ie950c4f49526257138ecc803df5ecfc115bc648d
Reviewed-on: https://boringssl-review.googlesource.com/c/33365
Reviewed-by: Adam Langley <agl@google.com>
2018-11-28 00:32:30 +00:00
David Benjamin
e157dc9208 Make symbol-prefixing work on 32-bit x86.
On Linux, this introduces yet another symbol to blacklist.

Change-Id: Ieafe45a25f3b41da6c6934dd9488f4ee400bcab9
Reviewed-on: https://boringssl-review.googlesource.com/c/33350
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-27 22:35:17 +00:00
David Benjamin
8c23d3a5df Make Windows symbol-prefixing work.
This teaches read_symbols.go to use debug/pe, and fixes miscellaneous
issues with NASM. It also reveals a problem with this strategy of
getting symbols out at the linker level: inline functions.  I'm thinking
a better long-term mechanism may be to parse our header files.

Change-Id: I11b008543a7a97db3db9d4062ee4ddb910d174b7
Reviewed-on: https://boringssl-review.googlesource.com/c/33349
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-27 22:13:22 +00:00
David Benjamin
c8cf62bba8 Support Windows-style ar files.
Apparently Windows' .lib files are also ar. Add tests.

Change-Id: Ie35f410268086b8fe6d4d1b491de3f30a46309dd
Reviewed-on: https://boringssl-review.googlesource.com/c/33348
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-27 22:06:15 +00:00