Commit Graph

804 Commits

Author SHA1 Message Date
David Benjamin
6f0c4db90e Enable upstream's Poly1305 code.
The C implementation is still our existing C implementation, but slightly
tweaked to fit with upstream's init/block/emits convention.

I've tested this by looking at code coverage in kcachegrind and

  valgrind --tool=callgrind --dump-instr=yes --collect-jumps=yes

(NB: valgrind 3.11.0 is needed for AVX2. And even that only does 64-bit AVX2,
so we can't get coverage for the 32-bit code yet. But I had to disable that
anyway.)

This was paired with a hacked up version of poly1305_test that would repeat
tests with different ia32cap and armcap values. This isn't checked in, but we
badly need a story for testing all the different variants.

I'm not happy with upstream's code in either the C/asm boundary or how it
dispatches between different versions, but just debugging the code has been a
significant time investment. I'd hoped to extract the SIMD parts and do the
rest in C, but I think we need to focus on testing first (and use that to
guide what modifications would help). For now, this version seems to work at
least.

The x86 (not x86_64) AVX2 code needs to be disabled because it's broken. It
also seems pretty unnecessary.
https://rt.openssl.org/Ticket/Display.html?id=4346

Otherwise it seems to work and buys us a decent performance improvement.
Notably, my Nexus 6P is finally faster at ChaCha20-Poly1305 than my Nexus 4!

bssl speed numbers follow:

x86
---
Old:
Did 1554000 ChaCha20-Poly1305 (16 bytes) seal operations in 1000536us (1553167.5 ops/sec): 24.9 MB/s
Did 136000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1003947us (135465.3 ops/sec): 182.9 MB/s
Did 30000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1022990us (29325.8 ops/sec): 240.2 MB/s
Did 1888000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000206us (1887611.2 ops/sec): 30.2 MB/s
Did 173000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1003036us (172476.4 ops/sec): 232.8 MB/s
Did 30000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1027759us (29189.7 ops/sec): 239.1 MB/s
New:
Did 2030000 ChaCha20-Poly1305 (16 bytes) seal operations in 1000507us (2028971.3 ops/sec): 32.5 MB/s
Did 404000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1000287us (403884.1 ops/sec): 545.2 MB/s
Did 83000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1001258us (82895.7 ops/sec): 679.1 MB/s
Did 2018000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000006us (2017987.9 ops/sec): 32.3 MB/s
Did 360000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1001962us (359295.1 ops/sec): 485.0 MB/s
Did 85000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1002479us (84789.8 ops/sec): 694.6 MB/s

x86_64, no AVX2
---
Old:
Did 2023000 ChaCha20-Poly1305 (16 bytes) seal operations in 1000258us (2022478.2 ops/sec): 32.4 MB/s
Did 466000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1002619us (464782.7 ops/sec): 627.5 MB/s
Did 90000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1001133us (89898.1 ops/sec): 736.4 MB/s
Did 2238000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000175us (2237608.4 ops/sec): 35.8 MB/s
Did 483000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1001348us (482349.8 ops/sec): 651.2 MB/s
Did 90000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1003141us (89718.2 ops/sec): 735.0 MB/s
New:
Did 2558000 ChaCha20-Poly1305 (16 bytes) seal operations in 1000275us (2557296.7 ops/sec): 40.9 MB/s
Did 510000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1001810us (509078.6 ops/sec): 687.3 MB/s
Did 115000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1006457us (114262.2 ops/sec): 936.0 MB/s
Did 2818000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000187us (2817473.1 ops/sec): 45.1 MB/s
Did 418000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1001140us (417524.0 ops/sec): 563.7 MB/s
Did 91000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1002539us (90769.5 ops/sec): 743.6 MB/s

x86_64, AVX2
---
Old:
Did 2516000 ChaCha20-Poly1305 (16 bytes) seal operations in 1000115us (2515710.7 ops/sec): 40.3 MB/s
Did 774000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1000300us (773767.9 ops/sec): 1044.6 MB/s
Did 171000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1004373us (170255.5 ops/sec): 1394.7 MB/s
Did 2580000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000144us (2579628.5 ops/sec): 41.3 MB/s
Did 769000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1000472us (768637.2 ops/sec): 1037.7 MB/s
Did 169000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1000320us (168945.9 ops/sec): 1384.0 MB/s
New:
Did 3240000 ChaCha20-Poly1305 (16 bytes) seal operations in 1000114us (3239630.7 ops/sec): 51.8 MB/s
Did 932000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1000059us (931945.0 ops/sec): 1258.1 MB/s
Did 217000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1003282us (216290.1 ops/sec): 1771.8 MB/s
Did 3187000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000100us (3186681.3 ops/sec): 51.0 MB/s
Did 926000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1000071us (925934.3 ops/sec): 1250.0 MB/s
Did 215000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1000479us (214897.1 ops/sec): 1760.4 MB/s

