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

[Paul Wouters <[e-mail]>]

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