-
Notifications
You must be signed in to change notification settings - Fork 372
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
Distinguish between trusted and untrusted signatures and keys. #1993
Distinguish between trusted and untrusted signatures and keys. #1993
Conversation
d77af33
to
ab39879
Compare
Demi, development of the OpenPGP parser is frozen now, remember? |
As a case, this may well be something rpm needs to handle, but going forward (ie assuming Sequoia and/or other "proper" implementation) I only see the in-tree parser maintained stripped to bare bone with no subkey support. So really, don't work on it anymore. |
Good point. Does this also apply to the keyring code and similar? My understanding is that more work is needed there, such as to using fingerprints instead of key IDs. Also, I think the tests that this PR includes are valuable; they should pass for Sequoia-based RPM as well. In any case, I could see this PR being useful for downstreams that can’t use Rust or need something they can backport. |
In the very least, it would be good if the test case were merged. |
The test case fails with current RPM, as RPM currently doesn’t know when it needs to distrust a signature. This is still nowhere close to a full OpenPGP implementation (that’s orders of magnitude more complex), but it is enough to make |
dece7a1
to
82654aa
Compare
@pmatilai would you be okay with merging this change so that the test case can be included, and so that LTS distributions have something they can backport if they wish? |
I'm a bit concerned about the new certificates added in 126d896. The certificates will expire in two years. If an OpenPGP backend handles expiration, then I think these tests will begin to fail. It would probably be better to recreate the certificates without an expiration time.
|
Good idea. Does the logic look good? |
eacc525
to
077f8fd
Compare
077f8fd
to
e675d47
Compare
NOTTRUSTED does seem like an entirely reasonable way to handle eg revoked subkeys as such, but sometimes sane behavior doesn't quite fit with the mad underlying OK/NOKEY etc semantics in rpm. I think this will require some more chewing before swallowing, if only because it quite fundamentally changes the landscape of rpm signature handling. Up to now, imported == trusted since beginning of times, and there has never been a single case where rpm returns NOTTRUSTED. Which means that callers have no clear idea what it means and where they might encounter it, and so there almost certainly will be code in and out of rpm which will behave in unexpected/unwanted ways when presented with something else than OK/FAIL/NOKEY return. This also calls for test for the case where an installed packages has an NOTTRUSTED signature. |
Using the Sequoia backend, all three of these tests pass. It seems that not only is the internal OpenPGP parser unable to deal with this, but it can't even import the certificate:
Any idea what is going on here? Is it perhaps because the internal parser rejects certificates, because the primary key is not signing capable? |
Did you try with #1993? Without #1993 RPM will reject a certificate that has a revoked subkey, if the revocation signature is the first one. #1993 makes the behavior reasonable. |
e675d47
to
4d19b42
Compare
@nwalfield you ran into a different bug: RPM is rejecting some of the signatures because they have critical subpackets it doesn’t understand. I will file a patch to ignore the primary user ID flag. |
I tried my test cases (09bce55) with your change (7e10e0d), but I see that rpmkeys still can't import the certificate:
|
I suggest adding some debug logging in the subpacket parsing code to see which subpacket gets rejected. |
HTH |
If the signature actually has more than one key usage subpacket in the hashed section, this is expected behavior, but I would also consider that to be a very strange signature. Otherwise, something has gone wrong somewhere. Can you check if the flag gets reset properly between signatures? |
Using this MR plus #2026, the test keys that I generated can now be imported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think these changes are an improvement to the internal parser, but the complexity of the internal parser increasingly scares me.
} | ||
} | ||
end: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file usually uses exit
, not end
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. It will be a while before I can fix this, though.
5nFoswUnFC2C9SpJeiZDLSgBAO5h6mRI9iafOref8xhCf6qAtnXD3J195VMX1zCU | ||
chEM | ||
=MpKy | ||
-----END PGP PUBLIC KEY BLOCK----- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This certificate expires and should be replaced with one that does not:
$ sq inspect < tests/data/keys/first-signing-key-revoked.asc
-t: OpenPGP Certificate.
Fingerprint: C9E4585A5D0201A9A077CE60D115BE9BEBDDD264
Public-key algo: EdDSA Edwards-curve Digital Signature Algorithm
Public-key size: 256 bits
Creation time: 2022-03-26 22:10:10 UTC
Expiration time: 2024-03-25 22:10:10 UTC (creation time + P730D)
Key flags: certification, signing
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That indeed should be done, sorry about that. I will be busy for a while with Qubes tasks.
(sig->saved & PGPDIG_UNTRUSTED)) | ||
res = RPMRC_NOTTRUSTED; | ||
else | ||
res = RPMRC_OK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @pmatilai that we shouldn't change the API.
I think we can emit this type of information via my proposed lint interface. We should, of course, consider returning richer error messages when we transition to a new API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m fine with using RPMRC_NOKEY
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we return NOKEY, then I think the whole certificate is ignored, which is not what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the better approach is to just ignore the subkey if it appears to be revoked, i.e., drop this change.
break; | ||
case PGPSUBTYPE_EMBEDDED_SIG: /* embedded signature */ | ||
impl = *p; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code makes me suspect that rpm is not checking the primary key binding signature, which signing-capable subkeys must have to be valid. Is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. RPM has never checked that signature, but it really should. It isn’t a security problem in the case of checking a signature immediately before installing the package, though, as the primary key is trusted to not have signed a subkey that doesn’t belong to it. I didn’t want to add even more complexity to this code, but feel free to add such a check.
if (plen < 2) | ||
return 1; /* missing reason code */ | ||
if (sigtype == PGPSIGTYPE_SUBKEY_REVOKE) | ||
impl = *p; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the reason for revocation subpacket only understood in the context of subkey revocations and not certificate revocations or user id revocations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RPM doesn’t implement those kinds of revocations. My assumption is that keys imported into RPM are a result of gpg2 --export --armor --export-options=export-minimal -- <TRUSTED_FINGERPRINT>
or similar.
selfsig->sigtype != PGPSIGTYPE_SUBKEY_BINDING)) | ||
xx = pgpVerifySelf(digp, selfsig, all, i); | ||
/* subkeys must be followed by binding or revocation signature */ | ||
int xx = pgpVerifySelf(digp, selfsig, all, i, prevtag); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are willing to admit multiple self signatures, which I think the last commit and this one now allow, then we shouldn't fail if a self-sig can't be verified. We should only fail if no self signatures can be verified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment about assumptions: GPG’s --export --export-options=export-minimal
should strip such signatures off, and RPM doesn’t have the machinery needed to judge if a given packet can be safely ignored. The Sequoia backend does and would handle this properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The complexity of the internal parser scares me too, but I suspect most of that complexity is essential. I wonder how GRUB’s parser fares.
break; | ||
case PGPSUBTYPE_EMBEDDED_SIG: /* embedded signature */ | ||
impl = *p; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. RPM has never checked that signature, but it really should. It isn’t a security problem in the case of checking a signature immediately before installing the package, though, as the primary key is trusted to not have signed a subkey that doesn’t belong to it. I didn’t want to add even more complexity to this code, but feel free to add such a check.
if (plen < 2) | ||
return 1; /* missing reason code */ | ||
if (sigtype == PGPSIGTYPE_SUBKEY_REVOKE) | ||
impl = *p; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RPM doesn’t implement those kinds of revocations. My assumption is that keys imported into RPM are a result of gpg2 --export --armor --export-options=export-minimal -- <TRUSTED_FINGERPRINT>
or similar.
selfsig->sigtype != PGPSIGTYPE_SUBKEY_BINDING)) | ||
xx = pgpVerifySelf(digp, selfsig, all, i); | ||
/* subkeys must be followed by binding or revocation signature */ | ||
int xx = pgpVerifySelf(digp, selfsig, all, i, prevtag); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment about assumptions: GPG’s --export --export-options=export-minimal
should strip such signatures off, and RPM doesn’t have the machinery needed to judge if a given packet can be safely ignored. The Sequoia backend does and would handle this properly.
} | ||
} | ||
end: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. It will be a while before I can fix this, though.
(sig->saved & PGPDIG_UNTRUSTED)) | ||
res = RPMRC_NOTTRUSTED; | ||
else | ||
res = RPMRC_OK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m fine with using RPMRC_NOKEY
instead.
5nFoswUnFC2C9SpJeiZDLSgBAO5h6mRI9iafOref8xhCf6qAtnXD3J195VMX1zCU | ||
chEM | ||
=MpKy | ||
-----END PGP PUBLIC KEY BLOCK----- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That indeed should be done, sorry about that. I will be busy for a while with Qubes tasks.
I'll rework the patch series. Is 6bf8f03 necessary? Also, this change isn't described by the commit message: DemiMarie@6bf8f03#diff-1b8795f690eee3ed483f0ce64cd81e4d42c681a13c98314a3250c4fac5159485R620 |
Thank you.
With the current code I believe it is a no-op, but it makes the code less error-prone and is an overall improvement. |
7e10e0d
to
8809f66
Compare
That sounds vaguely familiar: #1813 (comment) |
Right now the two are equivalent, but this will no longer be the case if one can parse a signature without actually loading the algorithm-specific data. This will be useful for re-parsing subkey binding and revocation signatures that have already been validated.
In OpenPGP, only certain types of packets are followed by one or more signatures. These include public keys, public subkeys, user IDs, and user attributes (which include photos). Signatures that revoke other signatures make this more complex, but RPM does not implement those. Therefore, do not advance `i` in pgpPrtParams() if the packet just parsed is not one of the four aforementioned types, since self-signatures are computed over the first packet and the immediately preceding packet. Also, do not set `prevtag`, since it is used in verifying self-signatures.
The pgpVerifySelf() API was rather clumsy. The type of the data packet should determine the type of its self-signature, not the other way around. This makes detecting wrongly-typed self-signatures much simpler. Now that pgpPrtParamsSubkeys() checks the type of self-signatures, subkey revocation signatures can be checked too. This allows importing keys that have revoked subkeys. Also add a bounds-check in pgpVerifySelf() in case the index passed is wrong.
According to RFC 4880, subkeys are supposed to only be followed by binding signatures, revocation signatures, and other subkeys. In practice, the most likely reason to get something else is a revocation signature of a type that RPM does not implement. This could include a revocation of a subkey binding signature or a revocation made by a designated revoker. In this case, it is not safe to import the key, as RPM might accept a signature made by a subkey whose secret portion has been compromised. Therefore, return an error in this case.
RPM could not handle revoked subkeys: old versions accepted signatures made by them, while more recent versions will usually fail to import the signature. This ignores such subkeys. Additionally, require all signatures following a subkey to be subkey binding or revocation signatures issued by the main key. Anything else is most likely an alternate method of revocation that RPM doesn’t implement, and so importing such a key would be unsafe.
RPMRC_NOKEY means that RPM does not have the key at all. In the case of a signature by a revoked subkey, RPM *does* have the key; it is just refusing to trust it. Therefore, RPMRC_NOTTRUSTED is the correct return code. The same is true if a subkey is incapable of signing: RPM has the key, but is refusing to trust signatures made by it.
Sequoia sets them in the keys it generates, causing breakage.
8809f66
to
4088faf
Compare
After way too much procrastinating and indecision... We wont be able to axe the internal parser in 4.19, dunno about 4.20. But that's a long way off, and until then we need to support what we ship. To avoid having to pile this complexity on and on and on to deal with subkey this and subkey that, I think the only possible solution is to saw off the limb to save the patient: remove the subkey support (#2278). That's where like 90% of the complexities seem to be. Clearly quite a bit of work has gone into this PR and I hate to throw that away, but it's not like I didn't warn right from the beginning. As most of this PR is dealing with subkey matters, merging it just to remove the added complexity right after doesn't make any sense. Lest it all go to waste: at least the bit to support Sequoia-generated keys deserve salvaging but that's really unrelated and should be filed separately. Ditto for test-cases, but only for the Sequoia backend. There may be others but lets first get the subkeys out, it'll be clearer then. Thanks and apologies, I should've been firm with this right from the start. I'd like to claim the crystal ball on this all was still fuzzy in the spring but that'd be lame. |
RPM needs to be able to differentiate between “key not found” (
RPMRC_NOKEY
) and “key or signature not trusted” (RPMRC_NOTTRUSTED
). A signature made by a revoked subkey is very much the latter situation.