The internal session cache is keyed on session ID, so this is completely
useless for clients (indeed we never look it up internally). Along the way,
tidy up ssl_update_cache to be more readable. The slight behavior change is
that SSL_CTX_add_session's return code no longer controls the external
callback. It's not clear to me what that could have accomplished. (It can only
fail on allocation error. We only call it for new sessions, so the duplicate
case is impossible.)
The one thing of value the internal cache might have provided is managing the
timeout. The SSL_CTX_flush_sessions logic would flip the not_resumable bit and
cause us not to offer expired sessions (modulo SSL_CTX_flush_sessions's delay
and any discrepancies between the two caches). Instead, just check expiration
when deciding whether or not to offer a session.
This way clients that set SSL_SESS_CACHE_CLIENT blindly don't accidentally
consume gobs of memory.
BUG=531194
Change-Id: If97485beab21874f37737edc44df24e61ce23705
Reviewed-on: https://boringssl-review.googlesource.com/6321
Reviewed-by: Adam Langley <alangley@gmail.com>
The original logic was rather confusing.
Change-Id: I097e57817ea8ec2dd65a413c8751fba1682e928b
Reviewed-on: https://boringssl-review.googlesource.com/6320
Reviewed-by: Adam Langley <alangley@gmail.com>
In doing so, fix the documentation for SSL_CTX_add_session and
SSL_CTX_remove_session. I misread the code and documented the behavior
on session ID collision wrong.
Change-Id: I6f364305e1f092b9eb0b1402962fd04577269d30
Reviewed-on: https://boringssl-review.googlesource.com/6319
Reviewed-by: Adam Langley <alangley@gmail.com>
A random 32-byte (so 256-bit) session ID is never going to collide with
an existing one. (And, if it does, SSL_CTX_add_session does account for
this, so the server won't explode. Just attempting to resume some
session will fail.)
That logic didn't completely work anyway as it didn't account for
external session caches or multiple connections picking the same ID in
parallel (generation and insertion happen at different times) or
multiple servers sharing one cache. In theory one could fix this by
passing in a sufficiently clever generate_session_id, but no one does
that.
I found no callers of these functions, so just remove them altogether.
Change-Id: I8500c592cf4676de6d7194d611b99e9e76f150a7
Reviewed-on: https://boringssl-review.googlesource.com/6318
Reviewed-by: Adam Langley <alangley@gmail.com>
This required switching anchors from <a name> to id attributes, which
also works. HTML gets unhappy when you nest <a> tags inside each other
and tagging the elements is somewhat tidier.
Change-Id: I64094d35a0e820e37be9e5dc8db013a50774190f
Reviewed-on: https://boringssl-review.googlesource.com/6314
Reviewed-by: Adam Langley <alangley@gmail.com>
Private structs shouldn't be shown. Also there's a few sections that are
really more implementation details than anything else.
Change-Id: Ibc5a23ba818ab0531d9c68e7ce348f1eabbcd19a
Reviewed-on: https://boringssl-review.googlesource.com/6313
Reviewed-by: Adam Langley <alangley@gmail.com>
Although Chromium actually uses SSL_(get_)state as part of its fallback
reason heuristic, that function really should go in the deprecated
bucket. I kept SSL_state_string_long since having a human-readable
string is probably useful for logging.
SSL_set_SSL_CTX was only half-documented as the behavior of this
function is very weird. This warrants further investigation and
rethinking.
SSL_set_shutdown is absurd. I added an assert to trip up clearing bits
and set it to a bitwise OR since clearing bits may mess up the state
machine. Otherwise there's enough consumers and it's not quite the same
as SSL_CTX_set_quiet_shutdown that I've left it alone for now.
Change-Id: Ie35850529373a5a795f6eb04222668ff76d84aaa
Reviewed-on: https://boringssl-review.googlesource.com/6312
Reviewed-by: Adam Langley <alangley@gmail.com>
It just calls CRYPTO_library_init and doesn't do anything else. If
anything, I'd like to make CRYPTO_library_init completely go away too.
We have CRYPTO_once now, so I think it's safe to assume that, if ssl/
ever grows initialization needs beyond that of crypto/, we can hide it
behind a CRYPTO_once and not burden callers.
Change-Id: I63dc362e0e9e98deec5516f4620d1672151a91b6
Reviewed-on: https://boringssl-review.googlesource.com/6311
Reviewed-by: Adam Langley <alangley@gmail.com>
SSL_in_connect_init and SSL_in_accept_init are removed as they're unused
both within the library and externally. They're also kind of silly.
Expand on how False Start works at the API level in doing so.
Change-Id: Id2a8e34b5bb8f28329e3b87b4c64d41be3f72410
Reviewed-on: https://boringssl-review.googlesource.com/6310
Reviewed-by: Adam Langley <alangley@gmail.com>
They were since added to crypto.h and implemented in the library proper.
Change-Id: Idaa2fe2d9b213e67cf7ef61ff8bfc636dfa1ef1f
Reviewed-on: https://boringssl-review.googlesource.com/6309
Reviewed-by: Adam Langley <alangley@gmail.com>
They're really not all that helpful, considering they're each used
exactly once. They're also confusing as it is ALMOST the case that
SSL_TXT_FOO expands to "FOO", but SSL_TXT_AES_GCM expand "AESGCM" and
the protocol versions have lowercase v's and dots.
Change-Id: If78ad8edb0c024819219f61675c60c2a7f3a36b0
Reviewed-on: https://boringssl-review.googlesource.com/6307
Reviewed-by: Adam Langley <alangley@gmail.com>
This callback is some combination of arguably useful stuff (bracket
handshakes, alerts) and completely insane things (find out when the
state machine advances). Deprecate the latter.
Change-Id: Ibea5b32cb360b767b0f45b302fd5f1fe17850593
Reviewed-on: https://boringssl-review.googlesource.com/6305
Reviewed-by: Adam Langley <alangley@gmail.com>
This compiled, so I guess everything we care about can do C++-style
comments, but better be uniform.
Change-Id: I9950c2df93cd81bb2bddb3a1e14e2de02c7e4807
Reviewed-on: https://boringssl-review.googlesource.com/6304
Reviewed-by: Adam Langley <alangley@gmail.com>
Also clean up the code slightly.
Change-Id: I066a389242c46cdc7d41b1ae9537c4b7716c92a2
Reviewed-on: https://boringssl-review.googlesource.com/6302
Reviewed-by: Adam Langley <alangley@gmail.com>
Like tls1.h, ssl3.h is now just a bundle of protocol constants.
Hopefully we can opaquify this struct in due time, but for now it's
still public.
Change-Id: I68366eb233702e149c92e21297f70f8a4a45f060
Reviewed-on: https://boringssl-review.googlesource.com/6300
Reviewed-by: Adam Langley <alangley@gmail.com>
This dates all the way to SSLeay 0.9.0b. At this point the
application/handshake interleave logic in ssl3_read_bytes was already
present:
((
(s->state & SSL_ST_CONNECT) &&
(s->state >= SSL3_ST_CW_CLNT_HELLO_A) &&
(s->state <= SSL3_ST_CR_SRVR_HELLO_A)
) || (
(s->state & SSL_ST_ACCEPT) &&
(s->state <= SSL3_ST_SW_HELLO_REQ_A) &&
(s->state >= SSL3_ST_SR_CLNT_HELLO_A)
)
The comment is attached to SSL3_ST_SR_CLNT_HELLO_A, so I suspect this is
what it was about. This logic is gone now, so let's remove that scary
warning.
Change-Id: I45f13b53b79e35d80e6074b0942600434deb0684
Reviewed-on: https://boringssl-review.googlesource.com/6299
Reviewed-by: Adam Langley <alangley@gmail.com>
Don't mark a certificate as self-signed if keyUsage is present and
certificate signing is not asserted.
PR#3979
(Imported from upstream's e272f8ef8f63298466494adcd29512797ab1eece.)
Change-Id: I3120832f32455e8e099708fa2491d85d3d4a3930
Reviewed-on: https://boringssl-review.googlesource.com/6341
Reviewed-by: Adam Langley <alangley@gmail.com>
RFC 5077 explicitly allows the server to change its mind and send no
ticket by sending an empty NewSessionTicket. See also upstream's
21b538d616.
CBS_stow handles this case somewhat, so we won't get confused about
malloc(0) as upstream did. But we'll still fill in a bogus SHA-256
session ID, cache the session, and send a ClientHello with bogus session
ID but empty ticket extension. (The session ID field changes meaning
significantly when the ticket is or isn't empty. Non-empty means "ignore
the session ID, but echo if it resuming" while empty means "I support
tickets, but am offering this session ID".
The other behavior change is that a server which changes its mind on a
resumption handshake will no longer override the client's session cache
with a ticket-less session.
(This is kind of silly. Given that we don't get completely confused due
to CBS_stow, it might not be worth bothering with the rest. Mostly it
bugged me that we send an indicator session ID with no ticket.)
Change-Id: Id6b5bde1fe51aa3e1f453a948e59bfd1e2502db6
Reviewed-on: https://boringssl-review.googlesource.com/6340
Reviewed-by: Adam Langley <alangley@gmail.com>
Now tls1.h is just a pile of protocol constants with no more circular
dependency problem.
I've preserved SSL_get_servername's behavior where it's simultaneously a
lookup of handshake state and local configuration. I've removed it from
SSL_get_servername_type. It got the logic wrong anyway with the order of
the s->session check.
(Searching through code, neither is used on the client, but the
SSL_get_servername one is easy.)
Change-Id: I61bb8fb0858b07d76a7835bffa6dc793812fb027
Reviewed-on: https://boringssl-review.googlesource.com/6298
Reviewed-by: Adam Langley <alangley@gmail.com>
Some ARM environments don't support |getauxval| or signals and need to
configure the capabilities of the chip at compile time. This change adds
defines that allow them to do so.
Change-Id: I4e6987f69dd13444029bc7ac7ed4dbf8fb1faa76
Reviewed-on: https://boringssl-review.googlesource.com/6280
Reviewed-by: Adam Langley <agl@google.com>
SSL_alert_desc_string_long was kept in the undeprecated bucket and one missing
alert was added. We have some uses and it's not completely ridiculous for
logging purposes.
The two-character one is ridiculous though and gets turned into a stub
that returns a constant string ("!" or "!!") because M2Crypto expects
it.
Change-Id: Iaf8794b5d953630216278536236c7113655180af
Reviewed-on: https://boringssl-review.googlesource.com/6297
Reviewed-by: Adam Langley <alangley@gmail.com>
(Documentation/deprecation will come in later commits.)
Change-Id: I3aba26e32b2e47a1afb5cedd44d09115fc193bce
Reviewed-on: https://boringssl-review.googlesource.com/6296
Reviewed-by: Adam Langley <alangley@gmail.com>
The only reason you'd want it is to tls_unique, and we have a better API
for that. (It has one caller and that is indeed what that caller uses it
for.)
Change-Id: I39f8e353f56f18becb63dd6f7205ad31f4192bfd
Reviewed-on: https://boringssl-review.googlesource.com/6295
Reviewed-by: Adam Langley <alangley@gmail.com>
This is redundant with SSL_get_error. Neither is very good API, but
SSL_get_error is more common. SSL_get_error also takes a return code
which makes it harder to accidentally call it at some a point other than
immediately after an operation. (Any other point is confusing since you
can have SSL_read and SSL_write operations going on in parallel and
they'll get mixed up.)
Change-Id: I5818527c30daac28edb552c6c550c05c8580292d
Reviewed-on: https://boringssl-review.googlesource.com/6294
Reviewed-by: Adam Langley <alangley@gmail.com>
It's pretty clearly pointless to put in the public header.
Change-Id: I9527aba09b618f957618e653c4f2ae379ddd0fdb
Reviewed-on: https://boringssl-review.googlesource.com/6293
Reviewed-by: Adam Langley <alangley@gmail.com>
Also added a SSL_CTX_set_select_certificate_cb setter for
select_certificate_cb so code needn't access SSL_CTX directly. Plus it
serves as a convenient anchor for the documentation.
Change-Id: I23755b910e1d77d4bea7bb9103961181dd3c5efe
Reviewed-on: https://boringssl-review.googlesource.com/6291
Reviewed-by: Adam Langley <alangley@gmail.com>
These are theh two remaining quirks (SSL_OP_LEGACY_SERVER_CONNECT
aside). Add counters so we can determine whether there are still clients
that trip up these cases.
Change-Id: I7e92f42f3830c1df675445ec15a852e5659eb499
Reviewed-on: https://boringssl-review.googlesource.com/6290
Reviewed-by: Adam Langley <alangley@gmail.com>
In doing so, simplify the mess around serializing the public key.
Channel ID specifies that you write x and y concatenated. Rather than
using the X9.62 serialization and chopping bits off, get the affine
coordinates and write them out in the same way we write r and s.
Also unify the P-256 sanity check around SSL_set1_tls_channel_id and
actually check the curve NID.
BUG=468889
Change-Id: I228877b736c9722e368d315064ce3ae6893adfc0
Reviewed-on: https://boringssl-review.googlesource.com/6201
Reviewed-by: Adam Langley <alangley@gmail.com>
Start converting the ones we can right now. Some of the messier ones
resize init_buf rather than assume the initial size is sufficient, so
those will probably wait until init_buf is gone and the handshake's
undergone some more invasive surgery. The async ones will also require
some thought. But some can be incrementally converted now.
BUG=468889
Change-Id: I0bc22e4dca37d9d671a488c42eba864c51933638
Reviewed-on: https://boringssl-review.googlesource.com/6190
Reviewed-by: Adam Langley <alangley@gmail.com>
This extends 79c59a30 to |RSA_public_encrypt|, |RSA_private_encrypt|,
and |RSA_public_decrypt|. It benefits Conscrypt, which expects these
functions to have the same signature as |RSA_public_private_decrypt|.
Change-Id: Id1ce3118e8f20a9f43fd4f7bfc478c72a0c64e4b
Reviewed-on: https://boringssl-review.googlesource.com/6286
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
It's missing fields and no one ever calls it.
Change-Id: I450edc1e29bb48edffb5fd3df8da19a03e4185ce
Reviewed-on: https://boringssl-review.googlesource.com/5821
Reviewed-by: Adam Langley <agl@google.com>
The warning is:
C4210: nonstandard extension used : function given file scope.
It is caused by function declarations that aren't at the top level in a
file.
Change-Id: Ib1c2ae64e15e66eb0a7255a29c0e560fbf55c2b2
Reviewed-on: https://boringssl-review.googlesource.com/6210
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Grouping along two axes is weird. Doesn't hugely matter which one, but
we should be consistent.
Change-Id: I80fb04d3eff739c08fda29515ce81d101d8542cb
Reviewed-on: https://boringssl-review.googlesource.com/6120
Reviewed-by: Adam Langley <agl@google.com>
The caller obligations for retransmit are messy, so I've peppered a few
other functions with mentions of it. There's only three functions, so
they're lumped in with the other core functions. They're irrelevant for
TLS, but important for DTLS.
Change-Id: Ifc995390952eef81370b58276915dcbe4fc7e3b5
Reviewed-on: https://boringssl-review.googlesource.com/6093
Reviewed-by: Adam Langley <agl@google.com>
Deprecate the client_cert_cb variant since you can't really configure
intermediates with it. (You might be able to by configuring the
intermediates without the leaf or key and leaving the SSL stack to
configure those, but that's really weird. cert_cb is simpler.)
Also document the two functions the callbacks may use to query the
CertificateRequest on the client.
Change-Id: Iad6076266fd798cd74ea4e09978e7f5df5c8a670
Reviewed-on: https://boringssl-review.googlesource.com/6092
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Adam Langley
Matt Braithwaite
Eric Roman
Brian Smith