Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be much more careful about copying data from the signature header #1577

Merged
merged 2 commits into from Mar 16, 2021

Conversation

pmatilai
Copy link
Member

Only look for known tags, and ensure correct type and size where known
before copying over. Bump the old arbitrary 16k count limit to 16M limit
though, it's not inconceivable that a package could have that many files.
While at it, ensure none of these tags exist in the main header,
which would confuse us greatly.

This is optimized for backporting ease, upstream can remove redundancies
and further improve checking later.

Fixes: RhBug:1935049, RhBug:1933867, RhBug:1935035, RhBug:1934125, ...

Fixes: CVE-2021-3421, CVE-2021-20271

@pmatilai
Copy link
Member Author

(reporter credits added to commit message)

Copy link
Contributor

@DemiMarie DemiMarie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I included a couple of minor performance suggestions, but those should not delay merging.

lib/package.c Outdated Show resolved Hide resolved
lib/package.c Show resolved Hide resolved
lib/package.c Outdated Show resolved Hide resolved
@pmatilai
Copy link
Member Author

I included a couple of minor performance suggestions, but those should not delay merging.

But that's exactly what such things tend to do, as I'm now wondering could there be some quirks code, especially very old versions, that cause it to actually rely on the put-sort cycle. It's extremely unlikely, but there have been stranger things (see commit da3a3a1)

pmatilai and others added 2 commits March 16, 2021 11:38
Only look for known tags, and ensure correct type and size where known
before copying over. Bump the old arbitrary 16k count limit to 16M limit
though, it's not inconceivable that a package could have that many files.
While at it, ensure none of these tags exist in the main header,
which would confuse us greatly.

This is optimized for backporting ease, upstream can remove redundancies
and further improve checking later.

Reported and initial patches by Demi Marie Obenour.

Fixes: RhBug:1935049, RhBug:1933867, RhBug:1935035, RhBug:1934125, ...

Fixes: CVE-2021-3421, CVE-2021-20271
Look up possible offending tags from the main header first in a separate
loop, this avoids having to re-sort after each headerPut() operation.
@pmatilai
Copy link
Member Author

Split the optimization part to a separate commit so the first one can be safely picked for old versions.
I merged the MINMEM part to the first commit though - I had some kind of reason to not put it there in the first place (probably just that MINMEM is a bit iffy in the first place), but thinking it again, MINMEM is what the header iteration does so in a way this is closer to the original behavior.

@pmatilai
Copy link
Member Author

Mind you, the suggested optimization to avoid multiple sorts totally makes sense and never occurred to me at all (too much staring at how it always did it), so thanks for that! Just that with critical fixes needing backports and all, other enhancements such as performance are best kept apart - I planned to do a PR for that separately sooner or later.

@pmatilai pmatilai merged commit f7b9759 into rpm-software-management:master Mar 16, 2021
@pmatilai pmatilai deleted the sigcve-pr branch June 21, 2021 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants