WIP: Introduce a common framing layer #120

bifurcation · 2019-02-17T16:44:23Z

This PR introduces a common framing layer that is used to encapsulate both Handshake and Application messages, encrypted or unencrypted. The most significant impact is that in order to make signing logic common to both of these cases, the signature now covers only the message in question, as opposed to also covering the transcript.

Fixes #101

beurdouche

How to handle the application secret generation in this common framework is interesting, we can either always include it or having it only in the case of Application messages which will degrade the symmetry.

beurdouche · 2019-02-27T09:04:45Z

draft-ietf-mls-protocol.md

+    opaque content[MLSPlaintext.length];
+    uint8 signature[MLSInnerPlaintext.sig_len];
+    uint16 sig_len;
+    ContentType type;


The signature must cover everything including the content type

beurdouche · 2019-02-27T09:06:23Z

draft-ietf-mls-protocol.md

+
+struct {
+    opaque content[MLSPlaintext.length];
+    uint8 signature[MLSInnerPlaintext.sig_len];


sig_len is undefined.

beurdouche · 2019-02-27T09:07:30Z

draft-ietf-mls-protocol.md

+struct {
+    opaque content[MLSPlaintext.length];
+    uint8 signature[MLSInnerPlaintext.sig_len];
+    uint16 sig_len;


If we are gonna do this, I would prefer opaque signature<0..2^16-1>
otherwise if the ciphersuite* gives the length of the signature we don't need this.

beurdouche · 2019-02-27T09:09:06Z

draft-ietf-mls-protocol.md

+        case application: Application;
+    }
+
+    opaque signature<1..2^16-1>;


I am confused now, why is there a second signature here ?
In both cases there is a signature that covers the content of group_id, epoch, sender and content type no ?

On the contrary, the inner one is the one we use, you don't want to sign a large padding.
What is important is that the inner one must cover those fields as done the message protection section.

Ok, then we're assuming that group_id, sender and epoch are still part of the Handshake struct? I was wondering if we could somehow avoid redundancy here.

beurdouche · 2019-02-27T09:12:35Z

draft-ietf-mls-protocol.md

+    opaque group_id<0..255>;
+    uint32 epoch;
+    uint32 sender;
+    uint32 generation;


While this is weakly-authenticated via the AEAD it should be in the MLSPlaintext header as well and covered by the signature (especially in the case of an Application message) to avoid forgery from an adversarial member who is not the sender.

It is also worth noting that this field even if not technically needed in the case of a Handshake message might be able to use this to advertise a counter for application messages previously sent by the sender, hence allowing recipients to know if they received all app messages from the sender. We could limit that to the previous epoch or not... Maybe there is something there or not...

What is the generation field for exactly?

beurdouche · 2019-02-27T09:13:30Z

draft-ietf-mls-protocol.md

-# Message Protection
+# Application Messages
+
+[[ XXX(RLB): It should be possible to simplify this section in light


I can do that when this PR is merged.

raphaelrobert · 2019-02-27T09:34:02Z

As a general rule, I think we should not prefix structs with "MLS" as it adds no semantic value

kkohbrok · 2019-02-27T09:42:07Z

draft-ietf-mls-protocol.md

+    handshake(1),
+    application(2),
+    (255)
+} ContentType;


Why would content ever be labelled invalid?

The idea is that the ContentType is used to mark where the padding bytes (all zero) end, so it needs to be non-zero.

kkohbrok · 2019-02-27T09:59:17Z

draft-ietf-mls-protocol.md

+    }
+
+    opaque signature<1..2^16-1>;
+} MLSPlaintext;


Do we need another epoch field, as well as another group_id field? Both should be implicitly verified upon successful decryption with the group secret from the epoch contained in MLSCiphertext, right?

We do not need to actually copy them twice indeed, only the outer one in Ciphertext is needed, but they need to be covered by both the AEAD and the internal signature.

kkohbrok · 2019-02-27T10:02:36Z

draft-ietf-mls-protocol.md

+    uint32 sender;
+    uint32 generation;
+    opaque cipertext<0..2^32-1>;
+} MLSCiphertext;


What do we need the sender field for in MLSCiphertext? We might even get some sort of identity hiding if we leave it out :-)

I was misunderstanding Richard's idea at first. I believe the idea is to encrypt the Plaintext with different keys depending if it is Handshake or Application. In that scenario, you need the sender and the generation in the clear to know which key to pick in the case of an Application message. Note that @raphaelrobert and I are thinking on suggesting to encrypt these under the the group key to hide them if necessary, we'll discuss this at some point. Note that if this is the intent here, the ContentType must also be outside to know if this is an Application message or an Handshake message.

