Re: [Suit] Follow-up AD review on draft-ietf-suit-manifest-23

[Roman Danyliw <[e-mail]>]

Hi Hannes!

Thank you for this update, tracking the feedback in github, and producing -23 -- it was a lot of work.  My primary intent in the AD re-review was to signal that I'm up-to-date on the change.  No pressure otherwise.  I suspect some of the remaining feedback may take WG discussion.

Thank you,
Roman

> -----Original Message-----
> From: Suit <[e-mail]> On Behalf Of Hannes Tschofenig
> Sent: Friday, September 22, 2023 12:02 PM
> To: Roman Danyliw <[e-mail]>; [e-mail]
> Subject: Re: [Suit] Follow-up AD review on draft-ietf-suit-manifest-23
> 
> Hey Roman,
> 
> I have addressed half of your issues already without creating issues for them.
> Then, when I ran out of steam, I added the remaining issues to the track the
> open items. Recently, there has been some progress with them with Brendan &
> Henk joining.
> 
> 
> Thanks again for the super-detailed feedback. It must have taken you a while to
> put this review together. Your input will make the document much better.
> 
> 
> Ciao
> Hannes
> 
> Am 22.09.2023 um 17:29 schrieb Roman Danyliw:
> > Hi!
> >
> > Previously, I had performed an AD review of draft-ietf-suit-manifest-22.  See
> https://mailarchive.ietf.org/arch/msg/suit/Ak_sFp1PaZcIRSol5Ge_xH2FN-w/.
> Thank you for tracking this feedback in github and then producing -23 which
> addressed some of this feedback.  For clarity, I'm resending this note to
> enumerate the residual AD feedback.  In almost all cases, I now provide a
> pointer to an issuer number.  I found two small things in the re-review.
> >
> > New Feedback:
> > ** Section 5.3.4.  Editorial and introduced in -23: Per “see {#ovr-integrated},
> are integrity-checked …”, there is misspelled Markdown reference.
> >
> > ** This document referenced RFC4122.  The bis for it, draft-ietf-uuidrev-
> rfc4122bis, is in IESG review.  Consider if there is a reason why it could not be
> used instead.  This feedback come during other IESG reviews of documents
> referencing RFC4122.
> >
> > Previous Feedback:
> >
> > ** 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?
> >
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/86
> >
> > Section 6.2
> >     has sufficient permissions imparted by its signatures
> >
> > What is the permission model associated with signatures?  What if a MAC is
> provided?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/87
> >
> > 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?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/88
> >
> > 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?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/89
> >
> > 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"
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/90
> >
> > 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.
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/91
> >
> > Section 6.4.  Editorial?
> >
> >     current := components\[component-index\]
> >
> > What does the backslash between the array index mean (aka, why not
> "components[component-index]"?)?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/92
> >
> > 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
> >
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/93
> >
> > 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?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/94
> >
> > 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)?
> >
> >
> >
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/95
> >
> > 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?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/96
> >
> > 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].
> >
> > -- Why does this explicitly define MTI digest algorithms, but is silent on MTI
> signature algorithms?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/97
> >
> > 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?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/98
> >
> > 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"?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/99
> >
> > Section 8.4.6.  suit-invoke is noted as being OPTIONAL to implement.  How
> can an update occur without calling this command?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/100
> >
> > 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.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?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/101
> >
> > 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?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/102
> >
> > Section 8.4.8.16.  Is there any additional guidance to provide on how to
> handle this parameter when parallel processing?
> >
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/103
> >
> > 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?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/104
> >
> > 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?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/105
> >
> > 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?
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/106
> >
> >   Section 9.  Which parts of the three presented models have standardized
> behaviors in this document?
> >
> >
> > ** https://github.com/suit-wg/manifest-spec/issues/108 and
> >
> > https://github.com/suit-wg/manifest-spec/issues/107
> >
> >
> > 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.
> >
> > Thanks,
> > Roman
> > _______________________________________________
> > Suit mailing list
> > [e-mail]
> > https://www.ietf.org/mailman/listinfo/suit
> 
> _______________________________________________
> Suit mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/suit