Re: [Lake] review of edhoc-12

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

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.



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