Change-Id: I32a40a73f96e029ac9096af826d15b22d9dcad28
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58745
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
HRSS itself remains in libcrypto because there are some direct users of
it. But this will let it be dropped by the linker in many cases.
Change-Id: I870eda30c9ed1d08693c770e9e7df45a2711b7df
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58645
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
These are already unused, though add and add_alias will need more work.
In doing so, simplify the X509V3_EXT_DYNAMIC business. I added some
cleanup calls to https://boringssl-review.googlesource.com/2208, but
that should have been in the error-handling path of
X509V3_EXT_add_alias, the only case that cares about this.
Update-Note: Removed unused API.
Bug: 590
Change-Id: Idd97366d90d7aab0ca2e020c76a7c8065b3dd7ff
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58765
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
I don't believe these codepaths have ever been run. All the built-in
X509V3_EXT_METHODs are ASN1_ITEM-based, as are all callers I found of
X509V3_EXT_add and X509V3_EXT_add_list.
Also document not to use those APIs because they're pointless and (for
now) not even thread-safe. Making them thread-safe is doable, but it'd
add rwlock contention in certificate verification, unless we first
rework certificate verification to ignore custom registrations, because
it never uses them anyway. But that only proves that this whole feature
was pointless, so this time may be better spent trying to get rid of
this API.
Update-Note: Externally-installed X509V3_EXT_METHODs now must be
ASN1_ITEM-based. I believe all existing ones already are. If there are
any that aren't, let us know. We'll either revert this for now, or
export a way to implement custom ASN1_ITEMs, or, most likely, try to
move the caller off custom X509V3_EXT_METHODs altogether. Like most of
OpenSSL's other global registration APIs, this one is unsafe (two
callers may conflict) and there isn't much reason to register it with
the library because OpenSSL doesn't do much with the registration
anyway.
Bug: 590
Change-Id: Ice0e246d50069e10e6cca8949f60fac474d0846c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58687
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This reverts a small portion of
8c8629bfd89436e5019b6bd3c65cff4bf1a76b76. The parsers for ANY remain
unchanged, but we inadvertently changed a corner case of ASN1_PRINTABLE
MSTRINGs. This is a huge mess.
utype in these switch cases is usually the type of the ASN1_ITEM, but,
with ANY and MSTRING, it is the tag of the value we found. (An MSTRING
or "multi-string" is a CHOICE of string-like types.)
When parsing ANY, this is moot because the is_supported_universal_type
logic ensures we'll never pass in an invalid type. When encoding ANY,
this only happens if you manually construct such an ASN1_TYPE.
MSTRINGs *should* be similar because of the bitmask they apply on tag
types. However, there is one MSTRING type whose bitmask,
B_ASN1_PRINTABLE, includes B_ASN1_UNKNOWN. ASN1_tag2bit, arbitrarily
maps eight unsupported tags to B_ASN1_UNKNOWN and instead of zero. These
are:
- ObjectDescriptor
- EXTERNAL
- REAL
- EMBEDDED PDV
- RELATIVE-OID
- TIME (note this is not the same as the X.509 Time CHOICE type)
- [UNIVERSAL 15], which is not even a defined type!
- CHARACTER STRING
(ENUMERATED is also mapped to B_ASN1_UNKNOWN, but it's supported.)
These eight tags were previously accepted in d2i_X509_NAME but
8c8629bfd89436e5019b6bd3c65cff4bf1a76b76 inadvertently started rejecting
them. For now, restore the default in the switch/case so that we accept
them again. Per https://crbug.com/boringssl/412, attribute values are
ANY DEFINED BY types, so we actually should be accepting *all* types. We
do not, because B_ASN1_PRINTABLE is completely incoherent. But because
ANY is the correct type, going from the original incoherent set, to
this new, smaller incoherent set is arguably a regression.
This is a minimal fix. Long-term, we should handle that ANY correctly,
and avoid unexpected ASN1_STRING type values, by mapping all unsupported
types to V_ASN1_OTHER. This would allow us to support all types
correctly. A follow-up change will do that.
Update-Note: The X.509 name parser will go back to accepting a handful
of universal tag types that were inadvertently rejected in
8c8629bfd89436e5019b6bd3c65cff4bf1a76b76. It is extremely unlikely that
anyone uses these as they're unsupported, obscure types. This CL also
makes our ASN1_TYPE encoder slightly more permissive again, if the
caller manually constructs an legacy in-memory representation of an
unsupported tag. But the follow-up change will restore the stricter
behavior.
Bug: 412, 561
Change-Id: Ia44a270f12f3021154761a1cd285707416d8787e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58705
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Change-Id: I6e53434246f3fef06d4f88924bfe1cbfad0543e8
Bug: chromium:1414562
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58205
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Change-Id: I43dd18f7d70ee06ca25affad0ab06e5d5ef8263d
Signed-off-by: wangjiale3 <wangjiale3@xiaomi.corp-partner.google.com>
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58489
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Also unexport PEM_proc_type and PEM_dek_info. They're never called
externally, just private functions within one file. Also, while I'm
here, fix the include guard on asn1/internal.h.
Bug: 516
Change-Id: I6961a65f638e7b464a8c349663898a954d7826b4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58548
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Change-Id: Idcf0fdcc88af509958e56052c1925f3f695bc3e3
Signed-off-by: wangjiale3 <wangjiale3@xiaomi.corp-partner.google.com>
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58487
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
We support BIO_gets on three BIOs. They're all slightly different. File
BIOs have the NUL truncation bug. fd BIOs swallow the embedded newline.
This CL fixes the second issue and updates the BIO_gets test to cover
all three.
See also upstream's https://github.com/openssl/openssl/pull/3442
Update-Note: BIO_gets on an fd BIO now returns the newline, to align
with memory and file BIOs. BIO_gets is primarily used in the PEM parser,
which tries to tolerate both cases, but this reduces the risk of some
weird bug that only appears in fd BIOs.
Change-Id: Ia8ffb8c67b6981d6ef7144a1522f8605dc01d525
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58552
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I4288988f3742f14b15f80a3023b716392a667631
Signed-off-by: wangjiale3 <wangjiale3@xiaomi.corp-partner.google.com>
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58485
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The outl <= 0, etc., checks are actually redundant with logic in the
wrappers, but it seems easier to just add the check and avoid worrying
about it. Maybe someday we'll make the internals use size_t and this
will be moot.
Bug: 516
Change-Id: I0bea5ac325c79b9765d822c816661fe4f2bcd4cc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58546
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Our test coverage for BIOs isn't great. Fill in missing memory BIO
tests, in preparation for reworking it a bit to be size_t-clean.
Change-Id: I77aeab93d6d9275c65e998d517463f4cc10efcf3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58545
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Passes test vectors, and should be constant time, but is currently
not optimized and neither the API nor the standard is stable.
Change-Id: I89b90877e023a43ee7238e11b86065444ab3bdec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57845
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
If we see a critical policy constraints extension, we have two options:
We can either process it, which requires running policy validation, or
reject the certificate. We and OpenSSL do neither by default, which
means we may accept certificate chains that we weren't supposed to.
This fixes it by enabling X.509 policy validation unconditionally and
makes X509_V_FLAG_POLICY_CHECK moot. As a side effect, callers no longer
need to do anything for CVE-2023-0466.
This is the opposite of [0]'s advice, which instead recommends skipping
the feature and rejecting critical policy contraints. That would be a
good move for a new X.509 implementation. Policy validation is
badly-designed, even by X.509's standards. But we have OpenSSL's history
of previously accepting critical policy constraints (even though it
didn't check it). I also found at least one caller that tests a chain
with policy constraints, albeit a non-critical one.
We now have an efficient policy validation implementation, so just
enable it.
Of course, fixing this bug in either direction has compatibility risks:
either we take on the compat risk of being newly incompatible with
policyConstraints-using PKIs, or we take on the compat risk of newly
rejecting certificates that were invalid due to a policy validation
error, but no one noticed. The latter case seems safer because the chain
is unambiguously invalid.
Update-Note: X.509 certificate verification (not parsing) will now
notice policy-validation-related errors in the certificate chain. These
include syntax errors in policy-related extensions, and chains with a
requireExplicitPolicy policy constraint that are valid for no
certificate policies. Such chains are unambiguously invalid. We just did
not check it before by default. This is an obscure corner of X.509 and
not expected to come up in most PKIs.
[0] https://www.ietf.org/archive/id/draft-davidben-x509-policy-graph-01.html#section-3.4.4
Fixed: 557
Change-Id: Icc00c7797bb95fd3b14570eb068543fd83cda7b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58426
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
OpenSSL interprets NULL and empty lists as {anyPolicy}. I intended to
implement this, but didn't quite get the NULL case right. Fix this and
add a test.
Change-Id: I50dbf02695f424697e28a6e0ec4fd50b2822f44f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58425
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This was not thread-safe and, until the previous CL, egregiously so. No
one uses this API, so remove it.
Update-Note: Various unused functions for registering named
X509_VERIFY_PARAMs were removed. These functions only exist to make
X509_VERIFY_PARAM_lookup return a custom value. Instead, applications
that want a particular X509_VERIFY_PARAM can just configure it directly,
rather than stashing it in library-global state and then looking it back
up with X509_VERIFY_PARAM_lookup.
Change-Id: I8d532a1a137c7abbc131f2cb5d12ba94e5728e2d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58386
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This is a double-pointer and both layers should be const. This matches
OpenSSL 1.1.1, so in addition to being more const-correct, we're more
OpenSSL-compatible.
Update-Note: Anything that defines a comparison function would need to
fix the type signature. I found only one external caller, Envoy, that
defines it. https://github.com/envoyproxy/envoy/pull/25051 fixes it.
(That we hadn't run into the upstream incompatibility suggests this is
just not a feature folks use outside the library much.)
Bumping BORINGSSL_API_VERSION, in case I missed any, and there's some
caller where we can't just use C++14 generic lambdas to smooth it over.
Fixed: 498
Change-Id: I8f07ff42215172aa65ad8819acf69b63d6d8e54c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56190
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These functions need a lot more work, documentation, warnings
that using them isn't a good idea, and really we should just remove them
entirely.
But, for now, this is a minimal fix to the most egregious of issues: not
only are the functions themselves not thread-safe (i.e. you must call it
in some program-global initialization), but using them puts you in a
state where future uses of the X.509 library are not thread-safe! Fix
the latter by sorting the list at the point we're already mutating
things.
Re-sorting a list after every addition is not a particularly sensible
implementation, but we can assume these lists will only ever contain
O(1) entries.
(The sort calls date to
https://boringssl-review.googlesource.com/c/boringssl/+/27304, but the
issue was there before. Prior to that CL, sk_FOO_find implicitly sorted
the list. That CL made sk_FOO_find itself a const operation, necessary
for this, and just added explicit sk_FOO_sort calls to preserve the
existing behavior, initially.)
Change-Id: I063b8e708eaf17dfe66c5a3e8d33733adb3297e9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58385
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
I mistakenly thought no one needed X509 as an ASN1_ITEM, but that wasn't
true. wpa_supplicant relies on this. Restore this and add a test for it.
As with the rest of the rewrite, this is currently a little tedious. I'm
hoping that, as the internals are rewritten with CBS and CBB, we can
establish some cleaner patterns and abstractions.
Bug: 547
Change-Id: I761ee058f8ec916b2ec7f4730a764d46d72f1f10
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58285
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Now that the preceding CL has isolated the X.509 signature hack, we can
apply the strictness across the legacy parser. This is particularly
important for the TBSCertificate parser, where it is ambiguous which
value one checks the signature over. (Officially, you're supposed to
re-encode as DER. In practice, people don't do this.)
This change means many of our primitive types are actually parsed as
DER. I've removed the bug references in the comment in the documentation
where I believe they're finally correct.
Update-Note: Non-minimal lengths in certificates are no longer accepted,
as required for standards compliance. The one exception is the signature
field, where we still carry an exception. Some of this was already
enforced by libssl's parser.
Bug: 354
Change-Id: I57cfa7df9e1ec5707390e9b32fe1ec6b5d8172f9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58186
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is a cursory conversion and is, currently, very tedious because it
needs to bridge calling conventions. After tasn_*.c and all the
underlying primitives have CBS/CBB-based calling conventions, this
should be a lot cleaner.
This is to break a dependency cycle:
- We'd like to rewrite d2i_X509 with CBS
- To do that, we need to rewrite its underlying types with CBS
- Those parsers are tied up in tasn_dec.c, so we effectively need to
rewrite tasn_dec.c with CBS.
- CBS is designed for DER, not BER, so such a change would most
naturally switch the TLV parser to require DER.
- We've *almost* done that already except
https://boringssl-review.googlesource.com/c/boringssl/+/51626 had to
stop at non-minimal definite lengths, which are allowed in BER but
forbidden in DER. See b/18228011 for a bunch of certificates which
have a non-minimal definite length at *just* the signature field.
- So, to do that, we'd ideally special case just that field, or BIT
STRINGs in general, to tolerate minimal lengths. That's easiest done
when d2i_X509 is CBS, so we can just do what we want in imperative
code. And thus we're back full circle.
So, detach X509 from the templates now. It's a bit tedious because we
need to switch calling conventions for now, but it breaks the cycle.
Later, we can revisit this and get all the benefits of a fully CBS-based
path.
For now, I haven't added an ASN1_ITEM. If it comes up, we can make an
EXTERN ASN1_ITEM.
Update-Note: The ASN1_ITEM removal means custom ASN.1 templates (which
are discouraged in favor of our much simpler CBS and CBB types) using
X509 will fail to compile. We don't believe anyone is relying on this,
but this can be restored if we find something.
Update-Note: Certificate parsing is slightly stricter: the outermost
TLVs, except for the signature field, no longer accept non-minimal
lengths, as mandated by DER. This strictness was broadly already applied
by the libssl parser.
Bug: 547
Change-Id: Ie5ad8ba4bb39f54fdd3dd45c53965b72a3850709
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58185
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
OpenSSL's ASN1_STRING representation has many cases. There's a grab-bag
V_ASN1_OTHER cases that can represent any element. But it is currently
only used for non-universal tags. Unknown universal tags go into the
type field directly.
This has a few problems:
- Certain high values, V_ASN1_NEG_INTEGER and V_ASN1_NEG_ENUMERATED,
are treated special. This was one of the two causes behind
CVE-2016-2108 and had to be worked around with V_ASN1_MAX_UNIVERSAL.
- OpenSSL can never compatibly support a new universal type in a
non-ASN1_STRING form. Otherwise ASN1_TYPE's union changes its
in-memory representation.
- It is a bit ambiguous when OpenSSL does or doesn't know the type.
- This is broadly implemented by having a default in all the
switch/cases, which is a little awkward.
- It's yet another "unknown tag" case when V_ASN1_OTHER covers such
cases just fine.
Remove this representation and use V_ASN1_OTHER. This more unambiguously
resolves CVE-2016-2108. ASN1_STRING's and ASN1_TYPE's respective type
fields are now a closed set. Update the documenthation accordingly.
Formally allowing universal types in ASN1_STRING also opens the door to
clearing the ASN1_PRINTABLE mess (https://crbug.com/boringssl/412).
BoringSSL currently rejects X.509 names that are actually valid, because
the OpenSSL X509_NAME representation cannot represent them. This allows
us to introduce an ASN1_STRING-based ANY representation, which just
represents all non-ASN1_STRING types in an V_ASN1_OTHER.
The implementation is a little clumsy (the way things tasn_dec.c is
written, I had to introduce yet another check), but I'm hoping that,
when the parser is rewritten with CBS, this can be integrated into a
single type dispatch.
Update-Note: This does not change the set of inputs accepted or rejected
by the ASN.1 parser. It does, however, change the in-memory
representation in edge cases. Unless the application was specifically
inspecting the in-memory representation for these unknown types, we
expect this to have no impact.
Fixed: 561
Change-Id: Ibf9550e285ce50b11c7609d28b139354b9dd41dc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58148
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Removing object reuse makes it dramatically simpler. Along the way, lift
the OID validity checker into crypto/bytestring, so we can use it more
generally. (Although the difference between invalid OID and unknown OID
is pretty academic, so this check isn't that important.)
For now I've preserved the existing behavior, where the OID validity
checker accepts arbitrarily large OID components. Though this results in
an oddity where the OID to string functions reject inputs that the
parser accepts. (There we only allow up to 2^64-1.)
Update-Note: When we removed object-reuse from all the d2i functions, we
missed one d2i_ASN1_OBJECT. See
https://boringssl-review.googlesource.com/c/boringssl/+/56647.
Otherwise, this CL is not expected to change behavior.
Change-Id: If4d2d83d9f3c96abfdc268e156f2cf3a9a903b0c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58147
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
These functions already don't go through tasn_*.c. Rewrite them to use
CBS and CBB. This removes some dependencies on ASN1_get_object and
ASN1_put_object.
Update-Note: d2i_ASN1_OBJECT and d2i_ASN1_BOOLEAN will no longer accept
non-minimal length prefixes (forbidden in DER). d2i_ASN1_BOOLEAN will
also no longer accept non-canonical representations of TRUE (also
forbidden in DER). This does not affect certificate parsing, as that
still goes through the old template system, though we will make a
similar change to those functions later.
Bug: 354, 548
Change-Id: I0b7aa96f47aca5c31ec4f702e27108b4106311f2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58145
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
ASN1_TYPE is a union of a bunch of pointer types. Often it takes a
shorthand and accesses a->value.ptr directly. This is allowed in C, but
not C++. Writing the switch/case barely takes more code, so just write
it that way.
Along the way, extract the code for cleaning up an ASN1_TYPE from
tasn_fre.c. This is a small step towards being able to use crypto/asn1's
types without depending on the templates. ASN1_TYPE_free still, for now,
calls into the templates. That will be fixable once tasn_*.c are
rewritten a bit further.
This also removes the weird hack here ASN1_primitive_free (an internal
function) with NULL ASN1_ITEM secretly meant to partially clean up the
ASN1_TYPE. We can just call the function directly now.
Bug: 574
Change-Id: Ie2ba41418801a366ab2f12eccc01e8dadf82c33e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58126
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
An in-progress rewrite of tasn_dec.c accidentally broke this, so add a
regression test.
Bug: 548
Change-Id: Iac6a23acbc08459187c96a2f6471f0aa97d445a1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58125
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Change-Id: I59bcacf10a59ffdf9709785727f5f8b73c992f6e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58026
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
RDNs are a SET OF attributes which means they should be sorted by
DER encoding length, then lexicographically. We didn't have any test
coverage for this.
Bug: 548
Change-Id: I542196aae26984aeee4f1c6774878b121675b0dc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58025
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Guarding the OPENSSL_memset was unnecessary since OPENSSL_memset with
zero length works fine. Also OPENSSL_memset, to workaround a C bug,
internally does the same check anyway. (But also this wasn't a context
where the C bug applied.)
Also don't bother calling EVP_MD_block_size a second time when we
already saved it into block_size.
Finally, don't bother filling key_block up to the whole
EVP_MAX_MD_BLOCK_SIZE size. I could believe the fixed size is easier for
the compiler to optimize, but the XORs in setting up an HMAC cannot
possibly be performance-sensitive, and using the actual length is
clearer.
Also add an assert that the hash's output size fits in the block size.
We're implicitly relying on this when hashing the key down.
Change-Id: I6ce37d41ea5bdbc8890bde7910e1b5651bc35709
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58027
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
With EVP_PKEY and EVP_PKEY_CTX opaque, these symbols don't appear in any
public APIs anymore. Make them internal, which also opens the door to
renaming them:
- EVP_PKEY_METHOD is more accurately EVP_PKEY_CTX_METHOD
- EVP_PKEY_ASN1_METHOD is more accurately EVP_PKEY_METHOD
Or perhaps the split doesn't mean much and we should fold them together.
Change-Id: I8a0f7c2e07445dc981c7cef697263e59dba7784e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57885
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
I did not have "find a bug in the assembler" on my bingo card today, but
here we are.
NASM 2.15, prior to 2.15.04, has a bug where, if a section that already
exists is referenced again with alignment qualifiers, it incorrect adds
padding and mangles the output. See
https://bugzilla.nasm.us/show_bug.cgi?id=3392701.
Work around this by suppressing the perlasm-emitted qualifiers the
second time a section is emitted. We likely don't need these qualifiers
because, for all sections we care about, NASM's defaults are fine, but
perlasm tries to align .text more aggressively than the default, so let
it do that.
Bug: chromium:1422018
Change-Id: Iade5702c139b70772d4957a83c8f9be86c8af97c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57825
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
ASN1_item_ex_i2d() does not take ownership of the memory pointed at by
*out, so it's the caller's responsibility to free it on error.
Change-Id: Id8cb70e50f280944418629a32b53fd4ca248b0bd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57805
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Also turn assertions into static_assert where we can.
These should be no-ops with existing assertions. The int assertion is
tighter, but we already assert this in constant_time_declassify_int. We
cannot support 64-bit int because it messes up integer promotion rules.
Change-Id: I628d2d7decdfa8bc01d8c6013bc7c20f927d63b1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57785
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This test could simulate the lack of MADV_WIPEONFORK on systems with it,
but couldn't simulate having it if the kernel didn't support it.
This change makes the presence / absence of
BORINGSSL_IGNORE_MADV_WIPEONFORK control whether MADV_WIPEONFORK is
"supported" in the test environment or not, and corrects the test for
the case where it's missing.
Change-Id: I23876788a0e0a4fd2a148f98b6b94e40880b6fc9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57745
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
We do not expect to support this combination, but other consumers of
BoringSSL may choose to.
Change-Id: Ifdafa6a0032af078343bb9ecd80eea89eee582be
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57705
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
No new tests because they're actually caught by our own tests. I just
forgot to put UBSan on CI! Will fix this shortly.
For BLAKE2, fix this by checking for zero. For c2i_ASN1_INTEGER,
rewrite it with CBS, which has the side effect of avoiding this. (It's
effectively maintaining in->data + start as a temporary, rather than
start itself.)
Bug: fuchsia:46910
Change-Id: I9366f1ba4fd0b0140d64c56e0534d7b060ab90e5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57687
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
If another project includes us as a subproject, as gRPC does,
CMAKE_SOURCE_DIR points to the top-level source directory, not ours.
PROJECT_SOURCE_DIR points to ours. Likewise, CMAKE_BINARY_DIR will
point to the top-level one.
gRPC doesn't consume this CMake build, but in preparation for
eventually unifying the two CMake builds, replace CMAKE_SOURCE_DIR and
CMAKE_BINARY_DIR with a combination of CMAKE_CURRENT_{SOURCE,BINARY}_DIR
and PROJECT_SOURCE_DIR.
There's one more CMAKE_SOURCE_DIR which controls some default install
directory. I've left that one alone for now as I'm not sure what to do
with it. Probably the answer is to, like in gRPC, disable the install
target by default when we're not the top-level source directory.
Bug: 542
Change-Id: Iea26bbef8a4637671fd0e7476101512e871e7e18
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57686
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We used to have a tower of fallbacks to support older Androids that were
missing getauxval. The comments say getauxval is available in Android
API level 20 or higher, but this wasn't right. It's actually API level
18 or higher per the NDK headers and
https://developer.android.com/ndk/guides/cpu-features
Android API level 18 is Android 4.3, or Jelly Bean MR2. Recent versions
of the NDK (starting r24, March 2022) don't even support Jelly Bean,
i.e. the minimum API level is 19, and the usage statistics in the latest
Android Studio stop at KitKat. As far as I know, nothing needs us to
support API levels 17 and below anymore.
Update-Note: BoringSSL now requires API level 18 or later. Projects
needing to support API level of 17 or below will fail to build due to
the use of getauxval. If any such projects exist, please contact
BoringSSL maintainers.
Change-Id: Iedc4836ffd701428ab6d11253d4ebd5a9121e667
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57506
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Setting the first argument to -1 works, but changing the syscall number
is more straightforward. It's doable on aarch64 too, if we use a
different regset.
Cq-Include-Trybots: luci.boringssl.try:android_aarch64
Change-Id: I6c3c2d3dc67c06a44b181f9086cb5c9d343d51bc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57587
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
At least on the devices we have on CI, PTRACE_SETREGS seems to break
things when passing in a slightly smaller structure. I'm guessing the
other registers get set to zero, so we SIGILL.
Cq-Include-Trybots: luci.boringssl.try:android_aarch64
Change-Id: I09da595dba2b6b70805c9a79c71c797c0f6635c0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57585
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I736c7dc17efcaa5c3d8bd5fdee36d2dcb86ae627
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57567
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Various constants and strings identifying the authors are currently
misplaced in .text. This change allows using execute-only .text on
platforms that enforce it by default, such as OpenBSD.
Modify x86_64-xlate.pl to replace .rodata with __DATA,__const for macs.
Adapt the nasm/masm path to emit an .rdata segment with alignment of 8.
This last change is not strictly needed but makes things explicit.
Change-Id: If716b892c1faabd85c6c70bdd75e145304841f83
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57445
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
