Okay so it creates an inaccessible header in the rpmdb, with indexes pointing to it:
error: rpmdbNextIterator: skipping h#       1 tag[61]: BAD, tag 261 type 6 offset 6989 count 1 len 65

Obviously this is not okay at all, but as far as database corruptions go this is relatively tame and fixing is easy: just don't copy any unexpected tag entries from the signature.

Doh, forgot to mention: getting rid of that inaccessible header is a simple matter of 'rpmdb --rebuilddb'. A header disappearing might break dependencies on the db-level etc but that's also easily fixable by just reinstalling the original package. So it's not anything like a "total data loss" corruption.

For all practical purposes, this is essentially the same as bug 1927747.

Thanks for the reproducer.

Created attachment 1760192 [details]
Avoid copying unknown tags

Unknown tags are not a legit reason for rpm to reject a package, doing so would be devastating to compatibility. However we have no legit reason to copy them either. While there are many more hardening tricks than can be done, this simple patch is enough to take off the edge and is trivially backportable throughout all rpm 4.x versions if need be.

(In reply to Panu Matilainen from comment #3)
> For all practical purposes, this is essentially the same as bug 1927747.
> 
> Thanks for the reproducer.

You’re welcome!

(In reply to Panu Matilainen from comment #4)

> Created attachment 1760192 [details]
> Avoid copying unknown tags
> 
> Unknown tags are not a legit reason for rpm to reject a package, doing so
> would be devastating to compatibility. However we have no legit reason to
> copy them either. While there are many more hardening tricks than can be
> done, this simple patch is enough to take off the edge and is trivially
> backportable throughout all rpm 4.x versions if need be.

I will test locally and make sure this fixes it.  Thanks for the quick
response!

Created attachment 1760275 [details]
Another crafted RPM package

This also triggers the bug.

(In reply to Demi Marie Obenour from comment #5)
> (In reply to Panu Matilainen from comment #3)
> > For all practical purposes, this is essentially the same as bug 1927747.
> > 
> > Thanks for the reproducer.
> 
> You’re welcome!
> 
> (In reply to Panu Matilainen from comment #4)
> 
> > Created attachment 1760192 [details]
> > Avoid copying unknown tags
> > 
> > Unknown tags are not a legit reason for rpm to reject a package, doing so
> > would be devastating to compatibility. However we have no legit reason to
> > copy them either. While there are many more hardening tricks than can be
> > done, this simple patch is enough to take off the edge and is trivially
> > backportable throughout all rpm 4.x versions if need be.
> 
> I will test locally and make sure this fixes it.  Thanks for the quick
> response!

The original reproducer no longer works, but I was able to craft a different
package that exhibits the same symptoms.  I suspect that the culprit is that
we don’t type- or length- check unverified entries in the signature header.

BTW, there is another security bug in RPM: if any trusted key signs a package
with a name starting with `gpg-pubkey`, anyone can use the RPMTAG_PUBKEYS
header to inject arbitrary trusted GPG keys into the database.  Your first
patch fixes that bug, too, but I will still file a separate report.

(In reply to Demi Marie Obenour from comment #7)
> The original reproducer no longer works, but I was able to craft a different
> package that exhibits the same symptoms.  I suspect that the culprit is that
> we don’t type- or length- check unverified entries in the signature header.

Please include any such reproducers you come up with, they're very valuable material.

And yes, it's by no means a complete fix, like said it merely takes off the edge (by limiting the surface to known tags). I'm still contemplating on how to best deal with this from the perspective that in the worst case we might need to backport to some *very* old and ugly rpm versions.

> but I will still file a separate report.

Please do, discussing multiple issues in a single report makes it impossible to keep track. As a rule of thumb, it's better to open a new bug at once if there's a reason to believe it's a separate issue. Closing bugs as duplicates if it turns out that way is far easier and the lesser evil compared to discussing/tracking multiple issues in one bug.

> Please include any such reproducers you come up with, they're very valuable material.

Doh, never mind - you already did. Thanks!

/me needs more coffee...

This is actually a pretty funny case in that it's the newly added "security" checks in rpm >= 4.14 that cause it to fail to reload a header it just produced. Older rpm versions just shrug and move on, but that in turn can lead to different kind of problems.

Created attachment 1760682 [details]
A more complete, mostly backportable patch

Only look for known tags, and ensure correct type before copying.
While at it, ensure none of these tags exist in the main header,
which would confuse us greatly.

In other words, close the gaping hole between the signature and main header.
This is the minimal backport effort approach, upstream can look to do more once all the surrounding embargoes are cleared some day.

Fixes: RhBug:1935049, RhBug:1933867, RhBug:1935035, RhBug:1934125, ...
Fixes: CVE-2021-3421, ...

Created attachment 1760843 [details]
Further (final?) revision, now with data count checks

A further revision (hopefully final) which includes additional checks for exact data counts for the items where it's possible. Upstream could do more, but this is optimized for backporting.

(In reply to Panu Matilainen from comment #12)
> Created attachment 1760843 [details]
> Further (final?) revision, now with data count checks
> 
> A further revision (hopefully final) which includes additional checks for
> exact data counts for the items where it's possible. Upstream could do more,
> but this is optimized for backporting.

Thank you!  Trivial nit: right now, if I understand correctly, this will cause
the header to be re-sorted several times.  Not sure if that matters, though.
RPMSIGTAG_PGP5 could also cause problems; should we just reject it in the
main header?

This does not fix bug 1915990, as DNF is still vulnerable if a package
does not have payload digests in the main header, or if `%_pkgverify_level`
is set to `none`.

(In reply to Demi Marie Obenour from comment #13)
> Trivial nit: right now, if I understand correctly, this will cause
> the header to be re-sorted several times. 

Why would it do that?

> RPMSIGTAG_PGP5 could also cause problems; should we just reject it in the
> main header?

Rpm does not use the tag for anything, and we dont even carry a type for it. Seems pretty harmless to me.

> 
> This does not fix bug 1915990, as DNF is still vulnerable if a package
> does not have payload digests in the main header, or if `%_pkgverify_level`
> is set to `none`.

This makes the reproducer package in bug 1915990 unreadable regardless of all verification switches, so yes it does fix it.

(In reply to Demi Marie Obenour from comment #13)
> Trivial nit: right now, if I understand correctly, this will cause
> the header to be re-sorted several times. 

Actually - it does get re-sorted several times, but that's not introduced in this patch. It always did headerIsEntry() on the main header which interleaved with headerPut()'s.

(In reply to Panu Matilainen from comment #14)
> > RPMSIGTAG_PGP5 could also cause problems; should we just reject it in the
> > main header?
> 
> Rpm does not use the tag for anything, and we dont even carry a type for it.
> Seems pretty harmless to me.

My worry is old 3rd-party code that queries for it.

> > This does not fix bug 1915990, as DNF is still vulnerable if a package
> > does not have payload digests in the main header, or if `%_pkgverify_level`
> > is set to `none`.
> 
> This makes the reproducer package in bug 1915990 unreadable regardless of
> all verification switches, so yes it does fix it.

Yes and no.  It is still possible to fool DNF if there is a signed package
with no payload digests in the main header, as rpmReadPackageFile() doesn’t
check the header+payload signature or the payload digests.  But that is a
separate bug.

*** Bug 1935049 has been marked as a duplicate of this bug. ***

*** Bug 1935035 has been marked as a duplicate of this bug. ***

Argh. Please do not mix up issues! Throwing "but it doesn't fix" claims on something entirely different is really quite harmful to getting these processed up.

> My worry is old 3rd-party code that queries for it.

They don't. That tag is some 20 years unused.

FEDORA-2021-8d52a8a999 has been submitted as an update to Fedora 33. https://bodhi.fedoraproject.org/updates/FEDORA-2021-8d52a8a999

FEDORA-2021-8d52a8a999 has been pushed to the Fedora 33 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --advisory=FEDORA-2021-8d52a8a999`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-8d52a8a999

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2021-8d52a8a999 has been pushed to the Fedora 33 stable repository.
If problem still persists, please make note of it in this bug report.