Subject: Re: [Perc] Alexey Melnikov's Discuss on draft-ietf-perc-srtp-ekt-diet-09: (with DISCUSS and COMMENT)

From: Richard Barnes <[email-masked]>
Date: Wed, 1 May 2019 15:11:53 -0400

Hey Alexey,

Good catch.  I agree that those bits are malformed.  Unless folks here
object, I will change those portions to say that the receiver MUST discard
the packet if it receives an unknown message type.  TBH, I don't really see
how we can do otherwise, unless we include a length field that is required
for all message types (or maybe all message types except type 0).

--RLB

On Wed, Feb 20, 2019 at 8:25 AM Alexey Melnikov <[email-masked]>
wrote:

> Alexey Melnikov has entered the following ballot position for
> draft-ietf-perc-srtp-ekt-diet-09: Discuss
>
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
>
>
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-perc-srtp-ekt-diet/
>
>
>
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
>
> The document is generally quite readable, which is great. But I have a few
> small issues I would like to get clarification on before recommending
> approval
> of this document:
>
> In 4.1:
>
>    Message Type: The last byte is used to indicate the type of the
>    EKTField.  This MUST be 2 for the FullEKTField format and 0 in
>    ShortEKTField format.  Values less than 64 are mandatory to
>    understand while other values are optional to understand.
>
> I thought I knew what this meant when I read it, and then I saw this:
>
>    A receiver
>    SHOULD discard the whole EKTField if it contains any message type
>    value that is less than 64 and that is not understood.
>
> "SHOULD discard ... EKTField" makes this field NOT mandatory. (If you said
> "SHOULD discard the whole packet", that would have been different.) Also,
> how
> "discard" different from the following sentence suggesting "ignore"? I
> think
> you have some inconsistencies/terminology problem here!
>
>    Message type
>    values that are 64 or greater but not implemented or understood can
>    simply be ignored.
>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> I share Benjamin's concern about extensibility.
>
> General:
>
> Mixture of normative TLS 1.2 and TLS 1.3 references is confusing in the
> document. There is a note later on explaining that either can be used, but
> it
> would be better for readers to see this note earlier in the document.
>
> In 4.1:
>
>   EKTMsgLength = 2BYTE;
>
> and similarly:
>
>    SPI = 2BYTE
>
> The document doesn't seem to say whether or not this is in network byte
> order.
>
> In 4.2.1:
>
>    2.  The EKTPlaintext field is computed from the SRTP Master Key,
>        SSRC, and ROC fields, as shown in Section 4.1.  The ROC, SRTP
>        Master Key, and SSRC used in EKT processing SHOULD be the same as
>        the one used in the SRTP processing.
>
> When can they be different? I.e. why is this not a MUST?
>
>    The computed value of the FullEKTField is written into the SRTP
>    packet.
>
> I think this might be misleading. Do you just mean appended to the end of
> the
> SRTP packet after encrypted data? If yes, please say so to avoid confusion
> with
> writing it into encrypted data before encryption.
>
> In 4.3:
>
>    When receiving a new EKTKey, implementations need to use the ekt_ttl
>    field (see Section 5.2.2) to create a time after which this key
>    cannot be used and they also need to create a counter that keeps
>    track of how many times the key has been used to encrypt data to
>    ensure it does not exceed the T value for that cipher (see ).
>
> Missing reference after "see" here.
>
> In 4.4.1:
>
>    The default EKT Cipher is the Advanced Encryption Standard (AES) Key
>    Wrap with Padding [RFC5649] algorithm.  It requires a plaintext
>    length M that is at least one octet, and it returns a ciphertext with
>    a length of N = M + (M mod 8) + 8 octets.
>
> I started looking at RFC 5649. Maybe I was tired and my math was wrong,
> but I
> couldn't figure out how you came up with the N value above. In particular,
> where is the "+ 8" coming from?
>
> In 4.7:
>
>    New sender:
>       A new sender SHOULD send a packet containing the FullEKTField as
>       soon as possible, always before or coincident with sending its
>       initial SRTP packet.  To accommodate packet loss, it is
>       RECOMMENDED that three consecutive packets contain the
>       FullEKTField be transmitted.  If the sender does not send a
>       FullEKTField in its initial packets and receivers have not
>       otherwise been provisioned with a decryption key, then decryption
>       will fail and SRTP packets will be dropped until the the receives
>
> Nit: duplicated "the".
>
>       a FullEKTField from the sender.
>
> In 6:
>
>    An attacker who tampers with the bits in FullEKTField can prevent the
>    intended receiver of that packet from being able to decrypt it.  This
>    is a minor denial of service vulnerability.  Similarly the attacker
>    could take an old FullEKTField from the same session and attach it to
>    the packet.  The FullEKTField would correctly decode and pass
>    integrity checks.  However, the key extracted from the FullEKTField ,
>    when used to decrypt the SRTP payload, would be wrong and the SRTP
>    integrity check would fail.  Note that the FullEKTField only changes
>    the decryption key and does not change the encryption key.  None of
>    these are considered significant attacks as any attacker that can
>    modify the packets in transit and cause the integrity check to fail.
>
> The last sentence seems to be incomplete. Did you mean "can" instead of the
> last "end"?
>
>
> _______________________________________________
> Perc mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/perc
>

---

From: Richard Barnes <[email-masked]>
Date: Wed, 1 May 2019 15:52:51 -0400

PR here:

https://github.com/ietf/perc-wg/pull/169

Some comments inline below...

On Wed, May 1, 2019 at 3:11 PM Richard Barnes <[email-masked]> wrote:

>> document. There is a note later on explaining that either can be used,
>> but it
Removed the normative reference to 1.2, moved the note forward.

>> The document doesn't seem to say whether or not this is in network byte
Clarified for the message length.  It actually doesn't matter for the SPI,
as long as an implementation is internally consistent, since this isn't an
integer, it's an opaque ID.

>> When can they be different? I.e. why is this not a MUST?
Changed to MUST.

>> SRTP packet after encrypted data? If yes, please say so to avoid
>> confusion with
Changed to "appended to".

>> Missing reference after "see" here.
Fixed.

>> where is the "+ 8" coming from?
It's the auth tag.  Basically, key wrap encrypts in 64-bit blocks and adds
a 64-bit auth tag.

https://tools.ietf.org/html/rfc5649#section-4.1

>>       will fail and SRTP packets will be dropped until the the receives
Also no noun after the article!

>> The last sentence seems to be incomplete. Did you mean "can" instead of
Yes, "can cause".