[openpgp] Review of crypto-refresh 07

[Falko Strenzke <[e-mail]>]

Hi Stephen,

Below you find the review of the draft in version 07 that I made. I also 
attached an HTML version of the same document.

  Review Crypto-Refresh 07 by Falko Strenzke

  I read the whole draft in version 07. I split my comments in three 
categories:

  * Major comments: Things that definitely should be corrected / 
addressed before publication in my view.
  * Spelling / Typos: Language errors that should be corrected.
  * Minor comments: Things I would prefer to correct but where, if they 
are not corrected, I would still consider the draft reach a sufficient 
quality to be published.


# Major comments

##  5.13.2. Version 2 Sym. Encrypted Integrity Protected Data Packet Format

In my opinion, the sentence "Furthermore, an implementation can securely 
reply to a message even if a recipients certificate is unknown by 
reusing the encrypted session key packets and replying with a different 
salt yielding a new, unique
   message key" has a considerable security implications. I raised these 
concerns already in 
https://gitlab.com/openpgp-wg/rfc4880bis/-/issues/78. In my 
understanding it is not relevant to discuss these security concerns 
themselves, as I
   think it is clear that they are well understood and consciously taken 
into account. However, I consider it necessary to point out the involved 
risks in the document,
   since the crypto-refresh already introduces the mechanism for PKESK 
reuse and any implementation can decide to use it once supporting v2 
SEIPD packets. Furthermore, existing inconsistencies regarding the 
description of the now modified
   encryption mechanism should be resolved.

To get everyone onto the same page I briefly summarize the new mechanism 
for PKESK reuse that is introduced in the crypto-refresh:

1. a number of one of more v5 PKESK packets encrypt the session key for 
each recipient
2. a per-message key is derived from the session key and a per-message 
salt value
3. a v2 SEIPD packet is encrypted using the per-message key
4. the recipient can reply reusing the same v5 PKESK packets he received 
using a different per-message salt value

Specifically my comments are the following:

First of all, I consider it necessary to update section 2.1 to reflect 
the new mechanism. There we find the statements:

 > Each symmetric key is used only once, for a single object. A new 
"session key" is generated as a random number for each object (sometimes 
referred to as a session). Since it is used only once, the session key 
is bound to the message and transmitted with it.

 > [...]

 > 2. The sending OpenPGP generates a random number to be used as a 
session key for this message only.

 > [...]

 > 4. The sending OpenPGP encrypts the message using the session key, 
which forms the remainder of the message.

This is clearly not true any more, as with the mechanism for PKESK reuse 
also session keys are reused and in v2 SEIPD packets the payload data is 
not encrypted by the session key. This introductionary section is 
relevant for anyone who wants to assess the security features of the 
OpenPGP protocol without working through the entire specification. Thus 
it should reflect the new mechanism appropriately.

Furthermore, I would strongly recommend to devote a section under the 
"Security Considerations" to the new mechanism.


## 9.2. ECC Curves for OpenPGP

 > The "Field Size (fsize)" column represents the field size of the 
group in number of octets, rounded up, such that [...] or scalars [...] 
for the curve can be represented in that many octets.

The assumption that a scalar, i.e. a group element, always fits into an 
array long enough to hold a field element, is not correct: according to 
Hasse's theorem for Weierstraß GF( p ) curves, we find for the base 
point order N

   N ≤ p + 2 √p + 1

Thus, depending on the curve, scalars such as the private key can be one 
bit longer than the field size, and thus also need one more octet. 
secp160k1, secp160r1, secp160r2, and secp224k1 are examples of a domain 
parameter sets where the representation of N needs one more octet than p.
I do not think that there is a need (or desire) to correct the OpenPGP 
specification in this point. I would not expect that the need arises to 
support any new elliptic curves, and the ones listed above most likely 
are not desired to be supported at all.
But I think a note should be given in the quoted section (or another 
appropriate place) regarding this restriction, as an OpenPGP 
implementation may behave undefined when attempting to use it with any 
of the curves listed above (in an extension implementation of any kind). 
Implementations should be prepared to reject curves with this property 
for security reasons.


## 9. Constants

 >Note that these tables are not exhaustive lists; an implementation MAY 
implement an algorithm not on these lists, so long as the algorithm 
numbers are chosen from the private or experimental algorithm range.

Is this statement really
correct given the "specification required" method for registering many 
of the constants? Maybe it makes sense to explain that here as well.

##  14. Security Considerations

### MD 5
 > They MAY continue to consider old signatures that used MD5 as valid.

Would it not be more appropriate to say at least "SHOULD NOT" if not 
"MUST NOT" ? MD5 is fully broken since a long time. See also the 
sentence a little bit further down explaining for the case of finding 
collisions
 >Should this ever occur, a revision will have to be made to this 
document to revise the allowed hash algorithms

### Table 32

"Table 32: Key length equivalences" would benefit from making clear to 
what algorithms "Asymmetric key size" pertains and from the addition of 
EC key sizes.

###  14.7. Avoiding Ciphertext Malleability

The list following the sentence "Any of the following OpenPGP data 
elements indicate that malleable ciphertext is present:" mixes instances 
of malleable ciphertext formats with non-malleable ciphertext formats 
where the ciphertext has been manipulated. This should be corrected.

# Spelling / Typos

##  5.2.3.5. Signature Subpacket Specification
"The purpose of the critical bit is to allow the signer to tell an 
evaluator that it would prefer a new, unknown feature to generate an 
error than be ignored." →  should read "*rather* than be ignored."

##  6. Radix-64 Conversions

"any printable encoding scheme that met the" → "that *meets* the"

## 9.1. Public-Key Algorithms
"Note that an implementation conforming to the previous version of this 
standard ([RFC4880]) have only DSA [...]" → "[...] *has* only DSA [...]"

