From 521b1a0db5041258096fbabdf8fc1e10ecc793cf Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Tue, 6 Aug 2024 10:35:59 -0600 Subject: [PATCH] Prevent crash on BER-encoded signedAttrs The code was assuming the object was DER-encoded, and the relevant integer was therefore in short form. Because I postponed the DER enforcement in deef7b7823f21914b17838f152a8bd510a348f54, the code should not make reckless assumptions about the signedAttrs encoding. Thanks to Niklas Vogel for reporting this. --- src/object/certificate.c | 86 ++++++++++++++++++++++++---------------- 1 file changed, 51 insertions(+), 35 deletions(-) diff --git a/src/object/certificate.c b/src/object/certificate.c index 2708c66d..1552afed 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -585,47 +585,47 @@ struct progress { /** * Skip the "T" part of a TLV. */ -static void +static int skip_t(ANY_t *content, struct progress *p, unsigned int tag) { - /* - * BTW: I made these errors critical because the signedData is supposed - * to be validated by this point. - */ + /* These errors happen when the object is not DER-encoded */ if (content->buf[p->offset] != tag) - pr_crit("Expected tag 0x%x, got 0x%x", tag, - content->buf[p->offset]); - + return pr_val_err("Expected tag 0x%x, got 0x%x.", + tag, content->buf[p->offset]); if (p->remaining == 0) - pr_crit("Buffer seems to be truncated"); + return pr_val_err("Buffer seems truncated."); + p->offset++; p->remaining--; + return 0; } /** * Skip the "TL" part of a TLV. */ -static void +static int skip_tl(ANY_t *content, struct progress *p, unsigned int tag) { ssize_t len_len; /* Length of the length field */ ber_tlv_len_t value_len; /* Length of the value */ - skip_t(content, p, tag); + if (skip_t(content, p, tag) != 0) + return -EINVAL; len_len = ber_fetch_length(true, &content->buf[p->offset], p->remaining, &value_len); if (len_len == -1) - pr_crit("Could not decipher length (Cause is unknown)"); + return pr_val_err("Could not decipher length (Unknown cause)."); if (len_len == 0) - pr_crit("Buffer seems to be truncated"); + return pr_val_err("Buffer seems truncated."); p->offset += len_len; p->remaining -= len_len; + return 0; } -static void +static int skip_tlv(ANY_t *content, struct progress *p, unsigned int tag) { int is_constructed; @@ -633,17 +633,19 @@ skip_tlv(ANY_t *content, struct progress *p, unsigned int tag) is_constructed = BER_TLV_CONSTRUCTED(&content->buf[p->offset]); - skip_t(content, p, tag); + if (skip_t(content, p, tag) != 0) + return -EINVAL; skip = ber_skip_length(NULL, is_constructed, &content->buf[p->offset], p->remaining); if (skip == -1) - pr_crit("Could not skip length (Cause is unknown)"); + return pr_val_err("Could not skip length (Unknown cause)."); if (skip == 0) - pr_crit("Buffer seems to be truncated"); + return pr_val_err("Buffer seems truncated."); p->offset += skip; p->remaining -= skip; + return 0; } /** @@ -654,12 +656,12 @@ struct encoded_signedAttrs { ber_tlv_len_t size; }; -static void +static int find_signedAttrs(ANY_t *signedData, struct encoded_signedAttrs *result) { -#define INTEGER_TAG 0x02 -#define SEQUENCE_TAG 0x30 -#define SET_TAG 0x31 + static const unsigned int INTEGER_TAG = 0x02; + static const unsigned int SEQUENCE_TAG = 0x30; + static const unsigned int SET_TAG = 0x31; struct progress p; ssize_t len_len; @@ -670,43 +672,55 @@ find_signedAttrs(ANY_t *signedData, struct encoded_signedAttrs *result) p.remaining = signedData->size; /* SignedData: SEQUENCE */ - skip_tl(signedData, &p, SEQUENCE_TAG); + if (skip_tl(signedData, &p, SEQUENCE_TAG) != 0) + return -EINVAL; /* SignedData.version: CMSVersion -> INTEGER */ - skip_tlv(signedData, &p, INTEGER_TAG); + if (skip_tlv(signedData, &p, INTEGER_TAG) != 0) + return -EINVAL; /* SignedData.digestAlgorithms: DigestAlgorithmIdentifiers -> SET */ - skip_tlv(signedData, &p, SET_TAG); + if (skip_tlv(signedData, &p, SET_TAG) != 0) + return -EINVAL; /* SignedData.encapContentInfo: EncapsulatedContentInfo -> SEQUENCE */ - skip_tlv(signedData, &p, SEQUENCE_TAG); + if (skip_tlv(signedData, &p, SEQUENCE_TAG) != 0) + return -EINVAL; /* SignedData.certificates: CertificateSet -> SET */ - skip_tlv(signedData, &p, 0xA0); + if (skip_tlv(signedData, &p, 0xA0) != 0) + return -EINVAL; /* SignedData.signerInfos: SignerInfos -> SET OF SEQUENCE */ - skip_tl(signedData, &p, SET_TAG); - skip_tl(signedData, &p, SEQUENCE_TAG); + if (skip_tl(signedData, &p, SET_TAG) != 0) + return -EINVAL; + if (skip_tl(signedData, &p, SEQUENCE_TAG) != 0) + return -EINVAL; /* SignedData.signerInfos.version: CMSVersion -> INTEGER */ - skip_tlv(signedData, &p, INTEGER_TAG); + if (skip_tlv(signedData, &p, INTEGER_TAG) != 0) + return -EINVAL; /* * SignedData.signerInfos.sid: SignerIdentifier -> CHOICE -> always * subjectKeyIdentifier, which is a [0]. */ - skip_tlv(signedData, &p, 0x80); + if (skip_tlv(signedData, &p, 0x80) != 0) + return -EINVAL; /* SignedData.signerInfos.digestAlgorithm: DigestAlgorithmIdentifier * -> AlgorithmIdentifier -> SEQUENCE */ - skip_tlv(signedData, &p, SEQUENCE_TAG); + if (skip_tlv(signedData, &p, SEQUENCE_TAG) != 0) + return -EINVAL; /* SignedData.signerInfos.signedAttrs: SignedAttributes -> SET */ /* We will need to replace the tag 0xA0 with 0x31, so skip it as well */ - skip_t(signedData, &p, 0xA0); + if (skip_t(signedData, &p, 0xA0) != 0) + return -EINVAL; result->buffer = &signedData->buf[p.offset]; len_len = ber_fetch_length(true, result->buffer, p.remaining, &result->size); if (len_len == -1) - pr_crit("Could not decipher length (Cause is unknown)"); + return pr_val_err("Could not decipher length (Unknown cause.)"); if (len_len == 0) - pr_crit("Buffer seems to be truncated"); + return pr_val_err("Buffer seems truncated."); result->size += len_len; + return 0; } /* @@ -788,7 +802,9 @@ certificate_validate_signature(X509 *cert, ANY_t *signedData, * Second option it is. */ - find_signedAttrs(signedData, &signedAttrs); + error = find_signedAttrs(signedData, &signedAttrs); + if (error) + goto end; error = EVP_DigestVerifyUpdate(ctx, &EXPLICIT_SET_OF_TAG, sizeof(EXPLICIT_SET_OF_TAG)); -- 2.47.3