Re: [openpgp] Review of crypto-refresh 07

[Andrey Jivsov <[e-mail]>]

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

The probability to hit this condition is at most 2^-80 for these new curves.

When this condition is known to occur, the key's security strength is only
40 bits. In general, we don't need to special-case odd key values, as they
are expected to be within the key space.

However, in this case there will need to be special processing due to the
overflow into the next 64-bit word (or a byte at least). This may result in
a side channel information, either due to time needed to process this new
condition, new code path, or even the key size on disk.

For these reasons I think the best fix for this is to treat the overflow of
private value over log2(p) bits as an error. If detected, the key should be
regenerated.

This error is an unfortunate side effect  of supporting new ECC curves.
However, the error handling can be hidden inside the key generation routine
with e.g. a maximum loop count over key generation fixed at 2.
Specifically, if we detect this condition, generate again, and if this
fails, return a key generation error to the higher level.



On Wed, Nov 16, 2022 at 12:21 AM Falko Strenzke <[e-mail]>
wrote:

> 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>
> _______________________________________________
> openpgp mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/openpgp
>