[Suit] AD review of draft-ietf-suit-manifest-22

[Roman Danyliw <[e-mail]>]

Hi!

I performed an AD review of draft-ietf-suit-manifest-22.  Thanks for all of the work in producing this key WG deliverable.  My feedback is as follows:

** Abstract.  Editorial.  s/find the that/find the/

** Section 1.
   While the transport of
   firmware images to the devices themselves is important there are
   already various techniques available.  Equally important is the
   inclusion of metadata about the conveyed firmware image

Sentence one is saying that there are already solutions to transporting firmware.  Is the second sentence suggesting that what those existing solutions are missing is the ability to include meta-data about that firmware.  If so, please make it clearer.  If that's not the distinction, could the thesis be made clearer.

** Section 1
   End-to-end security allows
   the author, who builds the firmware image, to be sure that no other
   party (including potential adversaries) can install firmware updates
   on IoT devices without adequate privileges.

Is "end-to-end encryption" being framed here as some kind of code signing infrastructure?

** Section 1.
   For confidentiality
   protected firmware images it is additionally required to encrypt the
   firmware image.

Good point.  However, how is this relevant given that this document doesn't describe or mandate encryption of firmware images.

** Section 1.
    a Firmware Author to reason about releasing a firmware

What does it mean to "reason about releasing a firmware" from the perspective of the authors?

** Section 1.
   *  a Device Operator to reason about the impact of a firmware.

   *  the Device Operator to manage distribution of firmware to devices.

What is implied by the distinction between "_a_ device operator" and "_the_ device operator"?

** Section 1.  
   * a Plant Manager to reason about timing and acceptance of firmware
      updates.

Who is a "plant manager"?  How are they different than an "device operator"? 

** Section 2.   Definition of manifest.
-- Section 1 says the manifest is:
   A manifest is a bundle of metadata describing one or more code or
   data payloads and how to:

   *  Obtain any dependencies

   *  Obtain the payload(s)

   *  Install them

   *  Verify them

   *  Load them into memory

   *  Invoke them

-- Section 2 says:

  A manifest is a bundle of metadata about the firmware
  for an IoT device, where to find the firmware, and the devices to
  which it applies.

There seem to be two definitions of manifest.  Does it need to be done twice?
Section 2 notes that the applicability of the firmware, but this isn't noted in Section 1.

** Section 2.  Envelope.  Consider defining or providing a forward reference to explain "severable elements" or providing an explicit definition.

** Section 3.
   Additional
   specifications describe functionality of advanced use cases, such as:
   ...
   Compression of firmware images

What specification should be referenced here?  Where is compressing firmware described?

** Section 5.  Bullet point #2. Editorial. s/.././

** Section 5.1

   The SUIT Envelope is a container that encloses the Authentication
   Block, the Manifest, any Severable Elements, and any integrated
   payloads.  

The diagram at the end of Section 5 lists that the "SUIT Envelope" also includes "human-readable text".  This text is not mentioned in this sentence.  My understanding from later text is that this "human readable text" is severable.  Why does it get a call out but the other severable elements don't?

** Section 5.1.  Editorial.
OLD
and integrated payloads in a way
   that would add substantial complexity with existing solutions.  

NEW
... and integrated payloads in a way that avoids substantial complexity that would be needed with existing solutions.

** Section 5.2.  The digest container appears to support both MACs and Signatures.  These are slightly different security constructs.  Is there guidance on when one would use one vs. the other that are specific to the manifest?

** Section 5.4.3
   Integrated Payloads are integrity-checked
   using Command Sequences, so they do not have Integrity Check Values
   present in the Manifest.

I don't understand "... are integrity-checked using Command Sequences."  The subsequent reference to 8.4.11 is about severable items.

** Section 6.1
   Selecting an older manifest in the event of failure of the latest
   valid manifest is a robustness mechanism that is necessary for
   supporting the requirements in [RFC9019], Section 3.5.

RFC9019 doesn't have a Section 3.5.

** Section 6.2
   Once a valid, authentic manifest has been selected, the manifest
   processor MUST examine the component list and verify that its maximum
   number of components is not exceeded and that each listed component
   is supported.

Is this text the same as saying check that the number of components listed in the manifest is not larger than the number in the actual system?

