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.

diff --git a/src/object/certificate.c b/src/object/certificate.c
index 2708c66d35a75bf6e859b06f6038c86a4108a997..1552afeda0faf13b1ced1605fb025ac8b2c5444a 100644 (file)
--- 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));