Unambiguous signatures

[TWal]

Signatures as specified in the MLS protocol document are ambiguous, in the sense that signatures produced for one purpose in the protocol can be (maliciously) used in a different place of the protocol.
Indeed, we found that for example, a KeyPackage can have the same serialization as MLSPlaintextTBS, which proves that one signature could be accepted for different structures.

The collisions we have found so far are quite artificial, so it is not super clear how to exploit this, but at least it makes it impossible to have provable security.

The solution is pretty simple, we simply prefix each signed value with a label, in a fashion similar to ExpandWithLabel.
That way, when calling SignWithLabel you can give the structure name in the Label argument.

[kkohbrok]

Great catch and a neat solution!

[TWal]

Something that is still bothering me is the MLSPlaintextTBS structure:

struct {
    select (MLSPlaintextTBS.sender.sender_type) {
        case member:
            GroupContext context;

        case preconfigured:
        case new_member:
            struct{};
    }

    WireFormat wire_format;
    opaque group_id<0..255>;
    uint64 epoch;
    Sender sender;
    opaque authenticated_data<0..2^32-1>;

    ContentType content_type;
    select (MLSPlaintextTBS.content_type) {
        case application:
          opaque application_data<0..2^32-1>;

        case proposal:
          Proposal proposal;

        case commit:
          Commit commit;
    }
} MLSPlaintextTBS;

Since the first select is done on a field defined after it, this structure is not parsable. We could say that's not a problem since we only need to serialize it, but for security purposes I would recommend to put it at the end, like this:

struct {
    WireFormat wire_format;
    opaque group_id<0..255>;
    uint64 epoch;
    Sender sender;
    opaque authenticated_data<0..2^32-1>;

    ContentType content_type;
    select (MLSPlaintextTBS.content_type) {
        case application:
          opaque application_data<0..2^32-1>;

        case proposal:
          Proposal proposal;

        case commit:
          Commit commit;
    }

    select (MLSPlaintextTBS.sender.sender_type) {
        case member:
            GroupContext context;

        case preconfigured:
        case new_member:
            struct{};
    }
} MLSPlaintextTBS;

What do you think about it?

[raphaelrobert]

Nice one! Crafting collisions is probably not straightforward, but eliminating the the risk altogether is the way to go.

[TWal]

It is actually quite easy to craft collisions, I made a script do find them: https://github.com/TWal/TLSCollisionFinder
(I wanted to check whether there was accidentally a way do disambiguate the structures)

[bifurcation]

I don't love the aesthetics here. Maybe we could solve this a little more elegantly by having a context input to SignWithLabel/VerifyWithLabel?

Alternatively: Do we even need this field any more? This field was added before membership_tag, and now the membership_tag already (a) demonstrates knowledge of the GroupContext and (b) prevents a non-member from synthesizing a valid MLSPlaintext.

[TWal]

I not sure the alternative is a good idea for several reasons, for example the membership_tag is present only in MLSPlaintext and not in MLSCiphertext, also I'm not confident in removing things from the signature without a solid proof that it is not necessary (for example, signing the tree hash seems like a reasonable thing to do?).

I feel like adding a context input is a bit weird. What do you think about this third option: adding an option<GroupContext> group_context; and say something like "group_context MUST contain a value if sender_type = ..." below? This pattern already exists for confirmation_tag for example.

[bifurcation]

I would prefer that we put these descriptions with the structs they belong to, as opposed to down here in the instructions. We already have prose that says what the signature covers; that should just specify the label as well. You could also include it as a comment in the syntax specification:

struct {
    ...
    // SignWithLabel(., "MLSPlaintextTBS", MLSPlaintextTBS)
    opaque signature<0..2^16-1>;
} KeyPackage;

struct {
    ...
    // SignWithLabel(., "GroupInfoTBS", GroupInfoTBS)
    opaque signature<0..2^16-1>;
} GroupInfo;

struct {
    ...
    // SignWithLabel(., "PublicGroupStateTBS", PublicGroupStateTBS)
    opaque signature<0..2^16-1>;
} PublicGroupState;

struct {
    ...
    // SignWithLabel(., "MLSPlaintextTBS", MLSPlaintextTBS)
    opaque signature<0..2^16-1>;

    // MAC(confirmation_key, GroupContext.confirmed_transcript_hash)
    optional<MAC> confirmation_tag;

    // MAC(membership_key, MLSPlaintextTBM);
    optional<MAC> membership_tag;
} MLSPlaintext;

(Note: I don't think GroupInfoTBS exists, but it probably should.)

[kkohbrok]

(#529 adds a GroupInfoTBS)

[TWal]

If every structure has a TBS associated with it, this seems like a nice formatting of how you must compute the signatures, however for KeyPackage there is no TBS (yet), it is said in the text above that the TBS contains "all of the fields except for the signature field".

Should we add a TBS for every structure that we sign? This is for example done for PublicGroupState where a TBS is explicitly described even if it contains all fields except the signature field in PublicGroupState.