GitLab

This silences a pile of -Wformat-signedness warnings. We still need
casts in a few places where the API gives int but really wanted
uint16_t. There I cast to unsigned instead of uint16_t for the sake of
not losing information.

With that, we should be -Wformat-signedness-clean on GCC, so enable the
warning.

Bug: 450
Change-Id: I3ab10348bb47d398b8b9b39acf360284a8ab04d7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50771

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Whether the order makes sense is another matter, but keep them aligned
so future flags have an easier time with it.

Change-Id: I3c3912039b593a55af86078b2e9768c76ee2ee14
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50770

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

The command-line parser is slightly showing its age: first, it is hard
to add new integral types, such as uint16_t, which is getting in the way
of fixing some of the -Wformat-signedness errors. Second, the parameter
extraction logic and skipping logic is duplicated in every type.

While I'm here, use a binary search to look up the flag, since we have
rather a lot of them. With more C++ template tricks, we could avoid the
std::function, but that seemed more trouble than was worth it,
especially since, prior to C++17, it's a little hard to convince
template argument deduction to infer one of the parameters.

Change-Id: I208f89d46371b31fc8b44487725296bcd9d7c8e7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50769

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Change-Id: Id7ef442d23d36c3d28fb52cb5d813238ff7e963a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50768

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

This fix isn't ideal, given the current space of possible version
values. But rather than make the printing code complicated, we should
make invalid versions impossible. I've left TODOs where that would be
needed.

Bug: 467, 450
Change-Id: I6c9ae97b8454182b0c1ab6ba2e070dc6d7d8b3f4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50767

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

X509_print_ex tries to print negative serial numbers in decimal. In
doing so, it ends up passing a signed long to %lx and trips
-Wformat-signed.

A minimal fix would be to cast to unsigned long, but this unsigned long
is the absolute value of a signed long (l = -l). This is tricky because
-LONG_MIN does not fit in long. It all works because the length check
only allows one bit short of sizeof(long)*8 bits (ASN1_INTEGER is
sign-and-magnitude).

Still, this is a whole lot of subtlety to account for an invalid case.
Instead, send negative serial numbers down the generic path.

Bug: 450
Change-Id: Ib215fd23863de27e01f7ededf95578f9c800da37
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50766

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

GCC has a warning that complains about even more type mismatches in
printf. Some of these are a bit messy and will be fixed in separate CLs.
This covers the easy ones.

The .*s stuff is unfortunate, but printf has no size_t-clean string
printer. ALPN protocol lengths are bound by uint8_t, so it doesn't
really matter.

The IPv6 printing one is obnoxious and arguably a false positive. It's
really a C language flaw: all types smaller than int get converted to
int when you do arithmetic. So something like this first doesn't
overflow the shift because it computes over int, but then the result
overall is stored as an int.

  uint8_t a, b;
  (a << 8) | b

On the one hand, this fixes a "missing" cast to uint16_t before the
shift. At the same time, the incorrect final type means passing it to
%x, which expects unsigned int. The compiler has forgotten this value
actually fits in uint16_t and flags a warning. Mitigate this by storing
in a uint16_t first.

The story doesn't quite end here. Arguments passed to variadic functions
go through integer promotion[0], so the argument is still passed to
snprintf as an int! But then va_arg allows for a signedness mismatch[1],
provided the value is representable in both types. The combination means
that %x, though actually paired with unsigned, also accept uint8_t and
uint16_t, because those are guaranteed to promote to an int that meets
[1]. GCC recognizes [1] applies here.

(There's also PRI16x, but that's a bit tedious to use and, in glibc, is
defined as plain "x" anyway.)

[0] https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions
[1] https://en.cppreference.com/w/c/variadic/va_arg

Bug: 450
Change-Id: Ic1d41356755a18ab922956dd2e07b560470341f4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50765

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>

Change-Id: Ia5db220d13cf42fac6958a2c7416743ca2991479
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50745

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Update-Note: ".example.com" as an input DNS name will no longer match
"www.example.com" in a certificate. (Note this does not impact wildcard
certificates. Rather, it removes a non-standard "reverse wildcard" that
OpenSSL implemented.)

Fixed: 463
Change-Id: I627e1bd00b8e4b810e9bb756f424f6230a99496e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50726

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

This replaces v3name_test.cc which is rather difficult to follow.
v3name_test.cc ran all pairs of names against each other, used a
default case-insensitivity rule, and then had a list of string
exceptions to that rule. This is hopefully easier for us to adjust
later. It also fixes a testing bug we wouldn't notice if an expected
"exception" didn't fire.

Sadly, we cannot use designated initializers in C++ yet. MSVC does not
support them until MSVC 2019.

Change-Id: Ia8e3bf5f57d33a9bf1fc929ba1e8cd2a270a8a24
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50725

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>

Always enable X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS and never enable
X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS.

Update-Note: BoringSSL will no longer accept wildcard patterns like
*www.example.com or www*.example.com. (It already did not accept
ww*w.example.com.) X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS will also be
ignored and can no longer be used to allow foo.bar.example.com to match
*.example.com.

