Fix IMA signature fubar, take III (#1833, RhBug:2018937)

At least ECDSA and RSA signatures can vary in length, but the IMA code
assumes constant lengths and thus may either place invalid signatures on
disk from either truncating or overshooting, and segfault if the stars are
just so.

As we can't assume static lengths and attempts to use maximum length
have proven problematic for other reasons, use a data structure that
can actually handle variable length data properly: store offsets into
the decoded binary blob and use them to calculate lengths when needed,
empty data is simply consequtive identical offsets. This avoids a whole
class of silly overflow issues with multiplying, makes zero-length data
actually presentable in the data structure and saves memory too.

Add tests to show behavior with variable length signatures and missing
signatures.

Additionally update the signing code to store the largest IMA signature
length rather than what happened to be last to be on the safe side.
We can't rely on this value due to invalid packages being out there,
but then we need to calculate the lengths on rpmfiles populate so there's
not a lot to gain anyhow.

Fixes: #1833
(cherry picked from commit 07f1d3132f0c7b7ecb69a47a9930edb534a9250e)

5 files changed, 78 insertions, 8 deletions

diff --git a/lib/rpmfi.c b/lib/rpmfi.c
index bac5c360e..1f5ca7a20 100644
--- a/lib/rpmfi.c
+++ b/lib/rpmfi.c
@@ -116,7 +116,7 @@ struct rpmfiles_s {
     struct fingerPrint_s * fps;	/*!< File fingerprint(s). */
 
     int digestalgo;		/*!< File digest algorithm */
-    int signaturelength;	/*!< File signature length */
+    uint32_t *signatureoffs;	/*!< File signature offsets */
     int veritysiglength;	/*!< Verity signature length */
     uint16_t verityalgo;	/*!< Verity algorithm */
     unsigned char * digests;	/*!< File digests in binary. */
@@ -578,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->signaturelength * ix);
+	size_t slen = 0;
+	if (fi->signatures != NULL && fi->signatureoffs != NULL) {
+	    uint32_t off = fi->signatureoffs[ix];
+	    slen = fi->signatureoffs[ix+1] - off;
+	    if (slen > 0)
+		signature = fi->signatures + off;
+	}
 	if (len)
-	    *len = fi->signaturelength;
+	    *len = slen;
     }
     return signature;
 }
@@ -1277,6 +1282,7 @@ rpmfiles rpmfilesFree(rpmfiles fi)
 	fi->flangs = _free(fi->flangs);
 	fi->digests = _free(fi->digests);
 	fi->signatures = _free(fi->signatures);
+	fi->signatureoffs = _free(fi->signatureoffs);
 	fi->veritysigs = _free(fi->veritysigs);
 	fi->fcaps = _free(fi->fcaps);
 
@@ -1506,6 +1512,48 @@ err:
     return;
 }
 
