1706270 - uidHasGoodSignature is shaky

Status: UNCONFIRMED

(MailNews Core :: Security: OpenPGP, enhancement, P3)

Description

[neal]

The function uidHasGoodSignature was recently added to Thunderbird (commit f29328195e91 from March 17th and first appeared in 78.9.1). Currently, it only has a single caller, which is used for a rare and not terribly important corner case, but I'm concerned that this function may be used in other situations in the future, which may negatively impact the user's security.

For reference, here is the function:

  uidHasGoodSignature(self_key_id, uid_handle) {
    let sig_count = new ctypes.size_t();
    if (RNPLib.rnp_uid_get_signature_count(uid_handle, sig_count.address())) {
      throw new Error("rnp_uid_get_signature_count failed");
    }

    let found_result = false;
    let is_good = false;
    for (let i = 0; !found_result && i < sig_count.value; i++) {
      let sig_handle = new RNPLib.rnp_signature_handle_t();

      if (
        RNPLib.rnp_uid_get_signature_at(uid_handle, i, sig_handle.address())
      ) {
        throw new Error("rnp_uid_get_signature_at failed");
      }

      let sig_id_str = new ctypes.char.ptr();
      if (RNPLib.rnp_signature_get_keyid(sig_handle, sig_id_str.address())) {
        throw new Error("rnp_signature_get_keyid failed");
      }

      if (sig_id_str.readString() == self_key_id) {
        found_result = true;

        let sig_validity = RNPLib.rnp_signature_is_valid(sig_handle, 0);
        is_good =
          sig_validity == RNPLib.RNP_SUCCESS ||
          sig_validity == RNPLib.RNP_ERROR_SIGNATURE_EXPIRED;
      }

      RNPLib.rnp_signature_handle_destroy(sig_handle);
    }

    return is_good;
  },

There are a couple of problems with this function.

