Commit Graph

5739 Commits

Author SHA1 Message Date
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
David Benjamin
1a0f908e92 Move __.SYMDEF handling to ar.go.
One less bit of special-casing in read_symbols.go. We filter out the
sysv-style symbol table, so we should filter out the macOS one too.

Add tests for util/ar to cover this and the Linux case.

Change-Id: Id16d8b0526c1b6e0149df1df4006848d7b3a4b2f
Reviewed-on: https://boringssl-review.googlesource.com/c/33347
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-27 21:53:36 +00:00
David Benjamin
00d72d342f Fix stack_test.cc in the prefixed build.
Uses of BORINGSSL_MAKE_DELETER must be inside BSSL_NAMESPACE_BEGIN for
the specializations to work.

Change-Id: Ib96cf5d235586b24c052973d7034c0e5a8019f17
Reviewed-on: https://boringssl-review.googlesource.com/c/33346
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-27 21:35:56 +00:00
David Benjamin
8b0dad4a7f Don't double-mangle C++ symbols on macOS.
The symbol-listing code already removes the leading underscore.

Change-Id: I2f93382af932e8027f2aa8596886ba685836b3a6
Reviewed-on: https://boringssl-review.googlesource.com/c/33345
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-27 20:57:56 +00:00
David Benjamin
abbc59896f Make read_symbols.go a bit more idiomatic.
It's more verbose, but trimming the panics should make it easier to move
to a library (e.g. a symbol checker) or unit test later.

Change-Id: Iab37eff2689955e58057528be092d6dd5d8d26bc
Reviewed-on: https://boringssl-review.googlesource.com/c/33344
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2018-11-27 19:42:20 +00:00
David Benjamin
045ee41928 Unexport and rename hex_to_string, string_to_hex, and name_cmp.
Squatting these names is rather rude. Also hex_to_string and
string_to_hex do the opposite of what one would expect, so rename them
to something a bit less confusing.

Update-Note: This removes some random utility functions. name_cmp is
very specific to OpenSSL's config file format, so it's unlikely anyone
is relying on it. I removed the one use of hex_to_string and
string_to_hex I could find.

Change-Id: I01554885ad306251e6982100d0b15cd89b1cdea7
Reviewed-on: https://boringssl-review.googlesource.com/c/33364
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-27 00:08:39 +00:00
David Benjamin
9113e0996f Satisfy golint.
Errors are supposed to be fragments that go into sentences, rather than
sentences themselves.

Change-Id: I6569fce25535475162c85e7b0db7eeb62c93febd
Reviewed-on: https://boringssl-review.googlesource.com/c/33324
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-21 23:31:08 +00:00
David Benjamin
bbc429148f Add a note that generated files are generated.
Folks keep assuming checked-in assembly files are the source. Between
the preprocessor, delocate, NASM not using the C preprocessor, and GAS's
arch-specific comment syntax, comment markers are kind of a disaster.
This set appears to work for now.

Change-Id: I48e26dafb444dfa310df80dcce87ac291fde8037
Reviewed-on: https://boringssl-review.googlesource.com/c/33304
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-21 20:05:05 +00:00
David Benjamin
6965d25602 Work around a JDK 11 TLS 1.3 bug.
JDK 11 shipped with a TLS 1.3 implementation enabled by default.
Unfortunately, that implementation does not work and fails to send the
SNI extension on resumption. See
https://bugs.openjdk.java.net/browse/JDK-8211806.

This means servers which enable TLS 1.3 will see JDK 11 clients work on
the first connection and then fail on all subsequent connections. Add
SSL_set_jdk11_workaround which configures a workaround to fingerprint
JDK 11 and disable TLS 1.3 with the faulty clients.

JDK 11 also implemented the downgrade signal, which means that
connections that trigger the workaround also must not send the downgrade
signal. Unfortunately, the downgrade signal's security properties are
sensitive to the existence of any unmarked TLS 1.2 ServerHello paths. To
salvage this, pick a new random downgrade marker for this scenario and
modify the client to treat it as an alias of the standard one.

