Check against the write encryption state, not the read state.
Change-Id: Ib3d8e02800e37bd089ef02c67a0b7e5dc009b1a5
Reviewed-on: https://boringssl-review.googlesource.com/8330
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
In TLS 1.3, the iv_length is equal to the explicit AEAD nonce length,
and is required to be at least 8 bytes.
Change-Id: Ib258f227d0a02c5abfc7b65adb4e4a689feffe33
Reviewed-on: https://boringssl-review.googlesource.com/8304
Reviewed-by: David Benjamin <davidben@google.com>
Both messages go between CCS and Finished. We weren't testing their relative
order and one of the state machine edges. Also test resume + NPN since that too
is a different handshake shape.
Change-Id: Iaeaf6c2c9bfd133103e2fb079d0e5a86995becfd
Reviewed-on: https://boringssl-review.googlesource.com/8196
Reviewed-by: Adam Langley <agl@google.com>
SSL_set_bio has some rather complex ownership story because whether rbio/wbio
are both owning depends on whether they are equal. Moreover, whether
SSL_set_bio(ssl, rbio, wbio) frees ssl->rbio depends on whether rbio is the
existing rbio or not. The current logic doesn't even get it right; see tests.
Simplify this. First, rbio and wbio are always owning. All the weird ownership
cases which we're stuck with for compatibility will live in SSL_set_bio. It
will internally BIO_up_ref if necessary and appropriately no-op the left or
right side as needed. It will then call more well-behaved ssl_set_rbio or
ssl_set_wbio functions as necessary.
Change-Id: I6b4b34e23ed01561a8c0aead8bb905363ee413bb
Reviewed-on: https://boringssl-review.googlesource.com/8240
Reviewed-by: Adam Langley <agl@google.com>
Their implementations expose a lot of really weird SSL_set_bio behavior. Note
that one test must be disabled as it doesn't even work. The subsequent commit
will re-enable it.
Change-Id: I4b7acadd710b3be056951886fc3e073a5aa816de
Reviewed-on: https://boringssl-review.googlesource.com/8272
Reviewed-by: Adam Langley <agl@google.com>
This is not very satisfactory.
Change-Id: I7e7a86f921e66f8f830c72eac084e9fea5ffd4d9
Reviewed-on: https://boringssl-review.googlesource.com/8270
Reviewed-by: David Benjamin <davidben@google.com>
By corrupting the X25519 and Newhope parts separately, the test shows
that both are in use. Possibly excessive?
Change-Id: Ieb10f46f8ba876faacdafe70c5561c50a5863153
Reviewed-on: https://boringssl-review.googlesource.com/8250
Reviewed-by: Adam Langley <agl@google.com>
The functions it calls all pass through <= 0 as error codes, not < 0.
Change-Id: I9d0d6b1df0065efc63f2d3a5e7f3497b2c28453a
Reviewed-on: https://boringssl-review.googlesource.com/8237
Reviewed-by: Adam Langley <agl@google.com>
There's a __pragma expression which allows this. Android builds us Windows with
MinGW for some reason, so we actually do have to tolerate non-MSVC-compatible
Windows compilers. (Clang for Windows is much more sensible than MinGW and
intentionally mimicks MSVC.)
MinGW doesn't understand MSVC's pragmas and warns a lot. #pragma warning is
safe to suppress, so wrap those to shush them. This also lets us do away with a
few ifdefs.
Change-Id: I1f5a8bec4940d4b2d947c4c1cc9341bc15ec4972
Reviewed-on: https://boringssl-review.googlesource.com/8236
Reviewed-by: Adam Langley <agl@google.com>
While most of OpenSSL's assembly allows out < in too, some of it doesn't.
Upstream seems to not consider this a problem (or, at least, they're failing to
make a decision on whether it is a problem, so we should assume they'll stay
their course). Accordingly, require aliased buffers to exactly align so we
don't have to keep chasing this down.
Change-Id: I00eb3df3e195b249116c68f7272442918d7077eb
Reviewed-on: https://boringssl-review.googlesource.com/8231
Reviewed-by: Adam Langley <agl@google.com>
Decrypting is very easy to do in-place, but encrypting in-place is a hassle.
The rules actually were wrong due to record-splitting. The aliasing prefix and
the alignment prefix actually differ by 1. Take it out for now in preparation
for tightening the aliasing rules.
If we decide to do in-place encrypt later, probably it'd be more useful to
return header + in-place ciphertext + trailer. (That, in turn, needs a
scatter/gather thing on the AEAD thanks to TLS 1.3's padding and record type
construction.) We may also wish to rethink how record-splitting works here.
Change-Id: I0187d39c541e76ef933b7c2c193323164fd8a156
Reviewed-on: https://boringssl-review.googlesource.com/8230
Reviewed-by: Adam Langley <agl@google.com>
Set ctx->error = X509_V_ERR_OUT_OF_MEM when verification cannot
continue due to malloc failure. Similarly for issuer lookup failures
and caller errors (bad parameters or invalid state).
Also, when X509_verify_cert() returns <= 0 make sure that the
verification status does not remain X509_V_OK, as a last resort set
it it to X509_V_ERR_UNSPECIFIED, just in case some code path returns
an error without setting an appropriate value of ctx->error.
Add new and some missing error codes to X509 error -> SSL alert switch.
(Imported from upstream's 5553a12735e11bc9aa28727afe721e7236788aab.)
Change-Id: I3231a6b2e72a3914cb9316b8e90ebaee009a1c5f
Reviewed-on: https://boringssl-review.googlesource.com/8170
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I0aaf9d926a81c3a10e70ae3ae6605d4643419f89
Reviewed-on: https://boringssl-review.googlesource.com/8210
Reviewed-by: Taylor Brandstetter <deadbeef@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This callback is used by BoringSSL tests in order to simulate the time,
so that the tests have repeatable results. This API will allow consumers
of BoringSSL to write the same sort of tests.
Change-Id: I79d72bce5510bbd83c307915cd2cc937579ce948
Reviewed-on: https://boringssl-review.googlesource.com/8200
Reviewed-by: David Benjamin <davidben@google.com>
It's useful, when combined with patching crypto/rand/deterministic.c in, for
debugging things. Also if we want to record fuzzer transcripts again, this
probably should be on.
Change-Id: I109cf27ebab64f01a13466f0d960def3257d8750
Reviewed-on: https://boringssl-review.googlesource.com/8192
Reviewed-by: David Benjamin <davidben@google.com>
Depending on bittedness of the runner, uint16 * uint16 can overflow an int.
There's other computations that can overflow a uint32 as well, so I just made
everything uint64 to avoid thinking about it too much.
Change-Id: Ia3c976987f39f78285c865a2d7688600d73c2514
Reviewed-on: https://boringssl-review.googlesource.com/8193
Reviewed-by: Adam Langley <agl@google.com>
The separation is purely historical (what happened to use an SSL_ctrl hook), so
put them all in one place. Make a vague attempt to match the order of the
header file, though we're still very far from matching.
Change-Id: Iba003ff4a06684a6be342e438d34bc92cab1cd14
Reviewed-on: https://boringssl-review.googlesource.com/8189
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
-timeout collides with go test's flags.
Change-Id: Icfc954915a61f1bb4d0acc8f02ec8a482ea10158
Reviewed-on: https://boringssl-review.googlesource.com/8188
Reviewed-by: David Benjamin <davidben@google.com>
Match the actual name of the type.
Change-Id: I0ad27196ee2876ce0690d13068fa95f68b05b0da
Reviewed-on: https://boringssl-review.googlesource.com/8187
Reviewed-by: David Benjamin <davidben@google.com>
Give them much more reasonable names.
Change-Id: Id14d983ab3231da21a4f987e662c2e01af7a2cd6
Reviewed-on: https://boringssl-review.googlesource.com/8185
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reorder states and functions by where they appear in the handshake. Remove
unnecessary hooks on SSL_PROTOCOL_METHOD.
Change-Id: I78dae9cf70792170abed6f38510ce870707e82ff
Reviewed-on: https://boringssl-review.googlesource.com/8184
Reviewed-by: David Benjamin <davidben@google.com>
Rather than reset the timer on every message, start it up immediately after
flushing one of our flights.
Change-Id: I97f8b4f572ceff62c546c94933b2700975c50a02
Reviewed-on: https://boringssl-review.googlesource.com/8180
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
It's unreachable and wouldn't work anyway. We'd never bubble up to the caller
to retry. As a consequence, the TLS side doesn't actually need to pay attention
to init_off.
(For now anyway. We'll probably need state of this sort once the write half is
all reworked. All the craziness with wpend_buf ought to be limited to the
SSL_write bits.)
Change-Id: I951534f6bbeb547ce0492d5647aaf76be42108a3
Reviewed-on: https://boringssl-review.googlesource.com/8179
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
It can be folded into dtls1_read_app_data. This code, since it still takes an
output pointer, does not yet process records atomically. (Though, being DTLS,
it probably should...)
Change-Id: I57d60785c9c1dd13b5b2ed158a08a8f5a518db4f
Reviewed-on: https://boringssl-review.googlesource.com/8177
Reviewed-by: David Benjamin <davidben@google.com>
This was probably the worst offender of them all as read_bytes is the wrong
abstraction to begin with. Note this is a slight change in how processing a
record works. Rather than reading one fragment at a time, we process all
fragments in a record and return. The intent here is so that all records are
processed atomically since the connection eventually will not be able to retain
a buffer holding the record.
This loses a ton of (though not quite all yet) those a2b macros.
Change-Id: Ibe4bbcc33c496328de08d272457d2282c411b38b
Reviewed-on: https://boringssl-review.googlesource.com/8176
Reviewed-by: David Benjamin <davidben@google.com>
read_close_notify is a very straight-forward hook and doesn't need much.
Change-Id: I7407d842321ea1bcb47838424a0d8f7550ad71ca
Reviewed-on: https://boringssl-review.googlesource.com/8174
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The business with ssl_record_prefix_len is rather a hassle. Instead, have
tls_open_record always decrypt in-place and give back a CBS to where the body
is.
This way the caller doesn't need to do an extra check all to avoid creating an
invalid pointer and underflow in subtraction.
Change-Id: I4e12b25a760870d8f8a503673ab00a2d774fc9ee
Reviewed-on: https://boringssl-review.googlesource.com/8173
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Alert handling is more-or-less identical across all contexts. Push it down from
read_bytes into the low-level record functions. This also deduplicates the code
shared between TLS and DTLS.
Now the only type mismatch managed by read_bytes is if we get handshake data in
read_app_data.
Change-Id: Ia8331897b304566e66d901899cfbf31d2870194e
Reviewed-on: https://boringssl-review.googlesource.com/8124
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This is getting a little repetitive.
Change-Id: Ib0fa8ab10149557c2d728b88648381b9368221d9
Reviewed-on: https://boringssl-review.googlesource.com/8126
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Move this logic out of dtls1_read_bytes and into dtls1_get_record. Only trigger
it when reading from the buffer fails. The other one shouldn't be necessary.
This exists to handle the blocking BIO case when the
BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT signal triggers, so we only need to do it when
timeouts actually trigger.
There also doesn't seem to be a need for most of the machinery. The
BIO_set_flags call seems to be working around a deficiency in the underlying
BIO. There also shouldn't be a need to check the handshake state as there
wouldn't be a timer to restart otherwise.
Change-Id: Ic901ccfb5b82aeb409d16a9d32c04741410ad6d7
Reviewed-on: https://boringssl-review.googlesource.com/8122
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The two modes are quite different. One of them requires the BIO honor an
extra BIO_ctrl. Also add an explanation at the top of
addDTLSRetransmitTests for how these tests work. The description is
scattered across many different places.
BUG=63
Change-Id: Iff4cdd1fbf4f4439ae0c293f565eb6780c7c84f9
Reviewed-on: https://boringssl-review.googlesource.com/8121
Reviewed-by: David Benjamin <davidben@google.com>
We've got it in entry points. That should be sufficient. (Do we even need it
there?)
Change-Id: I39b245a08fcde7b57e61b0bfc595c6ff4ce2a07a
Reviewed-on: https://boringssl-review.googlesource.com/8127
Reviewed-by: David Benjamin <davidben@google.com>
This cannot happen.
Change-Id: Ib1b473aa91d6479eeff43f7eaf94906d0b2c2a8f
Reviewed-on: https://boringssl-review.googlesource.com/8123
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
ssl->cert is never NULL. It gets created in SSL_new unconditionally.
Change-Id: I5c54c9c73e281e61a554820d61421226d763d33a
Reviewed-on: https://boringssl-review.googlesource.com/8125
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
In TLS 1.3, the actual record type is hidden within the encrypted data
and the record layer defaults to using a TLS 1.0 {3, 1} record version
for compatibility. Additionally the record layer no longer checks the
minor version of the record layer to maintain compatibility with the
TLS 1.3 spec.
Change-Id: If2c08e48baab170c1658e0715c33929d36c9be3a
Reviewed-on: https://boringssl-review.googlesource.com/8091
Reviewed-by: David Benjamin <davidben@google.com>
OpenSSL was actually super-buggy here (though known bugs on our end have been
fixed), but pyOpenSSL was confused and incorrectly documented that callers call
SSL_read after SSL_shutdown to do bidi shutdown, so we should probably support
this. Add a test that it works.
Change-Id: I2b6d012161330aeb4cf894bae3a0b6a55d53c70d
Reviewed-on: https://boringssl-review.googlesource.com/8093
Reviewed-by: Adam Langley <agl@google.com>
This keeps the naming convention in line with the actual spec.
Change-Id: I34673f78dbc29c1659b4da0e49677ebe9b79636b
Reviewed-on: https://boringssl-review.googlesource.com/8090
Reviewed-by: David Benjamin <davidben@google.com>
Windows SRWLOCK requires you call different functions here. Split
them up in preparation for switching Windows from CRITICAL_SECTION.
BUG=37
Change-Id: I7b5c6a98eab9ae5bb0734b805cfa1ff334918f35
Reviewed-on: https://boringssl-review.googlesource.com/8080
Reviewed-by: Adam Langley <agl@google.com>
This is easier to deploy, and more obvious. This commit reverts a few
pieces of e25775bc, but keeps most of it.
Change-Id: If8d657a4221c665349c06041bb12fffca1527a2c
Reviewed-on: https://boringssl-review.googlesource.com/8061
Reviewed-by: Adam Langley <agl@google.com>
This reverts commit c7eae5a326. pyOpenSSL
expects to be able to call |SSL_read| after a shutdown and get EOF.
Change-Id: Icc5faa09d644ec29aac99b181dac0db197f283e3
Reviewed-on: https://boringssl-review.googlesource.com/8060
Reviewed-by: Adam Langley <agl@google.com>
The TLS 1.3 spec has an explicit nonce construction for AEADs that
requires xoring the IV and sequence number.
Change-Id: I77145e12f7946ffb35ebeeb9b2947aa51058cbe9
Reviewed-on: https://boringssl-review.googlesource.com/8042
Reviewed-by: Adam Langley <agl@google.com>
Constants representing TLS 1.3 are added to allow for future work to be
flagged on TLS1_3_VERSION. To prevent BoringSSL from negotiating the
non-existent TLS 1.3 version, it is explicitly disabled using
SSL_OP_NO_TLSv1_3.
Change-Id: Ie5258a916f4c19ef21646c4073d5b4a7974d6f3f
Reviewed-on: https://boringssl-review.googlesource.com/8041
Reviewed-by: Adam Langley <agl@google.com>
