Commit Graph

151 Commits

Author SHA1 Message Date
David Benjamin
65dac9c8a3 Fix the name of OPENSSL_add_all_algorithms_conf.
I named the compatibility function wrong.

Change-Id: Idc289c317c5826c338c1daf58a2d3b26b09a7e49
Reviewed-on: https://boringssl-review.googlesource.com/8301
Reviewed-by: Adam Langley <agl@google.com>
2016-06-15 21:29:50 +00:00
David Benjamin
a353cdb671 Wrap MSVC-only warning pragmas in a macro.
There's a __pragma expression which allows this. Android builds us Windows with
MinGW for some reason, so we actually do have to tolerate non-MSVC-compatible
Windows compilers. (Clang for Windows is much more sensible than MinGW and
intentionally mimicks MSVC.)

MinGW doesn't understand MSVC's pragmas and warns a lot. #pragma warning is
safe to suppress, so wrap those to shush them. This also lets us do away with a
few ifdefs.

Change-Id: I1f5a8bec4940d4b2d947c4c1cc9341bc15ec4972
Reviewed-on: https://boringssl-review.googlesource.com/8236
Reviewed-by: Adam Langley <agl@google.com>
2016-06-09 21:29:36 +00:00
David Benjamin
83042a8292 Add a no-op OpenSSL_add_all_algorithms_conf.
More spring-cleaning of unnecessary incompatibilities. Since
OpenSSL_add_all_algorithms_conf doesn't specify a configuration file, it's
perfectly sound to have such a function.

Dear BoringSSL, please add all algorithms.

  Uh, sure. They were already all there, but I have added them!

PS: Could you also load all your configuration files while you're at it.

  ...I don't have any. Fine. I have loaded all configuration files which I
  recognize. *mutters under breath* why does everyone ask all these strange
  questions...

Change-Id: I57f956933d9e519445bf22f89853bd5f56904172
Reviewed-on: https://boringssl-review.googlesource.com/8160
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-06 15:58:02 +00:00
David Benjamin
919610b4c4 Fix memory leak on invalid ecPublicKey parameters.
One of the codepaths didn't free the group. Found by libFuzzer.

BUG=chromium:603893

Change-Id: Icb81f2f89a8c1a52e29069321498986b193a0e56
Reviewed-on: https://boringssl-review.googlesource.com/7685
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-04-15 19:43:26 +00:00
David Benjamin
046b27815e Decouple crypto/evp from the OID table.
BUG=chromium:499653

Change-Id: I4e8d4af3129dbf61d4a8846ec9db685e83999d5e
Reviewed-on: https://boringssl-review.googlesource.com/7565
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-31 22:12:46 +00:00
David Benjamin
0d76c402b8 Decouple crypto/ec from the OID table.
Instead, embed the (very short) encoding of the OID into built_in_curve.

BUG=chromium:499653

Change-Id: I0db36f83c71fbd3321831f54fa5022f8304b30cd
Reviewed-on: https://boringssl-review.googlesource.com/7564
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-31 22:12:09 +00:00
David Benjamin
981936791e Remove some easy obj.h dependencies.
A lot of consumers of obj.h only want the NID values. Others didn't need
it at all. This also removes some OBJ_nid2sn and OBJ_nid2ln calls in EVP
error paths which isn't worth pulling a large table in for.

BUG=chromium:499653

