This imports upstream's ea6b07b54c1f8fc2275a121cdda071e2df7bd6c1 along
with a bugfix in 987157f6f63fa70dbeffca3c8bc62f26e9767ff2.
In an SPKI, a DSA key is only an INTEGER, with the group information in
the AlgorithmIdentifier. But a standalone DSAPublicKey is more complex
(and apparently made up by OpenSSL). OpenSSL implemented this with a
write_params boolean and making DSAPublicKey a CHOICE.
Instead, have p_dsa_asn1.c encode an INTEGER directly. d2i_DSAPublicKey
only parses the standalone form. (That code will be replaced later, but
first do this in preparation for rewriting the DSA ASN.1 code.)
Change-Id: I6fbe298d2723b9816806e9c196c724359b9ffd63
Reviewed-on: https://boringssl-review.googlesource.com/7021
Reviewed-by: Adam Langley <agl@google.com>
Functions which lose object reuse and need auditing:
- d2i_ECParameters
- d2i_ECPrivateKey
This adds a handful of bytestring-based APIs to handle EC key
serialization. Deprecate all the old serialization APIs. Notes:
- An EC_KEY has additional state that controls its encoding, enc_flags
and conv_form. conv_form is left alone, but enc_flags in the new API
is an explicit parameter.
- d2i_ECPrivateKey interpreted its T** argument unlike nearly every
other d2i function. This is an explicit EC_GROUP parameter in the new
function.
- The new specified curve code is much stricter and should parse enough
to uniquely identify the curve.
- I've not bothered with a new version of i2d_ECParameters. It just
writes an OID. This may change later when decoupling from the giant
OID table.
- Likewise, I've not bothered with new APIs for the public key since the
EC_POINT APIs should suffice.
- Previously, d2i_ECPrivateKey would not call EC_KEY_check_key and it
was possible for the imported public and private key to mismatch. It
now calls it.
BUG=499653
Change-Id: I30b4dd2841ae76c56ab0e1808360b2628dee0615
Reviewed-on: https://boringssl-review.googlesource.com/6859
Reviewed-by: Adam Langley <agl@google.com>
In c0d9484902, we had to add support for
recognizing specified versions of named curves. I believe the motivation
was an ECPrivateKey encoded by OpenSSL without the EC_KEY's asn1_flag
set to OPENSSL_EC_NAMED_CURVE. Annoyingly, it appears OpenSSL's API
defaulted to the specified form while the tool defaulted to the named
form.
Add tests for this at the ECPrivateKey and the PKCS#8 level. The latter
was taken from Chromium's ec_private_key_unittest.cc which was the
original impetus for this.
Change-Id: I53a80c842c3fc9598f2e0ee7bf2d86b2add9e6c4
Reviewed-on: https://boringssl-review.googlesource.com/7072
Reviewed-by: Adam Langley <agl@google.com>
The function |ge_frombytes_negate_vartime|, as the name suggests,
negates its output. This change converts it to |ge_frombytes_vartime|
and, instead, does the negation explicitly when verifying signatures.
The latter function is more generally useful.
Change-Id: I465f8bdf5edb101a80ab1835909ae0ff41d3e295
Reviewed-on: https://boringssl-review.googlesource.com/7142
Reviewed-by: Arnar Birgisson <arnarb@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
An i2d compatibility function is rather long, so add CBB_finish_i2d for
part of it. It takes a CBB as input so only a 'marshal' function is
needed, rather than a 'to_bytes' one.
Also replace the *inp d2i update pattern with a slightly shorter one.
Change-Id: Ibb41059c9532f6a8ce33460890cc1afe26adc97c
Reviewed-on: https://boringssl-review.googlesource.com/6868
Reviewed-by: Adam Langley <agl@google.com>
CBS_asn1_ber_to_der currently uses heuristics because implicitly-tagged
constructed strings in BER are ambiguous with implicitly-tagged sequences. It's
not possible to convert BER to DER without knowing the schema.
Fortunately, implicitly tagged strings don't appear often so instead split the
job up: CBS_asn1_ber_to_der fixes indefinite-length elements and constructed
strings it can see. Implicitly-tagged strings it leaves uncoverted, but they
will only nest one level down (because BER kindly allows one to nest
constructed strings arbitrarily!).
CBS_get_asn1_implicit_string then performs the final concatenation at parse
time. This isn't much more complex and lets us parse BER more accurately and
also reject a number of mis-encoded values (e.g. constructed INTEGERs are not a
thing) we'd previously let through. The downside is the post-conversion parsing
code must be aware of this limitation of CBS_asn1_ber_to_der. Fortunately,
there's only one implicitly-tagged string in our PKCS#12 code.
(In the category of things that really really don't matter, but I had spare
cycles and the old BER converter is weird.)
Change-Id: Iebdd13b08559fa158b308ef83a5bb07bfdf80ae8
Reviewed-on: https://boringssl-review.googlesource.com/7052
Reviewed-by: Adam Langley <agl@google.com>
This is significantly less of a nuisance than having to explicitly type out
kRule5, kExpected5.
Change-Id: I61820c26a159c71e09000fbe0bf91e30da42205e
Reviewed-on: https://boringssl-review.googlesource.com/7000
Reviewed-by: Adam Langley <agl@google.com>
C has implicit conversion of |void *| to other pointer types so these
casts are unnecessary. Clean them up to make the code easier to read
and to make it easier to find dangerous casts.
Change-Id: I26988a672e8ed4d69c75cfbb284413999b475464
Reviewed-on: https://boringssl-review.googlesource.com/7102
Reviewed-by: David Benjamin <davidben@google.com>
I guess the comment "just a reference" was intended to mean that the
|mod| member is a weak reference to a |BIGNUM| owned by something else.
However, it is actually owned by the |bn_blinding_st|, as one can see
by reading |BN_BLINDING_new| and |BN_BLINDING_free|.
Change-Id: If2a681fc9d9db536170e0efb11fdab93e4f0baba
Reviewed-on: https://boringssl-review.googlesource.com/7112
Reviewed-by: David Benjamin <davidben@google.com>
We'd manually marked some of them hidden, but missed some. Do it in the perlasm
driver instead since we will never expose an asm symbol directly. This reduces
some of our divergence from upstream on these files (and indeed we'd
accidentally lose some .hiddens at one point).
BUG=586141
Change-Id: Ie1bfc6f38ba73d33f5c56a8a40c2bf1668562e7e
Reviewed-on: https://boringssl-review.googlesource.com/7140
Reviewed-by: Adam Langley <agl@google.com>
Updating the Perl docs to describe behavior of Strawberry Perl and possible
interaction with CMake on Windows.
Also adding a few other links and instructions for using CMake/Ninja to build
release mode with position independent code, since this seems generally useful.
Change-Id: I616c0d267da749fe90673bc9e8bde9ec181fec25
Reviewed-on: https://boringssl-review.googlesource.com/7113
Reviewed-by: David Benjamin <davidben@google.com>
The |_ex| versions of these functions are unnecessary because when they
are used, they are always passed |NULL| for |r|, which is what the
non-|_ex| versions do. Just use the non-|_ex| versions instead and
remove the |_ex| versions.
Also, drop the unused flags mechanism.
Change-Id: Ida4cb5a2d4c89d9cd318e06f71867aea98408d0d
Reviewed-on: https://boringssl-review.googlesource.com/7110
Reviewed-by: David Benjamin <davidben@google.com>
It is always the case that either |BN_ULLONG| is defined or
|BN_UMULT_LOHI| is defined because |BN_ULLONG| is defined everywhere
except 64-bit MSVC, and BN_UMULT_LOHI is defined for 64-bit MSVC.
Change-Id: I85e5d621458562501af1af65d587c0b8d937ba3b
Reviewed-on: https://boringssl-review.googlesource.com/7044
Reviewed-by: David Benjamin <davidben@google.com>
|a_is_minus_3| is calculated in |ec_GFp_simple_group_set_curve|, so
the custom |group_init| functions are unnecessary. Just as in
commit 9f1f04f313, it is never the case
that custom parameters are passed to the |group_set_curve| method for
these curves.
Change-Id: I18a38b104bc332e44cc2053c465cf234f4c5163b
Reviewed-on: https://boringssl-review.googlesource.com/7090
Reviewed-by: David Benjamin <davidben@google.com>
mul.c is the only file that uses these values.
Change-Id: I50a685cbff0f26357229e742f42e014434e9cebe
Reviewed-on: https://boringssl-review.googlesource.com/7061
Reviewed-by: David Benjamin <davidben@google.com>
Notably, putting Strawberry Perl in %PATH% will usually end up putting a copy
of gcc in %PATH%, which trips up people trying to build on Windows.
This is arguably misusing the variable (normally set by the generator), but it
should work.
Change-Id: I13a011eb33688ae928a56cce266edd2759a3cb32
Reviewed-on: https://boringssl-review.googlesource.com/7070
Reviewed-by: Adam Langley <agl@google.com>
It is always the case that |BN_ULLONG| is defined or we're building for
64-bit MSVC. Lots of code is trying to handle impossible cases where
neither of those is true.
Change-Id: Ie337adda1dfb453843c6e0999807dfa1afb1ed89
Reviewed-on: https://boringssl-review.googlesource.com/7043
Reviewed-by: David Benjamin <davidben@google.com>
Windows build failures seem to have been a CMake statefulness problem. Recipes
were changed to do clean builds each run.
Change-Id: Id5aefa53aead7e82e095d7dccbf88ad89a678c62
This allows much code to be subsequently simplified and removed.
Change-Id: I0ac256957c6eae9f35a70508bd454cb44f3f8653
Reviewed-on: https://boringssl-review.googlesource.com/7042
Reviewed-by: David Benjamin <davidben@google.com>
On failure, CBB_finish doesn't call CBB_cleanup. Also chain more of the ||s
together now that CBB_cleanup after failed CBB_init is legal.
(I don't think this is actually reachable because the CBB is guaranteed to be
flushed by this point.)
Change-Id: Ib16a0a185f15e13675ac2550c5e8e0926ceb7957
Reviewed-on: https://boringssl-review.googlesource.com/7051
Reviewed-by: Adam Langley <agl@google.com>
There was a test for 512 bit orders but not one for 521-bit orders.
Test 521-bit orders instead.
Change-Id: I61a76d02637ca55d8ae21834085311dd84fd870f
Reviewed-on: https://boringssl-review.googlesource.com/7011
Reviewed-by: David Benjamin <davidben@google.com>
node.js uses a memory BIO in the wrong mode which, for now, we work
around. It also passes in NULL (rather than empty) strings and a
non-NULL out-arg for |d2i_PKCS12_bio|.
Change-Id: Ib565b4a202775bb32fdcb76db8a4e8c54268c052
Reviewed-on: https://boringssl-review.googlesource.com/7012
Reviewed-by: Adam Langley <agl@google.com>
Otherwise it still thinks this is an RFC 5114 prime and kicks in the (now
incorrect) validity check.
Change-Id: Ie78514211927f1f2d2549958621cb7896f68b5ce
Reviewed-on: https://boringssl-review.googlesource.com/7050
Reviewed-by: Adam Langley <agl@google.com>
This slightly simplifies the SSL_ECDH code and will be useful later on
in reimplementing the key parsing logic.
Change-Id: Ie41ea5fd3a9a734b3879b715fbf57bd991e23799
Reviewed-on: https://boringssl-review.googlesource.com/6858
Reviewed-by: Adam Langley <agl@google.com>
Change acb24518 renamed some functions, but there were some dangling
references in bn_test.c. Thanks to Brian Smith for noticing.
This change has no semantic effect.
Change-Id: Id149505090566583834be3abce2cee28b8c248e2
Reviewed-on: https://boringssl-review.googlesource.com/7040
Reviewed-by: David Benjamin <davidben@google.com>
This is CVE-2016-0701 for OpenSSL, reported by Antonio Sanso. It is a no-op for
us as we'd long removed SSL_OP_DH_SINGLE_USE and static DH cipher suites. (We
also do not parse or generate X9.42 DH parameters.)
However, we do still have the APIs which return RFC 5114 groups, so we should
perform the necessary checks in case later consumers reuse keys.
Unlike groups we generate, RFC 5114 groups do not use "safe primes" and have
many small subgroups. In those cases, the subprime q is available. Before using
a public key, ensure its order is q by checking y^q = 1 (mod p). (q is assumed
to be prime and the existing range checks ensure y is not 1.)
(Imported from upstream's 878e2c5b13010329c203f309ed0c8f2113f85648 and
75374adf8a6ff69d6718952121875a491ed2cd29, but with some bugs fixed. See
RT4278.)
Change-Id: Ib18c3e84819002fa36a127ac12ca00ee33ea018a
Reviewed-on: https://boringssl-review.googlesource.com/7001
Reviewed-by: Adam Langley <agl@google.com>
Add guards for the architecture and OPENSSL_NO_ASM to
the assembly-language files in crypto/curve25519/asm.
The Dart compilation of BoringSSL includes all files,
because the architecture is not known when gyp is run.
Change-Id: I66f5ae525266b63b0fe3a929012b771d545779b5
Reviewed-on: https://boringssl-review.googlesource.com/7030
Reviewed-by: Adam Langley <agl@google.com>
Take the mappings for MD5 and SHA-224 values out of the code altogether. This
aligns with the current TLS 1.3 draft.
For MD5, this is a no-op. It is not currently possible to configure accepted
signature algorithms, MD5 wasn't in the hardcoded list, and we already had a
test ensuring we enforced our preferences correctly. MD5 also wasn't in the
default list of hashes our keys could sign and no one overrides it with a
different hash.
For SHA-224, this is not quite a no-op. The hardcoded accepted signature
algorithms list included SHA-224, so this will break servers relying on that.
However, Chrome's metrics have zero data points of servers picking SHA-224 and
no other major browser includes it. Thus that should be safe.
SHA-224 was also in the default list of hashes we are willing to sign. For
client certificates, Chromium's abstractions already did not allow signing
SHA-224, so this is a no-op there. For servers, this will break any clients
which only accept SHA-224. But no major browsers do this and I am not aware of
any client implementation which does such ridiculous thing.
(SHA-1's still in there. Getting rid of that one is going to take more effort.)
Change-Id: I6a765fdeea9e19348e409d58a0eac770b318e599
Reviewed-on: https://boringssl-review.googlesource.com/7020
Reviewed-by: Adam Langley <agl@google.com>
The |inline| must appear before the type.
Change-Id: Iecebbcc50024a846d7804228a858acfc33d68efd
Reviewed-on: https://boringssl-review.googlesource.com/7010
Reviewed-by: David Benjamin <davidben@google.com>
Finally, we can stick ScopedFOO in containers.
Change-Id: I3ed166575822af9f182e8be8f4db723e1f08ea31
Reviewed-on: https://boringssl-review.googlesource.com/6553
Reviewed-by: Adam Langley <alangley@gmail.com>
Although RFC 3279 allows both, per RFC 5912, keys must use a named curve
rather than spelling out the curve parameters. Although we do not allow
arbitrary curves, we do have to (pretty hackishly) recognize built-in
curves in ECPrivateKeys.
It seems the cause of this was that OpenSSL, unless you set asn1_flag on
the EC_GROUP, likes to encode keys by spelling out the parameters. This
is in violation of RFC 5915, though probably not in violation of one of
the other redundant ECC specifications. For more fun, it appears
asn1_flag defaults to *off* in the API and *on* in the command-line
tools.
I think the original cause was these defaults meant the pre-BoringSSL
Android/OpenSSL Chromium port wrote out Channel ID keys in this format.
By now this should no longer by an issue, but it'll warrant a bit more
investigation to be sure we can drop it.
For now, keep this logic out of SPKIs by not calling d2i_ECParameters.
d2i_ECParameters is a fairly pointless function when only named curves
are allowed. In testing other implementations, none of Firefox, Safari,
or IE11/Win will parse such certificates (i.e. the error is fatal and
unbypassable). Likewise, because Mac and Windows' underlying libraries
reject this, Chrome on Mac and Windows already rejects such things. Thus
this change should be compatible.
The following is the certificate and key I constructed to test with:
-----BEGIN CERTIFICATE-----
MIICwjCCAmqgAwIBAgIJANlMBNpJfb/rMAkGByqGSM49BAEwRTELMAkGA1UEBhMC
QVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGEludGVybmV0IFdpZGdp
dHMgUHR5IEx0ZDAeFw0xNDA0MjMyMzIxNTdaFw0xNDA1MjMyMzIxNTdaMEUxCzAJ
BgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5l
dCBXaWRnaXRzIFB0eSBMdGQwggFLMIIBAwYHKoZIzj0CATCB9wIBATAsBgcqhkjO
PQEBAiEA/////wAAAAEAAAAAAAAAAAAAAAD///////////////8wWwQg/////wAA
AAEAAAAAAAAAAAAAAAD///////////////wEIFrGNdiqOpPns+u9VXaYhrxlHQaw
zFOw9jvOPD4n0mBLAxUAxJ02CIbnBJNqZnjhE50mt4GffpAEQQRrF9Hy4SxCR/i8
5uVjpEDydwN9gS3rM6D0oTlF2JjClk/jQuL+Gn+bjufrSnwPnhYrzjNXazFezsu2
QGg3v1H1AiEA/////wAAAAD//////////7zm+q2nF56E87nKwvxjJVECAQEDQgAE
5itp4r9ln5e+Lx4NlIpM1Zdrt6keDUb73ampHp3culoB59aXqAoY+cPEox5W4nyD
SNsWGhz1HX7xlC1Lz3IiwaNQME4wHQYDVR0OBBYEFKuE0qyrlfCCThZ4B1VXX+Qm
jYLRMB8GA1UdIwQYMBaAFKuE0qyrlfCCThZ4B1VXX+QmjYLRMAwGA1UdEwQFMAMB
Af8wCQYHKoZIzj0EAQNHADBEAiBATB6aVJxDD6YAxEM4vf6Sbg2Ty334ldXpkNwc
TF+SngIgZ/f59kgDLf6YA04iLw1fUv5Wf1nLYJWwgrRFON5+zvw=
-----END CERTIFICATE-----
-----BEGIN EC PARAMETERS-----
MIH3AgEBMCwGByqGSM49AQECIQD/////AAAAAQAAAAAAAAAAAAAAAP//////////
/////zBbBCD/////AAAAAQAAAAAAAAAAAAAAAP///////////////AQgWsY12Ko6
k+ez671VdpiGvGUdBrDMU7D2O848PifSYEsDFQDEnTYIhucEk2pmeOETnSa3gZ9+
kARBBGsX0fLhLEJH+Lzm5WOkQPJ3A32BLeszoPShOUXYmMKWT+NC4v4af5uO5+tK
fA+eFivOM1drMV7Oy7ZAaDe/UfUCIQD/////AAAAAP//////////vOb6racXnoTz
ucrC/GMlUQIBAQ==
-----END EC PARAMETERS-----
-----BEGIN EC PRIVATE KEY-----
MHcCAQEEIAcPCHJ61KBKnN1ZyU2JaHcItW/JXTB3DujRyc4Ki7RqoAoGCCqGSM49
AwEHoUQDQgAE5itp4r9ln5e+Lx4NlIpM1Zdrt6keDUb73ampHp3culoB59aXqAoY
+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwQ==
-----END EC PRIVATE KEY-----
BUG=522228
Change-Id: I3723411a633dc07c4640027de07500293f8f7913
Reviewed-on: https://boringssl-review.googlesource.com/6853
Reviewed-by: Adam Langley <alangley@gmail.com>
OpenSSL accepts both OID 2.5.8.1.1 and OID 1.2.840.113549.1.1.1 for RSA
public keys. The latter comes from RFC 3279 and is widely implemented.
The former comes from the ITU-T version of X.509. Interestingly,
2.5.8.1.1 actually has a parameter, which OpenSSL ignores:
rsa ALGORITHM ::= {
KeySize
IDENTIFIED BY id-ea-rsa
}
KeySize ::= INTEGER
Remove support for 2.5.8.1.1 completely. In tests with a self-signed
certificate and code inspection:
- IE11 on Win8 does not accept the certificate in a TLS handshake at
all. Such a certificate is fatal and unbypassable. However Microsoft's
libraries do seem to parse it, so Chrome on Windows allows one to
click through the error. I'm guessing either the X.509 stack accepts
it while the TLS stack doesn't recognize it as RSA or the X.509 stack
is able to lightly parse it but not actually understand the key. (The
system certificate UI didn't display it as an RSA key, so probably the
latter?)
- Apple's certificate library on 10.11.2 does not parse the certificate
at all. Both Safari and Chrome on Mac treat it as a fatal and
unbypassable error.
- mozilla::pkix, from code inspection, does not accept such
certificates. However, Firefox does allow clicking through the error.
This is likely a consequence of mozilla::pkix and NSS having different
ASN.1 stacks. I did not test this, but I expect this means Chrome on
Linux also accepts it.
Given IE and Safari's results, it should be safe to simply remove this.
Firefox's data point is weak (perhaps someone is relying on being able
to click-through a self-signed 2.5.8.1.1 certificate), but it does
further ensure no valid certificate could be doing this.
The following is the 2.5.8.1.1 certificate I constructed to test with.
The private key is key.pem from ssl/test/runner:
-----BEGIN CERTIFICATE-----
MIICVTCCAb6gAwIBAgIJAPuwTC6rEJsMMA0GCSqGSIb3DQEBBQUAMEUxCzAJBgNV
BAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5ldCBX
aWRnaXRzIFB0eSBMdGQwHhcNMTQwNDIzMjA1MDQwWhcNMTcwNDIyMjA1MDQwWjBF
MQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UECgwYSW50
ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMIGcMAoGBFUIAQECAgQAA4GNADCBiQKBgQDY
K8imMuRi/03z0K1Zi0WnvfFHvwlYeyK9Na6XJYaUoIDAtB92kWdGMdAQhLciHnAj
kXLI6W15OoV3gA/ElRZ1xUpxTMhjP6PyY5wqT5r6y8FxbiiFKKAnHmUcrgfVW28t
Q+0rkLGMryRtrukXOgXBv7gcrmU7G1jC2a7WqmeI8QIDAQABo1AwTjAdBgNVHQ4E
FgQUi3XVrMsIvg4fZbf6Vr5sp3Xaha8wHwYDVR0jBBgwFoAUi3XVrMsIvg4fZbf6
Vr5sp3Xaha8wDAYDVR0TBAUwAwEB/zANBgkqhkiG9w0BAQUFAAOBgQAIZuUICtYv
w3cbpCGX6HNCtyI0guOfbytcdwzRkQaCsYNSDrTxrSSWxHwqg3Dl/RlvS+T3Yaua
Xkioadstwt7GDP6MwpIpdbjchh0XZd3kjdJWqXSvihUDpRePNjNS2LmJW8GWfB3c
F6UVyNK+wcApRY+goREIhyYupAHUexR7FQ==
-----END CERTIFICATE-----
BUG=522228
Change-Id: I031d03c0f53a16cbc749c4a5d8be6efca50dc863
Reviewed-on: https://boringssl-review.googlesource.com/6852
Reviewed-by: Adam Langley <alangley@gmail.com>
