Ballot for draft-ietf-suit-firmware-encryption
Yes
No Objection
No Record
Summary: Has enough positions to pass.
NIT: Here’s an improved version of your text: This line was likely not meant to be included :)
# Orie Steele, ART AD, comments for draft-ietf-suit-firmware-encryption-21 CC @OR13 * line numbers: - https://author-tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-suit-firmware-encryption-21.txt&submitcheck=True * comment syntax: - https://github.com/mnot/ietf-comments/blob/main/format.md * "Handling Ballot Positions": - https://ietf.org/about/groups/iesg/statements/handling-ballot-positions/ ## Comments Thanks to Martin Thomson for the very thorough ARTART review. I've extracted a few comments from the ARTART review, but I can also see that many of Martin's comments have already been addressed. ### Too many ciphers? From https://datatracker.ietf.org/doc/review-ietf-suit-firmware-encryption-20-artart-lc-thomson-2024-08-01/ ``` The draft defines the use of AES-CBC. I see no value in using that scheme. AES-CTR has value in that it doesn't change the size of the ciphertext. An AEAD has the advantage of being able to bind additional context and perform data origin authentication in exchange for the extra padding. CBC modes offer none of those advantages, but expand the size by almost the same amount. ``` I'm not a firmware expert, but I did find myself wondering what is MTI, and why the need for the degree of optionality the document enables. ### EDN ``` 386 Figure 3: Example showing the extended suit-directive-write. ``` I suggest explaining that this example instance is in EDN similar to how CDDL is called out above: ``` 350 Figure 2: CDDL of the SUIT_Parameters Extension. ``` ### why not https? ``` 393 i.e. "http://example.com/encrypted.bin". The encrypted payload will ``` Why not "https://example.com/encrypted-firmware" ? ### This deployment option is ...? ``` 515 1. Generate CEK 516 2. for i=1 to n 517 { 518 2a. Fetch KEK[Ri, S] 519 2b. ENC(CEK, KEK[Ri, S]) 520 } 521 3. ENC(payload, CEK) ``` and ... ``` 530 1. for i=1 to n 531 { 532 1a. Fetch KEK[Ri, S] 533 1b. Generate CEK[Ri, S] 534 1c. ENC(CEK[Ri, S], KEK[Ri, S]) 535 1d. ENC(payload, CEK[Ri, S]) 536 2. } ``` I was expecting to see a recommendation after this, given the discouragement after the first scenario in section 6.1.2. Also is it really necessary to give 3 options here?... Feels a bit like punting complexity to implementers / deployments. ### Algorithm mandatory but not tstr ``` 566 1 => int, ; algorithm identifier ``` Typically algorithm is optional in COSE Headers, but when present it can be int / tstr. I am confirming the intention is to have it be mandatory, and to restrict it to integers. Is it desired to exclude any specific integers (algorithms) here? (-5, -4, -3) Same comment later for: ``` 670 1 => int, ; algorithm identifier ``` (-31, -30, -29) ? If there is MTI coming from another layer of SUIT, it would be good to remind folks of that somewhere near these CDDLs. I see my questions are answered later here: ``` 706 * The COSE_KDF_Context.AlgorithmID field MUST contain the identifier 707 for the AES Key Wrap algorithm being used. This specification 708 uses the following values: A128KW (value -3), A192KW (value -4), 709 or A256KW (value -5) ``` Although its not clear if these are intended to be the only supported algorithms or which of them art MTI. ### ECDH-ES + HKDF forbidden? ``` 601 Another variant of the ES-DH algorithm, called ECDH-ES + HKDF, does 602 not utilize AES Key Wrap. However, this version is not covered in 603 this document. ``` Is there a reason to discourage future use of "ECDH-ES + HKDF" ? Why is it not a better choice for this document?... should we expect to see it used in the future? ### Per device payload encryption ``` 630 * The alternative is to encrypt the payload with a unique CEK for 631 each recipient, resulting in multiple manifests. This approach is 632 useful when payloads contain device-specific information. In this ``` I think this is meant to convey "encrypt each device specific payload with a unique content encryption key" ... "resulting in a manifest per device specific payload". I am getting thrown off by the lack of plurality here: "The alternative is to encrypt the payload" ### nil ct? ``` 655 ciphertext : bstr / nil, ``` But then later this parameter is described as "payload": ``` 864 / payload: / null / detached ciphertext /, ``` I think the correct EDN for this case is: ``` null / detached ciphertext / ``` ### Cross mode attack mitigation ``` 720 * The COSE_KDF_Context.SuppPubInfo.protected field MUST contain the 721 serialized content of the recipient_header_map_esdh field, which 722 contains (among other elements) the identifier of the content key 723 distribution method. ``` This is only true if algorithm is mandatory in the header. I believe this requirement is mitigating cross mode attacks, because a change in "COSE_KDF_Context.AlgorithmID" would cause a different content encryption key to be generated. ``` 668 recipient_header_map_esdh = 669 { 670 1 => int, ; algorithm identifier 671 * label => values ; extension point 672 } ``` It might be clearer to align "identifier of the content key distribution method" with "algorithm identifier" ... and to provide an example algorithm for both sides, so that it is obvious that both -5 and -31 are mixed into the key when they are used. You could also make this clearer by changing the CDDL: ``` 734 ? SuppPrivInfo : bstr -> bstr .cbor recipient_header_map_esdh, ``` ### Protected EDN ``` 1108 o protected: { / alg / 1: -29 / ECDH-ES+A128KW / } ``` Should this be (CDDL): ``` .cbor { 1: -29 } ``` Or EDN: ``` <<{ 1: -29 }>> ``` Or should it be expressed as bytes?... maybe bytes is better here. It might also be helpful to provide the full bytes in EDN of the KDF Context. ## Nits ### Diagrams From https://datatracker.ietf.org/doc/review-ietf-suit-firmware-encryption-20-artart-lc-thomson-2024-08-01/ ``` For the fancy diagrams showing the cipher modes (which are entirely unnecessary, in my view, there are citations you can use) aasvg recognizes ⊕, which would make the SVG more comprehensible; presently it is not. Figure 11 references the previous diagram for a legend. That diagram (Figure 1) does not include a legend; the next one (Figure 14) does. As far as legends go, it might be better to have those in text, rather than the figure. ```
Thank you for addressing my DISCUSS and COMMENT feedback.
I have nothing useful to add here....
# Éric Vyncke, INT AD, comments for draft-ietf-suit-firmware-encryption-21 Thank you for the work put into this document. Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and some nits. Special thanks to Akira Tsukamoto for the shepherd's detailed write-up including the WG consensus *but it lacks* the justification of the intended status. Please note that Suresh Krishnan is the IoT directorate reviewer (at my request) and you may want to consider this int-dir review as well when it will be available (no need to wait for it though): https://datatracker.ietf.org/doc/draft-ietf-suit-firmware-encryption/reviewrequest/20711/ I hope that this review helps to improve the document, Regards, -éric # COMMENTS (non-blocking) Thanks for using SVG rather than ASCII art ;-) ## Section 3 `Consequently, the device must be safeguarded against physical attacks` even if this is a lowercase "must", it is nevertheless normative and I wonder how can a constrained device can offer such protection against physical attacks. `this model represents the current standard practice for IoT firmware updates` a reference will be welcome. ## Section 5 `Here’s an improved version of your text:` probably a copy & paste error ? Is HTTP correct rather than HTTPS in `http://example.com/encrypted.bin`? ## Section 9 Probably due to my ignorance but is it true that `begins decrypting it sector by sector` applies to *all* IoT devices ? ## Section 10 Should probably be in the appendix, but this a matter of taste. # NITS (non-blocking / cosmetic) AFAIK, "i.e." is always followed by a comma. ## Section 8.2 After `There are three *conditions* to consider` I would expect C1, C2, C3 rather than Q1, Q2, Q3 ;-)