##  5.2.3.3.1. Algorithm-Specific Fields for Ed25519 signatures

* One time "little-endian" is in brackets, one time not.

##  11.3.2.1. Packet Versions in Encrypted Messages
 > Since a v1 SEIPD does not contain a symmetric algorithm identifier, 
so all ESK packets preceding a v1 SEIPD payload MUST be either v3 PKESK 
or v4 SKESK.

Delete the word "so" after the comma (or alternatively the initial "Since").



## 12.5. EC DH Algorithm (ECDH)

 > which SHOULD be one of three AES algorithms

Insert "[...] be one of *the* three [...]"


## 13.11. Meta-Considerations for Expansion

 > If OpenPGP is extended in a way that is not backwards-compatible, 
meaning that old implementations will not gracefully handle their 
absence of a new feature [...]

I don't understand this sentence. Is there a misspelling? Is it supposed 
to mean "[...] will not gracefully handle *the presence* of a new 
feature [...]" ?

## 14.7. Avoiding Ciphertext Malleability

 >When encrypting to one or more public keys: all recipient keys 
indicate support for version

Insert "If" after the colon.

##  14.8. Escrowed Revocation Signatures

"The preferred way for her to do this is produce" → "is *to* produce"

"rather than generating a revocation signature themselves." → 
"himself/herself".



# Minor comments

## A general note on terminology used to refer to bytes

The documents uses two ways to refer to bytes in an octet string:

* "left(most)", "right(most)". This is pretty much common sense in 
specifications and is almost accurate, since it seems common sense that 
an octet string is imagined as written with increasing indexes from left 
to right. Otherwise the terms "left" and "right" of
   course make no sense for information processing machines.
* "high (order)", "low (order)". Speaking of high and low order octets 
only makes sense when two conditions are fulfilled:
   * (a) the octet string has an interpretation as a number or
   * (b) the endianness of the number representation is known.

   There are instances of the use of this terminology where this 
additional context does not apply. Sometimes, in these cases a 
"leftmost" / "rightmost" definition is given additionally. Sometimes it 
stands for itself. My recommendation is to remove the "high order" / 
"low order" octet specifications for enhanced clarity and replace it 
everywhere with the "leftmost"/"rightmost" terminology.

## A general note on terminology used to refer to bits

* In certain places bits in a byte are referred to by number, e.g. "bit 
7". There is an instance where the meaning of this numbering is 
explained, but it is contained in a section under which it is not easily 
found. I recommended to
   introduce this notation in a preliminary section for easy reference.

## Section 2: "General Functions":
regarding the list:
 > digital signatures, encryption, compression, Radix-64 conversion"

The list could be extended by "message integrity", as AEAD is now 
supported which provides this security notion.

##  Table 1: S2K type registry
The text does not explain what the column "Generate?" indicates.


##  3.7.2.1. Secret-Key Encryption
 >Each row with "Generate?" marked as "No" is described for backward 
compatibility, and MUST NOT be generated.

This sentence makes a statement about generating table rows, which is 
clearly not what is meant.

For a proper sentence, insert "[...], and *keys with the given 
parameters* MUST NOT [...]"

## 4.1 Overview

 > In the event that a subfield length indicator within a packet implies 
inclusion of octets outside the range indicated in the packet header, a 
parser MUST truncate the subfield at the octet boundary indicated in the 
packet header. Such a
truncation renders the packet malformed and unusable.

If the packet becomes invalid altogether, it is not necessary to 
describe the truncation of the subfield. In the current form, this 
paragraph seems a bit confusing and bears the risk
that it is interpreted differently by readers. It should be clearly said 
by the specification if a packet becomes malformed and must be ignored / 
flagged as erroneous when one of its subfields refers to octets outside 
the packet's range.

## 4.2 Packet Headers

 >Bit 6 -- Always one (except for Legacy packet format)

If this is supposed to specify that a legacy packet can be determined by 
Bit 6 being 0 – that is not what is being said here. So this might be 
worth clarifying by an explicit statement.


##  5.2.3.3.1. Algorithm-Specific Fields for Ed25519 signatures

* This section specifies to encode numbers in little endian format into 
an MPI. An MPI is specified in Section 3.2 as using big endian encoding. 
So it would be useful to elaborate in detail how the encoding of the 
little endian value is to be performed.

## 5.7 Symmetrically Encrypted Data Packet (Tag 9)

 >As a pedantic clarification, [...]

The quoted subclause is superfluous, not fitting the style required for 
a formal specification and in my opinion should better be removed.

##  11.3. OpenPGP Messages

 > (comma represents sequential composition, and vertical bar separates 
alternatives)

I propose to add "([...] alternatives
*, where the comma operator takes precedence over the bar operator*)"



-- 

*MTG AG*
Dr. Falko Strenzke
Executive System Architect

Phone: [phone]
E-Mail: [e-mail]
Web: mtg.de <https://www.mtg.de>


*MTG Exhibitions 2022 – Let´s meet again!*

------------------------------------------------------------------------
<https://www.itsa365.de/de-de/companies/m/mtg-ag>

MTG AG - Dolivostr. 11 - 64293 Darmstadt, Germany
Commercial register: HRB 8901
Register Court: Amtsgericht Darmstadt
Management Board: Jürgen Ruf (CEO), Tamer Kemeröz
Chairman of the Supervisory Board: Dr. Thomas Milde

This email may contain confidential and/or privileged information. If 
you are not the correct recipient or have received this email in error,
please inform the sender immediately and delete this email. Unauthorised 
copying or distribution of this email is not permitted.

Data protection information: Privacy policy 
<https://www.mtg.de/en/privacy-policy>