Then deprecate the old functions. Thanks to upstream's
6977e8ee4a718a76351ba5275a9f0be4e530eab5 for the idea.
Change-Id: I916abd6fca2a3b2a439ec9902d9779707f7e41eb
Reviewed-on: https://boringssl-review.googlesource.com/6622
Reviewed-by: Adam Langley <agl@google.com>
It has no callers. I prepped for its removal earlier with
c05697c2c5
and then completely forgot.
Thanks to upstream's 6f78b9e824c053d062188578635c575017b587c5 for
the reminder. Quoth them:
> This only gets used to set a specific curve without actually checking
> that the peer supports it or not and can therefor result in handshake
> failures that can be avoided by selecting a different cipher.
It's also a very confusing API since it does NOT pass ownership of the
EC_KEY to the caller.
Change-Id: I6a00643b3a2d6746e9e0e228b47c2bc9694b0084
Reviewed-on: https://boringssl-review.googlesource.com/6621
Reviewed-by: Adam Langley <agl@google.com>
Cover not just the wrong version, but also other mistakes.
Change-Id: I46f05a9a37b7e325adc19084d315a415777d3a46
Reviewed-on: https://boringssl-review.googlesource.com/6610
Reviewed-by: Adam Langley <agl@google.com>
This is a remnant of ssl3_get_client_hello's old DTLS cookie logic, which has
since been removed. (If we ever need HelloVerifyRequest support on the server,
we'll implement something stateless in front.) We can switch this to something
more straightforward now.
See also upstream's 94f98a9019e1c0a3be4ca904b2c27c7af3d937c0,
Change-Id: Ie733030209a381a4915d6744fa12a79ffe972fa5
Reviewed-on: https://boringssl-review.googlesource.com/6601
Reviewed-by: Adam Langley <agl@google.com>
I don't think we're ever going to manage to enforce this, and it doesn't
seem worth the trouble. We don't support application protocols which use
renegotiation outside of the HTTP/1.1 mid-stream client auth hack.
There, it's on the server to reject legacy renegotiations.
This removes the last of SSL_OP_ALL.
Change-Id: I996fdeaabf175b6facb4f687436549c0d3bb0042
Reviewed-on: https://boringssl-review.googlesource.com/6580
Reviewed-by: Adam Langley <agl@google.com>
RFC 5746 forbids a server from downgrading or upgrading
renegotiation_info support. Even with SSL_OP_LEGACY_SERVER_CONNECT set
(the default), we can still enforce a few things.
I do not believe this has practical consequences. The attack variant
where the server half is prefixed does not involve a renegotiation on
the client. The converse where the client sees the renegotiation and
prefix does, but we only support renego for the mid-stream HTTP/1.1
client auth hack, which doesn't do this. (And with triple-handshake,
HTTPS clients should be requiring the certificate be unchanged across
renego which makes this moot.)
Ultimately, an application which makes the mistake of using
renegotiation needs to be aware of what exactly that means and how to
handle connection state changing mid-stream. We make renego opt-in now,
so this is a tenable requirement.
(Also the legacy -> secure direction would have been caught by the
server anyway since we send a non-empty RI extension.)
Change-Id: I915965c342f8a9cf3a4b6b32f0a87a00c3df3559
Reviewed-on: https://boringssl-review.googlesource.com/6559
Reviewed-by: Adam Langley <agl@google.com>
This dates to SSLeay 0.8.0 (or earlier). The use counter sees virtually
no hits.
Change-Id: Iff4c8899d5cb0ba4afca113c66d15f1d980ffe41
Reviewed-on: https://boringssl-review.googlesource.com/6558
Reviewed-by: Adam Langley <agl@google.com>
This dates to SSLeay 0.9.0. The Internet seems to have completely
forgotten what "D5" is. (I can't find reference to it beyond
documentation of this quirk.) The use counter we added sees virtually no
hits.
Change-Id: I9781d401acb98ce3790b1b165fc257a6f5e9b155
Reviewed-on: https://boringssl-review.googlesource.com/6557
Reviewed-by: Adam Langley <agl@google.com>
At least for newlib (Native Client) including sys/types.h
is not enough to get a timeval declaration.
Change-Id: I4971a1aacc80b6fdc12c0e81c5d8007ed13eb8b7
Reviewed-on: https://boringssl-review.googlesource.com/6722
Reviewed-by: Adam Langley <agl@google.com>
BUG=webrtc:5222
Change-Id: I8399bd595564dedbe5492b8ea6eb915f41367cbf
Reviewed-on: https://boringssl-review.googlesource.com/6690
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
yaSSL has a couple of bugs in their DH client implementation. This
change works around the worst of the two.
Firstly, they expect the the DH public value to be the same length as
the prime. This change pads the public value as needed to ensure this.
Secondly, although they handle the first byte of the shared key being
zero, they don't handle the case of the second, third, etc bytes being
zero. So whenever that happens the handshake fails. I don't think that
there's anything that we can do about that one.
Change-Id: I789c9e5739f19449473305d59fe5c3fb9b4a6167
Reviewed-on: https://boringssl-review.googlesource.com/6578
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Right now your options are:
- Bounce on a reference and deal with cleanup needlessly.
- Manually check the type tag and peek into the union.
We probably have no hope of opaquifying this struct, but for new code, let's
recommend using this function rather than the more error-prone thing.
Change-Id: I9b39ff95fe4264a3f7d1e0d2894db337aa968f6c
Reviewed-on: https://boringssl-review.googlesource.com/6551
Reviewed-by: Adam Langley <agl@google.com>
clang-format packing them tightly made newlines inconsistent which
wasn't very helpful.
Change-Id: I46a787862ed1f5b0eee101394e24c779b6bc652b
Reviewed-on: https://boringssl-review.googlesource.com/6517
Reviewed-by: Adam Langley <agl@google.com>
Trim the cipher table further. Those values are entirely determined by
algorithm_enc.
Change-Id: I355c245b0663e41e54e62d15903a4a9a667b4ffe
Reviewed-on: https://boringssl-review.googlesource.com/6516
Reviewed-by: Adam Langley <agl@google.com>
FIPS is the same as HIGH (but for CHACHA20), so those are redundant.
Likewise, MEDIUM vs HIGH was just RC4. Remove those in favor of
redefining those legacy rules to mean this.
One less field to keep track of in each cipher.
Change-Id: I2b2489cffb9e16efb0ac7d7290c173cac061432a
Reviewed-on: https://boringssl-review.googlesource.com/6515
Reviewed-by: Adam Langley <agl@google.com>
It's redundant with other cipher properties. We can express these in code.
Cipher rule matching gets a little bit complicated due to the confusing legacy
protocol version cipher rules, so add some tests for it. (It's really hard to
grep for uses of them, so I've kept them working to be safe.)
Change-Id: Ic6b3fcd55d76d4a51b31bf7ae629a2da50a7450e
Reviewed-on: https://boringssl-review.googlesource.com/6453
Reviewed-by: Adam Langley <agl@google.com>
The keylog BIO is internally synchronized by the SSL_CTX lock, but an
application may wish to log keys from multiple SSL_CTXs. This is in
preparation for switching Chromium to use a separate SSL_CTX per profile
to more naturally split up the session caches.
It will also be useful for routing up SSLKEYLOGFILE in WebRTC. There,
each log line must be converted to an IPC up from the renderer
processes.
This will require changes in Chromium when we roll BoringSSL.
BUG=458365,webrtc:4417
Change-Id: I2945bdb4def0a9c36e751eab3d5b06c330d66b54
Reviewed-on: https://boringssl-review.googlesource.com/6514
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I3072f884be77b8646e90d316154b96448f0cf2a1
Reviewed-on: https://boringssl-review.googlesource.com/6520
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
So long as we're not getting rid of them (the certificate variants may
be useful when we decouple from crypto/x509 anyway), get the types and
bounds checks right.
Also reject trailing data and require the input be a single element.
Note: this is a slight compatibility risk, but we did it for
SSL*_use_RSAPrivateKey_ASN1 previously and I think it's probably worth
seeing if anything breaks here.
Change-Id: I64fa3fc6249021ccf59584d68e56ff424a190082
Reviewed-on: https://boringssl-review.googlesource.com/6490
Reviewed-by: Adam Langley <agl@google.com>
This codepath should not actually be reachable, unless maybe the caller is
doing something really dumb. (Unconfiguring the key partway through the
connection.)
Change-Id: Ic8e0cfc3c426439016370f9a85be9c05509358f1
Reviewed-on: https://boringssl-review.googlesource.com/6483
Reviewed-by: Adam Langley <agl@google.com>
TLS resets it in t1_enc.c while DTLS has it sprinkled everywhere.
Change-Id: I78f0f0e646b4dc82a1058199c4b00f2e917aa5bc
Reviewed-on: https://boringssl-review.googlesource.com/6511
Reviewed-by: Adam Langley <agl@google.com>
Until we've done away with the d2i_* stack completely, boundaries need
to be mindful of the type mismatch. d2i_* takes a long, not a size_t.
Change-Id: If02f9ca2cfde02d0929ac18275d09bf5df400f3a
Reviewed-on: https://boringssl-review.googlesource.com/6491
Reviewed-by: Adam Langley <agl@google.com>
There's a few things that will be kind of a nuisance and possibly not worth it
(crypto/asn1 dumps a lot of undeclared things, etc.). But it caught some
mistakes. Even without the warning, making sure to include the externs before
defining a function helps catch type mismatches.
Change-Id: I3dab282aaba6023e7cebc94ed7a767a5d7446b08
Reviewed-on: https://boringssl-review.googlesource.com/6484
Reviewed-by: Adam Langley <agl@google.com>
Chromium's toolchains may now assume C++11 library support, so we may freely
use C++11 features. (Chromium's still in the process of deciding what to allow,
but we use Google's style guide directly, toolchain limitations aside.)
Change-Id: I1c7feb92b7f5f51d9091a4c686649fb574ac138d
Reviewed-on: https://boringssl-review.googlesource.com/6465
Reviewed-by: Adam Langley <agl@google.com>
dh.c had a 10k-bit limit but it wasn't quite correctly enforced. However,
that's still 1.12s of jank on the IO thread, which is too long. Since the SSL
code consumes DHE groups from the network, it should be responsible for
enforcing what sanity it needs on them.
Costs of various bit lengths on 2013 Macbook Air:
1024 - 1.4ms
2048 - 14ms
3072 - 24ms
4096 - 55ms
5000 - 160ms
10000 - 1.12s
UMA says that DHE groups are 0.2% 4096-bit and otherwise are 5.5% 2048-bit and
94% 1024-bit and some noise. Set the limit to 4096-bit to be conservative,
although that's already quite a lot of jank.
BUG=554295
Change-Id: I8e167748a67e4e1adfb62d73dfff094abfa7d215
Reviewed-on: https://boringssl-review.googlesource.com/6464
Reviewed-by: Adam Langley <agl@google.com>
The current check has two problems:
- It only runs on the server, where there isn't a curve list at all. This was a
mistake in https://boringssl-review.googlesource.com/1843 which flipped it
from client-only to server-only.
- It only runs in TLS 1.2, so one could bypass it by just negotiating TLS 1.1.
Upstream added it as part of their Suite B mode, which requires 1.2.
Move it elsewhere. Though we do not check the entire chain, leaving that to the
certificate verifier, signatures made by the leaf certificate are made by the
SSL/TLS stack, so it's reasonable to check the curve as part of checking
suitability of a leaf.
Change-Id: I7c12f2a32ba946a20e9ba6c70eff23bebcb60bb2
Reviewed-on: https://boringssl-review.googlesource.com/6414
Reviewed-by: Adam Langley <agl@google.com>
There seems to have been a merge error.
Change-Id: I72e5c2a45c148e31c90b28bedfff48f8ca6e3c8c
Reviewed-on: https://boringssl-review.googlesource.com/6455
Reviewed-by: Adam Langley <agl@google.com>
This exposes the ServerKeyExchange signature hash type used in the most recent
handshake, for histogramming on the client.
BUG=549662
Change-Id: I8a4e00ac735b1ecd2c2df824112c3a0bc62332a7
Reviewed-on: https://boringssl-review.googlesource.com/6413
Reviewed-by: Adam Langley <agl@google.com>
This is completely a no-op as currently tls12_get_psigalgs always returns a
hardcoded list which always includes SHA-1. But if this were to be made
configurable in the future, we should reject SHA-1 when configured to do so.
Change-Id: I7ab188eeff850d1e5f70b9522304812bab2d941a
Reviewed-on: https://boringssl-review.googlesource.com/6411
Reviewed-by: Adam Langley <agl@google.com>
This fixes a number of bugs with the original logic:
- If handshake messages are fragmented and writes need to be retried, frag_off
gets completely confused.
- The BIO_flush call didn't set rwstate, so it wasn't resumable at that point.
- The msg_callback call gets garbage because the fragment header would get
scribbled over the handshake buffer.
The original logic was also extremely confusing with how it handles init_off.
(init_off gets rewound to make room for the fragment header. Depending on
where you pause, resuming may or may not have already been rewound.)
For simplicity, just allocate a new buffer to assemble the fragment in and
avoid clobbering the old one. I don't think it's worth the complexity to
optimize that. If we want to optimize this sort of thing, not clobbering seems
better anyway because the message may need to be retransmitted. We could avoid
doing a copy when buffering the outgoing message for retransmission later.
We do still need to track how far we are in sending the current message via
init_off, so I haven't opted to disconnect this function from
init_{buf,off,num} yet.
Test the fix to the retry + fragment case by having the splitHandshake option
to the state machine tests, in DTLS, also clamp the MTU to force handshake
fragmentation.
Change-Id: I66f634d6c752ea63649db8ed2f898f9cc2b13908
Reviewed-on: https://boringssl-review.googlesource.com/6421
Reviewed-by: Adam Langley <agl@google.com>
This was a mistake from when we added async CertificateVerify support.
No test because the final state of each write state is semi-unreachable
due to the buffer BIO that gets installed on each handshake.
Change-Id: I0180926522113c8b1ca58b8c9c6dc37fb0dd8083
Reviewed-on: https://boringssl-review.googlesource.com/6412
Reviewed-by: Adam Langley <agl@google.com>
Later when TLS 1.3 comes around, we'll need SSL_CIPHER_get_max_version too. In
the meantime, hide the SSL_TLSV1_2 messiness behind a reasonable API.
Change-Id: Ibcc17cccf48dd99e364d6defdfa5a87d031ecf0a
Reviewed-on: https://boringssl-review.googlesource.com/6452
Reviewed-by: Adam Langley <agl@google.com>
This function allows one to extract the current IVs from an SSL
connection. This is needed for the CBC cipher suites with implicit IVs
because, for those, the IV can't be extracted from the handshake key
material.
Change-Id: I247a1d0813b7a434b3cfc88db86d2fe8754344b6
Reviewed-on: https://boringssl-review.googlesource.com/6433
Reviewed-by: Adam Langley <agl@google.com>
They run through completely different logic as only handshake is fragmented.
This'll make it easier to rewrite the handshake logic in a follow-up.
Change-Id: I9515feafc06bf069b261073873966e72fcbe13cb
Reviewed-on: https://boringssl-review.googlesource.com/6420
Reviewed-by: Adam Langley <agl@google.com>
This option causes clients to ignore HelloRequest messages completely.
This can be suitable in cases where a server tries to perform concurrent
application data and handshake flow, e.g. because they are trying to
“renew” symmetric keys.
Change-Id: I2779f7eff30d82163f2c34a625ec91dc34fab548
Reviewed-on: https://boringssl-review.googlesource.com/6431
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
That function doesn't do anything useful for DTLS. It's meant for tracking the
rest of the record we've already committed to by writing half of one. But one
cannot write half a datagram, so DTLS never tracks this. Just call
ssl_write_buffer_flush straight and don't touch wpend_*.
Change-Id: Ibe191907d64c955c7cfeefba26f5c11ad5e4b939
Reviewed-on: https://boringssl-review.googlesource.com/6418
Reviewed-by: Adam Langley <agl@google.com>
Although the DTLS transport layer logic drops failed writes on the floor, it is
actually set up to work correctly. If an SSL_write fails at the transport,
dropping the buffer is fine. Arguably it works better than in TLS because we
don't have the weird "half-committed to data" behavior. Likewise, the handshake
keeps track of how far its gotten and resumes the message at the right point.
This broke when the buffering logic was rewritten because I didn't understand
what the DTLS code was doing. The one thing that doesn't work as one might
expect is non-fatal write errors during rexmit are not recoverable. The next
timeout must fire before we try again.
This code is quite badly sprinkled in here, so add tests to guard it against
future turbulence. Because of the rexmit issues, the tests need some hacks
around calls which may trigger them. It also changes the Go DTLS implementation
from being completely strict about sequence numbers to only requiring they be
monotonic.
The tests also revealed another bug. This one seems to be upstream's fault, not
mine. The logic to reset the handshake hash on the second ClientHello (in the
HelloVerifyRequest case) was a little overenthusiastic and breaks if the
ClientHello took multiple tries to send.
Change-Id: I9b38b93fff7ae62faf8e36c4beaf848850b3f4b9
Reviewed-on: https://boringssl-review.googlesource.com/6417
Reviewed-by: Adam Langley <agl@google.com>
It's somewhat annoying to have to parse out the packetAdaptor mini-language.
Actually seeing those is only useful when debugging the adaptor itself, rather
than DTLS. Switch the order of the two middleware bits and add an escape hatch
to log the funny opcodes.
Change-Id: I249c45928a76b747d69f3ab972ea4d31e0680a62
Reviewed-on: https://boringssl-review.googlesource.com/6416
Reviewed-by: Adam Langley <agl@google.com>
89d4a68c introduced -Wmissing-field-initializers because it seemed
generally useful. However, Clang and GCC have differing opinions about
what counts as missing. This change should make Clang happy too.
Change-Id: I070c719f5c47f537207200d5399e093cc083e58f
This is a fairly timid, first step at trying to pack common structures a
little better.
This change reorders a couple of structures a little and turns some
variables into bit-fields. Much more can still be done.
Change-Id: Idbe0f54d66559c0ad654bf7e8dea277a771a568f
Reviewed-on: https://boringssl-review.googlesource.com/6394
Reviewed-by: Adam Langley <agl@google.com>
This reverts most of commit 271777f5ac. The old
ChaCha20-Poly1305, though being transitioned to the old name, should not change
in behavior. This also avoids adding a special-case to SSL_AEAD_CTX.
Also revert the name change to SSL_CIPHER_is_CHACHA20POLY1305. The one consumer
for that function doesn't need to distinguish the old and new variants, so
avoid unnecessary turbulence.
Change-Id: I5a6f97fccc5839d4d25e74e304dc002329d21b4b
Reviewed-on: https://boringssl-review.googlesource.com/6385
Reviewed-by: Adam Langley <agl@google.com>
These functions ultimately return the result of |BN_num_bits|, and that
function's return type is |unsigned|. Thus, these functions' return
type should also be |unsigned|.
Change-Id: I2cef63e6f75425857bac71f7c5517ef22ab2296b
Reviewed-on: https://boringssl-review.googlesource.com/6170
Reviewed-by: Adam Langley <alangley@gmail.com>
This change reduces unnecessary copying and makes the pre-RFC-7539
nonces 96 bits just like the AES-GCM, AES-CCM, and RFC 7539
ChaCha20-Poly1305 cipher suites. Also, all the symbols related to
the pre-RFC-7539 cipher suites now have "_OLD" appended, in
preparation for adding the RFC 7539 variants.
Change-Id: I1f85bd825b383c3134df0b6214266069ded029ae
Reviewed-on: https://boringssl-review.googlesource.com/6103
Reviewed-by: Adam Langley <alangley@gmail.com>
It seems OS X actually cares about symbol resolution and dependencies
when you create a dylib. Probably because they do two-level name
resolution.
(Obligatory disclaimer: BoringSSL does not have a stable ABI and is thus
not suitable for a traditional system-wide library.)
BUG=539603
Change-Id: Ic26c4ad23840fe6c1f4825c44671e74dd2e33870
Reviewed-on: https://boringssl-review.googlesource.com/6131
Reviewed-by: Adam Langley <alangley@gmail.com>
It's very annoying having to remember the right incant every time I want
to switch around between my build, build-release, build-asan, etc.,
output directories.
Unfortunately, this target is pretty unfriendly without CMake 3.2+ (and
Ninja 1.5+). This combination gives a USES_TERMINAL flag to
add_custom_target which uses Ninja's "console" pool, otherwise the
output buffering gets in the way. Ubuntu LTS is still on an older CMake,
so do a version check in the meantime.
CMake also has its own test mechanism (CTest), but this doesn't use it.
It seems to prefer knowing what all the tests are and then tries to do
its own output management and parallelizing and such. We already have
our own runners. all_tests.go could actually be converted tidily, but
generate_build_files.py also needs to read it, and runner.go has very
specific needs.
Naming the target ninja -C build test would be nice, but CTest squats
that name and CMake grumps when you use a reserved name, so I've gone
with run_tests.
Change-Id: Ibd20ebd50febe1b4e91bb19921f3bbbd9fbcf66c
Reviewed-on: https://boringssl-review.googlesource.com/6270
Reviewed-by: Adam Langley <alangley@gmail.com>
Right whether NPN is advertised can only be configured globally on the SSL_CTX.
Rather than adding two pointers to each SSL*, add an options bit to disable it
so we may plumb in a field trial to disable NPN.
Chromium wants to be able to route a bit in to disable NPN, but it uses SSL_CTX
incorrectly and has a global one, so it can't disconnect the callback. (That
really needs to get fixed. Although it's not clear this necessarily wants to be
lifted up to SSL_CTX as far as Chromium's SSLClientSocket is concerned since
NPN doesn't interact with the session cache.)
BUG=526713
Change-Id: I49c86828b963eb341c6ea6a442557b7dfa190ed3
Reviewed-on: https://boringssl-review.googlesource.com/6351
Reviewed-by: Adam Langley <alangley@gmail.com>
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>
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'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>
Also clean up the code slightly.
Change-Id: I066a389242c46cdc7d41b1ae9537c4b7716c92a2
Reviewed-on: https://boringssl-review.googlesource.com/6302
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
21b538d616b388fa0ce64ef54da3504253895cf8.
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>
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>
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>
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>
Add a slightly richer API. Notably, one can configure ssl_renegotiate_once to
only accept the first renego.
Also, this API doesn't repeat the mistake I made with
SSL_set_reject_peer_renegotiations which is super-confusing with the negation.
Change-Id: I7eb5d534e3e6c553b641793f4677fe5a56451c71
Reviewed-on: https://boringssl-review.googlesource.com/6221
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL's BIO_get_fd returns the fd or -1, not a boolean.
Change-Id: I12a3429c71bb9c9064f9f91329a88923025f1fb5
Reviewed-on: https://boringssl-review.googlesource.com/6080
Reviewed-by: Adam Langley <agl@google.com>
SSL 3.0 used to have a nice and simple rule around extensions. They don't
exist. And then RFC 5746 came along and made this all extremely confusing.
In an SSL 3.0 server, rather than blocking ServerHello extension
emission when renegotiation_info is missing, ignore all ClientHello
extensions but renegotiation_info. This avoids a mismatch between local
state and the extensions with emit.
Notably if, for some reason, a ClientHello includes the session_ticket
extension, does NOT include renegotiation_info or the SCSV, and yet the
client or server are decrepit enough to negotiate SSL 3.0, the
connection will fail due to unexpected NewSessionTicket message.
See https://crbug.com/425979#c9 for a discussion of something similar
that came up in diagnosing https://poodle.io/'s buggy POODLE check.
This is analogous to upstream's
5a3d8eebb7667b32af0ccc3f12f314df6809d32d.
(Not supporting renego as a server in any form anyway, we may as well
completely ignore extensions, but then our extensions callbacks can't
assume the parse hooks are always called. This way the various NULL
handlers still function.)
Change-Id: Ie689a0e9ffb0369ef7a20ab4231005e87f32d5f8
Reviewed-on: https://boringssl-review.googlesource.com/6180
Reviewed-by: Adam Langley <agl@google.com>
Some code relies on OpenSSL's behavior where it allowed for NULL. But this time
add a comment so people don't think this is the convention for new functions.
BUG=538292
Change-Id: I66566e0e24566fafe17e05369276248be3b05591
Reviewed-on: https://boringssl-review.googlesource.com/6070
Reviewed-by: Adam Langley <agl@google.com>
This didn't actually break anything, but it does make session lookup
quite slow.
Change-Id: I13615e8ccf6a46683a21774eb7c073318ae8c28c
Reviewed-on: https://boringssl-review.googlesource.com/6054
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This change makes the runner tests (in ssl/test/runner) act like a
normal Go test rather than being a Go binary. This better aligns with
some internal tools.
Thus, from this point onwards, one has to run the runner tests with `go
test` rather than `go run` or `go build && ./runner`.
This will break the bots.
Change-Id: Idd72c31e8e0c2b7ed9939dacd3b801dbd31710dd
Reviewed-on: https://boringssl-review.googlesource.com/6009
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The cipher suite rules could also be anchored on SSL_TXT_* if desired. I
currently documented them in prose largely because SSL_TXT_* also
defines protocol version strings and those are weird; SSL_TXT_TLSV1_1
isn't even a cipher rule. (And, in fact, those are the only SSL_TXT_*
macros that we can't blindly remove. I found some code that #ifdef's the
version SSL_TXT_* macros to decide if version-locked SSL_METHODs are
available.)
Also they clutter the header. I was thinking maybe we should dump a lot
of the random constants into a separate undocumented header or perhaps
just unexport them.
I'm slightly torn on this though and could easily be convinced in the
other direction. (Playing devil's advocate, anchoring on SSL_TXT_* means
we're less likely to forget to document one so long as adding a
SSL_TXT_* macro is the convention.)
Change-Id: Ide2ae44db9d6d8f29c24943090c210da0108dc37
Reviewed-on: https://boringssl-review.googlesource.com/5962
Reviewed-by: Adam Langley <agl@google.com>
The Android system BoringSSL has a couple of changes:
∙ ChaCha20-Poly1305 is disabled because it's not an offical
cipher suite.
∙ P-521 is offered in the ClientHello.
These changes were carried in the Android BoringSSL repo directly. This
change upstreams them when BORINGSSL_ANDROID_SYSTEM is defined.
Change-Id: If3e787c6694655b56e7701118aca661e97a5f26c
Or at least group them together and make a passing attempt to document
them. The legacy X.509 stack itself remains largely untouched and most
of the parameters have to do with it.
Change-Id: I9e11e2ad1bbeef53478c787344398c0d8d1b3876
Reviewed-on: https://boringssl-review.googlesource.com/5942
Reviewed-by: Adam Langley <agl@google.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>
We wish to be able to detect the use of RC4 so that we can flag it and
investigate before it's disabled.
Change-Id: I6dc3a5d2211b281097531a43fadf08edb5a09646
Reviewed-on: https://boringssl-review.googlesource.com/5930
Reviewed-by: David Benjamin <davidben@chromium.org>
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>
Existing documentation was moved to the header, very slightly tweaked.
Change-Id: Ife3c2351e2d7e6a335854284f996918039414446
Reviewed-on: https://boringssl-review.googlesource.com/5897
Reviewed-by: Adam Langley <agl@google.com>
These were already documented, though some of the documentation was
expanded on slightly.
Change-Id: I04c6276a83a64a03ab9cce9b9c94d4dea9ddf638
Reviewed-on: https://boringssl-review.googlesource.com/5896
Reviewed-by: Adam Langley <agl@google.com>
SCT and OCSP are part of the session data and as such shouldn't be sent
again to the client when resuming.
Change-Id: Iaee3a3c4c167ea34b91504929e38aadee37da572
Reviewed-on: https://boringssl-review.googlesource.com/5900
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
ssl.h should be first. Also two lines after includes and the rest of the
file.
Change-Id: Icb7586e00a3e64170082c96cf3f8bfbb2b7e1611
Reviewed-on: https://boringssl-review.googlesource.com/5892
Reviewed-by: Adam Langley <agl@google.com>
Forgot to fix these when I fixed the headers.
Change-Id: Ie45e624abc993e16e2d5a872ef00dba9029a38df
Reviewed-on: https://boringssl-review.googlesource.com/5891
Reviewed-by: Adam Langley <agl@google.com>
This gets the documentation into the ssl.h documentation, and removes
one of the circularly-dependent headers hanging off ssl.h. Also fixes
some typos; there were a few instances of "SSL *ctx".
Change-Id: I2a41c6f518f4780af84d468ed220fe7b0b8eb0d3
Reviewed-on: https://boringssl-review.googlesource.com/5883
Reviewed-by: Adam Langley <agl@google.com>
Also switch to the new variable names (SSL_CTX *ctx, SSL *ssl,
SSL_SESSION *session) for all documented functions.
Change-Id: I15e15a703b96af1727601108223c7ce3b0691f1d
Reviewed-on: https://boringssl-review.googlesource.com/5882
Reviewed-by: Adam Langley <agl@google.com>
This is arguably more commonly queried connection information than the
tls-unique.
Change-Id: I1f080536153ba9f178af8e92cb43b03df37110b5
Reviewed-on: https://boringssl-review.googlesource.com/5874
Reviewed-by: Adam Langley <agl@google.com>
Just the stuff that has been pulled out into sections already.
Change-Id: I3da6bc61d79ccfe2b18d888075dc32026a656464
Reviewed-on: https://boringssl-review.googlesource.com/5873
Reviewed-by: Adam Langley <agl@google.com>
Unfortunately, these are also some of the worst APIs in the SSL stack.
I've tried to capture all the things they expose to the caller. 0 vs -1
is intentionally left unexpanded on for now. Upstream's documentation
says 0 means transport EOF, which is a nice idea but isn't true. (A lot
of random functions return 0 on error and pass it up to the caller.)
https://crbug.com/466303 tracks fixing that.
SSL_set_bio is intentionally documented to NOT be usable when they're
already configured. The function tries to behave in this case and even
with additional cases when |rbio| and/or |wbio| are unchanged, but this
is buggy. For instance, this will explode:
SSL_set_bio(ssl, bio1, bio1);
SSL_set_bio(ssl, bio2, SSL_get_wbio(ssl));
As will this, though it's less clear this is part of the API contract
due to SSL taking ownership.
SSL_set_bio(ssl, bio1, bio2);
SSL_set_bio(ssl, bio2, bio1);
It also tries to handle ssl->bbio already existing, but I doubt it quite
works. Hopefully we can drop ssl->bbio eventually. (Why is this so
complicated...)
Change-Id: I5f9f3043915bffc67e2ebd282813e04afbe076e6
Reviewed-on: https://boringssl-review.googlesource.com/5872
Reviewed-by: Adam Langley <agl@google.com>
Last set of changes didn't do that.
Change-Id: Iae24e75103529ce4d50099c5cbfbcef0e10ba663
Reviewed-on: https://boringssl-review.googlesource.com/5871
Reviewed-by: Adam Langley <agl@google.com>
clang scan-build found a memory leak if the overflow codepath in
dtls1_hm_fragment is hit. Along the way, tidy up that function.
Change-Id: I3c4b88916ee56ab3ab63f93d4a967ceae381d187
Reviewed-on: https://boringssl-review.googlesource.com/5870
Reviewed-by: Adam Langley <agl@google.com>
SSL_get_client_CA_list is one of those dreaded functions which may query either
configuration state or handshake state. Moreover, it does so based on
|ssl->server|, which may not be configured until later. Also check
|ssl->handshake_func| to make sure |ssl| is not in an indeterminate state.
This also fixes a bug where SSL_get_client_CA_list wouldn't work in DTLS due to
the incorrect |ssl->version| check.
Change-Id: Ie564dbfeecd2c8257fd6bcb148bc5db827390c77
Reviewed-on: https://boringssl-review.googlesource.com/5827
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ifa44fef160fc9d67771eed165f8fc277f28a0222
Reviewed-on: https://boringssl-review.googlesource.com/5840
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
It was checking algorithm_mac rather than algorithm_enc.
(Coincidentally, it gave the right answer if you compiled out the
ChaCha20 ciphers since SSL_AES128GCM and SSL_AEAD shared a value.)
Change-Id: I17047425ef7fabb98969144965d8db9528ef8497
Reviewed-on: https://boringssl-review.googlesource.com/5850
Reviewed-by: Adam Langley <agl@google.com>
The size of the stack caused by this object is problematic for systems
that have smaller stacks because they expect many threads.
Change-Id: Ib8f03741f9dd96bf474126f001947f879e50a781
Reviewed-on: https://boringssl-review.googlesource.com/5831
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: Adam Langley <agl@google.com>
There's not enough in that file to really justify its own file now.
Change-Id: I6130cfce6c40fe9d46aa83dd83e6f38d87fdcf64
Reviewed-on: https://boringssl-review.googlesource.com/5823
Reviewed-by: Adam Langley <agl@google.com>
Quite a lot of consumers of the SSL stack will never need to touch files from
the SSL stack, but enough do that we can't just ditch them. Toss that all into
their own file so a static linker can drop it.
Change-Id: Ia07de939889eb09e3ab16aebcc1b6869ca8b75a0
Reviewed-on: https://boringssl-review.googlesource.com/5820
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>
This change adds the ability to configure ciphers specifically for
TLS ≥ 1.0. This compliments the existing ability to specify ciphers
for TLS ≥ 1.1.
This is useful because TLS 1.0 is the first version not to suffer from
POODLE. (Assuming that it's implemented correctly[1].) Thus one might
wish to reserve RC4 solely for SSLv3.
[1] https://www.imperialviolet.org/2014/12/08/poodleagain.html
Change-Id: I774d5336fead48f03d8a0a3cf80c369692ee60df
Reviewed-on: https://boringssl-review.googlesource.com/5793
Reviewed-by: Adam Langley <agl@google.com>
SSL_CTX gets memset to zero, so many of the values needn't be explicitly
initialized.
Change-Id: I0e707a0dcc052cd6f0a5dc8d037400170bd75594
Reviewed-on: https://boringssl-review.googlesource.com/5812
Reviewed-by: Adam Langley <agl@google.com>
setup_key_block is called when the first CCS resolves, but for resumptions this
is the incoming CCS (see ssl3_do_change_cipher_spec). Rather than set
need_record_splitting there, it should be set in the write case of
tls1_change_cipher_state.
This fixes a crash from the new record layer code in resumption when
record-splitting is enabled. Tweak the record-splitting tests to cover this
case.
This also fixes a bug where renego from a cipher which does require record
splitting to one which doesn't continues splitting. Since version switches are
not allowed, this can only happen after a renego from CBC to RC4.
Change-Id: Ie4e1b91282b10f13887b51d1199f76be4fbf09ad
Reviewed-on: https://boringssl-review.googlesource.com/5787
Reviewed-by: Adam Langley <agl@google.com>
Note that DTLS treats oversized ciphertexts different from everything else.
Change-Id: I71cba69ebce0debdfc96a7fdeb2666252e8d28ed
Reviewed-on: https://boringssl-review.googlesource.com/5786
Reviewed-by: Adam Langley <agl@google.com>
If the two extensions select different next protocols (quite possible since one
is server-selected and the other is client-selected), things will break. This
matches the behavior of NSS (Firefox) and Go.
Change-Id: Ie1da97bf062b91a370c85c12bc61423220a22f36
Reviewed-on: https://boringssl-review.googlesource.com/5780
Reviewed-by: Adam Langley <agl@google.com>
The maximum buffer size computation wasn't quite done right in
ssl_buffer.c, so we were failing with BUFFER_TOO_SMALL for sufficiently
large records. Fix this and, as penance, add 103 tests.
(Test that we can receive maximum-size records in all cipher suites.
Also test SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER while I'm here.)
BUG=526998
Change-Id: I714c16dda2ed13f49d8e6cd1b48adc5a8491f43c
Reviewed-on: https://boringssl-review.googlesource.com/5785
Reviewed-by: Adam Langley <agl@google.com>
The bidi shutdown code uses type = 0 as a special signal value, but code
elsewhere doesn't account for this.
BUG=526437
Change-Id: I090cee421633d70ef3b84f4daa811608031b9ed9
Reviewed-on: https://boringssl-review.googlesource.com/5771
Reviewed-by: Adam Langley <agl@google.com>
Bidirectional shutdown doesn't make sense over DTLS; you can't reuse the
underlying channel after receiving close_notify because the channel is
unordered. This removes one caller of dtls1_read_bytes.
Really close_notify makes no sense in DTLS. It can't even protect
against some kind of truncation because it's all unordered. But continue
to send it in case anything is (unreliably since the channel is lossy)
relying on close_notify to signal some kind of session end. This only
makes SSL_shutdown stop trying to wait for one once we've already
decided to shut down the connection.
BUG=526437
Change-Id: I6afad7cb7209c4aba0b96f9246b04c81d90987a9
Reviewed-on: https://boringssl-review.googlesource.com/5770
Reviewed-by: Adam Langley <agl@google.com>
Now that it even works at all (type = 0 bug aside), add tests for it.
Test both close_notify being received before and after SSL_shutdown is
called. In the latter case, have the peer send some junk to be ignored
to test that works.
Also test that SSL_shutdown fails on unclean shutdown and that quiet
shutdowns ignore it.
BUG=526437
Change-Id: Iff13b08feb03e82f21ecab0c66d5f85aec256137
Reviewed-on: https://boringssl-review.googlesource.com/5769
Reviewed-by: Adam Langley <agl@google.com>
When discarding a record, it's important to start reading the next one,
or the state machine retry signaling doesn't work.
BUG=526437
Change-Id: I5e4a5155310d097c0033cdf5d06712410a01ee08
Reviewed-on: https://boringssl-review.googlesource.com/5768
Reviewed-by: Adam Langley <agl@google.com>
The handshake state machine is still rather messy (we should switch to CBB,
split the key exchanges apart, and also pull reading and writing out), but this
version makes it more obvious to the compiler that |p| and |sig_len| are
initialized. The old logic created a synchronous-only state which, if enterred
directly, resulted in some variables being uninitialized.
Change-Id: Ia3ac9397d523fe299c50a95dc82a9b26304cea96
Reviewed-on: https://boringssl-review.googlesource.com/5765
Reviewed-by: Adam Langley <agl@google.com>
Our tests shouldn't panic if the program misbehaves.
Change-Id: I113e050222bcf48e5f25883f860dbc1c5c77e77e
Reviewed-on: https://boringssl-review.googlesource.com/5764
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>
The difference of two pointers is signed, even though it's always
non-negative here, so MSVC is complaining about signedness mismatch.
Change-Id: I5a042d06ed348540706b93310af3f60f3ab5f303
Reviewed-on: https://boringssl-review.googlesource.com/5766
Reviewed-by: Adam Langley <agl@google.com>
Rather than parse the fields in two passes, group the code relating to
one field together. Somewhat less annoying to add new fields. To keep
this from getting too unwieldy, add a few more helper functions for the
common field types.
Change-Id: Ia86c6bbca9dd212d5c35029363ea4d6b6426164a
Reviewed-on: https://boringssl-review.googlesource.com/5758
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>
The old empty record logic discarded the records at a very low-level.
Let the error bubble up to ssl3_read_bytes so the type mismatch logic
may kick in before the empty record is skipped.
Add tests for when the record in question is application data, before
before the handshake and post ChangeCipherSpec.
BUG=521840
Change-Id: I47dff389cda65d6672b9be39d7d89490331063fa
Reviewed-on: https://boringssl-review.googlesource.com/5754
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>
arm_arch.h is included from ARM asm files, but lives in crypto/, not
openssl/include/. Since the asm files are often built from a different
location than their position in the source tree, relative include paths
are unlikely to work so, rather than having crypto/ be a de-facto,
second global include path, this change moves arm_arch.h to
include/openssl/.
It also removes entries from many include paths because they should be
needed as relative includes are always based on the locations of the
source file.
Change-Id: I638ff43d641ca043a4fc06c0d901b11c6ff73542
Reviewed-on: https://boringssl-review.googlesource.com/5746
Reviewed-by: Adam Langley <agl@google.com>
Apparently we left those as zero. Oh well. This only affects
SSL_CIPHER_get_bits, but so long as we have the field, it ought to be correct.
Change-Id: I2878ec22c2f5a6263f805e04d9fd8448994639b7
Reviewed-on: https://boringssl-review.googlesource.com/5752
Reviewed-by: Adam Langley <agl@google.com>
This made sense when the cipher might have been standardized as-is, so a
DHE_RSA variant could appease the IETF. Since the standardized variant is going
to have some nonce tweaks anyway, there's no sense in keeping this around. Get
rid of one non-standard cipher suite value early. (Even if they were to be
standardized as-is, it's not clear we should implement new DHE cipher suites at
this point.)
Chrome UMA, unsurprisingly, shows that it's unused.
Change-Id: Id83d73a4294b470ec2e94d5308fba135d6eeb228
Reviewed-on: https://boringssl-review.googlesource.com/5750
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 get initialized in SSL_new and SSL_CTX_new, respectively.
Change-Id: Ib484108987a99f654d1a77fc473103f5cb393bd7
Reviewed-on: https://boringssl-review.googlesource.com/5676
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>
I'm not sure why one would ever want to externally know the curve list
supported by the server. The API is new as of 1.0.2 and has no callers.
Configuring curves will be much more useful when Curve25519 exists and the API
isn't terribly crazy, so keep that API around and promote it to a real
function.
BUG=404754
Change-Id: Ibd5858791d3dfb30d53dd680cb75b0caddcbb7df
Reviewed-on: https://boringssl-review.googlesource.com/5674
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 guarantees that we never read beyond the first record, even if the
first record is empty. Between removing SSL_set_read_ahead and DTLS
enforcing record boundaries, this means the buffer need never memmove
data.
The memmove isn't really much of a burden and we can probably just put
SSL_set_read_ahead back after the cleanup if desired. But while the
non-existant read_ahead is off, we should avoid reading more than needed.
(Also the current memmove logic is completely wrong for TLS. Checking
align != 0 doesn't make sense. The real reason to memmove is that the
next record may still be full size. So now line 209 of s3_pkt.c should
*actually* be unreachable.)
SSL_R_HTTPS_PROXY_REQUEST detection is now slightly less accurate, but
OpenSSL was already not parsing HTTP completely. We could asynchronously
read the extra 3 bytes once the first 5 match, but that seems
unnecessary. (Shall we just get rid of all these HTTP detectors? The
only consumer of those error codes is some diagnostics logic.)
BUG=468889
Change-Id: Ie3bf148ae7274795e1d048d78282d1d8063278ea
Reviewed-on: https://boringssl-review.googlesource.com/5714
Reviewed-by: Adam Langley <agl@google.com>
I'm not sure why I made a separate one. (Not quite how the V2ClientHello
code will look in the buffer-free API yet. Probably the future
refactored SSL_HANDSHAKE gadget will need separate entry points to
consume a handshake message or V2ClientHello and the driver deals with
framing.)
This also means that ssl3_setup_read_buffer is never called external to
ssl3_read_n.
BUG=468889
Change-Id: I872f1188270968bf53ee9d0488a761c772a11e9e
Reviewed-on: https://boringssl-review.googlesource.com/5713
Reviewed-by: Adam Langley <agl@google.com>
Rather than sniff for ClientHello, just fall through to standard logic
once weird cases are resolved.
This means that garbage will now read as WRONG_VERSION rather than
UNKNOWN_PROTOCOL, but the rules here were slightly odd anyway. This also
means we'll now accept empty records before the ClientHello (up to the
empty record limit), and process records of the wrong type with the
usual codepath during the handshake.
This shouldn't be any more risk as it just makes the ClientHello more
consistent with the rest of the protocol. A TLS implementation that
doesn't parse V2ClientHello would do the same unless it still
special-cased the first record. All newly-exposed states are reachable
by fragmenting ClientHello by one byte and then sending the record in
question.
BUG=468889
Change-Id: Ib701ae5d8adb663e158c391639b232a9d9cd1c6e
Reviewed-on: https://boringssl-review.googlesource.com/5712
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>
Due to a typo, when a server sent an unknown extension, the extension
number would be taken from a NULL structure rather than from the
variable of the same name that's in the local scope.
BUG=517935
Change-Id: I29d5eb3c56cded40f6155a81556199f12439ae06
Reviewed-on: https://boringssl-review.googlesource.com/5650
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>
A memory BIO is internally a BUF_MEM anyway. There's no need to bring
BIO_write into the mix. BUF_MEM is size_t clean.
Change-Id: I4ec6e4d22c72696bf47c95861771013483f75cab
Reviewed-on: https://boringssl-review.googlesource.com/5616
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 split was only needed for buffering records. Likewise, the extra
seq_num field is now unnecessary.
This also fixes a bug where dtls1_process_record will push an error on
the queue if the decrypted record is too large, which dtls1_get_record
will ignore but fail to clear, leaving garbage on the error queue. The
error is now treated as fatal; the reason DTLS silently drops invalid
packets is worrying about ease of DoS, but after SSL_AEAD_CTX_open, the
packet has been authenticated. (Unless it's the null cipher, but that's
during the handshake and the handshake is already DoS-able by breaking
handshake reassembly state.)
The function is still rather a mess. Later changes will clean this up.
BUG=468889
Change-Id: I96a54afe0755d43c34456f76e77fc4ee52ad01e3
Reviewed-on: https://boringssl-review.googlesource.com/5557
Reviewed-by: Adam Langley <agl@google.com>
The existing tests only went monotonic. Allow an arbitrary mapping
function. Also test by sending more app data. The handshake is fairly
resilient to replayed packets, whereas our test code intentionally
isn't.
Change-Id: I0fb74bbacc260c65ec5f6a1ca8f3cb23b4192855
Reviewed-on: https://boringssl-review.googlesource.com/5556
Reviewed-by: Adam Langley <agl@google.com>
The only point format that we ever support is uncompressed, which the
RFC says implementations MUST support. The TLS 1.3 and Curve25519
forecast is that point format negotiation is gone. Each curve has just
one point format and it's labeled, for historial reasons, as
"uncompressed".
Change-Id: I8ffc8556bed1127cf288d2a29671abe3c9b3c585
Reviewed-on: https://boringssl-review.googlesource.com/5542
Reviewed-by: Adam Langley <agl@google.com>
I don't think we had coverage for this check.
Change-Id: I5e454e69c1ee9f1b9760d2ef1431170d76f78d63
Reviewed-on: https://boringssl-review.googlesource.com/5544
Reviewed-by: Adam Langley <agl@google.com>
That got out of sync at some point.
Change-Id: I5a45f50f330ceb65053181afc916053a80aa2c5d
Reviewed-on: https://boringssl-review.googlesource.com/5541
Reviewed-by: Adam Langley <agl@google.com>
It's never called anywhere and doesn't return anything interesting.
Change-Id: I68e7e9cd7b74a72f61092ac5d2b5d2390e55a228
Reviewed-on: https://boringssl-review.googlesource.com/5540
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>
These are not in upstream and were probably introduced on accident by stray vim
keystrokes.
Change-Id: I35f51f81fc37e75702e7d8ffc6f040ce71321b54
Reviewed-on: https://boringssl-review.googlesource.com/5490
Reviewed-by: Adam Langley <agl@google.com>
With the fastradio stuff gone, the padding computation is slightly more
straight-forward.
Change-Id: I67ede92fdf5f34c265c7a44e4cdc1a5ce5416df2
Reviewed-on: https://boringssl-review.googlesource.com/5482
Reviewed-by: Adam Langley <agl@google.com>
This sort of test is more suitable for ssl_test than runner. This should
stress all the various cases around padding. Use tickets rather than
hostnames to inflate the ClientHello because there's a fairly tight
maximum hostname length.
Change-Id: Ibd43aaa7acb9bf5fa00a9d2548d2101e5bb147d3
Reviewed-on: https://boringssl-review.googlesource.com/5480
Reviewed-by: Adam Langley <agl@google.com>
These were used in the upstream Go code to fuzz-test the handshake
marshal/unmarshal functions. But we don't do that there so best to
remove them.
(The ClientHello equals function is still used, however, to test DTLS
retransmission.)
Change-Id: I950bdf4f7eefa2bca13c10f5328d2e6c586604e2
Reviewed-on: https://boringssl-review.googlesource.com/5470
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Fastradio was a trick where the ClientHello was padding to at least 1024
bytes in order to trick some mobile radios into entering high-power mode
immediately. After experimentation, the feature is being dropped.
This change also tidies up a bit of the extensions code now that
everything is using the new system.
Change-Id: Icf7892e0ac1fbe5d66a5d7b405ec455c6850a41c
Reviewed-on: https://boringssl-review.googlesource.com/5466
Reviewed-by: Adam Langley <agl@google.com>
This also removes support for the “old” Channel ID extension.
Change-Id: I1168efb9365c274db6b9d7e32013336e4404ff54
Reviewed-on: https://boringssl-review.googlesource.com/5462
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I5777b73f485da6534b407e6c531f8293898b9c06
Reviewed-on: https://boringssl-review.googlesource.com/5461
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I3b085cd13295e83c578c549763f0de82f39499a2
Reviewed-on: https://boringssl-review.googlesource.com/5460
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
In the ancient times, before ex_data and OpenSSL, SSLeay supported a
single app_data slot in various types. Later app_data begat ex_data, and
app_data was replaced by compatibility macros to ex_data index zero.
Today, app_data is still in use, but ex_data never reserved index zero
for app_data. This causes some danger where, if the first ex_data
registration did not use NULL callbacks, the registration's callbacks
would collide with app_data.
Instead, add an option to the types with app_data to reserve index zero.
Also switch SSL_get_ex_data_X509_STORE_CTX_idx to always return zero
rather than allocate a new one. It used to be that you used
X509_STORE_CTX_get_app_data. I only found one consumer that we probably
don't care about, but, to be safe and since it's easy, go with the
conservative option. (Although SSL_get_ex_data_X509_STORE_CTX_idx wasn't
guaranteed to alias app_data, in practice it always did. No consumer
ever calls X509_STORE_CTX_get_ex_new_index.)
Change-Id: Ie75b279d60aefd003ffef103f99021c5d696a5e9
Reviewed-on: https://boringssl-review.googlesource.com/5313
Reviewed-by: Adam Langley <agl@google.com>
Chromium's NaCl build has _POSIX_SOURCE already defined, so #undef it first.
The compiler used also dislikes static asserts with the same name.
Change-Id: I0283fbad1a2ccf98cdb0ca2a7965b15441806308
Reviewed-on: https://boringssl-review.googlesource.com/5430
Reviewed-by: Adam Langley <agl@google.com>
It switched from CBB_remaining to CBB_len partway through review, but
the semantics are still CBB_remaining. Using CBB_len allows the
len_before/len_after logic to continue working even if, in the future,
handshake messages are built on a non-fixed CBB.
Change-Id: Id466bb341a14dbbafcdb26e4c940a04181f2787d
Reviewed-on: https://boringssl-review.googlesource.com/5371
Reviewed-by: Adam Langley <agl@google.com>
Historically we had a bug around this and this change implements a test
that we were previously carrying internally.
Change-Id: Id181fedf66b2b385b54131ac91d74a31f86f0205
Reviewed-on: https://boringssl-review.googlesource.com/5380
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This removes the version field from RSA and instead handles versioning
as part of parsing. (As a bonus, we now correctly limit multi-prime RSA
to version 1 keys.)
Most consumers are also converted. old_rsa_priv_{de,en}code are left
alone for now. Those hooks are passed in parameters which match the old
d2i/i2d pattern (they're only used in d2i_PrivateKey and
i2d_PrivateKey).
Include a test which, among other things, checks that public keys being
serialized as private keys are handled properly.
BUG=499653
Change-Id: Icdd5f0382c4a84f9c8867024f29756e1a306ba08
Reviewed-on: https://boringssl-review.googlesource.com/5273
Reviewed-by: Adam Langley <agl@google.com>
Now that 11c0f8e54c has landed, none of
the cases of the switch in |ssl3_ctrl| ever break and so the “return 1”
at the end of the function is unreachable. MSVC is unhappy about that.
Change-Id: I001dc63831ba60d93b622ac095297e2febc5f078
Since it resets leaf, private key, and chain, it makes sense to also
clear custom key method tables.
Change-Id: If511b8f15a44674c31d068d36984e9189c5a9071
Reviewed-on: https://boringssl-review.googlesource.com/5356
Reviewed-by: Adam Langley <agl@google.com>
Also document them in the process. Almost done!
BUG=404754
Change-Id: I3333c7e9ea6b4a4844f1cfd02bff8b5161b16143
Reviewed-on: https://boringssl-review.googlesource.com/5355
Reviewed-by: Adam Langley <agl@google.com>
The APIs that are CTRL macros will be documented (and converted to
functions) in a follow-up.
Change-Id: I7d086db1768aa3c16e8d7775b0c818b72918f4c2
Reviewed-on: https://boringssl-review.googlesource.com/5354
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>
Without certificate slots this function doesn't do anything. It's new in
1.02 and thus unused, so get rid of it rather than maintain a
compatibility stub.
BUG=486295
Change-Id: I798fce7e4307724756ad4e14046f1abac74f53ed
Reviewed-on: https://boringssl-review.googlesource.com/5352
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>
This is in preparation for folding away certificate slots. extra_certs
and the slot-specific certificate chain will be the same.
SSL_CTX_get_extra_chain_certs already falls back to the slot-specific
chain if missing. SSL_CTX_get_extra_chain_certs_only is similar but
never falls back. This isn't very useful and is confusing with them
merged, so remove it.
BUG=486295
Change-Id: Ic708105bcf453dfe4e1969353d7eb7547ed2981b
Reviewed-on: https://boringssl-review.googlesource.com/5350
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>
These are new as of 1.0.2, not terribly useful of APIs, and are the only
reason we have to retain so many NIDs in the TLS_SIGALGS structure.
Change-Id: I7237becca09acc2ec2be441ca17364f062253893
Reviewed-on: https://boringssl-review.googlesource.com/5347
Reviewed-by: Adam Langley <agl@google.com>
It's never used and is partially broken right now; EVP_PKEY_DH doesn't
work.
Change-Id: Id6262cd868153ef731e3f4d679b2ca308cfb12a3
Reviewed-on: https://boringssl-review.googlesource.com/5343
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>
If ssl_do_write takes more than one iteration, ssl3_output_cert_chain
would be called an extra time. This is very unlikely in practice because
of the buffer BIO.
Change-Id: Ic1ae9752a8837bb404429fc60306c659208c6185
Reviewed-on: https://boringssl-review.googlesource.com/5340
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>
The SSL23_ST_foo macros are only used in ssl_stat.c.
However, these states are never set and can be removed.
Move the two remaining SSLv2 client hello record macros to ssl3.h
Change-Id: I76055405a9050cf873b4d1cbc689e54dd3490b8a
Reviewed-on: https://boringssl-review.googlesource.com/4160
Reviewed-by: Adam Langley <agl@google.com>
One tedious thing about using CBB is that you can't safely CBB_cleanup
until CBB_init is successful, which breaks the general 'goto err' style
of cleanup. This makes it possible:
CBB_zero ~ EVP_MD_CTX_init
CBB_init ~ EVP_DigestInit
CBB_cleanup ~ EVP_MD_CTX_cleanup
Change-Id: I085ecc4405715368886dc4de02285a47e7fc4c52
Reviewed-on: https://boringssl-review.googlesource.com/5267
Reviewed-by: Adam Langley <agl@google.com>
Rather than parse with d2i_ECDSA_SIG and reserialize, this is cleaner.
It's also clearer that i2d_PublicKey isn't being used for DER.
Change-Id: Iac57fb6badd1dfed1e66984e95a31f609b1538a4
Reviewed-on: https://boringssl-review.googlesource.com/5263
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>
It's still the case that we have many old compilers that can't cope with
anything else ☹.
Change-Id: Ie5a1987cd5164bdbde0c17effaa62aecb7d12352
Reviewed-on: https://boringssl-review.googlesource.com/5320
Reviewed-by: Adam Langley <agl@google.com>
Rather than four massive functions that handle every extension,
organise the code by extension with four smaller functions for each.
Change-Id: I876b31dacb05aca9884ed3ae7c48462e6ffe3b49
Reviewed-on: https://boringssl-review.googlesource.com/5142
Reviewed-by: Adam Langley <agl@google.com>
This test shouldn't trigger a renegotiation: the test is trying to
assert that without the legacy-server flag set, a server that doesn't
echo the renegotiation extension can't be connected to.
Change-Id: I1368d15ebc8f296f3ff07040c0e6c48fdb49e56f
Reviewed-on: https://boringssl-review.googlesource.com/5141
Reviewed-by: Adam Langley <agl@google.com>
Otherwise another thread may cause the session to be destroyed first.
Change-Id: I2084a28ece11540e1b8f289553161d99395e2d1f
Reviewed-on: https://boringssl-review.googlesource.com/5231
Reviewed-by: Adam Langley <agl@google.com>
Rather than rely on Chromium to query SSL_initial_handshake_complete in the
callback (which didn't work anyway because the callback is called afterwards),
move the logic into BoringSSL. BoringSSL already enforces that clients never
offer resumptions on renegotiation (it wouldn't work well anyway as client
session cache lookup is external), so it's reasonable to also implement
in-library that sessions established on a renegotiation are not cached.
Add a bunch of tests that new_session_cb is called when expected.
BUG=501418
Change-Id: I42d44c82b043af72b60a0f8fdb57799e20f13ed5
Reviewed-on: https://boringssl-review.googlesource.com/5171
Reviewed-by: Adam Langley <agl@google.com>
b4d65fda70 was written concurrently with
my updating runner to handle -resource-dir (in
7c803a65d5) and thus it didn't include the
needed change for the test that it added to handle it.
This change fixes that added test so that it can run with -resource-dir.
Change-Id: I06b0adfb3fcf3f11c061fe1c8332a45cd7cd2dbc
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>
To account for the changes in ticket renewal, Chromium will need to listen for
new_session_cb to determine whether the handshake produced a new session.
Chromium currently never caches sessions produced on a renegotiation. To retain
that behavior, it'll need to know whether new_session_cb is initial or not.
Rather than maintain duplicate state and listen for SSL_HANDSHAKE_DONE, it's
simpler to just let it query ssl->s3->initial_handshake_complete.
BUG=501418
Change-Id: Ib2f2541460bd09cf16106388e9cfdf3662e02681
Reviewed-on: https://boringssl-review.googlesource.com/5126
Reviewed-by: Adam Langley <agl@google.com>
If gdb is attached, it's convenient to be able to continue running.
Change-Id: I3bbb2634d05a08f6bad5425f71da2210dbb80cfe
Reviewed-on: https://boringssl-review.googlesource.com/5125
Reviewed-by: Adam Langley <agl@google.com>
This change adds flags to runner to allow it to be sufficiently
configured that it can run from any directory.
Change-Id: I82c08da4ffd26c5b11637480b0a79eaba0904d38
Reviewed-on: https://boringssl-review.googlesource.com/5130
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 9dcab127e14467733523ff7626da8906e67eedd6. The root problem
is dtls1_read_bytes is wrong, but we can get the right behavior now and add a
regression test for it before cleaning it up.
Change-Id: I4e5c39ab254a872d9f64242c9b77b020bdded6e6
Reviewed-on: https://boringssl-review.googlesource.com/5123
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 27c76b9b8010b536687318739c6f631ce4194688, CVE-2015-1791.
Rather than write a dup function, serializing and deserializing the object is
simpler. It also fixes a bug in the original fix where it never calls
new_session_cb to store the new session (for clients which use that callback;
how clients should handle the session cache is much less clear).
The old session isn't pruned as we haven't processed the Finished message yet.
RFC 5077 says:
The server MUST NOT assume that the client actually received the updated
ticket until it successfully verifies the client's Finished message.
Moreover, because network messages are asynchronous, a new SSL connection may
have began just before the client received the new ticket, so any such servers
are broken regardless.
Change-Id: I13b3dc986dc58ea2ce66659dbb29e14cd02a641b
Reviewed-on: https://boringssl-review.googlesource.com/5122
Reviewed-by: Adam Langley <agl@google.com>
Mirrors SSL_SESSION_to_bytes. It avoids having to deal with object-reuse, the
non-size_t length parameter, and trailing data. Both it and the object-reuse
variant back onto an unexposed SSL_SESSION_parse which reads a CBS.
Note that this changes the object reuse story slightly. It's now merely an
optional output pointer that frees its old contents. No d2i_SSL_SESSION
consumer in Google that's built does reuse, much less reuse with the assumption
that the top-level object won't be overridden.
Change-Id: I5cb8522f96909bb222cab0f342423f2dd7814282
Reviewed-on: https://boringssl-review.googlesource.com/5121
Reviewed-by: Adam Langley <agl@google.com>
