Skip to content

Commit

Permalink
Fix a UAF resulting from a bug in BIO_new_NDEF
Browse files Browse the repository at this point in the history
If the aux->asn1_cb() call fails in BIO_new_NDEF then the "out" BIO will
be part of an invalid BIO chain. This causes a "use after free" when the
BIO is eventually freed.

Based on an original patch by Viktor Dukhovni and an idea from Theo
Buehler.

Thanks to Octavio Galland for reporting this issue.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
  • Loading branch information
mattcaswell authored and t8m committed Feb 3, 2023
1 parent cbafa34 commit 8818064
Showing 1 changed file with 32 additions and 8 deletions.
40 changes: 32 additions & 8 deletions crypto/asn1/bio_ndef.c
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,19 @@ static int ndef_suffix(BIO *b, unsigned char **pbuf, int *plen, void *parg);
static int ndef_suffix_free(BIO *b, unsigned char **pbuf, int *plen,
void *parg);

/* unfortunately cannot constify this due to CMS_stream() and PKCS7_stream() */
/*
* On success, the returned BIO owns the input BIO as part of its BIO chain.
* On failure, NULL is returned and the input BIO is owned by the caller.
*
* Unfortunately cannot constify this due to CMS_stream() and PKCS7_stream()
*/
BIO *BIO_new_NDEF(BIO *out, ASN1_VALUE *val, const ASN1_ITEM *it)
{
NDEF_SUPPORT *ndef_aux = NULL;
BIO *asn_bio = NULL;
const ASN1_AUX *aux = it->funcs;
ASN1_STREAM_ARG sarg;
BIO *pop_bio = NULL;

if (!aux || !aux->asn1_cb) {
ERR_raise(ERR_LIB_ASN1, ASN1_R_STREAMING_NOT_SUPPORTED);
Expand All @@ -70,33 +76,51 @@ BIO *BIO_new_NDEF(BIO *out, ASN1_VALUE *val, const ASN1_ITEM *it)
out = BIO_push(asn_bio, out);
if (out == NULL)
goto err;
pop_bio = asn_bio;

BIO_asn1_set_prefix(asn_bio, ndef_prefix, ndef_prefix_free);
BIO_asn1_set_suffix(asn_bio, ndef_suffix, ndef_suffix_free);
if (BIO_asn1_set_prefix(asn_bio, ndef_prefix, ndef_prefix_free) <= 0
|| BIO_asn1_set_suffix(asn_bio, ndef_suffix, ndef_suffix_free) <= 0
|| BIO_ctrl(asn_bio, BIO_C_SET_EX_ARG, 0, ndef_aux) <= 0)
goto err;

/*
* Now let callback prepends any digest, cipher etc BIOs ASN1 structure
* needs.
* Now let the callback prepend any digest, cipher, etc., that the BIO's
* ASN1 structure needs.
*/

sarg.out = out;
sarg.ndef_bio = NULL;
sarg.boundary = NULL;

if (aux->asn1_cb(ASN1_OP_STREAM_PRE, &val, it, &sarg) <= 0)
/*
* The asn1_cb(), must not have mutated asn_bio on error, leaving it in the
* middle of some partially built, but not returned BIO chain.
*/
if (aux->asn1_cb(ASN1_OP_STREAM_PRE, &val, it, &sarg) <= 0) {
/*
* ndef_aux is now owned by asn_bio so we must not free it in the err
* clean up block
*/
ndef_aux = NULL;
goto err;
}

/*
* We must not fail now because the callback has prepended additional
* BIOs to the chain
*/

ndef_aux->val = val;
ndef_aux->it = it;
ndef_aux->ndef_bio = sarg.ndef_bio;
ndef_aux->boundary = sarg.boundary;
ndef_aux->out = out;

BIO_ctrl(asn_bio, BIO_C_SET_EX_ARG, 0, ndef_aux);

return sarg.ndef_bio;

err:
/* BIO_pop() is NULL safe */
(void)BIO_pop(pop_bio);
BIO_free(asn_bio);
OPENSSL_free(ndef_aux);
return NULL;
Expand Down

0 comments on commit 8818064

Please sign in to comment.