** Section 6.2. Editorial.
   If the manifest contains more than one component, each command
   sequence MUST begin with a Set Component Index.

"Set Component Index" has being introduced yet.  A forward reference would help.

** Section 6.2
   has sufficient permissions imparted by its signatures

What is the permission model associated with signatures?  What if a MAC is provided?

** Section 6.2.1
   Signature verification can be energy and time expensive on a
   constrained device.  
    ...
   A Recipient MAY choose to parse and execute only the
   SUIT_Common section of the manifest prior to signature     verification,
   if all of the below apply:

    ...

   *  The Recipient receives manifests over an unauthenticated channel,
      exposing it to more inauthentic or incompatible manifests, and

I'm having trouble understanding the situation.  It seems like there is a use case to ignore the expensive signature check and execute SUIT_Common.  However, why can this can only be done in a less when the channel an unauthenticated channel?

** Section 6.2.1
   When executing Common prior to authenticity validation, the Manifest
   Processor MUST first evaluate the integrity of the manifest using the
   SUIT_Digest present in the authentication block.

Is this action for security reasons or to catch non-malicious corruption?  I ask because if this manifest was attacker-generated with a bogus signature (but unchecked), won't the digest value be under attacker control so it will always be match?

** Section 6.3
       Executing an update MUST either result in an error, or a
       verifiably correct system state.

What is "verifiably correct" in this context of this manifest specification?  Being in a "verifiably correct" state seems like a much stronger statement than the "manifest was successfully applied/run/executed"

** Section 6.3.1.
Either: 1.  A fallback/recovery image is provided so that a disrupted
   system can apply the SUIT Manifest again. 2.  Manifests are
   constructed so that repeated partial invocations of any manifest
   sequence always results in a correct system configuration. 3.  A
   journal of manifest operations is stored in nonvolatile memory so
   that a repeated invocation does not alter nonvolatile memory up until
   the point of the previous failure.

-- Using "either" typically suggests a choice between two options.  Is this text saying one chooses between three options or all required?

-- Is #2 on this list of the same as Section 6.3's item 3 ("Executing the same manifest on multiple Recipients MUST result in the same system state.").  Is there a distinction being made about being able to reapply a successful vs. partial update?  If not, #2 doesn't seem to be needed because this property is already required per the previous section.

-- Per #2, here reinvocations of partial sequence only need to result in "correct system configurations".  In Section 6.3, the result of an invocation needs to be a "verifiably correct system state."  Recommend consistency. 

** Section 6.4. Typo. Missing spaces (maybe rendering issue?). s/Processor--a/Process -- a/

** Section 6.4.  Editorial?

   current := components\[component-index\]

What does the backslash between the array index mean (aka, why not "components[component-index]"?)?

** Table 1.  I appreciate the pseudo code to explain the commands.  As written some assumptions need to be made about the semantics.  Recommend explaining:

-- assert(): does this returns a Boolean?  Are there side effects like throwing an error?
-- store(): which is the source / target?
-- the syntax of "override parameters" and "set parameters"
-- now(): is a time value?
-- try-each-done

** Section 6.5.  Editorial  
   When a command is executed, it either 1. ... 2.. 3..

Either is typically used for two options.

** Section 6.5
   if component-index is true:

Given that the two previous references to the Boolean value "True" were in uppercase, should it also be capitalized in this pseudo-code?

** Section 6.7
   Advanced Recipients MAY make use of the Strict Order parameter and
   enable parallel processing of some Command Sequences, or it may
   reorder some Command Sequences.  

-- Who is an "advanced recipient"?  Does this phrase equate to a "non-constrained device"?

-- I'm unable to tell, is setting the Strict Order parameter the setting which allows the parallel processing?

** Section 6.7.  The text describes which commands trigger a halt to parallel processing.  Shouldn't Swap and Write also cause a halt?

** Section 7.7.  Is the "Check Slot Condition" the same as a "Check Component Slot" condition (as described in Table 1)?

** Section 8.
   The metadata for SUIT updates is composed of several primary
   constituent parts: the Envelope, Authentication Information,
   Manifest, and Severable Elements.

   For a diagram of the metadata structure, see Section 5.

Doesn't the Envelope contain the authentication information?

