Commit Graph

195 Commits

Author SHA1 Message Date
David Benjamin
0d56f888c3 Switch s to ssl everywhere.
That we're half and half is really confusing.

Change-Id: I1c2632682e8a3e63d01dada8e0eb3b735ff709ce
Reviewed-on: https://boringssl-review.googlesource.com/6785
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 23:28:22 +00:00
David Benjamin
974c7ba4ef Route DHE through the SSL_ECDH abstraction as well.
This unifies the ClientKeyExchange code rather nicely. ServerKeyExchange
is still pretty specialized. For simplicity, I've extended the yaSSL bug
workaround for clients as well as servers rather than route in a
boolean.

Chrome's already banished DHE to a fallback with intention to remove
altogether later, and the spec doesn't say anything useful about
ClientDiffieHellmanPublic encoding, so this is unlikely to cause
problems.

Change-Id: I0355cd1fd0fab5729e8812e4427dd689124f53a2
Reviewed-on: https://boringssl-review.googlesource.com/6784
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 23:17:32 +00:00
David Benjamin
4298d77379 Implement draft-ietf-tls-curve25519-01 in C.
The new curve is not enabled by default.

As EC_GROUP/EC_POINT is a bit too complex for X25519, this introduces an
SSL_ECDH_METHOD abstraction which wraps just the raw ECDH operation. It
also tidies up some of the curve code which kept converting back and
force between NIDs and curve IDs. Now everything transits as curve IDs
except for API entry points (SSL_set1_curves) which take NIDs. Those
convert immediately and act on curve IDs from then on.

Note that, like the Go implementation, this slightly tweaks the order of
operations. The client sees the server public key before sending its
own. To keep the abstraction simple, SSL_ECDH_METHOD expects to
generate a keypair before consuming the peer's public key. Instead, the
client handshake stashes the serialized peer public value and defers
parsing it until it comes time to send ClientKeyExchange. (This is
analogous to what it was doing before where it stashed the parsed peer
public value instead.)

It still uses TLS 1.2 terminology everywhere, but this abstraction should also
be compatible with TLS 1.3 which unifies (EC)DH-style key exchanges.
(Accordingly, this abstraction intentionally does not handle parsing the
ClientKeyExchange/ServerKeyExchange framing or attempt to handle asynchronous
plain RSA or the authentication bits.)

BUG=571231

Change-Id: Iba09dddee5bcdfeb2b70185308e8ab0632717932
Reviewed-on: https://boringssl-review.googlesource.com/6780
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 21:51:30 +00:00
David Benjamin
fc8251258d Convert ssl3_send_cert_verify to CBB.
In doing so, make the asynchronous portion look more like
ssl3_send_server_key_exchange. This is a considerably simpler structure,
so the save/resume doesn't need any state.

Mostly this means writing out the signature algorithm can now go through
CBB rather than a uint8_t* without bounds check.

Change-Id: If99fcffd0d41a84514c3d23034062c582f1bccb2
Reviewed-on: https://boringssl-review.googlesource.com/6771
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 17:40:47 +00:00
Adam Langley
5fb18c6b42 Make MSVC happy.
The MSVC build is failing with:
  ssl\s3_srvr.c(1363) : warning C4701: potentially uninitialized local variable 'digest_len' used

I don't believe that this warning is valid, but this change assigns a
value to |digest_len| to fix the build.

Change-Id: I20107a932bc16c880032cc1a57479b1a806aa8ea
Reviewed-on: https://boringssl-review.googlesource.com/6821
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 17:38:45 +00:00
David Benjamin
2a0b391ac9 Rewrite ssl3_send_server_key_exchange to use CBB.
There is some messiness around saving and restoring the CBB, but this is
still significantly clearer.

Note that the BUF_MEM_grow line is gone in favor of a fixed CBB like the
other functions ported thus far. This line was never necessary as
init_buf is initialized to 16k and none of our key exchanges get that
large. (The largest one can get is DHE_RSA. Even so, it'd take a roughly
30k-bit DH group with a 30k-bit RSA key.)