Change-Id: Id6dff578f993012e35b740a13b8e4f9c2edc0744
Reviewed-on: https://boringssl-review.googlesource.com/7563
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-31 20:50:33 +00:00
David Benjamin
17d729e61b Remove unnecessary include.
Change-Id: I24d0179ca5019e82ca1494c8773f373f8c09ce82
Reviewed-on: https://boringssl-review.googlesource.com/7566
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-28 15:57:17 +00:00
Piotr Sikora
fdb88ba2e9 Fix build with -Wwrite-strings.
Change-Id: If76154c8d255600e925a408acdc674fc7dad0359
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7526
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-24 03:11:20 +00:00
David Benjamin
51545ceac6 Remove a number of unnecessary stdio.h includes.
Change-Id: I6267c9bfb66940d0b6fe5368514210a058ebd3cc
Reviewed-on: https://boringssl-review.googlesource.com/7494
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-17 18:22:28 +00:00
Emily Stark
62e0219679 Handle empty keys in EVP_marshal_public_key()
Instead of crashing when an empty key is passed to
EVP_marshal_public_key(), return with an
EVP_R_UNSUPPORTED_ALGORITHM_ERROR. This brings e.g. X509_PUBKEY_set()
closer to how it behaved before 68772b31 (previously, it returned an
error on an empty public key rather than dereferencing pkey->ameth).

Change-Id: Ieac368725adb7f22329c035d9d0685b44b885888
Reviewed-on: https://boringssl-review.googlesource.com/7351
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-07 15:54:54 +00:00
David Benjamin
2c198fae28 Enforce that d2i_PrivateKey returns a key of the specified type.
If d2i_PrivateKey hit the PKCS#8 codepath, it didn't enforce that the key was
of the specified type.

Note that this requires tweaking d2i_AutoPrivateKey slightly. A PKCS #8
PrivateKeyInfo may have 3 or 4 elements (optional attributes), so we were
relying on this bug for d2i_AutoPrivateKey to work.

Change-Id: If50b7a742f535d208e944ba37c3a585689d1da43
Reviewed-on: https://boringssl-review.googlesource.com/7253
Reviewed-by: Adam Langley <agl@google.com>
2016-03-01 00:06:55 +00:00
David Benjamin
f4e447c16d Move ASN1_bn_print to a static function in evp/print.c.
It's not used anywhere else, in the library or consumers (Google ones or
ones I could find on Debian codesearch). This is a sufficiently
specialized function that the risk of a third-party library newly
depending on it is low. This removes the last include of asn1.h or
x509.h in crypto/evp.

(This is almost entirely cosmetic because it wasn't keeping the static linker
from doing the right thing anyway. But if we were want to separate the legacy
ASN.1 stack into its own decrepit-like target, we'll need to be pickier about
separation.)

Change-Id: I9be97c9321572e3a2ed093e1d50036b7654cff41
Reviewed-on: https://boringssl-review.googlesource.com/7080
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 23:35:10 +00:00
David Benjamin
921d906bb6 Reimplement d2i_PrivateKey.
Functions which lose object reuse and need auditing:
- d2i_PrivateKey

This removes evp_asn1.c's dependency on the old stack. (Aside from
obj/.) It also takes old_priv_decode out of EVP_ASN1_METHOD in favor of
calling out to the new-style function. EVP_ASN1_METHOD no longer has any
old-style type-specific serialization hooks, only the PKCS#8 and SPKI
ones.

BUG=499653

Change-Id: Ic142dc05a5505b50e4717c260d3893b20e680194
Reviewed-on: https://boringssl-review.googlesource.com/7027
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 23:33:50 +00:00
David Benjamin
6d3387d9c1 Reimplement d2i_AutoPrivateKey with the new ASN.1 stack.
This is kind of a ridiculous function. It would be nice to lose it, but
SSL_use_PrivateKey_file actually calls into it (by way of
d2i_PrivateKey_bio).

BUG=499653

Change-Id: I83634f6982b15f4b877e29f6793b7e00a1c10450
Reviewed-on: https://boringssl-review.googlesource.com/7026
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 22:55:56 +00:00
David Benjamin
8ebc0f55a0 Decouple the EVP and PEM code.
EVP_PKEY_asn1_find can already be private. EVP_PKEY_asn1_find_str is used
only so the PEM code can get at legacy encoders. Since this is all
legacy non-PKCS8 stuff, we can just explicitly list out the three cases
in the two places that need it. If this changes, we can later add a
table in crypto/pem mapping string to EVP_PKEY type.

