pld rpm 5.4.17

[Jeffrey Johnson]

> On Mar 2, 2017, at 4:05 PM, Jeffrey Johnson <n3npq at me.com> wrote:
> 
> 
>> On Mar 2, 2017, at 3:52 PM, Jakub Bogusz <qboosh at pld-linux.org <mailto:qboosh at 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