We may wish to pass data to the runner that contains NULs.
Change-Id: Id78dad0ad0b5b6d0537481c818e3febdf1740cc9
Reviewed-on: https://boringssl-review.googlesource.com/1603
Reviewed-by: Adam Langley <agl@google.com>
The return values are now 1/0, not 1/0/-1.
Change-Id: If65bb08a229c7944cb439ec779df461904d0ec19
Reviewed-on: https://boringssl-review.googlesource.com/1607
Reviewed-by: Adam Langley <agl@google.com>
09bd58d1f1 flipped a condition. Doing that
memset in the DTLS case breaks retransmits across a CCS and fails to memset in
the TLS case.
Strangely, it didn't break any tests, but I think that's a function of us
lacking renego tests. The sequence number doesn't seem to be used in the
initial handshake for TLS, so it stayed at zero. After a renego, that codepath
is relevant.
Change-Id: I369a524021857a82e181af7798c7a10fe6279550
Reviewed-on: https://boringssl-review.googlesource.com/1601
Reviewed-by: Adam Langley <agl@google.com>
Otherwise, in C, it becomes a K&R function declaration which doesn't actually
type-check the number of arguments.
Change-Id: I0731a9fefca46fb1c266bfb1c33d464cf451a22e
Reviewed-on: https://boringssl-review.googlesource.com/1582
Reviewed-by: Adam Langley <agl@google.com>
It doesn't appear to have ever been implemented on the client. The server code
stopped working anyway because it now skips the ssl_get_message call, so we
never cash in on the reuse_message, attempt to reprocess the repeated
ClientHello, and reject it thinking it's a second MS SGC restart.
Change-Id: Id536846e08460143f6fc0a550bdcc1b26b506b04
Reviewed-on: https://boringssl-review.googlesource.com/1580
Reviewed-by: Adam Langley <agl@google.com>
Remove all the logic managing key types that aren't being used anymore.
Change-Id: I101369164588048e64ba1c84a6b8aac8f3a221cd
Reviewed-on: https://boringssl-review.googlesource.com/1567
Reviewed-by: Adam Langley <agl@google.com>
DSA is not connected up to EVP, so it wouldn't work anyway. We shouldn't
advertise a cipher suite we don't support. Chrome UMA data says virtually no
handshakes end up negotiating one of these.
Change-Id: I874d934432da6318f05782ebd149432c1d1e5275
Reviewed-on: https://boringssl-review.googlesource.com/1566
Reviewed-by: Adam Langley <agl@google.com>
These are the variants where the CA signs a Diffie-Hellman keypair. They are
not supported by Chrome on NSS.
Change-Id: I569a7ac58454bd3ed1cd5292d1f98499012cdf01
Reviewed-on: https://boringssl-review.googlesource.com/1564
Reviewed-by: Adam Langley <agl@google.com>
In the fixed_ecdh case, it wasn't even implemented, but there was stub code for
it. It complicates the ClientKeyExchange (the client parameters become implicit
in the certificate) and isn't used.
Change-Id: I3627a37042539c90e05e59cd0cb3cd6c56225561
Reviewed-on: https://boringssl-review.googlesource.com/1563
Reviewed-by: Adam Langley <agl@google.com>
This also removes the 'LOW' strength class.
Change-Id: Iffd2356dadb4a4875c1547a613d51061101358fd
Reviewed-on: https://boringssl-review.googlesource.com/1562
Reviewed-by: Adam Langley <agl@google.com>
NULL, SRP, CAMELLIA, export ciphers, SSLv2, IDEA, and SEED are gone. Unknown
directives are silently ignored in the parser, so there is no need to retain
their masks and entries in the cipher suite aliases.
Change-Id: If43b9cbce56b3e1c401db764b88996940452a300
Reviewed-on: https://boringssl-review.googlesource.com/1561
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ia0daaaaf464cfa0e9d563d7f376ce2bb2e338685
Reviewed-on: https://boringssl-review.googlesource.com/1560
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
To align with what Chrome sends on NSS, remove all 3DES cipher suites except
RSA_WITH_3DES_EDE_CBC_SHA. This avoids having to order a PFS 3DES cipher
against a non-PFS 3DES cipher.
Remove the strength sort which wanted place AES_256_CBC ahead of AES_128_GCM
and is not especially useful (everything under 128 is either 3DES or DES).
Instead, explicitly order all the bulk ciphers. Continue to prefer PFS over
non-PFS and ECDHE over DHE.
This gives the following order in Chromium. We can probably prune it a bit
(DHE_DSS, DH_*) in a follow-up.
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 (0xcc14) Forward Secrecy 256
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (0xcc13) Forward Secrecy 256
TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (0xcc15) Forward Secrecy 256
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f) Forward Secrecy 128
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 (0xc02b) Forward Secrecy 128
TLS_DHE_DSS_WITH_AES_128_GCM_SHA256 (0xa2) Forward Secrecy* 128
TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 (0x9e) Forward Secrecy 128
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014) Forward Secrecy 256
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA (0xc00a) Forward Secrecy 256
TLS_DHE_RSA_WITH_AES_256_CBC_SHA (0x39) Forward Secrecy 256
TLS_DHE_DSS_WITH_AES_256_CBC_SHA (0x38) Forward Secrecy* 256
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013) Forward Secrecy 128
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA (0xc009) Forward Secrecy 128
TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33) Forward Secrecy 128
TLS_DHE_DSS_WITH_AES_128_CBC_SHA (0x32) Forward Secrecy* 128
TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011) Forward Secrecy 128
TLS_ECDHE_ECDSA_WITH_RC4_128_SHA (0xc007) Forward Secrecy 128
TLS_DH_DSS_WITH_AES_128_GCM_SHA256 (0xa4) 128
TLS_DH_RSA_WITH_AES_128_GCM_SHA256 (0xa0) 128
TLS_RSA_WITH_AES_128_GCM_SHA256 (0x9c) 128
TLS_DH_RSA_WITH_AES_256_CBC_SHA (0x37) 256
TLS_DH_DSS_WITH_AES_256_CBC_SHA (0x36) 256
TLS_RSA_WITH_AES_256_CBC_SHA (0x35) 256
TLS_DH_RSA_WITH_AES_128_CBC_SHA (0x31) 128
TLS_DH_DSS_WITH_AES_128_CBC_SHA (0x30) 128
TLS_RSA_WITH_AES_128_CBC_SHA (0x2f) 128
TLS_RSA_WITH_RC4_128_SHA (0x5) 128
TLS_RSA_WITH_RC4_128_MD5 (0x4) 128
TLS_RSA_WITH_3DES_EDE_CBC_SHA (0xa) 112
BUG=405091
Change-Id: Ib8dd28469414a4eb496788a57a215e7e21f8c37f
Reviewed-on: https://boringssl-review.googlesource.com/1559
Reviewed-by: Adam Langley <agl@google.com>
Just use the normal API for them.
Change-Id: Ibb5988611a86e8d39abda1e02087523d98defb51
Reviewed-on: https://boringssl-review.googlesource.com/1555
Reviewed-by: Adam Langley <agl@google.com>
RFC 6347 changed the meaning of server_version in HelloVerifyRequest. It should
now always be 1.0 with version negotiation not happening until ServerHello. Fix
runner.go logic and remove #if-0'd code in dtls1_get_hello_verify.
Enforce this in the runner for when we get DTLS 1.2 tests.
Change-Id: Ice83628798a231df6bf268f66b4c47b14a519386
Reviewed-on: https://boringssl-review.googlesource.com/1552
Reviewed-by: Adam Langley <agl@google.com>
Rather than switching the order of the ServerHello and HelloVerifyRequest
states and processing each twice, have the states follow the protocol order.
HelloVerifyRequest reading is optional and ServerHello is strict. Use the
send_cookie bit to determine whether we're expecting a cookie or not.
Fix the dtls1_stop_timer call in these states to consistently hit the end of a
server flight; the previous flight should not be cleared from the retransmit
buffer until the entire next flight is received. That said, OpenSSL doesn't
appear to implement the part where, on receipt of the previous peer flight, the
buffered flight is retransmitted. (With the exception of a SSL3_MT_FINISHED
special-case in dtls1_read_bytes.) So if the peer is also OpenSSL, this doesn't
do anything.
Also fix the DTLS test which wasn't actually asserting that the ClientHello
matched.
Change-Id: Ia542190972dbffabb837d32c9d453a243caa90b2
Reviewed-on: https://boringssl-review.googlesource.com/1551
Reviewed-by: Adam Langley <agl@google.com>
I see no internal users and the existence of a THIRD version encoding
complicates all version-checking logic. Also convert another version check to
SSL_IS_DTLS that was missed earlier.
Change-Id: I60d215f57d44880f6e6877889307dc39dbf838f7
Reviewed-on: https://boringssl-review.googlesource.com/1550
Reviewed-by: Adam Langley <agl@google.com>
This lets us put the SSL_CIPHER table in the data section. For type-checking,
make STACK_OF(SSL_CIPHER) cast everything to const SSL_CIPHER*.
Note that this will require some changes in consumers which weren't using a
const SSL_CIPHER *.
Change-Id: Iff734ac0e36f9e5c4a0f3c8411c7f727b820469c
Reviewed-on: https://boringssl-review.googlesource.com/1541
Reviewed-by: Adam Langley <agl@google.com>
Of the remaining implementations left, ssl3_, dtls1_, and ssl23_, dtls1_ is
redundant and can be folded into ssl3_. ssl23_ actually isn't; it sets 5
minutes rather than 2 hours. Two hours seems to be what everything else uses
and seems a saner default. Most consumers seem to override it anyway
(SSL_CTX_set_timeout). But it is a behavior change.
The method is called at two points:
- SSL_get_default_timeout
- SSL_CTX_new
Incidentally, the latter call actually makes the former never called internally
and the value it returns a lie. SSL_get_default_timeout returns the default
timeout of the /current/ method, but in ssl_get_new_session, the timeout is
shadowed by session_timeout on the context. That is initialized when
SSL_CTX_new is called. So, unless you go out of your way to
SSL_CTX_set_timeout(0), it always overrides. (And it actually used to a
difference because, for SSL23, the SSL_CTX's method is SSL23, but, when session
creation happens, the SSL's method is the version-specific one.)
Change-Id: I331d3fd69b726242b36492402717b6d0b521c6ee
Reviewed-on: https://boringssl-review.googlesource.com/1521
Reviewed-by: Adam Langley <agl@google.com>
Those ciphers go through EVP_AEAD now.
Change-Id: Ia97af9960223724f041dc2c249def9e626fd03f8
Reviewed-on: https://boringssl-review.googlesource.com/1520
Reviewed-by: Adam Langley <agl@google.com>
Also remove SSL_eNULL ciphers. They were broken anyway in the initial import
because of a lost 'else', but just remove them altogether.
Change-Id: Ie71cf1b45f8fc6883e209801443eddf7f2d058ba
Reviewed-on: https://boringssl-review.googlesource.com/1518
Reviewed-by: Adam Langley <agl@google.com>
It was added in OpenSSL 1.0.2, so nothing can be depending on it yet. If we
really want a Suite B profile, it seems better to generate a configuration for
the rest of the system rather than pepper the codebase with checks.
Change-Id: I1be3ebed0e87cbfe236ade4174dcf5bbc7e10dd5
Reviewed-on: https://boringssl-review.googlesource.com/1517
Reviewed-by: Adam Langley <agl@google.com>
The missing SSL 3.0 client support in runner.go was fairly minor.
Change-Id: Ibbd440c9b6be99be08a214dec6b93ca358d8cf0a
Reviewed-on: https://boringssl-review.googlesource.com/1516
Reviewed-by: Adam Langley <agl@google.com>
This was done for the server when parsing a session ticket, but it
wasn't done in the parsing function itself. That caused problems when
high level code used the parsing function directly to set a session for
the client code.
See comments in internal bug 7091840.
Change-Id: Iaa048c3df62cd9fe7a003af33805819e2556960a
The protocols are pretty similar; they were all basically redundant. The free
of s->tlsext_session_ticket (more fallout from the EAP-FAST patch) was moved to
SSL_free because that object's attached to s, not s->s3. This is relevant if
SSL_set_ssl_method gets called.
Change-Id: I14a896ba8a6a2c34ab1cb5f65311b117051228da
Reviewed-on: https://boringssl-review.googlesource.com/1509
Reviewed-by: Adam Langley <agl@google.com>
This removes some duplicate code in parsing command-line flags and, more
importantly, makes configuration available when constructing the SSL_CTX and
avoids a number of globals.
Change-Id: I26e2d2285b732f855a2c82752bc8e0db480c3b30
Reviewed-on: https://boringssl-review.googlesource.com/1502
Reviewed-by: Adam Langley <agl@google.com>
Run against openssl s_client and openssl s_server. This seems to work for a
start, although it may need to become cleverer to stress more of BoringSSL's
implementation for test purposes.
In particular, it assumes a reliable, in-order channel. And it requires that
the peer send handshake fragments in order. Retransmit and whatnot are not
implemented. The peer under test will be expected to handle a lossy channel,
but all loss in the channel will be controlled. MAC errors, etc., are fatal.
Change-Id: I329233cfb0994938fd012667ddf7c6a791ac7164
Reviewed-on: https://boringssl-review.googlesource.com/1390
Reviewed-by: Adam Langley <agl@google.com>
Gives bounds checks and asserts that there's nothing after the cookie.
Change-Id: I8f9753e0c72670e9960f73a5722cefd9c02696a9
Reviewed-on: https://boringssl-review.googlesource.com/1507
Reviewed-by: Adam Langley <agl@google.com>
Mirror the changes in s3_clnt.c.
Change-Id: I7af7080c6eea2a67cc994befa11e45d32eaa9615
Reviewed-on: https://boringssl-review.googlesource.com/1506
Reviewed-by: Adam Langley <agl@google.com>
Analogous fix for DTLS as upstream's c519e89f5c359b8c0f747519773284d9b6382791.
Change-Id: I8a56070ce2a1edf4e9ceb2fd8ce08552e25a1cf3
Reviewed-on: https://boringssl-review.googlesource.com/1504
Reviewed-by: Adam Langley <agl@google.com>
They weren't updated to account for DTLS 1.2.
Change-Id: I81b3bfcb84a46d7b233bb567976a7de37bc46b92
Reviewed-on: https://boringssl-review.googlesource.com/1503
Reviewed-by: Adam Langley <agl@google.com>
https://crbug.com/353579
Align behavior with NSS and report SSL_R_BAD_DH_P_LENGTH error
when size of the server's dh group is less than 512 bits.
Change-Id: I09f1828482f40b2283f7c6a69425819379399815
Reviewed-on: https://boringssl-review.googlesource.com/1480
Reviewed-by: Adam Langley <agl@google.com>
Even if OPENSSL_free() now simply is defined to free(), it is
still nice to consistently use OPENSSL_free, so that they can
easily be replaced. Many embedded platforms still have slow
allocation and free functions.
Change-Id: Ie8781591311f12c7f69206dbad6fc4a0c89d88b7
Reviewed-on: https://boringssl-review.googlesource.com/1490
Reviewed-by: Adam Langley <agl@google.com>
This is needed by Android because it passes this string to a handshake
callback. It's implemented in Android's OpenSSL in this patch:
https://android.googlesource.com/platform/external/openssl.git/+/master/patches/0003-jsse.patch
(Note that it's called |SSL_authentication_method| there.)
I didn't format this function in OpenSSL style because it's crazy and
because we'll probably clang-format ssl/ soon.
Change-Id: I865540511b50859c339da5d76ce37810449aa444
Android needs this and it was patched into their OpenSSL in
https://android.googlesource.com/platform/external/openssl.git/+/master/patches/0003-jsse.patch
It appears that this is needed because javax.net.ssl.SSLEngine has it as
part of its interface and thus it's part of the Android API. No idea why
anything would ever want to disable that though.
Change-Id: I9c6279a961637f44936889edbe269b9d5c19746d
ssl23_get_client_hello has lots of remnants of SSLv2 support and remnants of an
even older SSL_OP_NON_EXPORT_FIRST option (see upstream's
d92f0bb6e9ed94ac0c3aa0c939f2565f2ed95935) which complicates the logic.
Split it into three states and move V2ClientHello parsing into its own
function. Port it to CBS and CBB to give bounds checks on the V2ClientHello
parse.
This fixes a minor bug where, if the SSL_accept call in ssl23_get_client_hello
failed, cb would not be NULL'd and SSL_CB_ACCEPT_LOOP would get reported an
extra time.
It also unbreaks the invariant between s->packet, s->packet_length,
s->s3->rbuf.buf, and s->s3->rbuf.offset at the point the switch, although this
was of no consequence because the first ssl3_read_n call passes extend = 0
which resets s->packet and s->packet_length.
It also makes us tolerant to major version bumps in the ClientHello. Add tests
for TLS tolerance of both minor and major version bumps as well as the HTTP
request error codes.
Change-Id: I948337f4dc483f4ebe1742d3eba53b045b260257
Reviewed-on: https://boringssl-review.googlesource.com/1455
Reviewed-by: Adam Langley <agl@google.com>
Windows doesn't have ssize_t, sadly. There's SSIZE_T, but defining an
OPENSSL_SSIZE_T seems worse than just using an int.
Change-Id: I09bb5aa03f96da78b619e551f92ed52ce24d9f3f
Reviewed-on: https://boringssl-review.googlesource.com/1352
Reviewed-by: Adam Langley <agl@google.com>
ssl3_send_client_key_exchange were wrongly reported
by ssl3_send_client_certificate() and
ssl3_check_cert_and_algorithm()
Change-Id: I244d3d871b6b4f75a188fd386d52ffc4335d1f9b
Reviewed-on: https://boringssl-review.googlesource.com/1460
Reviewed-by: Adam Langley <agl@google.com>
It's a different handshake flow with more state machine coverage. We should
make sure to test the asynchronous version.
Change-Id: I0bb79ca7e6a86bd3cac66bac1f795a885d474909
Reviewed-on: https://boringssl-review.googlesource.com/1454
Reviewed-by: Adam Langley <agl@google.com>
Didn't have coverage for abbreviated handshakes with NewSessionTicket. Also add
some missing resumeSession flags so the tests match the comments.
Change-Id: Ie4d76e8764561f3f1f31e1aa9595324affce0db8
Reviewed-on: https://boringssl-review.googlesource.com/1453
Reviewed-by: Adam Langley <agl@google.com>
Also change MaxHandshakeRecordLength to 1 in the handshake coverage tests to
better stress the state machine.
Change-Id: I27fce2c000b3d4818fd2e9a47fb09d3f646dd1bd
Reviewed-on: https://boringssl-review.googlesource.com/1452
Reviewed-by: Adam Langley <agl@google.com>
Test all pairs of client and server version, except for the ones that require
SSLv3 client support in runner.go. That is, as yet, still missing.
Change-Id: I601ab49c5526cd2eb4f85d5d535570e32f218d5b
Reviewed-on: https://boringssl-review.googlesource.com/1450
Reviewed-by: Adam Langley <agl@google.com>
It's not part of SSL_OP_ALL and is unused, so remove it. Add a test that
asserts the version check works.
Change-Id: I917516594ec5a4998a8316782f035697c33d99b0
Reviewed-on: https://boringssl-review.googlesource.com/1418
Reviewed-by: Adam Langley <agl@google.com>
(This change originally applied to 1.0.1. In the switch to 1.0.2, the
DTLS specific client processing was removed and now the s3_clnt.c
functions are used. This caused most of the patch to be moot. What
remains is still useful however. For the original patch, see the change
against 1.0.1: 88ae012c8092852f03c50f6461175271104b4c8a)
CVE-2014-3510
Reviewed-by: Dr. Stephen Henson <steve@openssl.org>
(Imported from upstream's 1d7d0ed9c21403d79d602b6c7d76fdecf5e737da)
Change-Id: I666f9c48d603f2366cab821ae446a57360c3026b
Reviewed-on: https://boringssl-review.googlesource.com/1439
Reviewed-by: Adam Langley <agl@google.com>
CVE-2014-3509
(Imported from upstream's 92aa73bcbfad44f9dd7997ae51537ac5d7dc201e)
Change-Id: Ibc681897251081ae5ebfea0ff6ca9defd73fe0f5
Reviewed-on: https://boringssl-review.googlesource.com/1441
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
In a couple of functions, a sequence number would be calculated twice.
Additionally, in |dtls1_process_out_of_seq_message|, we know that
|frag_len| <= |msg_hdr->msg_len| so the later tests for |frag_len <
msg_hdr->msg_len| can be more clearly written as |frag_len !=
msg_hdr->msg_len|, since that's the only remaining case.
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Emilia Käsper <emilia@openssl.org>
(Imported from upstream's d345a24569edf0a966b3d6eaae525f0ca4c5e570)
Change-Id: I038f9f01a1d9379f1ee058b231d80e8b9ce6c2d7
Reviewed-on: https://boringssl-review.googlesource.com/1438
Reviewed-by: Adam Langley <agl@google.com>
Applying same fix as in dtls1_process_out_of_seq_message. A truncated
DTLS fragment would cause *ok to be clear, but the return value would
still be the number of bytes read.
Problem identified by Emilia Käsper, based on previous issue/patch by Adam
Langley.
Reviewed-by: Emilia Käsper <emilia@openssl.org>
(Imported from upstream's 3d5dceac430d7b9b273331931d4d2303f5a2256f)
Change-Id: Ibe30716266e2ee1489c98b922cf53edda096c23c
Reviewed-on: https://boringssl-review.googlesource.com/1437
Reviewed-by: Adam Langley <agl@google.com>
Previously, a truncated DTLS fragment in
|dtls1_process_out_of_seq_message| would cause *ok to be cleared, but
the return value would still be the number of bytes read. This would
cause |dtls1_get_message| not to consider it an error and it would
continue processing as normal until the calling function noticed that
*ok was zero.
I can't see an exploit here because |dtls1_get_message| uses
|s->init_num| as the length, which will always be zero from what I can
see.
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Emilia Käsper <emilia@openssl.org>
(Imported from upstream's aad61c0a57a3b6371496034db61675abcdb81811.)
Change-Id: I2fb0ea93b6e812e19723ada3351f842cc7b2fa91
Reviewed-on: https://boringssl-review.googlesource.com/1436
Reviewed-by: Adam Langley <agl@google.com>
The |pqueue_insert| function can fail if one attempts to insert a
duplicate sequence number. When handling a fragment of an out of
sequence message, |dtls1_process_out_of_seq_message| would not call
|dtls1_reassemble_fragment| if the fragment's length was zero. It would
then allocate a fresh fragment and attempt to insert it, but ignore the
return value, leaking the fragment.
This allows an attacker to exhaust the memory of a DTLS peer.
Fixes CVE-2014-3507
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Emilia Käsper <emilia@openssl.org>
(Imported from upstream's 8ca4c4b25e050b881f3aad7017052842b888722d.)
Change-Id: I387e3f6467a0041f6367965ed3c1ad4377b9ac08
Reviewed-on: https://boringssl-review.googlesource.com/1435
Reviewed-by: Adam Langley <agl@google.com>
In |dtls1_reassemble_fragment|, the value of
|msg_hdr->frag_off+frag_len| was being checked against the maximum
handshake message size, but then |msg_len| bytes were allocated for the
fragment buffer. This means that so long as the fragment was within the
allowed size, the pending handshake message could consume 16MB + 2MB
(for the reassembly bitmap). Approx 10 outstanding handshake messages
are allowed, meaning that an attacker could consume ~180MB per DTLS
connection.
In the non-fragmented path (in |dtls1_process_out_of_seq_message|), no
check was applied.
Fixes CVE-2014-3506
Wholly based on patch by Adam Langley with one minor amendment.
Reviewed-by: Emilia Käsper <emilia@openssl.org>
(Imported from upstream's 0598468fc04fb0cf2438c4ee635b587aac1bcce6)
Change-Id: I4849498eabb45ec973fcb988d639b23145891e25
Reviewed-on: https://boringssl-review.googlesource.com/1434
Reviewed-by: Adam Langley <agl@google.com>
The |item| variable, in both of these cases, may contain a pointer to a
|pitem| structure within |s->d1->buffered_messages|. It was being freed
in the error case while still being in |buffered_messages|. When the
error later caused the |SSL*| to be destroyed, the item would be double
freed.
Thanks to Wah-Teh Chang for spotting that the fix in 1632ef74 was
inconsistent with the other error paths (but correct).
Fixes CVE-2014-3505
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Emilia Käsper <emilia@openssl.org>
(Imported from upstream's 49850075555893c9c60d5b981deb697f3b9515ea)
Change-Id: Ie40007184f6194ba032b4213c18d36254e80aaa6
Reviewed-on: https://boringssl-review.googlesource.com/1432
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
When the write size was exactly SSL3_RT_MAX_PLAIN_LENGTH+1 and record
splitting is needed, an extra byte would be added to the max size of the
message to be written. This would cause the requested size to not exceed
the max. If the SSL_WANT_WRITE error were returned, the next packet
would not get the extra byte added to the max packet size since
record_split_done is set. Since a different set of arguments
(SSL3_RT_MAX_PLAIN_LENGTH+1 vs SSL3_RT_MAX_PLAIN_LENGTH) would be passed
to do_ssl3_write, it would return an "SSL3_WRITE_PENDING:bad write
retry" error.
To avoid a failure in the opposite direction, the max variable increment
is removed as well. This can happen when SSL_MODE_ENABLE_PARTIAL_WRITE
is not enabled and the call to ssl3_write_bytes contains, e.g., a buffer
of 2*SSL3_RT_MAX_PLAIN_LENGTH, where the first call into do_ssl3_write
succeeds writing the first SSL3_RT_MAX_PLAIN_LENGTH bytes, but writing
the second SSL3_RT_MAX_PLAIN_LENGTH bytes fails. This means the first
time the the second section of SSL3_RT_MAX_PLAIN_LENGTH bytes has called
do_ssl3_write with "max" bytes, but next call to ssl3_write_bytes in
turn calls into do_ssl3_write with "max+1" bytes.
Change-Id: Icf8453195c1145a54d31b8e8146801118207df03
Reviewed-on: https://boringssl-review.googlesource.com/1420
Reviewed-by: Kenny Root <kroot@google.com>
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Avoid needing to manually increment the reference count and using the right
lock, both here and in Chromium.
Change-Id: If116ebc224cfb1c4711f7e2c06f1fd2c97af21dd
Reviewed-on: https://boringssl-review.googlesource.com/1415
Reviewed-by: Adam Langley <agl@google.com>
Reference counting should be internal to the type, otherwise callers need to
know which lock to use.
Change-Id: If4d805876a321ef6dece115c805e605584ff311e
Reviewed-on: https://boringssl-review.googlesource.com/1414
Reviewed-by: Adam Langley <agl@google.com>
Add a framework for testing the asynchronous codepath. Move some handshake
state machine coverage tests to cover a range of record-layer and
handshake-layer asynchronicity.
This adds tests for the previous two async bugs fixed.
Change-Id: I422ef33ba3eeb0ad04766871ed8bc59b677b169e
Reviewed-on: https://boringssl-review.googlesource.com/1410
Reviewed-by: Adam Langley <agl@google.com>
Any ssl3_get_* function that takes ownership of something before the
ssl_get_message call can't early-return without cleanup work.
This fixes valgrind on ClientAuth-Server-Async.
Change-Id: Ie7f0b37cac4d4bb7e06c00bae091fee0386c22da
Reviewed-on: https://boringssl-review.googlesource.com/1413
Reviewed-by: Adam Langley <agl@google.com>
- DTLS server code didn't account for the new ClientHello state. This looks
like it only matters if a DTLS server uses select_certificate_cb and returns
asynchronously.
- State A transitions immediately to B and is redundant. No code distinguishes
A and B.
- The ssl_get_message call transitions to the second state (originally C). This
makes the explicit transition to C a no-op. More of a problem,
ssl_get_message may return asynchronously and remain in its second state if the
handshake body had not completed yet. Fix this by splitting state C in two.
Combined with the above change, this results in only the top few states getting
reshuffled.
This fixes the server async tests.
Change-Id: I46703bcd205988b118217b6424ba4f88e731be5a
Reviewed-on: https://boringssl-review.googlesource.com/1412
Reviewed-by: Adam Langley <agl@google.com>
With the removal of the freelist itself, these macros are
superfluous. Remove them in favore of OPENSSL_malloc and OPENSSL_free.
Change-Id: I4bfeff8ea087b9e16c7c32d7c1bdb7a07e7dd03e
Reviewed-on: https://boringssl-review.googlesource.com/1389
Reviewed-by: Adam Langley <agl@google.com>
The memory freelist maintained by OpenSSL claims to be a performance
optimization for platforms that have a slow malloc/free
implementation. This should not be the case on modern
linux/glibc. Remove the freelist as it poses a potential security
hazard of buffer-reuse that is of "initialized" memory that will not
be caught be tools such as valgrind.
Change-Id: I3cfa6a05f9bdfbbba7820060bae5a673dee43014
Reviewed-on: https://boringssl-review.googlesource.com/1385
Reviewed-by: Adam Langley <agl@google.com>
The code guarded by PKCS1_CHECK appears to be unhelpful, and the guard
is explicitly undefined in ssl_locl.h Remove both.
Change-Id: I3cd45a744a8f35b02181b1e48fd1ef11af5e6f4a
Reviewed-on: https://boringssl-review.googlesource.com/1383
Reviewed-by: Adam Langley <agl@google.com>
Use it to test DHE-RSA in BoringSSL.
Change-Id: I88f7bfa76507a6f60234d61d494c9f94b7df4e0a
Reviewed-on: https://boringssl-review.googlesource.com/1377
Reviewed-by: Adam Langley <agl@google.com>
Default to the number of CPUs. Avoids the tests launching 64 valgrinds in
parallel on machines without gobs of memory.
Change-Id: I9eeb365b48aa7407e303d161f90ce69a591a884c
Reviewed-on: https://boringssl-review.googlesource.com/1375
Reviewed-by: Adam Langley <agl@google.com>
Assert that inappropriate fallbacks are detected, but if the client_version
matches the server's highest version, do not abort the handshake.
Change-Id: I9d72570bce45e1eb23fc2b74a3c5fca10562e573
Reviewed-on: https://boringssl-review.googlesource.com/1373
Reviewed-by: Adam Langley <agl@google.com>
Should have test coverage there as long as we care about supporting it.
Change-Id: Ic67539228b550f2ebd0b543d5a58640913b0474b
Reviewed-on: https://boringssl-review.googlesource.com/1371
Reviewed-by: Adam Langley <agl@google.com>
A modern TLS library without full support for TLS does not make sense.
Change-Id: I032537d1412f6e4effc9a2dd47123baf0084b4c6
Reviewed-on: https://boringssl-review.googlesource.com/1382
Reviewed-by: Adam Langley <agl@google.com>
OPENSSL_NO_CAMELLIA has already been effectively defined, including in
opensslfeatures.h. This commit removes the last ifdef-protected code
guarded by OPENSSL_NO_CAMELLIA.
Change-Id: I58dc79dbe7a77843a641d9216f40f1d7d63fcc40
Reviewed-on: https://boringssl-review.googlesource.com/1380
Reviewed-by: Adam Langley <agl@google.com>
It's not built. The problem is worked around by the padding extension now.
Change-Id: If577efdae57d1bca4e0a626486fc0502c3567ebb
Reviewed-on: https://boringssl-review.googlesource.com/1374
Reviewed-by: Adam Langley <agl@google.com>
This change marks public symbols as dynamically exported. This means
that it becomes viable to build a shared library of libcrypto and libssl
with -fvisibility=hidden.
On Windows, one not only needs to mark functions for export in a
component, but also for import when using them from a different
component. Because of this we have to build with
|BORINGSSL_IMPLEMENTATION| defined when building the code. Other
components, when including our headers, won't have that defined and then
the |OPENSSL_EXPORT| tag becomes an import tag instead. See the #defines
in base.h
In the asm code, symbols are now hidden by default and those that need
to be exported are wrapped by a C function.
In order to support Chromium, a couple of libssl functions were moved to
ssl.h from ssl_locl.h: ssl_get_new_session and ssl_update_cache.
Change-Id: Ib4b76e2f1983ee066e7806c24721e8626d08a261
Reviewed-on: https://boringssl-review.googlesource.com/1350
Reviewed-by: Adam Langley <agl@google.com>
Chromium is no longer using it.
Change-Id: If56340627d2024ff3fb8561405dd0cfc6f4787cb
Reviewed-on: https://boringssl-review.googlesource.com/1346
Reviewed-by: Adam Langley <agl@google.com>
Use same logic when determining when to expect a client certificate for
both TLS and DTLS.
PR#3452
(Imported from upstream's 666a597ffb9bcf3ba2d49e711fcca28df91eff9d)
Change-Id: Ia267255a32c0b3b9a7da1c53f13ef6f620ff5ec1
One of the state transitions wasn't rewritten to CR_CHANGE. Add a test to
exercise this codepath. Also SSL_cutthrough_complete references the state.
Change-Id: Ib2f7ac5ac3f0348864efa93cf13cfd87454572f0
Reviewed-on: https://boringssl-review.googlesource.com/1337
Reviewed-by: Adam Langley <agl@google.com>
The length is the number of elements now, not the size in bytes. Caught by
ASan.
Change-Id: I4c5ccee61711e8d2e272b9bacd292dbff04b5133
Reviewed-on: https://boringssl-review.googlesource.com/1336
Reviewed-by: Adam Langley <agl@google.com>
Although the PKCS#1 padding check is internally constant-time, it is not
constant time at the crypto/ ssl/ API boundary. Expose a constant-time
RSA_message_index_PKCS1_type_2 function and integrate it into the
timing-sensitive portion of the RSA key exchange logic.
Change-Id: I6fa64ddc9d65564d05529d9b2985da7650d058c3
Reviewed-on: https://boringssl-review.googlesource.com/1301
Reviewed-by: Adam Langley <agl@google.com>
Now that the flag is set accurately, use it to enforce that the handshake and
CCS synchronization. If EXPECT_CCS is set, enforce that:
(a) No handshake records may be received before ChangeCipherSpec.
(b) There is no pending handshake data at the point EXPECT_CCS is set.
Change-Id: I04b228fe6a7a771cf6600b7d38aa762b2d553f08
Reviewed-on: https://boringssl-review.googlesource.com/1299
Reviewed-by: Adam Langley <agl@google.com>
Introduce a CR_CHANGE state just before entering CR_FINISHED_A. This replaces
the CCS_OK in the CR_FINISHED_A/CR_FINISHED_B case which otherwise would get
applied after partial reads of Finished. The other CCS_OK settings are
redundant with this one.
The copy in tls_secret_session_cb codepath is made unnecessary with
9eaeef81fa.
The copy in the normal session resumption case is unnecessary with
6444287806. Before that commit, OpenSSL would
potentially read Finished a state early. Now that we are strict (and get the
book-keeping correct) for expecting the NewSessionTicket message it too is
redundant.
Of particular note is the one after ssl3_send_finished. That was added in
response to upstream's PR#3400. I've reproduced the bug and concluded it was
actually a bug around expecting a NewSessionTicket message. That has been fixed
properly in 6444287806 by resetting
tlsext_expect_ticket on renegotiations.
Change-Id: I6a928386994fcd5efff26a5f0efb12b65bf7f299
Reviewed-on: https://boringssl-review.googlesource.com/1298
Reviewed-by: Adam Langley <agl@google.com>
Rename SSL3_ST_SR_POST_CLIENT_CERT to SSL3_ST_SR_CHANGE and have this be the
point at which CCS_OK is set. The copy before ssl3_get_finished is redundant as
we never transition to SR_FINISHED directly.
Change-Id: I3eefeb821e7ae53d52dacc587fdc59de9ea9a667
Reviewed-on: https://boringssl-review.googlesource.com/1297
Reviewed-by: Adam Langley <agl@google.com>
This removes the last case where the server generates an RSA key for the
ServerKeyExchange. Remove the code for this. Client support to accept them
still remains.
Leave the APIs for now, but they don't do anything anymore.
Change-Id: I84439e034cc575719f5bc9b3e501165e12b62107
Reviewed-on: https://boringssl-review.googlesource.com/1286
Reviewed-by: Adam Langley <agl@google.com>
Slightly cleaner; it means we can use CBS_stow.
Change-Id: I074aa2d73a79648013dea025ee531beeea2af4a2
Reviewed-on: https://boringssl-review.googlesource.com/1287
Reviewed-by: Adam Langley <agl@google.com>
Now the only case where temporary RSA keys are used on the server end is
non-signing keys.
Change-Id: I55f6c206e798dd28548c386fdffd555ccc395477
Reviewed-on: https://boringssl-review.googlesource.com/1285
Reviewed-by: Adam Langley <agl@google.com>
SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG and
SSL_OP_NETSCAPE_DEMO_CIPHER_CHANGE_BUG. Neither of them have code that's even
enabled.
Change-Id: I866aabe1aa37e8ee145aaeaecaff6704c3ad21bc
Reviewed-on: https://boringssl-review.googlesource.com/1284
Reviewed-by: Adam Langley <agl@google.com>
Also fix a place where fixes for the condition for sending ServerKeyExchange in
s3_srvr.c were never propogated to d1_srvr.c. Tidy up that logic to use
ssl_cipher_requires_server_key_exchange and simplify the PSK check.
Change-Id: Ie36d378f733e59a8df405bc869f2346af59bd574
Reviewed-on: https://boringssl-review.googlesource.com/1283
Reviewed-by: Adam Langley <agl@google.com>
This removes support code for a "stream_mac" mode only used by GOST. Also get
rid of this
/* I should fix this up TLS TLS TLS TLS TLS XXXXXXXX */
comment next to it. It's not actually related to GOST (dates to OpenSSL initial
commit), but isn't especially helpful at this point.
Change-Id: Ib13c6e27e16e0d1fb59ed0142ddf913b9abc20b7
Reviewed-on: https://boringssl-review.googlesource.com/1281
Reviewed-by: Adam Langley <agl@google.com>
Some ssl23 functions that can be folded into ssl3, declarations and macros that
don't exist anymore.
Change-Id: I8057fb0bab8b6fe7e4da7b90a4945f7f22e29cd9
Reviewed-on: https://boringssl-review.googlesource.com/1280
Reviewed-by: Adam Langley <agl@google.com>
Without SSLv2, all cipher suite values are 2 bytes. Represent them as a
uint16_t and make all functions pass those around rather than pointers.
This removes SSL_CIPHER_find as it's unused.
Change-Id: Iea0b75abee4352a8333a4b8e39a161430ae55ea6
Reviewed-on: https://boringssl-review.googlesource.com/1259
Reviewed-by: Adam Langley <agl@google.com>
Accepting them as a server is still necessary, but this code is unreachable.
Without SSLv2 support, none of the cipher suites are SSLv2, so
ssl23_no_ssl2_ciphers always returns true and we send a V3ClientHello.
Change-Id: I09030f2c6e375660453c74e4f094d95e9908c3e1
Reviewed-on: https://boringssl-review.googlesource.com/1258
Reviewed-by: Adam Langley <agl@google.com>
Test both when the peer doesn't support session tickets and when the server
promises a NewSessionTicket message but doesn't deliver.
Change-Id: I48f338094002beac2e6b80e41851c72822b3b9d5
Reviewed-on: https://boringssl-review.googlesource.com/1300
Reviewed-by: Adam Langley <agl@google.com>
Don't retain curve IDs in serialized form; serialization only happens when
writing and reading from the wire. The internal representation is a uint16_t
which matches the range of the value and avoids all the checks for the first
byte being 0.
This also fixes a bug in tls1_check_ec_tmp_key's suite B logic; the || should
have been &&, though now it's gone.
This doesn't relieve some of the other assumptions about curve IDs:
tls1_set_curves still assumes that all curve IDs are under 32, and
tls1_ec_curve_id2nid still assumes 0 is not a valid curve ID. Add a
compile-time assert and a comment to document this. We're up to 28 now, so this
may well need to be revised sooner or later.
Remove SSL_get_shared_curve as it's new and unused API, using it in a loop is
O(N^3), and lets us simplify a function.
Change-Id: I82778cb82648d82f7b5de8c5341e0e1febdf5611
Reviewed-on: https://boringssl-review.googlesource.com/1256
Reviewed-by: Adam Langley <agl@google.com>
The shim is now passed two file descriptors. In a session resumption test, the
second is used in an abbreviated handshake immediately after the first.
Change-Id: I1f348c05f1a8ff2881fb46fc9e869696f74071c6
Reviewed-on: https://boringssl-review.googlesource.com/1291
Reviewed-by: Adam Langley <agl@google.com>
Per spec, the server sends it iff it sends the extension in ServerHello. There
is no need to probe for whether Finished is or isn't sent. NSS is strict about
this (wait_new_session_ticket never transitions to wait_change_cipher without a
NewSessionTicket message), so this is safe.
Reset tlsext_ticket_expected in ssl_scan_serverhello_tlsext to ensure state
from the initial handshake doesn't confuse renegotiation. This is another one
of those per-handshake states that should be systematically reset on each
handshake. For now, reset it properly at least.
Change-Id: I7d16439ce632b9abf42f62d5d8e1303cb6f0be1f
Reviewed-on: https://boringssl-review.googlesource.com/1296
Reviewed-by: Adam Langley <agl@google.com>
ssl3_get_new_session_ticket is sensible and fills in a session_id for stateless
sessions, so the resumption will already be detected at this point. Remove the
codepath in ssl3_client_hello which allows for resuming sessions with empty
session_ids. The rest of the code doesn't allow it either.
This removes another codepath where we potentially probe a Finished message
early.
Change-Id: I2749b5c65c7ce98c6f30566d8716360ff1bba24c
Reviewed-on: https://boringssl-review.googlesource.com/1295
Reviewed-by: Adam Langley <agl@google.com>
tls_session_secret_cb is used for EAP-FAST which computes the master secret
externally and enters the abbreviated handshake. It appears to only have been
working because ssl3_check_finished would drive it into the appropriate state
afterwards. That, in turn, only has been working because EAP-FAST misuses the
session ticket extension for some other field, so ssl3_check_finished isn't a
no-op.
Instead, set s->hit so it follows the abbreviated state machine directly.
If we ever build wpa_supplicant with BoringSSL, this will require some testing.
(And, if not, this API should be removed.)
Change-Id: Ie2992a9bba049f7120522b996f739906e38a070e
Reviewed-on: https://boringssl-review.googlesource.com/1294
Reviewed-by: Adam Langley <agl@google.com>
This removes one place where we set CCS_OK. ssl3_get_cert_verify already knows
whether or not to expect a CertificateVerify message, so there is no need to
look ahead and potentially read ChangeCipherSpec early.
Change-Id: I80f4ec218b073c1007b01dbe1e3bd529fb848d37
Reviewed-on: https://boringssl-review.googlesource.com/1293
Reviewed-by: Adam Langley <agl@google.com>
Some test code to insert a bogus session ticket was retained. Also,
decryptTicket mutated its input, in turn, mutating the ClientHello,
breaking the Finished hash.
The latter fix should probably be merged upstream.
Change-Id: I6949f842c67e19df8742561fb0b849af9f5f099d
Reviewed-on: https://boringssl-review.googlesource.com/1290
Reviewed-by: Adam Langley <agl@google.com>
Finished isn't always the first post-CCS message.
Change-Id: I4f70eeed57cf732693d07212b096efb2594c5b3c
Reviewed-on: https://boringssl-review.googlesource.com/1288
Reviewed-by: Adam Langley <agl@google.com>
This change can probably be ported over to upstream crypto/tls. The current Go
TLS implementation ignores the signature and hash algorithm lists in
CertificateVerify and CertificateRequest. Take these into account so that our
tests assert OpenSSL fills them out correctly.
Also fix a bug in the original code where 'err' within the switch block get
shadowed.
Change-Id: I5d9c0b31ebb4662ecc767ed885a20707f0e86216
Reviewed-on: https://boringssl-review.googlesource.com/1253
Reviewed-by: Adam Langley <agl@google.com>
They pass, but this is an error case that is probably worth a test.
Change-Id: I37b2eec34a1781fa8342eea57ee4f9da81ce17ed
Reviewed-on: https://boringssl-review.googlesource.com/1257
Reviewed-by: Adam Langley <agl@google.com>
Custom RSA and ECDSA keys may not expose the key material. Plumb and "opaque"
bit out of the *_METHOD up to EVP_PKEY. Query that in ssl_rsa.c to skip the
sanity checks for certificate and key matching.
Change-Id: I362a2d5116bfd1803560dfca1d69a91153e895fc
Reviewed-on: https://boringssl-review.googlesource.com/1255
Reviewed-by: Adam Langley <agl@google.com>
Android never did this - they patched out the point in the code that set
the SSL3_FLAGS_DELAY_CLIENT_FINISHED flag when doing False Start.
Also, from the unittests it appears that NSS doesn't do this either.
Thus this change brings BoringSSL into line with existing behaviour.
SSL3_FLAGS_DELAY_CLIENT_FINISHED wasn't introduced with False Start,
it's an option in vanilla OpenSSL. But I can't find anything that uses
it and, since it's going to be untested, I've removed it completely in
this change.
Change-Id: I910537bfa35e74ab88778b83612cf5607d485969
Reviewed-on: https://boringssl-review.googlesource.com/1221
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
SSL reason codes corresponding to alerts have special values. Teach
make_errors.go that values above 1000 are reserved (otherwise it will assign
new values in that namespace). Also fix all the existing reason codes which
corresponded to alerts.
Change-Id: Ieabdf8fd59f4802938616934e1d84e659227cf84
Reviewed-on: https://boringssl-review.googlesource.com/1212
Reviewed-by: Adam Langley <agl@google.com>
This one in code that's not compiled though.
Change-Id: I8fb6c2df4669a70223889d31b233b577cf3e6b22
Reviewed-on: https://boringssl-review.googlesource.com/1211
Reviewed-by: Adam Langley <agl@google.com>
Got one of the conditions flipped.
Change-Id: I327a9c13e42865459e8d69a431b0d3a2bc6b54a5
Reviewed-on: https://boringssl-review.googlesource.com/1210
Reviewed-by: Adam Langley <agl@google.com>
Also fix some DTLS cookie bugs. rcvd_cookie is never referenced after being
saved (and the length isn't saved, so it couldn't be used anyway), and the
cookie verification failed to check the length.
For convenience, add a CBS_mem_equal helper function. Saves a bit of
repetition.
Change-Id: I187137733b069f0ac8d8b1bf151eeb80d388b971
Reviewed-on: https://boringssl-review.googlesource.com/1174
Reviewed-by: Adam Langley <agl@google.com>
This avoids duplicating the code to build the final premaster in PSK and
ECDHE_PSK. It also ports it to CBB for an initial trial of the API. Computing
the premaster secret now proceeds in four steps:
1. If a PSK key exchange (alg_a), look up the pre-shared key.
2. Compute the premaster secret based on alg_k. If PSK, it's all zeros.
3. If a PSK key exchange (alg_a), wrap the premaster in a struct with the
pre-shared key.
4. Use the possibly modified premaster to compute the master secret.
Change-Id: Ib511dd2724cbed42c82b82a676f641114cec5470
Reviewed-on: https://boringssl-review.googlesource.com/1173
Reviewed-by: Adam Langley <agl@google.com>
The only PSK cipher suite that computes the shared secret early is PSK. Also
there were two (unreachable because of earlier checks) codepaths where we're
exit this function without a master secret.
Change-Id: I3b64fc007b83c4bc46ddb6e14382fb285d8095f9
Reviewed-on: https://boringssl-review.googlesource.com/1172
Reviewed-by: Adam Langley <agl@google.com>
More consistent with ssl3_send_server_key_exchange and the message name.
Change-Id: If0f435a89bdf117297d349099708fff0bd5a6e98
Reviewed-on: https://boringssl-review.googlesource.com/1170
Reviewed-by: Adam Langley <agl@google.com>
This avoids having to do the CBS_skip dance and is better about returning the
right alert.
Change-Id: Id84eba307d7c67269ccbc07a38d9044b6f4f7c6c
Reviewed-on: https://boringssl-review.googlesource.com/1169
Reviewed-by: Adam Langley <agl@google.com>
Also tidy up some variable names and update RSA_verify call for it no longer
returning -1. Add CBS helper functions for dealing with C strings.
Change-Id: Ibc398d27714744f5d99d4f94ae38210cbc89471a
Reviewed-on: https://boringssl-review.googlesource.com/1164
Reviewed-by: Adam Langley <agl@google.com>
Previously, public headers lived next to the respective code and there
were symlinks from include/openssl to them.
This doesn't work on Windows.
This change moves the headers to live in include/openssl. In cases where
some symlinks pointed to the same header, I've added a file that just
includes the intended target. These cases are all for backwards-compat.
Change-Id: I6e285b74caf621c644b5168a4877db226b07fd92
Reviewed-on: https://boringssl-review.googlesource.com/1180
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Also tidy up a little now that {RSA,ECDSA}_verify don't have two separate error
codes.
Change-Id: Id0e9248f63766771032a131fd96d86d2596ade32
Reviewed-on: https://boringssl-review.googlesource.com/1168
Reviewed-by: Adam Langley <agl@google.com>
It's current a void* and gets explicitly cast everywhere. Make it a uint8_t and
only add the casts when converting it come init_buf, which internally stores a
char*.
Change-Id: I28bed129e46ed37ee1ce378d5c3bd0738fc1177f
Reviewed-on: https://boringssl-review.googlesource.com/1163
Reviewed-by: Adam Langley <agl@google.com>
Missing ServerKeyExchange is handled, but only because it hits an
ERR_R_INTERNAL_ERROR in ssl3_send_client_key_exchange in trying to find the
server ECDH parameters. Be strict about requiring it for ECDHE.
Change-Id: Ifce5b73c8bd14746b8a2185f479d550e9e3f84df
Reviewed-on: https://boringssl-review.googlesource.com/1157
Reviewed-by: Adam Langley <agl@google.com>
NPNServerTest in runner.go provides test coverage.
Change-Id: I5503ccbc4270e7f9f42ebc30c21e8077a430cf9f
Reviewed-on: https://boringssl-review.googlesource.com/1162
Reviewed-by: Adam Langley <agl@google.com>
Server client certificate tests provide test coverage.
Change-Id: I272b8099675f2a747f3ca878327c5f0b6936a988
Reviewed-on: https://boringssl-review.googlesource.com/1160
Reviewed-by: Adam Langley <agl@google.com>
Introduce a ssl_cipher_has_server_public_key to save the repeated
NULL/PSK/RSA_PSK[*] check. Don't allow skipping to ServerKeyExchange when
expecting Certificate; the messages expected are determined by the cipher
suite. The ssl3_get_server_public_key call is already guarded.
As the previous test demonstrates, this is safe because of the
ssl3_check_cert_and_algorithm call, but avoid the looseness in the parsing
there.
[*] NB: we don't implement RSA_PSK, and OpenSSL has never implemented it.
Change-Id: I0571e6bcbeb8eb883f77878bdc98d1aa3a287cf3
Reviewed-on: https://boringssl-review.googlesource.com/1156
Reviewed-by: Adam Langley <agl@google.com>
This works, but there's enough shared codepaths that it's worth a test to
ensure it stays that way.
Change-Id: I5d5a729811e35832170322957258304213204e3b
Reviewed-on: https://boringssl-review.googlesource.com/1155
Reviewed-by: Adam Langley <agl@google.com>
This drops the bits of logic that allowed Certificate messages to be optional
for a KRB5 cipher suite.
Change-Id: I2a71b7c13d7e76f4f5542d4074169f80f3617240
Reviewed-on: https://boringssl-review.googlesource.com/1154
Reviewed-by: Adam Langley <agl@google.com>
This can't happen because we don't implement RSA_PSK, but we probably should
check here.
Probably |sess_cert| shouldn't be attached to SSL_SESSION anyway; it's only
relevant when initializing the session and if it's accessed afterwards, it'll
be shared and cause problems.
Change-Id: Id868e523195f33c22e057f9b89dc02fe68e9b554
Reviewed-on: https://boringssl-review.googlesource.com/1153
Reviewed-by: Adam Langley <agl@google.com>
Fixes bug introduced in c26c802a89. Only one of
the two halves got flipped.
Change-Id: I0b3905ab22b0f83f093e1720af85594b1a970a7f
Reviewed-on: https://boringssl-review.googlesource.com/1152
Reviewed-by: Adam Langley <agl@google.com>
Match the other EVP_DigestSignFinal implementations. Fix the instances in
ssl/t1_enc.c which were not following the EVP_DigestSignFinal contract; on
entry, *out_len should contain the size of the buffer.
Change-Id: Icd44d97a4c98704dea975798c0101d5a37274d17
Reviewed-on: https://boringssl-review.googlesource.com/1130
Reviewed-by: Adam Langley <agl@google.com>
Instead of, in the pre-TLS-1.2 case, reaching into the EVP and manually
signing, compute the digest separately from signing. Then use EVP_PKEY_sign.
This will make it easier to implement https://crbug.com/347404 by having only
one signing codepath as well as make that logic simpler.
Also add a bounds check while we're here, although the buffer is too large to
actually matter.
runner.go client auth tests should cover code changes.
Change-Id: I7d87181bbcc5a761660412452e508d24c4725327
Reviewed-on: https://boringssl-review.googlesource.com/1122
Reviewed-by: Adam Langley <agl@google.com>
This is the first of reorganizing state between connection state and handshake
state. The existing set are retained in cert_st for the server; they are server
configuration. The client gets a copy in s->s3->tmp alongside other handshake
state.
With other handshake state moved there, hopefully we can reset that state in
one go and possibly not even maintain it when there is no handshake in
progress. Rather than currently where we sometimes confused connection state
and handshake state and have to reset as appropriate on renegotiate.
While I'm here, document the fields and name them something more useful than
'ctypes'.
Change-Id: Ib927579f0004fc5c6854fce2127625df669b2b6d
Reviewed-on: https://boringssl-review.googlesource.com/1113
Reviewed-by: Adam Langley <agl@google.com>
Resolve one of the TODOs since it's quick. Adjust the
-expect-server-name test to assert it both in the normal codepath and
in the early callback, to provide test coverage for
SSL_early_callback_ctx_extension_get.
Change-Id: I4d71158b9fd2f4fbb54d3e51184bd25d117bdc91
Reviewed-on: https://boringssl-review.googlesource.com/1120
Reviewed-by: Adam Langley <agl@google.com>
ClientHello and ServerHello are not allowed to include duplicate extensions.
Add a new helper function to check this and call as appropriate. Remove ad-hoc
per-extension duplicate checks which are no unnecessary.
Add runner.go tests to verify such message correctly rejected.
Change-Id: I7babd5b642dfec941459512869e2dd6de26a831c
Reviewed-on: https://boringssl-review.googlesource.com/1100
Reviewed-by: Adam Langley <agl@google.com>
This is the only codepath that allowed a cert_st to be shared between two
ssl_st's. Given that the cert_st currently contains some per-connection and
even per-handshake state, this probably doesn't work.
Remove the function altogether and don't ref-count cert_st.
Change-Id: I66d5346117cb59b6063e7b9b893d1c4b40cb6867
Reviewed-on: https://boringssl-review.googlesource.com/1110
Reviewed-by: Adam Langley <agl@google.com>
The function names are wrong.
Change-Id: Icbaeb541a2dcc504f69af81a7505e5cfbeed91f0
Reviewed-on: https://boringssl-review.googlesource.com/1101
Reviewed-by: Adam Langley <agl@google.com>
We handle it externally now.
Change-Id: Ib561f64078809645195fd1a859b3256499038847
Reviewed-on: https://boringssl-review.googlesource.com/1098
Reviewed-by: Adam Langley <agl@google.com>
We pulled in some of upstream's removal, but not the rest of it.
Change-Id: I8ea6681848f3c59955b7d2ce935c077d024245be
Reviewed-on: https://boringssl-review.googlesource.com/1099
Reviewed-by: Adam Langley <agl@google.com>
Done with unifdef with some manual edits to remove empty lines.
Change-Id: I40d163539cab8ef0e01e45b7dc6a1a0a37733c3e
Reviewed-on: https://boringssl-review.googlesource.com/1097
Reviewed-by: Adam Langley <agl@google.com>
Along the way, clean up the certificate types code to not have the
hard-coded fixed-size array.
Change-Id: If3e5978f7c5099478a3dfa37a0a7059072f5454a
Reviewed-on: https://boringssl-review.googlesource.com/1103
Reviewed-by: Adam Langley <agl@google.com>
SSL_OP_NETSCAPE_CA_DN_BUG is not included in SSL_OP_ALL.
Change-Id: I1635ad2721ed2742b1dff189d68bfc67a1c840a6
Reviewed-on: https://boringssl-review.googlesource.com/1102
Reviewed-by: Adam Langley <agl@google.com>
Building without RSA support is unreasonable. Changes were made by
running
find . -type f -name *.c | xargs unifdef -m -U OPENSSL_NO_RSA
find . -type f -name *.h | xargs unifdef -m -U OPENSSL_NO_RSA
using unifdef 2.10 and some newlines were removed manually.
Change-Id: Iea559e2d4b3d1053f28a4a9cc2f7a3d1f6cabd61
Reviewed-on: https://boringssl-review.googlesource.com/1095
Reviewed-by: Adam Langley <agl@google.com>
Since crypto/ebcdic.{c,h} are not present in BoringSSL, remove the #ifdefs
Changes were made by running
find . -type f -name *.c | xargs unifdef -m -U CHARSET_EBCDIC
find . -type f -name *.h | xargs unifdef -m -U CHARSET_EBCDIC
using unifdef 2.10.
An additional two ifdefs (CHARSET_EBCDIC_not) were removed manually.
Change-Id: Ie174bb00782cc44c63b0f9fab69619b3a9f66d42
Reviewed-on: https://boringssl-review.googlesource.com/1093
Reviewed-by: Adam Langley <agl@google.com>
If parsing the MKI value fails, clnt is never freed.
Change-Id: Ic85edf0d6efc54ca0828f333bc389c0dbf58f491
Reviewed-on: https://boringssl-review.googlesource.com/1072
Reviewed-by: Adam Langley <agl@google.com>
client_shim.cc and runner.go are generalized to handle both ends. Plumb a bit
through the test case to control which and add server versions of all the
cipher suite tests.
Change-Id: Iab2640b390f7ed7160c9a9bf6bb34b8bec761b2e
Reviewed-on: https://boringssl-review.googlesource.com/1091
Reviewed-by: Adam Langley <agl@google.com>
dc9b141127 added a default case when importing
the patch but accidentally falls through all the time.
Change-Id: Ieb9beeb9e3ffcf77f2842841eda7d28a62fe8072
Reviewed-on: https://boringssl-review.googlesource.com/1073
Reviewed-by: Adam Langley <agl@google.com>
It wasn't actually testing SSL_enable_fallback_scsv, just that not calling it
didn't send an SCSV. Plumb the 'flag' parameter to testCase through and add a
test that asserts it does get sent when expected. (Make it a []string since Go
doesn't distinguish nil string from "" and for flexibility.)
Change-Id: I124c01e045aebbed5c1d87b7196de7c2026f26f3
Reviewed-on: https://boringssl-review.googlesource.com/1071
Reviewed-by: Adam Langley <agl@google.com>
Found no users of the functions which control the feature. (Also I don't
particularly want to port all of that to CBS...)
Change-Id: I55da42c44d57252bd47bdcb30431be5e6e90dc56
Reviewed-on: https://boringssl-review.googlesource.com/1061
Reviewed-by: Adam Langley <agl@google.com>
Match most other functions. 03973096f4 changed
the call site, but not the function itself.
Change-Id: Iac018cbed8a454f50fb0da5e2419f503d8cb652d
Reviewed-on: https://boringssl-review.googlesource.com/1060
Reviewed-by: Adam Langley <agl@google.com>
This code doesn't even get built unless you go out of your way to pass an
extension value at build time.
Change-Id: I92ffcdfb18505c96e5ef390c8954a54cee19967f
Reviewed-on: https://boringssl-review.googlesource.com/1063
Reviewed-by: Adam Langley <agl@google.com>
This change adds the infrastructure to use stateful AEADs in ssl/ and
specifically wires in the stitched, RC4-MD5 AEAD. Over time, all
cipher suites will be supported via the AEAD interface and the old
EVP_CIPHER code will die off.
Change-Id: I44ed3ca2672e1342c6b632be08fee9272d113f8e
Reviewed-on: https://boringssl-review.googlesource.com/1044
Reviewed-by: Adam Langley <agl@google.com>
If we need an extension, we can implement it in-library.
Change-Id: I0eac5affcd8e7252b998b6c86ed2068234134b08
Reviewed-on: https://boringssl-review.googlesource.com/1051
Reviewed-by: Adam Langley <agl@google.com>
This gives us systematic bounds-checking on all the parses. Also adds a
convenience function, CBS_memdup, for saving the current contents of a CBS.
Change-Id: I17dad74575f03121aee3f771037b8806ff99d0c3
Reviewed-on: https://boringssl-review.googlesource.com/1031
Reviewed-by: Adam Langley <agl@google.com>
Since all AEAD ciphers now go through EVP_AEAD interface, the code which
uses EVP_Cipher interface no longer needs any of AEAD handling logic.
This also removes EVP_CTRL_AEAD_TLS1_AAD from GCM interface, which was
duplicating non-TLS-specific GCM logic and is not used anymore.
Change-Id: I5ddae880e7bc921337f9149a0acfdd00c9a478c3
Now that the consuming code in ssl/ is removed, there is no need for this.
Leave SSL_COMP and STACK_OF(SSL_COMP) for now so as not to break any code which
manipulates the output of SSL_COMP_get_compression_methods to disable
compression.
Change-Id: Idf0a5debd96589ef6e7e56acf5d9259412b7d7a1
OpenSSL added SSL_get0_certificate_types and fixed the truncation (and
subsequent parse error) by adding an alternate copy of the data.
http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=9f27b1eec3175305e62eed87faa80e231f319ca0
Make SSL_get_client_certificate_types call SSL_get0_certificate_types to query
the new list. Remove when Chromium is switched over.
Also remove a now unnecessary cast because SSL_get_client_certificate_types
fixed the type of tmp.ctypes. Further fix it to use a size_t and match the
cert_st copy OpenSSL added.
BUG=388000
Change-Id: Ic6653e10e5a3c3ac6b3fe2a2322f388d6ffb0a06
With this change, calling SSL_enable_fallback_scsv on a client SSL* will
cause the fallback SCSV to be sent.
This is intended to be set when the client is performing TLS fallback
after a failed connection. (This only happens if the application itself
implements this behaviour: OpenSSL does not do fallback automatically.)
The fallback SCSV indicates to the server that it should reject the
connection if the version indicated by the client is less than the
version supported by the server.
See http://tools.ietf.org/html/draft-bmoeller-tls-downgrade-scsv-02.
Change-Id: I478d6d5135016f1b7c4aaa6c306a1a64b1d215a6
Allow CCS after finished has been sent by client: at this point
keys have been correctly set up so it is OK to accept CCS from
server. Without this renegotiation can sometimes fail.
PR#3400
(Imported from upstream's 90d94ce39ecc2fad7fb2b8eb6bde0c669a65ee81)
In the ssl_cipher_get_evp() function, fix off-by-one errors in index
validation before accessing arrays.
PR#3375
(Imported from upstream's 3d86077427f93dc46b18fee706b567ec32ac232a)
Based on an original patch by Joel Sing (OpenBSD) who also originally
identified the issue.
(Imported from upstream's 728bd41a159ea16a60111e7c1120ec2a005507b3)
If application uses tls_session_secret_cb for session resumption set the
CCS_OK flag.
(Imported from upstream's a21f350a76b34b66dcaf9c1676baec945f32e980)
A buffer overrun attack can be triggered by sending invalid DTLS fragments
to an OpenSSL DTLS client or server. This is potentially exploitable to
run arbitrary code on a vulnerable client or server.
Fixed by adding consistency check for DTLS fragments.
Thanks to Jüri Aedla for reporting this issue.
(Imported from upstream's eb6508d50c9a314b88ac155bd378cbd79a117c92)
Only accept change cipher spec when it is expected instead of at any
time. This prevents premature setting of session keys before the master
secret is determined which an attacker could use as a MITM attack.
Thanks to KIKUCHI Masashi (Lepidum Co. Ltd.) for reporting this issue
and providing the initial fix this patch is based on.
(Imported from upstream's 77719aefb8f549ccc7f04222174889615d62057b)
Unnecessary recursion when receiving a DTLS hello request can be used to
crash a DTLS client. Fixed by handling DTLS hello request without
recursion.
Thanks to Imre Rad (Search-Lab Ltd.) for discovering this issue.
(Imported from upstream's 8942b92c7cb5fa144bd79b7607b459d0b777164c)
Make sure there is an extra 4 bytes for server done message when
NETSCAPE_HANG_BUG is defined.
PR#3361
(Imported from upstream's 856a4585d6f7a856b90c93792cf1c1ed968d4a4b)
Regression test against CVE-2014-0160 (Heartbleed).
More info: http://mike-bland.com/tags/heartbleed.html
(Imported from upstream's 2312a84ca17c5ac133581552df7024957cf15bc8)
A missing bounds check in the handling of the TLS heartbeat extension
can be used to reveal up to 64k of memory to a connected client or
server.
Thanks for Neel Mehta of Google Security for discovering this bug and to
Adam Langley <agl@chromium.org> and Bodo Moeller <bmoeller@acm.org> for
preparing the fix (CVE-2014-0160)
(Imported from upstream's 7e840163c06c7692b796a93e3fa85a93136adbb2)
Don't clear verification errors from the error queue unless
SSL_BUILD_CHAIN_FLAG_CLEAR_ERROR is set.
If errors occur during verification and
SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR is set return 2 so applications can
issue warnings.
(Imported from upstream's 2dd6976f6d02f98b30c376951ac38f780a86b3b5)
New flags to build certificate chains. The can be used to rearrange
the chain so all an application needs to do is add all certificates
in arbitrary order and then build the chain to check and correct them.
Add verify error code when building chain.
(Imported from upstream's c5ea65b157e17743c881b9e348524b0281b3d39f)
If an application calls the macro SSL_CTX_get_extra_chain_certs
return either the old "shared" extra certificates or those associated
with the current certificate.
This means applications which call SSL_CTX_use_certificate_chain_file
and retrieve the additional chain using SSL_CTX_get_extra_chain_certs
will still work. An application which only wants to check the shared
extra certificates can call the new macro
SSL_CTX_get_extra_chain_certs_only
(Imported from upstream's e0d4272a583c760ce008b661b79baf8b3ff24561 and
3bff195dca617c4ec1630945fef93b792b418cc8)
Fix a bug in handling of 128 byte long PSK identity in
psk_client_callback.
OpenSSL supports PSK identities of up to (and including) 128 bytes in
length. PSK identity is obtained via the psk_client_callback,
implementors of which are expected to provide a NULL-terminated
identity. However, the callback is invoked with only 128 bytes of
storage thus making it impossible to return a 128 byte long identity and
the required additional NULL byte.
This CL fixes the issue by passing in a 129 byte long buffer into the
psk_client_callback. As a safety precaution, this CL also zeroes out the
buffer before passing it into the callback, uses strnlen for obtaining
the length of the identity returned by the callback, and aborts the
handshake if the identity (without the NULL terminator) is longer than
128 bytes.
Move ECC SSL extensions to the end.
WebSphere Application Server 7.0 appears to be intolerant of an empty
extension at the end. To that end, also ensure we never send an empty
padding extension.
PSK identity hint can be stored in SSL_CTX and in SSL/SSL_SESSION,
similar to other TLS parameters, with the value in SSL/SSL_SESSION
taking precedence over the one in SSL_CTX. The value in SSL_CTX is
shared (used as the default) between all SSL instances associated
with that SSL_CTX, whereas the value in SSL/SSL_SESSION is confined
to that particular TLS/SSL connection/session.
The existing implementation of TLS-PSK does not correctly distinguish
between PSK identity hint in SSL_CTX and in SSL/SSL_SESSION. This
change fixes these issues:
1. SSL_use_psk_identity_hint does nothing and returns "success" when
the SSL object does not have an associated SSL_SESSION.
2. On the client, the hint in SSL_CTX (which is shared between
multiple SSL instances) is overwritten with the hint received from
server or reset to NULL if no hint was received.
3. On the client, psk_client_callback is invoked with the hint from
SSL_CTX rather than from current SSL/SSL_SESSION (i.e., the one
received from the server). Issue #2 above masks this issue.
4. On the server, the hint in SSL/SSL_SESSION is ignored and the hint
from SSL_CTX is sent to the client.
5. On the server, the hint in SSL/SSL_SESSION is reset to the one in
SSL_CTX after the ClientKeyExchange message step.
This change fixes the issues by:
* Adding storage for the hint in the SSL object. The idea being that
the hint in the associated SSL_SESSION takes precedence.
* Reading the hint during the handshake only from the associated
SSL_SESSION object.
* Initializing the hint in SSL object with the one from the SSL_CTX
object.
* Initializing the hint in SSL_SESSION object with the one from the
SSL object.
* Making SSL_use_psk_identity_hint and SSL_get_psk_identity_hint
set/get the hint to/from SSL_SESSION associated with the provided
SSL object, or, if no SSL_SESSION is available, set/get the hint
to/from the provided SSL object.
* Removing code which resets the hint during handshake.
Fix limit checks in ssl_add_clienthello_tlsext and
ssl_add_serverhello_tlsext.
Some of the limit checks reference p rather than ret. p is the original
buffer position, not the current one. Fix those and rename p to orig so
it's clearer.
This change implements equal-preference groups of cipher suites. This
allows, for example, a server to prefer one of AES-GCM or ChaCha20
ciphers, but to allow the client to pick which one. When coupled with
clients that will boost AES-GCM in their preferences when AES-NI is
present, this allows us to use AES-GCM when the hardware exists and
ChaCha20 otherwise.
This patch removes support for empty records (which is almost
universally disabled via SSL_OP_ALL) and adds optional support for 1/n-1
record splitting.
The latter is not enabled by default, since it's not typically used on
servers, but it should be enabled in web browsers since there are known
attacks in that case (see BEAST).
This patch adds support for a different cipher list when the connection
is using TLS 1.1. This is intended to support the case where we want to
use AES with >= TLS 1.1 clients but RC4 otherwise because of the BEAST
attack.
This change adds functions to check membership of various cipher
families. Clients and servers need this in order to optimise the size of
records because different families have different amounts of prefix and
postfix overhead.
Limit the number of empty records that will be processed consecutively
in order to prevent ssl3_get_record from never returning.
Reported by "oftc_must_be_destroyed" and George Kadianakis.
(Called "cut through" for historical reasons in this patch.)
Enables SSL3+ clients to send application data immediately following the
Finished message even when negotiating full-handshakes. With this
patch, clients can negotiate SSL connections in 1-RTT even when
performing full-handshakes.
Initial fork from f2d678e6e89b6508147086610e985d4e8416e867 (1.0.2 beta).
(This change contains substantial changes from the original and
effectively starts a new history.)
