Re: [openpgp] Fingerprints

["Neal H. Walfield" <[e-mail]>]

Hi Derek,

On Fri, 25 Nov 2022 17:22:52 +0100,
Derek Atkins wrote:
> On Fri, November 25, 2022 11:02 am, Neal H. Walfield wrote:
> > Hi,
> >
> > The crypto-refresh defines fingerprints as follows:
> >
> >> 5.5.4.  Key IDs and Fingerprints
> >>
> >>    A v5 fingerprint is the 256-bit SHA2-256 hash of...
> >
> > draft-koch defines fingerprints as follows:
> >
> >> 12.2.  Key IDs and Fingerprints
> >>
> >>    A V5 fingerprint is the 256-bit SHA2-256 hash of...
> >
> 
> Sorry, I don't see the issue here?  Both versions here are using "the
> 256-bit SHA2-256 hash of".
> 
> So where is the issue?  What am I missing?

My observation is that both fingerprint versions (crypto-refresh and
draft-koch) use the same representation.  I don't mean that they both
use the same hash algorithm, but that they both have the same length
and use the same alphabet, and are thus indistinguishable.


I'll try to make it a bit clearer why this is problematic from an
implementation perspective.

An OpenPGP implementation needs to represent fingerprints in some way.
In Sequoia, we use something like this:

  enum Fingerprint {
      V4([u8; 20]),
      V5([u8; 32]),
      V6([u8; 32]),
      Invalid(Box<[u8]>),
  }

So when the Sequoia OpenPGP parser encounters a third-party
certification that has an Issuer Fingerprint subpacket, it creates,
say, a `Fingerprint::V6(FPR)`.

This works, because the Issuer Fingerprint subpacket includes both the
version and the fingerprint:

  (1 octet key version number, N octets of fingerprint)

  ...

  https://datatracker.ietf.org/doc/html/draft-ietf-openpgp-crypto-refresh-07#section-5.2.3.32

Sequoia can also serialize OpenPGP data structures.  Because we have
all the information available, we don't need to keep the original data
around.  We just reserialize it based on the in-memory data
structures.

From our perspective, the above representation has the advantage that
it keeps all of the relevant information together.

Unfortunately, the above representation is not so great, when dealing
with fingerprints where the version is ambiguous.

For instance, to encrypt a message using `sq`, our command-line tool,
one would do:

  sq encrypt --recipient-cert FPR ...

In the past, we could infer the version from the length of the
fingerprint, and this allowed us to store the fingerprint as
`Fingerprint::V4(FPR)`.  Then we could iteratore over all certificates
and look for the right one.  Here's the idea (this is not how we
actually do it, and the code is not actually Rust):

  // Collect all recipients.
  let recipients = Vec::new();
  for fpr in args.recipient_cert() {
      for cert in keyring.iter() {
          if cert.fingerprint() == fpr {
              recipients.push(cert);
          }
      }
  }

When draft-koch and crypto-refresh have different versions, but the
same length and alphabet, this is no longer possible.

I see several possible solutions:

  1. Use two data types, `VersionedFingerprint` and
    `VersionlessFingerprint`.

    This increases implementation complexity, but is doable.  In
    addition to having two data types, they won't interact all that
    well with each other.  For instance, because we expect equal to be
    transitive (`a == b` and `b == c` implies `a == c`), we can't
    easily compare `VersionedFingerprint`s and
    `VersionlessFingerprint`s:

      `VersionedFingerprint(5; FPR) == VersionlessFingerprint(FPR)`
      and
      `VersionedFingerprint(6; FPR) == VersionlessFingerprint(FPR)`
      does NOT imply
      `VersionedFingerprint(5; FPR)` == `VersionedFingerprint(6; FPR)`.

    Looking at RNP, I see it uses the `VersionlessFingerprint`
    variant:

      https://github.com/rnpgp/rnp/blob/4a2d42d9591ecf7428a738247181c988f6fc0ff2/src/lib/types.h#L100

    When it compares a key's fingerprint (`key->fp`), and an issuer
    fingerprint (`sig.keyfp()`), it appears that it sometimes doesn't
    check that the versions match:

      if (subsig.sig.has_keyfp() && (key->fp() == subsig.sig.keyfp())) {

      https://github.com/rnpgp/rnp/blob/e5602874e959b05e0c8af9ea33680d06c4fe6811/src/librekey/rnp_key_store.cpp#L251

    Note: I don't think this is a security problem, but I opened an
    issue:

      https://github.com/rnpgp/rnp/issues/1944

    Not checking the version could cause some confusion, because some
    implementations may process the same data differently.

  2. Modify the standard to define the fingerprint to be the version
    plus the hash.

    This has a few advantages:

      - A fingerprint is now unambiguous

      - It allows the use of a single Fingerprint data type, as a
        Fingerprint, by definition, must always carry a version.

      - It does not require any changes to the wire format (as far as
        I can see, all uses of fingerprints in crypto-refresh include the
        version).

  3. Don't bump the version in the crypto-refresh.

    It appears safe to assume that `SHA2-256(..)` will be unique.  So,
    we could just reuse v5.  As Daniel H., I think, points out
    elsewhere, this would make parsing the packets more complicated.

I prefer 2.


I hope that clarifies the issue a bit.


Thanks,

Neal