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

Avoid parsing key usage flags from hashed key ID subpackets #2026

Merged

Conversation

DemiMarie
Copy link
Contributor

@DemiMarie DemiMarie commented Apr 14, 2022

A key ID subpacket is not a key usage flags subpacket and must not be
interpreted as such. This caused RPM to wrongly reject a signature that
had both key ID and key usage flags subpackets in the hashed section.

@pmatilai I believe this fixes a regression in 4.18 alpha.

@DemiMarie DemiMarie force-pushed the add-missing-break branch 3 times, most recently from e5e9cca to d5b13d0 Compare April 14, 2022 21:00
@DemiMarie DemiMarie changed the title Add missing break Avoid parsing key usage flags from hashed key ID subpackets Apr 14, 2022
@DemiMarie DemiMarie force-pushed the add-missing-break branch 2 times, most recently from a3f7bbb to 147f2e0 Compare April 14, 2022 21:05
@nwalfield
Copy link
Contributor

The missing break is obviously a bug.

It appears that the missing break has existed since support for the issuer subpacket was added in 2001 (3b820b2#diff-b4eac15fda646a3b73f5cd251f33387979eadc71ba52f769bd64b10bd877365cL470). That commit included a comment that control was supposed to fall through, but I think using a comment instead of breaking was misguided. Unfortunately, the comment was later removed :/.
When @DemiMarie added support for the KEY_FLAGS subpacket in 598a771, she understandably missed that the issuer subpacket handling didn't have its own break statement.

I agree that this fixes a bug, the patch should be applied, and, due to unfortunate circumstances, represents a regression in 4.18 alpha.

@nwalfield
Copy link
Contributor

FWIW, I think the commit message should just be 'Add missing break statement, regression introduced in ...'

@DemiMarie
Copy link
Contributor Author

@nwalfield is the new commit message better? @pmatilai friendly ping, and apologies for getting it wrong the first time. @nwalfield thanks for the testing that caught this before it made it into a release.

@nwalfield
Copy link
Contributor

I think the commit message is better, but in the end @pmatilai needs to decide what is best.

@pmatilai
Copy link
Member

The fact that it's a regression fix should be stated in the summary line. That's what people will look at first when looking for relevant changes, and "add missing break" says nothing about the severity of it.

@pmatilai
Copy link
Member

Something like "Fix OpenPGP key ID parsing regression" would be more like the headline that such a thing needs. The rest of the message is fine.

This fixes a regression in 598a771,
which caused RPM to parse key flags from a hashed key ID subpacket.  As
a result, RPM would wrongly reject a signature that had both key ID and
key usage flags subpackets in the hashed section.
@pmatilai pmatilai merged commit 7f83013 into rpm-software-management:master Apr 22, 2022
@pmatilai
Copy link
Member

Thanks for the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants