dh.c had a 10k-bit limit but it wasn't quite correctly enforced. However,
that's still 1.12s of jank on the IO thread, which is too long. Since the SSL
code consumes DHE groups from the network, it should be responsible for
enforcing what sanity it needs on them.
Costs of various bit lengths on 2013 Macbook Air:
1024 - 1.4ms
2048 - 14ms
3072 - 24ms
4096 - 55ms
5000 - 160ms
10000 - 1.12s
UMA says that DHE groups are 0.2% 4096-bit and otherwise are 5.5% 2048-bit and
94% 1024-bit and some noise. Set the limit to 4096-bit to be conservative,
although that's already quite a lot of jank.
BUG=554295
Change-Id: I8e167748a67e4e1adfb62d73dfff094abfa7d215
Reviewed-on: https://boringssl-review.googlesource.com/6464
Reviewed-by: Adam Langley <agl@google.com>
The current check has two problems:
- It only runs on the server, where there isn't a curve list at all. This was a
mistake in https://boringssl-review.googlesource.com/1843 which flipped it
from client-only to server-only.
- It only runs in TLS 1.2, so one could bypass it by just negotiating TLS 1.1.
Upstream added it as part of their Suite B mode, which requires 1.2.
Move it elsewhere. Though we do not check the entire chain, leaving that to the
certificate verifier, signatures made by the leaf certificate are made by the
SSL/TLS stack, so it's reasonable to check the curve as part of checking
suitability of a leaf.
Change-Id: I7c12f2a32ba946a20e9ba6c70eff23bebcb60bb2
Reviewed-on: https://boringssl-review.googlesource.com/6414
Reviewed-by: Adam Langley <agl@google.com>
It's easier to put libFuzzer.a into the source directory than to install
it globally.
Change-Id: I4dc7b56f81c7aa0371475c68d23368b025186505
Reviewed-on: https://boringssl-review.googlesource.com/6461
Reviewed-by: Adam Langley <agl@google.com>
The file armv8-mont.pl is taken from upstream. The speed ups are fairly
modest (~30%) but seem worthwhile.
Before:
Did 231 RSA 2048 signing operations in 1008671us (229.0 ops/sec)
Did 11208 RSA 2048 verify operations in 1036997us (10808.1 ops/sec)
Did 342 RSA 2048 (3 prime, e=3) signing operations in 1021545us (334.8 ops/sec)
Did 32000 RSA 2048 (3 prime, e=3) verify operations in 1016162us (31491.0 ops/sec)
Did 45 RSA 4096 signing operations in 1039805us (43.3 ops/sec)
Did 3608 RSA 4096 verify operations in 1060283us (3402.9 ops/sec)
After:
Did 300 RSA 2048 signing operations in 1009772us (297.1 ops/sec)
Did 12740 RSA 2048 verify operations in 1075413us (11846.6 ops/sec)
Did 408 RSA 2048 (3 prime, e=3) signing operations in 1016139us (401.5 ops/sec)
Did 33000 RSA 2048 (3 prime, e=3) verify operations in 1017510us (32432.1 ops/sec)
Did 52 RSA 4096 signing operations in 1067678us (48.7 ops/sec)
Did 3408 RSA 4096 verify operations in 1062863us (3206.4 ops/sec)
Change-Id: Ife74fac784067fce3668b5c87f51d481732ff855
Reviewed-on: https://boringssl-review.googlesource.com/6444
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 52e028b9de371da62c1e51b46592517b1068d770.)
Change-Id: If980d774671b9b5ba997db3fd7d4043525a85609
Reviewed-on: https://boringssl-review.googlesource.com/6445
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
PKCS#12 files may not necessarily include keys.
Change-Id: Ibb43b609783b02aa9cbb192fea377081169666ff
Reviewed-on: https://boringssl-review.googlesource.com/6456
Reviewed-by: Adam Langley <agl@google.com>
There seems to have been a merge error.
Change-Id: I72e5c2a45c148e31c90b28bedfff48f8ca6e3c8c
Reviewed-on: https://boringssl-review.googlesource.com/6455
Reviewed-by: Adam Langley <agl@google.com>
This exposes the ServerKeyExchange signature hash type used in the most recent
handshake, for histogramming on the client.
BUG=549662
Change-Id: I8a4e00ac735b1ecd2c2df824112c3a0bc62332a7
Reviewed-on: https://boringssl-review.googlesource.com/6413
Reviewed-by: Adam Langley <agl@google.com>
This is completely a no-op as currently tls12_get_psigalgs always returns a
hardcoded list which always includes SHA-1. But if this were to be made
configurable in the future, we should reject SHA-1 when configured to do so.
Change-Id: I7ab188eeff850d1e5f70b9522304812bab2d941a
Reviewed-on: https://boringssl-review.googlesource.com/6411
Reviewed-by: Adam Langley <agl@google.com>
When building in OPENSSL_NO_ASM mode, MSVC complains about unreachable
code. The redundant initialization of |i| is the main problem. The
skipping of the first test of the condition |i < num| with |goto| was
also confusing.
It turns out that |bn_mul_mont| is only called when assembly language
optimizations are available, but in that case the assmebly language
versions will always be used instead. Although this code will be
compiled in |OPENSSL_NO_ASM| builds, it is never called in
|OPENSSL_NO_ASM| builds. Thus, it can just be removed.
Change-Id: Id551899b2602824978edc1a1cb0703b76516808d
Reviewed-on: https://boringssl-review.googlesource.com/5550
Reviewed-by: Adam Langley <agl@google.com>
I've used these defines to easy the update of BoringSSL in Android
because Android's external/boringssl is a different git repository from
the rest of Android and thus it's not possible to land changes the
atomically update several things at once.
For this I tended just to add this define in the Android copy of
BoringSSL, but we're starting to see that bleed into other situations
now so it's looking like this will be generally useful.
These defines may be added when useful but shouldn't build up: once the
change has been done, the #if'ed code elsewhere that uses it should be
cleaned up. So far, that's worked ok. (I.e. we've had a BORINGSSL_201509
that correctly disappeared.)
Change-Id: I8cbb4731efe840cc798c970d37bc040b16a4a755
Reviewed-on: https://boringssl-review.googlesource.com/6442
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Not sure if we want to leave bio.h and bytestring.h's instance as-is, but the
evp.h ones are just baffling.
Change-Id: I485c2e355ba93764da0c4c72c48af48b055a8500
Reviewed-on: https://boringssl-review.googlesource.com/6454
Reviewed-by: Adam Langley <agl@google.com>
This fixes a number of bugs with the original logic:
- If handshake messages are fragmented and writes need to be retried, frag_off
gets completely confused.
- The BIO_flush call didn't set rwstate, so it wasn't resumable at that point.
- The msg_callback call gets garbage because the fragment header would get
scribbled over the handshake buffer.
The original logic was also extremely confusing with how it handles init_off.
(init_off gets rewound to make room for the fragment header. Depending on
where you pause, resuming may or may not have already been rewound.)
For simplicity, just allocate a new buffer to assemble the fragment in and
avoid clobbering the old one. I don't think it's worth the complexity to
optimize that. If we want to optimize this sort of thing, not clobbering seems
better anyway because the message may need to be retransmitted. We could avoid
doing a copy when buffering the outgoing message for retransmission later.
We do still need to track how far we are in sending the current message via
init_off, so I haven't opted to disconnect this function from
init_{buf,off,num} yet.
Test the fix to the retry + fragment case by having the splitHandshake option
to the state machine tests, in DTLS, also clamp the MTU to force handshake
fragmentation.
Change-Id: I66f634d6c752ea63649db8ed2f898f9cc2b13908
Reviewed-on: https://boringssl-review.googlesource.com/6421
Reviewed-by: Adam Langley <agl@google.com>
This was a mistake from when we added async CertificateVerify support.
No test because the final state of each write state is semi-unreachable
due to the buffer BIO that gets installed on each handshake.
Change-Id: I0180926522113c8b1ca58b8c9c6dc37fb0dd8083
Reviewed-on: https://boringssl-review.googlesource.com/6412
Reviewed-by: Adam Langley <agl@google.com>
The previous logic only defined
|SHA512_BLOCK_CAN_MANAGE_UNALIGNED_DATA| when the assembly language
optimizations were enabled, but
|SHA512_BLOCK_CAN_MANAGE_UNALIGNED_DATA| is also useful when the C
implementations are used.
If support for ARM processors that don't support unaligned access is
important, then it might be better to condition the enabling of
|SHA512_BLOCK_CAN_MANAGE_UNALIGNED_DATA| on ARM based on more specific
flags.
Change-Id: Ie8c37c73aba308c3ccf79371ce5831512e419989
Reviewed-on: https://boringssl-review.googlesource.com/6402
Reviewed-by: Adam Langley <agl@google.com>
Most functions can take this in as const. Note this changes an
RSA_METHOD hook, though one I would not expect anyone to override.
Change-Id: Ib70ae65e5876b01169bdc594e465e3e3c4319a8b
Reviewed-on: https://boringssl-review.googlesource.com/6419
Reviewed-by: Adam Langley <agl@google.com>
Later when TLS 1.3 comes around, we'll need SSL_CIPHER_get_max_version too. In
the meantime, hide the SSL_TLSV1_2 messiness behind a reasonable API.
Change-Id: Ibcc17cccf48dd99e364d6defdfa5a87d031ecf0a
Reviewed-on: https://boringssl-review.googlesource.com/6452
Reviewed-by: Adam Langley <agl@google.com>
This removes a sharp corner in the API where |ECDH_compute_key| assumed
that callers were either using ephemeral keys, or else had already
checked that the public key was on the curve.
A public key that's not on the curve can be in a small subgroup and thus
the result can leak information about the private key.
This change causes |EC_POINT_set_affine_coordinates_GFp| to require that
points are on the curve. |EC_POINT_oct2point| already does this.
Change-Id: I77d10ce117b6efd87ebb4a631be3a9630f5e6636
Reviewed-on: https://boringssl-review.googlesource.com/5861
Reviewed-by: Adam Langley <agl@google.com>
This change fixes up several comments (many of which were spotted by
Kenny Root) and also changes doc.go to detect cases where comments don't
start with the correct word. (This is a common error.)
Since we have docs builders now, these errors will be found
automatically in the future.
Change-Id: I58c6dd4266bf3bd4ec748763c8762b1a67ae5ab3
Reviewed-on: https://boringssl-review.googlesource.com/6440
Reviewed-by: Adam Langley <agl@google.com>
This function allows one to extract the current IVs from an SSL
connection. This is needed for the CBC cipher suites with implicit IVs
because, for those, the IV can't be extracted from the handshake key
material.
Change-Id: I247a1d0813b7a434b3cfc88db86d2fe8754344b6
Reviewed-on: https://boringssl-review.googlesource.com/6433
Reviewed-by: Adam Langley <agl@google.com>
They run through completely different logic as only handshake is fragmented.
This'll make it easier to rewrite the handshake logic in a follow-up.
Change-Id: I9515feafc06bf069b261073873966e72fcbe13cb
Reviewed-on: https://boringssl-review.googlesource.com/6420
Reviewed-by: Adam Langley <agl@google.com>
The documentation in md32_common.h is now (more) correct with respect
to the most important details of the layout of |HASH_CTX|. The
documentation explaining why sha512.c doesn't use md32_common.h is now
more accurate as well.
Before, the C implementations of HASH_BLOCK_DATA_ORDER took a pointer
to the |HASH_CTX| and the assembly language implementations took a
pointer to the hash state |h| member of |HASH_CTX|. (This worked
because |h| is always the first member of |HASH_CTX|.) Now, the C
implementations take a pointer directly to |h| too.
The definitions of |MD4_CTX|, |MD5_CTX|, and |SHA1_CTX| were changed to
be consistent with |SHA256_CTX| and |SHA512_CTX| in storing the hash
state in an array. This will break source compatibility with any
external code that accesses the hash state directly, but will not
affect binary compatibility.
The second parameter of |HASH_BLOCK_DATA_ORDER| is now of type
|const uint8_t *|; previously it was |void *| and all implementations
had a |uint8_t *data| variable to access it as an array of bytes.
This change paves the way for future refactorings such as automatically
generating the |*_Init| functions and/or sharing one I-U-F
implementation across all digest algorithms.
Change-Id: I6e9dd09ff057c67941021d324a4fa1d39f58b0db
Reviewed-on: https://boringssl-review.googlesource.com/6405
Reviewed-by: Adam Langley <agl@google.com>
Although those are only created by code owned by RSA_METHOD, custom RSA_METHODs
shouldn't be allowed to squat our internal fields and then change how you free
things.
Remove 'method' from their names now that they're not method-specific.
Change-Id: I9494ef9a7754ad59ac9fba7fd463b3336d826e0b
Reviewed-on: https://boringssl-review.googlesource.com/6423
Reviewed-by: Adam Langley <agl@google.com>
This restores the original semantics of the finished hook.
Change-Id: I70da393c7e66fb6e3be1e2511e08b34bb54fc0b4
Reviewed-on: https://boringssl-review.googlesource.com/6422
Reviewed-by: Adam Langley <agl@google.com>
Having a single RSA_METHOD means they all get pulled in. Notably, RSA key
generation pulls in the primality-checking code.
Change-Id: Iece480113754da090ddf87b64d8769f01e05d26c
Reviewed-on: https://boringssl-review.googlesource.com/6389
Reviewed-by: Adam Langley <agl@google.com>
This will allow a static linker (with -ffunction-sections since things aren't
split into files) to drop unused parts of DH and DSA. Notably, the parameter
generation bits pull in primality-checking code.
Change-Id: I25087e4cb91bc9d0ab43bcb267c2e2c164e56b59
Reviewed-on: https://boringssl-review.googlesource.com/6388
Reviewed-by: Adam Langley <agl@google.com>
Removing the function codes continued to sample __func__ for compatibility with
ERR_print_errors_cb, but not ERR_error_string_n. We can just emit
OPENSSL_internal for both. ERR_print_errors_cb already has the file and line
number available which is strictly more information than the function name.
(ERR_error_string_n does not, but we'd already turned that to
OPENSSL_internal.)
This shaves 100kb from a release build of the bssl tool.
In doing so, put an unused function code parameter back into ERR_put_error to
align with OpenSSL. We don't need to pass an additional string in anymore, so
OpenSSL compatibility with anything which uses ERR_LIB_USER or
ERR_get_next_error_library costs nothing. (Not that we need it.)
Change-Id: If6af34628319ade4145190b6f30a0d820e00b20d
Reviewed-on: https://boringssl-review.googlesource.com/6387
Reviewed-by: Adam Langley <agl@google.com>
MSVC unhelpfuly says: warning C4146: unary minus operator applied to
unsigned type, result still unsigned.
Change-Id: Ia1e6b9fc415908920abb1bcd98fc7f7a5670c2c7
This change incorporates Intel's P-256 implementation. The record of
Intel's submission under CLA is in internal bug number 25330687.
Before:
Did 3582 ECDH P-256 operations in 1049114us (3414.3 ops/sec)
Did 8525 ECDSA P-256 signing operations in 1028778us (8286.5 ops/sec)
Did 3487 ECDSA P-256 verify operations in 1008996us (3455.9 ops/sec)
build/tool/bssl is 1434704 bytes after strip -s
After:
Did 8618 ECDH P-256 operations in 1027884us (8384.2 ops/sec)
Did 21000 ECDSA P-256 signing operations in 1049490us (20009.7 ops/sec)
Did 8268 ECDSA P-256 verify operations in 1079481us (7659.2 ops/sec)
build/tool/bssl is 1567216 bytes after strip -s
Change-Id: I147971a8e19849779c8ed7e20310d41bd4962299
Reviewed-on: https://boringssl-review.googlesource.com/6371
Reviewed-by: Adam Langley <agl@google.com>
This option causes clients to ignore HelloRequest messages completely.
This can be suitable in cases where a server tries to perform concurrent
application data and handshake flow, e.g. because they are trying to
“renew” symmetric keys.
Change-Id: I2779f7eff30d82163f2c34a625ec91dc34fab548
Reviewed-on: https://boringssl-review.googlesource.com/6431
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The documentation in md32_common.h is now (more) correct with respect
to the most important details of the layout of |HASH_CTX|. The
documentation explaining why sha512.c doesn't use md32_common.h is now
more accurate as well.
Before, the C implementations of HASH_BLOCK_DATA_ORDER took a pointer
to the |HASH_CTX| and the assembly language implementations tool a
pointer to the hash state |h| member of |HASH_CTX|. (This worked
because |h| is always the first member of |HASH_CTX|.) Now, the C
implementations take a pointer directly to |h| too.
The definitions of |MD4_CTX|, |MD5_CTX|, and |SHA1_CTX| were changed to
be consistent with |SHA256_CTX| and |SHA512_CTX| in storing the hash
state in an array. This will break source compatibility with any
external code that accesses the hash state directly, but will not
affect binary compatibility.
The second parameter of |HASH_BLOCK_DATA_ORDER| is now of type
|const uint8_t *|; previously it was |void *| and all implementations
had a |uint8_t *data| variable to access it as an array of bytes.
This change paves the way for future refactorings such as automatically
generating the |*_Init| functions and/or sharing one I-U-F
implementation across all digest algorithms.
Change-Id: I30513bb40b5f1d2c8932551d54073c35484b3f8b
Reviewed-on: https://boringssl-review.googlesource.com/6401
Reviewed-by: Adam Langley <agl@google.com>
BN_mod_exp_mont_consttime does not modify its |BN_MONT_CTX| so that
value should be const.
Change-Id: Ie74e48eec8061899fd056fbd99dcca2a86b02cad
Reviewed-on: https://boringssl-review.googlesource.com/6403
Reviewed-by: Adam Langley <agl@google.com>
This simply converts a cipher suite string to the list of cipher suites
that it implies.
Change-Id: Id8b31086715d619ea6601c40a6eb84dc0d8c500d
Reviewed-on: https://boringssl-review.googlesource.com/6370
Reviewed-by: Adam Langley <agl@google.com>
That function doesn't do anything useful for DTLS. It's meant for tracking the
rest of the record we've already committed to by writing half of one. But one
cannot write half a datagram, so DTLS never tracks this. Just call
ssl_write_buffer_flush straight and don't touch wpend_*.
Change-Id: Ibe191907d64c955c7cfeefba26f5c11ad5e4b939
Reviewed-on: https://boringssl-review.googlesource.com/6418
Reviewed-by: Adam Langley <agl@google.com>
Although the DTLS transport layer logic drops failed writes on the floor, it is
actually set up to work correctly. If an SSL_write fails at the transport,
dropping the buffer is fine. Arguably it works better than in TLS because we
don't have the weird "half-committed to data" behavior. Likewise, the handshake
keeps track of how far its gotten and resumes the message at the right point.
This broke when the buffering logic was rewritten because I didn't understand
what the DTLS code was doing. The one thing that doesn't work as one might
expect is non-fatal write errors during rexmit are not recoverable. The next
timeout must fire before we try again.
This code is quite badly sprinkled in here, so add tests to guard it against
future turbulence. Because of the rexmit issues, the tests need some hacks
around calls which may trigger them. It also changes the Go DTLS implementation
from being completely strict about sequence numbers to only requiring they be
monotonic.
The tests also revealed another bug. This one seems to be upstream's fault, not
mine. The logic to reset the handshake hash on the second ClientHello (in the
HelloVerifyRequest case) was a little overenthusiastic and breaks if the
ClientHello took multiple tries to send.
Change-Id: I9b38b93fff7ae62faf8e36c4beaf848850b3f4b9
Reviewed-on: https://boringssl-review.googlesource.com/6417
Reviewed-by: Adam Langley <agl@google.com>
It's somewhat annoying to have to parse out the packetAdaptor mini-language.
Actually seeing those is only useful when debugging the adaptor itself, rather
than DTLS. Switch the order of the two middleware bits and add an escape hatch
to log the funny opcodes.
Change-Id: I249c45928a76b747d69f3ab972ea4d31e0680a62
Reviewed-on: https://boringssl-review.googlesource.com/6416
Reviewed-by: Adam Langley <agl@google.com>
89d4a68c introduced -Wmissing-field-initializers because it seemed
generally useful. However, Clang and GCC have differing opinions about
what counts as missing. This change should make Clang happy too.
Change-Id: I070c719f5c47f537207200d5399e093cc083e58f
