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

[Richard Barnes <[e-mail]>]

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 <[e-mail]> wrote:

> 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 <[e-mail]>
> 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.
>>
>
Removed the normative reference to 1.2, moved the note forward.



> 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.
>>
>
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.



> 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?
>>
>
Changed to 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.
>>
>
Changed to "appended to".



>
>> 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.
>>
>
Fixed.



> 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?
>>
>
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



> 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".
>>
>
Also no noun after the article!



>       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"?
>>
>
Yes, "can cause".



>
>>
>> _______________________________________________
>> Perc mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/perc
>>
>