[secdir] Secdir early review of draft-ietf-ippm-capacity-protocol-04

[Brian Weis via Datatracker <[e-mail]>]

Reviewer: Brian Weis
Review result: Has Issues

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.

General comments

(1) The guidance of what bytes are included in the SHA-256 calculation
and result placed in the authDigest field might need some enhancement.

— Some places say that the authDigest field is zeroed (e.g., Section
4.2.2 ) and some says “The SHA-256 calculation SHALL cover all the
preceding fields.”, which seems to omit the auhDigest field. I guess
these aren't mutually exclusive statements, but it would be cleaner
to have all of the description in one place so it is clear.

— Section 5.1.1 doesn’t say the network addresses and headers are
included in the digest. It’s generally good practice to include
more context into the digest calculation so that a valid message
(e.g., Setup Request) can’t be extracted and placed in a different
measurement flow that is not intended by the original sender. This is
a substitution attack.

— If network addresses and headers aren’t included in the digest,
then it will be important to describe why the substitution attack
isn’t a threat. I understand that if NAT on the client that including
it's network address is not possible, but there's still an issue.

— Note that the inclusion of authUnixTime with the receiver
checking “acceptable immediacy” is only a partial mitigation for a
substitution attack, since the receiver’s notion of “acceptable
immediacy” is apparently quite long from an attacker's perspective
“(+/- 2.5 minutes)” and any other message including this PDU sent
fron an attacker (e.g., with different network headers) during this
period will be acceptable to the receiver.  Reducing the window
size won’t completely mitigate this attack either.

(2) There doesn’t seem to be a definition for the MBZ octets and
remainder bits, or a description of their purpose. (I’m guessing
it’s “must be zero” but even that should be defined for the reader.)

Specific comments

Section 4.2.4.

— The use of AES-GCM with a long-lived symmetric key (such as one
on an RFC 7210 key chain) is not safe. The IV used by GCM is not a
random IV such as used by some other modes of which you might be
aware (e.g., CBC), but rather it is a counter that must NEVER
repeat with that key. Ensuring no repeats is not operationally
feasible for a key kept on a key chain. I.e., The counter must keep
incrementing between packets, reliably stored over a reboot so that
it isn't used again, and when the counter reaches the maximum value
the key is not longer usable. I would recommend specifying CBC mode
and a randomly chosen IV.

— Also, there are some additional parameters that need to determined
to guarantee interoperability. For example, which octets are
encrypted, if a partial block needs to be encrypted how is it padded
out before encryption to make a full block, and requirements on IV
generation (e.g., that is must be from a good quality random number
generator).

— Unauthenticated encryption is generally seen as risky, so when
encryption is included the the authDigest should also be used as
it is in the other modes. Figure 2 should have both authDigest AND
IV fields.

Section 4.2.5. This section mentions that DTLS is not useful because
“The replay protection would remove duplicated packets and prevent
transparent measurement of this impairment.”. IPsec will also
transparently remove duplicate packets, and since TLS is based on
TCP, which has duplicate segment protection, I would expect that
the application would also never see duplicate measurement PDUs
from TLS either. This is generally seen as a feature for network
encryption methods.

Section 10. “Client-server authentication and integrity protection
for feedback messages conveying measurements is REQUIRED.” Since
one of the methods allows “unauthenticated mode for all messages”,
I think this is meant to start with “Support for client-server
authentication ….”. That is, the REQUIRED language means
it must be implemented, but not used.

Nits

— Section 4.2 says “There are four security modes of operation”,
which is also stated elsewhere, but there seems to actually be five.
I understand that the fifth one is actually an external security
mode that can be combined with one of the four modes (e.g.,
“unauthenticated mode for all messages”) in the PDU. It might be
clearer to take the fifth one out of the list and just make it a
paragraph.

— Section 4.2.1 s/miss-match/mismatch/