Encrypted Payloads in SUIT Manifests
draft-ietf-suit-firmware-encryption-24

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

# É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 ;-)

I have nothing useful to add here....