> On Mar 2, 2017, at 4:05 PM, Jeffrey Johnson <n3...@me.com> wrote: > > >> On Mar 2, 2017, at 3:52 PM, Jakub Bogusz <qbo...@pld-linux.org >> <mailto:qbo...@pld-linux.org>> wrote: >> >> >> As far as I understand the code, rdl is size of immutable entry infos >> part, while off is an offset in tags data part. >> And when immutable tags data is short enough (shorter than entry infos >> of immutable part), this check refuses to load header. >> > > Yes. there is a “immutable region” header and trailer, where the > offset field is used as a double check on the tags in the immutable region. > >> IMO the checks should be like in the attached patch. >> With it, the two refused packages are accessible again. >> > > I’ve applied the patch and will do a few tests before checking in. > > One item I note (just scanning the patch) is > > - if (rdl < REGION_TAG_COUNT || rdl > (rpmuint32_t)(off+nb)) > + if (rdl < REGION_TAG_COUNT || rdl > (rpmuint32_t)(il * > REGION_TAG_COUNT)) > > The variable il is derived and may be tainted, while off and nb are de facto > positioning > within the header memory blob. And yes, it may not matter.
(after spending some time resurrecting my ancient implementation memories) The test above prevents nested “immutable regions” (but works fine for the only case that matters: a single “immutable region”. Its not so much tainting, but rather the total number of index entries, not the number of index entries in the immutable region, that is stored in il. The original intent was to be able to embed nested regions in order to make appended tags (there are several) also immutable (and verifiable with digest/signature). I can try to describe with pictures (but it may just make matters worse): A = 16b index a = variable length data region associated with tag A So a (RPMv3) header looked like (white space added for readability) ABCD abcd Now assume X,Y = immutable region tag begin markers x,y = immutable region tag end trailers So the usual Header (for a long time now) looks like X ABCD abcd x where ABCD are tags, and X is the region marker. After installation other tags {J, K, L} are appended, so a header looks like X ABCD JKL abcd x jkl permitting the original immutable region plaintext to be recreated to verify digest/signature even after installation. A header with nested immutable regions would then look like Y X ABCD QRST abcd x qrst y where Q,R,S,T are tags associated with the outer immutable region. I hope the above explains the very obscure implementation. I’ll rework your patch slightly to do the argument check before doing the memcpy but preserve the original check on {off,nb}. I prefer sticking with “last known good” until there is a clearer indication that something needs to be changed. Meanwhile — if you have either of the 2 failing headers — I can attempt to do forensics on what the root cause failure for headerCopyLoad() is, and perhaps guess at what happened. hth _______________________________________________ pld-devel-en mailing list pld-devel-en@lists.pld-linux.org http://lists.pld-linux.org/mailman/listinfo/pld-devel-en