This uses ssl3_read_bytes for now. We still need to dismantle that
function and then invert the handshake state machine, but this gets
things closer to the right shape as an intermediate step and is a large
chunk in itself. It simplifies a lot of the CCS/handshake
synchronization as a lot of the invariants much more clearly follow from
the handshake itself.
Tests need to be adjusted since this changes some error codes. Now all
the CCS/Handshake checks fall through to the usual
SSL_R_UNEXPECTED_RECORD codepath. Most of what used to be a special-case
falls out naturally. (If half of Finished was in the same record as the
pre-CCS message, that part of the handshake record would have been left
unconsumed, so read_change_cipher_spec would have noticed, just like
read_app_data would have noticed.)
Change-Id: I15c7501afe523d5062f0e24a3b65f053008d87be
Reviewed-on: https://boringssl-review.googlesource.com/6642
Reviewed-by: Adam Langley <agl@google.com>
With server-side renegotiation gone, handshake_fragment's only purpose
in life is to handle a fragmented HelloRequest (we probably do need to
support those if some server does 1/n-1 record-splitting on handshake
records). The logic to route the data into
ssl3_read_bytes(SSL3_RT_HANDSHAKE) never happens, and the contents are
always a HelloRequest prefix.
This also trims a tiny bit of per-connection state.
Change-Id: Ia1b0dda5b7e79d817c28da1478640977891ebc97
Reviewed-on: https://boringssl-review.googlesource.com/6641
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 06cf881a3a10d5af3c1255c08cfd0c6ddb5f1cc3,
9f040d6decca7930e978784c917f731e5c45e8f0, and
9f6795e7d2d1e35668ad70ba0afc480062be4e2e.
Change-Id: I27d90e382867a5fe988d152b31f8494e001a6a9f
Reviewed-on: https://boringssl-review.googlesource.com/6628
Reviewed-by: Adam Langley <agl@google.com>
update.py used to be used only on Windows until very recently, but
Windows and non-Windows have been at the same clang revision for
a while now. So even a few months ago update.py and update.sh
would've contained the same clang revision.
BUG=chromium:494442
Change-Id: Ie9127a1c49e31a7810ee431f8e662350c245917c
Reviewed-on: https://boringssl-review.googlesource.com/6620
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Sometimes BadRSAClientKeyExchange-1 fails with DATA_TOO_LARGE_FOR_MODULUS if
the corruption brings the ciphertext above the RSA modulus. Ensure this does
not happen.
Change-Id: I0d8ea6887dfcab946fdf5d38f5b196f5a927c4a9
Reviewed-on: https://boringssl-review.googlesource.com/6731
Reviewed-by: Adam Langley <agl@google.com>
Avoids bouncing on the lock, but it doesn't really matter since it's all
taking read locks. If we're declaring that callbacks don't get to see
every object being created, they shouldn't see every object being
destroyed.
CRYPTO_dup_ex_data also already had this optimization, though it wasn't
documented.
BUG=391192
Change-Id: I5b8282335112bca3850a7c0168f8bd7f7d4a2d57
Reviewed-on: https://boringssl-review.googlesource.com/6626
Reviewed-by: Adam Langley <agl@google.com>
This callback is never used. The one caller I've ever seen is in Android
code which isn't built with BoringSSL and it was a no-op.
It also doesn't actually make much sense. A callback cannot reasonably
assume that it sees every, say, SSL_CTX created because the index may be
registered after the first SSL_CTX is created. Nor is there any point in
an EX_DATA consumer in one file knowing about an SSL_CTX created in
completely unrelated code.
Replace all the pointers with a typedef to int*. This will ensure code
which passes NULL or 0 continues to compile while breaking code which
passes an actual function.
This simplifies some object creation functions which now needn't worry
about CRYPTO_new_ex_data failing. (Also avoids bouncing on the lock, but
it's taking a read lock, so this doesn't really matter.)
BUG=391192
Change-Id: I02893883c6fa8693682075b7b130aa538a0a1437
Reviewed-on: https://boringssl-review.googlesource.com/6625
Reviewed-by: Adam Langley <agl@google.com>
Then deprecate the old functions. Thanks to upstream's
6977e8ee4a718a76351ba5275a9f0be4e530eab5 for the idea.
Change-Id: I916abd6fca2a3b2a439ec9902d9779707f7e41eb
Reviewed-on: https://boringssl-review.googlesource.com/6622
Reviewed-by: Adam Langley <agl@google.com>
It has no callers. I prepped for its removal earlier with
c05697c2c5
and then completely forgot.
Thanks to upstream's 6f78b9e824c053d062188578635c575017b587c5 for
the reminder. Quoth them:
> This only gets used to set a specific curve without actually checking
> that the peer supports it or not and can therefor result in handshake
> failures that can be avoided by selecting a different cipher.
It's also a very confusing API since it does NOT pass ownership of the
EC_KEY to the caller.
Change-Id: I6a00643b3a2d6746e9e0e228b47c2bc9694b0084
Reviewed-on: https://boringssl-review.googlesource.com/6621
Reviewed-by: Adam Langley <agl@google.com>
Remove the custom copy of those helpers.
Change-Id: I810c3ae8dbf7bc0654d3e9fb9900c425d36f64aa
Reviewed-on: https://boringssl-review.googlesource.com/6611
Reviewed-by: Adam Langley <agl@google.com>
Cover not just the wrong version, but also other mistakes.
Change-Id: I46f05a9a37b7e325adc19084d315a415777d3a46
Reviewed-on: https://boringssl-review.googlesource.com/6610
Reviewed-by: Adam Langley <agl@google.com>
This is a remnant of ssl3_get_client_hello's old DTLS cookie logic, which has
since been removed. (If we ever need HelloVerifyRequest support on the server,
we'll implement something stateless in front.) We can switch this to something
more straightforward now.
See also upstream's 94f98a9019e1c0a3be4ca904b2c27c7af3d937c0,
Change-Id: Ie733030209a381a4915d6744fa12a79ffe972fa5
Reviewed-on: https://boringssl-review.googlesource.com/6601
Reviewed-by: Adam Langley <agl@google.com>
I don't think we're ever going to manage to enforce this, and it doesn't
seem worth the trouble. We don't support application protocols which use
renegotiation outside of the HTTP/1.1 mid-stream client auth hack.
There, it's on the server to reject legacy renegotiations.
This removes the last of SSL_OP_ALL.
Change-Id: I996fdeaabf175b6facb4f687436549c0d3bb0042
Reviewed-on: https://boringssl-review.googlesource.com/6580
Reviewed-by: Adam Langley <agl@google.com>
RFC 5746 forbids a server from downgrading or upgrading
renegotiation_info support. Even with SSL_OP_LEGACY_SERVER_CONNECT set
(the default), we can still enforce a few things.
I do not believe this has practical consequences. The attack variant
where the server half is prefixed does not involve a renegotiation on
the client. The converse where the client sees the renegotiation and
prefix does, but we only support renego for the mid-stream HTTP/1.1
client auth hack, which doesn't do this. (And with triple-handshake,
HTTPS clients should be requiring the certificate be unchanged across
renego which makes this moot.)
Ultimately, an application which makes the mistake of using
renegotiation needs to be aware of what exactly that means and how to
handle connection state changing mid-stream. We make renego opt-in now,
so this is a tenable requirement.
(Also the legacy -> secure direction would have been caught by the
server anyway since we send a non-empty RI extension.)
Change-Id: I915965c342f8a9cf3a4b6b32f0a87a00c3df3559
Reviewed-on: https://boringssl-review.googlesource.com/6559
Reviewed-by: Adam Langley <agl@google.com>
This dates to SSLeay 0.8.0 (or earlier). The use counter sees virtually
no hits.
Change-Id: Iff4c8899d5cb0ba4afca113c66d15f1d980ffe41
Reviewed-on: https://boringssl-review.googlesource.com/6558
Reviewed-by: Adam Langley <agl@google.com>
This dates to SSLeay 0.9.0. The Internet seems to have completely
forgotten what "D5" is. (I can't find reference to it beyond
documentation of this quirk.) The use counter we added sees virtually no
hits.
Change-Id: I9781d401acb98ce3790b1b165fc257a6f5e9b155
Reviewed-on: https://boringssl-review.googlesource.com/6557
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's d88ef40a1e5c81d0d32b4a431e55f5456e678dd2 and
943c4ca62b3f5a160340d57aecb9413407a06e15.)
Change-Id: Idd52aebae6839695be0f3a8a7659adeec6650b98
Reviewed-on: https://boringssl-review.googlesource.com/6556
Reviewed-by: Adam Langley <agl@google.com>
Previously, android_compat_hacks.c and android_compat_keywrap.c
were added to crypto_sources when multiple build platforms were
specified in one invocation.
Change-Id: I4fd8bffc4785bef0148d12cd6f292d79c043b806
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/6566
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
In some cases it would be good to restrict the input range of scalars
given to |EC_METHOD::mul| to be [0, order-1]. This is a first step
towards that goal.
Change-Id: I58a25db06f6c7a68a0ac1fe79794b04f7a173b23
Reviewed-on: https://boringssl-review.googlesource.com/6562
Reviewed-by: Adam Langley <agl@google.com>
|EC_GROUP_get0_order| doesn't require any heap allocations and never
fails, so it is much more convenient and more efficient for callers to
call.
Change-Id: Ic60f768875e7bc8e74362dacdb5cbbc6957b05a6
Reviewed-on: https://boringssl-review.googlesource.com/6532
Reviewed-by: Adam Langley <agl@google.com>
At least for newlib (Native Client) including sys/types.h
is not enough to get a timeval declaration.
Change-Id: I4971a1aacc80b6fdc12c0e81c5d8007ed13eb8b7
Reviewed-on: https://boringssl-review.googlesource.com/6722
Reviewed-by: Adam Langley <agl@google.com>
Native Client doesn't support fcntl natively and its default
implemention just returns ENOSYS.
Change-Id: Id8615e2f6f0a75a1140f8efd75afde471ccdf466
Reviewed-on: https://boringssl-review.googlesource.com/6721
Reviewed-by: Adam Langley <agl@google.com>
BUG=webrtc:5222
Change-Id: I8399bd595564dedbe5492b8ea6eb915f41367cbf
Reviewed-on: https://boringssl-review.googlesource.com/6690
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Windows does support anonymous unions but warns about it. Since I'm not
sure what warnings we have enabled in Chromium, this change just drops
the union for Windows.
Change-Id: I914f8cd5855eb07153105250c0f026eaedb35365
Reviewed-on: https://boringssl-review.googlesource.com/6631
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
wpa_supplicant needs access to the internals of SHA_CTX. We supported
this only for builds with ANDROID defined previously but that's a pain
for wpa_supplicant to deal with. Thus this change enables it
unconditionally.
Perhaps in the future we'll be able to get a function to do this into
OpenSSL and BoringSSL.
Change-Id: Ib5d088c586fe69249c87404adb45aab5a7d5cf80
Reviewed-on: https://boringssl-review.googlesource.com/6630
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
I messed up and missed that we were carrying a diff on x86_64-mont5.pl. This
was accidentally dropped in https://boringssl-review.googlesource.com/6616.
To confirm the merge is good now, check out at this revision and run:
git diff e701f16bd69b6f251ed537e40364c281e85a63b2^ crypto/bn/asm/x86_64-mont5.pl > /tmp/A
Then in OpenSSL's repository:
git diff d73cc256c8e256c32ed959456101b73ba9842f72^ d73cc256c8e256c32ed959456101b73ba9842f72 crypto/bn/asm/x86_64-mont5.pl > /tmp/B
And confirm the diffs vary in only metadata:
diff -u /tmp/A /tmp/B
--- /tmp/A 2015-12-03 11:53:23.127034998 -0500
+++ /tmp/B 2015-12-03 11:53:53.099314287 -0500
@@ -1,8 +1,8 @@
diff --git a/crypto/bn/asm/x86_64-mont5.pl b/crypto/bn/asm/x86_64-mont5.pl
-index 38def07..3c5a8fc 100644
+index 388e3c6..64e668f 100755
--- a/crypto/bn/asm/x86_64-mont5.pl
+++ b/crypto/bn/asm/x86_64-mont5.pl
-@@ -1770,6 +1770,15 @@ sqr8x_reduction:
+@@ -1784,6 +1784,15 @@ sqr8x_reduction:
.align 32
.L8x_tail_done:
add (%rdx),%r8 # can this overflow?
@@ -18,7 +18,7 @@
xor %rax,%rax
neg $carry
-@@ -3116,6 +3125,15 @@ sqrx8x_reduction:
+@@ -3130,6 +3139,15 @@ sqrx8x_reduction:
.align 32
.Lsqrx8x_tail_done:
add 24+8(%rsp),%r8 # can this overflow?
@@ -34,7 +34,7 @@
mov $carry,%rax # xor %rax,%rax
sub 16+8(%rsp),$carry # mov 16(%rsp),%cf
-@@ -3159,13 +3177,11 @@ my ($rptr,$nptr)=("%rdx","%rbp");
+@@ -3173,13 +3191,11 @@ my ($rptr,$nptr)=("%rdx","%rbp");
my @ri=map("%r$_",(10..13));
my @ni=map("%r$_",(14..15));
$code.=<<___;
Change-Id: I3fb5253783ed82e4831f5bffde75273bd9609c23
Reviewed-on: https://boringssl-review.googlesource.com/6618
Reviewed-by: Adam Langley <agl@google.com>
Avoid seg fault by checking mgf1 parameter is not NULL. This can be
triggered during certificate verification so could be a DoS attack
against a client or a server enabling client authentication.
Thanks to Loïc Jonas Etienne (Qnective AG) for discovering this bug.
CVE-2015-3194
(Imported from upstream's c394a488942387246653833359a5c94b5832674e and test
data from 00456fded43eadd4bb94bf675ae4ea5d158a764f.)
Change-Id: Ic97059d42722fd810973ccb0c26c415c4eaae79a
Reviewed-on: https://boringssl-review.googlesource.com/6617
Reviewed-by: Adam Langley <agl@google.com>
When parsing a combined structure pass a flag to the decode routine
so on error a pointer to the parent structure is not zeroed as
this will leak any additional components in the parent.
This can leak memory in any application parsing PKCS#7 or CMS structures.
CVE-2015-3195.
Thanks to Adam Langley (Google/BoringSSL) for discovering this bug using
libFuzzer.
PR#4131
(Imported from upstream's cc598f321fbac9c04da5766243ed55d55948637d, with test
from our original report. Verified ASan trips up on the test without the fix.)
Change-Id: I007d93f172b2f16bf6845d685d72717ed840276c
Reviewed-on: https://boringssl-review.googlesource.com/6615
Reviewed-by: Adam Langley <agl@google.com>
yaSSL has a couple of bugs in their DH client implementation. This
change works around the worst of the two.
Firstly, they expect the the DH public value to be the same length as
the prime. This change pads the public value as needed to ensure this.
Secondly, although they handle the first byte of the shared key being
zero, they don't handle the case of the second, third, etc bytes being
zero. So whenever that happens the handshake fails. I don't think that
there's anything that we can do about that one.
Change-Id: I789c9e5739f19449473305d59fe5c3fb9b4a6167
Reviewed-on: https://boringssl-review.googlesource.com/6578
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Right now your options are:
- Bounce on a reference and deal with cleanup needlessly.
- Manually check the type tag and peek into the union.
We probably have no hope of opaquifying this struct, but for new code, let's
recommend using this function rather than the more error-prone thing.
Change-Id: I9b39ff95fe4264a3f7d1e0d2894db337aa968f6c
Reviewed-on: https://boringssl-review.googlesource.com/6551
Reviewed-by: Adam Langley <agl@google.com>
Some strange toolchains can have an implicit (or explicit) fcntl.h include,
so let's avoid using the name 'open' for local functions. This should not
cause any trouble.
Change-Id: Ie131b5920ac23938013c2c03302b97a7418c7180
Reviewed-on: https://boringssl-review.googlesource.com/6540
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
BN_LLONG is only ever used in #ifdefs. The actual type is BN_ULLONG. Switch the
ifdefs to check on BN_ULLONG and remove BN_LLONG. Also fix signedness of all
the constants (potentially avoiding undefined behavior in some operations).
Change-Id: I3e7739bbe14c50ea7db04fc507a034a8cb315a5f
Reviewed-on: https://boringssl-review.googlesource.com/6518
Reviewed-by: Adam Langley <agl@google.com>
I screwed up the |EVP_CIPHER| parameters for XTS when I first imported
it, and there were no tests to catch it. (The problem was that
|EVP_CIPH_XTS_MODE| means “the key size is actually twice what it says
here.”)
With these changes, OpenSSL's tests pass.
(Along the way, make a few other things about XTS slightly less
decrepit.)
Change-Id: Icbfbc5e6d532d1c132392ee366f9cab42802d674
Reviewed-on: https://boringssl-review.googlesource.com/6529
Reviewed-by: Adam Langley <agl@google.com>
Initialization by multiple consumers on ARM is still problematic due to
CRYPTO_set_NEON_{capable,functional}, until we reimplement that in-library, but
if that is called before the first CRYPTO_library_init, this change makes it
safe.
BUG=556462
Change-Id: I5845d09cca909bace8293ba7adf09a3bd0d4f943
Reviewed-on: https://boringssl-review.googlesource.com/6519
Reviewed-by: Adam Langley <agl@google.com>
The |ri| field was only used in |BN_MONT_CTX_set|, so make it a local
variable of that function.
Change-Id: Id8c3d44ac2e30e3961311a7b1a6731fe2c33a0eb
Reviewed-on: https://boringssl-review.googlesource.com/6526
Reviewed-by: Adam Langley <agl@google.com>
The comment in |BN_mod_inverse_ex| makes it clear that |BN_BITS2| was
intended. Besides fixing the code to match the comment, remove
the now-unused |BN_BITS| and the already-unused |BN_MASK| to prevent
future confusion of this sort.
On MSVC builds there seems to be very little difference in performance
between the two code paths according to |bssl speed|.
Change-Id: I765b7b3d464e2057b1d7952af25b6deb2724976a
Reviewed-on: https://boringssl-review.googlesource.com/6525
Reviewed-by: Adam Langley <agl@google.com>
Previously, both crypto/dh and crypto/ec defined |TOBN| macros that did
the same thing, but which took their arguments in the opposite order.
This change makes the code consistently use the same macro. It also
makes |STATIC_BIGNUM| available for internal use outside of crypto/bn.
Change-Id: Ide57f6a5b74ea95b3585724c7e1a630c82a864d9
Reviewed-on: https://boringssl-review.googlesource.com/6528
Reviewed-by: Adam Langley <agl@google.com>
clang-format packing them tightly made newlines inconsistent which
wasn't very helpful.
Change-Id: I46a787862ed1f5b0eee101394e24c779b6bc652b
Reviewed-on: https://boringssl-review.googlesource.com/6517
Reviewed-by: Adam Langley <agl@google.com>
