RSABadValueTooLong should have the true one as a suffix, not a prefix,
so that the version check still works. Also do the padding manually to
catch a few other bad padding cases. This is sufficient coverage so that
disabling any one comparison in the padding check flags some failure.
Change-Id: Ibcad284e5ecee3e995f43101c09e4cf7694391e9
Reviewed-on: https://boringssl-review.googlesource.com/21904
Reviewed-by: Steven Valdez <svaldez@google.com>
The C side no longer supports DHE, so there is no longer a need for the
Go side to anymore.
Change-Id: I5084177becd369779a4008a41f4838cb31adcfde
Reviewed-on: https://boringssl-review.googlesource.com/15664
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
These will be used to test the C implementation.
BUG=187
Change-Id: If397eaa51885c8140a63c5f731ce58a8ad6949aa
Reviewed-on: https://boringssl-review.googlesource.com/14452
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Static RSA key exchange in SSLv3 does not have a length prefix. We were
checking the ClientHello version rather than the final version.
Change-Id: I2d0e9d3b5a368a7caf8b1ca69f1631400a847f52
Reviewed-on: https://boringssl-review.googlesource.com/14314
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Matt Braithwaite <mab@google.com>
Reviewed-by: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Change-Id: Ie947ab176d10feb709c6e135d5241c6cf605b8e8
Reviewed-on: https://boringssl-review.googlesource.com/12700
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is in preparation for implementing the version extension and is
probably what we should have done from the beginning as it makes
intolerance bugs simpler.
This means knobs like SendClientVersion and SendServerVersion deal with
the wire values while knobs like NegotiateVersion and MaxVersion deal
with logical versions. (This matches how the bugs have always worked.
SendFoo is just a weird post-processing bit on the handshake messages
while NegotiateVersion actually changes how BoGo behaves.)
BUG=90
Change-Id: I7f359d798d0899fa2742107fb3d854be19e731a4
Reviewed-on: https://boringssl-review.googlesource.com/11300
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Plain PSK omits the ServerKeyExchange when there is no hint and includes
it otherwise (it should have always sent it), while other PSK ciphers
like ECDHE_PSK cannot omit the hint. Having different capabilities here
is odd and RFC 4279 5.2 suggests that all PSK ciphers are capable of
"[not] provid[ing] an identity hint".
Interpret this to mean no identity hint and empty identity hint are the
same state. Annoyingly, this gives a plain PSK implementation two
options for spelling an empty hint. The spec isn't clear and this is not
really a battle worth fighting, so I've left both acceptable and added a
test for this case.
See also https://android-review.googlesource.com/c/275217/. This is also
consistent with Android's PskKeyManager API, our only consumer anyway.
https://developer.android.com/reference/android/net/PskKeyManager.html
Change-Id: I8a8e6cc1f7dd1b8b202cdaf3d4f151bebfb4a25b
Reviewed-on: https://boringssl-review.googlesource.com/11087
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
[Tests added by davidben.]
Change-Id: I0d54a4f8b8fe91b348ff22658d95340cdb48b089
Reviewed-on: https://boringssl-review.googlesource.com/8850
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
TLS 1.3 will go through very different code than everything else. Even
SSL 3.0 is somewhat special-cased now. Move the invalid signature tests
there and run at all versions.
Change-Id: Idd0ee9aac2939c0c8fd9af2ea7b4a22942121c60
Reviewed-on: https://boringssl-review.googlesource.com/8775
Reviewed-by: David Benjamin <davidben@google.com>
Now that the odd client/server split (a remnant from the original
crypto/tls code not handling signing-hash/PRF mismatches) is gone, it
can just be pulled from the config.
Change-Id: Idb46c026d6529a2afc2b43d4afedc0aa950614db
Reviewed-on: https://boringssl-review.googlesource.com/8723
Reviewed-by: David Benjamin <davidben@google.com>
Saves worrying about forgetting it. (And indeed I forgot it in the TLS
1.3 code.)
Change-Id: Ibb55a83eddba675da64b7cf2c45eac6348c97784
Reviewed-on: https://boringssl-review.googlesource.com/8722
Reviewed-by: David Benjamin <davidben@google.com>
This way we can test failing client auth without having to worry about
first getting through server auth.
Change-Id: Iaf996d87ac3df702a17e76c26006ca9b2a5bdd1f
Reviewed-on: https://boringssl-review.googlesource.com/8721
Reviewed-by: David Benjamin <davidben@google.com>
Implement in both C and Go. To test this, route config into all the
sign.go functions so we can expose bugs to skip the check.
Unfortunately, custom private keys are going to be a little weird since
we can't check their curve type. We may need to muse on what to do here.
Perhaps the key type bit should return an enum that includes the curve?
It's weird because, going forward, hopefully all new key types have
exactly one kind of signature so key type == sig alg == sig alg prefs.
Change-Id: I1f487ec143512ead931e3392e8be2a3172abe3d2
Reviewed-on: https://boringssl-review.googlesource.com/8701
Reviewed-by: David Benjamin <davidben@google.com>
{sha256,ecdsa} should not be silently accepted for an RSA key.
Change-Id: I0c0eea5071f7a59f2707ca0ea023a16cc4126d6a
Reviewed-on: https://boringssl-review.googlesource.com/8697
Reviewed-by: David Benjamin <davidben@google.com>
In preparation for TLS 1.3 using its actual handshake, switch most tests
to TLS 1.3 and add liberal TODOs for the tests which will need TLS 1.3
variants.
In doing so, move a few tests from basic tests into one of the groups.
Also rename BadECDSACurve to BadECDHECurve (it was never ECDSA) and add
a test to make sure FALLBACK_SCSV is correctly sensitive to the maximum
version.
Change-Id: Ifca6cf8f7a48d6f069483c0aab192ae691b1dd8e
Reviewed-on: https://boringssl-review.googlesource.com/8560
Reviewed-by: David Benjamin <davidben@google.com>
TLS 1.3 defines a new SignatureScheme uint16 enum that is backwards
compatible on the wire with TLS1.2's SignatureAndHashAlgorithm. This
change updates the go testing code to use a single signatureAlgorithm
enum (instead of 2 separate signature and hash enums) in preparation for
TLS 1.3. It also unifies all the signing around this new scheme,
effectively backporting the change to TLS 1.2.
For now, it does not distinguish signature algorithms between 1.2 and
1.3 (RSA-PSS instead of RSA-PKCS1, ECDSA must match curve types). When
the C code is ready make a similar change, the Go code will be updated
to match.
[Originally written by nharper, tweaked significantly by davidben.]
Change-Id: If9a315c4670755089ac061e4ec254ef3457a00de
Reviewed-on: https://boringssl-review.googlesource.com/8450
Reviewed-by: David Benjamin <davidben@google.com>
By corrupting the X25519 and Newhope parts separately, the test shows
that both are in use. Possibly excessive?
Change-Id: Ieb10f46f8ba876faacdafe70c5561c50a5863153
Reviewed-on: https://boringssl-review.googlesource.com/8250
Reviewed-by: Adam Langley <agl@google.com>
This is easier to deploy, and more obvious. This commit reverts a few
pieces of e25775bc, but keeps most of it.
Change-Id: If8d657a4221c665349c06041bb12fffca1527a2c
Reviewed-on: https://boringssl-review.googlesource.com/8061
Reviewed-by: Adam Langley <agl@google.com>
CECPQ1 is a new key exchange that concatenates the results of an X25519
key agreement and a NEWHOPE key agreement.
Change-Id: Ib919bdc2e1f30f28bf80c4c18f6558017ea386bb
Reviewed-on: https://boringssl-review.googlesource.com/7962
Reviewed-by: David Benjamin <davidben@google.com>
Previously, SSL_ECDH_METHOD consisted of two methods: one to produce a
public key to be sent to the peer, and another to produce the shared key
upon receipt of the peer's message.
This API does not work for NEWHOPE, because the client-to-server message
cannot be produced until the server's message has been received by the
client.
Solve this by introducing a new method which consumes data from the
server key exchange message and produces data for the client key
exchange message.
Change-Id: I1ed5a2bf198ca2d2ddb6d577888c1fa2008ef99a
Reviewed-on: https://boringssl-review.googlesource.com/7961
Reviewed-by: David Benjamin <davidben@google.com>
Found by libFuzzer combined with some experimental unsafe-fuzzer-mode patches
(to be uploaded once I've cleaned them up a bit) to disable all those pesky
cryptographic checks in the protocol.
Change-Id: I9153164fa56a0c2262c4740a3236c2b49a596b1b
Reviewed-on: https://boringssl-review.googlesource.com/7282
Reviewed-by: Adam Langley <agl@google.com>
This injects an interface to abstract between elliptic.Curve and a
byte-oriented curve25519. The C implementation will follow a similar
strategy.
Note that this slightly tweaks the order of operations. The client sees
the server public key before sending its own. To keep the abstraction
simple, ecdhCurve 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.)
BUG=571231
Change-Id: I771bb9aee0dd6903d395c84ec4f2dd7b3e366c75
Reviewed-on: https://boringssl-review.googlesource.com/6777
Reviewed-by: Adam Langley <agl@google.com>
Sometimes BadRSAClientKeyExchange-1 fails with DATA_TOO_LARGE_FOR_MODULUS if
the corruption brings the ciphertext above the RSA modulus. Ensure this does
not happen.
Change-Id: I0d8ea6887dfcab946fdf5d38f5b196f5a927c4a9
Reviewed-on: https://boringssl-review.googlesource.com/6731
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 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>
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>
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>
DH groups less than 1024 bits are clearly not very safe. Ideally servers
would switch to ECDHE because 1024 isn't great either, but this will
serve for the short term.
BUG=490240
Change-Id: Ic9aac714cdcdcbfae319b5eb1410675d3b903a69
Reviewed-on: https://boringssl-review.googlesource.com/4813
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Both on the client and the server.
Change-Id: I9892c6dbbb29938154aba4f53b10e8b5231f9c47
Reviewed-on: https://boringssl-review.googlesource.com/4071
Reviewed-by: Adam Langley <agl@google.com>
The old test just sent an empty ServerKeyExchange which is sufficient as we
reject the message early. But be more thorough and implement the actual
ephemeral key logic in the test server.
Change-Id: I016658762e4502c928c051e14d69eea67b5a495f
Reviewed-on: https://boringssl-review.googlesource.com/3650
Reviewed-by: Adam Langley <agl@google.com>
The client_version needs to be preserved, both for the RSA key exchange and
(when this codepath is used for TLS) for the SChannel renego workaround. Fix
the tests to enforce this so the cipher suite version tests catch this.
Change-Id: I0c42dc3ec4830f3724026b400e5066e7a7f1ee97
Reviewed-on: https://boringssl-review.googlesource.com/2551
Reviewed-by: Adam Langley <agl@google.com>
Ensure that the client rejects it with UNEXPECTED_MESSAGE, not by attempting to
decode it.
Change-Id: Ifc5613cf1152e0f7dcbee73e05df1ef367dfbfd5
Reviewed-on: https://boringssl-review.googlesource.com/2232
Reviewed-by: Adam Langley <agl@google.com>
pskKeyAgreement is now a wrapper over a base key agreement.
Change-Id: Ic18862d3e98f7513476f878b8df5dcd8d36a0eac
Reviewed-on: https://boringssl-review.googlesource.com/2053
Reviewed-by: Adam Langley <agl@google.com>
Only the three plain PSK suites for now. ECDHE_PSK_WITH_AES_128_GCM_SHA256 will
be in a follow-up.
Change-Id: Iafc116a5b2798c61d90c139b461cf98897ae23b3
Reviewed-on: https://boringssl-review.googlesource.com/2051
Reviewed-by: Adam Langley <agl@google.com>
If this is part of SSL_OP_ALL, we should have a test for it.
Change-Id: Ia72422beb2da6434726e78e174f3416f90f7c897
Reviewed-on: https://boringssl-review.googlesource.com/1695
Reviewed-by: Adam Langley <agl@google.com>
The missing SSL 3.0 client support in runner.go was fairly minor.
Change-Id: Ibbd440c9b6be99be08a214dec6b93ca358d8cf0a
Reviewed-on: https://boringssl-review.googlesource.com/1516
Reviewed-by: Adam Langley <agl@google.com>
Run against openssl s_client and openssl s_server. This seems to work for a
start, although it may need to become cleverer to stress more of BoringSSL's
implementation for test purposes.
In particular, it assumes a reliable, in-order channel. And it requires that
the peer send handshake fragments in order. Retransmit and whatnot are not
implemented. The peer under test will be expected to handle a lossy channel,
but all loss in the channel will be controlled. MAC errors, etc., are fatal.
Change-Id: I329233cfb0994938fd012667ddf7c6a791ac7164
Reviewed-on: https://boringssl-review.googlesource.com/1390
Reviewed-by: Adam Langley <agl@google.com>
It's not part of SSL_OP_ALL and is unused, so remove it. Add a test that
asserts the version check works.
Change-Id: I917516594ec5a4998a8316782f035697c33d99b0
Reviewed-on: https://boringssl-review.googlesource.com/1418
Reviewed-by: Adam Langley <agl@google.com>
Use it to test DHE-RSA in BoringSSL.
Change-Id: I88f7bfa76507a6f60234d61d494c9f94b7df4e0a
Reviewed-on: https://boringssl-review.googlesource.com/1377
Reviewed-by: Adam Langley <agl@google.com>
This works, but there's enough shared codepaths that it's worth a test to
ensure it stays that way.
Change-Id: I5d5a729811e35832170322957258304213204e3b
Reviewed-on: https://boringssl-review.googlesource.com/1155
Reviewed-by: Adam Langley <agl@google.com>
Initial fork from f2d678e6e89b6508147086610e985d4e8416e867 (1.0.2 beta).
(This change contains substantial changes from the original and
effectively starts a new history.)
