MSVC doesn't like unary minus on unsigned types. Also, the speed test
always failed because the inputs were all zeros and thus had small
order.
Change-Id: Ic2d3c2c9bd57dc66295d93891396871cebac1e0b
It can fail on FreeBSD when library is not linked against either
threading library and results in init routine not being executed
at all, leading to errors in other parts of the code.
Change-Id: I1063f6940e381e6470593c063fbfecf3f47991cd
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/6522
Reviewed-by: Adam Langley <agl@google.com>
Relevant code was removed in 5d5e39f5d2.
Change-Id: I198844064030c04f88e5541f2bbaa29ae13d14bb
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/6521
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I3072f884be77b8646e90d316154b96448f0cf2a1
Reviewed-on: https://boringssl-review.googlesource.com/6520
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
So long as we're not getting rid of them (the certificate variants may
be useful when we decouple from crypto/x509 anyway), get the types and
bounds checks right.
Also reject trailing data and require the input be a single element.
Note: this is a slight compatibility risk, but we did it for
SSL*_use_RSAPrivateKey_ASN1 previously and I think it's probably worth
seeing if anything breaks here.
Change-Id: I64fa3fc6249021ccf59584d68e56ff424a190082
Reviewed-on: https://boringssl-review.googlesource.com/6490
Reviewed-by: Adam Langley <agl@google.com>
This codepath should not actually be reachable, unless maybe the caller is
doing something really dumb. (Unconfiguring the key partway through the
connection.)
Change-Id: Ic8e0cfc3c426439016370f9a85be9c05509358f1
Reviewed-on: https://boringssl-review.googlesource.com/6483
Reviewed-by: Adam Langley <agl@google.com>
TLS resets it in t1_enc.c while DTLS has it sprinkled everywhere.
Change-Id: I78f0f0e646b4dc82a1058199c4b00f2e917aa5bc
Reviewed-on: https://boringssl-review.googlesource.com/6511
Reviewed-by: Adam Langley <agl@google.com>
stdint.h already has macros for this. The spec says that, in C++,
__STDC_CONSTANT_MACROS is needed, so define it for bytestring_test.cc.
Chromium seems to use these macros without trouble, so I'm assuming we
can rely on them.
Change-Id: I56d178689b44d22c6379911bbb93d3b01dd832a3
Reviewed-on: https://boringssl-review.googlesource.com/6510
Reviewed-by: Adam Langley <agl@google.com>
Until we've done away with the d2i_* stack completely, boundaries need
to be mindful of the type mismatch. d2i_* takes a long, not a size_t.
Change-Id: If02f9ca2cfde02d0929ac18275d09bf5df400f3a
Reviewed-on: https://boringssl-review.googlesource.com/6491
Reviewed-by: Adam Langley <agl@google.com>
Triggered by RT#3989.
(Imported from upstream's fbab8baddef8d3346ae40ff068871e2ddaf10270. This
doesn't seem to affect us, but avoid getting out of sync.)
Change-Id: I164e2a72e4b75e286ceaa03745ed9bcbf6c3e32e
Reviewed-on: https://boringssl-review.googlesource.com/6512
Reviewed-by: Adam Langley <agl@google.com>
To no great surprise, ASAN didn't like this test and I suspect that
Chromium, with its crashing allocator, won't like it either. Oh well.
Change-Id: I235dbb965dbba186f8f37d7df45f8eac9addc7eb
Reviewed-on: https://boringssl-review.googlesource.com/6496
Reviewed-by: Adam Langley <agl@google.com>
People expect to do:
CBB foo;
if (!CBB_init(&foo, 100) ||
…
…) {
CBB_cleanup(&foo);
return 0;
}
However, currently, if the allocation of |initial_capacity| fails in
|CBB_init| then |CBB_cleanup| will operate on uninitialised values. This
change makes the above pattern safe.
Change-Id: I3e002fda8f0a3ac18650b504e7e84a842d4165ca
Reviewed-on: https://boringssl-review.googlesource.com/6495
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
OpenSSH calls |RAND_seed| before jailing in the expectation that that
will be sufficient to ensure that later RAND calls are successful.
See internal bug 25695426.
Change-Id: I9d3f5665249af6610328ac767cb83059bb2953dd
Reviewed-on: https://boringssl-review.googlesource.com/6494
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
|EC_POINT_point2oct| would encode ∞, which is surprising, and
|EC_POINT_oct2point| would decode ∞, which is insane. This change
removes both behaviours.
Thanks to Brian Smith for pointing it out.
Change-Id: Ia89f257dc429a69b9ea7b7b15f75454ccc9c3bdd
Reviewed-on: https://boringssl-review.googlesource.com/6488
Reviewed-by: Adam Langley <agl@google.com>
In the case of a compressed point, the decompression ensures that the
point is on the curve. In the uncompressed case,
|EC_POINT_set_affine_coordinates_GFp| checks that the point is on the
curve as of 38feb990a1.
Change-Id: Icd69809ae396838b4aef4fa89b3b354560afed55
Reviewed-on: https://boringssl-review.googlesource.com/6487
Reviewed-by: Brian Smith <brian@briansmith.org>
Reviewed-by: Adam Langley <agl@google.com>
There's a few things that will be kind of a nuisance and possibly not worth it
(crypto/asn1 dumps a lot of undeclared things, etc.). But it caught some
mistakes. Even without the warning, making sure to include the externs before
defining a function helps catch type mismatches.
Change-Id: I3dab282aaba6023e7cebc94ed7a767a5d7446b08
Reviewed-on: https://boringssl-review.googlesource.com/6484
Reviewed-by: Adam Langley <agl@google.com>
|EC_GFp_nistz256_method| is not marked |OPENSSL_EXPORT| so only the
built-in P-256 curve uses it. |EC_GROUP_set_generator| doesn't allow
the generator to be changed for any |EC_GROUP| for a built-in curve.
Consequently, there's no way (except some kind of terrible abuse) that
the nistz code could be executed with a non-default generator.
Change-Id: Ib22f00bc74c103b7869ed1e35032b1f3d26cdad2
Reviewed-on: https://boringssl-review.googlesource.com/6446
Reviewed-by: Adam Langley <agl@google.com>
Found with -Wtype-limits.
Change-Id: I5580f179425bc6b09ff2a8559fce121b0cc8ae14
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/6463
Reviewed-by: Adam Langley <agl@google.com>
Found with -Wtype-limits.
Change-Id: I41cdbb7e6564b715dfe445877a89594371fdeef0
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/6462
Reviewed-by: Adam Langley <agl@google.com>
Chromium's toolchains may now assume C++11 library support, so we may freely
use C++11 features. (Chromium's still in the process of deciding what to allow,
but we use Google's style guide directly, toolchain limitations aside.)
Change-Id: I1c7feb92b7f5f51d9091a4c686649fb574ac138d
Reviewed-on: https://boringssl-review.googlesource.com/6465
Reviewed-by: Adam Langley <agl@google.com>
dh.c had a 10k-bit limit but it wasn't quite correctly enforced. However,
that's still 1.12s of jank on the IO thread, which is too long. Since the SSL
code consumes DHE groups from the network, it should be responsible for
enforcing what sanity it needs on them.
Costs of various bit lengths on 2013 Macbook Air:
1024 - 1.4ms
2048 - 14ms
3072 - 24ms
4096 - 55ms
5000 - 160ms
10000 - 1.12s
UMA says that DHE groups are 0.2% 4096-bit and otherwise are 5.5% 2048-bit and
94% 1024-bit and some noise. Set the limit to 4096-bit to be conservative,
although that's already quite a lot of jank.
BUG=554295
Change-Id: I8e167748a67e4e1adfb62d73dfff094abfa7d215
Reviewed-on: https://boringssl-review.googlesource.com/6464
Reviewed-by: Adam Langley <agl@google.com>
The current check has two problems:
- It only runs on the server, where there isn't a curve list at all. This was a
mistake in https://boringssl-review.googlesource.com/1843 which flipped it
from client-only to server-only.
- It only runs in TLS 1.2, so one could bypass it by just negotiating TLS 1.1.
Upstream added it as part of their Suite B mode, which requires 1.2.
Move it elsewhere. Though we do not check the entire chain, leaving that to the
certificate verifier, signatures made by the leaf certificate are made by the
SSL/TLS stack, so it's reasonable to check the curve as part of checking
suitability of a leaf.
Change-Id: I7c12f2a32ba946a20e9ba6c70eff23bebcb60bb2
Reviewed-on: https://boringssl-review.googlesource.com/6414
Reviewed-by: Adam Langley <agl@google.com>
It's easier to put libFuzzer.a into the source directory than to install
it globally.
Change-Id: I4dc7b56f81c7aa0371475c68d23368b025186505
Reviewed-on: https://boringssl-review.googlesource.com/6461
Reviewed-by: Adam Langley <agl@google.com>
The file armv8-mont.pl is taken from upstream. The speed ups are fairly
modest (~30%) but seem worthwhile.
Before:
Did 231 RSA 2048 signing operations in 1008671us (229.0 ops/sec)
Did 11208 RSA 2048 verify operations in 1036997us (10808.1 ops/sec)
Did 342 RSA 2048 (3 prime, e=3) signing operations in 1021545us (334.8 ops/sec)
Did 32000 RSA 2048 (3 prime, e=3) verify operations in 1016162us (31491.0 ops/sec)
Did 45 RSA 4096 signing operations in 1039805us (43.3 ops/sec)
Did 3608 RSA 4096 verify operations in 1060283us (3402.9 ops/sec)
After:
Did 300 RSA 2048 signing operations in 1009772us (297.1 ops/sec)
Did 12740 RSA 2048 verify operations in 1075413us (11846.6 ops/sec)
Did 408 RSA 2048 (3 prime, e=3) signing operations in 1016139us (401.5 ops/sec)
Did 33000 RSA 2048 (3 prime, e=3) verify operations in 1017510us (32432.1 ops/sec)
Did 52 RSA 4096 signing operations in 1067678us (48.7 ops/sec)
Did 3408 RSA 4096 verify operations in 1062863us (3206.4 ops/sec)
Change-Id: Ife74fac784067fce3668b5c87f51d481732ff855
Reviewed-on: https://boringssl-review.googlesource.com/6444
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 52e028b9de371da62c1e51b46592517b1068d770.)
Change-Id: If980d774671b9b5ba997db3fd7d4043525a85609
Reviewed-on: https://boringssl-review.googlesource.com/6445
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
PKCS#12 files may not necessarily include keys.
Change-Id: Ibb43b609783b02aa9cbb192fea377081169666ff
Reviewed-on: https://boringssl-review.googlesource.com/6456
Reviewed-by: Adam Langley <agl@google.com>
There seems to have been a merge error.
Change-Id: I72e5c2a45c148e31c90b28bedfff48f8ca6e3c8c
Reviewed-on: https://boringssl-review.googlesource.com/6455
Reviewed-by: Adam Langley <agl@google.com>
This exposes the ServerKeyExchange signature hash type used in the most recent
handshake, for histogramming on the client.
BUG=549662
Change-Id: I8a4e00ac735b1ecd2c2df824112c3a0bc62332a7
Reviewed-on: https://boringssl-review.googlesource.com/6413
Reviewed-by: Adam Langley <agl@google.com>
This is completely a no-op as currently tls12_get_psigalgs always returns a
hardcoded list which always includes SHA-1. But if this were to be made
configurable in the future, we should reject SHA-1 when configured to do so.
Change-Id: I7ab188eeff850d1e5f70b9522304812bab2d941a
Reviewed-on: https://boringssl-review.googlesource.com/6411
Reviewed-by: Adam Langley <agl@google.com>
When building in OPENSSL_NO_ASM mode, MSVC complains about unreachable
code. The redundant initialization of |i| is the main problem. The
skipping of the first test of the condition |i < num| with |goto| was
also confusing.
It turns out that |bn_mul_mont| is only called when assembly language
optimizations are available, but in that case the assmebly language
versions will always be used instead. Although this code will be
compiled in |OPENSSL_NO_ASM| builds, it is never called in
|OPENSSL_NO_ASM| builds. Thus, it can just be removed.
Change-Id: Id551899b2602824978edc1a1cb0703b76516808d
Reviewed-on: https://boringssl-review.googlesource.com/5550
Reviewed-by: Adam Langley <agl@google.com>
I've used these defines to easy the update of BoringSSL in Android
because Android's external/boringssl is a different git repository from
the rest of Android and thus it's not possible to land changes the
atomically update several things at once.
For this I tended just to add this define in the Android copy of
BoringSSL, but we're starting to see that bleed into other situations
now so it's looking like this will be generally useful.
These defines may be added when useful but shouldn't build up: once the
change has been done, the #if'ed code elsewhere that uses it should be
cleaned up. So far, that's worked ok. (I.e. we've had a BORINGSSL_201509
that correctly disappeared.)
Change-Id: I8cbb4731efe840cc798c970d37bc040b16a4a755
Reviewed-on: https://boringssl-review.googlesource.com/6442
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Not sure if we want to leave bio.h and bytestring.h's instance as-is, but the
evp.h ones are just baffling.
Change-Id: I485c2e355ba93764da0c4c72c48af48b055a8500
Reviewed-on: https://boringssl-review.googlesource.com/6454
Reviewed-by: Adam Langley <agl@google.com>
This fixes a number of bugs with the original logic:
- If handshake messages are fragmented and writes need to be retried, frag_off
gets completely confused.
- The BIO_flush call didn't set rwstate, so it wasn't resumable at that point.
- The msg_callback call gets garbage because the fragment header would get
scribbled over the handshake buffer.
The original logic was also extremely confusing with how it handles init_off.
(init_off gets rewound to make room for the fragment header. Depending on
where you pause, resuming may or may not have already been rewound.)
For simplicity, just allocate a new buffer to assemble the fragment in and
avoid clobbering the old one. I don't think it's worth the complexity to
optimize that. If we want to optimize this sort of thing, not clobbering seems
better anyway because the message may need to be retransmitted. We could avoid
doing a copy when buffering the outgoing message for retransmission later.
We do still need to track how far we are in sending the current message via
init_off, so I haven't opted to disconnect this function from
init_{buf,off,num} yet.
Test the fix to the retry + fragment case by having the splitHandshake option
to the state machine tests, in DTLS, also clamp the MTU to force handshake
fragmentation.
Change-Id: I66f634d6c752ea63649db8ed2f898f9cc2b13908
Reviewed-on: https://boringssl-review.googlesource.com/6421
Reviewed-by: Adam Langley <agl@google.com>
This was a mistake from when we added async CertificateVerify support.
No test because the final state of each write state is semi-unreachable
due to the buffer BIO that gets installed on each handshake.
Change-Id: I0180926522113c8b1ca58b8c9c6dc37fb0dd8083
Reviewed-on: https://boringssl-review.googlesource.com/6412
Reviewed-by: Adam Langley <agl@google.com>
The previous logic only defined
|SHA512_BLOCK_CAN_MANAGE_UNALIGNED_DATA| when the assembly language
optimizations were enabled, but
|SHA512_BLOCK_CAN_MANAGE_UNALIGNED_DATA| is also useful when the C
implementations are used.
If support for ARM processors that don't support unaligned access is
important, then it might be better to condition the enabling of
|SHA512_BLOCK_CAN_MANAGE_UNALIGNED_DATA| on ARM based on more specific
flags.
Change-Id: Ie8c37c73aba308c3ccf79371ce5831512e419989
Reviewed-on: https://boringssl-review.googlesource.com/6402
Reviewed-by: Adam Langley <agl@google.com>
Most functions can take this in as const. Note this changes an
RSA_METHOD hook, though one I would not expect anyone to override.
Change-Id: Ib70ae65e5876b01169bdc594e465e3e3c4319a8b
Reviewed-on: https://boringssl-review.googlesource.com/6419
Reviewed-by: Adam Langley <agl@google.com>
Later when TLS 1.3 comes around, we'll need SSL_CIPHER_get_max_version too. In
the meantime, hide the SSL_TLSV1_2 messiness behind a reasonable API.
Change-Id: Ibcc17cccf48dd99e364d6defdfa5a87d031ecf0a
Reviewed-on: https://boringssl-review.googlesource.com/6452
Reviewed-by: Adam Langley <agl@google.com>
This removes a sharp corner in the API where |ECDH_compute_key| assumed
that callers were either using ephemeral keys, or else had already
checked that the public key was on the curve.
A public key that's not on the curve can be in a small subgroup and thus
the result can leak information about the private key.
This change causes |EC_POINT_set_affine_coordinates_GFp| to require that
points are on the curve. |EC_POINT_oct2point| already does this.
Change-Id: I77d10ce117b6efd87ebb4a631be3a9630f5e6636
Reviewed-on: https://boringssl-review.googlesource.com/5861
Reviewed-by: Adam Langley <agl@google.com>
This change fixes up several comments (many of which were spotted by
Kenny Root) and also changes doc.go to detect cases where comments don't
start with the correct word. (This is a common error.)
Since we have docs builders now, these errors will be found
automatically in the future.
Change-Id: I58c6dd4266bf3bd4ec748763c8762b1a67ae5ab3
Reviewed-on: https://boringssl-review.googlesource.com/6440
Reviewed-by: Adam Langley <agl@google.com>
