[ippm] Re: Secdir early review of draft-ietf-ippm-ioam-data-integrity-07

[Benjamin Kaduk <[e-mail]>]

Hi Justin,

Thank you for your thoughtful replies; my apologies for failing to see and
respond to them for so long.  I reply now, after having reviewed the -12,
so in some places I will essentially defer to the newer review content.

Also inline (and trimming where there is nothing more to say) ...

On Fri, Jan 26, 2024 at 04:57:18PM +0100, Justin Iurman wrote:
> Hi Ben,
> 
> Thank you very much for this awesome and huge review.
> 
> Please see inline ([JI] tag).
> 
> On 12/22/23 00:09, Benjamin Kaduk via Datatracker wrote:
> > Reviewer: Benjamin Kaduk
> > Review result: Serious Issues
> > 
> > # secdir review of draft-ietf-ippm-ioam-data-integrity-06
> > CC kaduk
> > 
> > I have reviewed this document as part of the security directorate's
> > ongoing effort to review all IETF documents being processed by the
> > IESG. These comments were written primarily for the benefit of the
> > security area directors. Document editors and WG chairs should treat
> > these comments just like any other last call comments.
> > 
> > The summary of the review is that this is still early-stage work and we haven't
> > landed on the right cryptographic mechanisms yet -- what's currently in place
> > has some issues that would be very significant issues for some deployment
> > sites/scenarios, and there are some aspects that are not fully specified yet. A
> > lot of my comments cover topics that will merit mention in the security
> > considerations section, though I did not attempt to call each one out as such
> > or produce an exhaustive list at the end.
> > 
> > ## Discuss
> > 
> > ### Signature vs MAC
> > 
> > I'm pretty uncomfortable about using the word "signature" to describe the
> > protection that we're applying here.  The current text in §5 reads as if the
> > need for a symmetric-key mechanism is an intrinsic requirement of the mechanism
> > (which seems reasonable given the use-case -- true asymmetric signatures would
> > be too computationally expensive to be suitable here), but in that case it
> > seems that we are really expecting a MAC (message authentication code) rather
> > than a signature (that provides both data authentication and source
> > authentication).  In particular, we cite NIST.800-38D for the AES-256-GCM
> > "signature algorithm", when I think we are really using GMAC -- the NIST
> > document talks about GMAC a lot and does not use the word "signature" anywhere,
> > which seems particularly illustrative to me.
> 
> [JI] Good point, it is indeed GMAC and "signature" is quite ambiguous. That
> said, this document not only defines a new Integrity Protection Method, it
> also defines new Option-Types and a generic header (the generic part of the
> document). We therefore propose to replace "signature" (and similar) with
> "Integrity Check Value" as the new generic term. The reason we want to keep
> a generic term is for future Integrity Protection Methods (who knows what
> they will use). Note that section 5 defining the new Integrity Protection
> Method would of course mention GMAC when going into details.

Thank you; I like this approach.

