GitLab

p256-x86_64.c can't build in MSVC 2013 in debug mode along because of
ecp_nistz256_points_mul's very awkward calling convention (at least one of
g_scalar/p_scalar must be non-NULL).

MSVC 2015 seems to be okay with it, but this branch doesn't build with 2015 yet
and is currently used by gRPC folks. Later they'll move to a branch that builds
with 2015, so the issue won't be pressing. Though the EC_METHOD mul calling
convention is still a little screwy.

The simplest immediate option is to suppress the warning on this branch. The
warning will be left alive on master (which now requires 2015 anyway), but we
should add release-mode builders.

Change-Id: Ia9274887f49dbe8f53c44c046b6f1323425702b0
Reviewed-on: https://boringssl-review.googlesource.com/7987

Reviewed-by: Adam Langley <[email protected]>

The high bits of the type get used for the V_ASN1_NEG bit, so when used with
ASN1_ANY/ASN1_TYPE, universal tags become ambiguous. This allows one to create
a negative zero, which should be impossible. Impose an upper bound on universal
tags accepted by crypto/asn1 and add a test.

BUG=590615

Change-Id: I363e01ebfde621c8865101f5bcbd5f323fb59e79
Reviewed-on: https://boringssl-review.googlesource.com/7238

Reviewed-by: Adam Langley <[email protected]>

(Imported from upstream's 3661bb4e7934668bd99ca777ea8b30eedfafa871.)

Fix bug where i2c_ASN1_INTEGER mishandles zero if it is marked as
negative.

Thanks to Huzaifa Sidhpurwala <[email protected]> and Hanno Böck
<[email protected]> for reporting this issue.

BUG=590615

Change-Id: I8959e8ae01510a5924862a3f353be23130eee554
Reviewed-on: https://boringssl-review.googlesource.com/7199

Reviewed-by: David Benjamin <[email protected]>

See OpenSSL df057ea6c8a20e4babc047689507dfafde59ffd6.

Change-Id: Ife10dc13ca335cd51434d7769ff85c6929f10226
Reviewed-on: https://boringssl-review.googlesource.com/7172

Reviewed-by: David Benjamin <[email protected]>

While it's always safe to read |SSL_MAX_SSL_SESSION_ID_LENGTH| bytes
from an |SSL_SESSION|'s |session_id| array, the hash function would do
so with without considering if all those bytes had been written to.

This change checks |session_id_length| before possibly reading
uninitialised memory. Since the result of the hash function was already
attacker controlled, and since a lookup of a short session ID will
always fail, it doesn't appear that this is anything more than a clean
up.

BUG=586800

Change-Id: I5f59f245b51477d6d4fa2cdc20d40bb6b4a3eae7
Reviewed-on: https://boringssl-review.googlesource.com/7150

Reviewed-by: David Benjamin <[email protected]>

crypto/x509 parses the SPKI on-demand, so we weren't actually exercising the
SPKI code.

Change-Id: I2e16045bd35dbe04d4b8d8b45939c8885e09a550
Reviewed-on: https://boringssl-review.googlesource.com/7161

Reviewed-by: Adam Langley <[email protected]>

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 <[email protected]>

Change-Id: If33d3a11a0b48403fc009688b9300c92e5494d94
Reviewed-on: https://boringssl-review.googlesource.com/7160

Reviewed-by: Adam Langley <[email protected]>

Currently, we correctly refuse to parse version 0 multi-prime keys, but we
still parse version 1 two-prime keys. Both should be rejected.

I missed an additional clause in the spec originally. It seems otherPrimeInfos
is marked OPTIONAL not because it is actually optional, but because they wanted
the two RSAPrivateKey forms to share one definition. The prose rules following
the definition imply that otherPrimeInfos' presence is entirely determined by
the version:

    * version is the version number, for compatibility with future
      revisions of this document.  It shall be 0 for this version of the
      document, unless multi-prime is used, in which case it shall be 1.

            Version ::= INTEGER { two-prime(0), multi(1) }
               (CONSTRAINED BY
               {-- version must be multi if otherPrimeInfos present --})

and:

    * otherPrimeInfos contains the information for the additional primes
      r_3, ..., r_u, in order.  It shall be omitted if version is 0 and
      shall contain at least one instance of OtherPrimeInfo if version
      is 1.

Change-Id: I458232a2e20ed68fddcc39c4c45333f33441f70b
Reviewed-on: https://boringssl-review.googlesource.com/7143

Reviewed-by: Adam Langley <[email protected]>

That was probably more complexity than we needed. Nothing uses it
anymore, now that getting to the PKCS#8 logic isn't especially tedious.

Change-Id: I4f0393b1bd75e71664f65e3722c14c483c13c5cf
Reviewed-on: https://boringssl-review.googlesource.com/6867

Reviewed-by: Adam Langley <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

There's nothing in here.

Change-Id: I3a501389e7e237b2e6907f27d2eb788a298d6c03
Reviewed-on: https://boringssl-review.googlesource.com/6877

Reviewed-by: Adam Langley <[email protected]>

BUG=499653

Change-Id: I16963fb198609d7fc0df6c57923cda3e13350753
Reviewed-on: https://boringssl-review.googlesource.com/6875

Reviewed-by: Adam Langley <[email protected]>

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 <[email protected]>

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 <[email protected]>

It doesn't do anything.

Change-Id: Ifcc2c824faf6012d2a442208b8204a32e141a650
Reviewed-on: https://boringssl-review.googlesource.com/7073

Reviewed-by: Adam Langley <[email protected]>

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 <[email protected]>

In c0d94849, 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 <[email protected]>

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 <[email protected]>
Reviewed-by: David Benjamin <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Change-Id: I7af2c87fe6e7513aa2603d5e845a4db87ab14fcc
Reviewed-on: https://boringssl-review.googlesource.com/7101

Reviewed-by: David Benjamin <[email protected]>

Some instances were missed in eca509c8.

Change-Id: I53a6bd944fbf0df439b8e6f9db761f61d7237ba2
Reviewed-on: https://boringssl-review.googlesource.com/7103

Reviewed-by: David Benjamin <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Change-Id: I4e1ed0aaddf4dc516a81155ef62dba138f8495ae
Reviewed-on: https://boringssl-review.googlesource.com/7120

Reviewed-by: Adam Langley <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Change-Id: If9c5031855c0acfafb73caba169e146f0e16f706
Reviewed-on: https://boringssl-review.googlesource.com/7093

Reviewed-by: David Benjamin <[email protected]>

Change-Id: I995a445fea1de7f85ec917694abb8273a82339d3
Reviewed-on: https://boringssl-review.googlesource.com/7092

Reviewed-by: David Benjamin <[email protected]>

Change-Id: Ia80372316e67822d44b8b90f7983f3ef773ed0fd
Reviewed-on: https://boringssl-review.googlesource.com/7091

Reviewed-by: David Benjamin <[email protected]>

|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 9f1f04f3, 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 <[email protected]>

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 <[email protected]>