Normally this would be pretty scary:
if (...) {
} if (...) {
}
But it's an early return anyway.
Change-Id: I0a8965b5e294d3aaa803be47f4006ea0311c431d
Reviewed-on: https://boringssl-review.googlesource.com/6851
Reviewed-by: Adam Langley <alangley@gmail.com>
It takes ownership of the buffer, so it's not actually const. The
const-ness gets dropped once it transits through EVP_PKEY_CTX_ctrl.
Also compare against INT_MAX explicitly for the overflow check. I'm not sure
whether the casting version is undefined, but comparing against INT_MAX matches
the rest of the codebase when transiting in and out of signed ints.
Change-Id: I131165a4b5f0ebe02c6db3e7e3e0d1af5b771710
Reviewed-on: https://boringssl-review.googlesource.com/6850
Reviewed-by: Adam Langley <alangley@gmail.com>
It's never used. It's not clear why one would want such a thing.
EVP_PKEY_CTX has no way for callers to register callbacks, which means
there shouldn't be a way for the library to present you an EVP_PKEY_CTX
out-of-context. (Whereas app_data/ex_data makes sense on SSL because of
its numerous callbacks or RSA because of RSA_METHOD.)
Change-Id: I55af537ab101682677af34f6ac1f2c27b5899a89
Reviewed-on: https://boringssl-review.googlesource.com/6849
Reviewed-by: Adam Langley <alangley@gmail.com>
This removes the last caller of EVP_PKEY_copy_parameters within the
library.
Change-Id: I6af138d364973b18f52baf55c36c50a24a56bd44
Reviewed-on: https://boringssl-review.googlesource.com/6848
Reviewed-by: Adam Langley <alangley@gmail.com>
foo_init hooks are never implemented. Even upstream never uses them. The
flags member is also never used. We also don't expose paramgen, so
remove it.
Change-Id: I51d9439316c5163520ab7168693c457f33e59417
Reviewed-on: https://boringssl-review.googlesource.com/6846
Reviewed-by: Adam Langley <alangley@gmail.com>
These are never called. Group parameters are not secret anyway. This is
a remnant of upstream's EC_GROUP_clear_free.
Change-Id: I23a4076eae8e4561abddbe74d0ba72641532f229
Reviewed-on: https://boringssl-review.googlesource.com/6823
Reviewed-by: Adam Langley <alangley@gmail.com>
There's many ways to serialize a BIGNUM, so not including asn1 in the name is
confusing (and collides with BN_bn2cbb_padded). Since BN_asn12bn looks
ridiculous, match the parse/marshal naming scheme of other modules instead.
Change-Id: I53d22ae0537a98e223ed943e943c48cb0743cf51
Reviewed-on: https://boringssl-review.googlesource.com/6822
Reviewed-by: Adam Langley <alangley@gmail.com>
The code was using `#define INLINE` instead, but we have `inline` so
use it.
Change-Id: Id05eaec4720061c5d9a7278e20127c2bebcb2495
Reviewed-on: https://boringssl-review.googlesource.com/6976
Reviewed-by: David Benjamin <davidben@google.com>
Commit 75a64c08fc missed one case where
the GCC syntax should have been replaced with |alignas|.
Change-Id: Iebdaa9c9a2c0aff171f0b5d4daac607e351a4b7e
Reviewed-on: https://boringssl-review.googlesource.com/6974
Reviewed-by: David Benjamin <davidben@google.com>
The uses of |memcpy| to cast pointer-to-function to pointer-to-data and
back again did not have well-defined semantics. Use a union instead to
avoid the need for such a conversion get well-defined semantics.
Change-Id: I8ee54a83ba75440f7bc78c194eb55e2cf09b05d8
Reviewed-on: https://boringssl-review.googlesource.com/6972
Reviewed-by: David Benjamin <davidben@google.com>
Casting a pointer-to-non-volatile to pointer-to-volatile can be a no-op
as the compiler only requires volatile semantics when the pointed-to
object is a volatile object and there are no pointers-to-non-volatile
involved. This probably doesn't matter unless building with the MSVC
-volatile:iso flag, and maybe not even then, but it is good practice
anyway.
Change-Id: I94900d3dc61de3b8ce2ddecab2811907a9a7adbf
Reviewed-on: https://boringssl-review.googlesource.com/6973
Reviewed-by: David Benjamin <davidben@google.com>
Division isn't constant-time on Intel chips so the code was adding a
large multiple of md_size to try and force the operation to always take
the maximum amount of time.
I'm less convinced, these days, that compilers aren't going to get smart
enough to optimise that away so use Barrett reduction instead.
Change-Id: Ib8c514192682a2fcb4b1fb7e7c6dd1301d9888d0
Reviewed-on: https://boringssl-review.googlesource.com/6906
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <alangley@gmail.com>
node.js is, effectively, another bindings library. However, it's better
written than most and, with these changes, only a couple of tiny fixes
are needed in node.js. Some of these changes are a little depressing
however so we'll need to push node.js to use APIs where possible.
Changes:
∙ Support verify_recover. This is very obscure and the motivation
appears to be https://github.com/nodejs/node/issues/477 – where it's
not clear that anyone understands what it means :(
∙ Add a few, no-op #defines
∙ Add some members to |SSL_CTX| and |SSL| – node.js needs to not
reach into these structs in the future.
∙ Add EC_get_builtin_curves.
∙ Add EVP_[CIPHER|MD]_do_all_sorted – these functions are limited to
decrepit.
Change-Id: I9a3566054260d6c4db9d430beb7c46cc970a9d46
Reviewed-on: https://boringssl-review.googlesource.com/6952
Reviewed-by: Adam Langley <agl@google.com>
Apparently OpenSSL's API is made entirely of initialization functions.
Some external libraries like to initialize with OPENSSL_config instead.
Change-Id: I28efe97fc5eb21309f560c84112b80e947f8bb17
Reviewed-on: https://boringssl-review.googlesource.com/6981
Reviewed-by: Adam Langley <agl@google.com>
With these stubs, cURL should not need any BoringSSL #ifdefs at all,
except for their OCSP #ifdefs (which can switch to the more generally
useful OPENSSL_NO_OCSP) and the workaround for wincrypt.h macro
collisions. That we intentionally leave to the consumer rather than add
a partial hack that makes the build sensitive to include order.
(I'll send them a patch upstream once this cycles in.)
Change-Id: I815fe67e51e80e9aafa9b91ae68867ca1ff1d623
Reviewed-on: https://boringssl-review.googlesource.com/6980
Reviewed-by: Adam Langley <agl@google.com>
Since the error string logic was rewritten, this hasn't done anything.
Change-Id: Icb73dca65e852bb3c7d04c260d591906ec72c15f
Reviewed-on: https://boringssl-review.googlesource.com/6961
Reviewed-by: Adam Langley <agl@google.com>
MSVC doesn't have stdalign.h and so doesn't support |alignas| in C
code. Define |alignas(x)| as a synonym for |__decltype(align(x))|
instead for it.
This also fixes -Wcast-qual warnings in rsaz_exp.c.
Change-Id: Ifce9031724cb93f5a4aa1f567e7af61b272df9d5
Reviewed-on: https://boringssl-review.googlesource.com/6924
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
After its initial assignment, |e| is immediately reassigned another
value and so the initial assignment from |BN_CTX_get| is useless. If
that were not the case, then the |BN_free(e)| at the end of the
function would be very bad.
Change-Id: Id63a172073501c8ac157db9188a22f55ee36b205
Reviewed-on: https://boringssl-review.googlesource.com/6951
Reviewed-by: David Benjamin <davidben@google.com>
This is only for Conscrypt which always calls the pair in succession. (Indeed
it wouldn't make any sense to not call it.) Remove those two APIs and replace
with a single merged API. This way incomplete EC_GROUPs never escape outside
our API boundary and EC_GROUPs may *finally* be made immutable.
Also add a test for this to make sure I didn't mess it up.
Add a temporary BORINGSSL_201512 define to ease the transition for Conscrypt.
Conscrypt requires https://android-review.googlesource.com/#/c/187801/ before
picking up this change.
Change-Id: I3706c2ceac31ed2313175ba5ee724bd5c74ef6e1
Reviewed-on: https://boringssl-review.googlesource.com/6550
Reviewed-by: Adam Langley <agl@google.com>
Fix casts from const to non-const where dropping the constness is
completely unnecessary. The changes to chacha_vec.c don't result in any
changes to chacha_vec_arm.S.
Change-Id: I2f10081fd0e73ff5db746347c5971f263a5221a6
Reviewed-on: https://boringssl-review.googlesource.com/6923
Reviewed-by: David Benjamin <davidben@google.com>
Fix the signness of the format flag in the |sscanf| call in cpu-intel.c.
Change-Id: I31251d79aa146bf9c78be47020ee83d30864a3d2
Reviewed-on: https://boringssl-review.googlesource.com/6921
Reviewed-by: David Benjamin <davidben@google.com>
This centralizes the conditional logic into openssl/base.h so that it
doesn't have to be repeated. The name |OPENSSL_PRINTF_FORMAT_FUNC| was
chosen in anticipation of eventually defining an
|OPENSSL_PRINTF_FORMAT_ARG| for MSVC-style parameter annotations.
Change-Id: I273e6eddd209e696dc9f82099008c35b6d477cdb
Reviewed-on: https://boringssl-review.googlesource.com/6909
Reviewed-by: David Benjamin <davidben@google.com>
Some combination of Chromium's copy of clang and Chromium's Linux sysroot
doesn't like syntax. It complains that "chosen constructor is explicit in
copy-initialization".
Change-Id: Ied6bc17b19421998f926483742510c81f732566b
Reviewed-on: https://boringssl-review.googlesource.com/6930
Reviewed-by: Adam Langley <agl@google.com>
This change imports the following changes from upstream:
6281abc79623419eae6a64768c478272d5d3a426
dfd3322d72a2d49f597b86dab6f37a8cf0f26dbf
f34b095fab1569d093b639bfcc9a77d6020148ff
21376d8ae310cf0455ca2b73c8e9f77cafeb28dd
25efcb44ac88ab34f60047e16a96c9462fad39c1
56353962e7da7e385c3d577581ccc3015ed6d1dc
39c76ceb2d3e51eaff95e04d6e4448f685718f8d
a3d74afcae435c549de8dbaa219fcb30491c1bfb
These contain the “altchains” functionality which allows OpenSSL to
backtrack when chain building.
Change-Id: I8d4bc2ac67b90091f9d46e7355cae878b4ccf37d
Reviewed-on: https://boringssl-review.googlesource.com/6905
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL upstream did a bulk reformat. We still have some files that have
the old OpenSSL style and this makes applying patches to them more
manual, and thus more error-prone, than it should be.
This change is the result of running
util/openssl-format-source -v -c .
in the enumerated directories. A few files were in BoringSSL style and
have not been touched.
This change should be formatting only; no semantic difference.
Change-Id: I75ced2970ae22b9facb930a79798350a09c5111e
Reviewed-on: https://boringssl-review.googlesource.com/6904
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
(Comment-only change; no functional difference.)
Some code was broken by the |d2i_ECDSA_SIG| change in 87897a8c. It was
passing in a pointer to an existing |ECDSA_SIG| as the first argument
and then simply assuming that the structure would be updated in place.
The comments on the function suggested that this was reasonable.
This change updates the comments that use similar wording to either note
that the function will never update in-place, or else to note that
depending on that is a bad idea for the future.
I've also audited all the uses of these functions that I can find and,
in addition to the one case with |d2i_ECDSA_SIG|, there are several
users of |d2i_PrivateKey| that could become a problem in the future.
I'll try to fix them before it does become an issue.
Change-Id: I769f7b2e0b5308d09ea07dd447e02fc161795071
Reviewed-on: https://boringssl-review.googlesource.com/6902
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
These symbols can show up in lists of large symbols but, so I
understand, these lists might not include the filename path. Thus |base|
as a symbol name is rather unhelpful.
This change renames the two precomputated tables to have slightly more
greppable names.
Change-Id: I77059250cfce4fa9eceb64e260b45db552b63255
Reviewed-on: https://boringssl-review.googlesource.com/6813
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <alangley@gmail.com>
rsa_default_encrypt allowed an RSA modulus 8 times larger than the
intended maximum size due to bits vs. bytes confusion.
Further, as |rsa_default_encrypt| got this wrong while
|rsa_default_verify_raw| got it right, factor out the duplicated logic
so that such inconsistencies are less likely to occur.
BUG=576856
Change-Id: Ic842fadcbb3b140d2ba4295793457af2b62d9444
Reviewed-on: https://boringssl-review.googlesource.com/6900
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Some build systems don't like two targets with the same base name and
the curve25519 code had x25519-x86_64.[Sc].
Change-Id: If8382eb84996d7e75b34b28def57829d93019cff
Reviewed-on: https://boringssl-review.googlesource.com/6878
Reviewed-by: Adam Langley <agl@google.com>
Commit 2b0180c37fa6ffc48ee40caa831ca398b828e680 attempted to do this but
only hit one of many BN_mod_exp codepaths. Fix remaining variants and
add a test for each method.
Thanks to Hanno Boeck for reporting this issue.
(Imported from upstream's 44e4f5b04b43054571e278381662cebd3f3555e6.)
Change-Id: Ic691b354101c3e9c3565300836fb6d55c6f253ba
Reviewed-on: https://boringssl-review.googlesource.com/6820
Reviewed-by: Adam Langley <agl@google.com>
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>
This relieves some complexity budget for adding Curve25519 to this
code.
This also adds a BN_bn2cbb_padded helper function since this seems to be a
fairly common need.
Change-Id: Ied0066fdaec9d02659abd6eb1a13f33502c9e198
Reviewed-on: https://boringssl-review.googlesource.com/6767
Reviewed-by: Adam Langley <agl@google.com>
This assembly is in gas syntax so is not built on Windows nor when
OPENSSL_SMALL is defined.
Change-Id: I1050cf1b16350fd4b758e4c463261b30a1b65390
Reviewed-on: https://boringssl-review.googlesource.com/6782
Reviewed-by: Adam Langley <agl@google.com>
These will be needed when we start writing variable-length things to a
CBB.
Change-Id: Ie7b9b140f5f875b43adedc8203ce9d3f4068dfea
Reviewed-on: https://boringssl-review.googlesource.com/6764
Reviewed-by: Adam Langley <agl@google.com>
A lot of commented-out code we haven't had to put them back, so these
can go now. Also remove the TODO about OAEP having a weird API. The API
is wrong, but upstream's shipped it with the wrong API, so that's what
it is now.
Change-Id: I7da607cf2d877cbede41ccdada31380f812f6dfa
Reviewed-on: https://boringssl-review.googlesource.com/6763
Reviewed-by: Adam Langley <agl@google.com>
There was a TODO to remove it once asn1_mac.h was trimmed. This has now
happened. Remove it and reset error codes for crypto/asn1.
Change-Id: Iaf2f3e75741914415372939471b135618910f95d
Reviewed-on: https://boringssl-review.googlesource.com/6761
Reviewed-by: Adam Langley <agl@google.com>
The function X509_verify_cert checks the value of |ctx->chain| at the
beginning, and if it is NULL then it initialises it, along with the value
of |ctx->untrusted|. The normal way to use X509_verify_cert() is to first
call X509_STORE_CTX_init(); then set up various parameters etc; then call
X509_verify_cert(); then check the results; and finally call
X509_STORE_CTX_cleanup(). The initial call to X509_STORE_CTX_init() sets
|ctx->chain| to NULL. The only place in the OpenSSL codebase where
|ctx->chain| is set to anything other than a non NULL value is in
X509_verify_cert itself. Therefore the only ways that |ctx->chain| could be
non NULL on entry to X509_verify_cert is if one of the following occurs:
1) An application calls X509_verify_cert() twice without re-initialising
in between.
2) An application reaches inside the X509_STORE_CTX structure and changes
the value of |ctx->chain| directly.
With regards to the second of these, we should discount this - it should
not be supported to allow this.
With regards to the first of these, the documentation is not exactly
crystal clear, but the implication is that you must call
X509_STORE_CTX_init() before each call to X509_verify_cert(). If you fail
to do this then, at best, the results would be undefined.
Calling X509_verify_cert() with |ctx->chain| set to a non NULL value is
likely to have unexpected results, and could be dangerous. This commit
changes the behaviour of X509_verify_cert() so that it causes an error if
|ctx->chain| is anything other than NULL (because this indicates that we
have not been initialised properly). It also clarifies the associated
documentation.
(Imported from upstream's 692f07c3e0c04180b56febc2feb57cd94395a7a2.)
Change-Id: I971f1a305f12bbf9f4ae955313d5557368f0d374
Reviewed-on: https://boringssl-review.googlesource.com/6760
Reviewed-by: Adam Langley <agl@google.com>
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>
We should reject RSA public keys with exponents of less than 3.
This change also rejects even exponents, although the usefulness
of such a public key is somewhat questionable.
BUG=chromium:541257
Change-Id: I1499e9762ba40a7cf69155d21d55bc210cd6d273
Reviewed-on: https://boringssl-review.googlesource.com/6710
Reviewed-by: Adam Langley <agl@google.com>
OPENSSL_SMALL will still cause the smaller base-point table to be used
and so won't be as fast at signing as the full version, but Ed25519 will
now work in those builds.
Without OPENSSL_SMALL:
Did 20000 Ed25519 key generation operations in 1008347us (19834.4 ops/sec)
Did 20000 Ed25519 signing operations in 1025594us (19500.9 ops/sec)
Did 6138 Ed25519 verify operations in 1001712us (6127.5 ops/sec)
Did 21000 Curve25519 base-point multiplication operations in 1019237us (20603.6 ops/sec)
Did 7095 Curve25519 arbitrary point multiplication operations in 1065986us (6655.8 ops/sec)
With (on the same machine):
Did 8415 Ed25519 key generation operations in 1020958us (8242.3 ops/sec)
Did 8952 Ed25519 signing operations in 1077635us (8307.1 ops/sec)
Did 6358 Ed25519 verify operations in 1047533us (6069.5 ops/sec)
Did 6620 Curve25519 base-point multiplication operations in 1008922us (6561.5 ops/sec)
Did 7183 Curve25519 arbitrary point multiplication operations in 1096285us (6552.1 ops/sec)
Change-Id: Ib443c0e2bdfd11e044087e66efd55b651a5667e7
Reviewed-on: https://boringssl-review.googlesource.com/6772
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
