Commit Graph

4848 Commits

Author SHA1 Message Date
Andres Erbsen
46304abf7d ec/p256.c: fiat-crypto field arithmetic (64, 32)
The fiat-crypto-generated code uses the Montgomery form implementation
strategy, for both 32-bit and 64-bit code.

64-bit throughput seems slower, but the difference is smaller than noise between repetitions (-2%?)

32-bit throughput has decreased significantly for ECDH (-40%). I am
attributing this to the change from varibale-time scalar multiplication
to constant-time scalar multiplication. Due to the same bottleneck,
ECDSA verification still uses the old code (otherwise there would have
been a 60% throughput decrease). On the other hand, ECDSA signing
throughput has increased slightly (+10%), perhaps due to the use of a
precomputed table of multiples of the base point.

64-bit benchmarks (Google Cloud Haswell):

with this change:
Did 9126 ECDH P-256 operations in 1009572us (9039.5 ops/sec)
Did 23000 ECDSA P-256 signing operations in 1039832us (22119.0 ops/sec)
Did 8820 ECDSA P-256 verify operations in 1024242us (8611.2 ops/sec)

master (40e8c921ca):
Did 9340 ECDH P-256 operations in 1017975us (9175.1 ops/sec)
Did 23000 ECDSA P-256 signing operations in 1039820us (22119.2 ops/sec)
Did 8688 ECDSA P-256 verify operations in 1021108us (8508.4 ops/sec)

benchmarks on ARMv7 (LG Nexus 4):

with this change:
Did 150 ECDH P-256 operations in 1029726us (145.7 ops/sec)
Did 506 ECDSA P-256 signing operations in 1065192us (475.0 ops/sec)
Did 363 ECDSA P-256 verify operations in 1033298us (351.3 ops/sec)

master (2fce1beda0):
Did 245 ECDH P-256 operations in 1017518us (240.8 ops/sec)
Did 473 ECDSA P-256 signing operations in 1086281us (435.4 ops/sec)
Did 360 ECDSA P-256 verify operations in 1003846us (358.6 ops/sec)

64-bit tables converted as follows:

import re, sys, math

p = 2**256 - 2**224 + 2**192 + 2**96 - 1
R = 2**256

def convert(t):
    x0, s1, x1, s2, x2, s3, x3 = t.groups()
    v = int(x0, 0) + 2**64 * (int(x1, 0) + 2**64*(int(x2,0) + 2**64*(int(x3, 0)) ))
    w = v*R%p
    y0 = hex(w%(2**64))
    y1 = hex((w>>64)%(2**64))
    y2 = hex((w>>(2*64))%(2**64))
    y3 = hex((w>>(3*64))%(2**64))
    ww = int(y0, 0) + 2**64 * (int(y1, 0) + 2**64*(int(y2,0) + 2**64*(int(y3, 0)) ))
    if ww != v*R%p:
        print(x0,x1,x2,x3)
        print(hex(v))
        print(y0,y1,y2,y3)
        print(hex(w))
        print(hex(ww))
        assert 0
    return '{'+y0+s1+y1+s2+y2+s3+y3+'}'

fe_re = re.compile('{'+r'(\s*,\s*)'.join(r'(\d+|0x[abcdefABCDEF0123456789]+)' for i in range(4)) + '}')
print (re.sub(fe_re, convert, sys.stdin.read()).rstrip('\n'))

32-bit tables converted from 64-bit tables

Change-Id: I52d6e5504fcb6ca2e8b0ee13727f4500c80c1799
Reviewed-on: https://boringssl-review.googlesource.com/23244
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>
2017-12-11 17:55:46 +00:00
David Benjamin
21baf6421a Fix CustomExtensions-Server-EarlyDataAccepted test.
It's misnamed but, more importantly, doesn't do anything because the
test client isn't sending early data to begin with. We really need to
make these tests less error-prone to write. With this fix, the test
actually notices if we remove the server-side 0-RTT check.

Also remove MaxEarlyDataSize from the other server tests which
erroneously set it. Any test with sets that was likely copy-and-pasted
incorrectly.

