From f53028009850015054e3de208f5190455412d7cb Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Mon, 7 Feb 2022 13:38:48 +0200 Subject: [PATCH] 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 handled with special zero-block at zero offset whose size is known beforehand. 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. 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 --- lib/rpmfi.c | 67 +++++++++++++++--- tests/Makefile.am | 1 + tests/data/RPMS/imatest-1.0-1.fc34.noarch.rpm | Bin 0 -> 5543 bytes tests/rpmpython.at | 11 +++ 4 files changed, 71 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 b67680c172..430bf046b6 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,18 @@ 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); - if (len) - *len = fi->signaturelength; + if (fi->signatures != NULL && fi->signatureoffs != NULL) { + uint32_t off = fi->signatureoffs[ix]; + size_t slen = 0; + + if (off != 0) { + signature = fi->signatures + off; + slen = fi->signatureoffs[ix+1] - off; + } + + if (len) + *len = slen; + } } return signature; } @@ -1277,6 +1285,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 +1515,49 @@ static void rpmfilesBuildNLink(rpmfiles fi, Header h) return; } +/* + * Convert a tag of variable len hex strings to binary presentation, + * accessed via offsets to a contiguous binary blob. Empty strings are + * special-cased to a block of zeros at offset 0 which makes them easy + * to distinguish elsewhere. 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; + int nzeros = 16; + + 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) + nzeros); + offs = xmalloc((num + 1) * sizeof(*offs)); + + memset(t, 0, nzeros); + t += nzeros; + + while ((s = rpmtdNextString(&td))) { + uint32_t len = strlen(s) / 2; + if (len == 0) { + offs[i] = 0; + } else { + 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; + + 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) { @@ -1654,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/tests/Makefile.am b/tests/Makefile.am index 60fd7cc269..f78e17c3e9 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 0000000000000000000000000000000000000000..9b47cf9dd09ba46532b0ee30d99ca209f9e54fd9 GIT binary patch literal 5543 zcmeHLO^h5z6|Py&5@+oQJ0Xzpqbx`uuSm~ybyxpcyJnMhY%hv-S6MGOR=}yMuG$%U zc6#XUiPx4uwv-4XK@KDWAtA`YLP(GZBz_FaAtGRciP3Y91+t>{xKIQoJ!0qVA(Z?Xa0oVk70|@~H(ClRxF;e{u3lj9vTH)L0$~?dLL$BI&C*4y;at(5iq3Dw27S`6dxL z3#AE!PGziRn#NXHErm_ODEGrCOkxo$D5OI1#40YsJP!gTQ)NY%@stOK`Jo^9LPl8{ zM3L})k%qpA{UG4JZz9eUl}js=P)TVNw>;M(Hd;y%XDOF)kPDt9B9bBI3!)@YahU5k z&!U_=D>_Lt?k7@5D&r=|Gaeb9+fXMeNd*@+R-ujjG|O@!G|x?#^4vFBnC7%*^`9;# za536+QFA{NVUk74+9=NuF%@Bz!lp*3R75e4Z0@HrP$tMC9r`l$ZKQG~Oe_V*;sZn= zOjF@&9ZRfIWr5TxinR(&s7)036ucm?HW5O|G)HW0;)ha*JPNU{5F3ZKgnheZBKhs$ z6VIAgw!e4$th{sxd))Zqrg-bR6@je?Y(-!z0$UN-iojL`wj%I<7=cj^u(7c*fN%Il zXY7hRA!Tfwa)J+_<2%0j8jy5ybjky$UHbV15P1c=)p5!j$e!=`Ux6fFa{TW=k{@>bWyh7{uK;Pj&p7@nko3RcIIWBL zV~+pBahJz5X)fxIqhpOt(xp`GhmMnN;*{5rd;&=N$ZJNN)=lyHx#K%GdE-NlQ;ebC zIO6!5o%~+MDdxDp;qtELTfpH%qXr^=%^QJq|EGa8|J#71|FDzammM8$KC!l6*&3-eIVJ(H+gf)apQQ)aqBpx zHKgBjJa(Mo4F8(+ywQ9^$H{Nv_W|K=^IjnCzu_Su>60De7lH7nN%0{5dm!o4eZ>C& zB>fAHKkwv^6Q5&L4`8SVppWVShruTYQlF5khP{r+M?QOT!PJLKd7tT-vehZpOlvUg z)}}n!J$|B9l&dhw-nukYy{?|zU#!V)pDoF{?CKMfE0^^qrw5xuv#-B*A6w~H1EagP zYjW19>p``*-8SoTZO}6E>~4xT2-lAf!PKhW)&1-WLbDduD}vcJ2EE&TV`l04NblP=TuZ~uAKE=Y8-x3~Yq7_q(2uRncC0aZkQ69H=Ym}nPM=s62f!+07Ot2(c z+Nk6C9<}ZNtsv#Lo-_9+VC`X5wyTaTO`Ca(!K7zcvwVk`X=|Z=5V#DapJr|S=8{oD7~sKi#~k*pLS%wU#M<>WMEMIY*Rj^SpZw{Q4_wc_ dJ2pFh?y>j%ar*O5{N|IZ_QxwPEL=FV`=2+Mh#LR^ literal 0 HcmV?d00001 diff --git a/tests/rpmpython.at b/tests/rpmpython.at index 4b801447cd..8289d84c61 100644 --- a/tests/rpmpython.at +++ b/tests/rpmpython.at @@ -624,6 +624,17 @@ for f in fi: ], []) +RPMPY_TEST([file sets 1],[ +ts = rpm.ts() +h = ts.hdrFromFdno('${RPMDATA}/RPMS/imatest-1.0-1.fc34.noarch.rpm') +files = rpm.files(h) +for f in files: + myprint('%s: %s' % (f.name, f.imasig.hex())) +], +[/usr/share/example1: 030204a598255400483046022100e5117bdafa73baaeb1f1dc46ecaa46981a62d417745a33532572b63dc6d95d16022100c789107ac5b91e2d915e1df3c7b78414f6b3f50899d44c1de381d0e938dfc82b +/usr/share/example2: 030204a598255400473045022100c10943795bff5d9c0db53dd4f8e4b845615fd08a2be295c30a80f5bdb4e6a41302203038840cc6abaab92acb56cb3e3ce520b17f22ff7444a8d5d0f703a44d5307a3 +], +[]) RPMPY_TEST([string pool 1],[ p = rpm.strpool() for s in ['foo', 'bar', 'foo', 'zoo']: