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

[Benjamin Kaduk via Datatracker <[e-mail]>]

Benjamin Kaduk 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:
----------------------------------------------------------------------

(0) (I came to this realization rather late in my review process, so
there may be places where the COMMENT and this discuss point are in
disagreement; this DISCUS takes precedence.)
I fear that the construction that separately distributes ENC_KEY and
MAC_KEY in an attempt to achieve privilege separation is fatally flawed.
In particular, the CBC encryption mode is a malleable encryption mode,
in that flipping a bit of ciphertext will filp the corresponding bit of
the next block of recovered plaintext (at the cost of completely
garbling the recovered block containing the bit that was modified).
Subsequent blocks are unaffected.  Typically we combine CBC mode with a
MAC such as HMAC in order to prevent such modifications from being
exposed as attack vectors, and while we do use HMAC here for that
purpose, we also introduce a separate class of actors that have access
to the HMAC key but not the encryption key.  Accordingly, those actors
can produce a new, valid, integrity tag after making a modification to
the ciphertext, allowing them to engage in attacks that make use of
ciphertext malleability.  Ciphertext malleability is particularly useful
as an attack vector when the structure of the plaintext being encrypted
is known, and there are portions of the plaintext that the application
will either ignore if they are garbled or are expected to be near random
in the normal case (and thus for which garbled output does not cause
rejection by the application).  In a SFC environment it seems highly
likely that the structure of the plaintext will be known or guessable,
and we don't have any real mechanisms to control what types of metadata
go into encrypted context headers, so it seems that we must act as if we
are exposed to this risk.

While §4.3 does have a note that use of GCM with HMAC is undesirable due
to the additional authentication tag, it may be unavoidable in order to
provide the properties that we need.

(1) Section 5.1 describes the MAC as:

   Message Authentication Code:   Covers the entire NSH data, excluding
      the Base header.  The Additional Authenticated Data (defined in
      [RFC7518]) MUST be the Service Path header, the unencrypted
      Context headers, and the inner packet on which the NSH is imposed.

This description seems to exclude from the MAC most of the MAC context
header itself (if we go by the corresponding figure), which is very bad
for security.  We definitely need to include under the MAC the MAC
context header bits from metadata class through and including at least
timestamp, and I think IV length as well.  (The IV itself would be
incorporated via the ciphertext, since the IV is an input to encryption,
but since the IV length field indicates whether or not encryption was
performed, we'd need to protect that information.)

Similarly, Section 5.2 has the description:

   Message Authentication Code:  Coves the entire NSH data.  The
      Additional Authenticated Data (defined in [RFC7518]) MUST be the
      entire NSH data (i.e., including the Base Header) excluding the
      Context Headers to be encrypted.

which on the face of it includes the field that holds the MAC itself
(and is not yet populated), i.e., is self-referential.

I think we need to be much more precise about the construction of the
AAD in both cases.  It's possible that the HMAC construction for the
no-encrypted-context-headers case can inherit a definition from the AAD
description, but if not we'll need to have some more precision there as
well.

(2) In order for the MAC-only construction in §7.2 to be compatible with
the AEAD integrity tag construction, we would need to include the 64-bit
AL after A.  While HMAC is intrinsically immune to length-extension
attacks, I think that having the explicit AL is useful to avoid any risk
of malleability, since the same MAC_KEY is used for constructing both
types of MACs.

(3) Section 5.1 describes the Timestamp field as an "unsigned 64-bit
integer value", which is inconsistent with the actual format given in
Section 6.

(4) Section 7.5 directs the verifier to check if "the value of the newly
generated digest is identical to the one enclosed in the NSH".  It is
critical for the security of the system that this comparison be done in
a constant-time manner that does not provide a side channel into whether
the generated digest and the value in the NSH share a common substring.

(5) Do the MAC context headers always have to be the last metadata
entries in the packet (to simplify the cryptographic calculations)?
Certainly the diagrams only show "unencrypted context headers" appearing
prior to the MAC context header, so if we expect unencrypted context
headers to appear after the MAC context header as well, we should be
clear about that both in the figures and in the specification for how to
prepare the AAD.


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Section 3

   o  Both encrypted and unencrypted Context Headers MAY be included in
      the same NSH.  That is, some Context Headers may be protected
      while others do not need to be protected.

In the security area we're pretty strongly moved towards a stance of
"encrypt everything that doesn't have a strong need to be plaintext
(since we don't want to assume that we can predict all threats and
attacks in advance), and QUIC has even taken that to new levels.  It's
quite surprising to see "don't need to be protected" as a justification
for lack of encryption, rather than "need to be unprotected".  Given
that we're going to be solving key distribution for integrity protection
anyway, it remains unclear to me why we wouldn't just encrypt
everything.

Section 4.1.1

Is the column heading for the middle column of Table 1 referring to the
Base Header and "Service Path Header"?  I'm not sure how to interpret
just "Service Header" other than the entire NSH, which doesn't seem
right...

   The Service Path Header (Section 2 of [RFC8300]) is not encrypted
   because SFFs use Service Index (SI) in conjunction with Service Path
   Identifier (SPI) for determining the next SF in the path.

It's good to see this spelled out like this; I'd love to have this kind
of explanation for every un-encrypted protocol element.

Section 4.1.2

   The solution provides integrity protection for the NSH data.  Two
   levels of assurance (LoAs) are supported.

I see (reading onward) that the two LoAs just differ in what is covered
by the integrity tag, but each involve only a single MAC.  There's a
reasonable argument that when there are overlapping needs for what data
needs protecting and who needs access to it, that multiple MACs (or
encryption, if needed) are most appropriate, which can be tailored to
the respective needs.  This is something that Phillip Hallam-Baker has
ascribed as the "short-blanket theory" of cryptography, in that it's
like a blanket that's too short to cover everything you want, so you
need to have more than one and put them in partially overlapping layers.
Perhaps this "short-blanket theory" is more applicable to Discuss point
(0) than what is covered by the MAC, though...

   The first level of assurance where all NSH data except the Base
   Header are integrity protected (Figure 2).  In this case, the NSH
   imposer may be a Classifier, an NSH-aware SF, or an SFC Proxy.  SFFs
   are not thus provided with authentication material.  [...]

There seems to be a gap in the chain of reasoning as to why SFFs are not
provided with authentication material.  It seems that the intent is
something along the lines of considering which entities need access to
the key material in order to successfully add an NSH or update context
headers, but that alone is not itself cause to say that other entities
can never be provided with the keys (subject to the security
considerations of group-shared PSKs, of course).  Furthermore, if a
particular SFF is trusted, it may have grounds to verify the integrity
tag even if it is administratively prohibited from writing such a tag.

   In both levels of assurance, the unencrypted Context Headers and the
   packet on which the NSH is imposed are subject to integrity
   protection.

Surely any encrypted Context Headers are also given integrity
protection, as excluding them from the MAC computation would be quite
complex.  I see that the encrypted context headers are not part of the
AAD input, but in the AEAD model the ciphertext is still given integrity
protection!

Section 4.2

   The authenticated encryption algorithm defined in [RFC7518] is used
   to provide NSH data integrity and to encrypt the Context Headers that
   carry privacy-sensitive metadata.

RFC 7518 (JSON Web Algorithms) is a very surprising reference for
generic AEAD encryption.  Continuing on, it becomes clear that we in
fact do not want a generic AEAD operation here, but instead have need
for separation of encryption and integrity protection due to the various
separations of roles that are possible.  Because this is a divergence
from current best practices (use of native AEAD cipher modes like GCM,
SIV, etc.), I might suggest a rather different introductory material:

% The AEAD interface specified in RFC 5116 is a very powerful tool for
% providing strong confidentiality and integrity protection for protocol
% messages, and has become quite widely adopted as a result.  For
% protecting NSH data we adhere to the spirit of the AEAD interface but
% use a slightly modified version in order to remain consistent with the
% privilege separation needed (e.g., in the second level of assurance,
% SFFs need to be able to update the message integrity tag but do not
% need access to the decrypted plaintext).  Accordingly, a compound AEAD
% mechanism combining CBC encryption and HMAC integrity tag is used, so
% that distribution of the encryption key can be limited.  Rather than
% define a new scheme from scratch, the AES_CBC_HMAC_SHA2 scheme from
% [RFC7518] is used.  In order to simplify key distribution, a single
% secret key material seed K is used, which can be kept private
% centrally or distributed to all parties that will have access to
% decrypted plaintext, and the secondary keys MAC_KEY and ENC_KEY are
% derived from K as discussed in Section 5.2.2.1 of [RFC7518].  Entities
% that need to update the integrity tag but are not authorized to see
% the decrypted plaintext will only receive MAC_KEY and not K or
% ENC_KEY.

   o  If the Context Headers are not encrypted, the Hashed Message
      Authentication Mode (HMAC) algorithm discussed in [RFC4868] is
      used to protect the integrity of the NSH data.

It's not clear to me that this is adding much value; HMAC is used
whether or not there are encrypted headers, and the RFC 4868 reference
is rather odd (see also my Discuss point (2)).

   The advantage of using the authenticated encryption algorithm is that
   NSH-aware SFs and SFC proxies only need to re-compute the message
   integrity of the NSH data after decrementing the Service Index (SI)
   and do not have to re-compute the ciphertext.  The other advantage is
   that SFFs do not have access to the ENC_KEY and cannot act on the
   encrypted Context Headers and, only in case of the second level of
   assurance, SFFs do have access to the MAC_KEY.  Similarly, an NSH-
   aware SF or SFC Proxy not allowed to decrypt the Context Headers will
   not have access to the ENC_KEY.

[If my above suggestion is used, this text has probably become mostly
redundant and could be trimmed down or removed.]

   The authenticated encryption process takes as input four-octet
   strings: a secret key (K), a plaintext (P), Additional Authenticated
   Data (A) (which contains the data to be authenticated, but not
   encrypted), and an Initialization Vector (IV).  The ciphertext value
   (E) and the Authentication Tag value (T) are provided as outputs.

   In order to decrypt and verify, the cipher takes as input K, IV, A,
   T, and E.  The output is either the plaintext or an error indicating
   that the decryption failed as described in Section 5.2.2.2 of
   [RFC7518].

This seems true for the abstract AEAD interface, but since we're peeling
off the HMAC integrity tag for standalone use, we may need to say
something about what inputs/outputs are needed for just using the
integrity tag without doing decryption.  In particular, we cannot use K
as an input for standalone HMAC verification!

Also, this is just the generic AEAD interface, so RFC 5116 is probably a
more apropos reference than RFC 7518.

Section 4.3

      Note: The use of Advanced Encryption Standard (AES) in Galois/
      Counter Mode (GCM) + HMAC may have CPU and packet size
      implications (need for a second 128-bit authentication tag).

[This might also be redundant if my earlier proposal is accepted.]

Section 4.4

   The document does not assume nor preclude the following:

   o  The same keying material is used for all the service functions
      used within an SFC-enabled domain.

   o  Distinct keying material is used per SFP by all involved SFC data
      path elements.

   o  Per-tenant keys are used.

I suggest a forward reference to the security considerations with a
brief note that the risk involved in using a group-shared symmetric key
increases with the size of the group to which it is shared.

   In order to accommodate deployments relying upon keying material per
   SFC/SFP and also the need to update keys after encrypting NSH data
   for a certain amount of time, this document uses key identifiers to
   unambiguously identify the appropriate keying material.  Doing so
   allows to address the problem of synchronization of keying material.

I'd suggest making a statement about the scope in which key identifiers
need to be unique.

Section 4.5

   An NSH-aware SF or SFC Proxy that needs to decrypt some Context
   Headers uses ENC_Key and the decryption algorithm for the key
   identifier being carried in the NSH.

I think we need to say that decryption MUST NOT proceed unless the
integrity tag has been successfully validated.  To decrypt without
checking the integrity tag violates the AEAD interface.  Alternately, we
could only ever describe decryption as the AEAD operation that uses K as
input (not just ENC_KEY) -- §7.6 seems to attempt to take this latter
approach, but the document as a whole is not consistent about it.

Section 5, 5.x

I strongly suggest changing "Key Length" to "keyid length", since there
is already convention on "key length" (e.g., for AES-128 vs AES-192 vs
AES-256).

Section 5.1

   Key Identifier:  Carries a variable-length Key Identifier object used
      to identify and deliver keys to SFC data plane elements.  This
      identifier is helpful to accommodate deployments relying upon
      keying material per SFC/SFP.  The key identifier helps in
      resolving the problem of synchronization of keying material.

I suggest clarifying that a single key identifier is used to lookup both
the associated ENC_KEY and the associated MAC_KEY (or, alternately, that
it is logically bound to K and remind the reader that ENC_KEY and
MAC_KEY are extracted from K and might be separately distributed).

   Key Length:  Variable.  Carries the length of the key identifier.
   [...]
   IV Length:  Carries the length of the IV (Section 5.2 of [RFC7518]).
      If encryption is not used, IV length is set to zero (that is, no
      "Initialization Vector" is included).

Please also specify that the Key/IV lengths are measured in bytes (the
reference for IV length, at least, talks about IVs in bits).

I strongly suggest using a value other than zero as the sentinel value
for "no encryption" -- there already exist modes like AES-SIV (RFC 5297)
that do not require an IV input, which would be a natural use for
zero-length IV.

Additionally, one might imagine using a non-zero-length IV as input to
an HMAC calculation as a per-message nonce.  On cursory examination,
since we are already using timestamp-based replay detection, the
additional nonce is unlikely to add much value, but I'm not willing to
completely rule it out on the basis of the minimal analysis I performed.

Section 7.1

   Only one instance of "MAC and Encrypted Metadata" Context Header
   (Section 5) is allowed.  If multiple instances of "MAC and Encrypted
   Metadata" Context Header are included in an NSH packet, the SFC data

I think Roman may have mentioned this already, but please clarify if
this is one instance per NSH or per packet (e.g., in hierarchical SFC we
might have more than one NSH in a packet).

Please also clarify it if is permitted to have both MAC#1 and MAC#2
present in the same packet.  (IIUC there are not obvious situations
where such a setup would be useful, but it's still good to be clear
about expectations.)

Section 7.2

   If the Context Headers are not encrypted, the HMAC algorithm
   discussed in [RFC4868] is used to integrity protect the target NSH
   data.  [...]

It's not really clear to me why RFC 4868 is the right reference here, as
opposed to (e.g.) RFC 2104.  In particular, see my Discuss point (2).

   The Message Authentication Code (T) computation process can be
   illustrated as follows:

         T = HMAC-SHA-256-128(MAC_KEY, A)

I agree with the other reviewer that wondered about just using "MAC" (or
maybe "HMAC") rather than "HMAC-SHA-256-128" here (I do see the "can be
illustrated as" qualifier).

It's also a little weird to reuse T and A from the AEAD construction (or
RFC 4868) here without additional discussion on how they apply and how
the value of A is constructed.

   An entity in the SFP that intends to update the NSH MUST follow the
   above behavior to maintain message integrity of the NSH for
   subsequent validations.

I'm not sure that "intends to" is needed; the requirement applies to
actions, not intent.

Section 7.3

   In an SFC-enabled domain where pervasive monitoring [RFC7258] is
   possible, all Context Headers carrying privacy-sensitive metadata
   MUST be encrypted; [...]

I agree with the other reviewer that noted that this seems to
effectively be an unqualified "MUST", since there is always some
possibility of monitoring.

         MAC_KEY = initial MAC_KEY_LEN octets of K,
         ENC_KEY = final ENC_KEY_LEN octets of K,
         E = CBC-PKCS7-ENC(ENC_KEY, P),
         M = MAC(MAC_KEY, A || IV || E || AL),
         T = initial T_LEN octets of M.
         MAC and Encrypted Metadata = E || T

I recognize that this is just copy+paste from RFC 7518 (with the
addition of the last line), but it seems a glaring omission that the
CBC-PKCS7-ENC() invocation does not include the IV as an input!
(In §5.2.2.1 of RFC 7518 we do see that "The plaintext is CBC encrypted
using PKCS #7 padding using ENC_KEY as the key and the IV", so it's just
the figure/formula that omits the IV.)

We also don't define or mention T_LEN in this document other than in
this formula and the analogous one for MAC-without-encryption, which
might be worth remedying.

Section 7.4

I suggest adding another word (like "Prevention") to the section title.

   The received NSH is accepted by an NSH-aware node if the Timestamp
   (TS) in the NSH is recent enough to the reception time of the NSH
   (TSrt).  The following formula is used for this check:

             -Delta < (TSrt - TS) < +Delta

(There's no real reason why the accepted timestamp skew parameter Delta
needs to be symmetric in the positive and negative directions.)

   Replay attacks within the Delta window may be detected by an NSH-
   aware node by recording a unique value derived, for example, from the
   NSH data and Original packet (e.g., using SHA2).  [...]

Note that the unique value MUST NOT use as input any bits that are not
covered by the MAC.  Indeed, just using the MAC itself as the unique
value would be a fairly straightforward scheme, I think.

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.  [...]

This sort of requirement (as written) would seem to support the position
of some other reviewers that this document would need to formally update
RFC 8300.

Section 9

We might discuss that this mechanism is only for metadata type 2 and not
type 1 (as well as that with the relatively small fixed length of type 1
metadata, it's quite challenging to provide useful cryptographic
protection), so that type-1 metadata still has no generic protection
mechanism.  A given metadata element could define its own protection
scheme, of course.

   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.  [...]

>From a very quick read, draft-nguyen-sfc-security-architecture proposes
a probe-based scheme that monitors all middleboxes (or at least those in
the expected path) to detect a middlebox-bypass attack and verify that
the probe traversed the expected SFP.  I am not sure that this document
on its own suffices to prevent the middlebox-bypass attack for all
traffic (i.e., the probe-based scheme might still be needed).  If some
network entities know what the expected service index will be for a
given SPI at a given node, then middlebox-bypass attacks could be
natively detected, but my understanding was that such advance knowledge
of expected service index was not going to be available.

   The solution specified in this document does not provide data origin
   authentication.

I would have expected more in-depth discussion of the security
considerations for a group-shared PSK scheme, including how the risk of
key loss/compromise goes up when the key is distributed to more
locations.

Section 9.1

   No device other than the NSH-aware SFs in the SFC-enabled domain
   should be able to update the integrity protected NSH data.

What about classifiers and SFC proxies?

Section 9.2

I expected to see some remarks here analogous to the second paragraph of
§9.1 (even if just by reference to that section).

Section 9.3

   Section 5.6 of [RFC8633] describes best current practices to be
   considered in deployments where SFC data plane elements use NTP for
   time synchronization purposes.

The referenced section covers NTP symmetric mode only.  Is client/server
mode not a BCP for trusted networks and broadcast/symmetric the only
options?

NITS

Section 1

   network infrastructures against them, network slicing, etc.  Because
   of the proliferation of such advanced SFs together with complex
   service deployment constraints that demand more agile service
   delivery procedures, operators need to rationalize their service
   delivery logics and master their complexity while optimising service
   activation time cycles.  The overall problem space is described in
   [RFC7498].

The "their" in "master their complexity" is a bit ambiguous between
"operators" and "service delivery logics" (noting that the previous use
of "their" did refer to "operators").  (The sentence as a whole reads a
bit like marketing language, but that's not intended to be an actionable
comment.)

   This specification fills that gap.  Concretely, this document adds
   integrity protection and optional encryption of sensitive metadata
   directly to the NSH (Section 4); integrity protects the packet
   payload and provides replay protection (Section 7.4).  [...]

The clause after the semicolon needs to be able to stand alone as a
complete sentence in order to be grammatical, but it currently has no
subject.  Maybe prefixing "it also" would be enough to fix it.

Section 4.1.2

   The integrity protection scope is explicitly signaled to NSH-aware
   SFs and SFC proxies in the NSH by means of a dedicated MD Type
   (Section 5).

Signaled to SFFs as well, since in the second LoA they can use it.

Section 4.6

   As discussed in [RFC8459], an SFC-enabled domain (called, upper-level
   domain) may be decomposed into many sub-domains (called, lower-level
   domains).  In order to avoid maintaining state to restore back upper-
   lower NSH information at the boundaries of lower-level domains, two

I think s/upper-lower/upper-layer/ (across the line break)?

Section 5.1

   MAC#1 Context Header is a variable-length Context Header that carries
   the Message Authentication Code (MAC) for the Service Path Header,
   Context Headers, and the inner packet on which NSH is imposed,
   calculated using MAC_KEY and optionally Context Headers encrypted
   using ENC_KEY.  [...]

Comma after "calculated using MAC_KEY", please.  ("optionally Context
Headers [...]" are not used along with MAC_KEY in the calculation)

Section 5.2

   Message Authentication Code:  Coves the entire NSH data.  The

s/Coves/Covers/

Section 6

      The epoch is 1970-01-01T00:00Z in UTC time.  [...]

(IIUC, the "in UTC time" is redundant with the trailing 'Z'.)

Section 7.1

   element.  The details of sending notification alarms (i.e., the
   parameters affecting the transmission of the notification alarms
   depend on the information in the context header such as frequency,
   thresholds, and content in the alarm) SHOULD be configurable.

The grammar in the parenthetical seems off to me.  Possibly
s/depend/depending/ would be a fix, but I'm not sure.

   NSH-aware SFs and SFC proxies MUST re-apply the integrity protection
   if any modification is made to the Context Headers (strip a Context
   Header, update the content of an existing Context Header, insert a
   new Context Header).

I think maybe an "e.g.," in the parenthetical is in order, since this
doesn't seem to include things like "decrement the TTL".

Section 9

   The interaction between the SFC data plane elements and a key
   management system MUST NOT be transmitted in clear since this would
   completely destroy the security benefits of the integrity protection
   solution defined in this document.  The secret key (K) must have an
   expiration time assigned as the latest point in time before which the

This first sentence seems like it could probably be a paragraph of its
own; the subsequent discussion of expiration time seems to be mostly
unrelated.

We might also add another sentence to reiterate that "the security and
integrity of the key-distribution mechanism is vital to the security of
the system as a whole".

   o  Attacker replacing the packet on which the NSH is imposed with a
      bogus packet.

I'd probably say "modified or bogus".

   In an SFC-enabled domain where the above attacks are possible, (1)

This is perhaps ambiguous about whether an "any" or "all" criterion
should be used.  Perhaps that's desired :)