Re: [sfc] Roman Danyliw's Discuss on draft-ietf-sfc-nsh-integrity-06: (with DISCUSS and COMMENT)

[tirumal reddy <[e-mail]>]

Hi Roman,

Please see inline

On Wed, 14 Jul 2021 at 00:33, Roman Danyliw via Datatracker <
[e-mail]> wrote:

> Roman Danyliw has entered the following ballot position for
> draft-ietf-sfc-nsh-integrity-06: Discuss
>
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
>
>
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-sfc-nsh-integrity/
>
>
>
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
>
> ** Section 4.6.  This section explains that an upper NSH can be
> encapsulated in
> a lower NSH, and that “the Upper-NSH information is carried along the
> lower-level chain without modification.”  I read this to mean that the
> upper
> and lower NSH can independently be protected with different keys.  The text
> even helpfully points out that “Keying material used at the upper-level
> domain
> SHOULD NOT be the same as the one used by a lower-level domain.”  Such a
> construct suggests that there are multiple MAC/Encrypted Metadata context
> headers, one for the upper and another for the lower.  However, Section 7.1
> later notes that “Only one instance of "MAC and Encrypted Metadata" Context
> Header (Section 5) is allowed.”  This seems like conflict.  What am I
> missing?
>

It means only one instance "MAC and Encrypted Metadata" Context Header
(Section 5) is allowed in an NSH-level (one instance in the Upper-NSH and
another in the Lower-NSH), we will update text for better clarity.


>
> ** Section 7.2.  On computing the HMAC in an integrity only situation:
>
> -- This section defines the MAC as “T = HMAC-SHA-256-128(MAC_KEY, A)”.
> Previously, A was defined as the Additional Authenticated Data (per Section
> 4.2).  Since this isn’t the AEAD use case, there is no A. It seems that
> this
> should be something closer to: “T = HMAC-SHA-256-128(MAC_KEY, <part of the
> packet you want to hash>)”.
>

Good catch, will update to "T = HMAC-SHA-256-128(MAC_KEY, target NSH data)"


>
> -- The text would benefit from a description on how to serialize the
> packet for
> hashing.  For example, Figure 6 and 7 are helpful logical descriptions of
> the
> integrity scope.  However, the MAC field itself is depicted as part of the
> what
> should get hashed.  Should that field be zeroed out? Removed ?
>

Yes, we will update text as follows:

The NSH imposer sets the MAC field to zero and then computes the message
integrity for the target NSH data (depending on the integrity protection
scope discussed in Section 5) using MAC_KEY and HMAC algorithm. It inserts
the computed digest in the MAC field in the "MAC and Encrypted Metadata"
Context Header.


>
> ** Section 9.
>    The attacks discussed in [I-D.nguyen-sfc-security-architecture] are
>    handled owing to the solution specified in this document, except for
>    attacks dropping packets.
>
> The above reference highlights the following attackers – “There are many
> types
> of compromised switches attack: packet dropping, packet duplicating, packet
> manipulating, incorrect forwarding,  eavesdropping, weight adjusting,
> man-in-the-middle, state-spoofing, control-channel hijacking, etc.”  Per
> the
> security services in this document, it doesn’t seem like all are mitigated
> by
> this draft as described above:
>
> -- packet dropping = noted as not being handled
>
> -- packet manipulating, eavesdropping, weight adjusting, man-in the-middle,
> state-spoofing, and control-channel hijacking = appear to be handled if
> both
> security services are applied
>

Yes.


>
> -- packet duplicating = this draft doesn’t not provide a standardized
> approach
> for mitigating this issue
>

Replay attack is detected using timestamp and recording a unique
value within the delta window derived from the NSH data and packet (see
https://datatracker.ietf.org/doc/html/draft-ietf-sfc-nsh-integrity#section-7.4
).


>
> -- incorrect forwarding = doesn’t appear to be mitigated.
>

If the context headers are encrypted, an attacker receiving forwarded NSH
from an compromised switch will not be able to decrypt the context headers.


>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> ** Section 1.
>    Thus, the NSH
>    does not have to rely upon an underlying transport encapsulation for
>    security and confidentiality.
>
> Isn’t confidentiality just a subset of the provided security properties,
> or is
> something else intended?  Recommend s/for security and confidentiality/for
> security/
>

Yes, I will update the text.


>
> ** Section 2.  Per “Key Identifier:  Is used to identify and deliver keys
> to
> authorized entities.  See, for example, 'kid' usage in [RFC7635]”. How
> does a
> key identifier “delivery keys”?  Doesn’t another protocol provide the
> delivery
> using the kid as an identifier?
>

Yes, kid is typically used as an identifier by a KMS protocol.


