Re: [openpgp] personal review of draft-06

[Daniel Kahn Gillmor <[e-mail]>]

On Thu 2022-06-30 21:08:00 +0100, Stephen Farrell wrote:
> I gave draft-06 a full read through

Sorry for the delay in processing this.  I've pushed the stuff that i
thought was easy to do, in-charter, and non-controversial to the
"further editorial cleanup" MR 270
(https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/270)

The stuff that i didn't get to i've replied to in more detail here.
Note that there were three groups of additional groups of changes that
i'd appreciate closer review of, which are on MRs !271, !272, and !273
as described below.

> (3) 3.7.2 - we don't have any "MUST implement" here -
> is that intended? Argon2 is RECOMMENDED to use but I
> didn't see any MUST (or missed it:-). I guess one could
> argue that a MUST isn't needed there for interop but
> nonetheless maybe worth considering.

I don't think there is a MUST here.  An OpenPGP implementation that
doesn't do S2K at all will still work with unlocked keys, asymmetric
decryption and signing, and of course encryption and signature
verification.

> (5) 5.12.1: If there's deployment already (I don't
> know) shouldn't we define e.g. a PNG encoding format
> now?  Also, the [JFIF] reference is from 1996! Is that
> still correct? (I've no objection to the current text
> if that's what's really supported but wouldn't be
> surprised if more was commonly supported and there's an
> easy opportunity here to improve interop.)

I don't think adding PNG would be in-charter for the current WG.
However, !270 includes an update to the reference to ISO/IEC 10918-5,
which is a formalization of the 1996 de facto standard.

> (8) 11.1.1 (and elsewhere): can many marker packets be
> present? Same question for padding packets in 11.4.
> The phrasing is a bit ambiguous for me and it could
> lead to different implementers doing different things
> in a way that'd harm interop and might not be spotted
> so easily.

I think this is a question for the interop test suite -- the text as
written reads to me like an arbitrary number of padding and marker
packets can appear (and be ignored) anywhere in any stream, but i'm sure
that implementations don't all conform to that.

https://tests.sequoia-pgp.org/#Marker_Packet

I've opened this to get more visibility:

https://gitlab.com/sequoia-pgp/openpgp-interoperability-test-suite/-/issues/107

However, i'm not convinced that i have a clear understanding of how to
treat this in the current draft.

> - general: the version numbers mentioned are a bit
>    confusing/all-over-the-place, would it be useful to
>    have a table somewhere to help the reader?

I agree, and this is a bit of a mess, but i think the tables that we
have now are probably good enough to make some sense here.  These tables
in the current draft are:
https://www.ietf.org/archive/id/draft-ietf-openpgp-crypto-refresh-08.html#table-16
and
https://www.ietf.org/archive/id/draft-ietf-openpgp-crypto-refresh-08.html#table-27,
but their equivalents were already present in draft -06, so i don't know
how to do better.

I've opened https://gitlab.com/openpgp-wg/rfc4880bis/-/issues/162 to
track the broader issue, in case a future WG has more of an inspiration
to clean up further.

> - toc: 11.3.2.1/13.3.1.1 stand out as the only entries
>    at that level

11.3.2.1 is about Packet Versions as a further constraint on packet
sequences for encrypted messags.  I've added a peer remark about OPS
packet version alignment so it's not on its own.

13.3.1.1 is about the special case of "uncompressed" compression, in the
same way that 13.2.1 is about the special case of "plaintext"
encryption.  I don't think it's a problem to call out either of these
cases with no additional peer-level callouts.

> - 5.1.3/5.1.4: "MUST make a new PKCS#1 encoding for
>    each key" I guess that's related to some timing
>    attack or something?  If so, a reference might be
>    good.  (Is there a related security consideration?)

Reading RFC 8107 says:

      It is RECOMMENDED that the pseudorandom octets in Step 2 in
      Section 7.2.1 be generated independently for each encryption
      process, especially if the same data is input to more than one
      encryption process.  Haastad's results [HAASTAD] are one
      motivation for this recommendation.

i've filed an erratum for that document at
https://www.rfc-editor.org/errata/eid7405 (the author's name is Hastad,
not Haastad), but i think this is the motivation.

I admit i haven't tried to work out the details of whether Hastad's
attack is plausible for OpenPGP with modern, reasonable public keys
whose secret keys are not known to the attacker.

> - 5.2.3.5 - what does "in a polite manner" mean? Not
>    clear to this reader anyway.

Great question!  this is in RFC 4880 too, but it's just as vague there.
I've created MR 271 from branch "subpacket-implementation-guidance"
(https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/271) that
tries to clarify, but it gets into more substantial (non-editorial)
changes, so i hope it can get some extra review.