Change-Id: Idc24bc1590e0316946022341185285418ab8c77b
Reviewed-on: https://boringssl-review.googlesource.com/23944
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-12-11 15:54:15 +00:00
David Benjamin
eb9232f06f Fully reduce scalars in EC_POINT_mul.
Along the way, this allows us to tidy up the invariants associated with
EC_SCALAR. They were fuzzy around ec_point_mul_scalar and some
computations starting from the digest in ECDSA. The latter I've put into
the type system with EC_LOOSE_SCALAR.

As for the former, Andres points out that particular EC implementations
are only good for scalars within a certain range, otherwise you may need
extra work to avoid the doubling case. To simplify curve
implementations, we reduce them fully rather than do the looser bit size
check, so they can have the stronger precondition to work with.

Change-Id: Iff9a0404f89adf8f7f914f8e8246c9f3136453f1
Reviewed-on: https://boringssl-review.googlesource.com/23664
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>
2017-12-08 17:55:54 +00:00
David Benjamin
2b63addf6a Use uint32_t for unicode code points.
The newer clang-cl is unhappy about the tautological comparison on
Windows, but the comparison itself is unnecessary anyway, since the
values will never exceed uint32_t.

I think the reason it's not firing elsewhere is because on other 64-bit
platforms, it is not tautological because long is 64-bit. On other
32-bit platforms, I'm not sure we actually have a standalone trunk clang
builder right now.