Fixes: 462
Change-Id: I004e087bf70f4c3f249235cd864d9e19cc9a5102
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50705

Reviewed-by: Adam Langley <agl@google.com>

I believe, with this, we have aligned with OpenSSL 1.1.x on the
crypto/x509 and crypto/asn1 types that are now opaque. Strangely,
OpenSSL kept X509_ALGOR public. We may wish to hide that one too later,
but we can leave it for now.

Update-Note: Use X509_REVOKED accessors rather than reaching into the
struct.

Bug: 425
Change-Id: Ib47944648a8693ed7078ffe94f7b557022debe30
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50685

Reviewed-by: Adam Langley <agl@google.com>

Change-Id: I9ba12ad7b3cfc9a6d1015da728cec45e4b71dcc9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50665


Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

This allows code that uses OpenSSL's suggested pattern for 1.1.0 [1] to
work.

[1] https://wiki.openssl.org/index.php/Hostname_validation

Change-Id: I6d1b983074d5ad8645400cef887c1cc20f7bf2a1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50645

Reviewed-by: David Benjamin <davidben@google.com>

rust-openssl, rather than using Rust's existing error types, exposes the
OpenSSL error queue as the error type in its public callback types.
Supporting a simplified version of ERR_set_error_data is simple enough,
so it's easiest just to add this function.

Unlike OpenSSL's, we don't attempt to support non-string error data. We
also don't try to retain borrowed pointers. If the caller did not pass
ownership, make a copy internally.

Change-Id: I909eebc2867ab1f3b9975546a106ee1f762bf516
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50625

Reviewed-by: Adam Langley <agl@google.com>

OpenSSL 1.1.0 made this structure opaque. I don't think we particularly
need to make it opaque, but external code uses it. Also add
RSA_test_flags.

Change-Id: I136d38e72ec4664c78f4d1720ec691f5760090c1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50605

Reviewed-by: Adam Langley <agl@google.com>

This is filled in by X509_CRL_sort but never used.

Change-Id: Ifacd56fcc090386faa6c11b73b44191a31221ef8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50545

Reviewed-by: Adam Langley <agl@google.com>

Change-Id: If115d3ab25e2e538447a130f469debe25a92ce5c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50525


Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

rust-openssl uses this constant to determine PKCS12_create's defaults.
Define it so it does not need to be redefined in
https://boringssl-review.googlesource.com/c/boringssl/+/49645

Change-Id: Ife4a6597de430a411932096618406b9fcd721f43
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50505

Reviewed-by: Adam Langley <agl@google.com>

This was added in OpenSSL 1.1.x. It is slightly different from
SSL_pending in that it also reports buffered transport data.

Change-Id: I81e217aad1ceb6f4c31c36634a546e12b6dc8dfc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50445


Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

HPKE draft-12 has no changes from draft-08 except that the test vectors
were refreshed and some fields in the JSON file renamed. Also fix the
test vector reference to point to copy from the spec rather than the
(identical) copy from the reference implementation.

Change-Id: Icd4fd467672cc8701fcd2b262ac90c5adc05ac39
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50465

Reviewed-by: Adam Langley <agl@google.com>

The non-_ex EVP_CIPHER_CTX Final functions are a bit interesting. Unlike
EVP_DigestFinal(_ex), where the non-_ex version calls EVP_MD_CTX_cleanup
for you, the EVP_CIPHER_CTX ones do not automatically cleanup.
EVP_CipherFinal and EVP_CipherFinal_ex are identical in all releases
where they exist.

This appears to date to OpenSSL 0.9.7:

Prior to OpenSSL 0.9.7, EVP_MD_CTX and EVP_CIPHER_CTX did not use void*
data fields. Instead, they just had a union of context structures for
every algorithm OpenSSL implemented.

EVP_MD_CTX was truly cleanup-less. There were no EVP_MD_CTX_init or
EVP_MD_CTX_cleanup functions at all. EVP_DigestInit filled things in
without reference to the previous state. EVP_DigestFinal didn't cleanup
because there was nothing to cleanup.

EVP_CIPHER_CTX was also a union, but for some reason did include
EVP_CIPHER_CTX_init and EVP_CIPHER_CTX_cleanup. EVP_CIPHER_CTX_init
seemed to be optional: EVP_CipherInit with non-NULL EVP_CIPHER similarly
didn't reference the previous state. EVP_CipherFinal did not call
EVP_CIPHER_CTX_cleanup, but EVP_CIPHER_CTX_cleanup didn't do anything.
It called an optional cleanup hook on the EVP_CIPHER, but as far as I
can tell, no EVP_CIPHER implemented it.

Then OpenSSL 0.9.7 introduced ENGINE. The union didn't work anymore, so
EVP_MD_CTX and EVP_CIPHER_CTX contained void* with allocated
type-specific data. The introduced EVP_MD_CTX_init and
EVP_MD_CTX_cleanup. For (imperfect!) backwards compatibility,
EVP_DigestInit and EVP_DigestFinal transparently called init/cleanup for
you. EVP_DigestInit_ex and EVP_DigestFinal_ex became the more flexible
versions that left init/cleanup to the caller.

