rpm icon indicating copy to clipboard operation
rpm copied to clipboard

rpm should not use short gpg key ids in messages

Open keszybz opened this issue 2 years ago • 23 comments

Copied over from https://bugzilla.redhat.com/show_bug.cgi?id=2174373:

Description of problem: (Inspired by https://bugzilla.redhat.com/show_bug.cgi?id=2170878.) Short gpg key ids are easy to spoof and generally should not be used [e.g. 1]. rpm prints them in various messages:

warning: google-chrome-stable_current_x86_64.rpm: Header V4 DSA/SHA1 Signature, key ID 7fac5991: NOKEY

There is really no point in trying to save a few bytes. Please print at least the "long" 16-digit hash. With the short id the user cannot even reliably look up the key online.

In other output, please print the full hash:

$ rpm -qi util-linux | rg Signature
Signature   : RSA/SHA256, Sat 21 Jan 2023 11:02:21 AM CET, Key ID 809a8d7ceb10b464

The full finger print is 6A51BBABBA3D5467B6171221809A8D7CEB10B464 and it is just easier to do verification if the full hash is known.

Version-Release number of selected component (if applicable): rpm-4.18.0-10.fc38.x86_64

[1] https://security.stackexchange.com/questions/84280/short-openpgp-key-ids-are-insecure-how-to-configure-gnupg-to-use-long-key-ids-i

keszybz avatar Mar 01 '23 11:03 keszybz

Are there scripts out there that parse this output? If so, they will almost certainly break. Perhaps it would be better to do:

warning: google-chrome-stable_current_x86_64.rpm: Header V4 DSA/SHA1 Signature, key ID 7fac5991, fingerprint: 6A51BBABBA3D5467B6171221809A8D7CEB10B464: NOKEY

nwalfield avatar Mar 01 '23 11:03 nwalfield

Yep, various things do parse that "informational" warning from rpm and will break if changed. Doesn't mean we can never change it, just that we need to be mindful of doing so.

I remember starting towards "use fingerprints everywhere" goal once upon a time but ran into sufficient obstacles to get sidetracked / gave up / something to that effect. The biggest obstacle of them all was probably the gpg-pubkey entries in the rpmdb, with all manner of things expecting the exact version/release "semantics".

pmatilai avatar Mar 01 '23 11:03 pmatilai

Well, anything which parses this output and makes use of it is broken. I shouldn't be using short ids for anything, particularly not in a script. So I think it's entirely reasonable to just change the output.

Also, I'm not sure if too many things would break. I'd expect that even sloppy parsers would just take field by position of by some regexp with [0-9a-f]+ and not be bothered by the change in length.

keszybz avatar Mar 01 '23 11:03 keszybz

Well, anything which parses this output and makes use of it is broken. I shouldn't be using short ids for anything, particularly not in a script. So I think it's entirely reasonable to just change the output.

Unfortunately, we can't afford to do that. There are config management tools and layered package managers that parse RPM's output. And we have the model of "it's stable unless documented otherwise". So we have to work with that.

Conan-Kudo avatar Mar 04 '23 21:03 Conan-Kudo

What "config management tools" are that? Maybe they need to be warned that the output will change and then the output in rpm adjusted after a while. Short gpg ids are widely known to be a bad idea at least since 2011 [1]. Is the idea to set every mistake that was ever made in stone?

[1] http://www.asheesh.org/note/debian/short-key-ids-are-bad-news.html

keszybz avatar Mar 05 '23 18:03 keszybz

They have been used in some places we know (eg yum) and then there are all those cases we don't know about.

It's not so much the message that needs changing, but the idiotic API that emits this message (needs to die). But to be able to deprecate that API (which happens to be one of the oldest APIs in rpm) we need something better first, which is a long time coming.

pmatilai avatar Mar 09 '23 07:03 pmatilai

OK, sketched a patch for moving from short to long Key IDs in the messages: #3292. This is trivial to do but may be not trivial to deal with.

Moving to Fingerprints is not quite as trivial. We do have the means to get Fingerprints from PGP keys but so far we lack that for signatures. I am currently still digging through the backends to see what would be needed to get them.

ffesti avatar Sep 10 '24 10:09 ffesti

I am currently still digging through the backends to see what would be needed to get them.

The relevant function is pgpDigParamsSignID. Unfortunately, v3 signature packets don't include the fingerprint, but only the (long) key ID. In practice, this is only a hint: we only know that the hint is correct after we've positively verified the signature at which point we know what key created the signature and therefore we can derive the signing key's fingerprint.

I think we need to enumerate the cases where the key ID is used or emitted, and figure out what is best on a case-by-case basis.

nwalfield avatar Sep 10 '24 10:09 nwalfield

Internally it looks like most places are just using the Key ID. Especially the rpmkeyring is using it as a key in the hash of all Pubkeys. If we don't have any hope to move that to Fingerprints as the v3 signatures won't offer them anyway, we might need to go down another route and just verify signatures more often and get the Fingerprints from the successfully verifying Pubkey.

I am currently already compiling a list of all usages of the Key ID. I'll report back when I got the complete list.

ffesti avatar Sep 10 '24 11:09 ffesti

Hashing by keyid is not an issue, assuming it to be an unique identifier is. We can teach the keyring to expect multiple keys per keyid instead, nothing in the API deals with keyids so this should be largely an internal matter. The gpg-pubkey entries are a bigger and more visible issue.

pmatilai avatar Sep 11 '24 06:09 pmatilai

Here my findings so far:

PGP KeyID and FingerPrint usage in RPM

  • PGP Fingerprint: 20 or 32 bytes hash of public key (depending on key version v4 vs v6)
  • (Long) KeyId: first/last 8 bytes of Fingerprint
  • Short KeyId: last 4 bytes of long KeyId

Short KeyId is prone to (accidental) collisions Long KeyId still can have collisions if searched for by an attacker

  • pgpPubkeyFingerprint() is not used at all execpt in one test case
  • const uint8_t *pgpDigParamsSignID(pgpDigParams digp) returns Long KeyId of signatures.

Signatures only return KeyID. Fingerprint just not available for v3 PGP sigs. v4 PGP signatures contain Fingerprint but RPM does of have any means to get them right now. We could get the Fingerprint from the matching key - if available.

Short KeyID in RPM

  • rpmio/rpmpgp.c L56 pgpIdentItem()
    • PR #3292 moves to long KeyId
  • See Pubkey Packages
  • rpmvs.c: sinfo->keyid Used to suppress repeat log messages - needs to die but is not critical

Using FingerPrint instead of KeyID

const uint8_t *pgpDigParamsSignID(pgpDigParams digp);

Usage:

  • rpmkeyring.c: internally to find Pubkey for signatures. Probably fine, may need handling of conflicting KeyIDs.
  • lib/formats.c: pgpsigFormat() Human readable signature with KeyID. May need FingerPrints iff available in sig or keyring?
  • rpmts.c: Creation of Pubkey packages.
  • rpmvs.c: sinfo->keyid Must die!
  • rpmpgp.c pgpIdentItem() formats Public Keys or Signatures
    • Used short KeyID (see #3292)
    • Public API in rpmpgp.h
    • used in rpmvs.c -> rpmsinfoMsg() used through out the code base:
      • package.c: handleHdrVS() which is passed to rpmvsVerify(RPMSIG_VERIFIABLE_TYPE) as callback in rpmReadPackageFile() and headerCheck()
      • rpmchecksig.c: vfyCb() passed as callback to rpmvsVerify(RPMSIG_VERIFIABLE_TYPE) in rpmpkgVerifySigs
      • transaction.c: vfyCb() passed as callback to rpmvsVerify(RPMSIG_VERIFIABLE_TYPE) in verifyPackageFiles() ultimately in rpmtsRun()
      • rpmgensig.c: msgCb passed as callback to rpmvsVerify(RPMSIG_DIGEST_TYPE) in checkPkg() ultimately in rpmPkg*Sign()
      • So it is all used in rpmvsVerify(), first 3 have verified signatures available and the last one is about digests only.

Pubkey Packages

  • gpg-pubkey packages are created on key import.
  • Short KeyID as Version and Creation time as release
  • Same as Provides EVR with key version as Epoch
  • Long KeyID as gpg(xxx) Provides
  • No Fingerprints in sight.
  • All just package tags - no runtime check.

Sub Keys

Support for subkeys seems very rudimentary.

  • No way to list installed subkeys.
  • No way to connect sub keys to gpg-pubkey packages
  • pgpPubkeyFingerprint() only works on raw key data and returns the Fingerprint of the primary key
    • Turns out that this is on purpose and we should only show the main keys fingerprint

ToDo

Sub projects

  • Print Fingerprint for Pubkeys in rpmsinfoMsg() see #3321
    • Store key with check result of signature
  • Offer a way to get information on the actually installed keys e.g. #3332
  • #3333
  • Support multiple entries per KeyId in keyring to deal with Key ID collisions safely #3334
  • #3313
    • Obsoletes: Check if EVR and Provides match stored key in loadKeyringFromDB

ffesti avatar Sep 17 '24 08:09 ffesti

Get rid of lib/package.c stashKeyid() and rpmsinfo_s.keyid

The thing that needs to die is the static container in there, it should be hooked to the transaction perhaps. The short keyid use needs to go of course.

Add --list to rpmkeys utility

You already did that earlier this year in 1dc7e76fa51bca54f0eb75660ab6e68216289eb6 :joy: But it's another place that deals with short keyids so needs updating along with everything else.

pmatilai avatar Sep 17 '24 08:09 pmatilai

Check if EVR and Provides match stored key in loadKeyringFromDB

I think there's an underlying assumption that gpg-pubkey's can only enter the db through rpmtsImportPubkey(). Which would probably be fine, if that assumption held. Imported keys have the distinct quality that they have no architecture, so it's easy to detect them, we don't allow installing packages without an architecture. ...unless the package is called gpg-pubkey :facepalm: :rofl:

pmatilai avatar Sep 17 '24 09:09 pmatilai

Here my findings so far:

Thanks for working on this! A few comments:

PGP KeyID and FingerPrint usage in RPM

* PGP Fingerprint: 20 or 32 bytes hash of public key (depending on key algorithm)

To clarify: v4 fingerprints are 20 bytes and v6 fingerprints are 32 bytes. So, it depends on the key version.

* (Long) KeyId: last 8 bytes of Fingerprint

Except for v6 keys where it is the first 8 bytes...

Signatures only return KeyID. Fingerprint just not available for v3 PGP sigs. v4 PGP signatures contain Fingerprint but RPM does of have any means to get them right now. We could get the Fingerprint from the matching key - if available.

Technically, the fingerprint (or key ID) is only known after the signature is verified. At that point we know what certificate made the signature and can derive the fingerprint.

The issue that you are hinting at is that a signature may contain a key ID or a fingerprint. As this is unauthenticated, this is purely hearsay, and we should avoid using it. Of course, if we can't verify a signature, then we have to use it...

* Short KeyID as Version and Creation time as release

Using the key ID (or the fingerprint) as the version is problematic as a certificate can change (e.g., get a new subkey, have its expiration changed) without the fingerprint being changed.

Support for subkeys seems very rudimentary.

* No way to list installed subkeys.

* No way to connect sub keys to gpg-pubkey packages

* pgpPubkeyFingerprint() only works on raw key data and returns the
  Fingerprint of the primary key

In general, I think that subkeys should be treated as an implementation detail. If at all possible they should not be shown to users. And, I don't think it is necessary for users to be able to address certificates by subkey fingerprints; they should use the certificate's fingerprint.

ToDo

* API: Add variant of pgpPubkeyFingerprint() that works for sub keys, too.

What's the use case?

* Offer a way to get information on the actually installed keys e.g.
  
  * Fingerprint format for pubkeys tag to get actual
    Fingerprint of installed key(s)
  * PGP key format that gives rpm -qi like output of the actual keys
  * Add --list to rpmkeys utility

Perhaps add a --dump-keyring (feel free to bikeshed the name), which dumps all of the keys. This would make it easy to inspect the certificates using something like sq.

* Support multiple entries per KeyId in keyring

What's the motivation for this?

nwalfield avatar Sep 17 '24 09:09 nwalfield

(Also note that the fingerprint information is not mandatory for v4 sigs)

mlschroe avatar Sep 17 '24 12:09 mlschroe

(Also note that the fingerprint information is not mandatory for v4 sigs)

Right. Technically, the fingerprint (or key ID) is only known after the signature is verified. At that point we know what certificate made the signature and can derive the fingerprint.

The issue that you are hinting at is that a signature may contain a key ID or a fingerprint. As this is unauthenticated, this is purely hearsay, and we should avoid using it.

nwalfield avatar Sep 17 '24 12:09 nwalfield

OK, I guess I am not quite getting this sub key business yet. Doesn't the signature list the KeyId of the subkey it was created with? Or does it point to the main key and assumes the subkey will be found there?

After looking at this overall mess we figured a way to solve a lot of the issues with the pgp-pubkey packages would be to just get rid of them completely: #3313 Guess the next steps are making rpmkeys fully support both pubkeys in the rpmdb and as files on disk. There already is code to load keys from files but many of the surrounding pieces are still missing. We need to get to the point where all operations about PGPG keys can go through the rpmkeys utility and no rpm commands are needed.

ffesti avatar Sep 18 '24 12:09 ffesti

The signature points to the subkey.

mlschroe avatar Sep 18 '24 12:09 mlschroe

OK, I guess I am not quite getting this sub key business yet. Doesn't the signature list the KeyId of the subkey it was created with? Or does it point to the main key and assumes the subkey will be found there?

As a first approximation, subkeys are an implementation detail and shouldn't be exposed to users.

An OpenPGP certificate is a collection of components. The components include the primary key, subkeys, and user IDs. A certificate's identity, i.e., its fingerprint, is derived from the primary key (its the hash of the primary key). This means that the certificate's identity doesn't change even if the certificate is updated. This is important for authentication. Say I go to FOSDEM and visit the Qubes developers. They usually wear T-Shirts with their release key's fingerprint. I can take a picture, and when I go home, record that the Qubes release key has the fingerprint 427F11FD0FAA4B080123F01CDDFA1A3E36879494 (e.g., using sq pki link add). When I download a Qubes image in n years, the certificate may have been updated (e.g., it has a new signing subkey, the expiration time has been extended, etc.), but the certificate still has the same fingerprint, so I know that the signature is authentic. (Note: Fedora uses a different OpenPGP certificate for each release. I'd argue that this is not a good practice, as it makes authenticating the downloads a lot more difficult: I have to manually check a new certificate every six months. Creating a new signing subkey every six months, however, is not a problem for the end users.)

Having mutliple keys also allows for domain separation. For instance, by default, Sequoia has a separate signing-capable subkey; the primary key cannot be used for signing data. This prevents attacks like the shambles attack, which reuses a data signature as a certification.

The various components in a certificate are linked to the primary key via signatures. This means that if I've authenticated a certificate (i.e., its primary key), then I can chain forward (or backwards) and authenticate the various components.

A signature packet includes a hint about the (sub)key that was used to create it. Once we check that the (subkey) actually created the signature, we can then chain backwards to the primary key.

We need to get to the point where all operations about PGPG keys can go through the rpmkeys utility and no rpm commands are needed.

That sounds good to me.

nwalfield avatar Sep 18 '24 13:09 nwalfield

@ffesti wrote: Guess the next steps are making rpmkeys fully support both pubkeys in the rpmdb and as files on disk.

Yeah that would be some of the next steps (whether the next I dunno, depends)

pmatilai avatar Sep 19 '24 10:09 pmatilai

* Support multiple entries per KeyId in keyring

What's the motivation for this?

The phrase probably originates from me, and maybe is not to the point. The point is, rpm currently assumes a strict 1:1 relationship between keys and keyid's, and since keyid's can conflict, that assumption is clearly wrong and needs fixing (one way or the other).

pmatilai avatar Sep 19 '24 10:09 pmatilai

* Support multiple entries per KeyId in keyring

What's the motivation for this?

The phrase probably originates from me, and maybe is not to the point. The point is, rpm currently assumes a strict 1:1 relationship between keys and keyid's, and since keyid's can conflict, that assumption is clearly wrong and needs fixing (one way or the other).

Thanks for the clarification.

nwalfield avatar Sep 19 '24 10:09 nwalfield

OK; I put the sub projects into their own tickets and updated the todo list. #3321 has shown that we are fine with the API of the backends as they are. So this whole thing is a bit easier than first thought. Next step is teaching rpmkeys to give some useful information on the installed keys (#3332).

With the plan to ditch the gpg-pubkey packages for the next release a lot of features making them more bearable are not longer needed. Ofc getting proper support for a proper key store will also require quite a few new pieces.

ffesti avatar Sep 26 '24 10:09 ffesti

All user-visible strings have now been updated to fingerprint where possible, and full keyid where not through multiple PR's, at least #3369 #3365 #3368 #3333 #3390 #3379 #2403 are related.

pmatilai avatar Oct 22 '24 07:10 pmatilai

Thank you!

keszybz avatar Oct 22 '24 16:10 keszybz