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
deef7b7, the code should not make
reckless assumptions about the signedAttrs encoding.

Thanks to Niklas Vogel for reporting this.

src/object/certificate.c

@@ -585,65 +585,67 @@ 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;
 	int skip;
 
 	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));