Re: [Lake] review of edhoc-12

[Göran Selander <[e-mail]>]

Hi, 

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

In this case we went for a mail thread to allow more visibility of the discussion. 

Thanks
Göran


> 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://protect2.fireeye.com/v1/url?k=31323334-501d5122-fe22d327-454445555731-499212e7ddad767f&q=1&e=f1d0e121-1c0d-422b-8b1b-ca672d8c8a74&u=https%3A%2F%2Fgithub.com%2Fcore-wg%2Foscore%2Fissues%2F263> [2]

> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-fe22d327-454445555731-c589ab9ddcc753fd&q=1&e=f1d0e121-1c0d-422b-8b1b-ca672d8c8a74&u=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-core-oscore-key-update%2F 

> (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://protect2.fireeye.com/v1/url?k=31323334-501d5122-fe22d327-454445555731-c91bc00ab62f9db5&q=1&e=f1d0e121-1c0d-422b-8b1b-ca672d8c8a74&u=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fhtml%2Fdraft-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://protect2.fireeye.com/v1/url?k=31323334-501d5122-fe22d327-454445555731-f9b2427a5945f1a1&q=1&e=f1d0e121-1c0d-422b-8b1b-ca672d8c8a74&u=https%3A%2F%2Fgithub.com%2Flake-wg%2Fedhoc%2Fpull%2F211%2Fcommits%2Fa72bad2a 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://protect2.fireeye.com/v1/url?k=31323334-501d5122-fe22d327-454445555731-5a3bbcb6fcd7b5fb&q=1&e=f1d0e121-1c0d-422b-8b1b-ca672d8c8a74&u=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fhtml%2Fdraft-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://protect2.fireeye.com/v1/url?k=31323334-501d5122-fe22d327-454445555731-8ca11aa0b522bd93&q=1&e=f1d0e121-1c0d-422b-8b1b-ca672d8c8a74&u=https%3A%2F%2Fwebee.technion.ac.il%2F%7Ehugo%2Fsigma-pdf.pdf [GS: Done]

> 

> Göran

>