From 5ff0b96adf5f43651f123acd2f83274dba920374 Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Mon, 7 Feb 2022 13:38:48 +0200 Subject: 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) --- lib/rpmfi.c | 61 +++++++++++++++++++++++--- sign/rpmsignfiles.c | 5 ++- tests/Makefile.am | 1 + tests/data/RPMS/imatest-1.0-1.fc34.noarch.rpm | Bin 0 -> 5543 bytes tests/rpmpython.at | 19 ++++++++ 5 files changed, 78 insertions(+), 8 deletions(-) create mode 100644 tests/data/RPMS/imatest-1.0-1.fc34.noarch.rpm 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 Binary files /dev/null and b/tests/data/RPMS/imatest-1.0-1.fc34.noarch.rpm differ 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']: -- cgit v1.2.1