>
> ** Section 7.2.  How does one serialize the packet data to construct A?
> Figure
> 7 is the closest guidance but it includes the MAC itself.
>

The MAC field is set to zero before computing the message integrity, we
will update the text.


>
> ** Section 7.4.
>    … legitimate duplicate packets will be
>    tagged by NSH-aware nodes involved in that segment as replay packets
>    unless sufficient entropy is added to the duplicate packet.
>
> What is the prescribed mechanism to add entropy?


Sequence numbers can be used as discussed in the note in Section 7.4


>
> ** Section 7.5
>    When an SFC data plane element receives an NSH packet, it MUST first
>    ensure that a "MAC and Encrypted Metadata" Context Header is
>    included.
>
> I was under the impression that the security protections specified in this
> document were optional in the overall SFC architecture.  Is there some
> caveat
> to add to this section to reminder readers?  Perhaps,  “When an SFC data
> plan
> element conforms to this specification, it MUST first …”
>

Yes, we will update.


>
> ** Section 7.5 and 7.6.  What is the error processing for receiving a
> packet
> with an unknown key identifier?
>

An error should be logged, we will update the draft.


>
> ** Section 9. Section 9.  Can you further clarify how is the reader
> supposed to
> interpret the guidance in RFC4107 which frames its scope as guidelines as
> to be
> “... use[d] by IETF working groups and protocol authors who are determining
> whether to mandate automated key    management and whether manual key
> management is acceptable.  Informed judgment is needed.”  Does this
> document
> take a position on automated vs. manual key management?
>
> ** Section 9.  Per “The secret key (K) must have an expiration time
> assigned …”:
>
> (I suspect many of these issues are out of scope …)
>
> -- who assigns that expiration time (the key management system)?
>

Yes.


>
> -- who knows about that expiration time, is it both the key management
> system
> and the participating nodes?

If the nodes know the expiration time, should
> they refuse to decrypt a message with an expired key?  This check of
> expiration
> is not the Section 7.5 or 7.6 procedures.
>

Yes, a KMS can assign keys for multiple key intervals.


> ** Section 9.  In the spirit of inclusive language, please s/A
> man-in-the-middle/An on-path-attacker/
>

Okay.


>
> ** Section 9.
>    In an SFC-enabled domain where the above attacks are possible, (1)
>    NSH data MUST be integrity-protected and replay-protected, and (2)
>    privacy-sensitive NSH metadata MUST be encrypted for confidentiality
>    preservation purposes.
>
> Related to the COMMENTs raised by Lars and Alvaro.
>
> It’s my understanding that the security protections here are optional for
> the
> SFC architecture – is this correct?  If so, I don’t follow the above
> guidance.
> With the exception of “data carried in the context headers having privacy
> sensitive information”, all SFC-enabled domains have the potential for the
> above described attacks.  It seems like this document is helpfully making
> integrity, replay and encrypted meta-data a requirement for the entire SFC
> architecture (without explicitly updating RFC8300)
>
> ** Section 9.1
>    An active attacker can potentially modify the Base header (e.g.,
>    decrement the TTL so the next SFF in the SFP discards the NSH
>    packet).  In the meantime, an active attacker can also drop NSH
>    packets.
>
> The phrase “In the meantime …” suggests that these two active attackers are
> coordinating.  Both of these sentences seem to be describing independent
> attackers.
>

Yes, I will remove "In the meantime".


>
> ==[ Editorial
> ** Abstract.  Typo.  This sentence doesn’t parse.
> OLD
> Also,
>    this specification allows to encrypt sensitive metadata that is
>    carried in the NSH.
>
> NEW
> Also, this specification allows for the encryption of sensitive metadata
> that
> is carried in the NSH.
>
> ** Section 1.  Editorial. s/to is an information/to is information/
>
> ** Section 1.  Editorial.  s/This specification fills that gap/This
> specification fills that gap for SFC/
>
> ** Section 1.  Editorial.  s/This specification limits thus access/This
> specification limits access/
>
> ** Section 3.  Editorial.  s/The NSH allows to share/The NSH shares/
>
> ** Section 4.1.2.  Editorial.  s/SFFs are not thus provided/SFFs are not
> provided/
>
> ** Section 4.2. Editorial.  Recommend being clearer on the section – s/The
> authenticated encryption algorithm defined in [RFC7518]/ The authenticated
> encryption algorithm defined in Section 5.2 of [RFC7518]/; or you could
> name
> the algorithm directly with the reference.
>
> ** Section 4.4. Editorial. s/Doing so allows to address the problem/Doing
> so
> addresses the problem/
>

Thanks, we will fix all the above nits in the next revision.

Cheers,
-Tiru