** Section 8.3.  Editorial.
   This specification requires Sig_structure to be populated
   as follows: * The external_aad field MUST be set to a zero-length
binary string (i.e. there is no external additional authenticated
   data). * The payload field contains the SUIT_Digest wrapped in a
   bstr, as per the requirements in Section 4.4 of [RFC9052].  All other
   fields in the Sig_structure are populated as described in Section 4.4
   of [RFC9052].

Something appears to be amiss with the rendering in the .txt version of this draft.  It looks like a bulleted list was intended but it came out in paragraph form with asterisks as spacers.

** Section 8.3.
The suit-authentication-wrapper contains a SUIT Digest Container (see Section 10) and one or more SUIT Authentication Blocks.

-- Where does the text describe the input to the digest stored in SUIT_Digest and the input to the SUIT Authentication Block .  Put in another way, what is covered by the digest and signature?

-- Are multiple Authentication Blocks signatures of the same thing with different keys/algorithms?

** Section 8.3
A SUIT_Envelope that has not had authentication information added MUST still contain the suit-authentication-wrapper element, but the content MUST be a list containing only the SUIT_Digest.
Is this saying that a digest is required but signatures are optional?

** Section 8.3
The algorithms used in SUIT_Authentication are defined by the profiles declared in [I-D.moran-suit-mti].

-- I am unsure how to read this sentence.  The reference is informative which suggests I can ignore it as an implementer.  However, the text suggests that the algorithms to use are in these profiles.

-- Why does this explicitly define MTI digest algorithms, but is silent on MTI signature algorithms?

** Section 8.4.4.  suit-text seems to encode human readable text.  BCP 18 (RFC2277) reminds us that "   Protocols that transfer text MUST provide for carrying information about the language of that text."  How does suit-text do that?

** Section 8.4.4

    | suit-text-vendor-domain         | The domain used to create the |
    |                                 | vendor-id condition           |
    +---------------------------------+-------------------------------+
    | suit-text-model-info            | The information used to       |
    |                                 | create the class-id condition |

-- What does it mean to "create a {vendor-id, class-id} condition"?  Is this text equivalent to:

"The domain is evaluated (checked?) using the  'Check Vendor Identifier' condition"

"The information is evaluated (checked?) using the 'Check Class Identifier' condition"?

** Section 8.4.6.  suit-invoke is noted as being OPTIONAL to implement.  How can an update occur without calling this command?

** Section 8.4.7.
   *  The reporting policy

   *  The result of the command

   *  The values of parameters consumed by the command

   *  The system information consumed by the command

   Together, these elements are called a Record.  A group of Records is
   a Report.

-- Would it be more precise to say, "Together, the subset of these elements emitted by the Reporting Engine are called a Report"?  My intent is to emphasize that not all Records have the full collection of records.

-- If suit-send-sysinfo-success is set, all of these fields are returned?  If it is not set (but the command was successful), only the "result of the command" is set?

** Section 8.4.8.1.  Editorial.
   Computing a type 5 UUID
s/type/version/

** Section 8.4.8.2.
   UUIDs MUST be created according to RFC 4122 [RFC4122].  UUIDs SHOULD
   use versions 3, 4, or 5, as described in RFC4122.  Versions 1 and 2
   do not provide a tangible benefit over version 4 for this
   application.

Sentence one says all UUIDs MUST conform to RFC4112.  Sentence two says UUID version 3 - 5 is preferred.  Per the third sentence, why even allow for UUID version 1 and 2.  Couldn't SUIT just required version 3 - 5?

** Section 8.4.8.3
   Private Enterprise Numbers are encoded as a relative OID, according
   to the definition in [RFC9090].

This text seems to make RFC9090 a normative reference.  Is there a reason this reference is not?

** Section 8.4.8.9
   If data is encoded this way, it should be small.  Large payloads
   written via this method will prevent the manifest from being held in
   memory during validation.  Typical applications include small
   configuration parameters.

Is it possible to provide quantitative or qualitative guidance on what "small" or "large" means?

** Section 8.4.8.16.  Is there any additional guidance to provide on how to handle this parameter when parallel processing?

** Section 8.4.9.3. 
This MAY be implemented as follows:

   // content & component must be same length // returns 0 for match
   bool check_content(content, component, length) { int residual = 0 for
   (i = 0; i < length; i++) { residual |= content[i] ^ component[i]; }
   return residual; }

--  Editorial. Please format/indent this code to improve readability

-- Since a normative MAY is being invoked, this is code is now normative.  Please provide a normative reference to the programming language this is showing (C99?)

-- there is a missing semicolon in the variable declaration of residual

-- from the code readability perspective, it's a little messy that residual is declared as int but the return type is bool

** Section 8.4.10.1.  Is there a reason this section repeats normative language on when to support integers, Boolean and an array when it was already stated in Section 6.5?

** Section 8.4.10.2
  suit-parameter-soft-failure (Section 8.4.8.15) is initialized to True
   at the beginning of each sequence.

Is this parameter reset to its previous state after the sequence?

** Section 8.4.10.7.
   When this is invoked, the
   manifest processor MAY be unloaded and execution continues in the
   Component Index.
...
If the executable code at Component Index is constructed in such a
   way that it does not unload the manifest processor, then the manifest
   processor may resume execution after the executable completes.

Why is unloading the manifest processor a normative MAY but the resumption is not?

** Section 8.4.10.  What are SUIT_Parameter_Compression_Info or SUIT_Parameter_Encryption_Info and where are they defined?

** Section 8.5.
   Elements are made severable by removing them from the manifest,
   encoding them in a bstr, and placing a SUIT_Digest of the bstr in the
   manifest so that they can still be authenticated.

Doesn't the signature need to be recomputed if an element is severed?

** Section 9.  Which parts of the three presented models have standardized behaviors in this document?

** Section 10.
-- Does "The values of the algorithm identifier are defined by [RFC9054]" mean that the identifier values comes from the IANA "COSE Algorithms" registry ( https://www.iana.org/assignments/cose/cose.xhtml#algorithms)?

-- The text lists SHA-256 (as a MUST) and SHAKE128, SHA-384, SHA-512 and SHAKE256 (as a MAY).  Does this mean that no other algorithms are permitted?

** Section 11.*.  All of the references appear to be just section numbers.  Should it be section numbers and a reference to this document?

** Section 12.
   A detailed security treatment can be found in the
   architecture [RFC9019] and in the information model [RFC9124]
   documents.

It isn't clear to me if the security considerations outlined in RFC9019/RFC9124 are intended to apply in their entirety.  If not all of them, which ones?  

-- Section 6.1 provides weaker guarantees than suggested in RFC9019:
Section 6.1 of this document says:

   Selecting an older manifest in the event of failure of the latest
   valid manifest is a robustness mechanism that is necessary for
   supporting the requirements in [RFC9019], Section 3.5.  It may not be
   appropriate for all applications.

Here are a few that I found which didn't seem to be addressed:

-- THREAT.IMG.EXPIRED.OFFLINE (Section 4.2.2 of RFC9124):
   If the threat is from a network actor, including an on-path attacker,
   or an intruder into a management system, then a user confirmation can
   mitigate this attack, simply by displaying an expiration date and
   requesting confirmation.  On the other hand, if the user is the
   attacker, then an online confirmation system (for example, a trusted
   timestamp server) can be used as a mitigation system.

How does one represent an expiration date in the manifest? Is there a standardized "online confirmation system"?

-- REQ.SEC.AUTH.IMG_TYPE (Section 4.3.5 of RFC9124)

   The type of payload MUST be authenticated.
   ...
   Implemented by:  Payload Format (Section 3.8), Signature
      (Section 3.15)

Section 8.3 of this document seems to suggest that it is possible to not have authenticated contents (digest only) -- "A SUIT_Envelope that has not had authentication information added MUST still contain the suit-authentication-wrapper element, but the content MUST be a list containing only the SUIT_Digest."
 
-- REQ.SEC.IMG.CONFIDENTIALITY (Section 4.3.12 of RFC9124)

The manifest information model MUST enable encrypted payloads.

...

Implemented by:  Encryption Wrapper (Section 3.20)

This document does not describe any mechanism to encrypt a payload.  Section 3 says:

This specification covers the core features of SUIT.  Additional
   specifications describe functionality of advanced use cases, such as:

   *  Firmware Encryption is covered in
      [I-D.ietf-suit-firmware-encryption]

However, this reference is not normative and isn't consider a core feature.

Regards,
Roman