Having such limits and tight assumptions on init_buf's initial size is
poor (but on par for the old code which usually just blindly assumed the
message would not get too large) and the size of the certificate chain
is much less obviously bounded, so those BUF_MEM_grows can't easily go.

My current plan is convert everything but those which legitimately need
BUF_MEM_grow to CBB, then atomically convert the rest, remove init_buf,
and switch everything to non-fixed CBBs. This will hopefully also
simplify async resumption. In the meantime, having a story for
resumption means the future atomic change is smaller and, more
importantly, relieves some complexity budget in the ServerKeyExchange
code for adding Curve25519.

Change-Id: I1de6af9856caaed353453d92a502ba461a938fbd
Reviewed-on: https://boringssl-review.googlesource.com/6770
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 17:23:58 +00:00
David Benjamin
af21bcf91f Remove other unnecessary BN_CTX allocations.
Functions which take a BN_CTX also accept NULL. Allocating a BN_CTX is
only useful if doing multiple operations, which we aren't.

Change-Id: Ib31113f214707cce6283e090ded0bf93ae5e7c12
Reviewed-on: https://boringssl-review.googlesource.com/6768
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 17:01:58 +00:00
David Benjamin
3f5b43df07 Simplify RSA key exchange padding check.
This check was fixed a while ago, but it could have been much simpler.

In the RSA key exchange, the expected size of the output is known, making the
padding check much simpler. There isn't any use in exporting the more general
RSA_message_index_PKCS1_type_2. (Without knowing the expected size, any
integrity check or swap to randomness or other mitigation is basically doomed
to fail.)

Verified with the valgrind uninitialized memory trick that we're still
constant-time.

Also update rsa.h to recommend against using the PKCS#1 v1.5 schemes.

Thanks to Ryan Sleevi for the suggestion.

Change-Id: I4328076b1d2e5e06617dd8907cdaa702635c2651
Reviewed-on: https://boringssl-review.googlesource.com/6613
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 00:10:14 +00:00
David Benjamin
a41280d8cb Pull ChangeCipherSpec into the handshake state machine.
This uses ssl3_read_bytes for now. We still need to dismantle that
function and then invert the handshake state machine, but this gets
things closer to the right shape as an intermediate step and is a large
chunk in itself. It simplifies a lot of the CCS/handshake
synchronization as a lot of the invariants much more clearly follow from
the handshake itself.

Tests need to be adjusted since this changes some error codes. Now all
the CCS/Handshake checks fall through to the usual
SSL_R_UNEXPECTED_RECORD codepath. Most of what used to be a special-case
falls out naturally. (If half of Finished was in the same record as the
pre-CCS message, that part of the handshake record would have been left
unconsumed, so read_change_cipher_spec would have noticed, just like
read_app_data would have noticed.)

