This gives coverage over needing to fragment something over multiple
records.
Change-Id: I2373613608ef669358d48f4e12f68577fa5a40dc
Reviewed-on: https://boringssl-review.googlesource.com/13101
Reviewed-by: Adam Langley <agl@google.com>
Mercifully, PKCS#12 does not actually make ContentInfo and SafeBag
mutually recursive. The top-level object in a PKCS#12 is a SEQUENCE of
data or encrypted data ContentInfos. Their payloads are a SEQUENCE of
SafeBags (aka SafeContents).
SafeBag is a similar structure to ContentInfo but not identical (it has
attributes in it which we ignore) and actually carries the objects.
There is only recursion if the SafeContents bag type is used, which we
do not process.
This means we don't need to manage recursion depth. This also no longer
allows trailing data after the SEQUENCE and removes the comment about
NSS. The test file still passes, so I'm guessing something else was
going on?
Change-Id: I68e2f8a5cc4b339597429d15dc3588bd39267e0a
Reviewed-on: https://boringssl-review.googlesource.com/13071
Reviewed-by: Adam Langley <agl@google.com>
Resolving the TODO here will be messier than the other implementations
but, to start with, remove this 'pivot element' thing. All that is just
to free some array contents without having to memset the whole thing to
zero.
Change-Id: Ifd6ee0b3815006d4f1f19c9db085cb842671c6dc
Reviewed-on: https://boringssl-review.googlesource.com/13057
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
BN_FLG_CONSTTIME is a ridiculous API and easy to mess up
(CVE-2016-2178). Instead, code that needs a particular algorithm which
preserves secrecy of some arguemnt should call into that algorithm
directly.
This is never set outside the library and is finally unused within the
library! Credit for all this goes almost entirely to Brian Smith. I just
took care of the last bits.
Note there was one BN_FLG_CONSTTIME check that was still reachable, the
BN_mod_inverse in RSA key generation. However, it used the same code in
both cases for even moduli and φ(n) is even if n is not a power of two.
Traditionally, RSA keys are not powers of two, even though it would make
the modular reductions a lot easier.
When reviewing, check that I didn't remove a BN_FLG_CONSTTIME that led
to a BN_mod_exp(_mont) or BN_mod_inverse call (with the exception of the
RSA one mentioned above). They should all go to functions for the
algorithms themselves like BN_mod_exp_mont_consttime.
This CL shows the checks are a no-op for all our tests:
https://boringssl-review.googlesource.com/c/12927/
BUG=125
Change-Id: I19cbb375cc75aac202bd76b51ca098841d84f337
Reviewed-on: https://boringssl-review.googlesource.com/12926
Reviewed-by: Adam Langley <alangley@gmail.com>
TLS 1.3 doesn't support renegotiation in the first place, but so callers
don't report TLS 1.3 servers as missing it, always report it as
(vacuously) protected against this bug.
BUG=chromium:680281
Change-Id: Ibfec03102b2aec7eaa773c331d6844292e7bb685
Reviewed-on: https://boringssl-review.googlesource.com/13046
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Change-Id: Ie9a0039931a1a8d48a82c11ef5c58d6ee084ca4c
Reviewed-on: https://boringssl-review.googlesource.com/13070
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
Avoid the X509_ALGOR dependency entirely. The public API is still using
the legacy ASN.1 structures for now, but the conversions are lifted to
the API boundary. Once we resolve that and the OID table dependency,
this module will no longer block unshipping crypto/asn1 and friends from
Chromium.
This changes the calling convention around the two kinds of PBE suites
we support. Each PBE suite provides a free-form encrypt_init function to
setup an EVP_CIPHER_CTX and write the AlgorithmIdentifer to a CBB. It
then provides a common decrypt_init function which sets up an
EVP_CIPHER_CTX given a CBS of the parameter. The common encrypt code
determines how to call which encrypt_init function. The common decrypt
code parses the OID out of the AlgorithmIdentifer and then dispatches to
decrypt_init.
Note this means the encryption codepath no longer involves parsing back
out a AlgorithmIdentifier it just serialized. We don't have a good story
to access an already serialized piece of a CBB in progress (reallocs can
invalidate the pointer in a CBS), so it's easier to cut this step out
entirely.
Also note this renames the "PBES1" schemes from PKCS#5 to PKCS#12. This
makes it easier to get at the PKCS#12 key derivation hooks. Although
PKCS#12 claims these are variants of PKCS#5's PBES1, they're not very
related. PKCS#12 swaps out the key derivation and even defines its own
AlgorithmIdentifier parameter structure (identical to the PKCS#5 PBES1
one). The only thing of PBES1 that survives is the CBC mode padding
scheme, which is deep in EVP_CIPHER for us. (Of course, all this musing
on layering is moot because we don't implement non-PKCS#12 PBES1 schemes
anyway.)
This also moves some of the random API features (default iteration
count, default salt generation) out of the PBE suites and into the
common code.
BUG=54
Change-Id: Ie96924c73a229be2915be98eab680cadd17326db
Reviewed-on: https://boringssl-review.googlesource.com/13069
Reviewed-by: Adam Langley <alangley@gmail.com>
This gets us closer to decoupling from crypto/asn1.
BUG=54
Change-Id: I06ec04ed3cb47c2f56a94c6defa97398bfd0e013
Reviewed-on: https://boringssl-review.googlesource.com/13066
Reviewed-by: Adam Langley <alangley@gmail.com>
This is not quite an end state (it still outputs an X509_ALGOR, the way
the generated salt is fed into key derivation is odd, and it uses the
giant OID table), but replaces a large chunk of it.
BUG=54
Change-Id: I0a0cca13e44e6a09dfaf6aed3b357cb077dc46d1
Reviewed-on: https://boringssl-review.googlesource.com/13065
Reviewed-by: Adam Langley <alangley@gmail.com>
Commit-Queue: Adam Langley <alangley@gmail.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Many of these parameters are constants.
Change-Id: I148dbea0063e478a132253f4e9dc71d5d20320c2
Reviewed-on: https://boringssl-review.googlesource.com/13064
Reviewed-by: Adam Langley <alangley@gmail.com>
Commit-Queue: Adam Langley <alangley@gmail.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is a very basic test, but it's something.
Change-Id: Ic044297e97ce5719673869113ce581de4621ebbd
Reviewed-on: https://boringssl-review.googlesource.com/13061
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
libcrypto can now be split in two, with everything that depends on
crypto/asn1 in a separate library. That said, Chromium still needs
crypto/pkcs8 to be implemented with CBS/CBB first. (Also libssl and
anything which uses X509* directly.)
BUG=54
Change-Id: Iec976ae637209882408457e94a1eb2465bce8d56
Reviewed-on: https://boringssl-review.googlesource.com/13059
Reviewed-by: Adam Langley <alangley@gmail.com>
Commit-Queue: Adam Langley <alangley@gmail.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Change-Id: Iad9b0898b3a602fc2e554c4fd59a599c61cd8ef7
Reviewed-on: https://boringssl-review.googlesource.com/13063
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
They're not called externally. Unexporting these will make it easier to
rewrite the PKCS{5,8,12} code to use CBS/CBB rather than X509_ALGOR.
Getting rid of those callers in Chromium probably won't happen for a
while since it's in our on-disk formats. (And a unit test for some NSS
client cert glue uses it.)
BUG=54
Change-Id: Id4148a2ad567484782a6e0322b68dde0619159fc
Reviewed-on: https://boringssl-review.googlesource.com/13062
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
This includes examples with both the NULL and empty passwords, thanks to
PKCS#12's password ambiguity.
Change-Id: Iae31840c1d31929fa9ac231509acaa80ef5b74bb
Reviewed-on: https://boringssl-review.googlesource.com/13060
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
The motiviation is that M2Crypto passes an ASN1_GENERALIZEDTIME to
this function. This is not distinct from ASN1_UTCTIME (both are
asn1_string_st), but ASN1_GENERALIZEDTIME uses a 4-digit year in its
string representation, whereas ASN1_UTCTIME uses a 2-digit year.
ASN1_UTCTIME_print previously did not return an error on such inputs.
So, stricten (?) the function, ensuring that it checks for trailing
data, and rejects values that are invalid for their place. Along the
way, clean it up and add tests.
Change-Id: Ia8298bed573f2acfdab96638ea69c78b5bba4e4b
Reviewed-on: https://boringssl-review.googlesource.com/13082
Reviewed-by: Adam Langley <alangley@gmail.com>
Commit-Queue: Adam Langley <alangley@gmail.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We missed this one. Microsoft doesn't usually pass their own level 4
warnings. Kenny Root reports this otherwise trips warnings on some
version of the Windows SDK.
Change-Id: I71dc837ff8ee52321483d26c3a2cd8676393d999
Reviewed-on: https://boringssl-review.googlesource.com/13110
Reviewed-by: Kenny Root <kroot@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
08b65f4e31 introduced a memory leak and
also got enums confused. Also fix a codepath that was missing an error
code.
Thanks to OSS-Fuzz which appears to have found it in a matter of hours.
Change-Id: Ia9e926c28a01daab3e6154d363d0acda91209a22
Reviewed-on: https://boringssl-review.googlesource.com/13104
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Neither Windows nor POSIX uses a char for SO_REUSEADDR. Windows uses
BOOL (which is actually int) and POSIX uses int. Windows also requires a
cast due to using char* instead of void*. Thanks to Daniel Hirche for
reporting.
Change-Id: I01c847c8da285f27f3c3cdf5ff58b53899098b82
Reviewed-on: https://boringssl-review.googlesource.com/13100
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Towards an eventual goal of opaquifying BoringSSL structs, we want
our consumers -- in this case, Android's libcore -- to not directly
manipulate BigNums; and it would be convenient for them if we would
perform the appropriate gymnastics to interpret little-endian byte
streams.
It also seems a priori a bit strange to have only big-endian varieties
of BN byte-conversions.
This CL provides little-endian equivalents of BN_bn2bin_padded
and BN_bin2bn.
BUG=97
Change-Id: I0e92483286def86d9bd71a46d6a967a3be50f80b
Reviewed-on: https://boringssl-review.googlesource.com/12641
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This adds support for setting 0-RTT mode on tickets minted by
BoringSSL, allowing for testing of the initial handshake knowledge.
BUG=76
Change-Id: Ic199842c03b5401ef122a537fdb7ed9e9a5c635a
Reviewed-on: https://boringssl-review.googlesource.com/12740
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Passing in an array of scalars was removed some time ago, but a few
remnants of it remain.
Change-Id: Id75abedf60b1eab59f24bf7232187675b63291ab
Reviewed-on: https://boringssl-review.googlesource.com/13056
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is a remnant of signature EVP_MDs. Detach them from
EVP_get_digestby{nid,obj}. Nothing appears to rely on this for those two
functions. Alas, Node.js appears to rely on it for EVP_get_digestbyname,
so keep that working.
This avoids causing every consumer's parsing to be unintentionally lax.
It also means fewer OIDs to transcribe when detaching the last of
libcrypto from the legacy ASN.1 stack and its giant OID table.
Note this is an externally visible change. There was one consumer I had
to fix, but otherwise everything handled things incorrectly due to this
quirk, so it seemed better to just fix the API rather than fork off a
second set.
Change-Id: I705e073bc05d946e71cd1c38acfa5e3c6b0a22b4
Reviewed-on: https://boringssl-review.googlesource.com/13058
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Passing in an array of scalars was removed some time ago, but a few
remnants of it remain.
Change-Id: Ia51dcf1f85116ec663e657cc8dbef7f23ffa2edb
Reviewed-on: https://boringssl-review.googlesource.com/13055
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This allows a server to be restarted immediately with the same port
without having to wait for socket timeout on crash/failure.
Change-Id: Ifcf58d46067f157dd504946f71b0b99d7fbad10c
Reviewed-on: https://boringssl-review.googlesource.com/13044
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Instead, use BN_mod_exp_mont_consttime of p - 2. This removes two more
call sites sensitive to BN_FLG_CONSTTIME. We're down to just that last
BN_mod_inverse modulo φ(n). (Sort of. It's actually not sensitive
because even mod inverses always hit the other codepath. Perhaps we
should just leave it alone.)
Note this comes with a slight behavior change. The BN_MONT_CTXs are
initialized a little earlier. If a caller calls RSA_generate_* and then
reaches into the struct to scrap all the fields on it, they'll get
confused. Before, they had to perform an operation on it to get
confused. This is a completely ridiculous thing to do.
Since we do this a lot, this introduces some convenience functions for
doing the Fermat's Little Theorem mod inverse and fixes a leak in the
DSA code should computing kinv hit a malloc error.
BUG=125
Change-Id: Iafcae2fc6fd379d161f015c90ff7050e2282e905
Reviewed-on: https://boringssl-review.googlesource.com/12925
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
As long as we still have this code, we should make sure it doesn't
regress.
Change-Id: I0290792aedcf667ec49b251d747ffbc141c0cec4
Reviewed-on: https://boringssl-review.googlesource.com/13053
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
SSL_write is remarkably complicated.
Change-Id: I1cb8d00af1b4c5e2d90187d5f87951f25e27f224
Reviewed-on: https://boringssl-review.googlesource.com/13050
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Instead, add ssl_has_certificate to the ssl3_send_cert_verify check. If
writing the empty Certificate does not complete synchronously (it almost
always does due to the buffer BIO), but if the buffer boundary is at
exactly the wrong place, write_message will need a retry but, having
cleared cert_request, we never re-enter ssl3_send_client_certificate.
This will later be moot when we've gotten rid of the buffer BIO, but
this is cleaner anyway and is closer to the TLS 1.3 code.
With this change, blindly taking away the BIO buffer in TLS (which is
not what we want since we want the entire flight in one write but is a
nice sanity check), only the SSL 3.0 no client certificate tests fail.
They too rely on some writes completing synchronously due to SSL 3.0
sending a warning alert. There is a similar bug when
tlsext_servername_callback returns SSL_TLSEXT_ERR_ALERT_WARNING.
Those will be resolved after reworking the write path since it's a bit
of a mess.
Change-Id: I56b4df6163cae1df263cf36f0d93046d0375a5ac
Reviewed-on: https://boringssl-review.googlesource.com/13052
Reviewed-by: David Benjamin <davidben@google.com>
We repeat this in a bunch of places.
Change-Id: Iee2c95a13e1645453f101d8be4be9ac78d520387
Reviewed-on: https://boringssl-review.googlesource.com/13051
Reviewed-by: David Benjamin <davidben@google.com>
There's an authenticator, so test that AES_unwrap_key notices invalid
inputs.
Change-Id: Icbb941f91ffd9c91118f956fd74058d241f91ecb
Reviewed-on: https://boringssl-review.googlesource.com/13047
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The write path for TLS is going to need some work. There are some fiddly
cases when there is a write in progress. Start adding tests to cover
this logic.
Later I'm hoping we can extend this flag so it drains the unfinished
write and thus test the interaction of read/write paths in 0-RTT. (We
may discover 1-RTT keys while we're in the middle of writing data.)
Change-Id: Iac2c417e4b5e84794fb699dd7cbba26a883b64ef
Reviewed-on: https://boringssl-review.googlesource.com/13049
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 13ab87083af862e4af752efa4b0552149ed2cc19.)
Change-Id: I2f7cf8454d28d47f5ca19544479b2ab98143a3ef
Reviewed-on: https://boringssl-review.googlesource.com/13048
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Iba299b1bcfe7de53bb0fa6b6cc4e607129905fd0
Reviewed-on: https://boringssl-review.googlesource.com/13054
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Channel ID is already enabled on the SSL. This dates to
49c7af1c42 which converted an instance of
tlsext_channel_id_enabled_new to it, but tlsext_channel_id_enabled_new
meant "if Channel ID is enabled, use the new one", not "enable Channel
ID".
Thanks to Eric Rescorla for catching this.
Change-Id: I2d5a82b930ffcbe5527a62a9aa5605ebb71a6b9f
Reviewed-on: https://boringssl-review.googlesource.com/13042
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This was noticed by observing we had one line of missing test coverage
in polyval.c. CRYPTO_POLYVAL_update_blocks acts 32 blocks at a time and
all existing test vectors are smaller than that.
Test vector obtained by just picking random values and seeing what our
existing implementation did if I modified CRYPTO_POLYVAL_update_blocks
to consume many more blocks at a time. Then I fixed the bug and ensured
the answer was still the same.
Change-Id: Ib7002dbc10952229ff42a17132c30d0e290d4be5
Reviewed-on: https://boringssl-review.googlesource.com/13041
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is a memory error for anything other than LHASH_OF(char), which
does not exist.
No code outside the library creates (or even queries) an LHASH, so we
can change this module freely.
Change-Id: Ifbc7a1c69a859e07650fcfaa067bdfc68d83fbbc
Reviewed-on: https://boringssl-review.googlesource.com/12978
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Use it to compare the contents of lh and dummy_lh are identical. Leave a
TODO for testing other LHASH cases.
Change-Id: Ifbaf17c196070fdff1530ba0e284030527855f5d
Reviewed-on: https://boringssl-review.googlesource.com/12977
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I0e8a24367cd33fa4aed2ca15bd369b8697f538e6
Reviewed-on: https://boringssl-review.googlesource.com/12974
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Use a std::map as the dummy lhash and use unique_ptr. This also improves
the test to check on pointer equality; we wish to ensure the lhash
stores the particular pointer value we asked for.
dummy_lh now also owns the pointers. It makes things simpler and since
LHASH doesn't free things, we weren't getting anything out of testing
that.
Change-Id: I97159175ca79a5874586650f272a7846100395e1
Reviewed-on: https://boringssl-review.googlesource.com/12976
Reviewed-by: Adam Langley <agl@google.com>
No source changes, just a rename.
Change-Id: Iaef406d2a04dc8c68c94eb2a98eec6378eaeab66
Reviewed-on: https://boringssl-review.googlesource.com/12975
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Rather than doing it right before outputing, treat this as a part of the
pipeline to finalize the certificate chain, and run it right after
cert_cb to modify the certificate configuration itself. This means
nothing else in the stack needs to worry about this case existing.
It also makes it easy to support in both TLS 1.2 and TLS 1.3.
Change-Id: I6a088297a54449f1f5f5bb8b5385caa4e8665eb6
Reviewed-on: https://boringssl-review.googlesource.com/12966
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ibba0271efa86e1b1af97f2a08b73677dfd236b7a
Reviewed-on: https://boringssl-review.googlesource.com/12986
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
BUG=97
Change-Id: I4799cc99511e73af44def1d4daa36a8b4699f62d
Reviewed-on: https://boringssl-review.googlesource.com/12904
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Get us a little closer to productionizing the coverage generation, which
will require taking all the logic out of the coverage script.
Change-Id: If410cc198a888ee87a84b1c2d532322682d3c44e
Reviewed-on: https://boringssl-review.googlesource.com/13043
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