Per the link above, JDK 11.0.2 will fix this bug. Hopefully the
workaround can be retired sometime after it is released.

Change-Id: I0627609a8cadf7cc214073eb7f1e880acdf613ef
Reviewed-on: https://boringssl-review.googlesource.com/c/33284
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-21 18:22:57 +00:00
David Benjamin
4f746a9073 Move ARM cpuinfo functions to the header.
ClusterFuzz folks want to switch to a shared library build, so call into
these another way. The new setup isn't quite ideal because the real code
builds as C and now tests as C++, but it should work.

Bug: chromium:907115
Change-Id: Ia1ffc18832739b09fee21b84ee5d181e61feaa15
Reviewed-on: https://boringssl-review.googlesource.com/c/33285
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-21 00:46:57 +00:00
Adam Langley
a3ba8b3289 Regenerate obj_dat.h
clang-format seems to have decided to format things differently now.
This will eliminate diff noise in the future when there are actual
changes.

Change-Id: I1f94cf0f0859023b6c926119f39bf0a587464e52
Reviewed-on: https://boringssl-review.googlesource.com/c/33266
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-11-19 20:26:03 +00:00
Adam Langley
c65a1f4949 go fmt
Change-Id: I48a1e9e27013bb91b783949b65463208516bb3d2
Reviewed-on: https://boringssl-review.googlesource.com/c/33265
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2018-11-19 20:20:01 +00:00
David Benjamin
293d9ee4e8 Support execute-only memory for AArch64 assembly.
Put data in .rodata and, rather than adr, use the combination of adrp :pg_hi21:
and add :lo12:. Unfortunately, iOS uses different syntax, so we must add more
transforms to arm-xlate.pl.

Tested manually by:

1. Use Android NDK r19-beta1

2. Follow usual instructions to configure CMake for aarch64, but pass
   -DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld -Wl,-execute-only".

3. Build. Confirm with readelf -l tool/bssl that .text is not marked
   readable.

4. Push the test binaries onto a Pixel 3. Test normally and with
   --cpu={none,neon,crypto}. I had to pass --gtest_filter=-*Thread* to
   crypto_test. There appears to be an issue with some runtime function
   that's unrelated to our assembly.

No measurable performance difference.

Going forward, to support this, we will need to apply similar changes to
all other AArch64 assembly. This is relatively straightforward, but may
be a little finicky for dual-AArch32/AArch64 files (aesv8-armx.pl).

Update-Note: Assembly syntax is a mess. There's a decent chance some
assembler will get offend.

Change-Id: Ib59b921d4cce76584320fefd23e6bb7ebd4847eb
Reviewed-on: https://boringssl-review.googlesource.com/c/33245
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2018-11-19 19:58:15 +00:00
David Benjamin
4188c3f495 Remove cacheline striping in copy_from_prebuf.
The standard computation model for constant-time code is that memory
access patterns must be independent of secret data.
BN_mod_exp_mont_consttime was previously written to a slightly weaker
model: only cacheline access patterns must be independent of secret
data. It assumed accesses within a cacheline were indistinguishable.

