[openpgp] Review of draft-ietf-openpgp-crypto-refresh-07

["Hammell, Jonathan F" <[e-mail]>]

UNCLASSIFIED / NON CLASSIFI?

Hi all,
As promised at IETF 115, I've reviewed draft-ietf-openpgp-crypto-refresh-07.  I haven't been following the mailing list too closely, so I hope my comments are not duplicating items already discussed.  

## Comments

- Section 5.2.3.4, paragraph 2: Examples of advisory information could be given, but more importantly, examples of subpackets that are security critical and must not be in the second field.  This is also discussed in 5.2.3.6, paragraph 2, so these two sections could refer to the same guidance if it was included in the Security Considerations, but it could also be indicated in each section describing subpacket types as recommended in following comments.

- In Section 5.2.3.12 (Preferred AEAD Ciphersuites), indicate this subpacket must be in the cryptographically-protected field of a signature packet to avoid this subpacket being removed to encourage use of default algorithms.  

- Section 5.2.3.17 (Revocable): If a signature is marked as non-revocable in an unhashed subpacket, what is the expected behavour if that signature is revoked?  Perhaps this subpacket should be recommended to be in the cryptographically-protected field.

- Section 5.2.3.32 (Issuer Fingerprint): This subpacket should also be recommended to be cryptographically-protected.

- Section 5.2.4.1 (Subpacket Hints): The recommendation is that an implementation use the "last subpacket in the signature" to resolve conflicting information, but the unhashed subpackets are in the second field and should be less trusted.

- Section 5.13.2 (V2 SEIPD Packet): The third paragraph describes the feature that the KDF mechanism allows a reply by reusing the encrypted session key packet.  Presumably, this allows for a reply-all feature without having to acquire everyone's certificate.  I could imagine an implementation supporting this silently, where the sender replying may not be aware that they have not verified the identity of all recipients.  This privacy consideration should be noted.  Perhaps, we could recommend that implementations have a user configuration option of whether to support this feature?  Alternatively, but maybe less functionally clear, there could be an indicator of whether to allow this in one of the packet formats.

Perhaps it should also be recommended that a sender encrypt to their own public key to allow decryption of replies without having to lookup the sent PKESK packet.

- Section 3.7.1, Table 1: I'm confused what the column "Generate?" indicates.

- Section 3.7.2 (String-to-Key Usage): it is confusing whether the caveat about high-entropy in the third sentence applies to only Salted S2K or both Simple S2K and Salted S2K.  If it applies to both, then it would be better to word as "Unless an implementation knows that the string is high entropy (for example ...), it MUST NOT use Simple S2K and SHOULD NOT use Salted S2K." 

- Sections 5.1.3 and 5.1.4 say "The value 'm' in the above formula is the plaintext value described above...", but "plaintext" is not mentioned above.  I assume it is "The resulting octet string" from the last sentence of 5.1.2, but the linking of this could be more clear.

- In the paragraph following the list in Section 5.3.2 (v5 SKESK), the first sentence is very long and it is not clear what it is describing.  I suggest "A key-encryption key is derived using HKDF (see [RFC5869]) with SHA256 has the hash algorithm.  The Initial Keying Material (IKM) for HKDF is the resulting key derived from S2K.  No salt is used.  The info parameter for HKDF is comprised of the Packet Tag in new format encoding (bits 7 and 6 set, bits 5-0 carry the packet tag), the packet version, and the cipher-algo and AEAD-mode used to encrypt the key material. [New paragraph]"  A diagram would help here.  See also the similar description in 5.5.3.

- In Section 5.5.3, the packet contents list is not clear about where the authentication tag is placed when the string-to-key octet is 253.  This is mentioned in text of the second last paragraph, but it would be helpful to have it as a conditional item in the contents list.

- Section 5.6, paragraph 3 says the Compressed Data Packet's body "contains an block".  Ignoring the incorrect indefinite article, the body does not comprise of just one block, but rather "a series of blocks" as described in RFC 1951. 

- The last paragraph of 15.13.2 (V2 SEIDP) says the nonce for AEAD mode consists of an initialization vector and 64-bit chunk index.  The nonce lengths given in 5.13.3, 5.13.4, 5.13.5 are equal to the IV length in Table 25 of 9.6.  This is inconsistent, and likely the column title in Table 25 should be "Nonce length (octets)".  

- Section 11.1.4, paragraph 2 has multiple interpretations for the "MUST" requirement.  Either the primary MUST always be an algorithm capable of making signatures in order to create self-signatures, or if one wishes to create self-signatures, then the primary key MUST be capable of making signatures.


## Nits

I realize that many of these exist in RFC 4880, but it might help readability if these can be addressed.

- Section 1.1 defines "OpenPGP" as "security software", but the last paragraph says "'OpenPGP' refers to the protocol described...".  These seem to conflict.

- Section 2.1 differentiates between the sender and the OpenPGP software implementation.  The latter is confusingly referred to as sending/receiving "OpenPGP" in the sequence of steps (see comment on the term above).  In 2.2, it is referred to as sending/receiving "software".  I suggest using "software" in 2.1 to be consistent with 2.2.