Why do you need the sender, though? Both handshake and application keys derived from the groupkey should be sender-independent.

Which key do you pick when receiving an application message ? :)

I'm confused. The key for handshake encryption is derived from the group_key, right? So why do we need to know the sender? We only need the group_id and the epoch to derive the key if I understand correctly.

beurdouche · 2019-02-27T11:15:13Z

draft-ietf-mls-protocol.md

+    opaque group_id<0..255>;
+    uint32 epoch;
+    uint32 sender;
+    ContentType type;


My initial interpretation was incorrect but, if I am correct now, above this should be removed.

bifurcation · 2019-02-27T23:34:01Z

I added some prose in the latest round of commits that should clarify what's going on here. I think that should address most of the comments.

@raphaelrobert - regarding the "MLS" prefix, I'm inclined to keep it, because (1) it mirrors TLS, and (2) just having Plaintext and Ciphertext seems odd to me.

kkohbrok · 2019-02-28T06:43:49Z

draft-ietf-mls-protocol.md

+                  | type          |
+                                  |
+                  * zero_padding  |
+~~~~~


Do I understand correctly that the fields that represent information that is shared between the different structs are not actually part of the encoded data structure but rather are just meant to be pointers to the respective "outer" struct? If that is the case, I think we should stay true to the principle of having the structs represent what is actually part of the data structure that is encoded and not what is composed locally to compute signatures. Please do correct me if I'm wrong in my understanding of the spec here.

bifurcation · 2019-04-22T21:24:42Z

Closing in favor of #131

Introduce a common framing layer

72c6fda

beurdouche requested changes Feb 27, 2019

View reviewed changes

kkohbrok reviewed Feb 27, 2019

View reviewed changes

beurdouche reviewed Feb 27, 2019

View reviewed changes

beurdouche mentioned this pull request Feb 27, 2019

Legacy - Common Framing serving as a base for PR #153 and #155 #131

Closed

bifurcation added 6 commits February 27, 2019 16:10

Elaborate the framing more clearly

f1729eb

Move sections around

9994e00

Move application key schedule forward

f3eee96

Merge branch 'master' into common-framing

5a8ec7d

Update changelog

e3fb7dd

Incorporate updates to key schedule

304fd87

kkohbrok reviewed Feb 28, 2019

View reviewed changes

bifurcation mentioned this pull request Feb 28, 2019

Use common framing #101

Closed

bifurcation closed this Apr 22, 2019

bifurcation deleted the common-framing branch November 14, 2019 19:10

beurdouche added enhancement security labels Dec 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Introduce a common framing layer #120

WIP: Introduce a common framing layer #120

bifurcation commented Feb 17, 2019

beurdouche left a comment •

edited

beurdouche Feb 27, 2019

beurdouche Feb 27, 2019

beurdouche Feb 27, 2019 •

edited

beurdouche Feb 27, 2019

beurdouche Feb 27, 2019

kkohbrok Feb 27, 2019

beurdouche Feb 27, 2019

beurdouche Feb 27, 2019

kkohbrok Feb 27, 2019

beurdouche Feb 27, 2019

raphaelrobert commented Feb 27, 2019

kkohbrok Feb 27, 2019 •

edited

bifurcation Feb 27, 2019

kkohbrok Feb 27, 2019

beurdouche Feb 27, 2019

kkohbrok Feb 27, 2019

beurdouche Feb 27, 2019 •

edited

kkohbrok Feb 27, 2019

beurdouche Feb 27, 2019

kkohbrok Feb 27, 2019

beurdouche Feb 27, 2019 •

edited

bifurcation commented Feb 27, 2019

kkohbrok Feb 28, 2019

bifurcation commented Apr 22, 2019

WIP: Introduce a common framing layer #120

WIP: Introduce a common framing layer #120

Conversation

bifurcation commented Feb 17, 2019

beurdouche left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beurdouche Feb 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphaelrobert commented Feb 27, 2019

kkohbrok Feb 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beurdouche Feb 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beurdouche Feb 27, 2019 • edited

Choose a reason for hiding this comment

bifurcation commented Feb 27, 2019

Choose a reason for hiding this comment

bifurcation commented Apr 22, 2019

beurdouche left a comment •

edited

beurdouche Feb 27, 2019 •

edited

kkohbrok Feb 27, 2019 •

edited

beurdouche Feb 27, 2019 •

edited

beurdouche Feb 27, 2019 •

edited