Update-Note: UTF8_getc and UTF8_putc were unexported. No one appears to
    be calling them. (We're a crypto library, not a Unicode library.)
Change-Id: I0949ddea3131dca5f55d04e672c3ccf2915c41ab
Reviewed-on: https://boringssl-review.googlesource.com/23844
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>
2017-12-08 17:51:34 +00:00
David Benjamin
296a61d600 bn/asm/rsaz-avx2.pl: fix digit correction bug in rsaz_1024_mul_avx2.
Credit to OSS-Fuzz for finding this.

CVE-2017-3738

(Imported from upstream's 5630661aecbea5fe3c4740f5fea744a1f07a6253 and
77d75993651b63e872244a3256e37967bb3c3e9e.)

Confirmed with Intel SDE that the fix makes the test vector pass and
that, without the fix, the test vector does not. (Well, we knew the
latter already, since it was our test vector.)

Change-Id: I167aa3407ddab3b434bacbd18e099c55aa40ac4c
Reviewed-on: https://boringssl-review.googlesource.com/23884
Reviewed-by: Adam Langley <agl@google.com>
2017-12-07 16:54:32 +00:00
David Benjamin
2bc937068d Add X509_NAME_get0_der from OpenSSL 1.1.0.
Change-Id: Iaa616a09f944ce720c11236b031d0fa9deb47db3
Reviewed-on: https://boringssl-review.googlesource.com/23864
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>
2017-12-06 17:49:04 +00:00
David Benjamin
d8dbde79f9 Don't allow negative EC_KEY private keys.
We check that the private key is less than the order, but we forgot the
other end.

Update-Note: It's possible some caller was relying on this, but since
    that function already checked the other half of the range, I'm
    expecting this to be a no-op change.

Change-Id: I4a53357d7737735b3cfbe97d379c8ca4eca5d5ac
Reviewed-on: https://boringssl-review.googlesource.com/23665
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>
2017-12-05 19:46:27 +00:00
David Benjamin
6979c7e8eb Disable Clang -Wtautological-constant-compare.
This is a recent Clang warning, but it's far too aggressive. The earlier
unsigned long silliness was worth fixing, but it otherwise complains on
32-bit platforms with:

  if (some_size_t > 0xffffffff) {
    ...
  }

which is unreasonable as, on 64-bit platforms, this check is meaningful
and requiring the programmer add ifdefs is error-prone. This matches
Chromium in https://crbug.com/767059.

Bug: chromium:767059
Change-Id: I0bb0f3a4b60f222e9d1b3c569471fbcf5518caed
Reviewed-on: https://boringssl-review.googlesource.com/23845
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>
2017-12-05 19:34:47 +00:00
David Benjamin
56d5d7085d Update tools.
The newer clang should hopefully fix the new linux_clang_cfi bot.

Note the VS2017 revision actually went backwards due to
db45606398

Change-Id: Icaca7a57596f063ccca490917d4b78813f2e9537
Reviewed-on: https://boringssl-review.googlesource.com/23824
Reviewed-by: Adam Langley <agl@google.com>
2017-12-04 21:39:54 +00:00
David Benjamin
494e4d0e89 Add an option for False Start without ALPN.
We can probably do this globally at this point since the cipher
requirements are much more restrict than they were in the beginning.
(Firefox, in particular, has done so far a while.) For now add a flag
since some consumer wanted this.

I'll see about connecting it to a Chrome field trial after our breakage
budget is no longer reserved for TLS 1.3.

Change-Id: Ib61dd5aae2dfd48b56e79873a7f3061a7631a5f8
Reviewed-on: https://boringssl-review.googlesource.com/23725
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>
2017-12-04 21:20:46 +00:00
David Benjamin
a5462d3050 Actually deprecate ERR_remove_thread_state.
Change-Id: I1f22e51bff8714550fbc73b116c08894c5b24d3d
Reviewed-on: https://boringssl-review.googlesource.com/23804
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-12-04 17:04:27 +00:00
Michał Janiszewski
d3ec6f1adb Add missing errno.h include to bio_test.cc
This fixes compilation on aarch64 and other architectures for Android.

Change-Id: I0b09ab06858c92d07e2376e244a4626a6af5037b
Reviewed-on: https://boringssl-review.googlesource.com/23764
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>
2017-12-04 01:32:37 +00:00
Adam Langley
bc37ad91fe Fix alignment-violating cast.
Change-Id: Id8b69bb6103dd938f4c6d0d2ec24f3d50ba5513c
Update-Note: fixes b/70034392
Reviewed-on: https://boringssl-review.googlesource.com/23744
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-12-01 22:32:17 +00:00
Steven Valdez
9986f6b045 Fix renegotiation with TLS 1.3 draft 22.
Change-Id: I87edf7e1fee07da4bc93cc7ab524b79991a4206e
Reviewed-on: https://boringssl-review.googlesource.com/23724
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>
2017-12-01 17:56:55 +00:00
David Benjamin
48eaa28a12 Make EC_POINT_mul work with arbitrary BIGNUMs again.
Rejecting values where we'd previous called BN_nnmod may have been
overly ambitious. In the long run, all the supported ECC APIs (ECDSA*,
ECDH_compute_key, and probably some additional new ECDH API) will be
using the EC_SCALAR version anyway, so this doesn't really matter.

Change-Id: I79cd4015f2d6daf213e4413caa2a497608976f93
Reviewed-on: https://boringssl-review.googlesource.com/23584
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>
2017-11-30 21:58:17 +00:00
David Benjamin
2fc4f362cd Revert "Support high tag numbers in CBS/CBB."
This reverts commit 66801feb17. This
turned out to break a lot more than expected. Hopefully we can reland it
soon, but we need to fix up some consumers first.

Note due to work that went in later, this is not a trivial revert and
should be re-reviewed.

Change-Id: I6474b67cce9a8aa03f722f37ad45914b76466bea
Reviewed-on: https://boringssl-review.googlesource.com/23644
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>
2017-11-30 21:57:17 +00:00
David Benjamin
095b6c9baa Also add a decoupled OBJ_obj2txt.
We need it in both directions. Also I missed that in OBJ_obj2txt we
allowed uint64_t components, but in my new OBJ_txt2obj we only allowed
uint32_t. For consistency, upgrade that to uint64_t.

Bug: chromium:706445
Change-Id: I38cfeea8ff64b9acf7998e552727c6c3b2cc600f
Reviewed-on: https://boringssl-review.googlesource.com/23544
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-30 18:21:48 +00:00
Steven Valdez
1530ef3ec5 Add early data input from file.
Change-Id: I93a54e7a67acddb196ed53ce7fe49c718553948d
Reviewed-on: https://boringssl-review.googlesource.com/23604
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-30 17:29:45 +00:00
David Benjamin
fb535892e5 runner: Rewrite some more parsers.
These were easy.

Change-Id: I5fc764b83d641b08b58ccbff36dbd28cb66efed0
Reviewed-on: https://boringssl-review.googlesource.com/23564
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-30 17:05:06 +00:00
Steven Valdez
c5c31abe2b Enforce compression_method in TLS 1.3 draft 22.
Change-Id: Ic99a949258e62cad168c2c39507ca63100a8ffe5
Reviewed-on: https://boringssl-review.googlesource.com/23264
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>
2017-11-29 22:19:04 +00:00
Steven Valdez
e6cefe41bb Update PR 1091 CL to use draft22 version.
Change-Id: Ifa811262fbca22222656da530f97daac3dcd6a5b
Reviewed-on: https://boringssl-review.googlesource.com/22944
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
2017-11-29 16:11:24 +00:00
David Benjamin
fc9c67599d Bound the input to the bn_mod_exp fuzzer.
This is not a speedy operation, so the fuzzers need a bit of help to
avoid timeouts.

Bug: chromium:786049
Change-Id: Ib56281b63eb6c895057f21254f0cc7c5c2d85ee4
Reviewed-on: https://boringssl-review.googlesource.com/23484
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-28 21:48:00 +00:00
David Benjamin
a7673facf8 runner: Parse CertificateRequest with byteReader.
Bug: 212
Change-Id: I0ad4df330360789b16fc9db70565abdb3ae42a8f
Reviewed-on: https://boringssl-review.googlesource.com/23448
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
2017-11-28 18:37:39 +00:00
David Benjamin
28b267b357 runner: Parse Certificate with byteReader.
Bug: 212
Change-Id: Ife51516ef0642730e601e146028b16ded99ab7ba
Reviewed-on: https://boringssl-review.googlesource.com/23447
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-28 17:42:49 +00:00
David Benjamin
bd911af514 runner: Parse SH/HRR/EE with byteReader.
Bug: 212
Change-Id: I454db0bfd59bac3729338c6f8d9e51efde0735eb
Reviewed-on: https://boringssl-review.googlesource.com/23446
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-28 17:03:39 +00:00
David Benjamin
7ce2378750 runner: Send the right alert for handshake message parsing failures.
This throws me off every time.

Change-Id: I19848927fe821f7656dea0343361d70dae4007c9
Reviewed-on: https://boringssl-review.googlesource.com/23445
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-28 16:55:49 +00:00
David Benjamin
47b8f00fdc Reimplement OBJ_txt2obj and add a lower-level function.
OBJ_txt2obj is currently implemented using BIGNUMs which is absurd. It
also depends on the giant OID table, which is undesirable. Write a new
one and expose the low-level function so Chromium can use it without the
OID table.

Bug: chromium:706445
Change-Id: I61ff750a914194f8776cb8d81ba5d3eb5eaa3c3d
Reviewed-on: https://boringssl-review.googlesource.com/23364
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
2017-11-27 21:29:00 +00:00
David Benjamin
be8c8b4b1d runner: Add a byteReader type and convert ClientHello parsing.
Bug: 212
Change-Id: Iecbd8fddef1b55a438947ad60780e08cb4260c48
Reviewed-on: https://boringssl-review.googlesource.com/23444
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-27 21:18:40 +00:00
Steven Valdez
8c9ceadc58 Add switch to enable draft 22.
Change-Id: I60dc085fa02c152adb12a505b453fe8f84670d8b
Reviewed-on: https://boringssl-review.googlesource.com/23464
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>
2017-11-27 20:51:30 +00:00
David Benjamin
56aaf164ac Pretty-print large INTEGERs and ENUMERATEDs in hex.
This avoids taking quadratic time to pretty-print certificates with
excessively large integer fields. Very large integers aren't any more
readable in decimal than hexadecimal anyway, and the i2s_* functions
will parse either form.

Found by libFuzzer.

Change-Id: Id586cd1b0eef8936d38ff50433ae7c819f0054f3
Reviewed-on: https://boringssl-review.googlesource.com/23424
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>
2017-11-27 18:38:50 +00:00
David Benjamin
27bc0f26c8 Fix CBS tag class docs.
Change-Id: Ia7b3b5d9ce833a9cdfb94c8e0923f3cf17555fdd
Reviewed-on: https://boringssl-review.googlesource.com/23449
Reviewed-by: Adam Langley <agl@google.com>
2017-11-27 17:47:47 +00:00
Daniel Wagner-Hall
2fce1beda0 Remove spurious ;
DECLARE_STACK_OF adds a trailing ; so we don't need a second one added
here.

Compiling a project using boringssl which uses -Werror,-Wextra-semi I
get errors:

```
third_party/boringssl/include/openssl/stack.h:374:1: error: extra ';' outside of a function [-Werror,-Wextra-semi]
DEFINE_STACK_OF(void)
^
third_party/boringssl/include/openssl/stack.h:355:3: note: expanded from macro 'DEFINE_STACK_OF'
  BORINGSSL_DEFINE_STACK_OF_IMPL(type, type *, const type *) \
  ^
third_party/boringssl/include/openssl/stack.h:248:25: note: expanded from macro 'BORINGSSL_DEFINE_STACK_OF_IMPL'
  DECLARE_STACK_OF(name);                                                      \
                        ^
third_party/boringssl/include/openssl/stack.h:375:1: error: extra ';' outside of a function [-Werror,-Wextra-semi]
DEFINE_SPECIAL_STACK_OF(OPENSSL_STRING)
^
third_party/boringssl/include/openssl/stack.h:369:3: note: expanded from macro 'DEFINE_SPECIAL_STACK_OF'
  BORINGSSL_DEFINE_STACK_OF_IMPL(type, type, const type)
  ^
third_party/boringssl/include/openssl/stack.h:248:25: note: expanded from macro 'BORINGSSL_DEFINE_STACK_OF_IMPL'
  DECLARE_STACK_OF(name);                                                      \
                        ^
2 errors generated.
```

Change-Id: Icc39e2341eb76544be72d2d7d0bd29e2f1ed0bf9
Reviewed-on: https://boringssl-review.googlesource.com/23404
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>
2017-11-24 13:29:07 +00:00
David Benjamin
e3b2a5d30d Const-correct X509_ALGOR_get0.
Matches the OpenSSL 1.1.0 spelling, which is what we advertise in
OPENSSL_VERSION_NUMBER now. Otherwise third-party code which uses it
will, in the long term, need ifdefs. Note this will require updates to
any existing callers (there appear to only be a couple of them), but it
should be straightforward.

Change-Id: I9dd1013609abca547152728a293529055dacc239
Reviewed-on: https://boringssl-review.googlesource.com/23325
Reviewed-by: Adam Langley <agl@google.com>
2017-11-22 22:52:38 +00:00
David Benjamin
61e9245543 Use some of the word-based functions for ECDSA verification.
This is only a hair faster than the signing change, but still something.
I kept the call to BN_mod_inverse_odd as that appears to be faster
(constant time is not a concern for verification).

Before:
Did 22855 ECDSA P-224 verify operations in 3015099us (7580.2 ops/sec)
Did 21276 ECDSA P-256 verify operations in 3083284us (6900.4 ops/sec)
Did 2635 ECDSA P-384 verify operations in 3032582us (868.9 ops/sec)
Did 1240 ECDSA P-521 verify operations in 3068631us (404.1 ops/sec)

After:
Did 23310 ECDSA P-224 verify operations in 3056226us (7627.1 ops/sec)
Did 21210 ECDSA P-256 verify operations in 3035765us (6986.7 ops/sec)
Did 2666 ECDSA P-384 verify operations in 3023592us (881.7 ops/sec)
Did 1209 ECDSA P-521 verify operations in 3054040us (395.9 ops/sec)

Change-Id: Iec995b1a959dbc83049d0f05bdc525c14a95c28e
Reviewed-on: https://boringssl-review.googlesource.com/23077
Reviewed-by: Adam Langley <agl@google.com>
2017-11-22 22:52:04 +00:00
David Benjamin
86c2b854b0 Don't use BN_nnmod to convert from field element to scalar.
Hasse's theorem implies at most one subtraction is necessary. This is
still using BIGNUM for now because field elements
(EC_POINT_get_affine_coordinates_GFp) are BIGNUMs.

This gives an additional 2% speedup for signing.

Before:
Did 16000 ECDSA P-224 signing operations in 1064799us (15026.3 ops/sec)
Did 19000 ECDSA P-256 signing operations in 1007839us (18852.2 ops/sec)
Did 1078 ECDSA P-384 signing operations in 1079413us (998.7 ops/sec)
Did 484 ECDSA P-521 signing operations in 1083616us (446.7 ops/sec)

After:
Did 16000 ECDSA P-224 signing operations in 1054918us (15167.1 ops/sec)
Did 20000 ECDSA P-256 signing operations in 1037338us (19280.1 ops/sec)
Did 1045 ECDSA P-384 signing operations in 1049073us (996.1 ops/sec)
Did 484 ECDSA P-521 signing operations in 1085492us (445.9 ops/sec)

Change-Id: I2bfe214f968eca7a8e317928c0f3daf1a14bca90
Reviewed-on: https://boringssl-review.googlesource.com/23076
Reviewed-by: Adam Langley <agl@google.com>
2017-11-22 22:51:53 +00:00
David Benjamin
a838f9dc7e Make ECDSA signing 10% faster and plug some timing leaks.
None of the asymmetric crypto we inherented from OpenSSL is
constant-time because of BIGNUM. BIGNUM chops leading zeros off the
front of everything, so we end up leaking information about the first
word, in theory. BIGNUM functions additionally tend to take the full
range of inputs and then call into BN_nnmod at various points.

All our secret values should be acted on in constant-time, but k in
ECDSA is a particularly sensitive value. So, ecdsa_sign_setup, in an
attempt to mitigate the BIGNUM leaks, would add a couple copies of the
order.

This does not work at all. k is used to compute two values: k^-1 and kG.
The first operation when computing k^-1 is to call BN_nnmod if k is out
of range. The entry point to our tuned constant-time curve
implementations is to call BN_nnmod if the scalar has too many bits,
which this causes. The result is both corrections are immediately undone
but cause us to do more variable-time work in the meantime.

Replace all these computations around k with the word-based functions
added in the various preceding CLs. In doing so, replace the BN_mod_mul
calls (which internally call BN_nnmod) with Montgomery reduction. We can
avoid taking k^-1 out of Montgomery form, which combines nicely with
Brian Smith's trick in 3426d10119. Along
the way, we avoid some unnecessary mallocs.

BIGNUM still affects the private key itself, as well as the EC_POINTs.
But this should hopefully be much better now. Also it's 10% faster:

Before:
Did 15000 ECDSA P-224 signing operations in 1069117us (14030.3 ops/sec)
Did 18000 ECDSA P-256 signing operations in 1053908us (17079.3 ops/sec)
Did 1078 ECDSA P-384 signing operations in 1087853us (990.9 ops/sec)
Did 473 ECDSA P-521 signing operations in 1069835us (442.1 ops/sec)

After:
Did 16000 ECDSA P-224 signing operations in 1064799us (15026.3 ops/sec)
Did 19000 ECDSA P-256 signing operations in 1007839us (18852.2 ops/sec)
Did 1078 ECDSA P-384 signing operations in 1079413us (998.7 ops/sec)
Did 484 ECDSA P-521 signing operations in 1083616us (446.7 ops/sec)

Change-Id: I2a25e90fc99dac13c0616d0ea45e125a4bd8cca1
Reviewed-on: https://boringssl-review.googlesource.com/23075
Reviewed-by: Adam Langley <agl@google.com>
2017-11-22 22:51:40 +00:00
David Benjamin
66801feb17 Support high tag numbers in CBS/CBB.
Android's attestion format uses some ludicrously large tag numbers:
https://developer.android.com/training/articles/security-key-attestation.html#certificate_schema

Add support for these in CBS/CBB. The public API does not change for
callers who were using the CBS_ASN1_* constants, but it is no longer the
case that tag representations match their DER encodings for small tag
numbers.

Chromium needs https://chromium-review.googlesource.com/#/c/chromium/src/+/783254,
but otherwise I don't expect this to break things.

Bug: 214
Change-Id: I9b5dc27ae3ea020e9edaabec4d665fd73da7d31e
Reviewed-on: https://boringssl-review.googlesource.com/23304
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-22 22:34:05 +00:00
David Benjamin
02514002fd Use dec/jnz instead of loop in bn_add_words and bn_sub_words.
Imported from upstream's a78324d95bd4568ce2c3b34bfa1d6f14cddf92ef. I
think the "regression" part of that change is some tweak to BN_usub and
I guess the bn_*_words was to compensate for it, but we may as well
import it. Apparently the loop instruction is terrible.

Before:
Did 39871000 bn_add_words operations in 1000002us (39870920.3 ops/sec)
Did 38621750 bn_sub_words operations in 1000001us (38621711.4 ops/sec)

After:
Did 64012000 bn_add_words operations in 1000007us (64011551.9 ops/sec)
Did 81792250 bn_sub_words operations in 1000002us (81792086.4 ops/sec)

loop sets no flags (even doing the comparison to zero without ZF) while
dec sets all flags but CF, so Andres and I are assuming that because
this prevents Intel from microcoding it to dec/jnz, they otherwise can't
be bothered to add more circuitry since every compiler has internalized
by now to never use loop.

Change-Id: I3927cd1c7b707841bbe9963e3d4afd7ba9bd9b36
Reviewed-on: https://boringssl-review.googlesource.com/23344
Reviewed-by: Adam Langley <agl@google.com>
2017-11-22 21:56:05 +00:00
David Benjamin
2056d7290a Remove DSA_sign_setup too.
Change-Id: Ib406e7d1653fa57a863dbd5d4eb04401caf5de0a
Reviewed-on: https://boringssl-review.googlesource.com/23284
Reviewed-by: Adam Langley <agl@google.com>
2017-11-22 21:01:11 +00:00
David Benjamin
42a8cbe37c Remove ECDSA_sign_setup and friends.
These allow precomputation of k, but bypass our nonce hardening and also
make it harder to excise BIGNUM. As a bonus, ECDSATest.SignTestVectors
is now actually covering the k^-1 and r computations.

Change-Id: I4c71dae162874a88a182387ac43999be9559ddd7
Reviewed-on: https://boringssl-review.googlesource.com/23074
Reviewed-by: Adam Langley <agl@google.com>
2017-11-22 20:23:40 +00:00
David Benjamin
8dc226ca8f Add some missing OpenSSL 1.1.0 accessors.
wpa_supplicant appear to be using these.

Change-Id: I1f220cae69162901bcd9452e8daf67379c5e276c
Reviewed-on: https://boringssl-review.googlesource.com/23324
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-22 18:43:38 +00:00
David Benjamin
855d5046c7 Unwind legacy SSL_PRIVATE_KEY_METHOD hooks.
After much procrastinating, we finally moved Chromium to the new stuff.
We can now delete this. This is a breaking change for
SSL_PRIVATE_KEY_METHOD consumers, but it should be trivial (remove some
unused fields in the struct). I've bumped BORINGSSL_API_VERSION to ease
any multi-sided changes that may be needed.

Change-Id: I9fe562590ad938bcb4fcf9af0fadeff1d48745fb
Reviewed-on: https://boringssl-review.googlesource.com/23224
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
2017-11-21 17:48:09 +00:00
David Benjamin
67623735e0 Fix memory leak on sk_X509_EXTENSION_push failure.
(Imported from upstream's c29f83c05f3a3c5641c5ddf054789a29d2163bf3.)

ext was being leaked. Upstream also did some stuff around *x which
wasn't strictly necessary (usually OpenSSL only provides basic
exception safety, not strong exception safety), but ah well.

Change-Id: I52d230990b05501b4cee6deee8dcacba4a926c18
Reviewed-on: https://boringssl-review.googlesource.com/23204
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-21 17:48:00 +00:00
David Benjamin
c367ee5439 Add a CFI build flag.
This uses Clang's CFI feature.

Bug: 201
Change-Id: I7a42ec73dc8bfb3893ec69f2d2f4d7e3a2fd2cc4
Reviewed-on: https://boringssl-review.googlesource.com/23225
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
2017-11-21 17:40:40 +00:00
Adam Langley
8c565fa86c Include a couple of missing header files.
mem.h for |OPENSSL_cleanse| and bn/internal.h for things like
|bn_less_than_words| and |bn_correct_top|.

Change-Id: I3c447a565dd9e4f18fb2ff5d59f80564b4df8cea
Reviewed-on: https://boringssl-review.googlesource.com/23164
Reviewed-by: Adam Langley <agl@google.com>
2017-11-20 20:36:38 +00:00
David Benjamin
8793942c5c Fix fuzzer mode suppressions.
Change-Id: I82f92019dccfaf927f7180a5af53c9ffae111861
Reviewed-on: https://boringssl-review.googlesource.com/23145
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>
2017-11-20 18:44:18 +00:00
David Benjamin
6d218d6d7a Remove unused function.
Change-Id: Id12ab478b6ba441fb1b6f4c2f9479384fc3fbdb6
Reviewed-on: https://boringssl-review.googlesource.com/23144
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-11-20 18:32:44 +00:00
David Benjamin
0a5f006736 Test that EC_POINT_mul works with the order.
|EC_POINT_mul| is almost exclusively used with reduced scalars, with
this exception. This comes from consumers following NIST SP 800-56A
section 5.6.2.3.2. (Though all our curves have cofactor one, so this
check isn't useful.)

Add a test for this so we don't accidentally break it.

Change-Id: I42492db38a1ea03acec4febdd7945c8a3933530a
Reviewed-on: https://boringssl-review.googlesource.com/23084
Reviewed-by: Adam Langley <agl@google.com>
2017-11-20 18:32:30 +00:00
David Benjamin
e7c95d91f8 Run TLS 1.3 tests at all variants and fix bugs.
We were only running a random subset of TLS 1.3 tests with variants and
let a lot of bugs through as a result.

- HelloRetryRequest-EmptyCookie wasn't actually testing what we were
  trying to test.

- The second HelloRetryRequest detection needs tweaks in draft-22.

- The empty HelloRetryRequest logic can't be based on non-empty
  extensions in draft-22.

- We weren't sending ChangeCipherSpec correctly in HRR or testing it
  right.

- Rework how runner reads ChangeCipherSpec by setting a flag which
  affects the next readRecord. This cuts down a lot of cases and works
  correctly if the client didn't send early data. (In that case, we
  don't flush CCS until EndOfEarlyData and runner deadlocks waiting for
  the ChangeCipherSpec to arrive.)

Change-Id: I559c96ea3a8b350067e391941231713c6edb2f78
Reviewed-on: https://boringssl-review.googlesource.com/23125
Reviewed-by: Steven Valdez <svaldez@chromium.org>
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>
2017-11-20 18:19:18 +00:00
David Benjamin
3bba5ccf35 Add EndOfEarlyData to per-message tests.
Change-Id: I9da9734625d1d9d2c783830d8b4aecd34f51acc6
Reviewed-on: https://boringssl-review.googlesource.com/23124
Reviewed-by: Steven Valdez <svaldez@chromium.org>
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>
2017-11-20 18:10:38 +00:00