Change-Id: I15c7501afe523d5062f0e24a3b65f053008d87be
Reviewed-on: https://boringssl-review.googlesource.com/6642
Reviewed-by: Adam Langley <agl@google.com>
2015-12-16 18:36:57 +00:00
David Benjamin
5ddffbb8bc Make SSL_(CTX_)?set_tmp_ecdh call SSL_(CTX_)?set1_curves.
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>
2015-12-15 20:28:47 +00:00
David Benjamin
53e5c2c225 Remove SSL_(CTX_)?set_ecdh_callback.
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>
2015-12-15 20:07:37 +00:00
David Benjamin
0bd71eb85d Remove weird ret negation logic.
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>
2015-12-15 19:24:51 +00:00
David Benjamin
ef5e515819 Remove SSL_OP_TLS_D5_BUG.
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>
2015-12-15 19:11:41 +00:00
Adam Langley
c4f25ce0c6 Work around yaSSL bug.
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>
2015-11-30 22:41:24 +00:00
David Benjamin
b324159be9 Fix ssl3_send_server_key_exchange error path.
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>
2015-11-16 23:27:27 +00:00
David Benjamin
af07365b49 Check for overflow when parsing a CBS with d2i_*.
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>
2015-11-16 23:17:42 +00:00
David Benjamin
6e80765774 Add SSL_get_server_key_exchange_hash.
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>
2015-11-06 22:35:28 +00:00
Brian Smith
274341dd6e Change the type of |EC_GROUP_get_degree| and friends to |unsigned|.
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>
2015-10-27 16:48:04 +00:00
nagendra modadugu
3398dbf279 Add server-side support for asynchronous RSA decryption.
Change-Id: I6df623f3e9bc88acc52043f16b34649b7af67663
Reviewed-on: https://boringssl-review.googlesource.com/5531
Reviewed-by: Adam Langley <alangley@gmail.com>
2015-10-26 20:26:20 +00:00
David Benjamin
dc2aea2231 Remove all the logic around custom session IDs and retrying on collisions.
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>
2015-10-26 19:00:14 +00:00
David Benjamin
82170248e7 Document the info callback.
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>
2015-10-26 18:12:22 +00:00
David Benjamin
cfdee213f8 Add use counters for SSL_OP_TLS_D5_BUG and SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER.
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>
2015-10-20 18:22:47 +00:00
David Benjamin
5638046cd7 Convert ssl3_send_server_hello to CBB.
BUG=468889

Change-Id: I899d67addbff01c64175f47b19ca2b688626405b
Reviewed-on: https://boringssl-review.googlesource.com/6191
Reviewed-by: Adam Langley <alangley@gmail.com>
2015-10-20 18:04:48 +00:00
Matt Braithwaite
4d11aff33a Don't record handshake hashes if no Channel ID
Change-Id: Ic0dcde436024e8d11f9b5986998cf6e808ff036b
Reviewed-on: https://boringssl-review.googlesource.com/6007
Reviewed-by: Adam Langley <agl@google.com>
2015-09-29 21:47:05 +00:00
David Benjamin
9e4e01ee14 Align the SSL stack on #include style.
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>
2015-09-15 23:32:07 +00:00
David Benjamin
1f9f9c4b51 Tidy up the ssl3_send_server_key_exchange slightly.
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>
2015-08-28 22:53:43 +00:00
David Benjamin
26416e9dde Remove the last of SESS_CERT.
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>
2015-08-28 22:45:59 +00:00
David Benjamin
b8d28cf532 Factor out the buffering and low-level record code.
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>
2015-08-28 22:01:02 +00:00
Paul Lietar
aeeff2ceee Server-side OCSP stapling support.
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>
2015-08-20 17:55:31 +00:00
David Benjamin
894f48c6b3 Only read 5 bytes (record header length) in sniffing for V2ClientHello.
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>
2015-08-17 20:58:09 +00:00
David Benjamin
45c6c3e8ef Use the record-layer buffer for sniffing V2ClientHellos.
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>
2015-08-17 20:56:21 +00:00
David Benjamin
97760d5254 Slightly simplify V2ClientHello sniffing.
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>
2015-08-17 20:48:06 +00:00
David Benjamin
d6a4ae97cd Simplify tls1_channel_id_hash.
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>
2015-08-07 01:16:33 +00:00
David Benjamin
9550c3ac8b Decouple the handshake buffer and digest.
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>
2015-08-07 01:10:33 +00:00
nagendra modadugu
601448aa13 Add server-side support for asynchronous signing.
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>
2015-07-31 01:14:29 +00:00
David Benjamin
821464e45f Remove old 'prepare' extensions functions.
These are no-ops now.

