There are a lot of d2i and i2d functions, and there will be even more
once asn1.h and x509.h are properly documented. We currently replicate
the text in each, but as a result a miss a few points:
- The i2d outp != NULL, *outp == NULL case isn't documented at all.
- We should call out what to do with *inp after d2i.
- Unlike our rewritten functions, object reuse is still quite rampant
with the asn1.h functions. I hope we can get rid of that but, until we
can, it would be nice to describe it in one place.
While I'm here, update a few references to the latest PKCS#1 RFC, and
try to align how we reference ASN.1 structures a bit. The d2i/i2d
functions say "ASN.1, DER-encoded RSA private key" while the CBS/CBB
functions say "DER-encoded RSAPrivateKey structure".
Bug: 426
Change-Id: I8d9a7b0aef3d6d9c8240136053c3b1704b09fd41
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49906
Reviewed-by: Adam Langley <agl@google.com>
d2i_ASN1_BOOLEAN and i2d_ASN1_BOOLEAN don't go through the macros
because ASN1_BOOLEAN is a slightly weird type (int instead of pointer).
Their tag checks were missing a few bits.
This does not affect any other d2i functions. Those already go through
the ASN1_ITEM machinery.
Change-Id: Ic892cd2a8b8f9ceb11e43d931f8aa6df921997d3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49866
Reviewed-by: Adam Langley <agl@google.com>
This makes it slightly clearer which ints are lengths and which are
substituting for T*. (ASN1_BOOLEAN is weird. It is the one non-pointer
representation in crypto/asn1.)
Change-Id: I93ff87264835e64c9f8613edae63e93731e77548
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49865
Reviewed-by: Adam Langley <agl@google.com>
crypto/asn1 represents an ASN.1 NULL value as a non-null ASN1_NULL*
pointer, (ASN1_NULL*)1. It is a non-null pointer because a null pointer
represents an omitted OPTIONAL NULL. It is an opaque pointer because
there is no sense in allocating anything.
This pointer cannot be dereferenced, yet ASN1_NULL is a typedef for int.
This is confusing and probably undefined behavior. (N1548, 6.3.2.3,
clause 7 requires pointer conversions between two pointer types be
correctly aligned, even if the pointer is never dereferenced. Strangely,
clause 5 above does not impose the same requirement when converting from
integer to pointer, though it mostly punts to the implementation
definition.) Of course, all of tasn_*.c is a giant strict aliasing
violation anyway, but an opaque struct pointer is a slightly better
choice here.
(Note that, although ASN1_BOOLEAN is also a typedef for int, that
situation is different: the ASN1_BOOLEAN representation is a plain
ASN1_BOOLEAN, not ASN1_BOOLEAN*, while the ASN1_NULL representation is a
pointer. ASN1_NULL could have had the same treatment and even used a
little less memory, but changing that would break the API.)
Update-Note: Code that was assuming ASN1_NULL was an int typedef will
fail to compile. Given this was never dereferencable, it is hard to
imagine anything relying on this.
Bug: 438
Change-Id: Ia0c652eed66e76f82a3843af1fc877f06c8d5e8f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49805
Reviewed-by: Adam Langley <agl@google.com>
The two headers already circularly import each other, and even have to
inspect each others' header guards to manage this. Keeping them
separate does not reduce include sizes. Fold them together so their
header guards are more conventional.
Bug: 426
Change-Id: Iaf96f5b2c8adb899d9c4a5b5094ed36fcb16de16
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49770
Reviewed-by: Adam Langley <agl@google.com>
This function is a little awkward. It mutates global data, so if two
libraries in the address space both attempt to define a custom OID, they
will conflict. But some existing code uses it so, as long as it does so,
we should make it thread-safe.
Along the way, I've switched it to a hash table and removed the ability
to overwrite existing entries. Previously, overwriting a built-in table
would crash (on platforms where const structures are write-protected).
Overwriting a dynamic table implemented this weird merging algorithm.
The one caller I've seen does not appear to need this feature.
I've also switched ASN1_STRING_TABLE_cleanup to a no-op, matching our
other global cleanup functions. This function is not safe to call
without global knowledge of all other uses of the library.
Update-Note: ASN1_STRING_TABLE_add no longer allows overwrite existing
entries. In most cases, this would crash or trigger a race condition
anyway.
Bug: 426
Change-Id: Ie024cca87feaef3ff10064b452f3a860844544da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49769
Reviewed-by: Adam Langley <agl@google.com>
Callers may as well use ASN1_mbstring_ncopy directly, but some code uses
this, so test it. I've intentionally not tested updating entries because
it crashes if you use a built-in one, and updating a dynamic one seems
unnecessary.
Change-Id: If760a751fbdcd1a2f14d5dcb08de2b0f2a8d3549
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49768
Reviewed-by: Adam Langley <agl@google.com>
There's a test in the file under ifdef, but that is not wired up into
the build.
Change-Id: Iec09277c7ce948c33303d12c325207de2188d908
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49766
Reviewed-by: Adam Langley <agl@google.com>
We have a ton of per-file rotation functions, often with generic names
that do not tell you whether they are uint32_t vs uint64_t, or rotl vs
rotr.
Additionally, (x >> r) | (x << (32 - r)) is UB at r = 0.
(x >> r) | (x << ((-r) & 31)) works for 0 <= r < 32, which is what
cast.c does. GCC and Clang recognize this pattern as a rotate, but MSVC
doesn't. MSVC does, however, provide functions for this.
We usually rotate by a non-zero constant, which makes this moot, but
rotation comes up often enough that it's worth extracting out. Some
particular changes to call out:
- I've switched sha256.c from rotl to rotr. There was a comment
explaining why it differed from the specification. Now that we have
both functions, it's simpler to just match the specification.
- I've dropped all the inline assembly from sha512.c. Compilers should
be able to recognize rotations in 2021.
Change-Id: Ia1030e8bfe94dad92514ed1c28777447c48b82f9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49765
Reviewed-by: Adam Langley <agl@google.com>
This was added in
https://boringssl-review.googlesource.com/c/boringssl/+/12980/, but does
not appear to be used anymore. The corresponding function does not exist
in OpenSSL.
This simplifies the tests slightly, some of which were inadvertently
specifying the boolean and some weren't.
Change-Id: I9b956dcd9f7151910f93f377d207c88273bd9ccf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49747
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
The OpenSSL X.509 verifier lacks a proper path builder. When there are
two paths available for a certificate, we pick one without looking at
expiry, etc.
In scenarios like one below, X509_V_FLAG_TRUSTED_FIRST will prefer
Leaf -> Intermediate -> Root1. Otherwise, we will prefer
Leaf -> Intermediate -> Root1Cross -> Root2:
Root2
|
Root1 Root1Cross
\ /
Intermediate
|
Leaf
If Root2 is expired, as with Let's Encrypt, X509_V_FLAG_TRUSTED_FIRST
will find the path we want. Same if Root1Cross is expired. (Meanwhile,
if Root1 is expired, TRUSTED_FIRST will break and leaving it off works.
TRUSTED_FIRST does not actually select chains with validity in mind. It
just changes the semi-arbitrary decision.)
OpenSSL 1.1.x now defaults to X509_V_FLAG_TRUSTED_FIRST by default, so
match them. Hopefully the shorter chain is more likely to be correct.
Update-Note: X509_verify_cert will now build slightly different chains
by default. Hopefully, this fixes more issues than it causes, but there
is a risk of trusted_first breaking other scenarios. Those scenarios
will also break OpenSSL 1.1.x defaults, so hopefully this is fine.
Bug: 439
Change-Id: Ie624f1f7e85a9e8c283f1caf24729aef9206ea16
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49746
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
There are two ways to configure an X509_STORE_CTX after
X509_STORE_CTX_init. One can either modify the already initialized
X509_VERIFY_PARAM or replace it. Modifying the existing one is more
common. Replacing it actually misses some defaults. (See issue #441 for
details.)
In preparation for actually being able to test changes to the default,
switch tests to that model. In doing so, no longer need to explicitly
configure the depth and can test that default. (Though we should write
tests for the depth at some point.)
Bug: 439, 441
Change-Id: I254a82585d70d44eb94920f604891ebfbff4af4c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49745
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
All the test vectors testing key length greater than the block length
were mislabelled as key length being equal to the block length. Add a
note to these test vectors indicating they are directly from the NIST
tests with the misleading input intact.
Change-Id: I9fe87971265ad48e9b835fccbe92306e1670b4d6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49705
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
In upstream, these functions take file and line number arguments. Update
ours to match. Guessing almost no one uses these, or we'd have caught
this earlier.
Change-Id: Ic09f8d8274065ac02efa78e70c215b87fa765b9f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49665
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Benjamin Brittain <bwb@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Node seems uncommonly sensitive to this, so let's write these functions
in a way that stays in sync and test this. See also
https://boringssl-review.googlesource.com/c/boringssl/+/49585
This does incur a cost across all BoringSSL consumers that use these
functions: as a result of Node indiscriminately exposing every cipher,
we end up pulling more and more ciphers into these getters. But that
ship sailed long ago, so, instead, document that EVP_get_cipherby*
should not be used by size-conscious callers.
EVP_get_digestby* probably should have the same warning, but I've left
it alone for now because we don't quite have the same proliferation of
digests as ciphers. (Though there are things in there, like MD4, that
ought to be better disconnected.)
Change-Id: I61ca406c146279bd05a52bed6c57200d1619c5da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
There is an obvious bug there: upon entry to 'vpaes_cbc_encrypt'
LR may get signed. However, on the 'cbc_abort' path the LR is
not going to be unsigned before 'ret' is executed.
Found by manual code inspection.
Co-authored-by: Russ Butler <russ.butler@arm.com>
Change-Id: I646cdfaee28db59aafbbd412d4bb6ba022eff15b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49605
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Linux module signing uses PKCS#7 / CMS because everything is awful and
broken. In order to make the lives of kernel developers easier, support
the calling pattern that the kernel uses to sign modules.
The kernel utility was written at a time when PKCS#7 was hard coded to
use SHA-1 for signing in OpenSSL and it reflects this: you can only
specify “sha1” on the command line, for example. As of OpenSSL 1.1.1, at
least, OpenSSL uses SHA-256 and thus so does this change.
Change-Id: I32b036123a0d8b272ec9e1c0130c45bf3ed0d2c7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49545
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
I was inspired to look at this again recently and noticed we could do a
bit better. Instead of a tower of selects, rely on all the cases being
mutually exclusive and use the ret |= mask & value formulation without
loss in clarity. We do need to fixup the invalid case slightly, but
since that computation is mostly independent, I'm guessing the CPU and
compiler are able to schedule it effectively.
Before:
Did 251000 base64 decode operations in 2002569us (159.4 MB/sec)
After:
Did 346000 base64 decode operations in 2005426us (219.5 MB/sec) [+37.7%]
Change-Id: I542167202fd4e94c93dd5a2519a97bc388072c89
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49525
Reviewed-by: Adam Langley <agl@google.com>
The i2d functions internally take a tag/class pair of parameters. If tag
is not -1, we override the tag with (tag, class). Otherwise, class is
ignored. (class is inconsistently called aclass or iclass.)
Historically, the remaning bits of class were repurposed to pass extra
flags down the structure. These had to be preserved in all recursive
calls, so the functions take apart and reassemble the two halves of
aclass/iclass. The only such flag was ASN1_TFLG_NDEF, which on certain
types, caused OpenSSL to encode indefinite-length encoding. We removed
this in https://boringssl-review.googlesource.com/c/boringssl/+/43889.
Due to these flags, if tag == -1, class should default to zero. However,
X509_NAME's callbacks pass -1, -1, instead of -1, 0, effectively setting
all flags. This wasn't noticed because none of the types below X509_NAME
pay attention to ASN1_TFLG_NDEF.
This CL does two things: First, it unwinds the remainder of the flags
machinery. If we ever need flags, we should pass it as a distinct
argument. Second, it fixes the X509_NAME calls and asserts that -1 is
always paired with 0.
Change-Id: I285a73a06ad16980617fe23d5ea7f260fc5dbf16
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49385
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
These are a little odd with the ASN1_ENCODING paths. And there were some
bugs previously around CHOICE types. Nothing defines them, inside or
outside BoringSSL, so remove them.
Change-Id: Id2954fef8ee9637f36f7511b51dc0adc2557e3ba
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49352
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
it->funcs is only an ASN1_AUX for ASN1_ITYPE_SEQUENCE and
ASN1_ITYPE_CHOICE. Fortunately, the other possible types for it->funcs
are larger than ASN1_AUX and we don't touch the result when we
shouldn't, so this is merely a strict aliasing violation.
Change-Id: I29e94249e0b137fe8df0b16254366ae6705c8784
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49351
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
See also 006906cddda37e24a66443199444ef4476697477 from OpenSSL, though
this CL uses a different strategy from upstream. Upstream makes
ASN1_item_ex_i2d continue to allow optionals and checks afterwards at
every non-optional call site. This CL pushes down an optional parameter
and says functions cannot omit items unless explicitly allowed.
I think this is a better default, though it is a larger change. Fields
are only optional when they come from an ASN1_TEMPLATE with the OPTIONAL
flag. Upstream's strategy misses top-level calls.
This CL additionally adds checks for optional ASN1_TEMPLATEs in contexts
where it doesn't make sense. Only fields of SEQUENCEs and SETs may be
OPTIONAL, but the ASN1_ITEM/ASN1_TEMPLATE split doesn't quite match
ASN.1 itself. ASN1_TEMPLATE is additionally responsible for
explicit/implicit tagging, and SEQUENCE/SET OF. That means CHOICE arms
and the occasional top-level type (ASN1_ITEM_TEMPLATE) use ASN1_TEMPLATE
but will get confused if marked optional.
As part of this, i2d_FOO(NULL) now returns -1 rather than "successfully"
writing 0 bytes. If we want to allow NULL at the top-level, that's not
too hard to arrange, but our CBB-based i2d functions do not.
Update-Note: Structures with missing mandatory fields can no longer be
encoded. Note that, apart from the cases already handled by preceding
CLs, tasn_new.c will fill in non-NULL empty objects everywhere. The main
downstream impact I've seen of this particular change is in combination
with other bugs. Consider a caller that does:
GENERAL_NAME *name = GENERAL_NAME_new();
name->type = GEN_DNS;
name->d.dNSName = DoSomethingComplicated(...);
Suppose DoSomethingComplicated() was actually fallible and returned
NULL, but the caller forgot to check. They'd now construct a
GENERAL_NAME with a missing field. Previously, this would silently
serialize some garbage (omitted field) or empty string. Now we fail to
encode, but the true error was the uncaught DoSomethingComplicated()
failure. (Which likely was itself a bug.)
Bug: 429
Change-Id: I37fe618761be64a619be9fdc8d416f24ecbb8c46
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49350
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
See https://github.com/openssl/openssl/issues/16538
Update-Note: A default-constructed object with a required ANY or
string-like CHOICE field cannot be encoded until the field is specified.
Note this affects i2d_X509: notBefore and notAfter are string-like
CHOICEs in OpenSSL.
Bug: 429
Change-Id: I97d971fa588ab72be25a4c1eb7310ed330f16c4f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49349
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
asn1_ex_i2c actually does have an error condition, it just wasn't being
handled.
628b3c7f2f, imported from upstream's
f3f8e72f494b36d05e0d04fe418f92b692fbb261, tried to check for OID-less
ASN1_OBJECTs and return an error. But it and the upstream change didn't
actually work. -1 in this function means to omit the object, so OpenSSL
was silently misinterpreting the input structure.
This changes the calling convention for asn1_ex_i2c to support this. It
is, unfortunately, a little messy because:
1. One cannot check for object presense without walking the
ASN1_ITEM/ASN1_TEMPLATE structures. You can *almost* check if *pval
is NULL, but ASN1_BOOLEAN is an int with -1 to indicate an omitted
optional. There are also FBOOLEAN/TBOOLEAN types that omit FALSE/TRUE
for DEFAULT. Thus, without more invasive changes, asn1_ex_i2c must be
able to report an omitted element.
2. While the i2d functions report an omitted element by successfully
writing zero bytes, i2c only writes the contents. It thus must
distinguish between an omitted element and an element with
zero-length contents.
3. i2c_ASN1_INTEGER and i2c_ASN1_BIT_STRING return zero on error rather
than -1. Those error paths are not actually reachable because they
only check for NULL. In fact, OpenSSL has even unexported them. But I
found a few callers. Rather than unwind all this and change the
calling convention, I've just made it handle 0 and map to -1 for now.
It's all a no-op anyway, and hopefully we can redo all this with CBB
later.
I've just added an output parameter for now.
In writing tests, I also noticed that the hand-written i2d_ASN1_OBJECT
and i2d_ASN1_BOOLEAN return the wrong value for errors, so I've fixed
that.
Update-Note: A default-constructed object with a required ASN1_OBJECT
field can no longer be encoded without initializing the ASN1_OBJECT.
Note this affects X509: the signature algorithm is an ASN1_OBJECT. Tests
that try to serialize an X509_new() must fill in all required fields.
(Production code is unlikely to be affected because the output was
unparsable anyway, while tests sometimes wouldn't notice.)
Bug: 429
Change-Id: I04417f5ad6b994cc5ccca540c8a7714b9b3af33d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49348
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This handles normal CHOICE types. A follow-up CL will handle MSTRING and
ANY types.
Update-Note: An invalid CHOICE object (e.g. GENERAL_NAME) will now fail
when encoded, rather than be silently omitted. In particular, CHOICE
objects are default-initialized by tasn_new.c in an empty -1 state.
Structures containing a required CHOICE field can no longer be encoded
without filling in the CHOICE.
Bug: 429
Change-Id: I7011deadf518ddc344a56b07a0e268ceaae17fe0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49347
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This function forgot to handle errors in ASN1_item_ex_i2d. It also
checked x509_name_canon for ret < 0, when x509_name_canon returns a
boolean. For consistency, I've switched to x509_name_encode to return a
boolean as well. It doesn't actually need to return a length because
it's responsible for filling in a->bytes.
(This is also far from thread-safe, but I'll figure out what to do there
separately.)
Bug: 429
Change-Id: I1dddeab320018be4b837f95001cbeeba4e25f0a1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49346
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
tasn_enc.c was missing lots of error checks and mixed up 0 and -1
returns. Document all the internal calling conventions, as best as I can
tell, and fix things up.
There are also error cases it forgets to check (it generally does not
notice missing non-OPTIONAL fields). This CL only addresses errors it
already tries to report. Subsequent CLs will add in the missing error
cases. And then if it all sticks, I'm hoping we can rewrite this with
CBB. Rewriting tsan_dec.c to CBS would also be good, but that will be
more difficult as we need to clear out BER first.
Update-Note: Some error cases which were silently misinterpreted as
missing OPTIONAL elements will now cause encoding to fail.
Bug: 429
Change-Id: Ibbb3eba08eb8f8f878930c9456edc8c74479aade
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49345
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
GCC 11.2.1 reportedly warns that CTR_DRBG_init may be passed an
uninitialized personalization buffer. This appears to be a false
positive, because personalization_len will be zero. But it's easy enough
to zero-initialize it, so silence the warning.
Bug: 432
Change-Id: I20f6b74e09f19962e8cae37d45090ff3d1c0215d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49245
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Later CLs will clean up the ClientHello construction a bit (draft-12
avoids computing ClientHelloOuter twice). I suspect the transcript
handling on the client can also be simpler, but I'll see what's
convenient after I've changed how ClientHelloOuter is constructed.
Changes of note between draft-10 and draft-13:
- There is now an ECH confirmation signal in both HRR and SH. We don't
actually make much use of this in our client right now, but it
resolves a bunch of weird issues around HRR, including edge cases if
HRR applies to one ClientHello but not the other.
- The confirmation signal no longer depends on key_share and PSK, so we
don't have to work around a weird ordering issue.
- ech_is_inner is now folded into the main encrypted_client_hello code
point. This works better with some stuff around HRR.
- Padding is moved from the padding extension, computed with
ClientHelloInner, to something we fill in afterwards. This makes it
easier to pad up the whole thing to a multiple of 32. I've accordingly
updated to the latest recommended padding construction, and updated
the GREASE logic to match.
- ech_outer_extensions is much easier to process because the order is
required to be consistent. We were doing that anyway, and now a simple
linear scan works.
- ClientHelloOuterAAD now uses an all zero placeholder payload of the
same length. This lets us simplify the server code, but, for now, I've
kept the client code the same. I'll follow this up with a CL to avoid
computing ClientHelloOuter twice.
- ClientHelloOuterAAD is allowed to contain a placeholder PSK. I haven't
filled that in and will do it in a follow-up CL.
Bug: 275
Change-Id: I7464345125c53968b2fe692f9268e392120fc2eb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48912
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This unexports X509, X509_CINF, X509_NAME_ENTRY, X509_NAME, X509_OBJECT,
X509_LOOKUP_METHOD, X509_STORE, X509_LOOKUP, and X509_STORE_CTX.
Note this means X509_STORE_CTX can no longer be stack-allocated.
Update-Note: Patch cl/390055173 into the roll that includes this. This
unexports most of the X.509 structs, aligning with OpenSSL. Use the
accessor APIs instead.
Bug: 425
Change-Id: I53e915bfae3b8dc4b67642279d0e54dc606f2297
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48985
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
V_ASN1_APP_CHOOSE has been discouraged by OpenSSL since 2000:
https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=CHANGES;h=824f421b8d331ba2a2009dbda333a57493bedb1e;hb=fb047ebc87b18bdc4cf9ddee9ee1f5ed93e56aff#l10848
Instead, upstream recommends an MBSTRING_* constant.
https://www.openssl.org/docs/man1.1.1/man3/X509_NAME_add_entry_by_NID.html
This function is a bit overloaded:
MBSTRING_* means "Decode my input from this format and then re-encode it
using whatever string type best suits the NID (usually UTF8String, but
some NIDs require PrintableString)".
V_ASN1_APP_CHOOSE means "This is a Latin-1 string. Without looking at
the NID, pick one of PrintableString, IA5String, or T61String".
The latter is almost certainly not what callers want. If they want a
particular type, they can always force it by passing a particular
V_ASN1_* constant. This removes the only use of ASN1_PRINTABLE_type
within the library, though there is one external use still.
Update-Note: V_ASN1_APP_CHOOSE is removed. I only found one use, which
has been fixed.
Change-Id: Id36376dd0ec68559bbbb366e2305d42be5ddac67
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49067
Reviewed-by: Adam Langley <agl@google.com>
The old loop read one byte past the length. It also stopped the loop
too early on interior NUL. See also upstream's
https://github.com/openssl/openssl/pull/16433, though I've opted to
rewrite the function entirely rather than use their fix.
Also deduplicate the PrintableString check.
Change-Id: Ia8bd282047c2a2ed1d5e71a68a3947c7c108df95
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49066
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
strchr is interprets the trailing NUL as part of the string, so
is_printable thought NUL was allowed. Just write the code in the obvious
way and let the compiler figure it out. (It seems to make a clever
bitmask or something.)
Update-Note: ASN1_mbstring_ncopy will no longer allow PrintableString
for strings containing NUL.
Change-Id: I3675191ceb44c06f0ac7b430f88272cabf392d35
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49065
Reviewed-by: Adam Langley <agl@google.com>
The bulk of RSA_check_key is spent in bn_div_consttime, which is a naive
but constant-time long-division algorithm for the few places that divide
by a secret even divisor: RSA keygen and RSA import. RSA import is
somewhat performance-sensitive, so pick some low-hanging fruit:
The main observation is that, in all but one call site, the bit width of
the divisor is public. That means, for an N-bit divisor, we can skip the
first N-1 iterations of long division because an N-1-bit remainder
cannot exceed the N-bit divisor.
One minor nuisance is bn_lcm_consttime, used in RSA keygen has a case
that does *not* have a public bit width. Apply the optimization there
would leak information. I've implemented this as an optional public
lower bound on num_bits(divisor), which all but that call fills in.
Before:
Did 5060 RSA 2048 private key parse operations in 1058526us (4780.2 ops/sec)
Did 1551 RSA 4096 private key parse operations in 1082343us (1433.0 ops/sec)
After:
Did 11532 RSA 2048 private key parse operations in 1084145us (10637.0 ops/sec) [+122.5%]
Did 3542 RSA 4096 private key parse operations in 1036374us (3417.7 ops/sec) [+138.5%]
Bug: b/192484677
Change-Id: I893ebb8886aeb8200a1a365673b56c49774221a2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49106
Reviewed-by: Adam Langley <agl@google.com>
See upstream commits:
32f3b98d1302d4c0950dc1bf94b50269b6edbd95
432f8688bb72e21939845ac7a69359ca718c6676
7bb50cbc4af78a0c8d36fdf2c141ad1330125e2f
8c74c9d1ade0fbdab5b815ddb747351b8b839641
Change-Id: Iff614260c1b1582856edb4ae7a226f2e07537698
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49045
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Subsequent CLs will add some fuzzers, etc., that'll help with catching
this.
Change-Id: I10a8e4b2f23ffd07b124e725c1f7454e7ea6f2dd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49025
Reviewed-by: Adam Langley <agl@google.com>
See also 8393de42498f8be75cf0353f5c9f906a43a748d2 from upstream and
CBS-2021-3712. But rather than do that, I've rewritten it with CBS, so
it's a bit clearer. The previous commit added tests.
Change-Id: Ie52e28f07b9bf805c8730eab7be5d40cb5d558b6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49008
Reviewed-by: Adam Langley <agl@google.com>
See also 174ba8048a7f2f5e1fca31cfb93b1730d9db8300 from upstream. This
differs from the upstream CL in that:
- We don't silently drop trailing NULs.
- As a NUL-terminated C string, the empty string is a non-NULL pointer
to an array containing a zero byte. Use the latter consistently.
Change-Id: I99c6c4c26be5a1771c56c6ab356425f1b85be41d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49006
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This imports part of the fix for CVE-2021-3712, commits
d9d838ddc0ed083fb4c26dd067e71aad7c65ad16,
5f54e57406ca17731b9ade3afd561d3c652e07f2,
23446958685a593d4d9434475734b99138902ed2,
and bb4d2ed4091408404e18b3326e3df67848ef63d0 from upstream. The
others will be imported in follow-up CLs.
Change-Id: Ic35aeb3895935ee94b82a295efade32782e8d1bc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49005
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We fill in placeholder values of all zeros fairly often in TLS now,
as workarounds for messages being constructed in the wrong order.
draft-12 of ECH adds even more of these. Add a helper so we don't need
to interrupt an || chain with a memset.
Change-Id: Id4f9d988ee67598645a01637cc9515b475c1aec2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48909
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Kenny Root
Tamas Petz
Adam Langley
Shelley Vohr
Adam Langley