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

From: Richard Barnes <[email-masked]>
Date: Tue, 9 Oct 2018 15:06:46 -0400

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 <[email-masked]> 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
> [email-masked]
> https://www.ietf.org/mailman/listinfo/perc
>

---

From: Ben Campbell <[email-masked]>
Date: Tue, 9 Oct 2018 14:09:25 -0500

Thanks!.  I will add this to my list of things to look at after this week’s telechat.

Ben.

> On Oct 9, 2018, at 2:06 PM, Richard Barnes <[email-masked]> wrote:
> 
> Thanks for the review, Ben.  I've posted a PR with responses; notes inline below...
> 
> https://github.com/ietf/perc-wg/pull/161 <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 <[email-masked] <mailto:[email-masked]>> wrote:
> Hi,
> 
> 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.
> 
> ----------
> 
> §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.
> 
> §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?
> 
> - 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:
> 
> 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.
> 
> 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.
> 
> 
> - paragraph 8: “ An endpoint MUST NOT encrypt more than T different FullEKTField values using the same EKTKey.”
> 
> You mean RFC 8446?  Actually this line and the similar one below it are both unnecessary, so I've deleted them.
> 
> 
> §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...”
> 
> §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.
> 
> §4.6: s/ “other specification” / “another specification” ; or “other specifications”
> 
> Revised.
> 
> 
> §5.2, paragraph after figure 4: “ ... the extensions defined in this session allows the DTLS server...”
> [email-masked] <mailto:[email-masked]>
> https://www.ietf.org/mailman/listinfo/perc <https://www.ietf.org/mailman/listinfo/perc>

---

From: Ben Campbell <[email-masked]>
Date: Mon, 15 Oct 2018 18:03:20 -0500

The PR looks good to me. Once submitted with the PR applied,I think this is ready for IETF LC, subject to coordination with the framework and double drafts. Please submit when you are ready.

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

[...]

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

Sure, put a normative reference to RFC 6919 in there. The resulting downref discussion on the telechat should be fun. ;-)

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

I note that “SHOULD ... as soon as possible” has somewhat the same issue. But adding the note addresses my concern.

[...]
> *** Editorial Comments: ***

[...]

> - Step 6: s/crypto/cryptographic

Really? I’m not going to push the point, but I’m curious why? Especially since the financial industry is doing it’s best to steal “crypto”. But now I wonder if I’ve missed some subtle difference of definition.

[...]