| Commit message (Collapse) | Author | Files | Lines |
|---|
|
|
|
|
|
|
|
|
|
|
|
The inner content of SignedData is represented by a PKCS7 object, which
PKCS7_stream() assumes to be a plain data object and will thus access its
content via an ASN1_OCTET_STRING. This need not be the case after parsing.
In fact, the inner content type is essentially arbitrary.
If the inner content isn't one of the explicitly supported content types,
the fallback (via p7default_tt) will populate the union's d.other with an
ASN1_ANY which unravels to ASN1_TYPE_new() deep in the guts of tasn_dec,
allocating a 16-byte object on LP64 architectures. In that case, the
16-byte object is interpreted as an 24-byte ASN1_OCTET_STRING and if it
isn't NULL, the read+write to os->flags (a long at offset 16) is out of
bounds: os->flags | ASN1_STRING_FLAG_NDEF;
Add a check that the content is actually id-data before accessing the
d.data union member.
From Acts1631
|
|
Test case originally from openssl/openssl#31681, exercised via a direct
call to PKCS7_stream() as in a report from Acts1631.
To be fixed in pk7_lib.c r1.33
|
|
Do not access the PKCS7 content union without checking that it's actually
populated. Add NULL checks and fail. Whether that's the correct thing
to do is dubious, but since this has been broken since the "code" was
written a quarter century ago, clearly nobody ever wanted to do that.
Match OpenSSL behavior which also means more NULL checks than strictly
make sense.
CMS_stream() has very similar code, but it's not problematic in this
particular way because the content isn't OPTIONAL.
Part of a diff from Acts1631
|
|
The PKCS#7 standard marks the content element of the ContentInfo OPTIONAL.
Accordingly, a PKCS#7 object only containing a Content Type OID is valid:
SEQUENCE {
OBJECT_IDENTIFIER { 1.2.840.113549.1.7.4 }
}
Deserializing such an object works and therefore streaming should at least
have the decency of not segfaulting. Of course there's nothing decent about
PKCS#7 be it the standard or its OpenSSL "implementation".
Exercises a problem reported by Acts1361 and currently crashes.
To be fixed in pk7_lib.c r1.32.
|
|
|
|
dtls1_ctrl() and ssl3_ctrl() return long, but the intermediate return
value was stored in an int. Use long to avoid truncating values returned
by ssl3_ctrl().
CID 497395
From Yuji Hashimoto
ok tb jsing
|
|
The ex_data callback registry is process-wide, but this API could free
it while other threads were still using libcrypto, resulting in a
use-after-free.
Retain the public symbol as a compatibility no-op and mark it
deprecated. Move the actual cleanup to an internal function called by
OPENSSL_cleanup(). Replace the in-tree callers with OPENSSL_cleanup()
at final shutdown to preserve cleanup behavior and coverage.
Document both APIs and the requirement that OPENSSL_cleanup() only be
called after all threads and components have stopped using libcrypto.
ok tb
|
|
This looks like a potential out of bounds read or allowing trailing garbage
in peer_finished. It's harmless because CBS_len() is compared against the
s->s3->hs.peer_finished_len a few lines up, thus it is only cosmetic.
discussed with jsing
ok kenjiro
|
|
|
|
|
|
Per RFC 7301, section 3.2, "In the event that the server supports no
protocols that the client advertises, then the server SHALL respond
with a fatal "no_application_protocol" alert.
If a server does not do that and chooses a protocol that we have not
advertised, we should abort the handshake. The RFC does not specify
an alert for this case. BoringSSL chose illegal_parameter and OpenSSL
decode_error. I slightly prefer illegal_parameter, so went with that.
Reported by Acts1631 with a similar diff.
ok jsing kenjiro
|
|
This test currently fails and will be fixed in ssl_tlsect.c r1.168
|
|
Use server_list and selected instead of list and proto to reduce noise
in the next commit.
ok jsing kenjiro
|
|
Just a tiny little bit of lipstick on this entelodont. This is simpler
and does not change behavior as BN_bn2binpad() returns -1 on failure and
num on success.
jsing points out that BN_bn2binpad() is constant time.
ok jsing kenjiro
PS: henning, you owe me a significant amount of quality beverages for
making me look at this particular tire fire (and corresponding XS files).
|
|
When AES key unwrap with padding is in use, the EVP interface breaks its
contract and writes more than the outlen it initially reports to the output
buffer. This is an old, sneaky trap that the muppet set eons ago and many
victims walked right into it, including the muppet himself.
If inlen is larger than outlen, allocate inlen bytes to unwrap with padding
to avoid a buffer overwrite. This is a variant of OpenSSL's fix. Since we
do not support AES keywrap with padding no actual bufer overwrite occurs
here at the moment, but if we ever chose to do so (unlikely) this trap
would be avoided. There's plenty more traps that the next round of scas
will surely find in this absolute trashfire of CMS support code.
ok kenjiro
|
|
While not the advised way of using the verify callback (either by OpenSSL
or by us) in production, sometimes folks like to return 1 from everything
in the callback and then check the error return and make decicions about
things.
This fix ensures that such callbacks will see the hostname mismatch and
be able to act upon them.
Reported by Alexander Aleksandrovic Klimov
ok tb@
|
|
|
|
|
|
ok schwarze@ sthen@ krw@
|
|
The function has been deprecated for a long time: OpenSSL did so when
adding ASN1_STRING_get0_data in 1.1.0. beck removed it from OpenSSL 4.
The reason is that it returns a modifiable "string" and you should not
be modifying an ASN1_STRING this way. You should be using one of the
clumsy, inconvenient accessors.
schwarze merged the deprecation when adding documentation for
ASN1_STRING_get0_data() in 2018. Make this a bit more obvious: move
function to the end of the documented functions in this page, and mark
it deprecated in the SYNOPSIS, too.
I'm aware that deprecation does not work. I was hopeful to be able to
remove the function in the last cycle, but that did not happen and it
might not happen in this cycle either. It will happen at some point,
though.
Prompted by a question by brad
|
|
|
|
|
|
pointed out by kenjiro
|
|
with/ok kenjiro
|
|
ok kenjiro
|
|
Now that the legacy stack uses struct tls_content, store any remaining
record content directly, rather than fudging up a record that the legacy
stack will process on the next read.
ok kenjiro@ tb@
|
|
The receive buffer is not used for records created for sending. Avoid
allocating a maximum-sized buffer in tls13_record_new() and instead
allocate a header-sized buffer when tls13_record_recv() is first called.
The buffer will grow as needed once the record length is known.
This avoids an unnecessary allocation for outgoing records and reduces
the initial allocation size for incoming records.
ok tb jsing
|
|
|
|
|
|
Processing the key_share extension requires a supported_groups extension.
RFC 9846, section 9.2 explicitly mandates a missing_extension alert in
this situation, so use that instead of illegal_parameter.
We do have code in tls13_client_hello_required_extensions() implementing
the requirements on extensions of the relevant paragraph of that section
with the corrrect alerts. The error in the extension processing happens
before hitting these checks in this particular case.
Reported by Tom Gouville of the tlspuffin team as part of
https://github.com/libressl/portable/issues/1279
ok jsing kenjiro
|
|
|
|
|
|
POSIX leaves ownership of ENTRY.key with the caller. Our current behaviour
came from NetBSD when this code was imported in 2004. NetBSD and FreeBSD
both stopped freeing key values in July 2014. NetBSD in commit fe0a1a6 and
FreeBSD in commit e19f362.
Remove the call to free(3) to align ourselves with NetBSD, FreeBSD and
POSIX. Also adjust rsync for the change in semantics.
Flagged by Sortix os-test.
ok millert@, deraadt@
|
|
CID 25423
ok tb@
|
|
ok kettenis@ tb@
Restore commit; install media build error is not reproducible (deraadt@)
or was operator error (naddy@)
|
|
The conversion of X509V3_EXT_get_nid() from a table to a switch
omitted the OCSP no-check extension method.
Add the missing accessor declaration and switch case.
ok tb@
|
|
|
|
|
|
This API was removed a long time ago but I forgot to update the manpage
(probably because doing so is slightly non-trivial).
There's still a lot of API here that should be removed. OBJ_add_objects()
seems now unused while OBJ_new_nid() is used by the one consumer that is
single-handedly responsible for at least two dozen APIs that could otherwise
be removed from libcrypto with significant simplifications: security/xca.
It would appear the author is on a quest to figure out how to make use of
whatever undocumented garbage they can find in the openssl headers. That's
of course a hobby project you can pursue for decades.
|
|
OK tb@
|
|
ok kettenis@ tb@
|
|
Accept an optional roots file argument while retaining
/etc/ssl/cert.pem as the default. This allows portable regress users
to provide a CA bundle when the default path is unavailable.
ok tb
|
|
These functions are only used within their implementation files and do
not need external linkage. Mark them static.
ok tb
|
|
BUF_reverse() has no callers and is not part of the public API. Remove the
unused function, which also eliminates its -Wmissing-prototypes warning.
ok tb beck
|
|
Include the relevant internal headers in implementation files that define
internal functions.
Also make asn1_local.h and pkcs12_local.h self-contained by including their
corresponding public headers.
ok tb beck
|
