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>
