Quite a few functions reported wrong function names when pushing
to the error stack.
Change-Id: I84d89dbefd2ecdc89ffb09799e673bae17be0e0f
Reviewed-on: https://boringssl-review.googlesource.com/4080
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Reported by the LibreSSL project as a follow on to CVE-2015-0209
(Imported from upstream's 5e5d53d341fd9a9b9cc0a58eb3690832ca7a511f.)
Change-Id: Ic2e5dc5c96e316c55f76bedc6ea55b416be3287a
Reviewed-on: https://boringssl-review.googlesource.com/4049
Reviewed-by: Adam Langley <agl@google.com>
Fix segmentation violation when ASN1_TYPE_cmp is passed a boolean type. This
can be triggered during certificate verification so could be a DoS attack
against a client or a server enabling client authentication.
CVE-2015-0286
(Imported from upstream's e677e8d13595f7b3287f8feef7676feb301b0e8a.)
Change-Id: I5faefc190568504bb5895ed9816a6d80432cfa45
Reviewed-on: https://boringssl-review.googlesource.com/4048
Reviewed-by: Adam Langley <agl@google.com>
Previously, if SIGILL was received between the signal handler being
installed and before the sigsetjmp, the process would longjmp to a
random location.
Change-Id: I9e6143a17ff3db0e1b00ece68fce161801461010
Reviewed-on: https://boringssl-review.googlesource.com/3950
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Allocate and free ASN.1 string types directly instead of going through
the ASN.1 item code.
(Imported from upstream's 3d6aa6d441fe8124d247dffee5c68c2e5efd8258.)
Change-Id: I617283e67071a792f219ed08f19078afc223e2f5
Reviewed-on: https://boringssl-review.googlesource.com/4041
Reviewed-by: Adam Langley <agl@google.com>
Previously, ASN1_UTCTIME_cmp_time_t would return 1 if s > t, -1 if
s < t, and 0 if s == t.
This behavior was broken in upstream's
904348a4922333106b613754136305db229475ea, resulting in the opposite time
comparison behavior.
PR#3706
(Imported from upstream's da27006df06853a33b132133699a7aa9d4277920.)
Change-Id: I9c1c28ba21d82ff4b587e33b262f46be4e846ff9
Reviewed-on: https://boringssl-review.googlesource.com/4043
Reviewed-by: Adam Langley <agl@google.com>
Td4 is an array of u8. A u8 << int promotes the u8 to an int first then shifts.
If the mathematical result of a shift (as modelled by lhs * 2^{rhs}) is not
representable in an integer, behaviour is undefined. In other words, you can't
shift into the sign bit of a signed integer. Fix this by casting to u32
whenever we're shifting left by 24.
(For consistency, cast other shifts, too.)
Caught by -fsanitize=shift
Submitted by Nick Lewycky (Google)
(Imported from upstream's 8b37e5c14f0eddb10c7f91ef91004622d90ef361.)
Change-Id: Id0f98d1d65738533c6ddcc3c21bc38b569d74793
Reviewed-on: https://boringssl-review.googlesource.com/4040
Reviewed-by: Adam Langley <agl@google.com>
When printing out an ASN.1 structure, if the type is an item template don't
fall through and attempt to interpret as a primitive type.
(Imported from upstream's 5dc1247a7494f50c88ce7492518bbe0ce6f124fa.)
Change-Id: Ica39757792cbf3f83879953b67838927ddbdb809
Reviewed-on: https://boringssl-review.googlesource.com/4009
Reviewed-by: Adam Langley <agl@google.com>
Fixed assorted missing return value checks in v3_cpols.c
(Imported from upstream's c5f2b5336ab72e40ab91e2ca85639f51fa3178c6.)
Change-Id: I95e4157fc689201ca32e1c8e51d04801ba478685
Reviewed-on: https://boringssl-review.googlesource.com/4008
Reviewed-by: Adam Langley <agl@google.com>
The call to asn1_do_adb can return NULL on error, so we should check the
return value before attempting to use it.
(Imported from upstream's 34a7ed0c39aa3ab67eea1e106577525eaf0d7a00.)
Change-Id: Ia43cdc73b5f1d16e6fc907b5aaf13c9df5a9958c
Reviewed-on: https://boringssl-review.googlesource.com/4007
Reviewed-by: Adam Langley <agl@google.com>
ASN1_primitive_new takes an ASN1_ITEM * param |it|. There are a couple
of conditional code paths that check whether |it| is NULL or not - but
later |it| is deref'd unconditionally. If |it| was ever really NULL then
this would seg fault. In practice ASN1_primitive_new is marked as an
internal function in the public header file. The only places it is ever
used internally always pass a non NULL parameter for |it|. Therefore, change
the code to sanity check that |it| is not NULL, and remove the conditional
checking.
(Imported from upstream's 9e488fd6ab)
Change-Id: Icbb13cd00d0ec5529871b678b0bcc465956a7572
Reviewed-on: https://boringssl-review.googlesource.com/4006
Reviewed-by: Adam Langley <agl@google.com>
In the event of an error |rr| could be NULL. Therefore don't assume you can
use |rr| in the error handling code.
(Imported from upstream's 8c5a7b33c6269c3bd6bc0df6b4c22e4fba03b485.)
Change-Id: I0b392991ce8170dc418e93003af256d535d1e2e8
Reviewed-on: https://boringssl-review.googlesource.com/4005
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 0b142f022e2c5072295e00ebc11c5b707a726d74.)
Modified further because these ought to just be BIO_puts.
Change-Id: I8b70d70d0f626bb19b455adb4f0e08bacf453c1d
Reviewed-on: https://boringssl-review.googlesource.com/4002
Reviewed-by: Adam Langley <agl@google.com>
Firstly, it was odd that AES-NI was a special case. Secondly, I have a
need coming up for being able to get the block function and not create a
GCM context.
Change-Id: Ie87de5e7ea42dc042d302c5eafecbc6af03c714b
Reviewed-on: https://boringssl-review.googlesource.com/3910
Reviewed-by: Adam Langley <agl@google.com>
At least the linker can discard this function in the cases where nobody
is calling it.
Change-Id: I30050e918e6bc1dd9c97cc70f3a56408701abebc
Reviewed-on: https://boringssl-review.googlesource.com/3724
Reviewed-by: Adam Langley <agl@google.com>
Because NTLM authentication is still a thing.
Change-Id: I3308a8431c82f0b614e09ce3e5efac1526881f1e
Reviewed-on: https://boringssl-review.googlesource.com/3723
Reviewed-by: Adam Langley <agl@google.com>
This allows the current RC4 state of an SSL* to be extracted. We have
internal uses for this functionality.
Change-Id: Ic124c4b253c8325751f49e7a4c021768620ea4b7
Reviewed-on: https://boringssl-review.googlesource.com/3722
Reviewed-by: Adam Langley <agl@google.com>
* Eliminate the possibility of multiple lock IDs having the same
value (CRYPTO_LOCK_FIPS2 and CRYPTO_LOCK_OBJ were both 40 prior to
this commit).
* Remove unused lock IDs.
* Automatically guarantee that lock IDs and lock names stay in sync.
Change-Id: If20e462db1285fa891595a7e52404ad011ff16f6
Reviewed-on: https://boringssl-review.googlesource.com/3923
Reviewed-by: Adam Langley <agl@google.com>
May as well use this convenience function when we can. A little tidier. Even
fixes a leak on malloc failure in eckey_type2param.
Change-Id: Ie48dd98f2fe03fa9911bd78db4423ab9faefc63d
Reviewed-on: https://boringssl-review.googlesource.com/3772
Reviewed-by: Adam Langley <agl@google.com>
Upstream needs this to deal with size_t, but our low-level DES APIs take
size_t, so this is not a concern.
Change-Id: I9dc4c7248c5dd9515246a4b224147b932328a400
Reviewed-on: https://boringssl-review.googlesource.com/3882
Reviewed-by: Adam Langley <agl@google.com>
Fix a few remnants of them being unsigned long. Also rename extremely unhelpful
variable names in SSL_get_error. i is now ret_code to match the header.
Change-Id: Ic31d6626bfe09c9e21c03691dfc716c5573833ea
Reviewed-on: https://boringssl-review.googlesource.com/3881
Reviewed-by: Adam Langley <agl@google.com>
Now that much of EVP has been normalized to 0/1, a lot of code can just use
boolean operators. (As can some code which was already using them...)
Change-Id: I6bb17edfd6f67050bf1706d59d8f37df57535faa
Reviewed-on: https://boringssl-review.googlesource.com/3875
Reviewed-by: Adam Langley <agl@google.com>
No code within BoringSSL or Google (grep for EVP_PKEY_CTX_(ctrl|get|set)) is
sensitive to the various failure cases. Normalize it all to 0/1 for simplicity.
This does carry a slight risk: any new ctrl hooks we import from upstream that,
like EVP_PKEY_CTX_get_rsa_oaep_md, return something other than success/failure
cannot be called directly via EVP_PKEY_CTX_ctrl. They instead need to
internally be routed through a struct like CBS and only called through the
wrappers. To that end, unexport EVP_PKEY_CTX_ctrl and require that callers use
the wrappers. No code in Google uses it directly and, if need be, switching to
the wrapper would be an incredibly upstreamable patch.
Change-Id: I3fd4e5a1a0f3d4d1c4122c52d4c74a5105b99cd5
Reviewed-on: https://boringssl-review.googlesource.com/3874
Reviewed-by: Adam Langley <agl@google.com>
This is the only EVP_PKEY ctrl hook which returns something other than a
boolean.
Change-Id: Ic226aef168abdf72e5d30e8264a559ed5039a055
Reviewed-on: https://boringssl-review.googlesource.com/3873
Reviewed-by: Adam Langley <agl@google.com>
This removes another place where we're internally sensitive to the
success/failure conditions.
Change-Id: I18fecf6457e841ba0afb718397b9b5fd3bbdfe4c
Reviewed-on: https://boringssl-review.googlesource.com/3872
Reviewed-by: Adam Langley <agl@google.com>
All EVP_PKEY types return 1 on that. (It can go away entirely when
EVP_PKEY_HMAC is gone.) This removes a place internally where we're sensitive
to the failure code.
Change-Id: Ic6cda2da9337ba7ef1c66a18e40c5dcc44fcf840
Reviewed-on: https://boringssl-review.googlesource.com/3871
Reviewed-by: Adam Langley <agl@google.com>
Imported from upstream's 918bb86529. (The
remainder is in code we've since deleted.)
Change-Id: Ie878272114086ba60a0fd5eae059b641b00ec0c4
Reviewed-on: https://boringssl-review.googlesource.com/3831
Reviewed-by: Adam Langley <agl@google.com>
Turn them into static functions that take in an hm_fragment. It's not
immediately obvious that the frag_off/frag_len bounds checks and the msg_len
consistency check are critical to avoiding an out-of-bounds write. Better to
have dtls1_hm_fragment_mark also check internally.
Also rework the bitmask logic to be clearer and avoid a table.
Change-Id: Ica54e98f66295efb323e033cb6c67ab21e7d6cbc
Reviewed-on: https://boringssl-review.googlesource.com/3765
Reviewed-by: Adam Langley <agl@google.com>
Notably, drop all special cases around receiving a message in order and
receiving a full message. It makes things more complicated and was the source
of bugs (the MixCompleteMessageWithFragments tests added in this CL did not
pass before). Instead, every message goes through an hm_fragment, and
dtls1_get_message always checks buffered_messages to see if the next is
complete.
The downside is that we pay one more copy of the message data in the common
case. This is only during connection setup, so I think it's worth the
simplicity. (If we want to optimize later, we could either tighten
ssl3_get_message's interface to allow the handshake data being in the
hm_fragment's backing store rather than s->init_buf or swap out s->init_buf
with the hm_fragment's backing store when a mesasge completes.
This CL does not address ssl_read_bytes being an inappropriate API for DTLS.
Future work will revise the handshake/transport boundary to align better with
DTLS's needs. Also other problems that I've left as TODOs.
Change-Id: Ib4570d45634b5181ecf192894d735e8699b1c86b
Reviewed-on: https://boringssl-review.googlesource.com/3764
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I582eaa2ff922bbf1baf298a5c6857543524a8d4e
Reviewed-on: https://boringssl-review.googlesource.com/3810
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
It's unclear why .extern was being suppressed, it's also a little
unclear how the Chromium build was working without this. None the less,
it's causing problems with Android and it's more obviously correct to
make these symbols as hidden.
Change-Id: Id13ec238b80b8bd08d8ae923ac659835450e77f8
Reviewed-on: https://boringssl-review.googlesource.com/3800
Reviewed-by: Adam Langley <agl@google.com>
This avoids cluttering up the diff and making merge conflicts a pain. It does,
however, mean we need to generate err_data.c ahead of time in Chromium and
likely other downstream builds. It also adds a build dependency on Go.
Change-Id: I6e0513ed9f50cfb030f7a523ea28519590977104
Reviewed-on: https://boringssl-review.googlesource.com/3790
Reviewed-by: Adam Langley <agl@google.com>
Some code, sadly, tests the error and the extra error is breaking it.
Change-Id: I89eabadf5d2c5f7dd761030da33dd4c3f2ac8382
Reviewed-on: https://boringssl-review.googlesource.com/3720
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Instead, add a separate init_with_direction hook. Normal AEADs ignore the
direction, while legacy AEADs must be initialized with it. This avoids
maintaining extra state to support the delayed initialization.
Change-Id: I25271f0e56ee2783a2fd4d4026434154d58dc0a8
Reviewed-on: https://boringssl-review.googlesource.com/3731
Reviewed-by: Adam Langley <agl@google.com>
There's no good reason to do this, and it doesn't work; HMAC checks the length
of the key and runs it through the hash function if too long. The reuse occurs
after this check.
This allows us to shave 132 bytes off HMAC_CTX as this was the only reason it
ever stored the original key. It also slightly simplifies HMAC_Init_ex's
logic.
Change-Id: Ib56aabc3630b7178f1ee7c38ef6370c9638efbab
Reviewed-on: https://boringssl-review.googlesource.com/3733
Reviewed-by: Adam Langley <agl@google.com>
We've already initialized the context, HMAC_Init has questionable behavior
around NULL keys, and this avoids a size_t truncation.
Change-Id: Iab6bfc24fe22d46ca4c01be6129efe0630d553e6
Reviewed-on: https://boringssl-review.googlesource.com/3732
Reviewed-by: Adam Langley <agl@google.com>
(There are times when I actually miss C++ templates.)
Change-Id: I3db56e4946ae4fb919105fa33e2cfce3c7542d37
Reviewed-on: https://boringssl-review.googlesource.com/3700
Reviewed-by: Adam Langley <agl@google.com>
Håvard Molland
David Benjamin
Adam Langley
Brian Smith