pld rpm 5.4.17

[Jakub Bogusz]

On Tue, Feb 28, 2017 at 11:18:01PM -0500, Jeffrey Johnson wrote:
> 
> > On Feb 28, 2017, at 3:42 PM, Jakub Bogusz <qboosh at pld-linux.org> wrote:
> > 
> > On Thu, Feb 23, 2017 at 07:15:22PM -0500, Jeffrey Johnson wrote:
> >> 
> >>> On Feb 23, 2017, at 3:36 PM, Jakub Bogusz <qboosh at pld-linux.org> wrote:
> >>> 
> >>> On Thu, Feb 23, 2017 at 01:43:14PM -0500, Jeffrey Johnson wrote:
> >>>> This one is left though:
> >>>> 
> >>>>> error: db3: header #187105280 cannot be loaded -- skipping.
> >>>>> error: db3: header #4127850496 cannot be loaded -- skipping.
> >>>> 
> >>>> How to check what these "headers" mean?
> >>>> (old, unsupported keys? some old packages with missing fields which are
> >>>> now required?)
> >>>> The error message is printed on a headerCopyLoad() failure.
> >>>> 
> >>>> The failure is usually an indication of header damage of some sort, not missing fields now required.
> >>>> 
> >>>> The numbers  are  primary keys into Packages printed in in decimal.
> >>>> 
> >>>> The values appear to be in the wrong-endian order when converted to hex
> >>>> 	0xB270000
> >>>> 	0xF60A0000
> >>>> 
> >>>> You can try finding the header by doing, say, rpm -q ???querybynumber 0xB270000
> >>>> (or its reverse: its unclear what order).
> >>> 
> >>> Same result for bigger values, no result after swapping bytes:
> >>> 
> >>> -bash-4.4# rpm -q --querybynumber 0xF60A0000
> >>> error: rpmdb: header #4127850496 cannot be loaded -- skipping.
> >>> -bash-4.4# rpm -q --querybynumber 0x00000AF6
> >>> -bash-4.4# rpm -q --querybynumber 0xB270000
> >>> error: rpmdb: header #187105280 cannot be loaded -- skipping.
> >>> -bash-4.4# rpm -q --querybynumber 0x0000270B
> >>> -bash-4.4#
> >>> 
> >> 
> >> So at least simply reproducible with ???querybynumber ;-)
> >> 
> >> If you just need a fix, db_dump will give you {KEY, VALUE} pairs in hex.
> >> Find the 2 {KEY,VALUES} and delete the 2 occurrences of 2 lines of hex in a text editor.
> >> Feed the result to db_load to recreate Packages.
> >> rpm ???rebuild will recreate the indices.
> > 
> > I investigated these two entries. They doesn't seem to be invalid, but
> > are not accepted after this change:
> > 
> > http://rpm5.org/cvs/filediff?f=rpm/rpmdb/header.c&v1=1.198.2.23&v2=1.198.2.24 <http://rpm5.org/cvs/filediff?f=rpm/rpmdb/header.c&v1=1.198.2.23&v2=1.198.2.24>
> > 
> > more precisely, this part:
> > 
> > -/*@=sizeoftype@*/
> > -               rpmuint32_t * stei = (rpmuint32_t *)
> > -                       memcpy(alloca(nb), dataStart + off, nb);
> > +               /* XXX copy to fix alignment problems */
> > +                rpmuint32_t * stei = (rpmuint32_t *)
> > +                          memcpy(alloca(nb), dataStart + off, nb);
> > +               if ((off + nb) > dl)
> > +                   goto errxit;
> >                rdl = (rpmuint32_t)-ntohl(stei[2]);     /* negative offset */
> > -assert((rpmint32_t)rdl >= 0);  /* XXX insurance */
> > -               ril = (rpmuint32_t)(rdl/sizeof(*pe));
> > -               if (hdrchkTags(ril) || hdrchkData(rdl))
> > +               if (rdl < REGION_TAG_COUNT || rdl > (rpmuint32_t)(off+nb))
> >                    goto errxit;
> > 
> > (nb. IMO "if ((off + nb) > dl)" check should be done before memcpy???)
> 
> Ah yes ??? fixing problems with AFL (American Fuzzy Lop) was totally annoying there.
> You can tell because of the remnant printf???s that do not align with generally accepted
> C coding practices.
> 
> Meanwhile, we may be working at cross purposes, with packages that have already been installed
> by older (or heavily patched) versions of rpm.
> 
> What SHOULD have prevented that problem is verifying (always) the header SHA1 while installing
> which ensures the header was structurally intact, preventing the later failure of headerCopyLoad() that
> was displayed as an error message.
> 
> But perhaps there is something better that can be done: O looked at that specific code path
> for a couple of days with AFL fuzzed headers: what you see is my best effort (by _ALWAYS_
> verifying the header SHA1. The risk of adding stricter tests is that previously known good
> headers might fail.
> 
> If you have a better solution (other than disabling stricter tests), I???ll be happy to add to

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.

IMO the checks should be like in the attached patch.
With it, the two refused packages are accessible again.


-- 
Jakub Bogusz    http://qboosh.pl/
-------------- next part --------------
--- rpm-5.4.17/rpmdb/header.c.orig	2017-02-25 09:37:52.627550403 +0100
+++ rpm-5.4.17/rpmdb/header.c	2017-03-02 21:12:16.348808677 +0100
@@ -998,14 +998,15 @@
 	    if (off < 0)
 		goto errxit;
 	    if (off) {
+                rpmuint32_t * stei;
 		size_t nb = REGION_TAG_COUNT;
-		/* XXX copy to fix alignment problems */
-                rpmuint32_t * stei = (rpmuint32_t *)
-                          memcpy(alloca(nb), dataStart + off, nb);
 		if ((off + nb) > dl)
 		    goto errxit;
+		/* XXX copy to fix alignment problems */
+                stei = (rpmuint32_t *)
+                          memcpy(alloca(nb), dataStart + off, nb);
 		rdl = (rpmuint32_t)-ntohl(stei[2]);	/* negative offset */
-		if (rdl < REGION_TAG_COUNT || rdl > (rpmuint32_t)(off+nb))
+		if (rdl < REGION_TAG_COUNT || rdl > (rpmuint32_t)(il * REGION_TAG_COUNT))
 		    goto errxit;
 		ril = (rpmuint32_t)(rdl/sizeof(*pe));
 	    } else {