Re: [Perc] AD Evaluation of draft-ietf-perc-srtp-ekt-diet-08

[Richard Barnes <[e-mail]>]

Thanks for the review, Ben.  I've posted a PR with responses; notes inline
below...

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

Apropos of your follow-up comment, I have sent a ping to the last address I
have for Dan Wing, and will update the PR with whatever I hear back from
him.


On Thu, Aug 9, 2018 at 6:28 PM Ben Campbell <[e-mail]> wrote:

> Hi,
>
> This is my AD Evaluation of draft-ietf-perc-srtp-ekt-diet-08.
>
> The document is mostly in good shape. It’s easy to read and understand.
> However, I have a few substantive comments/questions and some editorial
> comments. I would like to at least discuss the substantive comments before
> going to IETF LC.
>
> Thanks!
>
> Ben.
>
> ----------
>
> *** Substantive Comments: ***
>
>
> §3: Is there a reason not to use the RFC 8174 boilerplate? There are at
> least a few instances of lowercase keywords in places that don’t seem
> intended as normative.
>

Not intentional.  I fixed one lower-case "must" and changed over to 8174.



> §4.2.1, first paragraph: Is there never a situation where you send neither
> version?  Also, did I miss a description of the purpose and semantics for
> ShortEKTField, other than “send it when you don’t send the full version”?
>

No, there is never such a situation.  EKT always adds at least one byte of
overhead to every packet.  Because RTP lacks an internal length indicator,
the receiver of an EKT-enabled flow needs to know how much EKT data to
strip off the end of any given packet.  Even if the answer is "zero", you
still need a way to signal that, which is what the ShortEKTField does.



> §4.2.2, step 3: "If the EKT decryption operation returns an authentication
> failure, then the packet processing stops.”
>
> .... and then what?
>

Clarified that EKT processing is definitely over, and the packet SHOULD be
discarded.



> - Step 6: "This applies in transforms such as [ I-D.ietf-perc-double] for
> replacing just half the key, but SHOULD return a processing error
> otherwise.”
>
> I don’t understand that sentence. Is the point to say that using a “too
> short” key to replace part of the old key is only valid for perc-double or
> similar transforms, and receiving one in other contexts is an error?
>

Revised to clarify and tighten.


> - Next Sentence: "Outbound packets SHOULD continue to use the old SRTP
> Master Key for 250 ms after” Does that imply a normative requirement for
> recipients to keep old keys around for at least that long? There’s
> currently an implementation suggestion about that, but it doesn’t use MUST
> or SHOULD.
> (Editorial: The sentence seems to belong in the “outbound” section, not
> the “inbound” section.)
>

Yeah, moved this to the outbound section.  Added MAY-level inbound
considerations to the implementation notes -- either the receiver can
accept packet loss or do trial decryption.



> §4.3:
>
> - “ This ciphertext value MAY be cached by an SRTP
>    receiver to minimize computational effort by noting when the SRTP
>    master key is unchanged and avoiding repeating the steps defined in ..”
>
> Are we really talking about the recipient? How would the recipient know
> that the key has not changed without processing the ciphertext value?
> (Also, the sentence is unfinished--maybe the answer would be clear if the
> rest of the sentence came out of hiding :-) )
>

The unclear ending is a missing cross-reference to Section 4.2.  I think
the receiver optimization here is that if you cache the tag, you can memcmp
instead of decrypting.  Same on the sender side, memcpy vs. encrypt.
Updated to reflect that.


> - “ The receiver may want to have a sliding window to retain old SRTP
>    Master Keys (and related context) for some brief period of time, so
>    that out of order packets can be processed as well as packets sent
>    during the time keys are changing.”
>
> See comment to §4.2.2, step 6. “may want” seems weak given that the sender
> SHOULD keep using the old key for a period of time.
>

Revised this as noted above, though note that MAY WISH TO is defined in RFC
6919.



> §4.7: “A new sender SHOULD send a packet containing the FullEKTField as
> soon as possible...”
>
> Why not MUST? Would it ever make sense not to do this? It seems like the
> mechanism doesn’t work very well otherwise.
>

How would you comply with a "MUST ... as soon as possible"?  I don't think
we want to dictate a schedule here, but I've added a note that packet loss
can result if you don't.



> §6, first paragraph: “or other” - what other?
>

Clarified.



> - paragraph 8: “ An endpoint MUST NOT encrypt more than T different
> FullEKTField values using the same EKTKey.”
>
> Is there a reciprocal requirement for decryption?
>

No.  The risk attaches once the ciphertext is transmitted.



> - last paragraph: “...SHOULD be limited using the ekt_ttl”
>
> Why not MUST?
>

Upgraded.



> §7.3: Should this cite RFC 5246 instead of RFC 4366?
>

You mean RFC 8446?  Actually this line and the similar one below it are
both unnecessary, so I've deleted them.



> *** Editorial Comments: ***
>
> §2: “...each endpoint picks there own SRTP master key...:
> s/their/its
>

Done.



> §4.1, first paragraph: Please reference figures by number. I can imagine
> some future RFC rendering failing to  maintain the relative positions.
> (This occurs a few times throughout the document.)
>
> §4.2.2, step 3:
> Should "The EKTCiphertext authentication is checked and is decrypted” be
> something like “
> The EKTCiphertext field authentication is checked and its contents
> decrypted” I assume you don’t mean to say its authentication is decrypted.
>

"The EKTCiphertext is authenticated and decrypted..." -- since these
operations are combined in many SRTP transforms.



> Also, there’s a number of occurrences of “The [fieldname]...” that should
> probably say “The [fieldname] field...”
>

This doesn't seem like a bad ambiguity to me.



> - Step 6: s/crypto/cryptographic
>

Disagree.



> - Step 7: is the replay protection part relevant to the step?
>

Deleted.  Added a note about replay to the security considerations.



> §4.3, third paragraph: This is the first mention of ekt_ttl. It would help
> to have a brief explanation or at least a forward reference.
>

Added a forward reference



> §4.6: s/ “other specification” / “another specification” ; or “other
> specifications”
>

Revised.



> §4.7, third paragraph: Both occurrences of “which” need preceding commas.
>

Done.



> §5.2, paragraph after figure 4: “ ... the extensions defined in this
> session allows the DTLS server...”
> Defined in this “session” or this “section”?
>

Actually, "in this document"



> §, paragraph 7: “... causing the receiving system will decrypt...”
> s/will/to
>

Done


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