+/*
+ * Convert a tag of variable len hex strings to binary presentation,
+ * accessed via offsets to a contiguous binary blob. Empty values
+ * are represented by identical consequtive offsets. The offsets array
+ * always has one extra element to allow calculating the size of the
+ * last element.
+ */
+static uint8_t *hex2binv(Header h, rpmTagVal tag, rpm_count_t num,
+			uint32_t **offsetp)
+{
+    struct rpmtd_s td;
+    uint8_t *bin = NULL;
+    uint32_t *offs = NULL;
+
+    if (headerGet(h, tag, &td, HEADERGET_MINMEM) && rpmtdCount(&td) == num) {
+	const char *s;
+	int i = 0;
+	uint8_t *t = bin = xmalloc(((rpmtdSize(&td) / 2) + 1));
+	offs = xmalloc((num + 1) * sizeof(*offs));
+
+	while ((s = rpmtdNextString(&td))) {
+	    uint32_t slen = strlen(s);
+	    uint32_t len = slen / 2;
+	    if (slen % 2) {
+		bin = rfree(bin);
+		offs = rfree(offs);
+		goto exit;
+	    }
+	    offs[i] = t - bin;
+	    for (int j = 0; j < len; j++, t++, s += 2)
+		*t = (rnibble(s[0]) << 4) | rnibble(s[1]);
+	    i++;
+	}
+	offs[i] = t - bin;
+	*offsetp = offs;
+    }
+
+exit:
+    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)
 {
@@ -1658,9 +1706,8 @@ static int rpmfilesPopulate(rpmfiles fi, Header h, rpmfiFlags flags)
     fi->signatures = NULL;
     /* grab hex signatures from header and store in binary format */
     if (!(flags & RPMFI_NOFILESIGNATURES)) {
-	fi->signaturelength = headerGetNumber(h, RPMTAG_FILESIGNATURELENGTH);
-	fi->signatures = hex2bin(h, RPMTAG_FILESIGNATURES,
-				 totalfc, fi->signaturelength);
+	fi->signatures = hex2binv(h, RPMTAG_FILESIGNATURES,
+				 totalfc, &fi->signatureoffs);
     }
 
     fi->veritysigs = NULL;
diff --git a/sign/rpmsignfiles.c b/sign/rpmsignfiles.c
index b143c5b9b..372ba634c 100644
--- a/sign/rpmsignfiles.c
+++ b/sign/rpmsignfiles.c
@@ -98,8 +98,9 @@ rpmRC rpmSignFiles(Header sigh, Header h, const char *key, char *keypass)
     td.count = 1;
 
     while (rpmfiNext(fi) >= 0) {
+	uint32_t slen = 0;
 	digest = rpmfiFDigest(fi, NULL, NULL);
-	signature = signFile(algoname, digest, diglen, key, keypass, &siglen);
+	signature = signFile(algoname, digest, diglen, key, keypass, &slen);
 	if (!signature) {
 	    rpmlog(RPMLOG_ERR, _("signFile failed\n"));
 	    goto exit;
@@ -110,6 +111,8 @@ rpmRC rpmSignFiles(Header sigh, Header h, const char *key, char *keypass)
 	    goto exit;
 	}
 	signature = _free(signature);
+	if (slen > siglen)
+	    siglen = slen;
     }
 
     if (siglen > 0) {
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 6d41ef93c..0840075fb 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -102,6 +102,7 @@ EXTRA_DIST += data/RPMS/hello-2.0-1.i686.rpm
 EXTRA_DIST += data/RPMS/hello-2.0-1.x86_64.rpm
 EXTRA_DIST += data/RPMS/hello-2.0-1.x86_64-signed.rpm
 EXTRA_DIST += data/RPMS/hlinktest-1.0-1.noarch.rpm
+EXTRA_DIST += data/RPMS/imatest-1.0-1.fc34.noarch.rpm
 EXTRA_DIST += data/SRPMS/foo-1.0-1.src.rpm
 EXTRA_DIST += data/SRPMS/hello-1.0-1.src.rpm
 EXTRA_DIST += data/SOURCES/hello.c
diff --git a/tests/data/RPMS/imatest-1.0-1.fc34.noarch.rpm b/tests/data/RPMS/imatest-1.0-1.fc34.noarch.rpm
new file mode 100644
index 000000000..9b47cf9dd
--- /dev/null
+++ b/tests/data/RPMS/imatest-1.0-1.fc34.noarch.rpm
diff --git a/tests/rpmpython.at b/tests/rpmpython.at
index 8128263e3..c8a52d469 100644
--- a/tests/rpmpython.at
+++ b/tests/rpmpython.at
@@ -602,6 +602,25 @@ for f in fi:
 ],
 [])
 
+RPMPY_TEST([file sets 1],[
+ts = rpm.ts()
+for p in ['imatest-1.0-1.fc34.noarch.rpm', 'capstest-1.0-1.noarch.rpm']:
+  h = ts.hdrFromFdno('${RPMDATA}/RPMS/%s' % p)
+  files = rpm.files(h)
+  for f in files:
+     sig = f.imasig
+     if sig:
+        sig = sig.hex()
+     myprint('%s: %s' % (f.name, sig))
+],
+[/usr/share/example1: 030204a598255400483046022100e5117bdafa73baaeb1f1dc46ecaa46981a62d417745a33532572b63dc6d95d16022100c789107ac5b91e2d915e1df3c7b78414f6b3f50899d44c1de381d0e938dfc82b
+/usr/share/example2: 030204a598255400473045022100c10943795bff5d9c0db53dd4f8e4b845615fd08a2be295c30a80f5bdb4e6a41302203038840cc6abaab92acb56cb3e3ce520b17f22ff7444a8d5d0f703a44d5307a3
+/a/emptyCaps1: None
+/a/emptyCaps2: None
+/a/noCaps: None
+],
+[])
+
 RPMPY_TEST([string pool 1],[
 p = rpm.strpool()
 for s in ['foo', 'bar', 'foo', 'zoo']: