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.

Sign up for GitHub

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

Jump to bottom

Check that old-format packet lengths are correct #1711

Merged
merged 2 commits into from Nov 1, 2021
Merged

Check that old-format packet lengths are correct #1711

merged 2 commits into from Nov 1, 2021

Conversation

DemiMarie
Copy link
Contributor

@DemiMarie DemiMarie commented Jun 17, 2021 •
edited

If an old-format subpacket claimed to have a length that is too large for the buffer, the code would not detect it and ​silently accept the packet. This adds a stricter check for that case, with a regression test.

An earlier version of this PR suggested that this is a security vulnerability. It is not, but it is still a bug and should be fixed.

Fixes #1777.

@pmatilai
Copy link
Member

FWIW, this is an example of a case where it's probably would've been better to just use a single PR for these two closely related changes, because now we just have a conflict instead.

@DemiMarie
Copy link
Contributor Author

FWIW, this is an example of a case where it's probably would've been better to just use a single PR for these two closely related changes, because now we just have a conflict instead.

Good to know, thanks!

@DemiMarie
Copy link
Contributor Author

I chose to make pgpGet() take a size_t * argument, mostly for consistency with pgpLen(). I would be fine with a different choice, though.

@DemiMarie
Copy link
Contributor Author

@pmatilai please let me know if you would prefer pgpGet() to take a different pointer type for its argument.

@DemiMarie DemiMarie changed the title Use pgpGet() instead of manual bounds checks Simplify bounds check in old-format packet parsing Jun 30, 2021
@DemiMarie DemiMarie closed this Aug 5, 2021
@DemiMarie DemiMarie changed the title Simplify bounds check in old-format packet parsing Fix out-of-bounds read parsing old-format packets Aug 5, 2021
@DemiMarie DemiMarie reopened this Aug 5, 2021
@DemiMarie
Copy link
Contributor Author

@pmatilai @ffesti ping

@DemiMarie
Simplify bounds check in old-format packet parsing
0c9490d 
a44f026 added additional bounds checks
to pgpGet().  Use these checks to simplify the old-format packet parsing
code by using pgpGet() instead of a manual bounds check.
@DemiMarie
Add test for OpenPGP packet parsing bug
76c4d95 
This checks that old-format OpenPGP packets with excessive lengths are
rejected.
@DemiMarie
Copy link
Contributor Author

Rebased on master and added a regression test.

@DemiMarie DemiMarie mentioned this pull request Sep 18, 2021
Closed
@DemiMarie DemiMarie changed the title Fix out-of-bounds read parsing old-format packets Check that old-format subpacket lengths are correct Oct 21, 2021
@DemiMarie DemiMarie changed the title Check that old-format subpacket lengths are correct Check that old-format packet lengths are correct Oct 21, 2021
@pmatilai
Copy link
Member

pmatilai commented Nov 1, 2021

If in some alternative reality time travel is understood by 2007, note to self back then: size_t is silly here 🙄

Anyway, having looked at it a bit more, while I'd prefer unsigned int, that'd need to be changed (back) consistently throughout the codebase and ... seems just too much trouble at the moment. Thanks for the patch.

@pmatilai pmatilai merged commit ad97e2f into rpm-software-management:master Nov 1, 2021
@DemiMarie DemiMarie deleted the decodePkt-pgpGet branch November 1, 2021 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bogus OpenPGP packet not correctly rejected
2 participants
@DemiMarie @pmatilai