With this, EVP_PKEY_ASN1_METHOD is no longer exposed in the public API
and nothing outside of EVP_PKEY reaches into it. Unexport all of that.

Change-Id: Iab661014247dbdbc31e5e9887364176ec5ad2a6d
Reviewed-on: https://boringssl-review.googlesource.com/6871
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 22:50:21 +00:00
David Benjamin
3f4f7ee08f PEM_write_bio_PrivateKey is always PKCS#8.
Every key type which has a legacy PEM encoding also has a PKCS#8
encoding. The fallback codepath is never reached.

This removes the only consumer of pem_str, so that may be removed from
EVP_PKEY_ASN1_METHOD.

Change-Id: Ic680bfc162e1dc76db8b8016f6c10f669b24f5aa
Reviewed-on: https://boringssl-review.googlesource.com/6870
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 22:41:17 +00:00
David Benjamin
8c07ad3e3b Pull EVP_PKEY print hooks out of the main method table.
This allows the static linker to drop it in consumers which don't need this
stuff (i.e. all sane ones), once crypto/x509 falls off. This cuts down
on a number of dependencies from the core crypto bits on crypto/asn1 and
crypto/x509.

BUG=499653

Change-Id: I76a10a04dcc444c1ded31683df9f87725a95a4e6
Reviewed-on: https://boringssl-review.googlesource.com/5660
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 22:40:44 +00:00
David Benjamin
17727c6843 Move all signature algorithm code to crypto/x509.
All the signature algorithm logic depends on X509_ALGOR. This also
removes the X509_ALGOR-based EVP functions which are no longer used
externally. I think those APIs were a mistake on my part. The use in
Chromium was unnecessary (and has since been removed anyway). The new
X.509 stack will want to process the signatureAlgorithm itself to be
able to enforce policies on it.

This also moves the RSA_PSS_PARAMS bits to crypto/x509 from crypto/rsa.
That struct is also tied to crypto/x509. Any new RSA-PSS code would
have to use something else anyway.

BUG=499653

Change-Id: I6c4b4573b2800a2e0f863d35df94d048864b7c41
Reviewed-on: https://boringssl-review.googlesource.com/7025
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 22:39:02 +00:00
David Benjamin
93a69b4f8f Move X.509 signature algorithm tests to the crypto/x509 layer.
This is in preparation for moving the logic itself to crypto/x509, so
the lower-level functions will not be as readily available.

Change-Id: I6507b895317df831ab11d0588c5b09bbb2aa2c24
Reviewed-on: https://boringssl-review.googlesource.com/7023
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 22:38:50 +00:00
David Benjamin
da295d35f2 Drop the DSA signature printing hook.
It's only used by crypto/x509, and we don't even support DSA in
crypto/x509 anymore since the EVP_PKEY_CTX hooks aren't wired up.

Change-Id: I1b8538353eb51df353cf9171b1cbb0bb47a879a3
Reviewed-on: https://boringssl-review.googlesource.com/7024
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 22:19:37 +00:00
David Benjamin
e30a09e604 Implement new PKCS#8 parsers.
As with SPKI parsers, the intent is make EVP_PKEY capture the key's
constraints in full fidelity, so we'd have to add new types or store the
information in the underlying key object if people introduce variant key
types with weird constraints on them.

Note that because PKCS#8 has a space for arbitrary attributes, this
parser must admit a hole. I'm assuming for now that we don't need an API
that enforces no attributes and just ignore trailing data in the
structure for simplicity.

BUG=499653

Change-Id: I6fc641355e87136c7220f5d7693566d1144a68e8
Reviewed-on: https://boringssl-review.googlesource.com/6866
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 17:24:10 +00:00
David Benjamin
440f103771 Remove support for mis-encoded PKCS#8 DSA keys.
Previously, OpenSSL supported many different DSA PKCS#8 encodings. Only
support the standard format. One of the workaround formats (SEQUENCE of
private key and public key) seems to be a workaround for an old Netscape
bug. From inspection, NSS seems to have fixed this from the first open
source commit.

Change-Id: I1e097b675145954b4d7a0bed8733e5a25c25fd8e
Reviewed-on: https://boringssl-review.googlesource.com/7074
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 16:32:31 +00:00
David Benjamin
239a0abfd5 Slightly simplify and deprecate i2d_{Public,Private}Key.
There are all the type-specific serializations rather than something
tagged with a type. i2d_PrivateKey's PKCS#8 codepath was unreachable
because every EVP_PKEY type has an old_priv_encode function.

To prune EVP_PKEY_ASN1_METHOD further, replace i2d_PrivateKey into a
switch case so we don't need to keep old_priv_encode around. This cuts
down on a case of outside modules reaching into crypto/evp method
tables.

Change-Id: I30db2eed836d560056ba9d1425b960d0602c3cf2
Reviewed-on: https://boringssl-review.googlesource.com/6865
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 16:31:26 +00:00
David Benjamin
32fdc512ca Remove param_decode and param_encode EVP_PKEY hooks.
They're only used by a pair of PEM functions, which are never used.

BUG=499653

Change-Id: I89731485c66ca328c634efbdb7e182a917f2a963
Reviewed-on: https://boringssl-review.googlesource.com/6863
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 16:30:29 +00:00
David Benjamin
68772b31b0 Implement new SPKI parsers.
Many consumers need SPKI support (X.509, TLS, QUIC, WebCrypto), each
with different ways to set signature parameters. SPKIs themselves can
get complex with id-RSASSA-PSS keys which come with various constraints
in the key parameters. This suggests we want a common in-library
representation of an SPKI.

This adds two new functions EVP_parse_public_key and
EVP_marshal_public_key which converts EVP_PKEY to and from SPKI and
implements X509_PUBKEY functions with them. EVP_PKEY seems to have been
intended to be able to express the supported SPKI types with
full-fidelity, so these APIs will continue this.

This means future support for id-RSASSA-PSS would *not* repurpose
EVP_PKEY_RSA. I'm worried about code assuming EVP_PKEY_RSA implies
acting on the RSA* is legal. Instead, it'd add an EVP_PKEY_RSA_PSS and
the data pointer would be some (exposed, so the caller may still check
key size, etc.) RSA_PSS_KEY struct. Internally, the EVP_PKEY_CTX
implementation would enforce the key constraints. If RSA_PSS_KEY would
later need its own API, that code would move there, but that seems
unlikely.

Ideally we'd have a 1:1 correspondence with key OID, although we may
have to fudge things if mistakes happen in standardization. (Whether or
not X.509 reuses id-ecPublicKey for Ed25519, we'll give it a separate
EVP_PKEY type.)

DSA parsing hooks are still implemented, missing parameters and all for
now. This isn't any worse than before.

Decoupling from the giant crypto/obj OID table will be a later task.

BUG=522228

Change-Id: I0e3964edf20cb795a18b0991d17e5ca8bce3e28c
Reviewed-on: https://boringssl-review.googlesource.com/6861
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 16:28:07 +00:00
David Benjamin
2dc469e066 Remove dead header file.
There's nothing in here.

Change-Id: I3a501389e7e237b2e6907f27d2eb788a298d6c03
Reviewed-on: https://boringssl-review.googlesource.com/6877
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 01:34:15 +00:00
David Benjamin
3cadf63c68 Remove DSA write_params.
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>
2016-02-16 23:54:38 +00:00
David Benjamin
666973b8e9 Add tests for EC keys with specified curves.
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>
2016-02-16 21:51:32 +00:00
Brian Smith
5ba06897be Don't cast |OPENSSL_malloc|/|OPENSSL_realloc| result.
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>
2016-02-11 22:07:56 +00:00
David Benjamin
4e3d17a7e7 Remove redundant logic to compute EC public key.
d2i_ECPrivateKey already computes it as of
9f5a314d35.

