Re: [MLS] Two poorly defined aspects of the spec

Richard Barnes <rlb@ipv.sx> Tue, 18 August 2020 14:40 UTC

Return-Path: <rlb@ipv.sx>
X-Original-To: mls@ietfa.amsl.com
Delivered-To: mls@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 43BD53A0BE3 for <mls@ietfa.amsl.com>; Tue, 18 Aug 2020 07:40:35 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.896
X-Spam-Level:
X-Spam-Status: No, score=-1.896 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_NONE=0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=ipv-sx.20150623.gappssmtp.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Cybfcq7KzLvi for <mls@ietfa.amsl.com>; Tue, 18 Aug 2020 07:40:32 -0700 (PDT)
Received: from mail-qv1-xf35.google.com (mail-qv1-xf35.google.com [IPv6:2607:f8b0:4864:20::f35]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 5417D3A0BD7 for <mls@ietf.org>; Tue, 18 Aug 2020 07:40:32 -0700 (PDT)
Received: by mail-qv1-xf35.google.com with SMTP id x7so9628058qvi.5 for <mls@ietf.org>; Tue, 18 Aug 2020 07:40:32 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipv-sx.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=OzE7jdt9nLbh+k95/ppyhu011K3/Q4vD81CpBCWZhFo=; b=cPhZ3hudr8+P0pbMf9huFTMHm7ojmR2hp0h/v/qpn6pvBEejFPrQu0RzgIj97WcB8G ceDMBWiJkfYZW8t+nhilFQpi0j9JgR85dYYDrf5nnYCq4m9lXgIS9cBgcfJhjL+Nt1jm P7x3+qrgcWUUXRZxDSEmSQAevNi1kn4hQGkfQdkRs9FpTC1ytdseUp2MWsJmt1qa7DZ0 0EG0PT+sE5CD8JJQIiiqivhHmfZn7wGIJeNGCUkXjc/WYzacwr21dZJT7UvsBVwMN7UN TqntbO4eNyOShCxNClTv3kmJsHIpoeAexH1UMeLnfzrgeEnuXoUL4gLsXxaG4dFTbiTs pD5Q==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=OzE7jdt9nLbh+k95/ppyhu011K3/Q4vD81CpBCWZhFo=; b=ic8vZUf6YJQtIuyKCiXvuLXws33MSTTO8plBvkRaO06IAuGDd9iB41CklWHMXefBR4 GMJeU5c9zdE/rvLChvxzVO38mry2L27TMeidNe832Inm1PAItF733JZRGQJJMKO5hNnW jvbPOYfxLwXhnmD/f9kBbVcKpvchATF82qtEXmyrX+iHXFdhRYgJ31BCjzVkNN2mxb+B H0RKCi18VmSgCZ8nJdDZvRr5tE7Wn93FaGCnYEWvDfCzHmAvwsI+52maMSYdj8cgJGJd mS6S9xrl9fSGthwkzA9+LPXwk/5MlZzmsw0q8fxFFyjXEyAnC/+KvCoxg9elShSxsU/Q OAMA==
X-Gm-Message-State: AOAM532a6aeM41nmCGd9ahMfzZH0Xga9QWqfOyyrPJ6efF3kYhDTsMoE rpVBGSyr+HCi3BNKblhMkRETJHZcalDnToqnjzaT8A==
X-Google-Smtp-Source: ABdhPJwNDZYjMx97gWhuWP9HNzbtITvE3MDDel5bA1rJE8YujqN1AzKF5WWu+1gspWcEXr2pHBNtMoDUqPMQNQxOoac=
X-Received: by 2002:ad4:44f2:: with SMTP id p18mr19222259qvt.137.1597761631028; Tue, 18 Aug 2020 07:40:31 -0700 (PDT)
MIME-Version: 1.0
References: <af4a72d6f3ae4c809367222386340e6c@aalto.fi> <CAL02cgRX6m_Ygo1wwWL_Sd3ygYKGZLs=TwsDVksy3V7zUqZEqg@mail.gmail.com> <cc19f450a410431cb3be124c96417c60@aalto.fi> <CAL02cgS77qKv5zT38P4BsAvsAi_vyPDs9oEWxYuYJikAyopNHQ@mail.gmail.com> <597d43a04e4947fb80a8fa6830f41a0d@aalto.fi>
In-Reply-To: <597d43a04e4947fb80a8fa6830f41a0d@aalto.fi>
From: Richard Barnes <rlb@ipv.sx>
Date: Tue, 18 Aug 2020 10:40:10 -0400
Message-ID: <CAL02cgRVQQk7+L5obL_Dcc1eWx--mke7w__M3WktPDbQ1pqhMA@mail.gmail.com>
To: Cornelissen Eric <eric.cornelissen@aalto.fi>
Cc: "mls@ietf.org" <mls@ietf.org>
Content-Type: multipart/alternative; boundary="000000000000741e3905ad27de03"
Archived-At: <https://mailarchive.ietf.org/arch/msg/mls/CbBEhH5A7OyoP6Mf5FFXgSU0r30>
Subject: Re: [MLS] Two poorly defined aspects of the spec
X-BeenThere: mls@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Messaging Layer Security <mls.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/mls>, <mailto:mls-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/mls/>
List-Post: <mailto:mls@ietf.org>
List-Help: <mailto:mls-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/mls>, <mailto:mls-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 18 Aug 2020 14:40:35 -0000

Hi Eric,

I agree with (2).  I think the stuff that's in the spec right now about
verifying group creation is mainly just a legacy of the fact that we used
to have separate Init and Add operations.  When those got coalesced into
Welcome, it must have seemed appealing to enable recipients to continue to
distinguish the cases.  But as you point out, that's not really tenable.

So please feel free to send a PR!

Thanks,
--Richard

On Tue, Aug 18, 2020 at 10:27 AM Cornelissen Eric <eric.cornelissen@aalto.fi>
wrote:

> Thanks Richard
>
> Regarding my observations about Welcome. I would need some context about
> whether or not we want joiners to verify group creation. (*)
>
>   1. We want to verify group creation.
>
>
>   In this case, first of all, the changes in
> <https://github.com/mlswg/mls-protocol/pull/385>#385
> <https://github.com/mlswg/mls-protocol/pull/385>
> <https://github.com/mlswg/mls-protocol/pull/385> need to be reverted (as
> pointed
>   out in that thread and in #390
> <https://github.com/mlswg/mls-protocol/issues/390>
> <https://github.com/mlswg/mls-protocol/issues/390>). Then, to prevent the
> attack for which that change
>   was introduce I would propose the following: disallow the use of
> "add-only" commits
>   on group creation AND requiring the initial group member(s) to verify
> that a "full"
>   commit was used in the group creation.
>
>   2. We don't need to verify group creation.
>
>   In this case the only thing (I think) that has to change is that the
> last paragraph of
>   the should be remove. (and #390
> <https://github.com/mlswg/mls-protocol/issues/390> can be closed.)
>
>
> As I stated earlier, I would personally be in favor of option 2 as 1) it
> is the simpler solution, 2) the existing method of detecting group creation
> is not foolproof (**), and 3) I don't see a reason to verify the group
> creation process. In fact, a malicious client can always prevent joiners
> from being able to do this verification by simply performing a separate
> commit before adding the actual group members (***).
>
>
> If you agree that 2 is the way to go, I'll get out a PR as soon as
> possible.
>
> Regards,
> Eric
>
>
> (*): The git history isn't of much help here. As far as I can tell the
> group creation verification paragraph was added in 5490d0e
> <https://github.com/mlswg/mls-protocol/commit/5490d0ebfa1b41fff0ee54966ef375c37dadab68>
> /2f2f5d8
> <https://github.com/mlswg/mls-protocol/commit/2f2f5d813037f7cf3c3d0d78612bc506f31eef5e>
> /#239 <https://github.com/mlswg/mls-protocol/pull/239>. None of which
> provide any insight into why the verification is useful. Perhaps this was
> discussed in a meeting somewhere?
> (**): Nor do I think there exists any effective way of detecting group
> creation.
> (***): In which case reverting #385
> <https://github.com/mlswg/mls-protocol/pull/385>
> <https://github.com/mlswg/mls-protocol/pull/385> would only hurt the
> malicious client.
>
> ------------------------------
> *From:* Richard Barnes <rlb@ipv.sx>
> *Sent:* Tuesday, August 11, 2020 6:22:32 PM
> *To:* Cornelissen Eric
> *Cc:* mls@ietf.org
> *Subject:* Re: [MLS] Two poorly defined aspects of the spec
>
> I filed #388 to fix (1) and (1a).
>
> https://github.com/mlswg/mls-protocol/pull/388
>
> Not sure what to do with your observations about Welcome.  If you have
> thoughts about how to clarify, please send a PR!
>
> --Richard
>
>
> On Tue, Aug 11, 2020 at 9:47 AM Cornelissen Eric <
> eric.cornelissen@aalto.fi> wrote:
>
>> Hi Richard,
>>
>>
>> 1. In that case I would agree that clarifying the indices is the way to
>> go. I personally think just adding *interim_[0] = 0* to the current
>> definition would suffice and be clearer than your suggestion here, but
>> that's just personal taste.
>>
>>
>> 2. The "secrets" array makes a ton of sense.
>>
>>
>> If the welcome is ignored when the verification fails, then the fact that
>> this is not strictly true seems to make it impossible to continue with a
>> group after all but one person left (unfortunately, the paragraph
>> doesn't specify what should happen if the verification fails). This is
>> not a cryptographic problem, but I'd imagine this being confusing for end
>> users *if* the application layer doesn't deal with it properly.
>>
>>
>> However, as I just pointed out in #385
>> <https://github.com/mlswg/mls-protocol/pull/385>, it seems that we (I)
>> made it impossible for joiners to verify group creation at all.
>>
>>
>>
>> Regards,
>>
>> Eric
>>
>> ------------------------------
>> *From:* Richard Barnes <rlb@ipv.sx>
>> *Sent:* Monday, August 10, 2020 5:22:47 PM
>> *To:* Cornelissen Eric
>> *Cc:* mls@ietf.org
>> *Subject:* Re: [MLS] Two poorly defined aspects of the spec
>>
>> Hi Eric,
>>
>> Thanks for the analysis here.  More good finds.
>>
>> 1. I think the right answer is a little of both.  If you initialize a
>> one-member group, before any Commit messages have been sent, then it should
>> be the zero-length octet string.  But then as soon as the group is used for
>> something, it is updated.  Maybe the way to clarify this is to fix the
>> indices.  If we have the interim hash belong to the new epoch, then we
>> could have something like the following:
>>
>> interim_[0] = 0
>> confirmed_[n] = Hash(interim_[n] || MLSPlaintextCommitContent_[n]);
>> interim_[n+1] = Hash(confirmed_[n] || MLSPlaintextCommitAuthData_[n]);
>>
>>
>> 1.a. BTW, this raises another issue: I had thought we had removed direct
>> dependencies on hash functions when we move from invoking HKDF to invoking
>> the KDF from HPKE.  But this points out that we still have raw hash
>> invocations.  I filed an issue for this:
>>
>> https://github.com/mlswg/mls-protocol/issues/386
>>
>> FWIW, I think the latter course there is the right answer, i.e., to keep
>> the raw hash invocations and clarify that the ciphersuite specifies a hash
>> function.
>>
>>
>> 2. It seems like the right reference here is probably to the "secrets"
>> array in the Welcome message.  There's one entry there for each member
>> being added, so if len(secrets) = len(group_info.tree.leaves) - 1, then
>> there was only one member in the group to start with.  It's not strictly
>> true to say that this is a new creation, since one commit could remove all
>> but one of the members of the group and re-add a new set of members.  But
>> this distinction doesn't really seem salient.
>>
>>
>>
>> On Thu, Aug 6, 2020 at 6:02 AM Cornelissen Eric <
>> eric.cornelissen@aalto.fi> wrote:
>>
>>> Hi All,
>>>
>>>
>>> I have two questions about things that are unclear to me in the current
>>> state of the spec as they are undefined/poorly defined.
>>>
>>>
>>>
>>> 1. What is the intended value of *interim_transcript_hash_**[0]*?
>>>
>>> From the current draft I cannot definitively (i.e. with 100% certainty)
>>> say whether the value of the first *interim_transcript_hash *should be:
>>>
>>> a. "the zero-length octet string" following the last line of Group
>>> state section
>>> <https://github.com/mlswg/mls-protocol/blob/39455d2ea5e8fb42e8f0f0624bddd8c56675da0e/draft-ietf-mls-protocol.md#group-state>
>>> <https://github.com/mlswg/mls-protocol/blob/master/draft-ietf-mls-protocol.md#group-state>,
>>> or
>>> b. a value derived based on the MLSPlaintextCommitAutData for the first
>>> commit concatenated with 0:
>>>     interim_transcript_hash_[0]
>>>         = Hash(confirmed_transcript_hash_[0] ||
>>> MLSPlaintextCommitAuthData_[0])
>>>         = Hash(0 || MLSPlaintextCommitAuthData_[0])
>>>
>>> The draft strongly suggests to me that it is the former, but the latter
>>> seems to more accurately reflect the statement "The
>>> *confirmed_transcript_hash* field contains a running hash over the
>>> messages that led to this state."
>>>
>>> In the case that *a* is correct I would suggest updating the definition
>>> of *interim_transcript_hash_**[n]* to:
>>>
>>>     interim_transcript_hash_[0] = 0
>>>     interim_transcript_hash_[n] =
>>>         Hash(confirmed_transcript_hash_[n] ||
>>>             MLSPlaintextCommitAuthData_[n]);
>>>
>>> In the case that *b* is correct I would suggest adding a "warning" that
>>> says something along the lines of "*as a placeholder value" *to the
>>> place(s) where the draft says that the *interim_transcript_hash* of a
>>> group is initialized to 0.
>>>
>>>
>>> 2. What is the `members` array mentioned in the Group creation section
>>> <https://github.com/mlswg/mls-protocol/blob/39455d2ea5e8fb42e8f0f0624bddd8c56675da0e/draft-ietf-mls-protocol.md#group-creation>
>>> ?
>>>
>>> In that section it is stated that "A new member receiving a Welcome
>>> message can recognize group creation if the number of entries in the
>>> `members` array is equal to the number of leaves in the tree minus one."
>>> First of all, it is not clear what this `members` array is. It seems this
>>> refers to a field of the now-removed Init struct. Interestingly this struct
>>> was removed in the very same PR that added the above sentence (see #239
>>> <https://github.com/mlswg/mls-protocol/pull/239>). But I wasn't able to
>>> really figure out what the draft-09 equivalent of it should be.
>>>
>>> In addition, I'm pretty sure this technique would no longer work to
>>> detect group creation (but that would depend on how the sentence should be
>>> updated).
>>>
>>>
>>> Regards,
>>> Eric
>>> _______________________________________________
>>> MLS mailing list
>>> MLS@ietf.org
>>> https://www.ietf.org/mailman/listinfo/mls
>>>
>>