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>
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>
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>
RSA_PSK is really weird in that it takes a Certificate, but you're not
expected to verify it. It's just a funny way to transmit an RSA key.
(They probably should have used the RSA_EXPORT ServerKeyExchange
spelling.) Some code now already doesn't account for it right around
certificate verification.
Given ECDHE_PSK exists, hopefully there will never be any need to add
this.
Change-Id: Ia64dac28099eaa9021f8d915d45ccbfd62872317
Reviewed-on: https://boringssl-review.googlesource.com/5941
Reviewed-by: Adam Langley <agl@google.com>
Allow configuring digest preferences for the private key. Some
smartcards have limited support for signing digests, notably Windows
CAPI keys and old Estonian smartcards. Chromium used the supports_digest
hook in SSL_PRIVATE_KEY_METHOD to limit such keys to SHA1. However,
detecting those keys was a heuristic, so some SHA256-capable keys
authenticating to SHA256-only servers regressed in the switch to
BoringSSL. Replace this mechanism with an API to configure digest
preference order. This way heuristically-detected SHA1-only keys may be
configured by Chromium as SHA1-preferring rather than SHA1-requiring.
In doing so, clean up the shared_sigalgs machinery somewhat.
BUG=468076
Change-Id: I996a2df213ae4d8b4062f0ab85b15262ca26f3c6
Reviewed-on: https://boringssl-review.googlesource.com/5755
Reviewed-by: Adam Langley <agl@google.com>
EVP_MD_CTX_copy_ex was implemented with a memcpy, which doesn't work well when
some of the pointers need to be copied, and ssl_verify_cert_chain didn't
account for set_ex_data failing.
Change-Id: Ieb556aeda6ab2e4c810f27012fefb1e65f860023
Reviewed-on: https://boringssl-review.googlesource.com/5911
Reviewed-by: Adam Langley <agl@google.com>
Applications may require the stapled OCSP response in order to verify
the certificate within the verification callback.
Change-Id: I8002e527f90c3ce7b6a66e3203c0a68371aac5ec
Reviewed-on: https://boringssl-review.googlesource.com/5730
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Move cert_chain to the SSL_SESSION. Now everything on an SSL_SESSION is
properly serialized. The cert_chain field is, unfortunately, messed up
since it means different things between client and server.
There exists code which calls SSL_get_peer_cert_chain as both client and
server and assumes the existing semantics for each. Since that function
doesn't return a newly-allocated STACK_OF(X509), normalizing between the
two formats is a nuisance (we'd either need to store both cert_chain and
cert_chain_full on the SSL_SESSION or create one of the two variants
on-demand and stash it into the SSL).
This CL does not resolve this and retains the client/server difference
in SSL_SESSION. The SSL_SESSION serialization is a little inefficient
(two copies of the leaf certificate) for a client, but clients don't
typically serialize sessions. Should we wish to resolve it in the
future, we can use a different tag number. Because this was historically
unserialized, existing code must already allow for cert_chain not being
preserved across i2d/d2i.
In keeping with the semantics of retain_only_sha256_of_client_certs,
cert_chain is not retained when that flag is set.
Change-Id: Ieb72fc62c3076dd59750219e550902f1ad039651
Reviewed-on: https://boringssl-review.googlesource.com/5759
Reviewed-by: Adam Langley <agl@google.com>
It's completely redundant with the copy in the SSL_SESSION except it
isn't serialized.
Change-Id: I1d95a14cae064c599e4bab576df1dd156da4b81c
Reviewed-on: https://boringssl-review.googlesource.com/5757
Reviewed-by: Adam Langley <agl@google.com>
Gets another field out of the SSL_SESSION.
Change-Id: I9a27255533f8e43e152808427466ec1306cfcc60
Reviewed-on: https://boringssl-review.googlesource.com/5756
Reviewed-by: Adam Langley <agl@google.com>
This begins decoupling the transport from the SSL state machine. The buffering
logic is hidden behind an opaque API. Fields like ssl->packet and
ssl->packet_length are gone.
ssl3_get_record and dtls1_get_record now call low-level tls_open_record and
dtls_open_record functions that unpack a single record independent of who owns
the buffer. Both may be called in-place. This removes ssl->rstate which was
redundant with the buffer length.
Future work will push the buffer up the stack until it is above the handshake.
Then we can expose SSL_open and SSL_seal APIs which act like *_open_record but
return a slightly larger enum due to other events being possible. Likewise the
handshake state machine will be detached from its buffer. The existing
SSL_read, SSL_write, etc., APIs will be implemented on top of SSL_open, etc.,
combined with ssl_read_buffer_* and ssl_write_buffer_*. (Which is why
ssl_read_buffer_extend still tries to abstract between TLS's and DTLS's fairly
different needs.)
The new buffering logic does not support read-ahead (removed previously) since
it lacks a memmove on ssl_read_buffer_discard for TLS, but this could be added
if desired. The old buffering logic wasn't quite right anyway; it tried to
avoid the memmove in some cases and could get stuck too far into the buffer and
not accept records. (The only time the memmove is optional is in DTLS or if
enough of the record header is available to know that the entire next record
would fit in the buffer.)
The new logic also now actually decrypts the ciphertext in-place again, rather
than almost in-place when there's an explicit nonce/IV. (That accidentally
switched in https://boringssl-review.googlesource.com/#/c/4792/; see
3d59e04bce96474099ba76786a2337e99ae14505.)
BUG=468889
Change-Id: I403c1626253c46897f47c7ae93aeab1064b767b2
Reviewed-on: https://boringssl-review.googlesource.com/5715
Reviewed-by: Adam Langley <agl@google.com>
This is a simpler implementation than OpenSSL's, lacking responder IDs
and request extensions support. This mirrors the client implementation
already present.
Change-Id: I54592b60e0a708bfb003d491c9250401403c9e69
Reviewed-on: https://boringssl-review.googlesource.com/5700
Reviewed-by: Adam Langley <agl@google.com>
They're not called (new in 1.0.2). We actually may well need to
configure these later to strike ECDSA from the list on Chrome/XP
depending on what TLS 1.3 does, but for now striking it from the cipher
suite list is both necessary and sufficient. I think we're better off
removing these for now and adding new APIs later if we need them.
(This API is weird. You pass in an array of NIDs that must be even
length and alternating between hash and signature NID. We'd also need a
way to query the configured set of sigalgs to filter away. Those used to
exist but were removed in
https://boringssl-review.googlesource.com/#/c/5347/. SSL_get_sigalgs is
an even uglier API and doesn't act on the SSL_CTX.)
And with that, SSL_ctrl and SSL_CTX_ctrl can *finally* be dropped. Don't
leave no-op wrappers; anything calling SSL_ctrl and SSL_CTX_ctrl should
instead switch to the wrapper macros.
BUG=404754
Change-Id: I5d465cd27eef30d108eeb6de075330c9ef5c05e8
Reviewed-on: https://boringssl-review.googlesource.com/5675
Reviewed-by: Adam Langley <agl@google.com>
This change stores the size of the group/modulus (for RSA/DHE) or curve
ID (for ECDHE) in the |SSL_SESSION|. This makes it available for UIs
where desired.
Change-Id: I354141da432a08f71704c9683f298b361362483d
Reviewed-on: https://boringssl-review.googlesource.com/5280
Reviewed-by: Adam Langley <agl@google.com>
This isn't called and, with the fixed-DH client cert types removed, is
only useful if a server wishes to not accept ECDSA certificates or
something.
BUG=404754
Change-Id: I21d8e1a71aedf446ce974fbeadc62f311ae086db
Reviewed-on: https://boringssl-review.googlesource.com/5673
Reviewed-by: Adam Langley <agl@google.com>
These are unused (new as of 1.0.2). Although being able to separate the
two stores is a reasonable thing to do, we hope to remove the
auto-chaining feature eventually. Given that, SSL_CTX_set_cert_store
should suffice. This gets rid of two more ctrl macros.
BUG=404754,486295
Change-Id: Id84de95d7b2ad5a14fc68a62bb2394f01fa67bb4
Reviewed-on: https://boringssl-review.googlesource.com/5672
Reviewed-by: Adam Langley <agl@google.com>
Rather than support arbitrarily many handshake hashes in the general
case (which the PRF logic assumes is capped at two), special-case the
MD5/SHA1 two-hash combination and otherwise maintain a single rolling
hash.
Change-Id: Ide9475565b158f6839bb10b8b22f324f89399f92
Reviewed-on: https://boringssl-review.googlesource.com/5618
Reviewed-by: Adam Langley <agl@google.com>
Rather than iterate over handshake_dgsts itself, it can just call
tls1_handshake_digest.
Change-Id: Ia518da540e47e65b13367eb1af184c0885908488
Reviewed-on: https://boringssl-review.googlesource.com/5617
Reviewed-by: Adam Langley <agl@google.com>
The handshake hash is initialized from the buffer as soon as the cipher
is known. When adding a message to the transcript, independently update
the buffer and rolling hash, whichever is active. This avoids the
complications around dont_free_handshake_buffer and EMS.
BUG=492371
Change-Id: I3b1065796a50fd1be5d42ead7210c2f253ef0aca
Reviewed-on: https://boringssl-review.googlesource.com/5615
Reviewed-by: Adam Langley <agl@google.com>
It's purely the PRF function now, although it's still different from the
rest due to the _DEFAULT field being weird.
Change-Id: Iaea7a99cccdc8be4cd60f6c1503df5be2a63c4c5
Reviewed-on: https://boringssl-review.googlesource.com/5614
Reviewed-by: Adam Langley <agl@google.com>
It's a property of just algorithm_enc and hopefully AES-GCM will
continue to be the only true AEAD that requires this. Simpler to just
keep it in ssl_aead_ctx.c.
Change-Id: Ib7c060a3de2fa8590b2dc36c23a5d5fabff43b07
Reviewed-on: https://boringssl-review.googlesource.com/5613
Reviewed-by: Adam Langley <agl@google.com>
Take the sequence number as a parameter. Also replace satsub64be with
the boring thing: convert to uint64_t and subtract normally.
BUG=468889
Change-Id: Icab75f872b5e55cf4e9d68b66934ec91afeb198b
Reviewed-on: https://boringssl-review.googlesource.com/5558
Reviewed-by: Adam Langley <agl@google.com>
The RSA key exchange needs decryption and is still unsupported.
Change-Id: I8c13b74e25a5424356afbe6e97b5f700a56de41f
Reviewed-on: https://boringssl-review.googlesource.com/5467
Reviewed-by: Adam Langley <agl@google.com>
This change mirrors upstream's custom extension API because we have some
internal users that depend on it.
Change-Id: I408e442de0a55df7b05c872c953ff048cd406513
Reviewed-on: https://boringssl-review.googlesource.com/5471
Reviewed-by: Adam Langley <agl@google.com>
This is unused. It seems to be distinct from the automatic chain
building and was added in 1.0.2. Seems to be an awful lot of machinery
that consumers ought to configure anyway.
BUG=486295
Change-Id: If3d4a2761f61c5b2252b37d4692089112fc0ec21
Reviewed-on: https://boringssl-review.googlesource.com/5353
Reviewed-by: Adam Langley <agl@google.com>
This allows us to remove the confusing EVP_PKEY argument to the
SSL_PRIVATE_KEY_METHOD wrapper functions. It also simplifies some of the
book-keeping around the CERT structure, as well as the API for
configuring certificates themselves. The current one is a little odd as
some functions automatically route to the slot while others affect the
most recently touched slot. Others still (extra_certs) apply to all
slots, making them not terribly useful.
Consumers with complex needs should use cert_cb or the early callback
(select_certificate_cb) to configure whatever they like based on the
ClientHello.
BUG=486295
Change-Id: Ice29ffeb867fa4959898b70dfc50fc00137f01f3
Reviewed-on: https://boringssl-review.googlesource.com/5351
Reviewed-by: Adam Langley <agl@google.com>
The distinction was not well-enforced in the code. In fact, it wasn't
even possible to use the RSA_SIGN slot because ssl_set_pkey and
ssl_set_cert would always use the RSA_ENC slot.
A follow-up will fold away the mechanism altogether, but this is an easy
initial simplfication.
BUG=486295
Change-Id: I66b5bf3e6dc243dac7c75924c1c1983538e49060
Reviewed-on: https://boringssl-review.googlesource.com/5349
Reviewed-by: Adam Langley <agl@google.com>
There's no need to store more than the TLS values.
Change-Id: I1a93c7c6aa3254caf7cc09969da52713e6f8acf4
Reviewed-on: https://boringssl-review.googlesource.com/5348
Reviewed-by: Adam Langley <agl@google.com>
After the custom key method support, the EVP_PKEY parameter is somewhat
confusing (to be resolved with the certificate slots removal) as it must
always refer to a private key. ssl3_cert_verify_hash is sometimes used
with the peer's public key. If custom keys were supported on the server,
this would break.
Fix this by passing a pkey_type parameter and letting the caller decide
whether this uses SSL_PRIVATE_KEY_METHOD or not.
Change-Id: I673b92579a84b4561f28026ec0b1c78a6bfee440
Reviewed-on: https://boringssl-review.googlesource.com/5341
Reviewed-by: Adam Langley <agl@google.com>
Missed that there were two of them.
Change-Id: Ibab169ef1f75be9c5ad1ffa0f232629e76a4512d
Reviewed-on: https://boringssl-review.googlesource.com/5332
Reviewed-by: Adam Langley <agl@google.com>
It (incorrectly) thinks some variables are uninitialized. It also gets confused
about some const parameters.
Change-Id: Ie2b3a5336692e7293cf03d6a4cd5345d30b628b3
Reviewed-on: https://boringssl-review.googlesource.com/5330
Reviewed-by: Adam Langley <agl@google.com>
Use more sensible variable names. Also move some work between the helpers and
s3_srvr.c a little; the session lookup functions now only return a new session.
Whether to send a ticket is now an additional output to avoid the enum
explosion around renewal. The actual SSL state is not modified.
This is somewhat cleaner as s3_srvr.c may still reject a session for other
reasons, so we avoid setting ssl->session and ssl->verify_result to a session
that wouldn't be used. (They get fixed up in ssl_get_new_session, so it didn't
actually matter.)
Change-Id: Ib52fabbe993b5e2b7408395a02cdea3dee66df7b
Reviewed-on: https://boringssl-review.googlesource.com/5235
Reviewed-by: Adam Langley <agl@google.com>
This change also switches the behaviour of the client. Previously the
client would send the SCSV rather than the extension, but now it'll only
do that for SSLv3 connections.
Change-Id: I67a04b8abbef2234747c0dac450458deb6b0cd0a
Reviewed-on: https://boringssl-review.googlesource.com/5143
Reviewed-by: Adam Langley <agl@google.com>
This adds a new API, SSL_set_private_key_method, which allows the consumer to
customize private key operations. For simplicity, it is incompatible with the
multiple slots feature (which will hopefully go away) but does not, for now,
break it.
The new method is only routed up for the client for now. The server will
require a decrypt hook as well for the plain RSA key exchange.
BUG=347404
Change-Id: I35d69095c29134c34c2af88c613ad557d6957614
Reviewed-on: https://boringssl-review.googlesource.com/5049
Reviewed-by: Adam Langley <agl@google.com>
Turns out the safer/simpler method still wasn't quite right. :-)
session->sess_cert isn't serialized and deserialized, which is poor. Duplicate
it manually for now. Leave a TODO to get rid of that field altogether as it's
not especially helpful. The certificate-related fields should be in the
session. The others probably have no reason to be preserved on resumptions at
all.
Test by making bssl_shim.cc assert the peer cert chain is there or not as
expected.
BUG=501220
Change-Id: I44034167629720d6e2b7b0b938d58bcab3ab0abe
Reviewed-on: https://boringssl-review.googlesource.com/5170
Reviewed-by: Adam Langley <agl@google.com>
Most of the logic was redundant with checks already made in
ssl3_get_server_certificate. The DHE check was missing an ECDHE half
(and was impossible). The ECDSA check allowed an ECDSA certificate for
RSA. The only non-redundant check was a key usage check which,
strangely, is only done for ECDSA ciphers.
(Although this function called X509_certificate_type and checked sign
bits, those bits in X509_certificate_type are purely a function of the
key type and don't do anything.)
Change-Id: I8df7eccc0ffff49e4cfd778bd91058eb253b13cb
Reviewed-on: https://boringssl-review.googlesource.com/5047
Reviewed-by: Adam Langley <agl@google.com>
This doesn't even change behavior. Unlike local configuration, the peer
can never have multiple certificates anyway. (Even with a renego, the
SESS_CERT is created anew.)
This does lose the implicit certificate type check, but the certificate
type is already checked in ssl3_get_server_certificate and later checked
post-facto in ssl3_check_cert_and_algorithm (except that one seems to
have some bugs like it accepts ECDSA certificates for RSA cipher suites,
to be cleaned up in a follow-up). Either way, we have the certificate
mismatch tests for this.
BUG=486295
Change-Id: I437bb723bb310ad54ee4150eda67c1cfe43377b3
Reviewed-on: https://boringssl-review.googlesource.com/5044
Reviewed-by: Adam Langley <agl@google.com>
With SSL2 gone, there's no need for this split between the abstract
cipher framework and ciphers. Put the cipher suite table in ssl_cipher.c
and move other SSL_CIPHER logic there. With that gone, prune the
cipher-related hooks in SSL_PROTOCOL_METHOD.
BUG=468889
Change-Id: I48579de8bc4c0ea52781ba1b7b57bc5b4919d21c
Reviewed-on: https://boringssl-review.googlesource.com/4961
Reviewed-by: Adam Langley <agl@google.com>
They're redundant with each other.
Change-Id: I17e7ff8c4e0b1486986dd866fd99673fa2aaa494
Reviewed-on: https://boringssl-review.googlesource.com/4959
Reviewed-by: Adam Langley <agl@google.com>
All ciphers are implemented by an EVP_AEAD.
Change-Id: Ifa754599a34e16bf97e1a4b84a271c6d45462c7c
Reviewed-on: https://boringssl-review.googlesource.com/4958
Reviewed-by: Adam Langley <agl@google.com>
The ctrl hooks are left alone since they should just go away.
Simplifying the cipher story will happen in the next CL.
BUG=468889
Change-Id: I979971c90f59c55cd5d17554f1253158b114f18b
Reviewed-on: https://boringssl-review.googlesource.com/4957
Reviewed-by: Adam Langley <agl@google.com>