> - 5.2.3.7 - "may have a" - is that "MAY have a"? (1.1
>    would imply not, so just checking it's not an error)
>
> - 5.2.3.7 - "in fact has" seems to me "MUST have" - is
>    that right?
>
> - 5.2.3.7 - "user name" - that's the 1st occurence of
>    that term - "user ID" is used elsewhere - is that
>    deliberate? Or does that need to be defined? (Perhaps
>    as (part of) the value contained in a user ID?)
>
> - 5.2.3.7: is "rewriting the signature" clear enough?

I think i've cleaned all of these up, but these changes seem substantive
enough to warrant their own MR 272 for branch "clarify-self-signatures"
(https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/272)

> - 5.2.3.21: I don't understand the 0x00 entries in the
>    1st column of table 9.
>
> - 5.2.3.21: Why is table 10 present but empty?

I've dealt with these on a separate branch
("clean-up-notation-registries") to clean up the documentation about
notations, which is now MR 273.  Please review!
(https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/273)

> - 5.9: This is probably handled by all implementations
>    already but I wondered if there's anything worth
>    saying about the characters that can be used in
>    filenames? Some OSes/filesystems e.g. don't like a
>    colon in a file name. The only reason this could be
>    worth a mention would be if some implementation
>    commonly truncated the name from the packet (at an
>    "illegal" character) when writing to disk which could
>    lead to overwriting some sensitive data.  Probably not
>    worth a mention though as I'm not sure there's a
>    stable enough list of dodgy characters to be useful
>    for those creating packets.

I didn't make this modification, but if someone has a reference to a
"safe characters for embedded filenames" document, i'd be happy to have
this draft point to it.

> - 5.9: Is [PAX] still a reaosnable reference to use?

Sure, it's certainly still valid.  If someone has a reliable reference
for the tar archive format or something else more popular than pax, i'd
be happy to review a change, though.

> - 5.10: RFC2822 has been obsoleted by 5322. I don't
>    recall that should make a difference but might be
>    worth someone checking there're no i18n things in
>    5322 that'd cause issues.

I think this was meant to be 5.11 ("User ID Packet").  The User ID
convention is actually *not* either 2822 or 5322.  but fixing it is
probably out of scope for the current charter (see
https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/23), so i
think it's best to leave it so that the crypto-refresh is wrong in the
same way as RFC 4880, rather than creating two subtly different
incorrect statements about the uid conventions.

> - section 7: Should we say we now prefer PGP/MIME?
>    (rfc3156) I'm not sure if both that and this
>    (cleartext signatures as per this section) are in
>    widespread use, so I'm not recommending we make a
>    change just asking.

Certainly PGP/MIME is preferable to the cleartext signature framework,
but PGP/MIME only works in the e-mail context.  I think a document that
says how to use OpenPGP in e-mail should clearly and unequivocally
indicate that "inline signing" is a terrible idea, but i don't think
that belongs in the crypto-refresh.

> - 9.5: Can we do better than "any recent signature" for
>    md5 etc?  Maybe not, but no harm asking:-) We do say
>    a bit more for what "old signature" means, but is
>    recent == !old?

I'm not sure i know how to be more precise here.  I think it's up to
implementers to pick the appropriate cutoff dates.  I didn't propose any
changes.

> - 9.5: Does "MUST NOT validate" mean that you must not
>    signal that a signature is valid or that you must not
>    run the validation algorithm? I assume the former.
>    Not sure if it's worth being that bit more precise
>    though, is it?

I think it's either, really -- certainly must not indicate that such a
signature is valid, but if you can't indicate that it's valid, why would
you bother to run the algorithm?  I think this text is unambiguous as it
stands (albeit without a clear cutoff) so i didn't propose a concrete
change.

> - 10.2.1.1: I guess we could ask that new image formats
>    already have some MIME type assigned as well. Would
>    we want that or is it ok as-is?  There are a *lot* of
>    such image format at [2] - probbaly more than we'd
>    ever want but we could say the ones defined for PGP
>    need to be on that list too.
>
>     [2] https://www.iana.org/assignments/media-types/media-types.xhtml#image

I don't think there's any specific reason to require a MIME type for
image attribute types.  For one thing, i don't think there's much
appetite for image attributes these days, and this change would likely
not be in-charter anyway.

> - 10.3.1.1: Not sure if referring to the table numbers
>    here is right - we probably want to refer to the IANA
>    registries by name instead. (Can be fixed later
>    though.)

I've left this up to any future corrections to IANA instructions.

Hopefully the concrete changes i've proposed as a result of this review
will be easier and quicker to incorporate in the draft!

     --dkg