Re: [openpgp] Review of crypto-refresh 07

[Paul Wouters <[e-mail]>]

On Wed, 16 Nov 2022, Falko Strenzke wrote:

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

Tanks for the review!

> # 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 agree this should be further discussed on the list with proposed text
changes once we know the behaviour we want to allow/forbid.


> ## 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.

Agree that the list should come up with some textual improvements that
clarify and cover these cases.

> ## 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.

The text "specification required" is a requirement to get an IANA number,
in which case you are on "these lists". So I believe the text is correct
and no changes are needed.

> ##  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

The alternative to not even checking MD5 is worse. The text states to
use this only for old (stored) messages, not new incoming messages.

> ### 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.

There is no specific algorithm for these "rough estimates" of strength.
We are simply quoting NIST and they have generalized these numbers. I do
not think a text change is needed here.

> ###  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.

Do you have specific text in mind? Split the listing in two, or remove
the entries that have no malleable ciphertext ?

> # 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."

Fixed.

> ##  6. Radix-64 Conversions
>
> "any printable encoding scheme that met the" → "that *meets* the"

I think "met" works fine here? But if others agree "meets" is better, we
can make that change.

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

But the text is have only "DSA (17) and Elgamal (16)", so I think that
is correct.

> ##  5.2.3.3.1. Algorithm-Specific Fields for Ed25519 signatures
>
> * One time "little-endian" is in brackets, one time not.

Fixed.

> ##  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").

Fixed.

> ## 12.5. EC DH Algorithm (ECDH)
>
>>  which SHOULD be one of three AES algorithms
>
> Insert "[...] be one of *the* three [...]"

Fixed.

> ## 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 [...]" 
> ?

The precense of a new feature should be safe. Either it can be ignored
or it is has its Critical bit set. If an implementation sees a Critical
feature it does not know, it knows to throw a fatal error. If it sees a
feature it does not know without Critical set, it can safely ignore it.
This is what is meant by "backwards-compatible".

What is left would be a change that is somehow critical but cannot be
expressed in such a way, which is what is meant here.

> ## 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.

I cannot locate this text? Perhaps it was changed since you read it, or
this was a note of you based on an older version?

> ##  14.8. Escrowed Revocation Signatures
>
> "The preferred way for her to do this is produce" → "is *to* produce"

Fixed.

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

I think "themselves" is clear.

> # 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.

I would like to see this suggestion discussed on the list and follow the
WG consensus on this.

> ## 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.

Please provide text and location where you would want this.

> ## 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.

That was also true before AEAD though, even if it was external to the
encryption algorithm? I would also say that "digital signature" is
covering the "message integrity" part. But if more people feel we should
add it to this list, we can change it.

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

Agreed. I added the following line (but am open to improved text of
course):

 	If the "Generate?" column is not "Y", the S2K entry is used only for reading in backwards compatibility mode and should not be used to generate new output.


> ##  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.

Changed to:

 	If the "Generate?" column is not "Y", the Secret Key protection details entry is used only for reading in backwards compatibility mode and MUST NOT be used to generate new output.

> ## 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.

I think what this is trying to do is prevent out of bounds writing,
which could possibly lead to remote code execution. I've changed it to:

 	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 abort without writing outside the indicated range and MUST
 	treat the packet as malformed and unusable.


> ## 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.

I don't think it says that? It says that if it is not one, it might be a
legacy packet? But it is surely not a non-legacy packet. If someone has
proposed text they prefer, let's discuss on the list.

> ##  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.

This seems more that a spelling issue and worthy of list discussion :)

> ## 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.

I would also like to hear from others here before making a change.

> ##  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*)"

I don't think that is needed, as | represents an alternative, so clearly
it works as an OR and then the binding becomes logical anyway, eg:

 	: Signature Packet, OpenPGP Message | One-Pass Signed Message.

Though reading this now, that makes no sense for this one:

 	: OpenPGP Message | OpenPGP Message, Padding Packet.

So even your interpretation is wrong :)

Perhaps this last one should have been:

 	: OpenPGP Message , OpenPGP Message | Padding Packet.

This could use some clarification on the list as well before making changes.

Thanks again for the review!

Paul