EVP_CIPHER_CTX got the same treatment with
EVP_CipherInit/EVP_CipherInit_ex, but *not*
EVP_CipherFinal/EVP_CipherFinal_ex. The latter did the same thing. The
history seems to be that 581f1c84940d77451c2592e9fa470893f6c3c3eb
introduced the Final/Final_ex split, with the former doing an
auto-cleanup, then 544a2aea4ba1fad76f0802fb70d92a5a8e6ad85a undid it.
Looks like the motivation is that EVP_CIPHER_CTX objects are often
reused to do multiple operations with a single key. But they missed that
the split functions are now unnecessary.

Amusingly, OpenSSL's documentation incorrectly said that EVP_CipherFinal
cleaned up after the call until it was fixed in
538860a3ce0b9fd142a7f1a62e597cccb74475d3. The fix says that some
releases cleaned up, but there were, as far as I can tell, no actual
releases with that behavior.

I've put the new Final functions in the deprecated section, purely
because there is no sense in recommending two different versions of the
same function to users, and Final_ex seems to be more popular. But there
isn't actually anything wrong with plain Final.

Change-Id: Ic2bfda48fdcf30f292141add8c5f745348036852
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50485

Reviewed-by: Adam Langley <agl@google.com>

Change-Id: I12ab8e9209bd5fdff75c42332d4d35a461302b61
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50425

Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Testing the Python 3 recipe migration.

Change-Id: I428f08eaf426cf0bbd4b53f9f6932df7d15ad2ee
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50326

Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

As of https://boringssl-review.googlesource.com/26968, digest_to_scalar
should output a fully-reduced value.

Change-Id: I9fccc62413b17184eb3aa6fa5cd87d7e7849e2eb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50325

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

Change-Id: I4af18ce3de2a01a8a5184096b07354bcbd24caf1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50265

Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

d2i_ASN1_OBJECT had a similar set of bugs in as in
https://boringssl-review.googlesource.com/c/boringssl/+/49866.

This does not affect any other d2i functions. Those already go through
the ASN1_ITEM machinery.

Update-Note: d2i_ASN1_OBJECT will now notice more incorrect tags. It was
already checking for tag number 6, so it is unlikely anyone was relying
on this as a non-tag-checking parser.

Change-Id: I30f9ad28e3859aeb7a38c0ea299cd2e30002abce
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50290

Reviewed-by: Adam Langley <agl@google.com>

Update-Note: PKCS#7 and PKCS#12 parsers will now reject BER constructed
BIT STRINGs. We were previously misparsing them, as was OpenSSL. Given
how long the incorrect parse has been out there, without anyone noticing
(other parsers handle it correctly), it is unlikely these exist.

Change-Id: I61d317461cc59480dc9f772f88edc7758206d20d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50289

Reviewed-by: Adam Langley <agl@google.com>

DER requires BIT STRING padding bits be zero.

Bug: 354
Change-Id: Id59154cc4e77f91df8b9ff1eb1b09514116808da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50288

Reviewed-by: Adam Langley <agl@google.com>

This simplifies the ASN1_get_object calling convention and removes
another significant source of tasn_dec.c complexity. This change does
not affect our PKCS#7 and PKCS#12 parsers.

Update-Note: Invalid certificates (and the few external structures using
asn1t.h) with BER indefinite lengths will now be rejected.

Bug: 354
Change-Id: I723036798fc3254d0a289c77b105fcbdcda309b2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50287

Reviewed-by: Adam Langley <agl@google.com>

Constructed strings are a BER mechanism where a string type can be
represented as a tree of constructed nodes and primitive leaves, that
then have to be concatenated by the parser. This is prohibited in DER
and a significant source of complexity in our parser.

Note this change does not affect our PKCS#7 and PKCS#12 parsers (where
BER is sadly necessary for interop) because those use CBS.

Update-Note: Invalid certificates (and the few external structures using
asn1t.h) with BER constructed strings will now be rejected.

Bug: 354
Change-Id: I5a8ee028ec89ed4f2d5c099a0588f2029b864580
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50286

Reviewed-by: Adam Langley <agl@google.com>

X509V3_EXT_d2i should notice if an extension has extra data at the end.

Update-Note: Some previously accepted invalid certicates may be
rejected, either in certificate verification or in X509_get_ext_d2i.

Bug: 352
Change-Id: Iacbb74a52d15bf3318b4cb8271d44b0f0a2df137
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50285

Reviewed-by: Adam Langley <agl@google.com>

We haven't done this in a while. This also tests more codepaths in
in the previous Python 3 update.

libc++ required a few more build tweaks. Also the CMake update was
necessary to update the NDK. Older CMake cannot detect CMAKE_LINKER
in the newer NDK.

Change-Id: I59ab1c6b074b805dd4b8a6ab596c4cf469d5bfa9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50167


Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

pcy_int.h was especially weird because it is even missing include guards
and its dependencies.

Change-Id: Idccfe23b74b47641bcfc802f78f3ee6fe479b781
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50245


Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>

Although the compiler will hopefully optimize it out, this is
technically a VLA. The new Android NDK now warns about this.

Change-Id: Ib9f38dc73c40e90ab61105f29a635c453f1477a1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50185


Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>