- Section 2.1 seems to overload the term "message".  I recommend Step 3 says "These 'encrypted session keys' are prefixed to the message."  Then in Step 4, replace "... which forms the remainder of the message" with "... which is attached to the message" similar to Step 4 of Section 2.2.  

- Section 2.1, Step 6 mentions decompression, but there was no mention of optionally compressing the message in earlier steps.

- In Section 2.2, the term "hash code" is used, but that only appears in this section.  Section 5.2.2 uses "hash value" and the verb "hashes" is used in other places. For example, this could be simplified as "The sending software hashes the message".

- Section 2.2, Step 4, refers to "the binary signature", but this suggests a conversion that is not described here.  I recommend removing "binary".

- A brief opening sentence could be added to Section 2.3: "OpenPGP implementations may support the compression of data."

- The Example in Section 3.2 says that "all numbers are in hexadecimal" but, for example, the value "511" is decimal.  Replace with "all numbers in the octet strings identified by square brackets are in hexadecimal".

- In Section 3.2.1, remove the second "used" in the first line and add a closing parenthesis to "(see Section 12.2".

- Section 3.3 introduces Key IDs and says that they cannot be assumed to be unique, but it would be helpful to at least mention fingerprints as the better option for a unique value. I suggest renaming 3.3 as "Key IDs and Fingerprints" and then replace the last sentence with "A fingerprint is cryptographically unique per key, but may be calculated differently according to the version of the key.  See Section 5.5.4 for more details on Key IDs and fingerprints."

- Section 3.4 cites ISO/IEC 10646:1993, but there are many newer editions.  Alternatively, the citation could be to the Unicode Consortium (https://www.unicode.org/reports/).

- In Section 3.7, the second sentence starts with "They are used...", but is is not the S2K specifier that is used "to encrypt the secret part of private keys".  Instead, start the sentence with "Passphrases requiring use of S2K conversion are currently used in two places:...", then the last part of the sentence can be simplified to remove "... to convert passphrases to encryption keys ..."

- The second sentence of Section 3.7.1.1, paragraph 2 says "The manner in which this is done depends on the size of the session key (which will depend on the cipher used)..." This could be more clear as follows: "The manner in which this is done depends on the required size of the session key (which depends on the cipher the session key will be used with)..."

- The last paragraph of 3.7.1.3 (Iterated and Salted S2K) is confusing: "Then the salt, followed by the passphrase data, is repeatedly hashed until the number of octets specified by the octet count has been hashed..."  In the previous paragraph, it is explained that the octet count is not an iteration count.  So what I think is meant here is that "Then the salt, followed by the passphrase data, is repeatedly processed as input to each hash context until the number of octets specified by the octet count has been reached.  The input is truncated to the octet count, except if the octet count is less than the initial size of the salt plus passphrase.  That is, at least one copy of the full salt plus passphrase will be provided as input to each hash context regardless of the octet count."

- In Section 3.7.1.4 (Argon2): s/kibibytes/kilobytes

- Section 4.2 does not make it clear that Bit 6 of the Packet Tag is used to identify Legacy packet formats.  It says Bit 6 is "Always one (except for Legacy packet format)", but it would be helpful in the following paragraph to start with "A Packet Tag with a zero in Bit 6 indicates a Legacy packet format."  Or add this sentence to the beginning of 4.2.2. 

- The last paragraph of Section 4.2.1.4 (Partial Body Lengths) lists data packet types, and maybe should be revised to include "encrypted and integrity protected" data packets, unless it is not intended for that packet type as per the final sentence.  

- In the first sentence of Section 4.2.3 (Packet Length Examples), clarify as s/"packet lengths"/"packet body lengths".  Despite the final paragraph of this section, this was still ambiguous.  

- In Section 5.1, the initialism "SEIPD" is used but not defined.  There is a reference to Section 11.3.12.1, but it is not defined there either.  

- Sections 5.1 and 5.3 say "Zero or more [PKESK] packets and/or [SKESK] packets may precede an encryption container...", but with "zero or more" you don't need to use the word "may".  In 5.3, there is a typo here as well (s/"a an"/an)

- In Section 5.5 it says there are "two major versions", but 5.5.2 says there are "three versions".

- In Section 5.5.3, the items listed as "[Optional]" seem like they are actually conditional.  The list in Section 7 is more clear for describing conditional items.

- Section 5.5.3, item 6: s/"an one-octet"/"a one-octet"

- Section 5.5.5.6, bullet 3 says it is a "variable-length field", but the sub-items are each one octet.

- In the list in Section 5.13.2, add the word "identifier" to the end of "cipher algorithm" and "AEAD algorithm"

- Section 9.1, Table 19, should note that ID 0 is reserved since other reserved fields in the registry are noted.

- Section 11.3.1, sentence following the list: s/"a series OpenPGP packets"/"a series of OpenPGP packets"

- Section 11.3.2.1, paragraph 2: s/"since a v1 SEIPD... , so all ESK packets"/"since a v1 SEIPD... , all ESK packets"


Best regards,
Jonathan

--
Jonathan Hammell, 
Canadian Centre for Cyber Security