[MLS Spec change] Make padding arbitrary-size and all-zero (#650)

[raphaelrobert]

Link to the exact changes
mlswg/mls-protocol#650

Description of the changes

Fixes #642

Also requires that padding be all-zero. Before, the sender was allowed to put arbitrary octets in there.

[duesee]

Okay, so we have two changes here ...

struct {
    select (MLSCiphertext.content_type) {
        case application:
          opaque application_data<V>;
        case proposal:
          Proposal proposal;
        case commit:
          Commit commit;
    }

    MLSMessageAuth auth;
-   opaque padding<V>;
+   opaque padding[length_of_padding];
} MLSCiphertextContent;

... and the added requirement that the padding is all-zero ...

The padding field MUST be filled with all zero bytes. A receiver MUST verify that
there are no non-zero bytes in the padding field, and if this check fails, the
enclosing MLSCiphertext MUST be rejected as malformed.

[duesee]

First question: Where does length_of_padding come from?

Okay ...

A receiver identifies the padding field in a plaintext decoded from MLSCiphertext.ciphertext by first decoding the content and the auth field; then the padding field comprises any remaining octets of plaintext.

So there is no length field anymore but the length_of_padding is inferred as full decrypted MLSCiphertextContent - (len(content_type) + len(auth)).

Hm... not sure if this is better than before. Implicit lengths may collide with derive(TlsDeserialize).

[duesee]

I've opened a draft PR #1074. Can you have a look if this looks sane? Specifically, how does this ValSem thing work exactly? Can I just add another ValSemXXX for the padding check?

[duesee]

I'm leaning towards thinking that this was a good spec change. Making the length implicit forces us to consume all decrypted bytes. With an explicit length it could happen that someone encodes trailing data. Not sure about the consequences but doesn't seem great.