First, a legitimate signature may not have an Issuer subpacket and hence rnp_signature_get_keyid will return NULL. (The only subpacket that a signature MUST include is the signature creation time subpacket). Several years ago the OpenPGP community introduced the Issuer Fingerprint subpacket to replace the Issuer subpacket. Currently, most implementations add both an Issuer subpacket and an Issuer Fingerprint subpacket to a signature. But, it would be unfortunately if Thunderbird were responsible for holding back this transition. (I've attached an example key whose signatures only use the Issuer Fingerprint subpacket and not the Issuer subpacket.)

Second, it is entirely possible for a signature to have multiple legitimate keyids. For instance, when moving from v4 to the upcoming v5 key format, it will be useful to create a v5 certificate with the same key material as an existing v4 certificate, and to add both keyids/fingerprints to signatures created with that key material. These keys will have different fingerprints and key ids, but signatures created by either variant can be verified using the other variant, because they share the same key material. By including both key ids in a signature, an OpenPGP client can identify all relevant certificates and find one that it can use. This allows users to more quickly transition to v5-based certificates and profit from the additional security protections that they offer while allowing them to continue to interact with correspondences who only have an OpenPGP implementation that supports the v4 format.

Finally, found_result is set the first time a signature with a matching key id is encountered. But, the first signature that has a matching key id might be invalid, and a later signature might be valid. found_result should only be set when rnp_signature_is_valid is also set.

I find it a bit unfortunate that the semantics of uidHasGoodSignature are not documented at all. What exactly is a "good signature"? Why is a signature that has expired considered good? Why is a third-party signature not considered to be good? This lack of documentation makes this type of audit more difficult.

Comment 1

[neal]

Attachment: no-issuer-subpacket.pgp

Secret-Key Packet, new CTB, 88 bytes
    Version: 4
    Creation time: 2021-04-20 08:55:21 UTC
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Pk size: 256 bits
    Fingerprint: C4086D4C3DC5043CF8E7AC004A3BF46D5138B26C
    KeyID: 4A3BF46D5138B26C
  
    Secret Key:
  
        Unencrypted
  
Signature Packet, new CTB, 199 bytes
    Version: 4
    Type: DirectKey
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Hash algo: SHA512
    Hashed area:
      Signature creation time: 2021-04-20 08:55:21 UTC (critical)
      Key expiration time: P1095DT62781S (critical)
      Symmetric algo preferences: AES256, AES128
      Notation: [e-mail]
        00000000  7a 85 20 3b 4b 5c c9 a6  4b 73 32 49 a2 70 df bb
        00000010  5b cb 21 fc 22 89 20 24  11 86 4e 7a 7e 30 51 ca
      Hash preferences: SHA512, SHA256
      Key flags: C (critical)
      Features: MDC
      Issuer Fingerprint: C4086D4C3DC5043CF8E7AC004A3BF46D5138B26C
    Digest prefix: 6503
    Level: 0 (signature over data)
  
User ID Packet, new CTB, 33 bytes
    Value: <[e-mail]>
  
Signature Packet, new CTB, 202 bytes
    Version: 4
    Type: PositiveCertification
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Hash algo: SHA512
    Hashed area:
      Signature creation time: 2021-04-20 08:55:21 UTC (critical)
      Key expiration time: P1095DT62781S (critical)
      Symmetric algo preferences: AES256, AES128
      Notation: [e-mail]
        00000000  f9 d8 5c 46 8c a9 97 e2  cc 08 ae bb b0 2d 68 59
        00000010  42 57 ec c4 00 1b 27 47  b9 d8 c5 b1 5b 67 c3 02
      Hash preferences: SHA512, SHA256
      Primary User ID: true (critical)
      Key flags: C (critical)
      Features: MDC
      Issuer Fingerprint: C4086D4C3DC5043CF8E7AC004A3BF46D5138B26C
    Digest prefix: 3F37
    Level: 0 (signature over data)
  
Secret-Subkey Packet, new CTB, 88 bytes
    Version: 4
    Creation time: 2021-04-20 08:55:21 UTC
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Pk size: 256 bits
    Fingerprint: 766308CDF936166FF748764DAD5253BBA6015327
    KeyID: AD5253BBA6015327
  
    Secret Key:
  
        Unencrypted
  
Signature Packet, new CTB, 372 bytes
    Version: 4
    Type: SubkeyBinding
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Hash algo: SHA512
    Hashed area:
      Signature creation time: 2021-04-20 08:55:21 UTC (critical)
      Key expiration time: P1095DT62781S (critical)
      Notation: [e-mail]
        00000000  82 82 24 9a 9c b8 29 c4  56 95 74 24 fb f9 ee 19
        00000010  2b d5 2e a6 09 1d c5 c7  db 3a 6d a3 88 de 77 f2
      Key flags: S (critical)
      Features: MDC
      Embedded signature:  (critical)
        Signature Packet
          Version: 4
          Type: PrimaryKeyBinding
          Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
          Hash algo: SHA512
          Hashed area:
            Signature creation time: 2021-04-20 08:55:21 UTC (critical)
            Notation: [e-mail]
              00000000  bb e1 43 ad 5c 26 72 a0  21 4a 47 df b2 4d eb 97
              00000010  89 fa 73 94 17 d5 e9 2c  10 b4 4e 7c c8 7d 57 a8
            Issuer Fingerprint: 766308CDF936166FF748764DAD5253BBA6015327
          Digest prefix: BDB8
          Level: 0 (signature over data)
        
      Issuer Fingerprint: C4086D4C3DC5043CF8E7AC004A3BF46D5138B26C
    Digest prefix: 9D60
    Level: 0 (signature over data)
  
Secret-Subkey Packet, new CTB, 93 bytes
    Version: 4
    Creation time: 2021-04-20 08:55:21 UTC
    Pk algo: ECDH public key algorithm
    Pk size: 256 bits
    Fingerprint: 473543567B4A2F8CEBF68B9E47B308B0F3C5963F
    KeyID: 47B308B0F3C5963F
  
    Secret Key:
  
        Unencrypted
  
Signature Packet, new CTB, 191 bytes
    Version: 4
    Type: SubkeyBinding
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Hash algo: SHA512
    Hashed area:
      Signature creation time: 2021-04-20 08:55:21 UTC (critical)
      Key expiration time: P1095DT62781S (critical)
      Notation: [e-mail]
        00000000  f6 0d ca e8 9c a6 2c bc  64 dd b7 c7 6e d2 72 66
        00000010  9b 25 95 20 15 7a 39 4e  34 4a bb d7 92 b5 9a 5d
      Key flags: EtEr (critical)
      Features: MDC
      Issuer Fingerprint: C4086D4C3DC5043CF8E7AC004A3BF46D5138B26C
    Digest prefix: EC28
    Level: 0 (signature over data)
  

Comment 2

[neal]

Attachment: multiple-issuer-subpacket.pgp

Secret-Key Packet, new CTB, 88 bytes
    Version: 4
    Creation time: 2021-04-20 08:59:42 UTC
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Pk size: 256 bits
    Fingerprint: BE31081E1A3FF0E0D3AE30E595927FA0A3E92052
    KeyID: 95927FA0A3E92052
  
    Secret Key:
  
        Unencrypted
  
Signature Packet, new CTB, 196 bytes
    Version: 4
    Type: DirectKey
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Hash algo: SHA512
    Hashed area:
      Signature creation time: 2021-04-20 08:59:42 UTC (critical)
      Key expiration time: P1095DT62781S (critical)
      Symmetric algo preferences: AES256, AES128
      Issuer: 95927FA0A3E92052
      Issuer: 0123456789ABCDEF
      Notation: [e-mail]
        00000000  3b 62 7b ea 36 bc d1 d5  cf 7a b4 5c 2f 5a 49 52
        00000010  f9 ef 6c 10 0c b3 d7 02  e2 b0 c6 49 ec 19 c7 32
      Hash preferences: SHA512, SHA256
      Key flags: C (critical)
      Features: MDC
    Digest prefix: D1FF
    Level: 0 (signature over data)
  
User ID Packet, new CTB, 39 bytes
    Value: <[e-mail]>
  
Signature Packet, new CTB, 199 bytes
    Version: 4
    Type: PositiveCertification
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Hash algo: SHA512
    Hashed area:
      Signature creation time: 2021-04-20 08:59:42 UTC (critical)
      Key expiration time: P1095DT62781S (critical)
      Symmetric algo preferences: AES256, AES128
      Issuer: 95927FA0A3E92052
      Issuer: 0123456789ABCDEF
      Notation: [e-mail]
        00000000  1e e6 b6 6e 3a 74 a9 02  c0 88 5b 3d 7a 8f 79 f8
        00000010  ce 7e da 6c 46 1e fe a0  ba 03 4c 11 6c ab 4c 77
      Hash preferences: SHA512, SHA256
      Primary User ID: true (critical)
      Key flags: C (critical)
      Features: MDC
    Digest prefix: AAF1
    Level: 0 (signature over data)
  
Secret-Subkey Packet, new CTB, 88 bytes
    Version: 4
    Creation time: 2021-04-20 08:59:42 UTC
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Pk size: 256 bits
    Fingerprint: 3F48FEB91E85FD7DB4E450A80673D969FFD80906
    KeyID: 0673D969FFD80906
  
    Secret Key:
  
        Unencrypted
  
Signature Packet, new CTB, 366 bytes
    Version: 4
    Type: SubkeyBinding
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Hash algo: SHA512
    Hashed area:
      Signature creation time: 2021-04-20 08:59:42 UTC (critical)
      Key expiration time: P1095DT62781S (critical)
      Issuer: 95927FA0A3E92052
      Issuer: 0123456789ABCDEF
      Notation: [e-mail]
        00000000  06 43 cc 1b db 44 77 37  07 18 6a e3 85 5b 13 19
        00000010  8e d2 46 ca c8 19 16 e6  8e c4 1a 1e ef 7f 30 31
      Key flags: S (critical)
      Features: MDC
      Embedded signature:  (critical)
        Signature Packet
          Version: 4
          Type: PrimaryKeyBinding
          Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
          Hash algo: SHA512
          Hashed area:
            Signature creation time: 2021-04-20 08:59:42 UTC (critical)
            Issuer: 0673D969FFD80906
            Issuer: 0123456789ABCDEF
            Notation: [e-mail]
              00000000  5a 7c 1e 9c 49 72 31 49  1e 95 e6 2e b8 bb 21 70
              00000010  25 c7 d4 c9 97 62 43 bc  6f bb 1d a6 10 60 40 ea
          Digest prefix: 0A28
          Level: 0 (signature over data)
        
    Digest prefix: 8A46
    Level: 0 (signature over data)
  
Secret-Subkey Packet, new CTB, 93 bytes
    Version: 4
    Creation time: 2021-04-20 08:59:42 UTC
    Pk algo: ECDH public key algorithm
    Pk size: 256 bits
    Fingerprint: E4B9871BD8FD39C19307DE8A39BE84AB5998AD21
    KeyID: 39BE84AB5998AD21
  
    Secret Key:
  
        Unencrypted
  
Signature Packet, new CTB, 188 bytes
    Version: 4
    Type: SubkeyBinding
    Pk algo: EdDSA Edwards-curve Digital Signature Algorithm
    Hash algo: SHA512
    Hashed area:
      Signature creation time: 2021-04-20 08:59:42 UTC (critical)
      Key expiration time: P1095DT62781S (critical)
      Issuer: 95927FA0A3E92052
      Issuer: 0123456789ABCDEF
      Notation: [e-mail]
        00000000  a6 d9 c6 18 99 ad 8d 5b  bd 2a f7 9c 39 21 c3 19
        00000010  b1 84 64 f9 d1 5d 6a e6  50 65 ef b7 2e ea 43 6e
      Key flags: EtEr (critical)
      Features: MDC
    Digest prefix: 14EA
    Level: 0 (signature over data)
  

Comment 3

[neal]

In my report, I wrote: "Second, it is entirely possible for a signature to have multiple legitimate keyids." This is the case, but this point does not really apply to self-signatures. It is relevant to signatures over data and third-party certifications.

Comment 4

[Nickolay Olshevsky]

First, a legitimate signature may not have an Issuer subpacket and hence rnp_signature_get_keyid will return NULL. (The only subpacket that a signature MUST include is the signature creation time subpacket). Several years ago the OpenPGP community introduced the Issuer Fingerprint subpacket to replace the Issuer subpacket. Currently, most implementations add both an Issuer subpacket and an Issuer Fingerprint subpacket to a signature. But, it would be unfortunately if Thunderbird were responsible for holding back this transition. (I've attached an example key whose signatures only use the Issuer Fingerprint subpacket and not the Issuer subpacket.)

Just a quick note:

1. rnp_signature_get_keyid() will not return NULL but error code in case keyid is not available.
2. if issuer subpacket is not present then it will look for the issuer fingerprint, and return part of it (which equals to the issuer key id as per RFC).

Comment 5

[neal]

Thanks for clarifying Nckolay. With respect to 1, I think you might be misremembering:

rnp_result_t
rnp_signature_get_keyid(rnp_signature_handle_t handle, char **result)
try {
    if (!handle || !result) {
        return RNP_ERROR_NULL_POINTER;
    }
    if (!handle->sig) {
        return RNP_ERROR_BAD_PARAMETERS;
    }
    if (!handle->sig->sig.has_keyid()) {
        *result = NULL;
        return RNP_SUCCESS;
    }

It looks like if the signature does not have a keyid, then it returns NULL and success.

The comment in rnp.h also seems to agree:

 *  Note: if key id is not available from the signature then NULL value will
 *        be stored to result.
 ...
RNP_API rnp_result_t rnp_signature_get_keyid(rnp_signature_handle_t sig, char **result);

Comment 6

[Nickolay Olshevsky]

Yeah, that point I missed, thanks. Absence of keyid is considered as successful function execution, but with empty result.
Basically was focused on 2 - that issued keyid is borrowed from the issuer fingerprint subpacket if there is no issuer keyid.

Comment 7

[Thomas D. (:thomas8)]

Anything we should do about this?

Comment 8

[neal]

The first thing that needs to be done is to clearly document the required semantics (this requires to many other functions in RNP.jsm). Whether a User ID is "good" is highly dependent on the context. Second, I would strongly recommend leaving the evaluation of self signatures and binding signatures to the OpenPGP implementation. The semantics are extremely tricky. See this test from the OpenPGP Interoperability Test Suite for an example of what I mean. If RNP doesn't actually implement the desired semantics, then I would open a bug report against RNP and request the desired functionality.

Comment 9

[Magnus Melin [:mkmelin]]

So is there some better way to currently in RNP to achieve the result for https://searchfox.org/comm-central/rev/3869c8207618a1f123d39c4d2195d3c4eb919bdc/mail/extensions/openpgp/content/modules/RNP.jsm#516?

Comment 10

[neal]

As you know, we've reimplemented the RNP interface that Thunderbird uses in terms of Sequoia, so I am broadly familiar with the RNP interface.

That said, I don't know if RNP provides the functionality that you need. But, RNP has been responsive to your requests in the past, so I'd recommend that if they don't have such an interface, that you ask for it. Nevertheless, they would first need a precise description of the desired semantics, which is missing.

Comment 11

[Nickolay Olshevsky]

We may update API, for sure, but that should require some practical use. In this particular case the only possible outcome I see is that some userid with tricky signature combination will not be displayed in GUI. Or am I wrong?

Do you have any real-world usage scenario, test key, whatever else, which is compatible with other implementations, and exposes any problem in this case?

Comment 12

[Wayne Mery (:wsmwk)]

Neal?

(In reply to Nickolay Olshevsky from comment #11)

We may update API, for sure, but that should require some practical use. In this particular case the only possible outcome I see is that some userid with tricky signature combination will not be displayed in GUI. Or am I wrong?

Do you have any real-world usage scenario, test key, whatever else, which is compatible with other implementations, and exposes any problem in this case?