arm, Nexus 4
---
Old:
Did 430248 ChaCha20-Poly1305 (16 bytes) seal operations in 1000153us (430182.2 ops/sec): 6.9 MB/s
Did 115250 ChaCha20-Poly1305 (1350 bytes) seal operations in 1000549us (115186.8 ops/sec): 155.5 MB/s
Did 27000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1030124us (26210.4 ops/sec): 214.7 MB/s
Did 451750 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000549us (451502.1 ops/sec): 7.2 MB/s
Did 118000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1001557us (117816.6 ops/sec): 159.1 MB/s
Did 27000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1024263us (26360.4 ops/sec): 215.9 MB/s
New:
Did 553644 ChaCha20-Poly1305 (16 bytes) seal operations in 1000183us (553542.7 ops/sec): 8.9 MB/s
Did 126000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1000396us (125950.1 ops/sec): 170.0 MB/s
Did 27000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1000336us (26990.9 ops/sec): 221.1 MB/s
Did 559000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1001465us (558182.3 ops/sec): 8.9 MB/s
Did 124000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1000824us (123897.9 ops/sec): 167.3 MB/s
Did 28000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1034854us (27057.0 ops/sec): 221.7 MB/s

aarch64, Nexus 6P
---
Old:
Did 358000 ChaCha20-Poly1305 (16 bytes) seal operations in 1000358us (357871.9 ops/sec): 5.7 MB/s
Did 45000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1022386us (44014.7 ops/sec): 59.4 MB/s
Did 8657 ChaCha20-Poly1305 (8192 bytes) seal operations in 1063722us (8138.4 ops/sec): 66.7 MB/s
Did 350000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000074us (349974.1 ops/sec): 5.6 MB/s
Did 44000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1007907us (43654.8 ops/sec): 58.9 MB/s
Did 8525 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1042644us (8176.3 ops/sec): 67.0 MB/s
New:
Did 713000 ChaCha20-Poly1305 (16 bytes) seal operations in 1000190us (712864.6 ops/sec): 11.4 MB/s
Did 180000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1004249us (179238.4 ops/sec): 242.0 MB/s
Did 41000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1005811us (40763.1 ops/sec): 333.9 MB/s
Did 775000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000719us (774443.2 ops/sec): 12.4 MB/s
Did 182000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1003529us (181360.0 ops/sec): 244.8 MB/s
Did 41000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1010576us (40570.9 ops/sec): 332.4 MB/s

Change-Id: Iaa4ab86ac1174b79833077963cc3616cfb08e686
Reviewed-on: https://boringssl-review.googlesource.com/7226
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 16:05:14 +00:00
David Benjamin
a211aee545 Add SSL_CIPHER_has_SHA256_HMAC.
Change-Id: I05a8f5d1778aba1813fe4d34b4baa21849158218
Reviewed-on: https://boringssl-review.googlesource.com/7215
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 01:33:11 +00:00
David Benjamin
e593fed378 Rename opensslfeatures.h to opensslconf.h.
Some software #includes opensslconf.h which typically contains settings that we
put in opensslfeatures.h (a header name not in OpenSSL). Rename it to
opensslconf.h.

Change-Id: Icd21dde172e5e489ce90dd5c16ae4d2696909fb6
Reviewed-on: https://boringssl-review.googlesource.com/7216
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 01:32:50 +00:00
David Benjamin
a473e554b4 Add BIO_do_connect.
Some consumers of connect BIOs connect them explicitly, and we already have the
BIO_ctrl hooked up.

Change-Id: Ie6b14f8ceb272b560e2b534e0b6c32fae050475b
Reviewed-on: https://boringssl-review.googlesource.com/7217
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 01:32:21 +00:00
Brian Smith
76c6381c21 Return 0 on error in |EC_POINT_is_on_curve| instead of -1.
Callers of this function are not checking for the -1 result. Change
the semantics to match their expectations and to match the common
semantics of most other parts of BoringSSL.

