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

[Richard Barnes <[e-mail]>]

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=
[e-mail]> 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
> [e-mail]
> https://www.ietf.org/mailman/listinfo/mls
>