Give error message for failed PGP key import #2097
Conversation
|
I want SHA-1 to die, but this is a bit too much. First, for v4 OpenPGP keys (the current version), you have to use SHA1 to compute the Fingerprint and the Key ID. There is no other way. The Fingerprint and Key ID are used everywhere in the OpenPGP specification. For instance, a signature includes an Issuer field, which contains the Key ID of the key that generated the signature. If you can't compute the Key ID, then you can't designate (name) the certificate that generated the signature, but have to try all available signing-capable keys. That's a performance disaster. The good news is that the Key ID is self authenticating: if you verify the signature, then you know that the Key ID is correct. Second, a hash algorithm provides several security properties. The two most important are collision resistance and second pre-image resistance. SHA1's collision resistance has been broken, but its second pre-image resistance is fine. A second pre-image attack is when an attacker finds a second text that has the same hash as a given text. This type of attack is much harder, because it can't exploit the birthday paradox. Collision resistance is only needed when the attacker can choose both messages. Since an attacker doesn't control the key, self signatures don't need a hash with collision resistance (the attacker doesn't control the key material). And since a fingerprint is basically the hash of a key, it also doesn't need a hash with collision resistance. In short, yes, data signatures using SHA-1 should be rejected. Yes, it would be nice to have self signatures that don't use SHA-1 since there are alternatives. But, a v4 OpenPGP implementation must be able to compute the fingerprint. Note: v5 keys will use SHA256. The RFC draft is in working group last call. But even if it is standardized today, it will take years before enough users adopt v5 keys so that we can start rejecting v4 keys. |
|
Yeah... @ffesti , are you sure this is where it's tripping on in the failing case? Like @nwalfield points out above, ability to compute SHA1 is required for v4 keys because it's a part of the RFC itself. So this is different from people "upgrading" their keys to use a stronger hash in the key itself (pardon my caveman terminology) to avoid the issue. |
|
Ok, looks like my initial test case was broken. |
This can happen when old keys are used on systems that have disabled SHA1 e.g. for FIPS requirements. This is less than ideal but there is currently no way to pass a meaningful error code up to rpmtsImportPubkey. rpmPubkeyNew just returns a valid key or NULL. See rhbz#2069877
|
If we add the message to openssl, we'd need to do that for libgcrypt too. I wonder if we could get by with returning ENOTSUP or such from the backend and log something centrally from pgpVerifySignature() instead. In any case, this is a hard error, not a warning. |
|
Getting libgcrypt to reject the PGP key is somehow less straight forward. While it is not surprising that it won't be affected by |
I think RPM will need to reject the key/signature itself. |
|
Oh, I didn't even know libgcrypt had a FIPS mode now. In any case, FIPS is just a side-issue here, the question is whether the underlying crypto library supports that key (algos and all) or not, and somehow communicating it back to the user. |
|
An OpenPGP certificate usually contains several keys. The keys are bound together using binding signatures. It is entirely possible and indeed reasonable for there to be multiple binding signatures for a key, e.g., to extend the expiration time. Thus, if a signature happens to use SHA-1, the whole certificate should not be rejected, just that signature. |
|
@nwalfield - I'm sure that's what a proper implementation should and would do, but in rpm world multi-key certificates aren't that common in the first place as they're usually tailored for the very purpose of using in rpm only, so we get by with little less finesse. |
|
Even if subkeys aren't used, there can still be multiple signatures. Here is There are three signature packets on the User ID packet, two of them use SHA-1. Note: an OpenPGP certificate is not valid without at least one valid self signature. |
to set a separate license to the source RPM. This can be useful if the sources have code under additional licenses that do not end up in the binary packeges. Resolves: rpm-software-management#2097
to pgpVerifySignature()
|
Not sure if I am really a fan of using errno codes here. But the whole return code handling is a mess and error handling in the crypto backends is very ... ehm ... basic. |
The crypto API's error reporting is indeed primitive. But, Sequoia has extremely (in my experience) rich error messages. This, of course, is lost at the API boundary. But that's the API's limitation, not the crypto backend's limitation. |
|
Yeah, issue here is not so much Sequoia but the sum of all crypto libs that are supported there. I'll add a separate ticket to make a plan for that. That's far outside of the scope of this little PR. |
|
Why are we even talking about rejecting such keys/signatures in RPM here? Isn't this supposed to be handled by the backend in use? IIUC, verification simply fails in the OpenSSL code if SHA-1 is used in FIPS mode so all we need is make it clear why that is to the user, don't we? Or is the concern that, as @nwalfield mentioned, some keys (including RH's own) may contain multiple signatures, some of which use SHA-1, for which we'd always print the warning? If that's the case, maybe we could detect we're running in FIPS mode and only print the message there, even though, that already sounds like an ugly hack. But IIRC, there's a similar check in DNF... |
|
Using v4 OpenPGP keys requires the use of SHA-1. SHA-1 is used to compute the fingerprint. This is actually safe as the security of the fingerprint relies on second pre-image resistance (finding a second message with the same hash), not collision resistance (finding two messages with the same hash). SHA-1 second pre-image resistance is still secure, only its collision resistance is broken. Of course, we are not happy to continue to use SHA-1 and the next version of the OpenPGP specification will use SHA-2 256 to compute fingerprints. But, it will take years until all v4 keys have been replaced by v5 keys. |
|
The discussion here is beside the point of the PR. This is in large parts my fault as the original patch also was beside the point. As it turns out this is unrelated to v4 OpenPGP signatures and their use of SHA-1. The current patch only returns the error of not supported older SHA-1 based keys/signatures that have been rejected by the openSSL back end due to system wide settings and issues an message to give the user a fighting chance to understand why the key is not working. |
|
The patch doesn't do what the PR title says as it only outputs anything on verify. |
|
That, and being specific to a crypto backend which is neither the current or future default are quite show-stoppers to me. A reasonable compromise could be just outputing signature parameters on failure (rpmPubkeyNew() time basically) without trying to pinpoint what the issue is, but that too runs into the issue of error reporting. OTOH, I'm not sure an rpmlog() call in rpmPubkeyNew() would be any worse than having it in the low-level PGP code. |
|
(FWIW, I believe the internal OpenPGP implementation is still the default for 4.18.) |
|
Yes, but for that the default is libgcrypt whereas this patch only does anything for openssl backend. |
|
Thanks for the clarification. |
|
Of course, this all is made even more entertaining by the fact that the internal and Sequoia parsers treat this very differently: Sequoia doesn't verify on import, only on actual use, ie verification. Whereas the internal backend verifies on key import, so this patch doesn't really have a chance to do anything on the backend it targets. Erm. So, I just contradicted myself here: because we verify the self-signatures on import, this does get called on import, when using the internal parser. Sorry. |
|
That tangle could be simplified a bit by ripping the subkey support out of the internal parser as kinda planned past 4.18. It still doesn't fix the error reporting of course. We could add better verify APIs of course to allow returning actual error messages back to callers, but we'd need to pass through multiple layers of historic goo... |
|
As I understand, 4.19 will have an soname bump so it should be possible to rework the API (and remove the old API). Or, if that is not sufficient, what are your concerns? |
|
Okay, adding an errno abuse hack (which I suggested, yes) to just one of our backends, a backend that will be considered deprecated in 4.19 anyway... it doesn't make sense really. |
due to missing SHA1 support. This can happen when old keys are used on
systems that have disabled SHA1 e.g. for FIPS requirements.
See rhbz#2069877