With DTLSv1_get_timeout de-ctrl-ified, the type checker complains about
OPENSSL_timeval. Existing callers all use the real timeval.
Now that OPENSSL_timeval is not included in any public structs, simply
forward-declare timeval itself in ssl.h and pull in winsock2.h in internal
headers.
Change-Id: Ieaf110e141578488048c28cdadb14881301a2ce1
Reviewed-on: https://boringssl-review.googlesource.com/4682
Reviewed-by: Adam Langley <agl@google.com>
“ECDHE-PSK-WITH-AES-128-GCM-SHA256” doesn't follow the standard naming
for OpenSSL: it was “-WITH-” in it and has a hyphen between “AES” and
“128”. This change fixes that.
Change-Id: I7465b1ec83e7d5b9a60d8ca589808aeee10c174e
Reviewed-on: https://boringssl-review.googlesource.com/4601
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This will be reverted in a minute. The bots should run both suites of tests and
report the names of all failing tests in the summary.
Change-Id: Ibe351017dfa8ccfd182b3c88eee413cd2cbdeaf0
This is a really dumb API wart. Now that we have a limited set of curves that
are all reasonable, the automatic logic should just always kick in. This makes
set_ecdh_auto a no-op and, instead of making it the first choice, uses it as
the fallback behavior should none of the older curve selection APIs be used.
Currently, by default, server sockets can only use the plain RSA key exchange.
BUG=481139
Change-Id: Iaabc82de766cd00968844a71aaac29bd59841cd4
Reviewed-on: https://boringssl-review.googlesource.com/4531
Reviewed-by: Adam Langley <agl@google.com>
See upstream's bd891f098bdfcaa285c073ce556d0f5e27ec3a10. It honestly seems
kinda dumb for a client to do this, but apparently the spec allows this.
Judging by code inspection, OpenSSL 1.0.1 also allowed this, so this avoids a
behavior change when switching from 1.0.1 to BoringSSL.
Add a test for this, which revealed that, unlike upstream's version, this
actually works with ecdh_auto since tls1_get_shared_curve also needs updating.
(To be mentioned in newsletter.)
Change-Id: Ie622700f17835965457034393b90f346740cfca8
Reviewed-on: https://boringssl-review.googlesource.com/4464
Reviewed-by: Adam Langley <agl@google.com>
Because RFC 6066 is obnoxious like that and IIS servers actually do this
when OCSP-stapling is configured, but the OCSP server cannot be reached.
BUG=478947
Change-Id: I3d34c1497e0b6b02d706278dcea5ceb684ff60ae
Reviewed-on: https://boringssl-review.googlesource.com/4461
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I489d00bc4ee22a5ecad75dc1eb84776f044566e5
Reviewed-on: https://boringssl-review.googlesource.com/4391
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This causes any unexpected handshake records to be met with a fatal
no_renegotiation alert.
In addition, restore the redundant version sanity-checks in the handshake state
machines. Some code would zero the version field as a hacky way to break the
handshake on renego. Those will be removed when switching to this API.
The spec allows for a non-fatal no_renegotiation alert, but ssl3_read_bytes
makes it difficult to find the end of a ClientHello and skip it entirely. Given
that OpenSSL goes out of its way to map non-fatal no_renegotiation alerts to
fatal ones, this seems probably fine. This avoids needing to account for
another source of the library consuming an unbounded number of bytes without
returning data up.
Change-Id: Ie5050d9c9350c29cfe32d03a3c991bdc1da9e0e4
Reviewed-on: https://boringssl-review.googlesource.com/4300
Reviewed-by: Adam Langley <agl@google.com>
A previous change in BoringSSL renamed ERR_print_errors_fp to
BIO_print_errors_fp as part of refactoring the code to improve the
layering of modules within BoringSSL. Rename it back for better
compatibility with code that was using the function under the original
name. Move its definition back to crypto/err using an implementation
that avoids depending on crypto/bio.
Change-Id: Iee7703bb1eb4a3d640aff6485712bea71d7c1052
Reviewed-on: https://boringssl-review.googlesource.com/4310
Reviewed-by: Adam Langley <agl@google.com>
The rest of ssl/ still includes things everywhere, but this at least fixes the
includes that were implicit from ssl/internal.h.
Change-Id: I7ed22590aca0fe78af84fd99a3e557f4b05f6782
Reviewed-on: https://boringssl-review.googlesource.com/4281
Reviewed-by: Adam Langley <agl@google.com>
This drops in a copy of a subset of golang.org/x/crypto/poly1305 to implement
Poly1305. Hopefully this will keep them from regression as we rework the record
layer.
Change-Id: Ic1e0d941a0a9e5ec260151ced8acdf9215c4b887
Reviewed-on: https://boringssl-review.googlesource.com/4257
Reviewed-by: Adam Langley <agl@google.com>
After sharding the session cache for fallbacks, the numbers have been pretty
good; 0.03% on dev and 0.02% on canary. Stable is at 0.06% but does not have
the sharded session cache. Before sharding, stable, beta, and dev had been
fairly closely aligned. Between 0.03% being low and the fallback saving us in
all but extremely contrived cases, I think this should be fairly safe.
Add tests for both the cipher suite and protocol version mismatch checks.
BUG=441456
Change-Id: I2374bf64d0aee0119f293d207d45319c274d89ab
Reviewed-on: https://boringssl-review.googlesource.com/3972
Reviewed-by: Adam Langley <agl@google.com>
This conceivably has a use, but NSS doesn't do this buffer either and it still
suffers from the same problems as the other uses of record_pqueue. This removes
the last use of record_pqueue. It also opens the door to removing pqueue
altogether as it isn't the right data structure for either of the remaining
uses either. (It's not clear it was right for record_pqueue either, but I don't
feel like digging into this code.)
Change-Id: If8a43e7332b3cd11a78a516f3e8ebf828052316f
Reviewed-on: https://boringssl-review.googlesource.com/4239
Reviewed-by: Adam Langley <agl@google.com>
If the key is missing, it seems the failure is assumed to be expected.
BUG=473924
Change-Id: I62edd9110fa74bee5e6425fd6786badf5398728c
Reviewed-on: https://boringssl-review.googlesource.com/4231
Reviewed-by: Adam Langley <agl@google.com>
Move the state to TestState rather than passing pointers to them everywhere.
Also move SSL_read and SSL_write retry loops into helper functions so they
aren't repeated everywhere. This also makes the SSL_write calls all
consistently account for partial writes.
Change-Id: I9bc083a03da6a77ab2fc03c29d4028435fc02620
Reviewed-on: https://boringssl-review.googlesource.com/4214
Reviewed-by: Adam Langley <agl@google.com>
Extend the False Start tests to optionally send an alert (thus avoiding
deadlock) before waiting for the out-of-order app data. Based on whether the
peer shuts off the connection before or after sending app data, we can
determine whether the peer False Started by observing purely external effects.
Change-Id: I8b9fecc29668e0b0c34b5fd19d0f239545011bae
Reviewed-on: https://boringssl-review.googlesource.com/4213
Reviewed-by: Adam Langley <agl@google.com>
Based on whether -false-start is passed, we expect SSL_CB_HANDSHAKE_DONE to or
not to fire. Also add a flag that asserts SSL_CB_HANDSHAKE_DONE does *not* fire
in any False Start test where the handshake fails after SSL_connect returns.
Change-Id: I6c5b960fff15e297531e15b16abe0b98be95bec8
Reviewed-on: https://boringssl-review.googlesource.com/4212
Reviewed-by: Adam Langley <agl@google.com>
That's a pretty obvious thing to test. I'm not sure how we forgot that one.
Change-Id: I7e1a7df6c6abbdd587e0f7723117f50d09faa5c4
Reviewed-on: https://boringssl-review.googlesource.com/4211
Reviewed-by: Adam Langley <agl@google.com>
ASan's own malloc interceptor isn't compatible with this mechanism; it doesn't
see calls to __libc_malloc.
Change-Id: Ibac5aa05c6e40f1c72dcee3a2597e96deffca62c
Reviewed-on: https://boringssl-review.googlesource.com/4191
Reviewed-by: Adam Langley <agl@google.com>
This is in preparation for using RAII in the unit tests. Those tests are built
in Chromium as well, but Chromium does not have C++11 library support across
all its toolchains. Compiler support is available, so add a partial
reimplementation of std::unique_ptr and std::move under crypto/test/. The
scopers for the crypto/ library are also moved there while the ones for ssl/
stay in ssl/test/.
Change-Id: I38f769acbc16a870db34649928575c7314b6e9f6
Reviewed-on: https://boringssl-review.googlesource.com/4120
Reviewed-by: Adam Langley <agl@google.com>
MinGW on Linux needs lowercase include files. On Windows this doesn't
matter since the filesystems are case-insensitive, but building
BoringSSL on Linux with MinGW has case-sensitive filesystems.
Change-Id: Id9c120d819071b041341fbb978352812d6d073bc
Reviewed-on: https://boringssl-review.googlesource.com/4090
Reviewed-by: Adam Langley <agl@google.com>
Partly inspired by the new state exposed in
dc3da93899, stress this codepath by spamming our
poor shim with warning alerts.
Change-Id: I876c6e52911b6eb57493cf3e1782b37ea96d01f8
Reviewed-on: https://boringssl-review.googlesource.com/4112
Reviewed-by: Adam Langley <agl@google.com>
Google C++ style allows these. It's also considerably less tedious and
error-prone than defining an out-of-line constructor.
Change-Id: Ib76ccf6079be383722433046ac5c5d796dd1f525
Reviewed-on: https://boringssl-review.googlesource.com/4111
Reviewed-by: Adam Langley <agl@google.com>
It was failing only on 32-bit for some reason. Part of TestConfig wasn't
getting initialized.
Change-Id: I2a3747a347a47b47e2357f34d32f8db86d6cc629
Reviewed-on: https://boringssl-review.googlesource.com/4110
Reviewed-by: Adam Langley <agl@google.com>
I'm guessing a previous iteration used dlsym to look up the real malloc.
Change-Id: I18be9ef4db4ed059400074c8507d4e2fea882fbc
Reviewed-on: https://boringssl-review.googlesource.com/4100
Reviewed-by: Adam Langley <agl@google.com>
Both on the client and the server.
Change-Id: I9892c6dbbb29938154aba4f53b10e8b5231f9c47
Reviewed-on: https://boringssl-review.googlesource.com/4071
Reviewed-by: Adam Langley <agl@google.com>
This is really just scar tissue with https://crbug.com/468889 being the real
underlying problem. But the test is pretty easy.
Change-Id: I5eca18fdcbde8665c0e6c3ac419a28152647d66f
Reviewed-on: https://boringssl-review.googlesource.com/4052
Reviewed-by: Adam Langley <agl@google.com>
In verifying the fix for CVE-2015-0291, I noticed we don't actually have any
test coverage for full handshakes on renegotiation. All our tests always do
resumptions.
Change-Id: Ia9b701e8a50ba9353fefb8cc4fb86e78065d0b40
Reviewed-on: https://boringssl-review.googlesource.com/4050
Reviewed-by: Adam Langley <agl@google.com>
Noticed these as I was poking around.
Change-Id: I93833a152583feced374c9febf7485bec7abc1c7
Reviewed-on: https://boringssl-review.googlesource.com/3973
Reviewed-by: Adam Langley <agl@google.com>
This fixes test flakiness on Windows.
BUG=467767
Change-Id: Ie69b5b43ddd524aadb15c53705f6ec860e928786
Reviewed-on: https://boringssl-review.googlesource.com/4001
Reviewed-by: Adam Langley <agl@google.com>
This callback receives information about the ClientHello and can decide
whether or not to allow the handshake to continue.
Change-Id: I21be28335fa74fedb5b73a310ee24310670fc923
Reviewed-on: https://boringssl-review.googlesource.com/3721
Reviewed-by: Adam Langley <agl@google.com>
This mostly[*] doesn't matter for TLS since the message would have been
rejected anyway, but, in DTLS, if the peer rejects our Finished, it will send
an encrypted alert. This will then cause it to hang, which isn't very helpful.
I've made the change on both TLS and DTLS so the two protocols don't diverge on
this point. It is true that we're accepting nominally encrypted and
authenticated alerts before Finished, but, prior to ChangeCipherSpec, the
alerts are sent in the clear anyway so an attacker could already inject alerts.
A consumer could only be sensitive to it being post-CCS if it was watching
msg_callback. The only non-debug consumer of msg_callback I've found anywhere
is some hostapd code to detect Heartbeat.
See https://code.google.com/p/webrtc/issues/detail?id=4403 for an instance
where the equivalent behavior in OpenSSL masks an alert.
[*] This does change behavior slightly if the peer sends a warning alert
between CCS and Finished. I believe this is benign as warning alerts are
usually ignored apart from info_callback and msg_callback. The one exception is
a close_notify which is a slightly new state (accepting close_notify during a
handshake seems questionable...), but they're processed pre-CCS too.
Change-Id: Idd0d49b9f9aa9d35374a9f5e2f815cdb931f5254
Reviewed-on: https://boringssl-review.googlesource.com/3883
Reviewed-by: Adam Langley <agl@google.com>
Notably, drop all special cases around receiving a message in order and
receiving a full message. It makes things more complicated and was the source
of bugs (the MixCompleteMessageWithFragments tests added in this CL did not
pass before). Instead, every message goes through an hm_fragment, and
dtls1_get_message always checks buffered_messages to see if the next is
complete.
The downside is that we pay one more copy of the message data in the common
case. This is only during connection setup, so I think it's worth the
simplicity. (If we want to optimize later, we could either tighten
ssl3_get_message's interface to allow the handshake data being in the
hm_fragment's backing store rather than s->init_buf or swap out s->init_buf
with the hm_fragment's backing store when a mesasge completes.
This CL does not address ssl_read_bytes being an inappropriate API for DTLS.
Future work will revise the handshake/transport boundary to align better with
DTLS's needs. Also other problems that I've left as TODOs.
Change-Id: Ib4570d45634b5181ecf192894d735e8699b1c86b
Reviewed-on: https://boringssl-review.googlesource.com/3764
Reviewed-by: Adam Langley <agl@google.com>
We actually don't really care about this special-case since we only test client
full handshakes where the runner sends the second Finished not the shim
(otherwise the overlap logic and retransmitting on every fragment would
probably break us), but it should probably live next to the fragmentation
logic.
Change-Id: I54097d84ad8294bc6c42a84d6f22f496e63eb2a8
Reviewed-on: https://boringssl-review.googlesource.com/3763
Reviewed-by: Adam Langley <agl@google.com>
If the peer fragments Finished into multiple pieces, there is no need to
retransmit multiple times.
Change-Id: Ibf708ad079e1633afd420ff1c9be88a80020cba9
Reviewed-on: https://boringssl-review.googlesource.com/3762
Reviewed-by: Adam Langley <agl@google.com>
The old test just sent an empty ServerKeyExchange which is sufficient as we
reject the message early. But be more thorough and implement the actual
ephemeral key logic in the test server.
Change-Id: I016658762e4502c928c051e14d69eea67b5a495f
Reviewed-on: https://boringssl-review.googlesource.com/3650
Reviewed-by: Adam Langley <agl@google.com>
That might be a reasonable check to make, maybe.
DTLS handshake message reading has a ton of other bugs and needs a complete
rewrite. But let's fix this and get a test in now.
Change-Id: I4981fc302feb9125908bb6161ed1a18288c39e2b
Reviewed-on: https://boringssl-review.googlesource.com/3600
Reviewed-by: Adam Langley <agl@google.com>
Test both asynchronous and synchronous versions. This callback is somewhat
different from others. It's NOT called a second time when the handshake is
resumed. This appears to be intentional and not a mismerge from the internal
patch. The caller is expected to set up any state before resuming the handshake
state machine.
Also test the early callback returning an error.
Change-Id: If5e6eddd7007ea5cdd7533b4238e456106b95cbd
Reviewed-on: https://boringssl-review.googlesource.com/3590
Reviewed-by: Adam Langley <agl@google.com>
This involves more synchronization with child exits as the kernel no longer
closes the pre-created pipes for free, but it works on Windows. As long as
TCP_NODELAY is set, the performance seems comparable. Though it does involve
dealing with graceful socket shutdown. I couldn't get that to work on Windows
without draining the socket; not even SO_LINGER worked. Current (untested)
theory is that Windows refuses to gracefully shutdown a socket if the peer
sends data after we've stopped reading.
cmd.ExtraFiles doesn't work on Windows; it doesn't use fds natively, so you
can't pass fds 4 and 5. (stdin/stdout/stderr are special slots in
CreateProcess.) We can instead use the syscall module directly and mark handles
as inheritable (and then pass the numerical values out-of-band), but that
requires synchronizing all of our shim.Start() calls and assuming no other
thread is spawning a process.
PROC_THREAD_ATTRIBUTE_HANDLE_LIST fixes threading problems, but requires
wrapping more syscalls. exec.Cmd also doesn't let us launch the process
ourselves. Plus it still requires every handle in the list be marked
inheritable, so it doesn't help if some other thread is launching a process
with bInheritHandles TRUE but NOT using PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
(Like Go, though we can take syscall.ForkLock there.)
http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx
The more natively Windows option seems to be named pipes, but that too requires
wrapping more system calls. (To be fair, that isn't too painful.) They also
involve a listening server, so we'd still have to synchronize with shim.Wait()
a la net.TCPListener.
Then there's DuplicateHandle, but then we need an out-of-band signal.
All in all, one cross-platform implementation with a TCP sockets seems
simplest.
Change-Id: I38233e309a0fa6814baf61e806732138902347c0
Reviewed-on: https://boringssl-review.googlesource.com/3563
Reviewed-by: Adam Langley <agl@google.com>
Upstream settled in this API, and it's also the one that we expect
internally and that third_party code will expect.
Change-Id: Id7af68cf0af1f2e4d9defd37bda2218d70e2aa7b
Reviewed-on: https://boringssl-review.googlesource.com/3542
Reviewed-by: Adam Langley <agl@google.com>
