Re: [Lake] review of edhoc-12

[Stephen Farrell <[e-mail]>]

Hiya,

On 14/12/2021 13:43, Göran Selander wrote:
> Hi Stephen,
> 
> Thanks for the review, it is recorded as github issue #202. Comments
> below.
> 
> A proposed update to the draft is in PR #211, but some of the
> comments we made into separate issues or additions to existing
> issues, as detailed in the comments.

I'll go over this later but do you prefer a mail thread
or discussion on the GH issue where there's one?

Either's fine by me...

Cheers,
S.

> 
> 
> 
> From: Lake <[e-mail]> on behalf of Stephen Farrell
> <[e-mail]> Date: Thursday, 11 November 2021 at
> 01:39 To: [e-mail] <[e-mail]> Subject: [Lake] review of
> edhoc-12
> 
> Hiya,
> 
> At our last interim I also promised to review edhoc. This is my late
> (apologies;-) review. This is all review by me as just another
> participant, not as co-chair. (So please do feel entirely free to
> ignore bits or just say no.)
> 
> Generally, I guess I could implement from this, were I fluent in
> CBOR/COSE etc, so I think it's in good shape. All but my first
> comment are editorial. I assumed that we'll have others who do crypto
> analysis and implementers who'll provide yet more detailed feedback
> as we go, so we're not quite there yet but getting pretty close IMO. 
> Good job!
> 
> Cheers, S.
> 
> 
> - Connection identifiers (which can be byte-strings) are sent in
> clear which could enable various network observer attacks for
> protocols that later send values obviously derived from connection
> IDs in clear. (I see from A.3 that one main use case does expose
> these values anyway.). To given an example of how this could be
> concerning, if some proxy (that just muxes packets) sits between I
> and R then those cleartext identifiers could allow an observer on
> that link to more easily do traffic analysis of a specific 
> initiator's traffic. Was any consideration given to deriving such
> identifiers in a less obvious manner? I'm not claiming that that'd be
> a great improvement, just asking. {GS: John provided a response in
> github issue #202 (second post on the issue). The discussion also
> continues on the future-OSCORE-update repo [1]. Here is my attempt to
> summarize the topic: 1. Connection identifiers are selected in EDHOC
> and may be used with EDHOC (as in A.3) or in OSCORE (as
> Sender/Recipient IDs). 2. The selection of connection IDs allows very
> short locally unique identifiers. Derived stochastically unique
> connection IDs would be much longer. 3. Connection IDs, either when
> used as in A.3 or in OSCORE, work as key identifiers to facilitate
> retrieval of the security context used to decrypt the message and
> therefore need to be in plain text when used for this purpose. 4.
> Connection IDs can become more difficult to trace by negotiating
> multiple connection IDs for one security context and/or by updating
> the identities in ways that does not reveal the binding in plain
> text. OSCORE is not using multi-path and therefore we don't  need
> multiple connection IDs like e.g. in QUIC. The update of identities
> is proposed to be included in Key Update for OSCORE (KUDOS, [2])
> instead of in EDHOC because: a. KUDOS assumes an existing OSCORE
> security context which can be used to encrypt the first message, and
> b. although it would be possible to link different messages of a
> single EDHOC key exchange to the same endpoints performing A.3, the
> main privacy implication is most likely on the application protocol. 
> With this in mind we propose to not change how connection IDs are
> established and used in EDHOC, but we should make a security
> consideration about this for which we opened a separate issue #213. 
> Is this sufficient? [1] Potential things to add to future update ·
> Issue #263 · core-wg/oscore ·
> GitHub<https://github.com/core-wg/oscore/issues/263> [2]
> https://datatracker.ietf.org/doc/draft-ietf-core-oscore-key-update/ 
> (Note also issue/PR #188/189 about padding of plaintext, the lenght
> of which also can be used to identify an endpoint.) }
> 
> 
> Editorial:
> 
> - 80 pages is still big, I understand its hard to delete stuff but
> hope we keep trying:-) [GS: Yes. There are things we can do, e.g. as
> you point out in sections 1.2 (gone in PR #211) and 3.5, and avoid
> repetition of key derivation in sections 4 and 5. But there is also a
> limit to what is worth the effort, and sometimes readability is
> improved by somewhat overlapping text although different in
> structure. We are now at 40 pages excluding security considerations /
> IANA / refs and appendices -  how long should an AKE be? (As a
> comparison, when I did my masters prof. Gabriel Barton told me that a
> master thesis is 10 pages long, anything else goes into appendices
> :-) ]
> 
> - 1.1, CWT, CSS and C509 could do with expansion here on 1st use.
> (Perversely, X.509 is probably sufficiently well known and disliked
> to not need such:-) That might also make the text before (or caption
> of) Figure 1 easier to read. [GS: Done]
> 
> - section 1.2: last para - this is repetitive, suggest making these
> points just once [GS: Content of 1.2 now merged into with 1.1 and
> shortened.]
> 
> (Aside on figures/captions: I always find it a useful exercise to
> ensure that a figure+caption can, by itself, make a meaningful slide
> to present with no additions. And then I remove most text that's
> already in the caption from elsewhere in the document. Might be worth
> a try.) [GS: I sympathize with this and gave it a try but had some
> problems in practice. The explanation of the content in Figure 1 is
> already 7 lines of compact text with expanded abbreviations and
> references which I think better stay immediately above the figure
> than in the caption. The terminology in Figure 2 is explaned in 10
> lines of text which is better structured as currently with bullets
> rather than running text in a caption. Figure 3 includes 10 types of
> protocol elements/primitives which again would make the caption
> unwieldly. While this principle was difficult to apply in general, it
> can make sense to expand the captions in Figures 8 and 9 to provide
> more text about the cipher suite negotiation so I tried there. I also
> made a consistency check of the figures harmonizing capitalization
> and minor updates (except Figure 4 which I already worked on in
> another PR, to avoid merge conflicts). ]
> 
> 
> - Figure 1: I don't get how the "Figure 1 shows..." sentence results
> in those example message sizes. I'm not doubting the numbers, but the
> text could be improved (maybe, as suggested above via caption text.) 
> [GS: The reference in the preceding sentence also applies here, we
> added that reference again.]
> 
> - 1.4: are "EDHOC authenticated with digital signatures" and "EDHOC
> authenticated with signature keys" different things? If not, using
> one term is likely better. If so, using terms that are more clearly
> different would be good. [GS: Replaced "digital signature" with
> "signature keys" consistently.]
> 
> - 1.5: which is normative, CDDL or English language text? We seem to
> have a bit of a mixture. [GS: CDDL defines the formats, and English
> language text complements the format definitions. While there may be
> a potential conflict, I didn't find any examples of that. The only
> places where I see an overlap are places like this: "message_2 SHALL
> be a CBOR Sequence (see {{CBOR}}) as defined below ~~~~~~~~~~~ CDDL 
> message_2 = ( G_Y_CIPHERTEXT_2 : bstr, C_R : bstr / int, ) 
> ~~~~~~~~~~~ " In this case the CDDL indicates that message_2 is a
> CDDL group (Section 2.1 (.2) in RFC 8610), and the English text makes
> this more specifc in that the particular group in this case is a CBOR
> Sequence. Unless there is an actual conflict, I'm not sure it adds to
> understanding neither to talk about a potential non-existing
> conflicts in general terms, nor to talk about CDDL groups in this
> document. ]
> 
> - Figure 2: I see why message 2 doesn't also use an AEAD(), but
> probably no harm to say that more explicitly here. Maybe moving some
> of the relevant text from section 8 to here would work. [GS: Giving
> the precise reason may be too much for a caption, but it includes now
> at least a high level motivation and the search item "SIGMA". ]
> 
> 
> - Figure 2: consider showing the AAD as an input to the AEAD()
> construct in the figure. That might be too cumbersome or it may help,
> not sure. {GS: We have changed notation in this figure a few times
> :-) AAD was included in e.g. [3]. AAD input has several components so
> does not make for simple figures. Hard to get right level of
> information and keep message content on one line. Further suggestions
> are welcome! } [3]
> https://datatracker.ietf.org/doc/html/draft-selander-ace-cose-ecdhe-09
>
> 
> 
> - 3.5.1, 3.5.2 and 3.5.3: this might be some text to shorten a lot -
> how much is it really needed? Could it be cut down to the stuff with
> 2119 terms and a bit more? (I'd keep the examples though.) [GS:
> Section 3.5 is 6 pages long, so, yes there is a potential to
> shortening. (Not that we haven't restructured it before ;-) Let's
> have another look, I made it a separate issue, #212.]
> 
> - 3.6: does EDHOC *really* support hash based sigs? What'd be the
> consequence for EDHOC of using a private key too many times or loss
> of state?  (Are you missing a reference to rfc8778 there too or is
> one embedded in COSE stuff somewhere?) [GS: In principle, yes, with a
> private cipher suite, using algorithms as defined in COSE, which
> should have the relevant references. The same would go for, e.g.,
> RSA. But since this is not a main use case I removed this example
> from the main text on cipher suites in section 3.6 and just left the
> note in the PQC considerations section 8.4. ]
> 
> - 4.1: Be good to clarify that the PRK_<foo> labels in 4.1.x are
> basically local key names. (Same for others in 4.2.) [GS: I didn't
> get this comment. I assume "label" here does not refer to the 'label
> part of the info data structure, since PRK_<foo> is not included in
> any label. Did you want the text to be explicit that the different
> PRK_<foo> are names of pseudo-random keys? (I added this.) ]
> 
> 
> - 4.1.2/3: You need to define R and I in each of these as (I guess)
> the static DH secret and not as the identities of the initiator and
> recipient which were (I thought) the previous uses of I and R. Might
> be better to use different names though. [GS: You are right that I
> and R are also used for something else, but not as identities of I
> and R, rather as abbreviation/shorthand for "Initiator" and
> "Responder", respectively. We thought it was OK to overload I and R
> on this point since there should be very small risk of confusion. (I
> wouldn't know how to calculate an ECDH shared secret from a public
> key and a public fixed text string.) I and R in the sense used in
> 4.1.2/3 is defined in 3.5.2,  I added a reminder about that in
> 4.1.2/3 with reference. ]
> 
> - 5.2.2: What does "truncated after the cipher suite selected for
> this session" mean? (Also be good to say order of preference means
> first in network byte order is most-preferred.) I'm also puzzled by
> "but all cipher suites which are more preferred than the selected
> cipher suite in the list MUST be included in SUITES_I." [GS: I made
> some updates to the text, have a look and see if it reads better: 
> https://github.com/lake-wg/edhoc/pull/211/commits/a72bad2a I wonder
> if the first part changed needs a different structure, here is a
> potential change which I didn't make, do you think I should? OLD *
> SUITES_I - array of cipher suites which the Initiator supports in
> order of preference, starting with the most preferred and ending with
> the cipher suite selected for this session. If the most preferred
> cipher suite is selected then SUITES_I is encoded as that cipher
> suite, i.e., as an int. The processing steps are detailed below and
> in {{wrong-selected}}. ALTERNATIVE * SUITES_I - array of cipher
> suites complying with the following conditions (processing steps are
> detailed in {{init-proc-msg1}} and {{wrong-selected}}): * All cipher
> suites in SUITES_I are supported by I * The cipher suites in SUITES_I
> are listed in order of preference by I, i.e., the first in network
> byte order is most preferred, etc. * The last cipher suite in
> SUITES_I is selected by I to be used in this EDHOC session * If the
> most preferred cipher suite coincides with the selected cipher suite
> (i.e. first cipher suite = last cipher suite in SUITES_I) then
> SUITES_I is encoded as that cipher suite, i.e., as an int instead of
> an array. ]
> 
> - 5.3.2: This seems to be the first occurrence of the "<<..>>"
> symbolism. A forward ref to C.1 would be good.
> 
> 
> [GS: Done]
> 
> 
> - 5.3.3: is it ok to pass EAD_2 to the application before checking
> authentication?
> 
> 
> [GS: Good question. If EAD_2 indeed contains external authorization
> data (e.g. information about identity of intended endpoint), then
> EAD_2 needs to be processed before you can verify the identity of the
> other endpoint and the integrity of the message. But if we look at
> EAD_3, the same things apply, but we also claim that EAD_3 is
> protected between Initiator and Responder, which with the current
> order of things isn't verified at the time when EAD processed. I
> added this to issue #210. ]
> 
> - section 6: I found this v. hard to follow. Suspect a re-write for
> clarity would be good. [GS] Thanks for sharing. Could you be a little
> bit more specifc what is hard to follow? Is a) the general error
> handling, b) the format of the error message, c) the currently
> defined error codes or how they are used, d) specifically the cipher
> suite negotation , e) all above, or something else? Note: Stefan also
> commented on ERR_CODE 0 for which we made an update of section 6.1
> Success, see PR #200.
> 
> - section 8: "EDHOC provides a minimum of 64-bit security..." could
> do with a reference to wherever that conclusion is derived. And
> 64-bit security isn't quite "in line" with TLS is it? [GS: Thanks,
> this text needs to be improved. The statement that the MAC must be at
> least 8 bytes is a change already included in master branch post -12,
> but the security level is also dependent on what algorithm is used.
> We removed "This is in line with IPsec, TLS, and COSE." and we added
> this to issue #201.]
> 
> 
> - 8.7: stating that a "truly random seed MUST" be provided isn't a
> sensible use of 2119, and isn't entirely under the control of an
> implementer (maybe the section title could be re-thought?).
> 
> 
> [GS: Right, here is a proposed update: OLD If no true random number
> generator is available, a truly random seed MUST be provided from an
> external source and used with a cryptographically secure pseudorandom
> number generator. NEW If no true random number generator is
> available, a random seed must be provided from an external source and
> used with a cryptographically secure pseudorandom number generator. 
> About the section title, this is a subsection of the security
> considerations and the content does have relevance when deciding
> about implementations, what would you propose instead? ]
> 
> - 8.7 (or somewhere): if some random values are visible (connection
> identifiers?) then it can make sense to derive those from a different
> random stream compared to that used for randomly picking secrets.
> That way the publicly visible random numbers are less likely to leak 
> information about the state of the PRNG used for secrets. Could be
> worth a mention. [GS: Good point. New issue #214.]
> 
> 
> - 8.7: discarding a message_1 because of G_X collision/reflection
> should also be stated earlier - it could very easily be missed here.
> 
> 
> [GS: Agree. We should add into the processing of message_1. This
> point is added to issue #201.]
> 
> 
> - 8.7: TEE => "cannot" be tampered is a little optimistic IMO. {GS:
> Agree, this is taken from the abstract of [4]: "A Trusted Execution
> Environment (TEE) is an environment that enforces that any code
> within that environment cannot be tampered with, and that any data
> used by such code cannot be read or tampered with by any code outside
> that environment."
> 
> [4]
> https://datatracker.ietf.org/doc/html/draft-ietf-teep-architecture-15
>
> 
Proposed change:
> OLD The use of a TEE enforces that code within that environment
> cannot be tampered with, and that any data used by such code cannot
> be read or tampered with by code outside that environment. NEW The
> use of a TEE aims at preventing code within that environment to be
> tampered with, and preventing data used by such code to be read or
> tampered with by code outside that environment. }
> 
> - 9.10: The well-known URI is not mentioned at all in the body of the
> spec but only here and then in an appendix. A forward ref to A.3 from
> 9.10 is probably enough to fix. The same may apply to other IANA
> registrations I guess. [GS: Done]
> 
> 
> - 10.1: are we confident that all the normative I-Ds will become RFCs
> in a sufficiently timely manner? I've no specific reason to think
> they won't, and they're all far along the process, but sometimes
> transitive dependencies can cause a lot of delay.  (Very sadly, we
> can't just ask Jim about this.)
> 
> 
> [GS: We think so. Those which are not RFCs would probably be OK as
> informative references.]
> 
> - A.1: "byte string shaped"? what's that mean? [GS: The parenthesis
> containing this text is removed. The meaning is hopefully explained
> by the example.]
> 
> 
> - Appendix D: Point 6 mentions an EUI-64. I'm not clear how that'd be
> used in edhoc. [GS: Reference to example in 3.5.3 added.]
> 
> 
> - The URL for SIGMA can use https so change to [1] [1]
> https://webee.technion.ac.il/~hugo/sigma-pdf.pdf [GS: Done]
> 
> Göran
>