Change-Id: Ib842d512571a06a45e52f30fe4bb8e98e9c37cf9
Reviewed-on: https://boringssl-review.googlesource.com/5481
Reviewed-by: Adam Langley <agl@google.com>
2015-07-29 19:21:18 +00:00
Adam Langley
49c7af1c42 Convert the Channel ID extension to the new system.
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>
2015-07-21 21:44:11 +00:00
David Benjamin
3570d73bf1 Remove the func parameter to OPENSSL_PUT_ERROR.
Much of this was done automatically with
  find . -name '*.c' | xargs sed -E -i '' -e 's/(OPENSSL_PUT_ERROR\([a-zA-Z_0-9]+, )[a-zA-Z_0-9]+, ([a-zA-Z_0-9]+\);)/\1\2/'
  find . -name '*.c' | xargs sed -E -i '' -e 's/(OPENSSL_PUT_ERROR\([a-zA-Z_0-9]+, )[a-zA-Z_0-9]+,  ([a-zA-Z_0-9]+\);)/\1\2/'

BUG=468039

Change-Id: I4c75fd95dff85ab1d4a546b05e6aed1aeeb499d8
Reviewed-on: https://boringssl-review.googlesource.com/5276
Reviewed-by: Adam Langley <agl@google.com>
2015-07-16 02:02:37 +00:00
David Benjamin
d1d8078025 Fold away certificate slots mechanism.
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>
2015-07-07 01:22:13 +00:00
David Benjamin
bb20f52383 Merge the RSA_ENC and RSA_SIGN certificate slots.
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>
2015-07-07 01:15:41 +00:00
David Benjamin
396a441421 ssl3_cert_verify_hash should take the EVP_PKEY type.
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>
2015-07-07 01:10:35 +00:00
David Benjamin
a8653208ec Add CBB_zero to set a CBB to the zero state.
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>
2015-07-01 19:45:43 +00:00
David Benjamin
e3aa1d9dd4 Cleanup ticket processing and session lookup.
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>
2015-07-01 19:33:23 +00:00
David Benjamin
b4d65fda70 Implement asynchronous private key operations for client auth.
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>
2015-06-18 22:14:51 +00:00
Adam Langley
ba5934b77f Tighten up EMS resumption behaviour.
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>
2015-06-03 22:05:50 +00:00
David Benjamin
a1c90a5ce1 Further tidy up cipher logic.
With SSL2 gone, there's no need for this split between the abstract
cipher framework and ciphers. Put the cipher suite table in ssl_cipher.c
and move other SSL_CIPHER logic there. With that gone, prune the
cipher-related hooks in SSL_PROTOCOL_METHOD.

BUG=468889

Change-Id: I48579de8bc4c0ea52781ba1b7b57bc5b4919d21c
Reviewed-on: https://boringssl-review.googlesource.com/4961
Reviewed-by: Adam Langley <agl@google.com>
2015-06-01 22:48:30 +00:00
David Benjamin
5c1ce2925d Decide whether or not to request client certificates early.
This allows us to merge two of the ssl3_digest_cached_records calls which were
almost, but not completely, redundant. Also catches a missing case: the buffer
may be discarded if doing session resumption but otherwise enabling client
authentication.

BUG=492371

Change-Id: I78e9a4a9cca665e89899ef97b815454c6f5c7e02
Reviewed-on: https://boringssl-review.googlesource.com/4885
Reviewed-by: Adam Langley <agl@google.com>
2015-05-27 21:53:16 +00:00
David Benjamin
4b30b28def Remove server-side renego session resumption check.
Servers can no longer renegotiate.

Change-Id: Id79d5753562e29d2872871f4f571552a019215fa
Reviewed-on: https://boringssl-review.googlesource.com/4884
Reviewed-by: Adam Langley <agl@google.com>
2015-05-27 21:51:19 +00:00
David Benjamin
5aea93e604 Deprecate and no-op SSL_VERIFY_CLIENT_ONCE.
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>
2015-05-27 21:50:24 +00:00
David Benjamin
74d8bc2503 Don't make SSL_MODE_*HELLO_TIME configurable.
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>
2015-05-27 21:47:59 +00:00