> > ### Signature as nonce for transit nodes
> > 
> > The specification of the actual cryptographic protection algorithm in §5
> > includes a provision (step 2) for a transit node to compute a new signature
> > that accounts for the additions it has made to the IOAM data.  It seems to be
> > saying that the corresponding cryptographic computation involves using the
> > received signature value as the nonce input for producing this new signature.
> > While SP800-38D does seem to permit using multiple IV lengths with a given key,
> > using the received signature as a nonce seems to set us up for using nonces of
> > length other than 96 bits, which (per {{GCM-Key-usage-limitations}}) strongly
> > limits how much traffic we can send in a given key.  It also would entail using
> > data received from the network as the nonce for a given node's private key,
> > which seems like it would make it easy for an attacker to induce (key,nonce)
> 
> [JI] Correct, see my reply under "GCM Key usage limitations" below.
> 
> > reuse.  (This might be mitigated if the transit nodes diligently validate the
> > received signature prior to using it as a nonce, but even that would not
> > obivate the need for a fully reliable replay detection mechanism for the
> > lifetime of the key, which seems prohibitively expensive.) This scheme also
> > seems to make validation quite complicated and expensive, since in order to
> > verify the final received signature we'd need to reconstruct the whole chain of
> > signature from initial encapsulation through all transit nodes.  While being
> > forced to validate all the intermediate signatures does provide a fairly strong
> > indication of non-tampering along the path, it's also a lot of recomputation. A
> > scheme where the transit nodes that regenerate the signature also generate a
> > new nonce that goes in the packet would be simpler/faster, at the cost of
> > trustng all the transit nodes to properly validate the received signature and
> > to be operating correctly.  This is a trade-off, and could be decided either
> > way, but if we opt to go for the stronger validation we should specifically say
> > that we made the choice to do that and accept the costlier validation and
> > replay protection.
> 
> [JI] Unfortunately, the trade-off you mention would defeat the purpose of
> this document (assumption: don't trust nodes), even though we totally agree
> on the fact it would be simpler/faster. We will add some text to
> specifically say why we chose to go that way.
> 
> [JI] OTOH, since we now require a 96-bit nonce based on a deterministic
> construction (see my reply for "GCM Key usage limitations" below), we cannot
> ask for a transit node to use the previous "signature" (i.e., GMAC tag in
> this context) as its nonce anymore. But we still need to kind of include the
> previous tag in the process, so that we can make sure we validate the whole
> chain, each node depending on previous ones on the path. What we suggest is
> therefore to also pass the previous tag to the AAD. Thoughts on this?

"Chaining" the ICV in this way (using the received ICV as a part of the AAD
input for the subsequent ICV) is safe and effective.  The only potential
concern comes in the form of needing to have the encoding rules for
constructing the AAD be "injective" such that you can unambiguously
identify which bytes were the received ICV, which bytes are field X, which
bytes are field Y, etc.  Since the ICV is fixed-length in this scheme,
that's not a concern.

> > ### Bit-level hash inputs (Protected flags for trace and DEX option-types)
> > 
> > I think we need to provide more clear guidance on how to handle the protected
> > flags for the trace and DEX option-types.  First, a note in passing that
> > interoperability does require locking in which flag bits are (not) covered by
> > the MAC at the time the protected option type is specified (i.e., now), which
> > blocks off future extensions since any new flag bits cannot be integrity
> > protected using this mechanism.  That said, it would be possible to divide the
> > unallocated flags range into a protected and unprotected portion, to leave a
> 
> [JI] Unfortunately, this is indeed a well known problem we chose to "ignore"
> as a trade-off. Of course, your proposition makes sense but sounds
> complicated. It would require to update both RFC 9197 and RFC 9326 (or are
> you thinking of having a centralized IANA registry for IOAM integrity
> fields?). Regardless, it seems tricky to divide flags into protected and
> unprotected portions. For example, the 4-bit flags from the Trace
> Option-Type has only one free bit left, namely bit 3 (RFC 9197 defines the
> Overflow flag on bit 0, while RFC 9322 defines the Loopback flag and Active
> flag resp. on bits 1 and 2). Do we want bit 3 to be protected or not? We
> don't know, because we can't predict the future. And since it is the only
> bit left, it's 50/50. Also, while bit 0 (Overflow) MUST NOT be protected
> (because it is mutable), bits 1 and 2 SHOULD/MUST be protected. If you want
> contiguous portions, it forces bit 3 to be protected as well. What if the
> future flag defined on bit 3 MUST NOT be protected (i.e., an Overflow-like
> flag)? For all these reasons, we chose to go the current way, that is to
> "freeze" header fields and delegate the decision to a document defining a
> new flag (which of course still makes interop difficult, but at least we
> won't break implementations when new flags arrive).

I think I was proposing a centralized registry for IOAM integrity fields,
but I am not tied to that approach -- I primarily  want to make sure that
implementors have a very unambiguous picture of what to do, and secondarily
have some reasonable path for extensibility that allows us to specify both
mutable things and things that can get integrity protection.  There are a
lot of potential ways to achieve these goals.

> [JI] The other (easiest) solution would be to *not* have integrity
> protection of the IOAM header (or, for instance, only have the Namespace-ID
> which is common to all IOAM Option-Types and is very important for the
> context of IOAM data). Been there, done that. It worked very well, and was
> so much lighter for, e.g., a Trace Option-Type with the Loopback flag set.
> But it was an incomplete solution. If you don't protect the header, then DEX
> or a Trace+Loopback (for example) would trigger actions on nodes, even if
> the header was altered (e.g., Loopback flag set by a compromised node on the
> path, making each IOAM node on the path to loop truncated packets with IOAM
> data back to the sender). Or, even simpler, if a compromised node alters the
> IOAM Trace-Type field (for a Trace Option-Type): you wouldn't notice either.
> Best case would be a malformed IOAM-Data-Fields block or malformed packet,
> worst case would be a misinterpretation of IOAM data. All that to say, we
> definitely need header protection too, even if it makes things a bit more
> challenging.

Agreed on this part.  I think (but do not have any hard evidence to confirm
it) that when I reviewed the -07 I specifically went through all the header
fields defined in RFC 9197 and made my own assessment of whether they
should be covered by integrity protection (and agreed with what was in the
-07 unless otherwise noted).  This belief, in turn, feeds into my question
in the review of the -12 about why we think we can get away without any
integrity protection of the various flag bits.

> > little bit of flexibility for the future. My main concern here, though, is to
> > concretely specify, at the bit level, what the input to the MAC (or hash)
> > function is -- do we mask out the unprotected bits (if so, to 0s or 1s?), or do
> > we literally just extract the two bits in question and make the bitstream input
> > to the MAC (or hash) be not byte aligned?  I strongly suggest the former, since
> > implementation handling for non-octet inputs to hash functions is very poor,
> > but reading the current text I would conclude that I must attempt to implement
> > the latter.  (I note on re-read that SP800-38D does require the inputs to have
> > bit lengths be a multiple of 8, so if we decide to use GMAC directly rather
> > than hash-and-MAC, we would need to specify padding if we opt for the latter
> > approach.)
> 
> [JI] The intent is indeed the former, i.e., we mask out the unprotected bits
> (to 0s). We initially thought it was unnecessary to specify it in the
> document and let implementers decide. But, as you mentioned, it is indeed
> important to specify it, thanks. We will add some text.

I didn't look at all the versions between -07 and -12, so I'm not sure if
this was never added or added then removed along with protection for the
flags.  Probably best to just respond to the review of the -12 rather than
here.

> > ### GCM Key usage limitations
> > 
> > Since we're using GMAC as the integrity protection mechanism, we need to look
> > at the GCM key usage limitations to know how many times a given key can be
> > used.  Unfortunately, what SP800-32D says is quite restrictive: its §8.3 lays
> > out scenarios where the total number of invocations of the authenticated
> > encryption function cannot exceed 2**32 for a given key.  Even if we have
> > unique keys per node generating a signtaure, this limit can still be hit fairly
> > quickly at modrately high traffic rates.  To avoid that limit we'd need to
> > exclusively use 96-bit IVs that use the "determinstic construction" and in that
> > case would be limited by the need to avoid reusing "invocation field" values on
> > a given device.  Depending on what deployment scenarios we are thinking about,
> > there's a significant chance that we'll need to have the protocol be able to
> > accomodate key rotations in order to avoid the key usage limits.  This might
> > take the place of an in-protocol key identifier field, or guidance to use some
> > other protocol element (such as Namespace-ID) to select which key to use.
> 
> [JI] Based on NIST recommendations, we will now require (MUST) a 96-bit
> nonce with a deterministic construction, which one will also follow their
> recommendations, namely a 32-bit fixed field and a 64-bit invocation field
> (probably more RECOMMENDED than MUST, to be discussed). The invocation field
> could be a counter or similar, or a timestamp (as a solution for replay
> attacks). The fixed field could be completely random (despite its name), or
> a 16-bit unique ID with a 16-bit random part, or completely fixed (e.g., a
> unique ID). Those should be recommendations, we probably don't want to force
> the content of a nonce. So, the limit will no longer be 2**32, it will be
> 2**S where S is the number of bits used by the invocation field. And,
> depending on how the fixed field is built, it can even be much bigger than
> 2**64. Anyway, such a "limit" is much better than 2**32, and a key could be
> used much much longer. Do we really need key rotations? Maybe, although not
> required in that case... but it might also be fine with the deterministic
> 96-bit nonce described above anyway, right? If you agree, we will rewrite
> the current text and add some new text to reflect the above.

Agreed that we should at most give recommendations for how to build the
nonce, and state only requirements for properties that the nonce needs to
satisfy.  I do think we need to have some mention that in some scenarios
key rotation will be needed, even if we largely leave it to
implementors/deployments for how to actually achieve the key rotation.
(I think I proposed some text for this in my review of the -12.)

> > ## Comments
> > 
> > ### Threat model
> > 
> > §1 lays out the scenario as having an IOAM-Domain that's under a single
> > administrative control but invokes the possibility of data collected in
> > untrused or semi-trusted environments as a motivation for integrity protection.
> >   Is this just a risk that nodes which are supposed to be under the domain's
> > administrative control get compromised, or are we intending to consider a
> > subtler scenario with semi-trusted entities being authorized parts of the
> > administrative domain directly, or something else?
> 
> [JI] The former, to keep things simple. I.e., a compromised node in a single
> IOAM domain. Do you think we should add more text here?

I'm mostly inclined to add more text here, but do not insist.

> > ### Separation of layers
> > 
> > While it's generally reasonable (as §3 does) to require the lower layer
> > protocol to handle threats at their own layer, I would probably call out that
> > since IOAM is defined as data fields rather than a dedicated packet structure,
> > we also rely on the lower layer to provide integrity protection for whch data
> > fields (that is, IOAM Option-Types) are present in a given packet.
> 
> [JI] Will remove that part.

For clarity, I was not proposing removing any text here, but rather to add
text stating that the integrity protection scheme we define does not
provide protection for the set of Option-Types that are or are not applied
to a given packet.  But I mention this again when reviewing the -12, so
hopefully that review comment is more clear than this one was.

> > ### threat: false error injection
> > 
> > The discussion in §3 around creating a failure report for a nonexistent failure
> > mentions the potential for additional processing/export by IOAM nodes along the
> > path.  Could this be a privacy concern where the additional reported data
> > contains "sensitive" information (for some definition of "sensitive")?  I am
> > not sure if it is worth also mentioning the time of humans who get to look at
> > the false positive reports and analyze them, only to ultimately discard them as
> > bogus.
> 
> [JI] This is more a question of availability than privacy. Although
> management/exporting is out of scope, it is supposed to have its own secure
> scheme. Do you think this sentence should be clearer?

I don't have any issues with the sentence in question.  I was more trying
to consider a broader question of whether any of the information that would
go into a failure report that was prompted by an injected error (as opposed
to a real error) would be information that we really do not want the
attacker to have.  If there was any such "sensitive" information in play,
then an attacker could inject the error and look at the resulting failure
report to learn something they shouldn't learn.  Looking through the types
of things we're putting in the IOAM data-fields themselves, I don't really
think there's a concern here, but it seemed worth asking the question.

> > ### threat: removal of fields
> > 
> > We cover modification and injection already, but might have a bit of discussion
> > on what happens when the attacker just removes some or all IOAM fields.
> 
> [JI] Fields removal was implicitly considered part of fields modification.
> Basically, the consequences would be the same for both modification or
> removal, with the latter having an additional possible consequence: ending
> up with a malformed packet. Will add specific text in fields modification to
> mention and distinguish it.

Ah, as I read this again, I think I did not write very clearly.  It seems
that an attacker can remove an entire IOAM Option-Type (headers, data
fields, everything) without detection if we are limited to using the
mechanisms in this document to detect the packet modification.  I mention
this again in my review of the -12, though, so maybe we should just discuss
the topic there.

> > ### IOAM-Data-Fields modification
> > 
> > In §3.1 I might expand "false picture of the paths in the network" to cover
> > both the notion of providing false paths in the network (topology-wise) and
> > providing false data about (real or false) paths in the network.
> 
> [JI] Not sure if we can easily distinguish the two. IMO, a false
> (topology-wise) path can't be provided in the network as is. If so, it would
> come indirectly from data modification as well. You might for instance
> switch some node data, therefore representing a path with nodes in different
> positions. OTOH, you might modify some data fields directly, making a node
> appear as healthy or faulty (even if it's not), and making the current path
> to be modified if the system reacts accordingly, whether it's a real or
> false path (which can be the same consequence if you change some nodes
> position too). Proposed change:
> 
> OLD:
> 
> "[...] an attacker can create a false picture of the paths in the network,
> the existence of faulty nodes and their location, and the network
> performance."
> 
> NEW:
> 
> "[...] an attacker can create a false picture of the network status. For
> example, the attacker can hide the existence of a faulty node or make a
> healthy node appear as faulty. Despite the likely global impact on network
> performance, a direct consequence can be a change in network paths, either
> based on false node positions or false data provided by the attacker to fool
> the system that ingests IOAM-Data-Fields."

I like this change (though it does not apply cleanly to the -12 anymore).
By calling out "make a healthy node appear as faulty" that does invite the
question of whether the attacker can make a faulty node appear healthy.  I
think that, regardless of what the answer is, it's probably worth
mentioning it in the document.

> > ### Option-Type Headers scope
> > 
> > In §3.2 we talk about the implications of changing the header of IOAM
> > Option-Types, but the discussion here is intrinsically limited to the
> > option-types defined at the time of this writing; we should probably
> > acknowledge that limitation explicitly (and possibly just say that the listed
> > implications are intended to be examples rather than exhaustive, as well). We
> 
> [JI] Well, we could do so. However, it would clearly make a difference for
> future Option-Types, which is not the intent here. More like the opposite,
> actually. Modifying future Option-Types' headers is assumed to be the same
> issue as the one described here, based on existing Option-Types. What we can
> do, though, is to rewrite that part a little so that it makes it clear that
> these are examples, not an exhaustive list of possibilities. Proposed
> change:
> 
> OLD:
> 
> "Changing the header of IOAM Option-Types may have several implications. An
> attacker can maliciously increase the processing overhead in nodes that
> process IOAM-Data-Fields and increase the on-the-wire overhead of
> IOAM-Data-Fields, for example by modifying the IOAM-Trace-Type field in the
> IOAM Trace Option-Type header. An attacker can also prevent some of the
> nodes that process IOAM-Data-Fields from incorporating IOAM-Data-Fields, by
> modifying the RemainingLen field in the IOAM Trace Option-Type header."
> 
> NEW:
> 
> "Changing the header of currently defined IOAM Option-Types, just like it is
> assumed for future IOAM Option-Types, may have several implications. The
> following list of examples is not exhaustive, and is based on existing IOAM
> Option-Types. An attacker could maliciously increase the processing overhead
> in nodes that process IOAM-Data-Fields and increase the on-the-wire overhead
> of IOAM-Data-Fields, by modifying the IOAM-Trace-Type field in the IOAM
> Trace Option-Type header. An attacker could also prevent some of the nodes
> that process IOAM-Data-Fields from incorporating IOAM-Data-Fields, by
> modifying the RemainingLen field in the IOAM Trace Option-Type header.
> [...]"

This looks good to me (and, eyeballing, I think I see it in the -12
already; hooray!).

> > ### Injection defenses
> > 
> > While I agree that the impacts of injection (§3.3,3.4) are similar to
> > modification in general, it does seem that an IOAM deployment would be able to
> > protect itself from injection (but not modification) if it know a priori what
> > IOAM mechanisms were going to be in use on each flow, so that unexpected ones
> > could be rejected.  That said, this scenario may not be worth mentioning in the
> > document, since an attacker in a position to inject IOAM content into otherwise
> > valid packets would very likely also be able to modify preexisting IOAM
> > content...but an off-path attacker could inject wholly new packets with IOAM
> > content while being unable to modify existing IOAM content.
> 
> [JI] Not sure what your intention is here. Are you suggesting we should
> remove both sections 3.3 and 3.4?

I was not proposing to remove any content.
This is related to the topic above about not providing integrity protection
for the set of IOAM Option-Types in a given packet.  In particular
(following what was written in the -07), an attacker could just inject a
new unprotected IOAM Option-Type into a given packet without detection.
The -12 has added a requirement to apply integrity protection to all IOAM
Option-Types within a given Namespace-ID, which would prevent this type of
injection scenario.  (As I write in my review of the -12, I think we can
get a similar level of protection without making such a stringent
requirement, but it's best to keep that discussion in the new review's
thread.)

> > ### Guidance on what fields to integrity protect
> > 
> > In §6.1 I'd want to include alongside the requirement for new integrity
> > protected option types to specfy which fields they protect, some guidance on
> > which sorts of things to protect or not protect.  (This would be a place to
> > codify the behavior I noted as being implicitly present in the document, that
> > fields that affect the structure/interpretation of the rest of the packet
> > should be integrity protected.)
> 
> [JI] Ack. Proposed change:
> 
> OLD:
> 
> "A document defining a new IOAM Integrity Protected Option-Type MUST define
> the IOAM Option-Type header fields involved in the integrity protection of
> IOAM-Data-Fields, as done in Section 4.1.1, Section 4.2.1, Section 4.3.1,
> and Section 4.4.1 of this document."
> 
> NEW:
> 
> "A document defining a new IOAM Integrity Protected Option-Type MUST specify
> the IOAM Option-Type header fields involved in the integrity protection of
> IOAM-Data-Fields, as done in Section 4.1.1, Section 4.2.1, Section 4.3.1,
> and Section 4.4.1 of this document. The following rule MUST be followed to
> determine what IOAM Option-Type header fields to include in the integrity
> protection: ''Mutable IOAM Option-Type header fields MUST NOT be integrity
> protected. Immutable IOAM Option-Type header fields that affect the
> structure or interpretation of IOAM-Data-Fields, or that trigger actions on
> IOAM nodes, MUST be integrity protected. IOAM Option-Type header bit fields
> can be partially protected (e.g., flags), following the same logic on a bit
> level (i.e., mutable vs immutable bits).''"

It looks like there's been some additional editing but the proposal here
did make it into the draft in some form.  This is definitely improved from
the -07; thank you!

My apologies again for being very slow to respond here.

-Ben