Re: [Perc] AD Evaluation of draft-ietf-perc-double-09

[Ben Campbell <[e-mail]>]

Works for me. The framework draft review is in my queue for tomorrow.

Thanks!

Ben.

> On Oct 15, 2018, at 5:42 PM, Richard Barnes <[e-mail]> wrote:
> 
> I would propose we put forward all three documents together.  (-double, -ekt-diet, and -framework)  They'll be more comprehensible to the ADs that way.
> 
> On Mon, Oct 15, 2018 at 6:40 PM Ben Campbell <[e-mail] <mailto:[e-mail]>> wrote:
> Thanks, the PR looks good to me. Please submit a revision when you are ready.
> 
> I think that, with the PR applied, it will be ready for IETF LC.  That should probably be done in tandem with the media framework (in my queue to review in the next couple of days. Authors and chairs: do you think we should also coordinate with ekt-diet?
> 
> Thanks!
> 
> Ben.
> 
> 
>> On Oct 5, 2018, at 9:48 AM, Richard Barnes <[e-mail] <mailto:[e-mail]>> wrote:
>> 
>> Hey Ben,
>> 
>> After talking with Cullen, I am now the stuckee for this.  I've filed a PR at:
>> 
>> https://github.com/ietf/perc-wg/pull/160 <https://github.com/ietf/perc-wg/pull/160>
>> 
>> Comments inline below.
>> 
>> On Tue, Jun 5, 2018 at 12:40 AM Ben Campbell <[e-mail] <mailto:[e-mail]>> wrote:
>> Hi,
>> 
>> This is my AD Evaluation of draft-ietf-perc-double-09. Thanks for this work; I think it is important and on the right track, but suffers from some readability issues, especially from §7 onwards. I’d like to discuss my comments prior to IETF last call.
>> 
>> Thanks!
>> 
>> Ben.
>> 
>> —————————————————
>> 
>> *** Substantive Comments ***
>> 
>> General: Does it make sense to progress this prior to draft-ietf-perc-private-media-framework rather than wait and progress them together? it seems like one really needs to read the framework to understand at least parts of this draft. (Which would, btw, suggest making the framework a normative reference.)
>> 
>> I think at this point, -framework has caught up a bit (I understand it's awaiting your review), so this seems OBE.  I agree that it will make easier reading for ADs to read the three docs (this, framework, and EKT) on the same telechat.
>> 
>> 
>> §2, Media Distributor: Should this be defined as switch-based? Otherwise it seems circular.
>> 
>> I've copied down the definition §1, which explicitly says that the MD does not need to interpret or change media content.  Also added a ref to -framework at this point.
>> 
>> 
>> §5 and subsections: These sections are confusing in the treatment of extensions. §5.1 step 3 says to truncate the header to remove any extensions. Yet other sections of text repeatedly mention the handling of extensions. I think the document needs a paragraph or two to describe the handling of RTP extensions at a high level.
>> 
>> I've added an overview of the transform, including thoughts on extensions, to the top of §5.
>> 
>> 
>> Along those lines, it’s not entirely clear what is meant by putting “ (12 + 4 * CC bytes)” in parentheses after the guidance to truncate in §5.1. Is that the amount to be truncated? Amount left after truncation? (Same question for §5.3)
>> 
>> Clarified that this is the amount left after truncation, i.e., take the first N bytes, in both places.
>> 
>> 
>> §5.2: It would be helpful to include a paragraph or two to describe the impact if multiple MDs modify the same field(s). I can infer that, but it would be better to be explicit. (IIRC, there is a passing mention in the security considerations, but it would be better to have more here.)
>> 
>> Added a paragraph.
>> 
>> 
>> §8: Can you offer any guidance about selecting 128 vs 256?
>> 
>> I don't really think there's much to say here; it's up to the user of the transform.  Use 128 unless you're paranoid, then use 256?  :)  Note that RFC 7714, which defines the AES-GCM transforms, does not provide similar guidance.
>> 
>> 
>> §9:
>> 
>> - The security considerations mainly summarize the mechanism. I’d like to see a description of the potential attacks or risks  and how the double mechanism mitigates them.
>> - “… this shouldn’t typically impact the strength of e2e integrity given the fact that there doesn’t exist header extensions defined today that needs e2e protection.  However, if future specifications define header extensions that needs e2e integrity protection, the input to inner transform may be modified to consider including the header.”
>> 
>> - 2nd paragraph: Why is the HBH key for an outgoing packet only “typically” different than the key for the incoming packet? Are there security implications if they are the same?
>> 
>> - 2nd to last paragraph, last sentence: Please elaborate on those risks?
>> 
>> I agree with your assessment that the existing security considerations were not that useful.  I've replaced them with an analysis that's more focused on what protections are provided with respect to which attackers.
>> 
>> The new version also calls out clearly that when a packet is re-encrypted, the outer keys MUST be different.
>> 
>> 
>> *** Editorial comments and nits ***
>> 
>> There are a number of abbreviations that need to be expanded on first use. Please remember that abbreviations should be expanded both in the abstract and the body.
>> 
>> - SRTP
>> - SSRC
>> - SEQ
>> - ROC
>> - PRF
>> - PT
>> - SEQ
>> - RTX
>> - RED
>> - FEC
>> 
>> I've expanded most of these.  I have left SSRC, SEQ, ROC, and PT, since in this context, they are basically opaque labels for RTP header fields.
>> 
>> 
>> §2:
>> - Please use the boilerplate from RFC8174. There are a number of lower case instances of normative keywords.
>> 
>> Done.
>> 
>> - Please use consistent sentence (or lack of sentence) structure for the definitions in the bullet list.
>> 
>> Done.
>> 
>> §3,
>> 
>> - " Generate key and salt values of the length required for the combined inner (end-to-end) and outer (hop-by-hop) algorithms.”: The document is inconsistent in whether it describes a single key with inner and outer halves, or separate inner and outer keys. I realize that this is an artifact of syntax, but it is likely to confuse a reader new to the topic.
>> 
>> The very next bullet says to glue them together.  Not sure how else to say this.
>> 
>> - Paragraph starting with “Obviously, if the Media Distributor…”: “Obviously” is a null word in context.
>> 
>> Deleted.
>> 
>> §3.1: This section would be much more approachable if you defined the terms (such as n, k, and x), rather than forcing readers to look them up in 3711. Also, the doubled crypto suite IDs are likely to be confusing to the reader who sees them here without the explanation later in the draft.
>> 
>> Done.
>> 
>> §5.2, step 2: “Change any parts of the RTP packet…” -The MS is limited to changing the 3 fields defined for the OHB, right, at least as defined in this draft? Not just anything it wants?
>> 
>> Correct.  Done.
>> 
>> §5.3, last two paragraphs: It’s sort of an odd construction to talk about “any of the following” with a single entry list..
>> 
>> Revised to clarify.
>> 
>> §7 and subsections: These could use another proofreading pass. In particular, it is imprecise about inner vs outer encryption, which actors do what, and has a number of pronouns with ambiguous antecedents. The heavy use of passive voice obscures the actors in multiple places. I will call out some specifics, but please do not treat my comments as exhaustive.
>> 
>> Did a revision pass, let me know what you think.
>> 
>> §7: “ The repair mechanism, when used with double, typically operates on the double encrypted data and encrypts
>>    them using only the HBH key.” Does “double encrypted” mean “outer encrypted”? Also, while I recognize “data” as plural, it’s plural in a non-countable sense; that is, s / them / it.
>> 
>> §7.1:
>> - “ … cached payloads MUST be the encrypted packets …” - Inner or outer? What device enforces this? (Consider active voice.)
>> - Which element represents “the other side”? Are we talking about an endpoint or MD? (Please stick to the defined terminology)
>> - “ When encrypting a retransmission packet, it MUST be encrypted the packet in repair mode.” - I can’t parse the phrase after the comma. Also, what device MUST encrypt it?
>> 
>> §7.2:
>> - “the primary encoding MAY contain…” - Is that MAY a grant of permission or a statement of fact?
>> 
>> - " The sender takes encrypted payload from the cached packets”- Missing article for encrypted payload? Or should this say “… takes encrypted packets from the cached payload…”?
>> 
>> - “ Any header extensions from the primary encoding are copied to the RTP packet that will carry the RED payload and the other RTP header information such as SSRC, SEQ, CSRC, etc are set to the same as the primary payload.  The RED RTP packet is then encrypted in repair mode and sent.”: This is confusing; parts are copied from the primary encoding and others are the same as in the primary encoding? Don’t both of those mean the same thing?
>> 
>> - “ The receiver decrypts the payload…”: Endpoint or MD? Is this inner or outer encryption?
>> 
>> - Last paragraph: Seems like that belongs in the FEC section. Also, does this document really need to make that sort of recommendation? It seems like a general statement about RED and FEC that is not specific to double.
>> 
>> 
>> §7.3:
>> 
>> - "When using Flex FEC [I-D.ietf-payload-flexible-fec-scheme] with double, the negotiation of double for the crypto is the out of band signaling that indicates that the repair packets MUST use the order of operations of SRTP followed by FEC when encrypting.” - The sentence is hard to parse. Also, is MUST a normative requirement or a statement of fact? (It seems odd to signal that you MUST do something).
>> - “…  data, which is already encrypted, it MUST be encrypted in repair mode packet.” s/ “, which” / “that”. Also, what actor MUST encrypt the data in repair mode? (Consider active voice.)
>> 
>> - “ The algorithm recommend…” s / recommend / recommended
>> 
>> Done.
>> 
>> §7.4:
>> - s/ "sent with [RFC4733]” / “sent using the mechanism in [RFC4733]”
>> - “ and the relay can not read it so it cannot be used” - missing comma between “it” and “so”.
>> 
>> Done.
>> 
>> §8, last paragraph: “ For packets in repair mode, the data they are caring is often already encrypted further increasing the size.” - I assume “caring” is not the intended word. Did you mean “carrying”? If so, consider s / “data they are carrying” / “data they carry”
>> 
>> Revised.
>> _______________________________________________
>> Perc mailing list
>> [e-mail] <mailto:[e-mail]>
>> https://www.ietf.org/mailman/listinfo/perc <https://www.ietf.org/mailman/listinfo/perc>
>