Some of the documentation had the right explanation but the incorrect
function names attached.
Change-Id: I7b479dae6d71a5ac7bc86df5a3890508c3b3d09f
Reviewed-on: https://boringssl-review.googlesource.com/5090
Reviewed-by: Adam Langley <agl@google.com>
If we're going to have PSK and use standard cipher suites, this might be
the best that we can do for the moment.
Change-Id: I35d9831b2991dc5b23c9e24d98cdc0db95919d39
Reviewed-on: https://boringssl-review.googlesource.com/5052
Reviewed-by: Adam Langley <agl@google.com>
This is the best PSK cipher suite, but it's non-standard and nobody is
using it. Trivial to bring back in the future if we have need of it.
Change-Id: Ie78790f102027c67d1c9b19994bfb10a2095ba92
Reviewed-on: https://boringssl-review.googlesource.com/5051
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
We shouldn't have protocol constraints that are sensitive to whether
data is returned synchronously or not.
Per https://boringssl-review.googlesource.com/#/c/4112/, the original
limitation was to avoid OpenSSL ABI changes. This is no longer a
concern.
Add tests for the sync and async case. Send the empty records in two
batches to ensure the count is reset correctly.
Change-Id: I3fee839438527e71adb83d437879bb0d49ca5c07
Reviewed-on: https://boringssl-review.googlesource.com/5040
Reviewed-by: Adam Langley <agl@google.com>
We have need of it internally.
Change-Id: I564af468728b22245e8eab384ea7018b7e88cc86
Reviewed-on: https://boringssl-review.googlesource.com/5022
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ic82ab5de4e231cdf6230ee7262c3c7539404d4a6
Reviewed-on: https://boringssl-review.googlesource.com/5020
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This change makes |CBS_get_any_asn1_element| only handle DER elements.
Another function, |CBS_get_any_ber_asn1_element| is exposed internally
for the cases where we need to process BER data.
Change-Id: I544141a1a3d7913986352a8fd9a6d00b9f282652
Reviewed-on: https://boringssl-review.googlesource.com/4994
Reviewed-by: Adam Langley <agl@google.com>
The client and server both have to decide on behaviour when resuming a
session where the EMS state of the session doesn't match the EMS state
as exchanged in the handshake.
Original handshake
| No Yes
------+--------------------------------------------------------------
|
R | Server: ok [1] Server: abort [3]
e No | Client: ok [2] Client: abort [4]
s |
u |
m |
e |
Yes | Server: don't resume No problem
| Client: abort; server
| shouldn't have resumed
[1] Servers want to accept legacy clients. The draft[5] says that
resumptions SHOULD be rejected so that Triple-Handshake can't be done,
but we'll rather enforce that EMS was used when using tls-unique etc.
[2] The draft[5] says that even the initial handshake should be aborted
if the server doesn't support EMS, but we need to be able to talk to the
world.
[3] This is a very weird case where a client has regressed without
flushing the session cache. Hopefully we can be strict and reject these.
[4] This can happen when a server-farm shares a session cache but
frontends are not all updated at once. If Chrome is strict here then
hopefully we can prevent any servers from existing that will try to
resume an EMS session that they don't understand. OpenSSL appears to be
ok here: https://www.ietf.org/mail-archive/web/tls/current/msg16570.html
[5] https://tools.ietf.org/html/draft-ietf-tls-session-hash-05#section-5.2
BUG=492200
Change-Id: Ie1225a3960d49117b05eefa5a36263d8e556e467
Reviewed-on: https://boringssl-review.googlesource.com/4981
Reviewed-by: Adam Langley <agl@google.com>
This implementation does not prompt for a password. It's just enough
to ensure that the many functions that take a tuple of
|pem_password_cb| and a |void *| to a password work in a reasonable
way when the latter is non-NULL.
Change-Id: Ic6bfc484630c67b5ede25277e14eb3b00c2024f0
Reviewed-on: https://boringssl-review.googlesource.com/4990
Reviewed-by: Adam Langley <agl@google.com>
The only flag is EVP_MD_CTX_FLAG_NO_INIT and no good can possibly come of
anyone outside EVP_PKEY_HMAC calling it. (And indeed no one calls it.
EVP_MD_CTX_set_flags has a caller in wpa_supplicant, but it uses
EVP_MD_CTX_FLAG_NON_FIPS_ALLOW which we don't define. The call is guarded by a
pair of ifdefs for some FIPS mode wpa_supplicant.)
Change-Id: I70ab8ffa646f3f75dfa4d37c96b9e82448ff1e40
Reviewed-on: https://boringssl-review.googlesource.com/4971
Reviewed-by: Adam Langley <agl@google.com>
It's never called externally and for good reason; the only flag to set is
EVP_MD_CTX_FLAG_NO_INIT which is an implementation detail of EVP_PKEY_HMAC
(hopefully to be removed eventually). Indeed, only EVP_PKEY_HMAC ever calls
this function. Except there's no need to because the HMAC_CTX has already been
initialized at that point. (And were it not initialized, that call would not
bode well for the poor HMAC_CTX.)
The legacy EVP_PKEY_HMAC API has test coverage and still works after this
change.
Change-Id: I2fb0bede3c24ad1519f9433f957606de15ba86c7
Reviewed-on: https://boringssl-review.googlesource.com/4970
Reviewed-by: Adam Langley <agl@google.com>
The SSL_PROTOCOL_METHOD table needs work, but this makes it clearer
exactly what the shared interface between the upper later and TLS/DTLS
is.
BUG=468889
Change-Id: I38931c484aa4ab3f77964d708d38bfd349fac293
Reviewed-on: https://boringssl-review.googlesource.com/4955
Reviewed-by: Adam Langley <agl@google.com>
Enough code fails to check their return codes anyway. We ought to make
it official.
Change-Id: Ie646360fd7073ea943036f5e21bed13df7e1b77a
Reviewed-on: https://boringssl-review.googlesource.com/4954
Reviewed-by: Adam Langley <agl@google.com>
The SHA-2 family has some exceptions, but they're all programmer errors
and should be documented as such. (Are the failure cases even
necessary?)
Change-Id: I00bd0a9450cff78d8caac479817fbd8d3de872b8
Reviewed-on: https://boringssl-review.googlesource.com/4953
Reviewed-by: Adam Langley <agl@google.com>
These defines are part of the the locking callbacks which have been
removed. However, code that still tries to provide locking callbacks
will need these values to compile.
The locking callback that such code tries to install will be ignored,
but that's harmless since BoringSSL handles locking itself now.
Change-Id: Ic84da8b52020ccd3ecc8913b4e41d366690c7649
Android needs to be able to read a PKCS#7 blob from a Java
InputStream. This change adds |BIO_read_asn1| which reads a single
ASN.1 object from the start of a BIO without overreading.
Change-Id: I74776e686529c8e58af1c26a4909f9bd4e87b707
If BN_rand is called with |bits| set to 1 and |top| set to 1 then a 1 byte
buffer overflow can occur.
See also upstream's efee575ad464bfb60bf72dcb73f9b51768f4b1a1. But rather than
making |BN_rand| fail, be consistent with the |bits| = 0 case and just don't
set the bits that don't exist. Add tests to ensure the degenerate cases behave.
Change-Id: I5e9fbe6fd8f7f7b2e011a680f2fbe6d7ed4dab65
Reviewed-on: https://boringssl-review.googlesource.com/4893
Reviewed-by: Adam Langley <agl@google.com>
The functions BN_rshift and BN_lshift shift their arguments to the right or
left by a specified number of bits. Unpredicatable results (including
crashes) can occur if a negative number is supplied for the shift value.
Thanks to Mateusz Kocielski (LogicalTrust), Marek Kroemeke and Filip Palian
for discovering and reporting this issue.
(Imported from upstream's 7cc18d8158b5fc2676393d99b51c30c135502107.)
Change-Id: Ib9f5e410a46df3d7f02a61374807fba209612bd3
Reviewed-on: https://boringssl-review.googlesource.com/4892
Reviewed-by: Adam Langley <agl@google.com>
This is documented as "Only request a client certificate on the initial TLS/SSL
handshake. Do not ask for a client certificate again in case of a
renegotiation." Server-side renegotiation is gone.
I'm not sure this flag has ever worked anyway, dating all the way back to
SSLeay 0.8.1b. ssl_get_new_session overwrites s->session, so the old
session->peer is lost.
Change-Id: Ie173243e189c63272c368a55167b8596494fd59c
Reviewed-on: https://boringssl-review.googlesource.com/4883
Reviewed-by: Adam Langley <agl@google.com>
(obj_dat.h and obj_mac.h are generated from the objects.txt change.)
See upstream's 3c161d081e2d30549e787437d05ffa08122a5114. Also see upstream's
12048657a91b12e499d03ec9ff406b42aba67366 to give zlib a better comment.
Change-Id: I86937f037f8e0f6179ba8072ccd972eca773c7ce
Reviewed-on: https://boringssl-review.googlesource.com/4882
Reviewed-by: Adam Langley <agl@google.com>
Never send the time as a client. Always send it as a server.
Change-Id: I20c55078cfe199d53dc002f6ee5dd57060b086d5
Reviewed-on: https://boringssl-review.googlesource.com/4829
Reviewed-by: Adam Langley <agl@google.com>
Yes, OpenSSL lets you randomly change its internal state. This is used
as part of server-side renegotiation. Server-side renegotiation is gone.
BUG=429450
Change-Id: Ic1b013705734357acf64e8bf89a051b2b7521c64
Reviewed-on: https://boringssl-review.googlesource.com/4828
Reviewed-by: Adam Langley <agl@google.com>
It's never called and the state is meaningless now.
Change-Id: I5429ec3eb7dc2b789c0584ea88323f0ff18920ae
Reviewed-on: https://boringssl-review.googlesource.com/4826
Reviewed-by: Adam Langley <agl@google.com>
When the peer or caller requests a renegotiation, OpenSSL doesn't
renegotiate immediately. It sets a flag to begin a renegotiation as soon
as record-layer read and write buffers are clear. One reason is that
OpenSSL's record layer cannot write a handshake record while an
application data record is being written. The buffer consistency checks
around partial writes will break.
None of these cases are relevant for the client auth hack. We already
require that renego come in at a quiescent part of the application
protocol by forbidding handshake/app_data interleave.
The new behavior is now: when a HelloRequest comes in, if the record
layer is not idle, the renegotiation is rejected as if
SSL_set_reject_peer_renegotiations were set. Otherwise we immediately
begin the new handshake. The server may not send any application data
between HelloRequest and completing the handshake. The HelloRequest may
not be consumed if an SSL_write is pending.
Note this does require that Chromium's HTTP stack not attempt to read
the HTTP response until the request has been written, but the
renegotiation logic already assumes it. Were Chromium to drive the
SSL_read state machine early and the server, say, sent a HelloRequest
after reading the request headers but before we've sent the whole POST
body, the SSL state machine may racily enter renegotiate early, block
writing the POST body on the new handshake, which would break Chromium's
ERR_SSL_CLIENT_AUTH_CERT_NEEDED plumbing.
BUG=429450
Change-Id: I6278240c3bceb5d2e1a2195bdb62dd9e0f4df718
Reviewed-on: https://boringssl-review.googlesource.com/4825
Reviewed-by: Adam Langley <agl@google.com>
The only case where renego is supported is if we are a client and the
server sends a HelloRequest. That is still needed to support the renego
+ client auth hack in Chrome. Beyond that, no other forms of renego will
work.
The messy logic where the handshake loop is repurposed to send
HelloRequest and the extremely confusing tri-state s->renegotiate (which
makes SSL_renegotiate_pending a lie during the initial handshake as a
server) are now gone. The next change will further simplify things by
removing ssl->s3->renegotiate and the renego deferral logic. There's
also some server-only renegotiation checks that can go now.
Also clean up ssl3_read_bytes' HelloRequest handling. The old logic relied on
the handshake state machine to reject bad HelloRequests which... actually that
code probably lets you initiate renego by sending the first four bytes of a
ServerHello and expecting the peer to read it later.
BUG=429450
Change-Id: Ie0f87d0c2b94e13811fe8e22e810ab2ffc8efa6c
Reviewed-on: https://boringssl-review.googlesource.com/4824
Reviewed-by: Adam Langley <agl@google.com>
Now that WebRTC honors packet boundaries (https://crbug.com/447431), we
can start enforcing them correctly. Configuring read-ahead now does
nothing. Instead DTLS will always set "read-ahead" and also correctly
enforce packet boundaries when reading records. Add tests to ensure that
badly fragmented packets are ignored. Because such packets don't fail
the handshake, the tests work by injecting an alert in the front of the
handshake stream and ensuring the DTLS implementation ignores them.
ssl3_read_n can be be considerably unraveled now, but leave that for
future cleanup. For now, make it correct.
BUG=468889
Change-Id: I800cfabe06615af31c2ccece436ca52aed9fe899
Reviewed-on: https://boringssl-review.googlesource.com/4820
Reviewed-by: Adam Langley <agl@google.com>
This isn't exhaustive. There are still failures in some tests which probably
ought to get C++'d first.
Change-Id: Iac58df9d98cdfd94603d54374a531b2559df64c3
Reviewed-on: https://boringssl-review.googlesource.com/4795
Reviewed-by: Adam Langley <agl@google.com>
tls1_enc is now SSL_AEAD_CTX_{open,seal}. This starts tidying up a bit
of the record-layer logic. This removes rr->input, as encrypting and
decrypting records no longer refers to various globals. It also removes
wrec altogether. SSL3_RECORD is now only used to maintain state about
the current incoming record. Outgoing records go straight to the write
buffer.
This also removes the outgoing alignment memcpy and simply calls
SSL_AEAD_CTX_seal with the parameters as appropriate. From bssl speed
tests, this seems to be faster on non-ARM and a bit of a wash on ARM.
Later it may be worth recasting these open/seal functions to write into
a CBB (tweaked so it can be malloc-averse), but for now they take an
out/out_len/max_out trio like their EVP_AEAD counterparts.
BUG=468889
Change-Id: Ie9266a818cc053f695d35ef611fd74c5d4def6c3
Reviewed-on: https://boringssl-review.googlesource.com/4792
Reviewed-by: Adam Langley <agl@google.com>
(This makes it possible to include opensslv.h when not linking SSL.)
Change-Id: Id88c5ff44a7099d33d8d4672f7ba88986ffd1526
Reviewed-on: https://boringssl-review.googlesource.com/4831
Reviewed-by: Adam Langley <agl@google.com>
SSL3_VERSION_MAJOR is the only MAJOR/MINOR number used internally or
externally.
Change-Id: I3f17175e73fd89887665accf1bfa680581f42dfe
Reviewed-on: https://boringssl-review.googlesource.com/4790
Reviewed-by: Adam Langley <agl@google.com>
Chromium's session cache has since been rewritten and no longer needs to
muck with those functions in tests.
Change-Id: I2defad81513210dca5e105757e04cbb677583251
Reviewed-on: https://boringssl-review.googlesource.com/4788
Reviewed-by: Adam Langley <agl@google.com>
Current thought is to organize this by:
- Core SSL_CTX APIs (creating, destroying)
- Core SSL APIs (creating destroying, maybe handshake, read, write as
well)
- APIs to configure SSL_CTX/SSL, roughly grouped by feature. Probably
options and modes are the first two sections. SSL_TXT_* constants can
be part of documenting cipher suite configuration.
- APIs to query state from SSL_CTX/SSL, roughly grouped by feature. (Or
perhaps these should be folded into the configuration sections?)
The functions themselves aren't reordered or reorganized to match the
eventual header order yet. Though I did do the s -> ssl rename on the
ones I've touched.
Also formally deprecate SSL_clear. It would be a core SSL API
except it's horrible.
Change-Id: Ia7e4fdcb7bad4e9ccdee8cf8c3136dc63aaaa772
Reviewed-on: https://boringssl-review.googlesource.com/4784
Reviewed-by: Adam Langley <agl@google.com>
The type names are perfectly serviceable. Most of them are
forward-declared in base.h.
Change-Id: Id03f5039a2d1bab82c68ade074a0e26cd3ab5ad9
Reviewed-on: https://boringssl-review.googlesource.com/4783
Reviewed-by: Adam Langley <agl@google.com>
Looks like it was the use in type_check.h that was still causing
problems, not that MSVC doesn't short-circuit #if statements.
Change-Id: I574e8dd463c46b0133a989b221a7bb8861b3eed9
At this point, none of these functions or macros are used so they can
just be deleted.
Change-Id: I8ed1aae7a252e886864bf43e3096eff2228183cd
Reviewed-on: https://boringssl-review.googlesource.com/4777
Reviewed-by: Adam Langley <agl@google.com>
These ASN.1 macros are the last references to the old-style OpenSSL
locks that remain. The ASN.1 reference count handling was changed in a
previous commit to use |CRYPTO_refcount_*| so these lock references were
unused anyway.
Change-Id: I1b27eef140723050a8e6878a1bea11da3409d0eb
Reviewed-on: https://boringssl-review.googlesource.com/4776
Reviewed-by: Adam Langley <agl@google.com>
|SSL_CTX| and |X509_STORE| have grown their own locks. Several static
locks have been added to hack around not being able to use a
|CRYPTO_once_t| in public headers. Lastly, support for calling
|SSL_CTX_set_generate_session_id| concurrently with active connections
has been removed. No other property of an |SSL_CTX| works like that.
Change-Id: Iff5fe3ee3fdd6ea9c9daee96f850b107ad8a6bca
Reviewed-on: https://boringssl-review.googlesource.com/4775
Reviewed-by: Adam Langley <agl@google.com>
It's no longer needed after the conversion to |CRYPTO_refcount_t|.
Change-Id: Ied129c4c247fcd426745fa016350528b7571aaaa
Reviewed-on: https://boringssl-review.googlesource.com/4774
Reviewed-by: Adam Langley <agl@google.com>
Convert reference counts in ssl/ to use |CRYPTO_refcount_t|.
Change-Id: I5d60f641b0c89b1ddfe38bfbd9d7285c60377f4c
Reviewed-on: https://boringssl-review.googlesource.com/4773
Reviewed-by: Adam Langley <agl@google.com>
This change converts the reference counts in crypto/ to use
|CRYPTO_refcount_t|. The reference counts in |X509_PKEY| and |X509_INFO|
were never actually used and so were dropped.
Change-Id: I75d572cdac1f8c1083c482e29c9519282d7fd16c
Reviewed-on: https://boringssl-review.googlesource.com/4772
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL has traditionally done reference counting with |int|s and the
|CRYPTO_add| function. Unless a special callback is installed (rare),
this is implemented by doing the reference count operations under a
lock.
This change adds infrastructure for handling reference counts and uses
atomic operations when C11 support is available.
Change-Id: Ia023ce432319efd00f77a7340da27d16ee4b63c3
Reviewed-on: https://boringssl-review.googlesource.com/4771
Reviewed-by: Adam Langley <agl@google.com>
OPENSSL_COMPILE_ASSERT implements a static assertion, but the error
message is a little weird because it's a hack around the fact that C,
traditionally, doesn't have static assertions.
C11 now does have _Static_assert (a.k.a. static_assert when one includes
assert.h) so we can use that when provided to get cleaner error
messages.
Change-Id: Ia3625dfb2988de11fd95ddba957f118c0d3183ff
Reviewed-on: https://boringssl-review.googlesource.com/4770
Reviewed-by: Adam Langley <agl@google.com>
DH groups less than 1024 bits are clearly not very safe. Ideally servers
would switch to ECDHE because 1024 isn't great either, but this will
serve for the short term.
BUG=490240
Change-Id: Ic9aac714cdcdcbfae319b5eb1410675d3b903a69
Reviewed-on: https://boringssl-review.googlesource.com/4813
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
SSLeay is a compatibility function for OpenSSL, but I got it wrong. It
doesn't return a string, it returns a number. This doesn't end up making
any difference, but it fixes a warning when building OpenSSH.
Change-Id: I327ab4f70313c93c18f81d8804ba4acdc3bc1a4a
Reviewed-on: https://boringssl-review.googlesource.com/4811
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This change exposes the functions needed to support arbitrary elliptic
curve groups. The Java API[1] doesn't allow a provider to only provide
certain elliptic curve groups. So if BoringSSL is an ECC provider on
Android, we probably need to support arbitrary groups because someone
out there is going to be using it for Bitcoin I'm sure.
Perhaps in time we can remove this support, but not yet.
[1] https://docs.oracle.com/javase/7/docs/api/java/security/spec/ECParameterSpec.html
Change-Id: Ic1d76de96f913c9ca33c46b451cddc08c5b93d80
Reviewed-on: https://boringssl-review.googlesource.com/4740
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
SSL_get_current_cipher is documented by upstream to return the cipher actually
being used. However, because it reads s->session, it returns information
pertaining to the session to be offered if queried before ServerHello or early
in an abbreviated handshake.
Logic around s->session needs more comprehensive cleanup but for just this
function, defining it to be the current outgoing cipher is close to the current
semantics but for fixing the initial state (s->session->cipher is populated
when sending CCS). Store it in the SSL_AEAD_CTX which seems a natural place to
associate state pertaining to a connection half.
BUG=484744
Change-Id: Ife8db27a16615d0dbb2aec65359537243e08af7c
Reviewed-on: https://boringssl-review.googlesource.com/4733
Reviewed-by: Adam Langley <agl@google.com>
This cuts down on one config knob as well as one case in the renego
combinatorial explosion. Since the only case we care about with renego
is the client auth hack, there's no reason to ever do resumption.
Especially since, no matter what's in the session cache:
- OpenSSL will only ever offer the session it just established,
whether or not a newer one with client auth was since established.
- Chrome will never cache sessions created on a renegotiation, so
such a session would never make it to the session cache.
- The new_session + SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION
logic had a bug where it would unconditionally never offer tickets
(but would advertise support) on renego, so any server doing renego
resumption against an OpenSSL-derived client must not support
session tickets.
This also gets rid of s->new_session which is now pointless.
BUG=429450
Change-Id: I884bdcdc80bff45935b2c429b4bbc9c16b2288f8
Reviewed-on: https://boringssl-review.googlesource.com/4732
Reviewed-by: Adam Langley <agl@google.com>
We have a lot of options that don't do anything.
Change-Id: I1681fd07d1272547d4face87917ce41029bbf0de
Reviewed-on: https://boringssl-review.googlesource.com/4731
Reviewed-by: Adam Langley <agl@google.com>
There's multiple different versions of this check, between
s->s3->have_version (only works at some points), s->new_session (really
weird and not actually right), s->renegotiate (fails on the server
because it's always 2 after ClientHello), and s->s3->tmp.finish_md_len
(super confusing). Add an explicit bit with clear meaning. We'll prune
some of the others later; notably s->renegotiate can go away when
initiating renegotiation is removed.
This also tidies up the extensions to be consistent about whether
they're allowed during renego:
- ALPN failed to condition when accepting from the server, so even
if the client didn't advertise, the server could.
- SCTs now *are* allowed during renego. I think forbidding it was a
stray copy-paste. It wasn't consistently enforced in both ClientHello
and ServerHello, so the server could still supply it. Moreover, SCTs
are part of the certificate, so we should accept it wherever we accept
certificates, otherwise that session's state becomes incomplete. This
matches OCSP stapling. (NB: Chrome will never insert a session created
on renego into the session cache and won't accept a certificate
change, so this is moot anyway.)
Change-Id: Ic9bd1ebe2a2dbe75930ed0213bf3c8ed8170e251
Reviewed-on: https://boringssl-review.googlesource.com/4730
Reviewed-by: Adam Langley <agl@google.com>
As of crbug.com/484543, Chromium's SSLClientSocket is not sensitive to whether
renegotiation is enabled or not. Disable it by default and require consumers to
opt into enabling this protocol mistake.
BUG=429450
Change-Id: I2329068284dbb851da010ff1fd398df3d663bcc3
Reviewed-on: https://boringssl-review.googlesource.com/4723
Reviewed-by: Adam Langley <agl@google.com>
This change makes it safe to call EVP_AEAD_CTX_cleanup after a failed
EVP_AEAD_CTX_init.
Change-Id: I608ed550e08d638cd7e941f5067edd3da4c850ab
Reviewed-on: https://boringssl-review.googlesource.com/4692
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
There's no real need to ever disable it, so this is one fewer configuration to
test. It's still disabled for DTLS, but a follow-up will resolve that.
Change-Id: Ia95ad8c17ae8236ada516b3968a81c684bf37fd9
Reviewed-on: https://boringssl-review.googlesource.com/4683
Reviewed-by: Adam Langley <agl@google.com>
inttypes.h kindly requires a feature macro in C++ on some platforms, due
to a bizarre footnote in C99 (see footnote 191 in section 7.8.1). As
bn.h is a public header, we must leak this wart to the consumer. On
platforms with unfriendly inttypes.h headers, using BN_DEC_FMT1 and
friends now require the feature macro be defined externally.
This broke the Chromium Android Clang builder:
http://build.chromium.org/p/chromium.linux/builders/Android%20Clang%20Builder%20%28dbg%29/builds/59288
Change-Id: I88275a6788c7babd0eae32cae86f115bfa93a591
Reviewed-on: https://boringssl-review.googlesource.com/4688
Reviewed-by: Adam Langley <agl@google.com>
Change |EC_KEY_new_by_curve_name| to report |ERR_R_MALLOC_FAILURE|
itself, so that reporting of |EC_R_UNKNOWN_GROUP| is not confused by
the caller's addition of a spurious |ERR_R_MALLOC_FAILURE|.
Change-Id: Id3f5364f01eb8e3597bcddd6484bc03d5578befb
Reviewed-on: https://boringssl-review.googlesource.com/4690
Reviewed-by: Adam Langley <agl@google.com>
Trusty doesn't have setjmp.h and nor does it have threads.
Change-Id: I005f7a009a13e6632513be9fab2bbe62294519a4
Reviewed-on: https://boringssl-review.googlesource.com/4660
Reviewed-by: Adam Langley <agl@google.com>
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>
Nothing ever uses those structs. This to avoid having any structs in the
public header which use struct timeval.
In doing so, move the protocol version constants up to ssl.h so dtls1.h
may be empty. This also removes TLS1_get_version and TLS1_get_client_version
as they're unused and depend on TLS1_VERSION_MAJOR. This still lets tls1.h
be included independently from ssl.h (though I don't think anyone ever includes
it...).
Change-Id: Ieac8b90cf94f7f1e742a88bb75c0ee0aa4b1414c
Reviewed-on: https://boringssl-review.googlesource.com/4681
Reviewed-by: Adam Langley <agl@google.com>
The interface for this is very similar to upstream, but the code is
quite different.
Support for “resuming” (i.e. calling |CMAC_Final| and then computing the
CMAC for an extension of the message) has been dropped. Also, calling
|CMAC_Init| with magic argument to reset it has been replaced with
|CMAC_Reset|.
Lastly, a one-shot function has been added because it can save an
allocation and that's what most callers actually appear to want to do.
Change-Id: I9345220218bdb16ebe6ca356928d7c6f055d83f6
Reviewed-on: https://boringssl-review.googlesource.com/4630
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The only place using it is export keying material which can do the
version check inline.
Change-Id: I1893966c130aa43fa97a6116d91bb8b04f80c6fb
Reviewed-on: https://boringssl-review.googlesource.com/4615
Reviewed-by: Adam Langley <agl@google.com>
It's only called for client certificates with NULL. The interaction with
extra_certs is more obvious if we handle that case externally. (We
shouldn't attach extra_certs if there is no leaf.)
Change-Id: I9dc26f32f582be8c48a4da9aae0ceee8741813dc
Reviewed-on: https://boringssl-review.googlesource.com/4613
Reviewed-by: Adam Langley <agl@google.com>
Next batch. Mostly a bunch of deprecated things. This switches
SSL_CTX_set_tmp_rsa from always failing to always succeeding. The latter
is probably a safer behavior; a consumer may defensively set a temporary
RSA key. We'll successfully "set it" and just never use the result.
Change-Id: Idd3d6bf4fc1a20bc9a26605bb9c77c9f799f993c
Reviewed-on: https://boringssl-review.googlesource.com/4566
Reviewed-by: Adam Langley <agl@google.com>
This is an API wart that makes it easy to accidentally reuse the server
DHE half for every handshake. It's much simpler to have only one mode.
This mirrors the change made to the ECDHE code; align with that logic.
Change-Id: I47cccbb354d70127ab458f99a6d390b213e4e515
Reviewed-on: https://boringssl-review.googlesource.com/4565
Reviewed-by: Adam Langley <agl@google.com>
The only difference is SSL_clear_num_renegotiations which is never
called.
Change-Id: Id661c71e89d34d834349ad1f1a296e332606e6cc
Reviewed-on: https://boringssl-review.googlesource.com/4564
Reviewed-by: Adam Langley <agl@google.com>
The API is unused and rather awkward (mixes output parameters with
return values, special-case for NULL).
Change-Id: I4396f98534bf1271e53642f255e235cf82c7615a
Reviewed-on: https://boringssl-review.googlesource.com/4560
Reviewed-by: Adam Langley <agl@google.com>
Also size them based on the limits in the quantities they control (after
checking bounds at the API boundary).
BUG=404754
Change-Id: Id56ba45465a473a1a793244904310ef747f29b63
Reviewed-on: https://boringssl-review.googlesource.com/4559
Reviewed-by: Adam Langley <agl@google.com>
Not going to bother adding the compatibility macros. If they get ifdef'd
out, all the better.
BUG=404754
Change-Id: I26414d2fb84ee1f0b15a3b96c871949fe2bb7fb1
Reviewed-on: https://boringssl-review.googlesource.com/4558
Reviewed-by: Adam Langley <agl@google.com>
This is a bitmask, so the number of bits available should be the same
across all platforms.
Change-Id: I98e8d375fc7d042aeae1270174bc8fc63fba5dfc
Reviewed-on: https://boringssl-review.googlesource.com/4556
Reviewed-by: Adam Langley <agl@google.com>
Document them while I'm here. This adds a new 'preprocessor
compatibility section' to avoid breaking #ifdefs. The CTRL values
themselves are defined to 'doesnt_exist' to catch anything calling
SSL_ctrl directly until that function can be unexported completely.
BUG=404754
Change-Id: Ia157490ea8efe0215d4079556a0c7643273e7601
Reviewed-on: https://boringssl-review.googlesource.com/4553
Reviewed-by: Adam Langley <agl@google.com>
Probably we'll want some simpler server-side API later. But, as things
stand, all consumers of these functions are #ifdef'd out and have to be
because the requisite OCSP_RESPONSE types are gone.
Change-Id: Ic82b2ab3feca14c56656da3ceb3651819e3eb377
Reviewed-on: https://boringssl-review.googlesource.com/4551
Reviewed-by: Adam Langley <agl@google.com>
It's unused, but for some old #ifdef branch in wpa_supplicant's EAP-FAST
hack, before SSL_set_session_ticket_ext_cb existed.
Change-Id: Ifc11fea2f6434354f756e04e5fc3ed5f1692025e
Reviewed-on: https://boringssl-review.googlesource.com/4550
Reviewed-by: Adam Langley <agl@google.com>
This avoids callers having to worry about |CRYPTO_add| and what the
correct lock to use it with is. (Esp since we'll probably change the way
that reference counts work in the future.)
Change-Id: I972bf0cc3be6099e0255e64a0fd50249062d1eb4
Reviewed-on: https://boringssl-review.googlesource.com/4623
Reviewed-by: Adam Langley <agl@google.com>
BoringSSL always uses uncompressed points. This function aborts if
another form is requested or does nothing if uncompressed points are
requested.
Change-Id: I80bc01444cdf9c789c9c75312b5527bf4957361b
I tried so hard to get rid of AES-192, but it's called from too many
places. I suspect that those places don't actually use it, but it's
dangerous to assume that.
Change-Id: I6208b64a463e3539973532abd21882e0e4c55a1c
“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>
Android uses BIO reference counting.
This reverts commit 9bde6aeb76.
Change-Id: Ibf4a7f42477549d10829a424ea3b52f09098666c
Reviewed-on: https://boringssl-review.googlesource.com/4472
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
These functions were #if 0'ed out in the code, which is a distraction.
Change-Id: I186196ab512565507476f9b56682bf59d003d85f
Reviewed-on: https://boringssl-review.googlesource.com/4604
Reviewed-by: Adam Langley <agl@google.com>
These are never used and no flags are defined anyway.
Change-Id: I206dc2838c5f68d87559a702dcb299b208cc7e1e
Reviewed-on: https://boringssl-review.googlesource.com/4493
Reviewed-by: Adam Langley <agl@google.com>
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>
054e682675 removed the compatibility include of
mem.h in crypto.h. mem.h doesn't exist in upstream which defines these
functions in crypto.h instead. The compatibility include should probably be
restored to avoid causing all kinds of grief when porting consumers over.
Change-Id: Idfe0f9b43ebee5df22bebfe0ed6dc85ec98b4de0
Reviewed-on: https://boringssl-review.googlesource.com/4530
Reviewed-by: Adam Langley <agl@google.com>
CRYPTO_MUTEX was the wrong size. Fortunately, Apple was kind enough to define
pthread_rwlock_t unconditionally, so we can be spared fighting with feature
macros. Some of the stdlib.h removals were wrong and clang is pick about
multiply-defined typedefs. Apparently that's a C11 thing?
BUG=478598
Change-Id: Ibdcb8de9e5d83ca28e4c55b2979177d1ef0f9721
Reviewed-on: https://boringssl-review.googlesource.com/4404
Reviewed-by: Adam Langley <agl@google.com>
This is taken from upstream, although it originally came from us. This
will only take effect on 64-bit systems (x86-64 and aarch64).
Before:
Did 1496 ECDH P-256 operations in 1038743us (1440.2 ops/sec)
Did 2783 ECDSA P-256 signing operations in 1081006us (2574.5 ops/sec)
Did 2400 ECDSA P-256 verify operations in 1059508us (2265.2 ops/sec)
After:
Did 4147 ECDH P-256 operations in 1061723us (3905.9 ops/sec)
Did 9372 ECDSA P-256 signing operations in 1040589us (9006.4 ops/sec)
Did 4114 ECDSA P-256 verify operations in 1063478us (3868.4 ops/sec)
Change-Id: I11fabb03239cc3a7c4a97325ed4e4c97421f91a9
We don't support the SSL BIO so this is a no-op change.
Change-Id: Iba9522b837ebb0eb6adc80d5df6dcac99abf2552
Reviewed-on: https://boringssl-review.googlesource.com/4360
Reviewed-by: Adam Langley <agl@google.com>
Instead, each module defines a static CRYPTO_EX_DATA_CLASS to hold the values.
This makes CRYPTO_cleanup_all_ex_data a no-op as spreading the
CRYPTO_EX_DATA_CLASSes across modules (and across crypto and ssl) makes cleanup
slightly trickier. We can make it do something if needbe, but it's probably not
worth the trouble.
Change-Id: Ib6f6fd39a51d8ba88649f0fa29c66db540610c76
Reviewed-on: https://boringssl-review.googlesource.com/4375
Reviewed-by: Adam Langley <agl@google.com>
No functions for using it were ever added.
Change-Id: Iaee6e5bc8254a740435ccdcdbd715b851d8a0dce
Reviewed-on: https://boringssl-review.googlesource.com/4374
Reviewed-by: Adam Langley <agl@google.com>
No wrappers were ever added and codesearch confirms no one ever added to it
manually. Probably anyone doing complex things with BIOs just made a custom
BIO_METHOD. We can put it back with proper functions if the need ever arises.
Change-Id: Icb5da7ceeb8f1da6d08f4a8854d53dfa75827d9c
Reviewed-on: https://boringssl-review.googlesource.com/4373
Reviewed-by: Adam Langley <agl@google.com>
Callers are required to use the wrappers now. They still need OPENSSL_EXPORT
since crypto and ssl get built separately in the standalone shared library
build.
Change-Id: I61186964e6099b9b589c4cd45b8314dcb2210c89
Reviewed-on: https://boringssl-review.googlesource.com/4372
Reviewed-by: Adam Langley <agl@google.com>
It's unused and requires ex_data support a class number per type.
Change-Id: Ie1fb55053631ef00c3318f3253f7c9501988f522
Reviewed-on: https://boringssl-review.googlesource.com/4371
Reviewed-by: Adam Langley <agl@google.com>
This is never used and we can make the built-in one performant.
Change-Id: I6fc7639ba852349933789e73762bc3fa1341b2ff
Reviewed-on: https://boringssl-review.googlesource.com/4370
Reviewed-by: Adam Langley <agl@google.com>
OpenSSH, especially, does some terrible things that mean that it needs
the EVP_CIPHER structure to be exposed ☹. Damian is open to a better API
to replace this, but only if OpenSSL agree too. Either way, it won't be
happening soon.
Change-Id: I393b7a6af6694d4d2fe9ebcccd40286eff4029bd
Reviewed-on: https://boringssl-review.googlesource.com/4330
Reviewed-by: Adam Langley <agl@google.com>
This introduces a per-RSA/DSA/DH lock. This is good for lock contention,
although pthread locks are depressingly bloated.
Change-Id: I07c4d1606fc35135fc141ebe6ba904a28c8f8a0c
Reviewed-on: https://boringssl-review.googlesource.com/4324
Reviewed-by: Adam Langley <agl@google.com>
Prior to this, BoringSSL was using OpenSSL's technique of having users
register a callback for locking operation. This change adds native mutex
support.
Since mutexes often need to be in objects that are exposed via public
headers, the non-static mutexes are defined in thread.h. However, on
Windows we don't want to #include windows.h for CRITICAL_SECTION and, on
Linux, pthread.h doesn't define pthread_rwlock_t unless the feature
flags are set correctly—something that we can't control in general
for public header files. Thus, on both platforms, the mutex is defined
as a uint8_t[] of equal or greater size and we depend on static asserts
to ensure that everything works out ok.
Change-Id: Iafec17ae7e3422325e587878a5384107ec6647ab
Reviewed-on: https://boringssl-review.googlesource.com/4321
Reviewed-by: Adam Langley <agl@google.com>
It appears that this reference “count” is set to one at creation and
never touched after that.
Change-Id: I3238a6d3dd702953771b8ec725c1c5712c648fba
Reviewed-on: https://boringssl-review.googlesource.com/4320
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>
There's this giant "Underdocumented functions" section in the middle, but it
doesn't look too silly once the "Deprecated methods" section is merged in with
the other deprecated functions.
Change-Id: Ib97d88b0f915f60e9790264474a9e4aa3e115382
Reviewed-on: https://boringssl-review.googlesource.com/4291
Reviewed-by: Adam Langley <agl@google.com>
Just about everything depends on SSL_CIPHER. Move it to the top as the first
section in ssl.h. Match the header order and the source file order and document
everything. Also make a couple of minor style guide tweaks.
Change-Id: I6a810dbe79238278ac480e5ced1447055715a79f
Reviewed-on: https://boringssl-review.googlesource.com/4290
Reviewed-by: Adam Langley <agl@google.com>
Mostly stuff that doc.go was grumpy about. The main change is to move the
version-specific headers to the bottom. Injecting them in the middle makes it
seem as if the definitions above the #include and those below are somehow
different, but it compiles fine with them at the bottom. (They have to be at
the bottom because those headers depend on ssl.h.)
Change-Id: Iaa4139d2f157c7a3fd0ea609b78ff11d2edfc7b0
Reviewed-on: https://boringssl-review.googlesource.com/4289
Reviewed-by: Adam Langley <agl@google.com>
It's no longer needed to distinguish ciphers from fake ciphers.
Change-Id: I1ad4990ba936b1059eb48f3d2f309eb832dd1cb5
Reviewed-on: https://boringssl-review.googlesource.com/4285
Reviewed-by: Adam Langley <agl@google.com>
Avoiding superflous references to MD5 makes it easier to audit the code
to find unsafe uses of it. It also avoids subtly encouraging users to
choose MD5 instead of a better alternative.
Change-Id: Ic78eb5dfbf44aac39e4e4eb29050e3337c4445cc
Reviewed-on: https://boringssl-review.googlesource.com/3926
Reviewed-by: Adam Langley <agl@google.com>
Beyond generally eliminating unnecessary includes, eliminate as many
includes of headers that declare/define particularly error-prone
functionality like strlen, malloc, and free. crypto/err/internal.h was
added to remove the dependency on openssl/thread.h from the public
openssl/err.h header. The include of <stdlib.h> in openssl/mem.h was
retained since it defines OPENSSL_malloc and friends as macros around
the stdlib.h functions. The public x509.h, x509v3.h, and ssl.h headers
were not changed in order to minimize breakage of source compatibility
with external code.
Change-Id: I0d264b73ad0a720587774430b2ab8f8275960329
Reviewed-on: https://boringssl-review.googlesource.com/4220
Reviewed-by: Adam Langley <agl@google.com>
The only dependency the low-level crypto modules have on code in
crypto/obj is their use of OBJ_nid2sn, which is trivial to avoid.
This facilitates future simplification of crypto/obj, including
possibly the removal of functions like OBJ_nid2sn and the complex
build infrastructure that supports them.
This change also removes EVP_CIPHER_name and EVP_MD_name.
Change-Id: I34ce7dc7e58d5c08b52f95d25eba3963590cf2f7
Reviewed-on: https://boringssl-review.googlesource.com/3932
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>
Avoiding superflous references to RC4 makes it easier to audit the code
to find unsafe uses of it. It also avoids subtly encouraging users to
choose RC4 instead of a better alternative.
Change-Id: Ia27d7f4cd465e143d30a28b36c7871f7c30411ea
Reviewed-on: https://boringssl-review.googlesource.com/3990
Reviewed-by: Adam Langley <agl@google.com>
These are all masks of some sort (except id which is a combined version and
cipher), so they should use fixed-size unsigned integers.
Change-Id: I058dd8ad231ee747df4b4fb17d9c1e2cbee21918
Reviewed-on: https://boringssl-review.googlesource.com/4283
Reviewed-by: Adam Langley <agl@google.com>
We shouldn't be wrapping system headers.
Change-Id: I77498f4ec869797050b276eb764d892f73782f9f
Reviewed-on: https://boringssl-review.googlesource.com/4282
Reviewed-by: Adam Langley <agl@google.com>
Match the other internal headers.
Change-Id: Iff7e2dd06a1a7bf993053d0464cc15638ace3aaa
Reviewed-on: https://boringssl-review.googlesource.com/4280
Reviewed-by: Adam Langley <agl@google.com>
These are the remaining untested cipher suites. Rather than add support in
runner.go, just remove them altogether. Grepping for this is a little tricky,
but nothing enables aNULL (all occurrences disable it), and all occurrences of
["ALL:] seem to be either unused or explicitly disable anonymous ciphers.
Change-Id: I4fd4b8dc6a273d6c04a26e93839641ddf738343f
Reviewed-on: https://boringssl-review.googlesource.com/4258
Reviewed-by: Adam Langley <agl@google.com>
Now that ERR is using thread-local storage, there's very little that the
THREADID code is doing and it can be turned into stub functions.
Change-Id: I668613fec39b26c894d029b10a8173c3055f6019
Since ERR will soon have thread-local storage, we don't need to worry
about high-performance implementations and thus don't need to be able to
switch two different implementations at run-time.
Change-Id: I0598054ee8a8b499ac686ea635a96f5d03c754e0
Amazingly, asn1_GetSequence isn't completely unused? Keep that around for now
and ditch everything else. This lets us enable C4311 in MSVC which is actually
a pretty reasonable warning.
Change-Id: I43bb9206b1745e8a68224f3a435713d2a74e04ea
Reviewed-on: https://boringssl-review.googlesource.com/4256
Reviewed-by: Adam Langley <agl@google.com>
The unused ex_data index declarations are commented out instead of
removed so that it is clear which values to avoid for any new ex_data
indexes added in the future.
Change-Id: Ia19da9631324492c5c7eeacc71453e6240c73870
Reviewed-on: https://boringssl-review.googlesource.com/3940
Reviewed-by: Adam Langley <agl@google.com>
These functions are useful for implementing non-ASN.1-based protocols
like JSON Web Signature (JWS) and they are even already used within
Chromium.
Change-Id: I58f41ca7beedc5a0b7a8c3da53f319aadff4c0e7
Reviewed-on: https://boringssl-review.googlesource.com/3936
Reviewed-by: Adam Langley <agl@google.com>
decrepit will contain algorithms that we really wish didn't exist any
longer. It won't be built by default in Chromium etc, but the code
will exist for crummy code that still needs it.
Change-Id: Ic307f5f0a69efe9e0a5fd54052f49d219e90dcdd
Sadly, it turns out that we have need of this, at least for now. The
code is taken from upstream and changed only as much as needed.
This only imports keys and doesn't know how to actually perform
operations on them for now.
Change-Id: I0db70fb938186cb7a91d03f068b386c59ed90b84
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>
It was only ever enabled for handshake and alert messages. The comments cite
renego as a use case though even then I'm not clear on why. The only use I see
is if, say, the Finished message and ClientKeyExchange came in out-of-order.
DTLS is unreliable so leaning on retransmit seems fine, and usually flights
will be packed into one packet where possible. NSS doesn't have any such
buffer and doesn't seem to have problems.
The buffering mechanism is also rather dubious. It stows away the entire packet
and read buffer---all 16K of it---and there may have been other records in that
packet.
Change-Id: Ic3b7bf817be380dc73102eec62c690ed093e6667
Reviewed-on: https://boringssl-review.googlesource.com/4238
Reviewed-by: Adam Langley <agl@google.com>
Compression is gone, so don't allow for compression overhead. With that fixed,
the second rr->length check in ssl3_get_record matches the length computation
which sizes the read buffer. The first is wrong and doesn't account for the
alignment padding. Move the second to the first.
Change-Id: I3f4f05de9fdf5c645ff24493bbfdf303dcc1aa90
Reviewed-on: https://boringssl-review.googlesource.com/4236
Reviewed-by: Adam Langley <agl@google.com>
Also check for overflow, although it really shouldn't happen.
Change-Id: I34dfe8eaf635aeaa8bef2656fda3cd0bad7e1268
Reviewed-on: https://boringssl-review.googlesource.com/4235
Reviewed-by: Adam Langley <agl@google.com>
Fix up the variable names. Also avoid the messy logic of checking whether the
label and context collide with the normal key expansion ones in the face of
adverserial inputs. Make that the caller's responsibility, just as it's already
the caller's responsibility to ensure that different calls don't overlap. (The
label should be a constant string in an IANA registry anyway.)
Change-Id: I062fadb7b6a18fa946b883be660ea9b3f0f6277c
Reviewed-on: https://boringssl-review.googlesource.com/4216
Reviewed-by: Adam Langley <agl@google.com>
Separate actually writing the fragment to the network from assembling it so
there is no need for is_fragment. record_split_done also needn't be a global;
as of 7fdeaf1101, it is always reset to 0 whether
or not SSL3_WANT_WRITE occurred, despite the comment.
I believe this is sound, but the pre-7fdeaf1 logic wasn't quiiite right;
ssl3_write_pending allows a retry to supply *additional* data, so not all
plaintext had been commited to before the IV was randomized. We could fix this
by tracking how many bytes were committed to the last time we fragmented, but
this is purely an optimization and doesn't seem worth the complexity.
This also fixes the alignment computation in the record-splitting case. The
extra byte was wrong, as demonstrated by the assert.
Change-Id: Ia087a45a6622f4faad32e501942cc910eca1237b
Reviewed-on: https://boringssl-review.googlesource.com/4234
Reviewed-by: Adam Langley <agl@google.com>
There's multiple sets of APIs for selecting the curve. Fold away
SSL_OP_SINGLE_ECDH_USE as failing to set it is either a no-op or a bug. With
that gone, the consumer only needs to control the selection of a curve, with
key generation from then on being uniform. Also clean up the interaction
between the three API modes in s3_srvr.c; they were already mutually exclusive
due to tls1_check_ec_tmp_key.
This also removes all callers of EC_KEY_dup (and thus CRYPTO_dup_ex_data)
within the library.
Change-Id: I477b13bd9e77eb03d944ef631dd521639968dc8c
Reviewed-on: https://boringssl-review.googlesource.com/4200
Reviewed-by: Adam Langley <agl@google.com>
Along the way, fix a host of missing failure checks. This will save some
headache when it comes time to run these under the malloc failure tests.
Change-Id: I3fd589bd094178723398e793d6bc578884e99b67
Reviewed-on: https://boringssl-review.googlesource.com/4126
Reviewed-by: Adam Langley <agl@google.com>
MIPS64 confusingly sets __mips__, but it's not a 32-bit platform. This
change updates the defines in base.h to recognise MIPS64 based on both
__mips__ and __LP64__ being defined.
Change-Id: I220f5d9c8f1cd7d3089cc013348e6f95cdee76d9
Reviewed-on: https://boringssl-review.googlesource.com/4093
Reviewed-by: Adam Langley <agl@google.com>
(system/keymaster is using them now.)
Change-Id: I8fba501005b9318b7d3a76bf1715fb772b23c49d
Reviewed-on: https://boringssl-review.googlesource.com/4092
Reviewed-by: Adam Langley <agl@google.com>
It's not actually CRYPTO_add_locked, despite the name. I guess they just needed
a name that didn't clash with CRYPTO_add.
Change-Id: I3fdee08bf75e9a4e1b5e75630707c0be5792599b
Reviewed-on: https://boringssl-review.googlesource.com/4102
Reviewed-by: Adam Langley <agl@google.com>
Within the library, only ssl_update_cache read them, so add a dedicated field
to replace that use.
The APIs have a handful of uninteresting callers so I've left them in for now,
but they now always return zero.
Change-Id: Ie4e36fd4ab18f9bff544541d042bf3c098a46933
Reviewed-on: https://boringssl-review.googlesource.com/4101
Reviewed-by: Adam Langley <agl@google.com>
Align with upstream's renames from a while ago. These names are considerably
more standard. This also aligns with upstream in that both "ECDHE" and "EECDH"
are now accepted in the various cipher string parsing bits.
Change-Id: I84c3daeacf806f79f12bc661c314941828656b04
Reviewed-on: https://boringssl-review.googlesource.com/4053
Reviewed-by: Adam Langley <agl@google.com>
(Thanks to William Hesse.)
Change-Id: I8479663250546a5ec0a024f80e50541f91d833bc
Reviewed-on: https://boringssl-review.googlesource.com/4020
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
They do not quite measure the same value for EC keys. "size" is a really weird
notion to generalize on so we should document what it means for each key type.
EVP_PKEY_size's meaning is most tied to signatures, thanks to EVP_SignFinal
implicitly using it as output bounds.
Change-Id: I7504c142818f8f90f8bcf6891c97a6adaf2d574e
Reviewed-on: https://boringssl-review.googlesource.com/4000
Reviewed-by: Adam Langley <agl@google.com>
C99 doesn't, technically, allow empty statements. Thus if a #define'ed
function ends in a semicolon, and the use of it also ends in a
semicolon, then the compiler sees “;;” at the end.
Since a choice has to be made, I prefer that the semicolon exist at the
“callsite” of a #define'ed fuction. But I haven't gone and changed
everything to follow that in this patch.
Change-Id: I1343e52a5ac6255db49aa053048d0df3225bcf43
Reviewed-on: https://boringssl-review.googlesource.com/3890
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
At least the linker can discard this function in the cases where nobody
is calling it.
Change-Id: I30050e918e6bc1dd9c97cc70f3a56408701abebc
Reviewed-on: https://boringssl-review.googlesource.com/3724
Reviewed-by: Adam Langley <agl@google.com>
Because NTLM authentication is still a thing.
Change-Id: I3308a8431c82f0b614e09ce3e5efac1526881f1e
Reviewed-on: https://boringssl-review.googlesource.com/3723
Reviewed-by: Adam Langley <agl@google.com>
This allows the current RC4 state of an SSL* to be extracted. We have
internal uses for this functionality.
Change-Id: Ic124c4b253c8325751f49e7a4c021768620ea4b7
Reviewed-on: https://boringssl-review.googlesource.com/3722
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>
* Eliminate the possibility of multiple lock IDs having the same
value (CRYPTO_LOCK_FIPS2 and CRYPTO_LOCK_OBJ were both 40 prior to
this commit).
* Remove unused lock IDs.
* Automatically guarantee that lock IDs and lock names stay in sync.
Change-Id: If20e462db1285fa891595a7e52404ad011ff16f6
Reviewed-on: https://boringssl-review.googlesource.com/3923
Reviewed-by: Adam Langley <agl@google.com>
No code within BoringSSL or Google (grep for EVP_PKEY_CTX_(ctrl|get|set)) is
sensitive to the various failure cases. Normalize it all to 0/1 for simplicity.
This does carry a slight risk: any new ctrl hooks we import from upstream that,
like EVP_PKEY_CTX_get_rsa_oaep_md, return something other than success/failure
cannot be called directly via EVP_PKEY_CTX_ctrl. They instead need to
internally be routed through a struct like CBS and only called through the
wrappers. To that end, unexport EVP_PKEY_CTX_ctrl and require that callers use
the wrappers. No code in Google uses it directly and, if need be, switching to
the wrapper would be an incredibly upstreamable patch.
Change-Id: I3fd4e5a1a0f3d4d1c4122c52d4c74a5105b99cd5
Reviewed-on: https://boringssl-review.googlesource.com/3874
Reviewed-by: Adam Langley <agl@google.com>
This is the only EVP_PKEY ctrl hook which returns something other than a
boolean.
Change-Id: Ic226aef168abdf72e5d30e8264a559ed5039a055
Reviewed-on: https://boringssl-review.googlesource.com/3873
Reviewed-by: Adam Langley <agl@google.com>
This removes another place where we're internally sensitive to the
success/failure conditions.
Change-Id: I18fecf6457e841ba0afb718397b9b5fd3bbdfe4c
Reviewed-on: https://boringssl-review.googlesource.com/3872
Reviewed-by: Adam Langley <agl@google.com>
All EVP_PKEY types return 1 on that. (It can go away entirely when
EVP_PKEY_HMAC is gone.) This removes a place internally where we're sensitive
to the failure code.
Change-Id: Ic6cda2da9337ba7ef1c66a18e40c5dcc44fcf840
Reviewed-on: https://boringssl-review.googlesource.com/3871
Reviewed-by: Adam Langley <agl@google.com>
Turn them into static functions that take in an hm_fragment. It's not
immediately obvious that the frag_off/frag_len bounds checks and the msg_len
consistency check are critical to avoiding an out-of-bounds write. Better to
have dtls1_hm_fragment_mark also check internally.
Also rework the bitmask logic to be clearer and avoid a table.
Change-Id: Ica54e98f66295efb323e033cb6c67ab21e7d6cbc
Reviewed-on: https://boringssl-review.googlesource.com/3765
Reviewed-by: Adam Langley <agl@google.com>
Replace unsigned long with the appropriate sized integer type.
Change-Id: I7b4641d84568f6c11efa25350a9e488a556fc92e
Reviewed-on: https://boringssl-review.googlesource.com/3766
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>
It happens to give the same value anyway (64 + 16), but only on accident.
Change-Id: I1415f4015e3de472dbeb9ada0d92607c9d1bcd40
Reviewed-on: https://boringssl-review.googlesource.com/3780
Reviewed-by: Adam Langley <agl@google.com>
This transcription bug comes from the start of BoringSSL and, as you can
imagine, was a complete delight to track down.
Change-Id: I3051934195098a1d3bf893b154389ec7f14d3609
Reviewed-on: https://boringssl-review.googlesource.com/3740
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Instead, add a separate init_with_direction hook. Normal AEADs ignore the
direction, while legacy AEADs must be initialized with it. This avoids
maintaining extra state to support the delayed initialization.
Change-Id: I25271f0e56ee2783a2fd4d4026434154d58dc0a8
Reviewed-on: https://boringssl-review.googlesource.com/3731
Reviewed-by: Adam Langley <agl@google.com>
There's no good reason to do this, and it doesn't work; HMAC checks the length
of the key and runs it through the hash function if too long. The reuse occurs
after this check.
This allows us to shave 132 bytes off HMAC_CTX as this was the only reason it
ever stored the original key. It also slightly simplifies HMAC_Init_ex's
logic.
Change-Id: Ib56aabc3630b7178f1ee7c38ef6370c9638efbab
Reviewed-on: https://boringssl-review.googlesource.com/3733
Reviewed-by: Adam Langley <agl@google.com>
(There are times when I actually miss C++ templates.)
Change-Id: I3db56e4946ae4fb919105fa33e2cfce3c7542d37
Reviewed-on: https://boringssl-review.googlesource.com/3700
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I7b6acc9004beb7b7090de1837814ccdff2e9930e
Reviewed-on: https://boringssl-review.googlesource.com/3680
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
EC_GROUP_copy is an rather unfriendly function; it doesn't work if the groups
have different[*] underlying EC_METHODs, but this notion is not exposed through
the API. I found no callers of EC_GROUP_copy in external code.
This leaves the precompute_mult functions as the remaining mutable API exposed
through EC_GROUP.
[*] Though, of the two EC_METHODs right now, simple.c is entirely unused.
Change-Id: Iabb52518005250fb970e12b3b0ea78b4f6eff4a0
Reviewed-on: https://boringssl-review.googlesource.com/3631
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>
(I got this wrong when reading the OpenSSL code.)
Change-Id: Ib289ef41d0ab5a3157ad8b9454d2de96d1f86c22
Reviewed-on: https://boringssl-review.googlesource.com/3620
Reviewed-by: Adam Langley <agl@google.com>
There is exactly one implementation and it doesn't fail. Plus a cleanup
function that can fail is very bad manners; the caller has no choice but to
leak at that point.
Change-Id: I5b524617ef37bc7d92273472fa742416ea7dfd43
Reviewed-on: https://boringssl-review.googlesource.com/3564
Reviewed-by: Adam Langley <agl@google.com>
RC4_CHAR is a bit in the x86(-64) CPUID information that switches the
RC4 asm code from using an array of 256 uint32_t's to 256 uint8_t's. It
was originally written for the P4, where the uint8_t style was faster.
(On modern chips, setting RC4_CHAR took RC4-MD5 from 458 to 304 MB/s.
Although I wonder whether, on a server with many connections, using less
cache wouldn't be better.)
However, I'm not too worried about a slowdown of RC4 on P4 systems these
days (the last new P4 chip was released nine years ago) and I want the
code to be simplier.
Also, RC4_CHAR was set when the CPUID family was 15, but Intel actually
lists 15 as a special code meaning "also check the extended family
bits", which the asm didn't do.
The RC4_CHAR support remains in the RC4 asm code to avoid drift with
upstream.
Change-Id: If3febc925a83a76f453b9e9f8de5ee43759927c6
Reviewed-on: https://boringssl-review.googlesource.com/3550
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
(Which is just an exported wrapper around ssl3_get_cipher_by_value.)
Change-Id: Ibba166015ce59e337ff50963ba20237ac4949aaf
Reviewed-on: https://boringssl-review.googlesource.com/3543
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>
