Re: [MLS] AD review draft-ietd-mls-protocol-16

For those noticing a lot of GitHub traffic today, here's where we stand:

* Since Paul put his comments in a format that could be consumed by a tool (
https://github.com/mnot/ietf-comments/), the tool to import them as issues
* Some issues with the tool caused a bunch of duplicate issues to be created
* I tagged these duplicates as "duplicate" and closed them.  Note that
these are "duplicate" only in the sense that they are literally identical
to another issue, not anything to do with the substance.

At this point, all of Paul's comments should be filed as issues. Please let
me know if you spot any missing.

--Richard

On Mon, Dec 5, 2022 at 12:56 PM Richard Barnes <rlb@ipv.sx> wrote:

> Thanks, Paul!  I'll take a look and generate some Issues/PRs for
> discussion on Thursday.
>
> On Mon, Dec 5, 2022 at 11:45 AM Paul Wouters <paul.wouters=
> 40aiven.io@dmarc.ietf.org> wrote:
>
>> # Sec AD review of draft-ietf-mls-protocol-15
>>
>> CC @paulwouters
>>
>> Please refer to
>> https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/
>> for more information about how to handle DISCUSS and COMMENT positions.
>>
>> This review uses the format specified in
>> https://github.com/mnot/ietf-comments/ which allows
>> automated tools to process items (eg to produce github issers)
>>
>>
>> ## DISCUSS
>>
>> ### Interaction of MLS and application
>>
>> I find the interaction with the application layer not clear. Not much is
>> mentioned in this
>> document. How can a group have an administrator? How is that enforced at
>> the MLS layer?
>> How do ACLs interact with MLS? (How are DoS attacks mitigated (eg spam
>> attempts to join any group). What hooks would be in the authentication
>> service and/or delivery server? How would clients know the properties of
>> these two services?
>>
>> This includes how it is not clear to me how race conditions are handled.
>> What if two members
>> try to expel each other at the same time? What if the group is so large,
>> members always get
>> their epoch wrong for things like Commit messages?
>>
>> ### Security against malicious members?
>>
>> There seems to be no group "owner", so inviting one member X, could that
>> member
>> than delete members from the group until the group is empty?
>>
>> Apparently, there is ACLs for this, but the document doesn't describe how
>> ACLS would
>> work or how these would be implemented.
>>
>>
>>
>> ### Should B receive its own Commit back?
>>
>> Figure 4 shows:
>> ```
>>
>>                                                              Group
>>    A              B     ...      Z          Directory        Channel
>>    |              |              |              |              |
>>    |              | Update(B)    |              |              |
>>    |              +------------------------------------------->|
>>    |              |              |              | Update(B)    |
>>    |<----------------------------------------------------------+
>>    |              |<-------------------------------------------+
>>    |              |              |<----------------------------+
>> ```
>> Does the Group channel really send Update(B) back to B ?
>> Same for the following "Commit(Upd)" from A that is sent back to A?
>>
>> ### Section 6.3 use of "member"
>> ```
>>    Each member of a group presents a credential that provides one or
>>    more identities for the member,
>> ```
>> I am unsure if "one or more identities for the member" refers to identies
>> for a single client or was meant as "a user can have multiple clients".
>> The way it reads now is that it would be a single client that can have
>> multiple identities. But then could a user have multiple identities on
>> one client that do not match multiple identities on other clients.
>>
>> ### ENUMs limited to 1 octet?
>> ```
>>  enum {
>>        reserved(0),
>>        mls10(1),
>>        (255)
>>    } ProtocolVersion;
>> ```
>> Would it really do harm to turn this into two octets? I don't think
>> message
>> size is a real concern here. Same for ContentType and SenderType.
>> Note: thank you for skipping 0 as a valid entry!! Implementers applaud
>> you :)
>>
>> ### Epoch rollover?
>>
>> What happens if members maliciously create huge amounts of epochs and it
>> hits 2^64 ?
>>
>> ### Padding with zeros
>>
>> While I appreciate the zeros only for padding to avoid side channel
>> communications,
>> wouldn't it be too late if the if there were non-zeros in the padding?
>> Since this
>> message could be encrypted to multiple parties in the part of the tree,
>> wouldn't
>> it make sense to send a kind of alert message to everyone? That way, at
>> least two
>> people in the tree couldn't work together ? So instead of "MUST be
>> rejected as malformed"
>> do something that MUST send a message to everyone with "some content
>> warning" ?
>>
>> ### XOR guard
>>
>> Should there be text saying that if the "reuse guard" random returns four
>> zero bytes,
>> it should be run again? Maybe add this to 7.3.2 where MLSSenderData has
>> additional
>> constrains.
>>
>> ### KDF.Nh
>> ```
>>    The key and nonce provided to the AEAD are computed as the KDF of the
>>    first KDF.Nh bytes of the ciphertext generated in the previous
>>    section.
>> ```
>> I don't think I understand the security model here? What happens if the
>> previous
>> ciphertext has 00's for the first KDF.Nh bytes?
>>
>> ### Time is not on our side?
>> ```
>> measured in seconds since the Unix epoch (1970-01-01T00:00:00Z)
>> ```
>> Why use a time format that dies in 2038 already? IRC is older than the
>> time given
>> to MLS to survive :P
>>
>> ### lifetime field
>> ```
>>    it is RECOMMENDED that the
>>    client verifies that the current time is within the range of
>>    the lifetime field.
>> ```
>> Why RECOMMENDED and not MUST ?
>>
>> ### Section 8.4
>> ```
>>    path_secret[0] is sampled at random
>> ```
>> What does this mean? What is sampling? What is the sample used for ?
>>
>> Similar in Section 8.5:
>> ```
>> Set the encryption_key to the public key of a freshly sampled key pair
>> ```
>>
>> ### Section 13/14 Race conditions ?
>>
>> In Section 13.4.3.2, but also elsewhere, I am confused how race
>> conditions are
>> handled. In this section, if two clients try to each add a a new member
>> for an
>> External Commit join, one of them will fail on the epoch being wrong.
>>
>> Isn't there a similar race with removing? And any other operation that
>> changes
>> the epoch? How are all these race conditions handled?
>>
>> Section 14 does not really clarify this to me.
>>
>> ### Section 15.1
>>
>> Why not at least always pad to a length of like 1000 bytes to avoid some
>> of
>> the very obvious information leaks by length? It would cover most "single
>> line"
>> conversation sentences. Maybe a SHOULD pad to a minimum of X ?
>>
>> ### NOOPs ?
>>
>> As stated before, I miss a defined NOOP command that allows for a
>> continuous IPTFS
>> like constant stream of data that would hide better when a member is
>> conversing at all
>> or not.
>>
>> ### Section 17
>>
>> Algorithm entries use two octets (65535) but only allow 255 Private Use
>> entries.
>> I think that might be a little low. Maybe allow 0xf000 - 0xffff or
>> something ?
>>
>> ### Section 17.6
>> ```
>> This document registers the "message/mls"
>> ```
>> Why not use "mls/message" and keep everything within one namespace?
>>
>> ## COMMENTS
>>
>> ### memberlist unavailble before joining
>> ```
>>    When a client A wants to establish a group with B and C, it first
>>    initializes a group state containing only itself and downloads
>>    KeyPackages for B and C.  For each member, A generates an Add and
>>    Commit message adding that member, and broadcasts them to the group.
>> ```
>> So if A joins before B, A cannot tell B will become a group member ? eg
>> the fact that A invited B and C is not visible to C or B and might come
>> as a surprise.
>>
>> ### Prospective member or member ?
>> ```
>>    It also generates a Welcome message and sends this directly to the
>>   new member
>> ```
>> Should this be "prospective member" or "invitee" ? They haven't joined
>> yet, have they?
>>
>> ### "server" is ambiguous here
>> ```
>>    Only after A
>>    has received its Commit message back from the server
>> ```
>> What server? The Delivery Service ?
>>
>>
>> ### Can?
>> ```
>>    The set of members
>>    involved in the group can change from one epoch to the next
>> ```
>>
>> Isn't it the definition of epoch that there is a change in members?
>> Paul: i think members refreshing their keys also means a new epoch?
>>
>> ### May implement correctly :-)
>>
>> ```
>>    An implementation MAY use
>>    any tree representation and associated algorithms, as long as they
>>    produce correct protocol messages.
>> ```
>> This MAY is super weird. Is it allowed to implement in such a way that
>> they to NOT produce correct protocol messages? :-)
>>
>> Remove the entire sentence or lowercase the MAY?
>>
>> ### 7 or 8 members
>>
>> Figure 10 claims to have seven members, but I count 8, namely
>> Y,T,X,A,B,E,F and G  (not counting W, the root)
>>
>> ### Setion 8.7 MAY
>> ```
>>    An implementation MAY use any
>>    algorithm that produces the correct tree in its internal
>>    representation.
>> ```
>> Another weird MAY here. Again, I would just lowercase it as this is just
>> a pointer to a human decision, not a 2119 based protocol enforcement.
>>
>> ### Section 9.5 MLS Exporter
>> I assume the export is always available to ALL members of the group ?
>> For example for use as PSK to authenticated a TLS video stream?
>>
>> ### Section 10.1 encryption size
>>
>> ```
>> Each key/nonce pair MUST NOT be used to encrypt more than one message.
>> ```
>> Isn't there technically also limits to how much one AEAD key and one
>> nonce can be used
>> to encrypt a (very very) large message? I guess this could not happen,
>> but is there some
>> formal message size limit in MLS ? (looking at the vectors at the start,
>> limits could be
>> higher than 2^64 ?)
>>
>> ### Section 10.2
>> ```
>>    Members MAY keep
>>    unconsumed values around for some reasonable amount of time to handle
>>    out-of-order message delivery.
>> ```
>> Why is this MAY and not SHOULD?
>>
>> ### Section 12
>>
>> What happens if on the same Delivery Service, two groups with the same
>> group id are created?
>> Why doesn't the protocol split the group id, so one part (prefix) is
>> always created by the DS ?
>>
>> ### Section 13.2
>> ```
>>    For a regular, i.e. not external, commit the list is invalid if any
>>    of the following occurs:
>>
>>    [...]
>> ```
>> Why not list the requirements instead of what makes it invalid ?
>>
>> ### Section 13.4.3.2
>>
>> That is quite some information that must be publicly available to allow
>> external Add requests.
>> This leaks information, eg same epoch means the group membership did not
>> change, etc etc. I
>> hope this will appear in the Security Considerations later :)
>>
>> ### Section 15.3
>> ```
>> This is in addition to ensuring that these nonce and key pairs are
>> promptly
>>    deleted when the epoch ends.
>> ```
>> This sentence with a hidden MUST is stuck between a sentence with MAY and
>> a sentence
>> with SHOULD. Perhaps make the MUST explicit ?
>>
>> ### Section 16
>> ```
>> though a complete security analysis is outside of the scope of this
>> document.
>> ```
>> Hmmm :P
>>
>> ### Section 16.2
>> ```
>>    The second form of authentication is that group members can verify a
>>    message originated from a particular member of the group.  This is
>>    guaranteed by a digital signature on each message from the sender's
>>    signature key.
>> ```
>> Maybe add a sentence saying this means there is no repudiation of
>> messages?
>>
>> ### Section 17.1
>> ```
>>    Additionally clients that run predominantly on mobile
>>    processors can choose ChaCha20Poly1305 over AES-GCM for performance
>>    reasons.
>> ```
>> Citation needed? I think most phones have AES(_GCM) in hardware too ?
>> Also compared to HPKE operations and the times for epoch/group reinit,
>> does this performance really matter? (eg waking up from sleep to increment
>> the epoch would make the symmetric encryption cost pretty irrelevant. I
>> would remove this sentence.
>>
>> ### Section 17.2
>> What is the logic to make MLS_128_DHKEMX25519_AES128GCM_SHA256_Ed25519 the
>> only MTI? It contains both FIPS and not FIPS items, pleasing no one.
>>
>> ### NITS
>>
>> I personally don't like the _underscored_ words in the html rendering of
>> the document
>>
>> It "blank" the new American spelling of "blanc" ?
>>
>> ```
>> has a corresponding _hash_ that summarizes the contents
>> ```
>>
>> I find "summarizes" a strange word here. It is not the hash contains a
>> summary one can read.
>>
>> [SECG] has no IETF equivalent? Should it?
>>
>> proposaland -> proposal and
>>
>> ```
>>            Figure 13: Cleaning up after removing the third member
>> ```
>> I would say "after removing member C"  (avoid ambiguity about "every
>> third")
>>
>> ```
>>    This section defines _tree
>>    hashes_, and _parent hashes_ are defined in Section 8.9.
>> ```
>> I read this wrong the first way, the "," vanishes due to the _unscores_.
>> Maybe:
>> This section defines _tree hashes_, the next section defines _parent
>> hashes_.
>> Or alternatively, leave onlthe the first sentence in 8.8, then put the
>> remainder and
>> next section in two subsections (so 8.8.1 and 8.8.2)
>>
>> ```
>> For example, a malicious group member could send
>> ```
>> Maybe: For example, a malicious group member could otherwise send
>>
>> ```
>> to previously compromised public keys.
>> ```
>> Maybe: to public keys whose private key was previously compromised.
>>
>> ```
>> Post-compromise security is also provided for new groups
>> ```
>>
>> This reads a little odd as "new groups" would have no history and thus no
>> previous
>> compromise.
>>
>> _______________________________________________
>> MLS mailing list
>> MLS@ietf.org
>> https://www.ietf.org/mailman/listinfo/mls
>>
>