Change-Id: I4ec537d7619e20e8ddfee80c72125e4c02cfaac1
Reviewed-on: https://boringssl-review.googlesource.com/7125
Reviewed-by: David Benjamin <davidben@google.com>
2016-02-24 22:00:58 +00:00
Steven Valdez
d8eea14443 BIO_new_mem_buf should take const void *
BIO_FLAGS_MEM_RDONLY keeps the invariant.

(Imported from upstream's a38a159bfcbc94214dda00e0e6b1fc6454a23b78)

Change-Id: I4cb35615d76b77929915e370dbb7fec1455da069
Reviewed-on: https://boringssl-review.googlesource.com/7214
Reviewed-by: David Benjamin <davidben@google.com>
2016-02-24 19:14:19 +00:00
Steven Valdez
b9824e2417 Handle SSL_shutdown while in init more appropriately
Calling SSL_shutdown while in init previously gave a "1" response,
meaning everything was successfully closed down (even though it
wasn't). Better is to send our close_notify, but fail when trying to
receive one.

The problem with doing a shutdown while in the middle of a handshake
is that once our close_notify is sent we shouldn't really do anything
else (including process handshake/CCS messages) until we've received a
close_notify back from the peer. However the peer might send a CCS
before acting on our close_notify - so we won't be able to read it
because we're not acting on CCS messages!

(Imported from upstream's f73c737c7ac908c5d6407c419769123392a3b0a9)
Change-Id: Iaad5c5e38983456d3697c955522a89919628024b
Reviewed-on: https://boringssl-review.googlesource.com/7207
Reviewed-by: David Benjamin <davidben@google.com>
2016-02-24 15:57:09 +00:00
David Benjamin
85003903fc Remove CRYPTO_set_NEON_functional.
This depends on https://codereview.chromium.org/1730823002/. The bit was only
ever targetted to one (rather old) CPU. Disable NEON on it uniformly, so we
don't have to worry about whether any new NEON code breaks it.

BUG=589200

Change-Id: Icc7d17d634735aca5425fe0a765ec2fba3329326
Reviewed-on: https://boringssl-review.googlesource.com/7211
Reviewed-by: Adam Langley <agl@google.com>
2016-02-23 23:19:46 +00:00
David Benjamin
de94238217 Fix SSL_get_{read,write}_sequence.
I switched up the endianness. Add some tests to make sure those work right.

Also tweak the DTLS semantics. SSL_get_read_sequence should return the highest
sequence number received so far. Include the epoch number in both so we don't
need a second API for it.

Change-Id: I9901a1665b41224c46fadb7ce0b0881dcb466bcc
Reviewed-on: https://boringssl-review.googlesource.com/7141
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 22:05:29 +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
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
fda22a7573 Reimplement DSA parsing logic with crypto/asn1.
Functions which lose object reuse and need auditing:
- d2i_DSA_SIG
- d2i_DSAPublicKey
- d2i_DSAPrivateKey
- d2i_DSAparams

BUG=499653

Change-Id: I1cc2ae10e1e77eb57da3a858ac8734a95715ce4b
Reviewed-on: https://boringssl-review.googlesource.com/7022
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 00:26:01 +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
2f6410ba4e Rewrite ECPrivateKey serialization.
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>
2016-02-16 23:51:09 +00:00
David Benjamin
375124b162 Parse BER for PKCS#12 more accurately.
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>
2016-02-16 19:32:50 +00:00
Adam Langley
d057454f90 Changes to support node.js's use of PKCS#12.
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>
2016-02-02 19:21:59 +00:00
David Benjamin
6014ea6248 Add EC_POINT_point2cbb.
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>
2016-02-02 19:04:33 +00:00
David Benjamin
47ebec1210 Validate DH public keys for RFC 5114 groups.
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>
2016-02-02 16:44:38 +00:00
David Benjamin
43946d44ae Update references to the extended master secret draft.
It's now an RFC too.

Change-Id: I2aa7a862bf51ff01215455e87b16f259fc468490
Reviewed-on: https://boringssl-review.googlesource.com/7028
Reviewed-by: Adam Langley <agl@google.com>
2016-02-02 16:37:55 +00:00
David Benjamin
415564fe2c Update draft-irtf-cfrg-curves-11 references to RFC 7748.
Change-Id: I6148df93a1748754ee6be9e2b98cc8afd38746cb
Reviewed-on: https://boringssl-review.googlesource.com/6960
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-28 00:53:26 +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
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
acb2451807 Rename the BIGNUM ASN.1 functions.
There's many ways to serialize a BIGNUM, so not including asn1 in the name is
confusing (and collides with BN_bn2cbb_padded). Since BN_asn12bn looks
ridiculous, match the parse/marshal naming scheme of other modules instead.

Change-Id: I53d22ae0537a98e223ed943e943c48cb0743cf51
Reviewed-on: https://boringssl-review.googlesource.com/6822
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-27 22:37:44 +00:00
David Benjamin
8f2d4e344c Fix documentation string.
SSL_CTX_set_retain_only_sha256_of_client_certs's comment wasn't quite right.

Change-Id: I40527eebf2988d117cd8bd544bb5f16936c2cbfb
Reviewed-on: https://boringssl-review.googlesource.com/6982
Reviewed-by: Adam Langley <agl@google.com>
2016-01-27 22:20:32 +00:00
David Benjamin
91b2501f02 Add functions for accessing read_sequence and write_sequence.
OpenSSL 1.1.0 doesn't seem to have these two, so this isn't based on anything.
Have them return uint64_t in preparation for switching the internal
representation to uint64_t so ssl_record_sequence_update can go away.

Change-Id: I21d55e9a29861c992f409ed293e0930a7aaef7a3
Reviewed-on: https://boringssl-review.googlesource.com/6941
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-27 22:03:57 +00:00
David Benjamin
cdd0b7e775 Add SSL_CTX_set_retain_only_sha256_of_client_certs.
We have the hook on the SSL_CTX, but it should be possible to set it without
reaching into SSL_CTX.

Change-Id: I93db070c7c944be374543442a8de3ce655a28928
Reviewed-on: https://boringssl-review.googlesource.com/6880
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-27 22:02:53 +00:00
David Benjamin
b83003ebc6 Don't initialize enc_method before version negotiation.
Move it into ssl->s3 so it automatically behaves correctly on SSL_clear.
ssl->version is still a mess though.

Change-Id: I17a692a04a845886ec4f8de229fa6cf99fa7e24a
Reviewed-on: https://boringssl-review.googlesource.com/6844
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-27 21:38:12 +00: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
Adam Langley
eac0ce09d8 Have doc.go parse struct comments.
In code, structs that happened to have a '(' somewhere in their body
would cause the parser to go wrong. This change fixes that and updates
the comments on a number of structs.

Change-Id: Ia76ead266615a3d5875b64a0857a0177fec2bd00
Reviewed-on: https://boringssl-review.googlesource.com/6970
Reviewed-by: Adam Langley <agl@google.com>
2016-01-26 23:23:23 +00:00
David Benjamin
f6494f4928 Add a SSL_get_pending_cipher API.
Conscrypt needs to, in the certificate verification callback, know the key
exchange + auth method of the current cipher suite to pass into
X509TrustManager.checkServerTrusted. Currently it reaches into the struct to
get it. Add an API for this.

Change-Id: Ib4e0a1fbf1d9ea24e0114f760b7524e1f7bafe33
Reviewed-on: https://boringssl-review.googlesource.com/6881
Reviewed-by: David Benjamin <davidben@google.com>
2016-01-26 21:51:02 +00:00
David Benjamin
7027d25c6b Also add a no-op stub for OPENSSL_config.
Apparently OpenSSL's API is made entirely of initialization functions.
Some external libraries like to initialize with OPENSSL_config instead.

Change-Id: I28efe97fc5eb21309f560c84112b80e947f8bb17
Reviewed-on: https://boringssl-review.googlesource.com/6981
Reviewed-by: Adam Langley <agl@google.com>
2016-01-26 15:48:51 +00:00
David Benjamin
e5aa791a1c Add a few more no-op stubs for cURL compatibility.
With these stubs, cURL should not need any BoringSSL #ifdefs at all,
except for their OCSP #ifdefs (which can switch to the more generally
useful OPENSSL_NO_OCSP) and the workaround for wincrypt.h macro
collisions. That we intentionally leave to the consumer rather than add
a partial hack that makes the build sensitive to include order.

(I'll send them a patch upstream once this cycles in.)

Change-Id: I815fe67e51e80e9aafa9b91ae68867ca1ff1d623
Reviewed-on: https://boringssl-review.googlesource.com/6980
Reviewed-by: Adam Langley <agl@google.com>
2016-01-26 15:48:41 +00:00
David Benjamin
232127d245 Fold EC_GROUP_new_curve_GFp and EC_GROUP_set_generator into a EC_GROUP_new_arbitrary.
This is only for Conscrypt which always calls the pair in succession. (Indeed
it wouldn't make any sense to not call it.) Remove those two APIs and replace
with a single merged API. This way incomplete EC_GROUPs never escape outside
our API boundary and EC_GROUPs may *finally* be made immutable.

Also add a test for this to make sure I didn't mess it up.

Add a temporary BORINGSSL_201512 define to ease the transition for Conscrypt.
Conscrypt requires https://android-review.googlesource.com/#/c/187801/ before
picking up this change.

Change-Id: I3706c2ceac31ed2313175ba5ee724bd5c74ef6e1
Reviewed-on: https://boringssl-review.googlesource.com/6550
Reviewed-by: Adam Langley <agl@google.com>
2016-01-21 22:35:46 +00:00
David Benjamin
95219feafd Fix some documentation comments.
The new OPENSSL_PRINTF_FORMAT_FUNC macro let doc.go catch a few problems. It
also confuses doc.go, but this CL doesn't address that. At some point we
probably need to give it a real C parser.

Change-Id: I39f945df04520d1e0a0ba390cac7b308baae0622
Reviewed-on: https://boringssl-review.googlesource.com/6940
Reviewed-by: Adam Langley <agl@google.com>
2016-01-21 22:12:08 +00:00
Brian Smith
8d3c43e4b1 Annotate |ERR_add_error_dataf| as |OPENSSL_PRINTF_FORMAT_FUNC|.
Besides being a good idea anyway, this avoids clang warning about using
a non-literal format string when |ERR_add_error_dataf| calls
|BIO_vsnprintf|.

Change-Id: Iebc84d9c9d85e08e93010267d473387b661717a5
Reviewed-on: https://boringssl-review.googlesource.com/6920
Reviewed-by: David Benjamin <davidben@google.com>
2016-01-21 20:59:18 +00:00
Brian Smith
061332f216 Define |OPENSSL_PRINTF_FORMAT_FUNC| for format string annotations.
This centralizes the conditional logic into openssl/base.h so that it
doesn't have to be repeated. The name |OPENSSL_PRINTF_FORMAT_FUNC| was
chosen in anticipation of eventually defining an
|OPENSSL_PRINTF_FORMAT_ARG| for MSVC-style parameter annotations.

Change-Id: I273e6eddd209e696dc9f82099008c35b6d477cdb
Reviewed-on: https://boringssl-review.googlesource.com/6909
Reviewed-by: David Benjamin <davidben@google.com>
2016-01-21 20:58:51 +00:00
Adam Langley
3a39b06011 Import “altchains” support.
This change imports the following changes from upstream:

6281abc79623419eae6a64768c478272d5d3a426
dfd3322d72a2d49f597b86dab6f37a8cf0f26dbf
f34b095fab1569d093b639bfcc9a77d6020148ff
21376d8ae310cf0455ca2b73c8e9f77cafeb28dd
25efcb44ac88ab34f60047e16a96c9462fad39c1
56353962e7da7e385c3d577581ccc3015ed6d1dc
39c76ceb2d3e51eaff95e04d6e4448f685718f8d
a3d74afcae435c549de8dbaa219fcb30491c1bfb

These contain the “altchains” functionality which allows OpenSSL to
backtrack when chain building.

Change-Id: I8d4bc2ac67b90091f9d46e7355cae878b4ccf37d
Reviewed-on: https://boringssl-review.googlesource.com/6905
Reviewed-by: Adam Langley <agl@google.com>
2016-01-19 17:02:31 +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
928f32a492 Add APIs to extract the SSL key block.
This is a companion to SSL_get_rc4_state and SSL_get_ivs which doesn't
require poking at internal state. Partly since it aligns with the
current code and partly the off chance we ever need to get
wpa_supplicant's EAP-FAST code working, the API allows one to generate
more key material than is actually in the key block.

Change-Id: I58bc3f2b017482dbb8567dcd0cd754947a95397f
Reviewed-on: https://boringssl-review.googlesource.com/6839
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-15 22:09:24 +00:00
David Benjamin
79978df4ec Move aead_{read,write}_ctx and next_proto_negotiated into ssl->s3.
Both are connection state rather than configuration state. Notably this
cuts down more of SSL_clear that can't just use ssl_free + ssl_new.

Change-Id: I3c05b3ae86d4db8bd75f1cd21656f57fc5b55ca9
Reviewed-on: https://boringssl-review.googlesource.com/6835
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-15 21:40:25 +00:00
David Benjamin
4119d42e7c Tidy up keyblock and CCS logic slightly.
Move the actual SSL_AEAD_CTX swap into the record layer. Also revise the
intermediate state we store between setup_key_block and
change_cipher_state. With SSL_AEAD_CTX_new abstracted out, keeping the
EVP_AEAD around doesn't make much sense. Just store enough to partition
the key block.

Change-Id: I773fb46a2cb78fa570f00c0a89339c15bbb1d719
Reviewed-on: https://boringssl-review.googlesource.com/6832
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-15 20:40:44 +00:00
David Benjamin
96ba15fc69 Add SSL_get_client_random and SSL_get_server_random.
wpa_supplicant needs to get at the client and server random. OpenSSL
1.1.0 added these APIs, so match their semantics.

Change-Id: I2b71ba850ac63e574c9ea79012d1d0efec5a979a
Reviewed-on: https://boringssl-review.googlesource.com/6830
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-15 20:15:29 +00:00
David Benjamin
fe5f7c7b56 Only reserve EVP_MAX_MD_SIZE for the Finished, not twice of it.
EVP_MAX_MD_SIZE is large enough to fit a MD5/SHA1 concatenation, and
necessarily is because EVP_md5_sha1 exists. This shaves 128 bytes of
per-connection state.

Change-Id: I848a8563dfcbac14735bb7b302263a638528f98e
Reviewed-on: https://boringssl-review.googlesource.com/6804
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 23:29:21 +00:00
David Benjamin
974c7ba4ef Route DHE through the SSL_ECDH abstraction as well.
This unifies the ClientKeyExchange code rather nicely. ServerKeyExchange
is still pretty specialized. For simplicity, I've extended the yaSSL bug
workaround for clients as well as servers rather than route in a
boolean.

Chrome's already banished DHE to a fallback with intention to remove
altogether later, and the spec doesn't say anything useful about
ClientDiffieHellmanPublic encoding, so this is unlikely to cause
problems.

Change-Id: I0355cd1fd0fab5729e8812e4427dd689124f53a2
Reviewed-on: https://boringssl-review.googlesource.com/6784
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 23:17:32 +00:00
David Benjamin
4cc36adf5a Make it possible to tell what curve was used on the server.
We don't actually have an API to let you know if the value is legal to
interpret as a curve ID. (This was kind of a poor API. Oh well.) Also add tests
for key_exchange_info. I've intentionally left server-side plain RSA missing
for now because the SSL_PRIVATE_KEY_METHOD abstraction only gives you bytes and
it's probably better to tweak this API instead.

(key_exchange_info also wasn't populated on the server, though due to a
rebasing error, that fix ended up in the parent CL. Oh well.)

Change-Id: I74a322c8ad03f25b02059da7568c9e1a78419069
Reviewed-on: https://boringssl-review.googlesource.com/6783
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 23:12:25 +00:00
David Benjamin
4298d77379 Implement draft-ietf-tls-curve25519-01 in C.
The new curve is not enabled by default.

As EC_GROUP/EC_POINT is a bit too complex for X25519, this introduces an
SSL_ECDH_METHOD abstraction which wraps just the raw ECDH operation. It
also tidies up some of the curve code which kept converting back and
force between NIDs and curve IDs. Now everything transits as curve IDs
except for API entry points (SSL_set1_curves) which take NIDs. Those
convert immediately and act on curve IDs from then on.

Note that, like the Go implementation, this slightly tweaks the order of
operations. The client sees the server public key before sending its
own. To keep the abstraction simple, SSL_ECDH_METHOD expects to
generate a keypair before consuming the peer's public key. Instead, the
client handshake stashes the serialized peer public value and defers
parsing it until it comes time to send ClientKeyExchange. (This is
analogous to what it was doing before where it stashed the parsed peer
public value instead.)

It still uses TLS 1.2 terminology everywhere, but this abstraction should also
be compatible with TLS 1.3 which unifies (EC)DH-style key exchanges.
(Accordingly, this abstraction intentionally does not handle parsing the
ClientKeyExchange/ServerKeyExchange framing or attempt to handle asynchronous
plain RSA or the authentication bits.)

BUG=571231

Change-Id: Iba09dddee5bcdfeb2b70185308e8ab0632717932
Reviewed-on: https://boringssl-review.googlesource.com/6780
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 21:51:30 +00:00
David Benjamin
c18ef750ee Allocate a NID for X25519.
No corresponding OID, but SSL_CTX_set1_curves assumes NIDs exist.

BUG=571231

Change-Id: Id5221cdc59132e26a89ae5f8978b946de690b4e0
Reviewed-on: https://boringssl-review.googlesource.com/6779
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 18:56:53 +00:00