We only implement four curves (P-224, P-256, P-384, and P-521) and only
advertise the latter three by default. Don't maintain entries corresponding to
all the unimplemented curves.
Change-Id: I1816a10c6f849ca1d9d896bc6f4b64cd6b329481
Reviewed-on: https://boringssl-review.googlesource.com/2843
Reviewed-by: Adam Langley <agl@google.com>
They both happen to be zero, but OBJ_undef is a type error; OBJ_foo expands to
a comma-separated list of integers.
Change-Id: Ia5907dd3bc83240b7cc98af6456115d2efb48687
Reviewed-on: https://boringssl-review.googlesource.com/2842
Reviewed-by: Adam Langley <agl@google.com>
When parsing ClientHello clear any existing extension state from
SRP login and SRTP profile.
(Imported from upstream's 4f605ccb779e32a770093d687e0554e0bbb137d3)
More state that should be systematically reset across handshakes. Add a reset
on the ServerHello end too since that was missed.
Change-Id: Ibb4549acddfd87caf7b6ff853e2adbfa4b7e7856
Reviewed-on: https://boringssl-review.googlesource.com/2838
Reviewed-by: Adam Langley <agl@google.com>
write_quota should only be decremented by 1 in datagram mode, otherwise we'll
underflow and always allow writes through. This does not cause any existing
tests to fail.
(It will be useful once the bug in dtls1_do_write is fixed.)
Change-Id: I42aa001d7264790a3726269890635f679497fb1c
Reviewed-on: https://boringssl-review.googlesource.com/2831
Reviewed-by: Adam Langley <agl@google.com>
The existing comments are not very helpful. This code is also quite buggy.
Document two of them as TODOs.
Change-Id: Idfaf93d9c3b8b1ee92f2fb0d292ef513b5f6d824
Reviewed-on: https://boringssl-review.googlesource.com/2830
Reviewed-by: Adam Langley <agl@google.com>
BIO_ctrls do not have terribly well-defined return values on error. (Though the
existing ones seem to all return 0, not -1, on nonexistant operation.)
Change-Id: I08497f023ce3257c253aa71517a98b2fe73c3f74
Reviewed-on: https://boringssl-review.googlesource.com/2829
Reviewed-by: Adam Langley <agl@google.com>
g_probably_mtu and dtls1_guess_mtu is a bunch of logic for guessing the right
MTU, but it only ever returns the maximum (the input is always zero). Trim that
down to only what it actually does.
Change-Id: If3afe3f68ccb36cbf9c4525372564d16a4bbb73f
Reviewed-on: https://boringssl-review.googlesource.com/2828
Reviewed-by: Adam Langley <agl@google.com>
The retry doesn't actually work when we're sending a non-initial fragment; the
s->init_off != 0 block will get re-run each iteration through and continually
prepend headers. It can also infinite loop if the BIO reports
BIO_CTRL_DGRAM_MTU_EXCEEDED but either fails to report an MTU or reports an MTU
that always rounds up to the minimum. See upstream's
d3d9eef31661633f5b003a9e115c1822f79d1870.
WebRTC doesn't participate in any of the MTU logic and inherits the default
MTU, so just remove it for now.
Change-Id: Ib2ed2ba016b7c229811741fb7369c015ba0b551f
Reviewed-on: https://boringssl-review.googlesource.com/2827
Reviewed-by: Adam Langley <agl@google.com>
That setting means that the MTU is provided externally via SSL_set_mtu.
(Imported from upstream's 001235778a6e9c645dc0507cad6092d99c9af8f5)
Change-Id: I4e5743a9dee734ddd0235f080aefe98a7365aaf6
Reviewed-on: https://boringssl-review.googlesource.com/2826
Reviewed-by: Adam Langley <agl@google.com>
Based in part on upstream's cf75017bfd60333ff65edf9840001cd2c49870a3. This
situation really shouldn't be able to happen, but between no static asserts
that the minimum MTU is always large enough and a bug in reseting the MTU later
(to be fixed be a follow-up import from upstream), check these and return a
useful error code.
Change-Id: Ie853e5d35a6a7bc9c0032e74ae71529d490f4fe2
Reviewed-on: https://boringssl-review.googlesource.com/2825
Reviewed-by: Adam Langley <agl@google.com>
BoringSSL currently retransmits non-deterministically on an internal timer
(rather than one supplied externally), so the tests currently fail flakily
depending on timing. Valgrind is a common source for this. We still assume an
in-order and reliable channel, but drop retransmits silently:
- Handshake messages may arrive with old sequence numbers.
- Retransmitted CCS records arrive from the previous epoch.
- We may receive a retransmitted Finished after we believe the handshake has
completed. (Aside: even in a real implementation, only Finished is possible
here. Even with out-of-order delivery, retransmitted or reordered messages
earlier in the handshake come in under a different epoch.)
Note that because DTLS renego and a Finished retransmit are ambiguous at the
record layer[*], this precludes us writing tests for DTLS renego. But DTLS
renego should get removed anyway. As BoringSSL currently implements renego,
this ambiguity is also a source of complexity in the real implementation. (See
the SSL3_MT_FINISHED check in dtls1_read_bytes.)
[*] As a further fun aside, it's also complex if dispatching renego vs Finished
after handshake message reassembly. The spec doesn't directly say the sequence
number is reset across renegos, but it says "The first message each side
transmits in /each/ handshake always has message_seq = 0". This means that such
an implementation needs the handshake message reassembly logic be aware that a
Finished fragment with high sequence number is NOT an out-of-order fragment for
the next handshake.
Change-Id: I35d13560f82bcb5eeda62f4de1571d28c818cc36
Reviewed-on: https://boringssl-review.googlesource.com/2770
Reviewed-by: Adam Langley <agl@google.com>
As of our 82b7da271f, an SSL_SESSION created
externally always has a cipher set. Unknown ciphers are rejected early. Prior
to that, an SSL_SESSION would only have a valid cipher or valid cipher_id
depending on whether it came from an internal or external session cache.
See upstream's 6a8afe2201cd888e472e44225d3c9ca5fae1ca62 and
c566205319beeaa196e247400c7eb0c16388372b for more context.
Since we don't get ourselves into this strange situation and s->cipher is now
always valid for established SSL_SESSION objects (the existence of
unestablished SSL_SESSION objects during a handshake is awkward, but something
to deal with later), do away with s->cipher_id altogether. An application
should be able to handle failing to parse an SSL_SESSION instead of parsing it
successfuly but rejecting all resumptions.
Change-Id: I2f064a815e0db657b109c7c9269ac6c726d1ffed
Reviewed-on: https://boringssl-review.googlesource.com/2703
Reviewed-by: Adam Langley <agl@google.com>
This CL removes the last of the EVP_CIPHER codepath in ssl/. The dead code is
intentionally not pruned for ease of review, except in DTLS-only code where
adding new logic to support both, only to remove half, would be cumbersome.
Fixes made:
- dtls1_retransmit_state is taught to retain aead_write_ctx rather than
enc_write_ctx.
- d1_pkt.c reserves space for the variable-length nonce when echoed into the
packet.
- dtls1_do_write sizes the MTU based on EVP_AEAD max overhead.
- tls1_change_cipher_state_cipher should not free AEAD write contexts in DTLS.
This matches the (rather confused) ownership for the EVP_CIPHER contexts.
I've added a TODO to resolve this craziness.
A follow-up CL will remove all the resultant dead code.
Change-Id: I644557f4db53bbfb182950823ab96d5e4c908866
Reviewed-on: https://boringssl-review.googlesource.com/2699
Reviewed-by: Adam Langley <agl@google.com>
This introduces another knob into SSL_AEAD_CTX to omit the version from the ad
parameter. It also allows us to fold a few more SSL3_ENC_METHOD hooks together.
Change-Id: I6540d410d4722f734093554fb434dab6e5217d4f
Reviewed-on: https://boringssl-review.googlesource.com/2698
Reviewed-by: Adam Langley <agl@google.com>
This lets us fold away the SSLv3-specific generate_master_secret. Once SSLv3
uses AEADs, others will fold away as well.
Change-Id: I27c1b75741823bc6db920d35f5dd5ce71b6fdbb3
Reviewed-on: https://boringssl-review.googlesource.com/2697
Reviewed-by: Adam Langley <agl@google.com>
Fix up the generate_master_secret parameter while we're here.
Change-Id: I1c80796d1f481be0c3eefcf3222f2d9fc1de4a51
Reviewed-on: https://boringssl-review.googlesource.com/2696
Reviewed-by: Adam Langley <agl@google.com>
size_t all the parameters. Also explicitly label label as label. This is in
preparation for pulling the PRF out into SSL3_ENC_METHOD so more of the
SSL3_ENC_METHOD hooks may be shared between SSLv3 and TLS once SSLv3 uses
stateful AEADs.
Also port away from EVP_PKEY_HMAC and use HMAC_CTX directly. The abstraction
doesn't buy much and is different from all the other EVP_DigestSign* functions.
There are few enough users within BoringSSL and Google that we can probably
deprecate and eventually remove it altogether.
Change-Id: I5d4529438c8a2a992fc199388a0c9e73bd6d2e06
Reviewed-on: https://boringssl-review.googlesource.com/2695
Reviewed-by: Adam Langley <agl@google.com>
The EVP_CIPHER codepath should no longer be used with TLS. It still exists for
DTLS and SSLv3. The AEAD construction in TLS does not allow for
variable-overhead AEADs, so stateful AEADs do not include the length in the ad
parameter. Rather the AEADs internally append the unpadded length once it is
known. EVP_aead_rc4_md5_tls is modified to account for this.
Tests are added (and RC4-MD5's regenerated) for each of the new AEADs. The
cipher tests are all moved into crypto/cipher/test because there's now a lot of
them and they clutter the directory listing.
In ssl/, the stateful AEAD logic is also modified to account for stateful AEADs
with a fixed IV component, and for AEADs which use a random nonce (for the
explicit-IV CBC mode ciphers).
The new implementation fixes a bug/quirk in stateless CBC mode ciphers where
the fixed IV portion of the keyblock was generated regardless. This is at the
end, so it's only relevant for EAP-TLS which generates a MSK from the end of
the key block.
Change-Id: I2d8b8aa11deb43bde2fd733f4f90b5d5b8cb1334
Reviewed-on: https://boringssl-review.googlesource.com/2692
Reviewed-by: Adam Langley <agl@google.com>
It's not worth saving the extra mallocs. This is preparation for moving SSLv3
to stateful AEADs; it'll share code TLS's SSL3_ENC_METHOD, but
tls1_generate_key_block is different, so that'll be pulled out into its own
hook.
Change-Id: I3f2136600758465c66ce23736041bb47f74efa6d
Reviewed-on: https://boringssl-review.googlesource.com/2690
Reviewed-by: Adam Langley <agl@google.com>
More modern versions of GCC (at least with aarch64) are warning about an
unused value in these locations. It's incorrect, but I guess that the
macro is confusing it.
Using a (void) tag is a little ugly but solves the problem.
Change-Id: If6ba5083ab6e501c81e7743ae1ed99a89565e57c
Reviewed-on: https://boringssl-review.googlesource.com/2810
Reviewed-by: Adam Langley <agl@google.com>
The minimum MTU (not consistently enforced) is just under 256, so it's
difficult to test everything, but this is a basic test. (E.g., without renego,
the only handshake message with encryption is Finished which fits in the MTU.)
It tests the server side because the Certificate message is large enough to
require fragmentation.
Change-Id: Ida11f1057cebae2b800ad13696f98bb3a7fbbc5e
Reviewed-on: https://boringssl-review.googlesource.com/2824
Reviewed-by: Adam Langley <agl@google.com>
This should have been removed with its dtls1_clear cousin in
8c88153465.
Change-Id: Ibf4ee67348f603285b26766568cbb92183b62cee
Reviewed-on: https://boringssl-review.googlesource.com/2823
Reviewed-by: Adam Langley <agl@google.com>
State on s3 gets freed in both ssl3_clear and ssl3_free. Considate to just
ssl3_free. This replaces the (SSL,ssl,ssl3)_clear calls in (SSL,ssl,ssl3)_new
with the state that was initialized. This results in a little code duplication
between SSL_new and SSL_clear because state is on the wrong object. I've just
left TODOs for now; some of it will need disentangling.
We're far from it, but going forward, separate state between s and s->s3 as:
- s contains configuration state, DTLS or TLS. It is initialized from SSL_CTX,
configurable directly afterwards, and preserved across SSL_clear calls.
(Including when it's implicitly set as part of a handshake callback.)
- Connection state hangs off s->s3 (TLS) and s->d1 (DTLS). It is reset across
SSL_clear. This should happen naturally out of a ssl_free/ssl_new pair.
The goal is to avoid needing separate initialize and reset code for anything;
the point any particular state is reset is the point its owning context is
destroyed and recreated.
Change-Id: I5d779010778109f8c339c07433a0777feaf94d1f
Reviewed-on: https://boringssl-review.googlesource.com/2822
Reviewed-by: Adam Langley <agl@google.com>
Configuration data inherited from the ctx happens in SSL_new. (This also gets
in the way of using ssl3_free/ssl3_new to implement SSL_clear.)
Change-Id: I2773af91abf4e1edc0c1a324bc1e94088d7c2274
Reviewed-on: https://boringssl-review.googlesource.com/2821
Reviewed-by: Adam Langley <agl@google.com>
A memory leak can occur in dtls1_buffer_record if either of the calls to
ssl3_setup_buffers or pqueue_insert fail. The former will fail if there
is a malloc failure, whilst the latter will fail if attempting to add a
duplicate record to the queue. This should never happen because
duplicate records should be detected and dropped before any attempt to
add them to the queue. Unfortunately records that arrive that are for
the next epoch are not being recorded correctly, and therefore replays
are not being detected. Additionally, these "should not happen" failures
that can occur in dtls1_buffer_record are not being treated as fatal and
therefore an attacker could exploit this by sending repeated replay
records for the next epoch, eventually causing a DoS through memory
exhaustion.
Thanks to Chris Mueller for reporting this issue and providing initial
analysis and a patch. Further analysis and the final patch was performed
by Matt Caswell from the OpenSSL development team.
CVE-2015-0206
(Imported from upstream's 7c6a3cf2375f5881ef3f3a58ac0fbd0b4663abd1).
Change-Id: I765fe61c75bc295bcc4ab356b8a5ce88c8964764
Reviewed-on: https://boringssl-review.googlesource.com/2782
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Add a dedicated error code to the queue for a handshake_failure alert in
response to ClientHello. This matches NSS's client behavior and gives a better
error on a (probable) failure to negotiate initial parameters.
BUG=https://crbug.com/446505
Change-Id: I34368712085a6cbf0031902daf2c00393783d96d
Reviewed-on: https://boringssl-review.googlesource.com/2751
Reviewed-by: Adam Langley <agl@google.com>
The |skip_message| variable was overly complex and, since we have at
least 32-bit ints, we know that a 24-bit value doesn't overflow an int.
Change-Id: I5c16fa979e1716f39cc47882c033bcf5bce3284c
Reviewed-on: https://boringssl-review.googlesource.com/2610
Reviewed-by: Adam Langley <agl@google.com>
RAND_pseudo_bytes just calls RAND_bytes now and only returns 0 or 1. Switch all
callers within the library call the new one and use the simpler failure check.
This fixes a few error checks that no longer work (< 0) and some missing ones.
Change-Id: Id51c79deec80075949f73fa1fbd7b76aac5570c6
Reviewed-on: https://boringssl-review.googlesource.com/2621
Reviewed-by: Adam Langley <agl@google.com>
No current use of ssl_cert_type passes a NULL EVP_PKEY, so it can be simplified
a little.
Change-Id: I2052cc3b6069cd30e4685ba8a6d0014016a4d712
Reviewed-on: https://boringssl-review.googlesource.com/2620
Reviewed-by: Adam Langley <agl@google.com>
Currently we don't express an opinion. Most sites aren't likely to have a
choice since it depends on what certificates they have available. But we may as
well order them.
Change-Id: I4fffa5e392f42e19823cb8faa2e9e15a6bb91086
Reviewed-on: https://boringssl-review.googlesource.com/2607
Reviewed-by: Adam Langley <agl@google.com>
Turns out the EVP_CIPH_FLAG_CUSTOM_CIPHER ciphers (i.e. legacy EVP_CIPHER
AES-GCM) have a completely different return value setup than the normal ones
which are the standard one/zero. (Except that they never return zero; see
TODO.)
Fix checks in ssl/ and remove remnants of EVP_CIPH_FLAG_CUSTOM_CIPHER in ssl/
as we're using EVP_AEAD now.
See CHANGES entry added in upstream's 3da0ca796cae6625bd26418afe0a1dc47bf5a77f.
Change-Id: Ia4d0ff59b03c35fab3a08141c60b9534cb7172e2
Reviewed-on: https://boringssl-review.googlesource.com/2606
Reviewed-by: Adam Langley <agl@google.com>
Those version checks are if renego tried to change the version, but at that
point we're out of the initial null cipher and should leave the version fixed.
(On the server end, the code in question was dead after the version negotiation
rewrite anyway.)
Change-Id: I3242ba11bc9981ccf7fdb867176d59846cc49dd9
Reviewed-on: https://boringssl-review.googlesource.com/2605
Reviewed-by: Adam Langley <agl@google.com>
This avoids needing a should_add_to_finished_hash boolean on do_write. The
logic in do_write was a little awkward because do_write would be called
multiple times if the write took several iterations. This also gets complex if
DTLS retransmits are involved. (At a glance, it's not obvious the
BIO_CTRL_DGRAM_MTU_EXCEEDED case actually works.)
Doing it as the handshake message is being prepared avoids this concern. It
also gives a natural point for the extended master secret logic which needs to
do work after the finished hash has been sampled.
As a bonus, we can remove s->d1->retransmitting which was only used to deal
with this issue.
Change-Id: Ifedf23ee4a6c5e08f960d296a6eb1f337a16dc7a
Reviewed-on: https://boringssl-review.googlesource.com/2604
Reviewed-by: Adam Langley <agl@google.com>
That comment is wrong as of TLS 1.2.
Change-Id: I900d5efc09d7468f2601d85f867833e43d046f6a
Reviewed-on: https://boringssl-review.googlesource.com/2603
Reviewed-by: Adam Langley <agl@google.com>
(Or should we just drop this? It only matters for servers trying to use client
auth.)
Change-Id: I50b6999375dc8f9246bf617f17929ae304503c57
Reviewed-on: https://boringssl-review.googlesource.com/2602
Reviewed-by: Adam Langley <agl@google.com>
Some of the messages did the computation manually which would bite us if we
tried to transplant them between DTLS and TLS. More importantly, it precludes
moving the handshake hash computation from ssl_do_write to
ssl_set_handshake_header.
Change-Id: I9d400deb0720e62cb1ab905242eb0679ad650a46
Reviewed-on: https://boringssl-review.googlesource.com/2600
Reviewed-by: Adam Langley <agl@google.com>
The frag_off/frag_len parameters are always zero, and the return value is never
used.
Change-Id: If7487b23c55f2a996e411b25b76a8e1651f25d8b
Reviewed-on: https://boringssl-review.googlesource.com/2601
Reviewed-by: Adam Langley <agl@google.com>
This change has no semantic effect (I hope!). It's just a reformatting
of a few files in ssl/. This is just a start – the other files in ssl/
should follow in the coming days.
Change-Id: I5eb3f4b18d0d46349d0f94d3fe5ab2003db5364e
David is heading out so I didn't want to block the previous batch of
changes for weeks. Thus I landed them as-is and this change tweaks a
couple of things that would normally have been addressed in code-review.
Change-Id: I2579dbc43d93fea34a52b4041f5511d70217aaf7
Ensure that both the client and the server emit a protocol_version alert
(except in SSLv3 where it doesn't exist) with a record-layer version which the
peer will recognize.
Change-Id: I31650a64fe9b027ff3d51e303711910a00b43d6f
This makes SSLv23_method go through DTLS_ANY_VERSION's version negotiation
logic. This allows us to get rid of duplicate ClientHello logic. For
compatibility, SSL_METHOD is now split into SSL_PROTOCOL_METHOD and a version.
The legacy version-locked methods set min_version and max_version based this
version field to emulate the original semantics.
As a bonus, we can now handle fragmented ClientHello versions now.
Because SSLv23_method is a silly name, deprecate that too and introduce
TLS_method.
Change-Id: I8b3df2b427ae34c44ecf972f466ad64dc3dbb171
Tested manually by replacing SSLv23_method() with TLSv1_2_method() in
bssl_shim. This is a large chunk of code which is not run in SSLv23_method(),
but it will be run after unification. It's split out separately to ease review.
Change-Id: I6bd241daca17aa0f9b3e36e51864a29755a41097
Match the DTLS code. Rather than sniffing the handshake state, use the
have_version bit.
Change-Id: I40e92f187647417c34b4cfdc3ad258f5562e781b
Reviewed-on: https://boringssl-review.googlesource.com/2588
Reviewed-by: Adam Langley <agl@google.com>
These tests use both APIs. This also modifies the inline version negotiation's
error codes (currently only used for DTLS) to align with SSLv23's error codes.
Note: the peer should send a protocol_version alert which is currently untested
because it's broken.
Upstream would send such an alert if TLS 1.0 was supported but not otherwise,
which is somewhat bizarre. We've actually regressed and never send the alert in
SSLv23. When version negotiation is unified, we'll get the alerts back.
Change-Id: I4c77bcef3a3cd54a039a642f189785cd34387410
Reviewed-on: https://boringssl-review.googlesource.com/2584
Reviewed-by: Adam Langley <agl@google.com>
Amend the version negotiation tests to test this new spelling of max_version.
min_version will be tested in a follow-up.
Change-Id: Ic4bfcd43bc4e5f951140966f64bb5fd3e2472b01
Reviewed-on: https://boringssl-review.googlesource.com/2583
Reviewed-by: Adam Langley <agl@google.com>
DTLS_method() can now negotiate versions without switching methods.
Change-Id: I0655b3221b6e7e4b3ed4acc45f1f41c594447021
Reviewed-on: https://boringssl-review.googlesource.com/2582
Reviewed-by: Adam Langley <agl@google.com>
SSL3_ENC_METHOD will remain version-specific while SSL_METHOD will become
protocol-specific. This finally removes all the version-specific portions of
SSL_METHOD but the version tag itself.
(SSL3_ENC_METHOD's version-specific bits themselves can probably be handled by
tracking a canonicalized protocol version. It would simplify version
comparisons anyway. The one catch is SSLv3 has a very different table. But
that's a cleanup for future. Then again, perhaps a version-specific method
table swap somewhere will be useful later for TLS 1.3.)
Much of this commit was generated with sed invocation:
s/method->ssl3_enc/enc_method/g
Change-Id: I2b192507876aadd4f9310240687e562e56e6c0b1
Reviewed-on: https://boringssl-review.googlesource.com/2581
Reviewed-by: Adam Langley <agl@google.com>
Now SSLv23 and DTLS_ANY_VERSION share version-related helper functions.
ssl3_get_method is temporary until the method switch is no longer necessary.
Put them all together so there's one place to refactor them when we add a new
version or implement min_version/max_version controls.
Change-Id: Ic28a145cad22db08a87fdb854480b22886c451c6
Reviewed-on: https://boringssl-review.googlesource.com/2580
Reviewed-by: Adam Langley <agl@google.com>
Missed this one. It requires that we be able to change an SSL_METHOD after the
after, which complicates compiling the version locking into min_version /
max_version configurations.
Change-Id: I24ba54b7939360bbfafe3feb355a65840bda7611
Reviewed-on: https://boringssl-review.googlesource.com/2579
Reviewed-by: Adam Langley <agl@google.com>
SSL_ST_BEFORE isn't a possible state anymore. It seems this state meant the
side wasn't known, back in the early SSLeay days. Now upstream guesses
(sometimes incorrectly with generic methods), and we don't initialize until
later. SSL_shutdown also doesn't bother to call ssl3_shutdown at all if the
side isn't initialized and SSL_ST_BEFORE isn't the uninitialized state, which
seems a much more sensible arrangement.
Likewise, because bare SSL_ST_BEFOREs no longer exist, SSL_in_init implies
SSL_in_before and there is no need to check both.
Change-Id: Ie680838b2f860b895073dabb4d759996e21c2824
Reviewed-on: https://boringssl-review.googlesource.com/2564
Reviewed-by: Adam Langley <agl@google.com>
These may as well be replaced with assertions. Get them out of the way of the
initialization.
Change-Id: Ie4ab8bdc018e4a1def7d3f6b3b172a77896bfc0a
Reviewed-on: https://boringssl-review.googlesource.com/2563
Reviewed-by: Adam Langley <agl@google.com>
They're always known now. Also fix the SSLv23_{client,server}_method
definitions still had their own macro invocations.
Change-Id: Ia13f29a27f2331d25a4051e83f2d5abc62fab981
Reviewed-on: https://boringssl-review.googlesource.com/2562
Reviewed-by: Adam Langley <agl@google.com>
There's an undefined one not used anywhere. The others ought to be const. Also
move the forward declaration to ssl.h so we don't have to use the struct name.
Change-Id: I76684cf65255535c677ec19154cac74317c289ba
Reviewed-on: https://boringssl-review.googlesource.com/2561
Reviewed-by: Adam Langley <agl@google.com>
else block got lost in a rewrite of this code.
Change-Id: I51f1655474ec8bbd4eccb4297124e8584329444e
Reviewed-on: https://boringssl-review.googlesource.com/2560
Reviewed-by: Adam Langley <agl@google.com>
The client_version needs to be preserved, both for the RSA key exchange and
(when this codepath is used for TLS) for the SChannel renego workaround. Fix
the tests to enforce this so the cipher suite version tests catch this.
Change-Id: I0c42dc3ec4830f3724026b400e5066e7a7f1ee97
Reviewed-on: https://boringssl-review.googlesource.com/2551
Reviewed-by: Adam Langley <agl@google.com>
There's no need to make that conditional.
Change-Id: Idac1aba42b22e3fe8e7731ae4ecb5ebc4183336c
Reviewed-on: https://boringssl-review.googlesource.com/2550
Reviewed-by: Adam Langley <agl@google.com>
Comparing data is a much easier idiom than CBS_skip + a CBS_len check.
Change-Id: I3efe925734c76f3494cad682445291ae83750a7e
Reviewed-on: https://boringssl-review.googlesource.com/2500
Reviewed-by: Adam Langley <agl@google.com>
The record-layer version of the ServerHello should match the final version. The
record-layer version of the ClientHello should be the advertised version, but
clamped at TLS 1.0. This is to ensure future rewrites do not regress this.
Change-Id: I96f1f0674944997ff38b562453a322ce61652635
Reviewed-on: https://boringssl-review.googlesource.com/2540
Reviewed-by: Adam Langley <agl@google.com>
Bruce Dawson pointed out that the shadowing of |ret| in |s3_srvr.c|
looked dodgy. It was actually deliberate (we don't want to reset the
default value of the function's |ret| variable with a successful return
from the callback) but it does look dodgy.
This change adds -Wshadow to ban variable shadowing and fixes all
current instances.
Change-Id: I1268f88b9f26245c7d16d6ead5bb9014ea471c01
Reviewed-on: https://boringssl-review.googlesource.com/2520
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 7a04b854d655785798d471df25ffd5036f3cc46b.)
This does not affect BoringSSL as ssl3_get_client_hello advances to yet another
state immediately after reading the message. But the state advance is correct.
It matches the normal exit for this function.
Change-Id: I8a664f2ad5f80beacbaf3e17a7786a5c9e8ef30e
Reviewed-on: https://boringssl-review.googlesource.com/2480
Reviewed-by: Piotr Sikora <piotr@cloudflare.com>
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 4b87706d20f0a2fdf2e8f1b90256e141c487ef47 and
eceef8fb865eb5de329b27ea472d4fdea4c290fe.)
Dead code.
Change-Id: I58120c3a9c42cb9db27f404774778222c3bb642a
Reviewed-on: https://boringssl-review.googlesource.com/2479
Reviewed-by: Adam Langley <agl@google.com>
PR#1767
(Imported from upstream's fe78f08d1541211566a5656395186bfbdc61b6f8)
Not sure this is reachable (upstream's PR references custom engines), but
better be tidy. Note this is slightly different from upstream's: EVP_Cipher is
documented to return -1 on failure, not 0.
Change-Id: I836f12b73c6912a8ae8cbd37cfd3d33466acbc9e
Reviewed-on: https://boringssl-review.googlesource.com/2478
Reviewed-by: Adam Langley <agl@google.com>
PR#3613
(Imported from upstream's fc3968a25ce0c16cab8730ec0d68a59856158029)
We don't care about GOST, but removing redundant code is reasonable. Also
switch that CRYPTO_add to EVP_PKEY_dup. Missed a spot.
Change-Id: I768ec546d987fb3d8bc3decf7ebf1a5590fbb6c2
Reviewed-on: https://boringssl-review.googlesource.com/2477
Reviewed-by: Adam Langley <agl@google.com>
Some code predated the RFCs themselves, but the RFCs now exist. Also remove
now obsolete comments and some unused #defines.
See upstream's cffeacd91e70712c99c431bf32a655fa1b561482. (Though this predates
it; I just remembered I never uploaded it.)
Change-Id: I5e56f0ab6b7f558820f72e84dfdbc71a8c23cb91
Reviewed-on: https://boringssl-review.googlesource.com/2475
Reviewed-by: Adam Langley <agl@google.com>
The ClientHello record is padded to 1024 bytes when
fastradio_padding is enabled. As a result, the 3G cellular radio
is fast forwarded to DCH (high data rate) state. This mechanism
leads to a substantial redunction in terms of TLS handshake
latency, and benefits mobile apps that are running on top of TLS.
Change-Id: I3d55197b6d601761c94c0f22871774b5a3dad614
MSVC doesn't like it when you compare the two.
Change-Id: I03c5ff2e2668ac2e536de8278e3a7c98a3dfd117
Reviewed-on: https://boringssl-review.googlesource.com/2460
Reviewed-by: Adam Langley <agl@google.com>
It just inserts extra flushes everywhere and isn't used.
Change-Id: I082e4bada405611f4986ba852dd5575265854036
Reviewed-on: https://boringssl-review.googlesource.com/2456
Reviewed-by: Adam Langley <agl@google.com>
Use it in ssl3_cert_verify_hash so signing a pre-TLS-1.2 handshake hash can go
through RSA_sign and be intercepted via RSA_METHOD appropriately. This avoids
Windows needing to intercept sign_raw. (CAPI keys cannot provide sign_raw,
unless the input size happens to be that of NID_md5_sha1.)
Also use it in processing ServerKeyExchange to avoid special-casing RSA.
BUG=crbug.com/437023
Change-Id: Ia07433f468b75fdf7bfc8fa90c9751639b2478e6
Reviewed-on: https://boringssl-review.googlesource.com/2420
Reviewed-by: David Benjamin <davidben@google.com>
Don't link with dl, except on Linux where we have malloc tests.
Change-Id: I7b23acc854172e64628a55acecfaa9a661f74f77
Reviewed-on: https://boringssl-review.googlesource.com/2453
Reviewed-by: Adam Langley <agl@google.com>
The comment has it right, but the rewritten code was wrong.
Change-Id: I450193c39fb62eae32aae090a3834dd83db53421
Reviewed-on: https://boringssl-review.googlesource.com/2444
Reviewed-by: Adam Langley <agl@google.com>
Replace the comment with a clearer one and reimplement it much more tidily. The
mask thing was more complicated than was needed.
This slightly changes behavior on the DTLS_ANY_VERSION side in that, if only
one method is enabled, we no longer short-circuit to the version-locked method
early. This "optimization" seems unnecessary.
Change-Id: I571c8b60ed16bd4357c67d65df0dd1ef9cc5eb57
Reviewed-on: https://boringssl-review.googlesource.com/2451
Reviewed-by: Adam Langley <agl@google.com>
It should be set correctly prior to entering the handshake. Don't mask bugs by
assigning it.
Change-Id: Ib9bca8fad68916b3b242aad8819e3760e59e777a
Reviewed-on: https://boringssl-review.googlesource.com/2443
Reviewed-by: Adam Langley <agl@google.com>
first_packet is a temporary connection-global flag set for the duration of some
call and then queried from other code. This kind of logic is too difficult to
reason through. It also incorrectly treats renegotiate ClientHellos as
pre-version-negotiation records. This eliminates the need to query
enc_write_ctx (which wasn't EVP_AEAD-aware anyway).
Instead, take a leaf from Go TLS's book and add a have_version bit. This is
placed on s->s3 as it is connection state; s->s3 automatically gets reset on
SSL_clear while s doesn't.
This new flag will also be used to determine whether to do the V2ClientHello
sniff when the version-locked methods merge into SSLv23_method. It will also
replace needing to condition s->method against a dummy DTLS_ANY_VERSION value
to determine whether DTLS version negotiation has happened yet.
Change-Id: I5c8bc6258b182ba4ab175a48a84eab6d3a001333
Reviewed-on: https://boringssl-review.googlesource.com/2442
Reviewed-by: Adam Langley <agl@google.com>
Supporting both schemes seems pointless. Now that s->server and s->state are
set appropriately late and get_ssl_method is gone, the only difference is that
the client/server ones have non-functional ssl_accept or ssl_connect hooks. We
can't lose the generic ones, so let's unify on that.
Note: this means a static linker will no longer drop the client or server
handshake code if unused by a consumer linking statically. However, Chromium
needs the server half anyway for DTLS and WebRTC, so that's probably a lost
cause. Android also exposes server APIs.
Change-Id: I290f5fb4ed558f59fadb5d1f84e9d9c405004c23
Reviewed-on: https://boringssl-review.googlesource.com/2440
Reviewed-by: Adam Langley <agl@google.com>
If SSL_clear is called before SSL_set_{connect,accept}_state (as SSL_new does
internally), s->state will get set prematurely. Likewise, s->server is set
based on the method's ssl_accept hook, but client SSL's may be initialized from
a generic SSL_METHOD too.
Since we can't easily get rid of the generic SSL_METHODs, defer s->state and
s->server initialization until the side is known.
Change-Id: I0972e17083df22a3c09f6f087011b54c699a22e7
Reviewed-on: https://boringssl-review.googlesource.com/2439
Reviewed-by: Adam Langley <agl@google.com>
It's unused. Also per the previous commit message, it historically had a bug
anyway.
Change-Id: I5868641e7938ddebbc0ffd72d218c81cd17c7739
Reviewed-on: https://boringssl-review.googlesource.com/2437
Reviewed-by: Adam Langley <agl@google.com>
If the state is SSL_ST_BEFORE, the SSL* was just initialized. Otherwise, we
don't want to call SSL_clear. The one case I found where we do is if a
handshake message is received and someone sets
SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS. This is apparently intended for external
consumers to set, but I see no code in Google that does.
Which is fortunate because it'll trigger SSL_clear. This retains the BIOs but
drops all connection state, including the record. If the client just initiated
renego, that's the ClientHello that's lost. The connection then hangs: the now
reset SSL* wants a ClientHello (under the null cipher because that too's been
dropped) while the peer wants an encrypted ServerHello.
Change-Id: Iddb3e0bb86d39d98155b060f9273a0856f2d1409
Reviewed-on: https://boringssl-review.googlesource.com/2436
Reviewed-by: Adam Langley <agl@google.com>
SSL_clear sets s->state and dtls1_clear sets cookie_len on the server. Setting
cookie_len on the server seems to serve no purpose but to let the callback know
how large the buffer is. This can be done just before calling the callback.
It also avoids a bug where the cookie check can be bypassed, should the server
not specify an app_verify_cookie_cb, by supplying a cookie of all zeros of the
maximum size. (Zero is fine because an empty cookie is rejected.)
The goal here is to avoid needing the SSL_clear calls in the handshake
functions. They are currently needed to fix the cookie_len setting when using
the generic method. (They get set wrong and then flipped back.)
Change-Id: I5095891bc0f7df62d83a9c84312fcf0b84826faa
Reviewed-on: https://boringssl-review.googlesource.com/2435
Reviewed-by: Adam Langley <agl@google.com>
s->server's value isn't final until SSL_connect or SSL_accept is called when
using the generic SSLv23_method or DTLS_method rather than the version-locked
ones. This makes the tests pass if bssl_shim uses those methods.
It would be nicer if the generic methods were gone and an SSL* could know from
creation which half it's destined for. Unfortunately, there's a lot of code
that uses those generic methods, so we probably can't get rid of them. If they
have to stay, it seems better to standardize on only having those, rather than
support both, even if standardizing on the side-specific ones would be
preferable.
Change-Id: I40e65a8842cd6706da92263a263f664336a7f3b3
Reviewed-on: https://boringssl-review.googlesource.com/2434
Reviewed-by: Adam Langley <agl@google.com>
SSL_ST_BEFORE is never standalone. As of upstream's
413c4f45ed0508d2242638696b7665f499d68265, SSL_ST_BEFORE is only ever set paired
with SSL_ST_ACCEPT or SSL_ST_CONNECT.
Conversely, SSL_ST_OK is never paired with SSL_ST_ACCEPT or SSL_ST_CONNECT. As
far as I can tell, this combination has never been possible.
Change-Id: Ifbc8f147be821026cf59f3d5038f0dbad3b0a1d2
Reviewed-on: https://boringssl-review.googlesource.com/2433
Reviewed-by: Adam Langley <agl@google.com>
It should already be assigned, as of upstream's
b31b04d951e9b65bde29657e1ae057b76f0f0a73. I believe these assignments are part
of the reason it used to appear to work. Replace them with assertions. So the
assertions are actually valid, check in SSL_connect / SSL_accept that they are
never called if the socket had been placed in the opposite state. (Or we'd be
in another place where it would have appeared to work with the handshake
functions fixing things afterwards.)
Now the only places handshake_func is set are in SSL_set_{connect,accept}_state
and the method switches.
Change-Id: Ib249212bf4aa889b94c35965a62ca06bdbcf52e1
Reviewed-on: https://boringssl-review.googlesource.com/2432
Reviewed-by: Adam Langley <agl@google.com>
This comment is no longer true. It dates from OpenSSL's initial commit, but
stopped being true in upstream's 413c4f45ed0508d2242638696b7665f499d68265.
Change-Id: I47377d992a00e3d57c795fef893e19e109dd6945
Reviewed-on: https://boringssl-review.googlesource.com/2431
Reviewed-by: Adam Langley <agl@google.com>
We intend to deprecate the version-locked methods and unify them. Don't expose
that there's a method swap. (The existing version-locked methods will merely be
a shorthand for configuring minimum/maximum versions.)
There is one consumer of SSL_get_ssl_method in internal code, but it's just
some logging in test-only code. All it's doing is getting the version as a
string which should be SSL_get_version instead.
While here, also remove dead ssl_bad_method function. Also the bogus
ssl_crock_st forward-declaration. The forward declaration in base.h should be
perfectly sufficient.
Change-Id: I50480808f51022e05b078a285f58ec85d5ad7c8e
Reviewed-on: https://boringssl-review.googlesource.com/2408
Reviewed-by: Adam Langley <agl@google.com>
When not offering to resume a session, the client populates s->session with a
fresh SSL_SESSION before the ServerHello is processed and, in DTLS_ANY_VERSION,
before the version is even determined. Don't create a fresh SSL_SESSION until
we know we are doing a full handshake.
This brings ssl3_send_client_hello closer to ssl23_client_hello in behavior. It
also fixes ssl_version in the client in DTLS_ANY_VERSION.
SSLv23_client_method is largely unchanged. If no session is offered, s->session
continues to be NULL until the ServerHello is received. The one difference is
that s->session isn't populated until the entire ServerHello is received,
rather than just the first half, in the case of a fragmented ServerHello. Apart
from info_callback, no external hooks get called between those points, so this
shouldn't expose new missing NULL checks.
The other client methods change significantly to match SSLv23_client_method's
behavior. For TLS, any exposed missing NULL checks are also in
SSLv23_client_method (and version-specific methods are already weird), so that
should be safe. For DTLS, I've verified that accesses in d1_*.c either handle
NULL or are after the ServerHello.
Change-Id: Idcae6bd242480e28a57dbba76ce67f1ac1ae1d1d
Reviewed-on: https://boringssl-review.googlesource.com/2404
Reviewed-by: Adam Langley <agl@google.com>
This fixes bugs that kept the tests from working:
- Resolve DTLS version and cookie before the session.
- In DTLS_ANY_VERSION, ServerHello should be read with first_packet = 1. This
is a regression from f2fedefdca. We'll want to
do the same for TLS, but first let's change this to a boolean has_version in a
follow-up.
Things not yet fixed:
- DTLS code is not EVP_AEAD-aware. Those ciphers are disabled for now.
- On the client, DTLS_ANY_VERSION creates SSL_SESSIONs with the wrong
ssl_version. The tests pass because we no longer enforce the match as of
e37216f56009fbf48c3a1e733b7a546ca6dfc2af. (In fact, we've gone from the server
ignoring ssl_version and client enforcing to the client mostly ignoring
ssl_version and the server enforcing.)
- ssl3_send_client_hello's ssl_version check checks for equality against
s->version rather than >.
Change-Id: I5a0dde221b2009413df9b9443882b9bf3b29519c
Reviewed-on: https://boringssl-review.googlesource.com/2403
Reviewed-by: Adam Langley <agl@google.com>
