[Rpm-maint] [rpm-software-management/rpm] Fix IMA signature fubar, take III (#1833, RhBug:2018937) (PR #1914)

[Demi Marie Obenour]

@DemiMarie requested changes on this pull request.

`hex2bin()` also should be fixed for the out of bounds read issue; suggestion below.

> +	uint8_t *t = bin = xmalloc(((rpmtdSize(&td) / 2) + 1));
+	offs = xmalloc((num + 1) * sizeof(*offs));

For future reviewers: there is no risk of overflow, even on 32-bit systems, because of header size limits.

> @@ -111,7 +111,7 @@ rpmRC rpmSignFiles(Header sigh, Header h, const char *key, char *keypass)
 	    goto exit;
 	}
 	signature = _free(signature);
-	if (slen > siglen)
+	if (slen >= siglen)

This change is a no-op

> +	    for (int j = 0; j < len; j++, t++, s += 2)
+		*t = (rnibble(s[0]) << 4) | rnibble(s[1]);

If the length of a string is odd (which means a malformed package) this will not perform out of bounds pointer arithmetic because of the terminating NUL bytes.  That said, such packages should really be rejected when they are being loaded, but that is for another time.

> +	offs[i] = t - bin;
+	*offsetp = offs;
+
+	rpmtdFreeData(&td);
+    }
+    return bin;
+}
+
+/* Convert a tag of hex strings to binary presentation */
+static uint8_t *hex2bin(Header h, rpmTagVal tag, rpm_count_t num, size_t len)
+{
+    struct rpmtd_s td;
+    uint8_t *bin = NULL;
+
+    if (headerGet(h, tag, &td, HEADERGET_MINMEM) && rpmtdCount(&td) == num) {
+	uint8_t *t = bin = xmalloc(num * len);

This should really be an `xmallocn` from #1915, if you choose to merge that first.

>  
-	uint8_t *t = bin = xmalloc(num * maxl);
-	int i = 0;
 	while ((s = rpmtdNextString(&td))) {
 	    if (*s == '\0') {

```suggestion
	    if (strlen(s) != len * 2) {
```

This will ensure that any junk digests (that have incorrect lengths) are ignored (and then, hopefully, cause package install to fail because they do not verify).  `len` is trusted here because is the return value of `rpmDigestLength()`.

> @@ -579,10 +578,15 @@ const unsigned char * rpmfilesFSignature(rpmfiles fi, int ix, size_t *len)
     const unsigned char *signature = NULL;
 
     if (fi != NULL && ix >= 0 && ix < rpmfilesFC(fi)) {
-	if (fi->signatures != NULL)
-	    signature = fi->signatures + (fi->signaturemaxlen * ix);
+	size_t slen = 0;
+	if (fi->signatures != NULL && fi->signatureoffs != NULL) {
+	    uint32_t off = fi->signatureoffs[ix];
+	    slen = fi->signatureoffs[ix+1] - off;

I would add an `assert(fi->signatureoffs[ix+1] >= off)` here, but it is up to you.

> +	offs[i] = t - bin;
+	*offsetp = offs;
+
+	rpmtdFreeData(&td);
+    }
+    return bin;
+}
+
+/* Convert a tag of hex strings to binary presentation */
+static uint8_t *hex2bin(Header h, rpmTagVal tag, rpm_count_t num, size_t len)
+{
+    struct rpmtd_s td;
+    uint8_t *bin = NULL;
+
+    if (headerGet(h, tag, &td, HEADERGET_MINMEM) && rpmtdCount(&td) == num) {
+	uint8_t *t = bin = xmalloc(num * len);

```suggestion
	uint8_t *t = bin = xmallocn(num, len);
```

-- 
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1914#pullrequestreview-876152123
You are receiving this because you are subscribed to this thread.

Message ID: <rpm-software-management/rpm/pull/1914/review/876152123 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20220208/85e49173/attachment.html>