Change-Id: Ie48b2319ee7d96d09c8e4f13d99de38bfa89be76
Reviewed-on: https://boringssl-review.googlesource.com/6857
Reviewed-by: Adam Langley <agl@google.com>
2016-02-02 16:23:05 +00:00
David Benjamin
2cdf398773 Remove pkey_base_id.
This is never accessed.

Change-Id: I4cade5e907ad4c03e9de7634b53ef965f7240087
Reviewed-on: https://boringssl-review.googlesource.com/6864
Reviewed-by: Adam Langley <agl@google.com>
2016-01-28 15:55:24 +00:00
David Benjamin
4f6acaf0da Use more C++11 features.
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>
2016-01-28 00:52:37 +00:00
David Benjamin
0a2c9938a5 Don't allow the specifiedCurve form of ECParameters in SPKIs.
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>
2016-01-28 00:51:14 +00:00
David Benjamin
f6094e05ef Don't allow EVP_PKEY_RSA2.
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>
2016-01-28 00:43:37 +00:00
David Benjamin
c612e61e1d Fix minor stylistic problem.
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>
2016-01-28 00:35:35 +00:00
David Benjamin
719594e512 Un-const EVP_PKEY_CTX_set0_rsa_oaep_label and fix overflow check.
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>
2016-01-28 00:34:38 +00:00
David Benjamin
b6155e60f3 Remove app_data from EVP_PKEY_CTX.
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>
2016-01-28 00:29:34 +00:00
David Benjamin
4e98e5c903 Implement pkey_ec_keygen with EC_KEY APIs.
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>
2016-01-28 00:28:43 +00:00
David Benjamin
692878a5f4 Remove EVP_PKEY_CTRL_EC_PARAMGEN_CURVE_NID.
This is never exposed.

Change-Id: I332bc45f724eb42d68a0839e72b21593d01005ee
Reviewed-on: https://boringssl-review.googlesource.com/6847
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-28 00:27:42 +00:00
David Benjamin
8ac35f0274 Remove unused EVP_PKEY_METHOD hooks.
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>
2016-01-28 00:23:46 +00:00
Adam Langley
ef7dba6ac7 Fix error in ce9d85ee.
I used a size_t out of habit, but |RSA_public_decrypt| is an old-style
function.

Change-Id: Ibd94d03743fe0099d61578ec15c19fa5333127db
2016-01-26 15:31:23 -08:00
Adam Langley
ce9d85eedd Tweaks for node.js
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>
2016-01-26 23:23:42 +00:00
David Benjamin
5aae776ede Remove calls to ERR_load_crypto_strings.
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>
2016-01-25 23:09:08 +00:00
Adam Langley
62882187c9 Update comments to better document in-place semantics.
(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>
2016-01-19 17:01:37 +00:00
David Benjamin
e13263d5e4 Resolve a few old TODOs.
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>
2015-12-22 00:14:35 +00:00
David Benjamin
c3ae38b4f8 Remove DH EVP_PKEY hooks.
They would never work. Better notice when callers depend on it than fail at
runtime.

This depends on https://android-review.googlesource.com/#/c/183610/ in
Conscrypt.

Change-Id: I3411f291416df834cf85850890617625a2e76939
Reviewed-on: https://boringssl-review.googlesource.com/6552
Reviewed-by: Adam Langley <agl@google.com>
2015-12-16 17:38:06 +00:00
Brian Smith
a3d9de05fb Add |EC_GROUP_get0_order| to replace |EC_GROUP_get_order|.
|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>
2015-12-15 18:18:13 +00:00
David Benjamin
28243c08db Add PSS parameter check.
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>
2015-12-03 16:47:12 +00:00
David Benjamin
cb852981cd Fix leak with ASN.1 combine.
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>
2015-12-03 16:43:34 +00:00