The CacheBleed attack (https://eprint.iacr.org/2016/224.pdf) showed this
assumption was false. Cache lines may be divided into cache banks, and
the researchers were able to measure cache bank contention pre-Haswell.
For Haswell, the researchers note "But, as Haswell does show timing
variations that depend on low address bits [19], it may be vulnerable to
similar attacks."

OpenSSL's fix to CacheBleed was not to adopt the standard constant-time
computation model. Rather, it now assumes accesses within a 16-byte
cache bank are indistinguishable, at least in the C copy_from_prebuf
path. These weaker models failed before with CacheBleed, so avoiding
such assumptions seems prudent. (The [19] citation above notes a false
dependence between memory addresses with a distance of 4k, which may be
what the paper was referring to.) Moreover, the C path is largely unused
on x86_64 (which uses mont5 asm), so it is especially questionable for
the generic C code to make assumptions based on x86_64.

Just walk the entire table in the C implementation. Doing so as-is comes
with a performance hit, but the striped memory layout is, at that point,
useless. We regain the performance loss (and then some) by using a more
natural layout. Benchmarks below.

This CL does not touch the mont5 assembly; I haven't figured out what
it's doing yet.

Pixel 3, aarch64:
Before:
Did 3146 RSA 2048 signing operations in 10009070us (314.3 ops/sec)
Did 447 RSA 4096 signing operations in 10026666us (44.6 ops/sec)
After:
Did 3210 RSA 2048 signing operations in 10010712us (320.7 ops/sec)
Did 456 RSA 4096 signing operations in 10063543us (45.3 ops/sec)

Pixel 3, armv7:
Before:
Did 2688 RSA 2048 signing operations in 10002266us (268.7 ops/sec)
Did 459 RSA 4096 signing operations in 10004785us (45.9 ops/sec)
After:
Did 2709 RSA 2048 signing operations in 10001299us (270.9 ops/sec)
Did 459 RSA 4096 signing operations in 10063737us (45.6 ops/sec)

x86_64 Broadwell, mont5 assembly disabled:
(This configuration is not actually shipped anywhere, but seemed a
useful data point.)
Before:
Did 14274 RSA 2048 signing operations in 10009130us (1426.1 ops/sec)
Did 2448 RSA 4096 signing operations in 10046921us (243.7 ops/sec)
After:
Did 14706 RSA 2048 signing operations in 10037908us (1465.0 ops/sec)
Did 2538 RSA 4096 signing operations in 10059986us (252.3 ops/sec)

Change-Id: If41da911d4281433856a86c6c8eadf99cd33e2d8
Reviewed-on: https://boringssl-review.googlesource.com/c/33268
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2018-11-19 19:10:09 +00:00
David Benjamin
5963bff237 Tidy up type signature of BN_mod_exp_mont_consttime table.
It's a table of BN_ULONGs. No particular need to use unsigned char.

Change-Id: I397883cef9f39fb162c2b0bfbd6a70fe399757a2
Reviewed-on: https://boringssl-review.googlesource.com/c/33267
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-19 17:44:44 +00:00
Andrii Shyshkalov
09f5a040d4 No longer set CQ-Verified label on CQ success/failure.
R=davidben@google.com

Bug: chromium:906576
Change-Id: I56b16a76fabe37822a1a7eb3f075a476f83818ea
Reviewed-on: https://boringssl-review.googlesource.com/c/33270
Reviewed-by: David Benjamin <davidben@google.com>
2018-11-19 16:59:05 +00:00
David Benjamin
46e12b03f9 Print a message when simulating CPUs.
Make it more obvious something is happening.

Change-Id: Ie68d1e96a9bedd4b572c1cc99910348f89f07624
Reviewed-on: https://boringssl-review.googlesource.com/c/33244
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-11-16 23:12:27 +00:00
David Benjamin
ce61710062 Move JSON test results code into a common module.
We can actually use modules now.

Change-Id: I0bd8abaf4e3318069f93fa17e89b4804d03944eb
Reviewed-on: https://boringssl-review.googlesource.com/c/33205
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
2018-11-16 20:13:31 +00:00
Jesse Selover
f241a59dcc In 0RTT mode, reverify the server certificate before sending early data.
Bug: chromium:347402
Change-Id: I1442b595ed7296b9d9fe88357565f68e1ab80ffd
Reviewed-on: https://boringssl-review.googlesource.com/c/32644
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-11-16 19:52:18 +00:00
Junghoon Jang
e8ba1e3b21 Support assembly building for arm64e architecture.
iPhone XS/XS MAX/XR uses arm64e.

Change-Id: I89bd6b9307176c03fdc1a402ce6b8df080b00653
Reviewed-on: https://boringssl-review.googlesource.com/c/33224
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-11-16 01:43:53 +00:00