Beyond generally eliminating unnecessary includes, eliminate as many
includes of headers that declare/define particularly error-prone
functionality like strlen, malloc, and free. crypto/err/internal.h was
added to remove the dependency on openssl/thread.h from the public
openssl/err.h header. The include of <stdlib.h> in openssl/mem.h was
retained since it defines OPENSSL_malloc and friends as macros around
the stdlib.h functions. The public x509.h, x509v3.h, and ssl.h headers
were not changed in order to minimize breakage of source compatibility
with external code.
Change-Id: I0d264b73ad0a720587774430b2ab8f8275960329
Reviewed-on: https://boringssl-review.googlesource.com/4220
Reviewed-by: Adam Langley <agl@google.com>
C4701 is "potentially uninitialized local variable 'buf' used". It
sometimes results in false positives, which can now be suppressed
using the macro OPENSSL_SUPPRESS_POTENTIALLY_UNINITIALIZED_WARNINGS.
Change-Id: I15068b5a48e1c704702e7752982b9ead855e7633
Reviewed-on: https://boringssl-review.googlesource.com/3160
Reviewed-by: Adam Langley <agl@google.com>
`cmake -GNinja .. -DCMAKE_BUILD_TYPE=Release` fails without this
patch, when building using MSVC 2013.
MSVC will detect (in release builds only, it seems) that functions that
call abort will never return, and then warn that any code after a call
to one of them is unreachable. Since we treat warnings as errors when
building, this breaks the build. While this is usually desirable, it
isn't desirable in this case.
Change-Id: Ie5f24b1beb60fd2b33582a2ceef4c378ad0678fb
Reviewed-on: https://boringssl-review.googlesource.com/3960
Reviewed-by: Adam Langley <agl@google.com>
The only dependency the low-level crypto modules have on code in
crypto/obj is their use of OBJ_nid2sn, which is trivial to avoid.
This facilitates future simplification of crypto/obj, including
possibly the removal of functions like OBJ_nid2sn and the complex
build infrastructure that supports them.
This change also removes EVP_CIPHER_name and EVP_MD_name.
Change-Id: I34ce7dc7e58d5c08b52f95d25eba3963590cf2f7
Reviewed-on: https://boringssl-review.googlesource.com/3932
Reviewed-by: Adam Langley <agl@google.com>
Limiting uses of crypto/bio to code that really need to it by avoiding
the use of BIO just to write to stdout/stderr.
Change-Id: I34e0f773161aeec073691e439ac353fb7b1785f3
Reviewed-on: https://boringssl-review.googlesource.com/3930
Reviewed-by: Adam Langley <agl@google.com>
A previous change in BoringSSL renamed ERR_print_errors_fp to
BIO_print_errors_fp as part of refactoring the code to improve the
layering of modules within BoringSSL. Rename it back for better
compatibility with code that was using the function under the original
name. Move its definition back to crypto/err using an implementation
that avoids depending on crypto/bio.
Change-Id: Iee7703bb1eb4a3d640aff6485712bea71d7c1052
Reviewed-on: https://boringssl-review.googlesource.com/4310
Reviewed-by: Adam Langley <agl@google.com>
Avoiding superflous references to RC4 makes it easier to audit the code
to find unsafe uses of it. It also avoids subtly encouraging users to
choose RC4 instead of a better alternative.
Change-Id: Ia27d7f4cd465e143d30a28b36c7871f7c30411ea
Reviewed-on: https://boringssl-review.googlesource.com/3990
Reviewed-by: Adam Langley <agl@google.com>
Prior to this change, when EC_GROUP_get0_generator fails, BN_CTX_end
would get called even though BN_CTX_start hadn't been called yet, in
the case where the caller-supplied |ctx| is not NULL.
Change-Id: I6f728e74f0167193891cdb6f122b20b0770283dc
Reviewed-on: https://boringssl-review.googlesource.com/4271
Reviewed-by: Adam Langley <agl@google.com>
Now that ERR is using thread-local storage, there's very little that the
THREADID code is doing and it can be turned into stub functions.
Change-Id: I668613fec39b26c894d029b10a8173c3055f6019
Since ERR will soon have thread-local storage, we don't need to worry
about high-performance implementations and thus don't need to be able to
switch two different implementations at run-time.
Change-Id: I0598054ee8a8b499ac686ea635a96f5d03c754e0
Amazingly, asn1_GetSequence isn't completely unused? Keep that around for now
and ditch everything else. This lets us enable C4311 in MSVC which is actually
a pretty reasonable warning.
Change-Id: I43bb9206b1745e8a68224f3a435713d2a74e04ea
Reviewed-on: https://boringssl-review.googlesource.com/4256
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: If15f2f0e2b4627318c9cdfbc76d5ca56a6894e3f
Reviewed-on: https://boringssl-review.googlesource.com/4270
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
all_tests.go will still complain if tab_test is missing.
Change-Id: I97c3684a4397caa55aaae2ec6555b16ee8366233
Reviewed-on: https://boringssl-review.googlesource.com/4250
Reviewed-by: Adam Langley <agl@google.com>
Implement ECDSA_SIG_new and ECDSA_SIG_free manually in preparation for
removing all crypto/asn1 dependencies from ECDSA signature verification.
Change-Id: I0e84d74fa8e757af0cfb09daef03d59f428143cc
Reviewed-on: https://boringssl-review.googlesource.com/4153
Reviewed-by: Adam Langley <agl@google.com>
Sadly, it turns out that we have need of this, at least for now. The
code is taken from upstream and changed only as much as needed.
This only imports keys and doesn't know how to actually perform
operations on them for now.
Change-Id: I0db70fb938186cb7a91d03f068b386c59ed90b84
After sharding the session cache for fallbacks, the numbers have been pretty
good; 0.03% on dev and 0.02% on canary. Stable is at 0.06% but does not have
the sharded session cache. Before sharding, stable, beta, and dev had been
fairly closely aligned. Between 0.03% being low and the fallback saving us in
all but extremely contrived cases, I think this should be fairly safe.
Add tests for both the cipher suite and protocol version mismatch checks.
BUG=441456
Change-Id: I2374bf64d0aee0119f293d207d45319c274d89ab
Reviewed-on: https://boringssl-review.googlesource.com/3972
Reviewed-by: Adam Langley <agl@google.com>
The encoding of an INTEGER should not have leading zeros, except to pad for the
sign bit.
Change-Id: I80d22818cf1d2ca9d27e215620392e1725372aa5
Reviewed-on: https://boringssl-review.googlesource.com/4218
Reviewed-by: Adam Langley <agl@google.com>
Also check for overflow, although it really shouldn't happen.
Change-Id: I34dfe8eaf635aeaa8bef2656fda3cd0bad7e1268
Reviewed-on: https://boringssl-review.googlesource.com/4235
Reviewed-by: Adam Langley <agl@google.com>
Separate actually writing the fragment to the network from assembling it so
there is no need for is_fragment. record_split_done also needn't be a global;
as of 7fdeaf1101, it is always reset to 0 whether
or not SSL3_WANT_WRITE occurred, despite the comment.
I believe this is sound, but the pre-7fdeaf1 logic wasn't quiiite right;
ssl3_write_pending allows a retry to supply *additional* data, so not all
plaintext had been commited to before the IV was randomized. We could fix this
by tracking how many bytes were committed to the last time we fragmented, but
this is purely an optimization and doesn't seem worth the complexity.
This also fixes the alignment computation in the record-splitting case. The
extra byte was wrong, as demonstrated by the assert.
Change-Id: Ia087a45a6622f4faad32e501942cc910eca1237b
Reviewed-on: https://boringssl-review.googlesource.com/4234
Reviewed-by: Adam Langley <agl@google.com>
This fixes the Windows build. Otherwise this collides with the symbol in
Chromium's //base. (The 'base' suffix is the name of the library, not some
Windows-ism.)
Change-Id: I65d755f08991978bd2040d53c401082b2fee65fa
Reviewed-on: https://boringssl-review.googlesource.com/4217
Reviewed-by: Adam Langley <agl@google.com>
It seems Android's inttypes.h refuses to define those macros on C++ unless
__STDC_FORMAT_MACROS is set. This unbreaks the roll on Android.
Change-Id: Iad6c971b4789f0302534d9e5022534c6124e0ff0
Reviewed-on: https://boringssl-review.googlesource.com/4202
Reviewed-by: Adam Langley <agl@google.com>
Not actually much of a change, but consistency.
Change-Id: If2ef7a8b698a229f5c494822d870767e1a61476e
Reviewed-on: https://boringssl-review.googlesource.com/4127
Reviewed-by: Adam Langley <agl@google.com>
Along the way, fix a host of missing failure checks. This will save some
headache when it comes time to run these under the malloc failure tests.
Change-Id: I3fd589bd094178723398e793d6bc578884e99b67
Reviewed-on: https://boringssl-review.googlesource.com/4126
Reviewed-by: Adam Langley <agl@google.com>
We don't really gain much from this one, but consistency.
Change-Id: I3f830c6d1ad65263bd1cc09372a5b810a8f690c0
Reviewed-on: https://boringssl-review.googlesource.com/4124
Reviewed-by: Adam Langley <agl@google.com>
This is in preparation for using RAII in the unit tests. Those tests are built
in Chromium as well, but Chromium does not have C++11 library support across
all its toolchains. Compiler support is available, so add a partial
reimplementation of std::unique_ptr and std::move under crypto/test/. The
scopers for the crypto/ library are also moved there while the ones for ssl/
stay in ssl/test/.
Change-Id: I38f769acbc16a870db34649928575c7314b6e9f6
Reviewed-on: https://boringssl-review.googlesource.com/4120
Reviewed-by: Adam Langley <agl@google.com>
Historically, OpenSSL has used callbacks for anything thread related,
but we don't actually have that many threading libraries to worry about:
just pthreads and Windows (I hope).
That suggests that it's quite reasonable to handle threading ourselves,
and eliminate the need for users to remember to install the thread
callbacks.
The first user of this would be ERR, which currently simulates
thread-local storage using a lock around a hash table keyed by the TID.
(Although I suspect that change will need some CMake work in order that
libpthread is automatically included with libcrypto when linking tests
etc, but not on Windows and without lots of ifs.)
Change-Id: I4dd088e3794506747f875c1f3e92b9bc6700fad2
Reviewed-on: https://boringssl-review.googlesource.com/4010
Reviewed-by: Adam Langley <agl@google.com>
(system/keymaster is using them now.)
Change-Id: I8fba501005b9318b7d3a76bf1715fb772b23c49d
Reviewed-on: https://boringssl-review.googlesource.com/4092
Reviewed-by: Adam Langley <agl@google.com>
gmtime_s first appeared in MSVCR80, but libmingwex has a helper function
that tries to find the symbol or falls back to an internal
implementation.
(Patch by Kenny Root.)
Change-Id: I96ef9cd7459d7e8202831a4e687dfbc055c9f50b
Reviewed-on: https://boringssl-review.googlesource.com/4091
Reviewed-by: Adam Langley <agl@google.com>
MinGW on Linux needs lowercase include files. On Windows this doesn't
matter since the filesystems are case-insensitive, but building
BoringSSL on Linux with MinGW has case-sensitive filesystems.
Change-Id: Id9c120d819071b041341fbb978352812d6d073bc
Reviewed-on: https://boringssl-review.googlesource.com/4090
Reviewed-by: Adam Langley <agl@google.com>
NEON_FUNCTIONAL is set by default in crypto.c. Chromium calls
|CRYPTO_set_NEON_functional| before |SSL_library_init| and thus the
getauxval path for CPU-feature detection was resetting the functional
flag, even on broken processors.
This change means that, apart from the default, only
|CRYPTO_set_NEON_functional| will change the NEON_FUNCTIONAL flag.
BUG=469511
Change-Id: I3d4dbbd9f4a5e33539f8559f90289e706ad17451
Reviewed-on: https://boringssl-review.googlesource.com/4170
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This affects bignum and sha. Also now that we're passing the SSE2 flag, revert
the change to ghash-x86.pl which unconditionally sets $sse2, just to minimize
upstream divergence. Chromium assumes SSE2 support, so relying on it is okay.
See https://crbug.com/349320.
Note: this change needs to be mirrored in Chromium to take.
bssl speed numbers:
SSE2:
Did 552 RSA 2048 signing operations in 3007814us (183.5 ops/sec)
Did 19003 RSA 2048 verify operations in 3070779us (6188.3 ops/sec)
Did 72 RSA 4096 signing operations in 3055885us (23.6 ops/sec)
Did 4650 RSA 4096 verify operations in 3024926us (1537.2 ops/sec)
Without SSE2:
Did 350 RSA 2048 signing operations in 3042021us (115.1 ops/sec)
Did 11760 RSA 2048 verify operations in 3003197us (3915.8 ops/sec)
Did 46 RSA 4096 signing operations in 3042692us (15.1 ops/sec)
Did 3400 RSA 4096 verify operations in 3083035us (1102.8 ops/sec)
SSE2:
Did 16407000 SHA-1 (16 bytes) operations in 3000141us (5468743.0 ops/sec): 87.5 MB/s
Did 4367000 SHA-1 (256 bytes) operations in 3000436us (1455455.1 ops/sec): 372.6 MB/s
Did 185000 SHA-1 (8192 bytes) operations in 3002666us (61611.9 ops/sec): 504.7 MB/s
Did 9444000 SHA-256 (16 bytes) operations in 3000052us (3147945.4 ops/sec): 50.4 MB/s
Did 2283000 SHA-256 (256 bytes) operations in 3000457us (760884.1 ops/sec): 194.8 MB/s
Did 89000 SHA-256 (8192 bytes) operations in 3016024us (29509.0 ops/sec): 241.7 MB/s
Did 5550000 SHA-512 (16 bytes) operations in 3000350us (1849784.2 ops/sec): 29.6 MB/s
Did 1820000 SHA-512 (256 bytes) operations in 3001039us (606456.6 ops/sec): 155.3 MB/s
Did 93000 SHA-512 (8192 bytes) operations in 3007874us (30918.8 ops/sec): 253.3 MB/s
Without SSE2:
Did 10573000 SHA-1 (16 bytes) operations in 3000261us (3524026.7 ops/sec): 56.4 MB/s
Did 2937000 SHA-1 (256 bytes) operations in 3000621us (978797.4 ops/sec): 250.6 MB/s
Did 123000 SHA-1 (8192 bytes) operations in 3033202us (40551.2 ops/sec): 332.2 MB/s
Did 5846000 SHA-256 (16 bytes) operations in 3000294us (1948475.7 ops/sec): 31.2 MB/s
Did 1377000 SHA-256 (256 bytes) operations in 3000335us (458948.8 ops/sec): 117.5 MB/s
Did 54000 SHA-256 (8192 bytes) operations in 3027962us (17833.8 ops/sec): 146.1 MB/s
Did 2075000 SHA-512 (16 bytes) operations in 3000967us (691443.8 ops/sec): 11.1 MB/s
Did 638000 SHA-512 (256 bytes) operations in 3000576us (212625.8 ops/sec): 54.4 MB/s
Did 30000 SHA-512 (8192 bytes) operations in 3042797us (9859.3 ops/sec): 80.8 MB/s
BUG=430237
Change-Id: I47d1c1ffcd71afe4f4a192272f8cb92af9505ee1
Reviewed-on: https://boringssl-review.googlesource.com/4130
Reviewed-by: Adam Langley <agl@google.com>
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>
I only imported half of upstream's c5f2b5336ab72e40ab91e2ca85639f51fa3178c6 on
accident.
Change-Id: Ice8185ca6770f915eb859e918f5db7d5ccdc7cc7
Reviewed-on: https://boringssl-review.googlesource.com/4045
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 9e488fd6ab2c295941e91a47ab7bcd346b7540c7)
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>
Upstream added another test vector in 4e049c52599d4a3fd918ba8570f49d88159e551b.
Change-Id: I17855dd479214657f0698b78f93e183cd6cb912e
Reviewed-on: https://boringssl-review.googlesource.com/3880
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 918bb8652969fd53f0c390c1cd909265ed502c7e. (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>
Change-Id: I7b6acc9004beb7b7090de1837814ccdff2e9930e
Reviewed-on: https://boringssl-review.googlesource.com/3680
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Upstream decided to make the caller free the scratch space rather than the
callee. May as well match. (Existing code is pretty inconsistent. This API
pattern needs to go.)
See upstream's 9e442d485008046933cdc7da65080f436a4af089.
Change-Id: I7c9fcae5778a74d6ae8e9f546e03fb2cf6e48426
Reviewed-on: https://boringssl-review.googlesource.com/3671
Reviewed-by: Adam Langley <agl@google.com>
EC_GROUP_copy is an rather unfriendly function; it doesn't work if the groups
have different[*] underlying EC_METHODs, but this notion is not exposed through
the API. I found no callers of EC_GROUP_copy in external code.
This leaves the precompute_mult functions as the remaining mutable API exposed
through EC_GROUP.
[*] Though, of the two EC_METHODs right now, simple.c is entirely unused.
Change-Id: Iabb52518005250fb970e12b3b0ea78b4f6eff4a0
Reviewed-on: https://boringssl-review.googlesource.com/3631
Reviewed-by: Adam Langley <agl@google.com>
They do the same thing. This removes all callers of EC_GROUP_copy outside
EC_GROUP_dup.
Change-Id: I65433ee36040de79e56483dfece774e01e2e2743
Reviewed-on: https://boringssl-review.googlesource.com/3630
Reviewed-by: Adam Langley <agl@google.com>
This reverts the non-ARM portions of 97999919bb.
x86_64 perlasm already makes .globl imply .hidden. (Confusingly, ARM does not.)
Since we don't need it, revert those to minimize divergence with upstream.
Change-Id: I2d205cfb1183e65d4f18a62bde187d206b1a96de
Reviewed-on: https://boringssl-review.googlesource.com/3610
Reviewed-by: Adam Langley <agl@google.com>
(I got this wrong when reading the OpenSSL code.)
Change-Id: Ib289ef41d0ab5a3157ad8b9454d2de96d1f86c22
Reviewed-on: https://boringssl-review.googlesource.com/3620
Reviewed-by: Adam Langley <agl@google.com>
This gives a standard PERL_EXECUTABLE configuration knob which is useful for
specifying a perl to use without having it in PATH.
Change-Id: I4b196b77e0b4666081a3f291fee3654c47925844
Reviewed-on: https://boringssl-review.googlesource.com/3570
Reviewed-by: Adam Langley <agl@google.com>
This involves more synchronization with child exits as the kernel no longer
closes the pre-created pipes for free, but it works on Windows. As long as
TCP_NODELAY is set, the performance seems comparable. Though it does involve
dealing with graceful socket shutdown. I couldn't get that to work on Windows
without draining the socket; not even SO_LINGER worked. Current (untested)
theory is that Windows refuses to gracefully shutdown a socket if the peer
sends data after we've stopped reading.
cmd.ExtraFiles doesn't work on Windows; it doesn't use fds natively, so you
can't pass fds 4 and 5. (stdin/stdout/stderr are special slots in
CreateProcess.) We can instead use the syscall module directly and mark handles
as inheritable (and then pass the numerical values out-of-band), but that
requires synchronizing all of our shim.Start() calls and assuming no other
thread is spawning a process.
PROC_THREAD_ATTRIBUTE_HANDLE_LIST fixes threading problems, but requires
wrapping more syscalls. exec.Cmd also doesn't let us launch the process
ourselves. Plus it still requires every handle in the list be marked
inheritable, so it doesn't help if some other thread is launching a process
with bInheritHandles TRUE but NOT using PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
(Like Go, though we can take syscall.ForkLock there.)
http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx
The more natively Windows option seems to be named pipes, but that too requires
wrapping more system calls. (To be fair, that isn't too painful.) They also
involve a listening server, so we'd still have to synchronize with shim.Wait()
a la net.TCPListener.
Then there's DuplicateHandle, but then we need an out-of-band signal.
All in all, one cross-platform implementation with a TCP sockets seems
simplest.
Change-Id: I38233e309a0fa6814baf61e806732138902347c0
Reviewed-on: https://boringssl-review.googlesource.com/3563
Reviewed-by: Adam Langley <agl@google.com>
Though this doesn't mean that masm becomes supported, the script is
still provided on don't-ask-in-case-of-doubt-use-nasm basis.
See RT#3650 for background.
(Imported from upstream's 2f8d82d6418c4de8330e2870c1ca6386dc9e1b34)
The data_word changes were already fixed with our
3e700bb3e8, but best to avoid diverging there.
Change-Id: Iab5455534e8bd632fb2b247ff792d411b105f17a
Reviewed-on: https://boringssl-review.googlesource.com/3581
Reviewed-by: Adam Langley <agl@google.com>
There is exactly one implementation and it doesn't fail. Plus a cleanup
function that can fail is very bad manners; the caller has no choice but to
leak at that point.
Change-Id: I5b524617ef37bc7d92273472fa742416ea7dfd43
Reviewed-on: https://boringssl-review.googlesource.com/3564
Reviewed-by: Adam Langley <agl@google.com>
RC4_CHAR is a bit in the x86(-64) CPUID information that switches the
RC4 asm code from using an array of 256 uint32_t's to 256 uint8_t's. It
was originally written for the P4, where the uint8_t style was faster.
(On modern chips, setting RC4_CHAR took RC4-MD5 from 458 to 304 MB/s.
Although I wonder whether, on a server with many connections, using less
cache wouldn't be better.)
However, I'm not too worried about a slowdown of RC4 on P4 systems these
days (the last new P4 chip was released nine years ago) and I want the
code to be simplier.
Also, RC4_CHAR was set when the CPUID family was 15, but Intel actually
lists 15 as a special code meaning "also check the extended family
bits", which the asm didn't do.
The RC4_CHAR support remains in the RC4 asm code to avoid drift with
upstream.
Change-Id: If3febc925a83a76f453b9e9f8de5ee43759927c6
Reviewed-on: https://boringssl-review.googlesource.com/3550
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
RC4_CHUNK is always defined, RC4_INT is always uint32_t and the
"register" keyword is an anachronism.
Change-Id: Ia752af30ba6bac0ee6216ce189fcf3888de73c6e
Reviewed-on: https://boringssl-review.googlesource.com/3544
Reviewed-by: Adam Langley <agl@google.com>
It was a mistake to remove this in the first place.
Change-Id: Icd97b4db01e49151daa41dd892f9da573ddc2842
Reviewed-on: https://boringssl-review.googlesource.com/3541
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
We are leaking asm symbols in Android builds because the asm code isn't
affected by -fvisibility=hidden. This change hides all asm symbols.
This assumes that no asm symbols are public API and that should be true.
Some points to note:
In crypto/rc4/asm/rc4-md5-x86_64.pl there are |RC4_set_key| and
|RC4_options| functions which aren't getting marked as hidden. That's
because those functions aren't actually ever generated. (I'm just trying
to minimise drift with upstream here.)
In crypto/rc4/asm/rc4-x86_64.pl there's |RC4_options| which is "public"
API, except that we've never had it in the header files. So I've just
deleted it. Since we have an internal caller, we'll probably have to put
it back in the future, but it can just be done in rc4.c to save
problems.
BUG=448386
Change-Id: I3846617a0e3d73ec9e5ec3638a53364adbbc6260
Reviewed-on: https://boringssl-review.googlesource.com/3520
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This is an import from ARM. Without this, one of the Android builds of
BoringSSL was failing with:
(sha512-armv4.o): requires unsupported dynamic reloc R_ARM_REL32; recompile with -fPIC
This is (I believe) a very misleading error message. The R_ARM_REL32
relocation type is the correct type for position independent code. But
unless the target symbol is hidden then the linker doesn't know that
it's not going to be overridden by a different ELF module.
Chromium probably gets away with this because of different default
compiler flags than Android.
Change-Id: I967eabc4d6b33d1e6635caaf6e7a306e4e77c101
Reviewed-on: https://boringssl-review.googlesource.com/3471
Reviewed-by: Adam Langley <agl@google.com>
It's never called in outside code. This too seems to be a remnant of the DSA
PKIX optional parameter stuff. This is confirmed both by a removed comment and
by the brief documentation at http://www.umich.edu/~x509/ssleay/x509_pkey.html
RFC 5480 does not allow ECDSA keys to be missing parameters, so this logic is
incorrect for ECDSA anyway. It was also failing to check
EVP_PKEY_copy_parameters' return value. And that logic looks pretty suspect if
you have a chain made up multiple certificate types.
Change-Id: Id6c60659a0162356c7f3eae5c797047366baae1c
Reviewed-on: https://boringssl-review.googlesource.com/3485
Reviewed-by: Adam Langley <agl@google.com>
Found while diagnosing some crashes and hangs in the malloc tests. This (and
the follow-up) get us further but does not quite let the malloc tests pass
quietly, even without valgrind. DTLS silently ignores some malloc failures
(confusion with silently dropping bad packets) which then translate to hangs.
Change-Id: Ief06a671e0973d09d2883432b89a86259e346653
Reviewed-on: https://boringssl-review.googlesource.com/3482
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I34ee66fcc53d3371591beee3373c46598c31b5c5
Reviewed-on: https://boringssl-review.googlesource.com/3460
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Reduces number of silly casts in OpenSSL code and likely most
applications. Consistent with (char *) for "peername" value from
X509_check_host() and X509_VERIFY_PARAM_get0_peername().
(Imported from upstream's e83c913723fac7432a7706812f12394aaa00e8ce.)
Change-Id: Id0fc11773a0cee8933978cd4bdbd8251fd7cfb5f
Pass address of X509_VERIFY_PARAM_ID peername to X509_check_host().
(Imported from upstream's 55fe56837a65ff505b492aa6aee748bf5fa91fec.)
Change-Id: Ic21bfb361b8eb25677c4c2175882fa95ea44fc31
(Imported from upstream's 8abffa4a73fcbf6536e0a42d736ed9211a8204ea,
9624b50d51de25bb2e3a72e81fe45032d80ea5c2 and
41e3ebd5abacfdf98461cdeb6fa97a4175b7aad3.)
Change-Id: Ic9099eb5704b19b4500229e89351371cc6184f9d
(This change is for a future change that increases the range of the
return values.)
(Imported from upstream's 3fc0b1edad0c75d7beb51fa77f63ffe817295e2c.)
Change-Id: I221d4ee0e90586f89f731e01ff4d813058173211
Just store NUL-terminated strings. This works better when we add
support for multiple hostnames.
(Imported from upstream's d93edc0aab98377f42dd19312248597a018a7889.)
Change-Id: Ib3bf8a8c654b829b4432782ba21ba55c3d4a0582
Caught by malloc valgrind tests on Basic-Client-Sync. Also one by inspection
and verified with valgrind. Those should pass now with the exception of
CRYPTO_free_ex_data being internally implemented with malloc.
(Clearly we next should make our malloc tests assert that the containing
function fails to catch when we fail to check for some error and things
silently move one.)
Change-Id: I56c51dc8a32a7d3c7ac907d54015dc241728c761
Reviewed-on: https://boringssl-review.googlesource.com/3440
Reviewed-by: Adam Langley <agl@google.com>
Win64 fires significantly more warnings than Win32. Also some recent
changes made it grumpy.
(We might want to reconsider enabling all of MSVC's warnings. Given the sorts
of warnings some of these are, I'm not sure MSVC's version of -Wall -Werror is
actually tenable. Plus, diverging from the Chromium build, especially before
the bots are ready, is going to break pretty readily.)
Change-Id: If3b8feccf910ceab4a233b0731e7624d7da46f87
Reviewed-on: https://boringssl-review.googlesource.com/3420
Reviewed-by: Adam Langley <agl@google.com>
This saves about 6-7k of error data.
Change-Id: Ic28593d4a1f5454f00fb2399d281c351ee57fb14
Reviewed-on: https://boringssl-review.googlesource.com/3385
Reviewed-by: Adam Langley <agl@google.com>
Some files in crypto/x509 were moved from crypto/asn1, so they emit errors from
another module. Fix make_errors.go to account for this: cross module errors
must use the foreign module as the first argument to OPENSSL_PUT_ERROR. Both
the function code and the error code should be declared in the foreign module.
Update make_errors.go to ignore cross-module error lines when deciding which
function tokens to emit.
Change-Id: Ic38377ddd56e22d033ef91318c30510762f6445d
Reviewed-on: https://boringssl-review.googlesource.com/3383
Reviewed-by: Adam Langley <agl@google.com>
We have got to get rid of that parameter...
Change-Id: I17f2d1282636f7d077f21dabdc135eecf9300998
Reviewed-on: https://boringssl-review.googlesource.com/3384
Reviewed-by: Adam Langley <agl@google.com>
Otherwise running git grep for a single function gives a ton of noise.
Change-Id: I18900d6269fd2be39ef9b579419aee1c7eca9143
Reviewed-on: https://boringssl-review.googlesource.com/3382
Reviewed-by: Adam Langley <agl@google.com>
Sort all the files before processing them.
Change-Id: Id6b4519fa22f1770bb2ba2a792f5c27de9ea452d
Reviewed-on: https://boringssl-review.googlesource.com/3380
Reviewed-by: Adam Langley <agl@google.com>
Also, Clang doesn't like static asserts with the same message and
ERR_free_strings should still free the error queues, although it's badly
misnamed now.
Change-Id: Ibff8eb50f93c0b56c3eeb17a300e8501a31c3ab8
Reviewed-on: https://boringssl-review.googlesource.com/3370
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Previously, error strings were kept in arrays for each subdirectory and
err.c would iterate over them all and insert them at init time to a hash
table.
This means that, even if you have a shared library and lots of processes
using that, each process has ~30KB of private memory from building that
hash table.
This this change, all the error strings are built into a sorted list and
are thus static data. This means that processes can share the error
information and it actually saves binary space because of all the
pointer overhead in the old scheme. Also it saves the time taken
building the hash table at startup.
This removes support for externally-supplied error string data.
Change-Id: Ifca04f335c673a048e1a3e76ff2b69c7264635be
Found by running malloc tests with -valgrind. Unfortunately, the next one is
deep in crypto/asn1 itself, so I'm going to stop here for now.
Change-Id: I7a33971ee07c6b7b7a98715f2f18e0f29380c0a1
Reviewed-on: https://boringssl-review.googlesource.com/3350
Reviewed-by: Adam Langley <agl@google.com>
The ENGINE code had a concept of a stable-ABI for METHODs, because that
might be a useful thing in the future when people want to have blobs
that wrap PKCS#11 or something.
However, at the moment nobody uses this feature and it didn't work very
well anyway: I hadn't updated |ENGINE_free| to free them all and
|set_method| was copying the methods, but not resetting the |is_static|
flag.
This change removes support for non-static methods. We can always put it
back later if we need.
Change-Id: Ic7401c8cb1cadd46b26a215f85bc48562efe9919
Reviewed-on: https://boringssl-review.googlesource.com/3300
Reviewed-by: Adam Langley <agl@google.com>
We assume that AArch64 systems are modern enough to have getauxval.
Change-Id: I4cc74f04ca5ed50b8ca1cfd00afeaaa01c6caca0
Reviewed-on: https://boringssl-review.googlesource.com/3280
Reviewed-by: Adam Langley <agl@google.com>
Tag number 31 is a long form tag that requires multiple octets. It
cannot be handled by adding a single uint8. Changed CBB_add_asn1()
to return 0 when it is passed in the extension for tag 31.
Change-Id: Ia33936d4f174d1a7176eb11da0b5c7370efb9416
CBB_init_fixed() should not call free because it can lead to use after
free or double free bugs. The caller should be responsible for
creating and destroying the buffer.
In the current code, ssl3_get_v2_client_hello() may free s->init_buf->data
via CBB_init_fixed(). It can also be freed via SSL_free(s) since
ssl3_get_v2_client_hello() doesn't set it to NULL and CBB_init_fixed()
can't set the caller's pointer to NULL.
Change-Id: Ia05a67ae25af7eb4fb04f08f20d50d912b41e38b
When addressing [1], I checked the AEAD code but brain-farted: a key is
aligned in that code, but it's the Poly1305 key, which doesn't matter
here.
It would be nice to align the ChaCha key too, but Android doesn't have
|posix_memalign| in the versions that we care about. It does have
|memalign|, but that's documented as "obsolete" and we don't have a
concept of an Android OS yet and I don't want to add one just for this.
So this change uses the buffer for loading the key again.
(Note that we never used to check for alignment of the |key| before
calling this. We must have gotten it for free somehow when checking the
alignment of |in| and |out|. But there are clearly some paths that don't
have an aligned key:
https://code.google.com/p/chromium/issues/detail?id=454308.)
At least the generation script started paying off immediately ☺.
[1] https://boringssl-review.googlesource.com/#/c/3132/1/crypto/chacha/chacha_vec.c@185
Change-Id: I4f893ba0733440fddd453f9636cc2aeaf05076ed
Reviewed-on: https://boringssl-review.googlesource.com/3270
Reviewed-by: Adam Langley <agl@google.com>
Thanks to an anonymous bug report.
Change-Id: Icdde78c82c8ee13fb64e0124712b240295677f63
Reviewed-on: https://boringssl-review.googlesource.com/3260
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
