When making a CRYPTO_BUFFER from a static, const buffer, there is no
need to make a copy of the data. Instead, we can reference it directly.
The hope is this will save a bit of memory in Chromium, since root store
certs will already in static data.
Moreover, by letting static CRYPTO_BUFFERs participate in pooling, we
can extend the memory savings to yet other copies of these certs. For
instance, if we make the root store updatable via component updater,
most of the updated roots will likely already be in the binary's copy.
Pooling will transparently dedup those and avoid retaining an extra
copy.
(I haven't gone as far as to give static CRYPTO_BUFFERs strong
references from the pool, since that seems odd. But something like
Chromium probably wants to intentionally leak the initial static ones so
that, when all references go away, they're still available for pooling.)
Change-Id: I05c25c5ff618f9f7a6ed21e4575cf659e7c32811
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50045
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
ASN1_ENCODING can be unexported because all types using it are now
hidden. This does mean external uses of <openssl/asn1t.h> can no longer
use ASN1_SEQUENCE_enc, but there do not seem to be any such uses.
ASN1_TLC and ASN1_TEMPLATE typedefs are only necessary for users of
asn1t.h. I'm hopeful we can do away with ASN1_TLC once I get to
reworking tasn_dec.c. ASN1_TEMPLATE is somewhat stuck, though all
references should be hidden behind macros.
ASN1_generate_* appear to only referenced within the library. Remove the
unused one and move the other to x509/internal.h. (asn1_gen.c is
currently in crypto/x509 rather than crypto/asn1, so I put it in
x509/internal.h to match. I'll leave figuring out that file to later.)
Annoyingly, asn1/internal.h now pulls in asn1t.h, but so it goes.
Change-Id: I8b43de3fa9647883103006e27907730d5531fd7d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50106
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
There are cases where people grep binaries for strings like OpenSSL
version strings in order to detect when out-dated versions of libraries
are being used. With BoringSSL you might find "OpenSSL 1.1.1
(compatible; BoringSSL)", if the linker didn't discard it, but that's
not very helpful for knowing how up-to-date BoringSSL is because we
hardly ever change it.
This change adds a distinct random value to search for that uniquely
identifies BoringSSL and includes a rough guide to how old the BoringSSL
copy is. The linker will hopefully not discard it because it's
refereneced from |OPENSSL_malloc|.
Change-Id: Ie2259fd17a55d249a538a8a161b0d755396dd7b8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49885
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
While I'm here, add missing parentheses around the B_ASN1_* bitmasks.
I've tossed ASN1_PRINTABLE into the deprecated bucket, though X509_NAME
relies on it, because it is a mess.
Bug: 407, 426
Change-Id: I287f60e98d6c9f237908011e1a816f4b4fb4433e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50105
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
After https://boringssl-review.googlesource.com/c/boringssl/+/45965,
X509_VAL became largely unusable. While it did still exist as an
ASN1_ITEM and we emitted d2i/i2d/new/free functions, there is no way to
access its contents. Thus, hide it entirely.
Interestingly, although we got that to stick a while ago, I missed that
OpenSSL actually keeps X509_VAL exported, so it's possible we'll find 3p
code that uses this later. Since a standalone X509_VAL isn't especially
useful to construct or encode, this is most likely to come up in code
defining new types with <openssl/asn1t.h>.
Still, if we need to rexport this later (revert this *and* bring back
the struct), it won't be a big deal. Nothing in the public API even
constrains X509 to use X509_VAL.
Update-Note: The last remnants of the now (barely usable) X509_VAL are
no longer exported. It is unlikely anyone was relying on this.
Bug: 425
Change-Id: I90975f2f7ec27753675d2b5fa18b5cc4716319f4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50085
Reviewed-by: Adam Langley <agl@google.com>
Outside the library, this function is practically useless. It creates an
empty ASN1_OBJECT, which can never be filled in because the struct is
private and there are no mutating setters.
(See https://boringssl-review.googlesource.com/c/boringssl/+/46164 and
https://boringssl-review.googlesource.com/c/boringssl/+/48326 for a
discussion on why it's important ASN1_OBJECTs are immutable.)
Update-Note: ASN1_OBJECT_new is no longer exported. While this function
does remain in OpenSSL, it is extremely unlikely anyone has found a use
for this function.
Bug: 452
Change-Id: I111a9a1ce3ca4d7aa717a3c3a03d34c05af8fdbd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50025
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
If the header is valid, but the body is truncated, ASN1_get_object
intentionally preserves the indefinite-length and constructed output
bits. This means callers who check for error with == 0x80 may read off
the end of the buffer on accident.
This is unlikely to break callers: 0x80 was already a possible error
value, so callers already needed to handle it. The original function's
aim in returning more information is unlikely to matter because callers
cannot distinguish 0x80 (could not parse header) and 0x80 (header was
valid, definite-length, and primitive, but length was too long).
Update-Note: ASN1_get_object's calling convention is slightly
simplified.
Bug: 451
Change-Id: If2b45c47e6b8864aef9fd5e04f313219639991ed
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50005
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We generate .S files for assembly, which means they run through the C
preprocessor first. In gas targets where # is the comment marker, there
is a conflict with cpp directives.
The comments actually rely on #This and #source not being directives. If
I begin a line with "if", the build fails. Since the C preprocessor is
responsible for removing C preprocessor comments, we should be able to
safely use // everywhere with less ambiguity.
(In fact, we were already relying on this for 32-bit ARM. The 32-bit ARM
gas line comment marker is @. 64-bit ARM uses //, and x86/x86_64/ppc64
use #.)
This reportedly causes issues for goma. See
https://bugs.chromium.org/p/boringssl/issues/detail?id=448#c3
Bug: 448
Change-Id: Ib58f3152691c1dbcccfc045f21f486b56824283d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49965
Reviewed-by: Adam Langley <agl@google.com>
Also fill in docs for some easy ASN1_STRING wrappers while I'm here.
(Not sure why they exist, but removing them is probably more trouble
than is worth it.)
Bug: 407, 426
Change-Id: Id12c5fbc84982728435d105d66a3b63e5f3a1d15
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49945
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Some BIO_write failures weren't handled. Otherwise would successfully
write truncated results. The other i2a functions all report -1 on
truncation, so match. While I'm here, write a test to make sure I didn't
break this.
Change-Id: If17d0209e75c15b3f37bceb1cdfb480fd2c62c4d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49931
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
They do the same thing, except i2a_ASN1_ENUMERATED has a bug and doesn't
handle negative values.
Change-Id: Ifb22aa4e4d6c441a39cf6b3702cce7f6d12a94ae
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49929
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The comparison should notice differences in bit count.
Update-Note: ASN1_STRING_cmp no longer incorrectly treats BIT STRINGs
with different padding bits as equal.
Bug: 446
Change-Id: I22b3fcc5d369540d029ca234e9b3b02402cec4c3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49928
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
ASN1_item_unpack was missing checks for trailing data. ASN1_item_pack's
error handling was all wrong. (Leaking the temporary on error, checking
the the wrong return value for i2d, would-be redundant check for NULL,
were the other check not wrong.)
Update-Note: ASN1_item_unpack now checks for trailing data.
Change-Id: Ibaa19ba2b264fca36dd21109e66f9558d373c58b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49927
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
In doing so, fix ASN1_item_pack to not use the ASN1_OCTET_STRING
typedef. The function makes an untyped ASN1_STRING.
With all these caveats, one might think that ASN1_BOOLEAN ASN1_ITEMs are
pretty useless. This is about right. They're really only usable embedded
as a field in another struct.
Bug: 426
Change-Id: Id7830b91b2d011038ce79ec848e17ad6241423e1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49926
Reviewed-by: Adam Langley <agl@google.com>
This is completely unchecked for now, as it all goes through tasn_enc.c.
But the only non-const encoders now are X509_NAME, and the functions
that call into it, so we can fix up the ones at the bottom.
I haven't done the macros that use the "name" or "fname" variants. The
set of macros for const are a little weird. But before expanding the
header macros out, I wanted to change the signatures on the macro side
once, so the compiler checks they're expanded correctly.
Update-Note: The type signature of some i2d functions, such as
i2d_ASN1_OCTET_STRING, is now const-correct.
Bug: 407
Change-Id: I03988f5591191b41ab4e7f014bd8d41cb071b39a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49908
Reviewed-by: Adam Langley <agl@google.com>
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>
David Benjamin
Adam Langley
Kenny Root
Tamas Petz
Shelley Vohr
Adam Langley