Subject: Re: [dtn] Benjamin Kaduk's Discuss on draft-ietf-dtn-bpsec-default-sc-08: (with DISCUSS and COMMENT)

From: "Birrane, Edward J." <[email-masked]>
Date: Thu, 1 Jul 2021 03:48:28 +0000

Benjamin,

  Thank you for this thorough and excellent review. 

  My replies are in-line below, and I have enumerated COMMENTS with C# to aid in referencing going forward; DISCUSSes were prior enumerated.

  Would the proposed changes below address your concerns regarding this document?

-Ed

---
Edward J. Birrane, III, Ph.D. (he/him/his)
Embedded Applications Group Supervisor
Space Exploration Sector
Johns Hopkins Applied Physics Laboratory
(W) 443-778-7423 / (F) 443-228-3839

> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> (D1) the construction for the HMAC input plaintext and GCM AAD seems to
> be "malleable" at the security context layer 
<SNIP>
> So, I think we need to include at least the scope flags as part of the IPPT/AAD
> in order to provide injectivity.  It might be worth considering adding
> additional framing and typing to make clear boundaries between the
> different parts of the IPPT/AAD, but my current understanding is that it
> would not be strictly necessary to do so.

D1: Agree with proposed change. Apply in a -09.

Agree that including the scope flags as part of the HMAC input plaintext and GCM AAD would prevent the condition you describe, as they would specify the fields to be included and the order in which they are to be included.

Recommend updating the IPPT and AAD construction algorithms to include the scope flags.

> (D2) There seems to be some risk associated with the current HMAC
> construction, since the HMAC with a given key over a given plaintext will be
> the same each time it is calculated. 
<SNIP>
>  I suspect that there may be some operational
> issues with the "unique nonce per HMAC" approach that would make it not
> terribly reliable in practice, but "use a constant-time comparison" should be
> fairly straightforward.

D2: Agree with proposed change. Apply in a -09.

Maintaining the state information associated with a nonce in a DTN will, as you mention, be problematic. Using a constant-time comparison to make the software isochronous would mitigate a timing attack.  

The question is whether to make a constant-time comparison a SHOULD or a MUST. Certainly making such a function is not difficult, but it might not be strictly necessary when, say, orbiting Mars. 

I propose adding this topic in security considerations, stating that a constant-time comparison SHOULD be used to protect against timing attacks and absent that, the impact of side-channel and timing attacks must (lowercase) be considered as part of implementation.

> (D3) While we do provide the standard guidance against using any given key
> with more than one algorithm (e.g., with HMAC-SHA-256 and AES256-GCM),
> there seem to be additional considerations relevant to this protocol that
> merit further discussion in the security considerations.
<SNIP>
> and could provide some modest mitigation
> techniques such as using distinct KEKs for receiving wrapped keys that have
> different intended usage.  That is, one KEK for receiving AES keys, another
> for HMAC keys, etc..  Attaching context (intended algorithm, etc.) to the KEK
> allows such context to be indirectly attached to the received wrapped keys,
> which otherwise would come without much context for intended usage.

D3: I agree that KEK re-use across contexts is problematic and we should address this in a -09.

Since the KEK is not included in the security block itself, this seems to be something that needs to be addressed in the security considerations - namely that the same KEK MUST NOT be used for other security contexts (certainly not between HMAC and GCM). A compliant security source would not use the same KEK to wrap both an HMAC-SHA-256 and an AES256-GCM key. And a security verifier or security acceptor would not use the same KEK to unwrap such keys.

> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> Section 3.1
> 
>    BIB-HMAC-SHA2 supports three variants of HMAC-SHA, based on the
>    supported length of the SHA-2 hash value.  These variants correspond
>    to "HMAC 256/256", "HMAC 384/384", and "HMAC 512/512" as defined in
>    [RFC8152] Table 7: HMAC Algorithm Values.  [...]
> 
> This is unfortunate timing, I suppose, but RFC 8152 is going to be replaced
> imminently by draft-ietf-cose-rfc8152bis-algs and draft-ietf-cose-rfc8152bis-
> struct, which are virtually certain to be published as RFCs before this
> document.

C1: Concur. Address in a -09.

The slight preference for using RFC8152 was that it was published and the information that the default security context references was unchanged. If we are virtually certain that changing references does not introduce a timing dependency we can update this in a -09.

> Section 3.2
> 
>        NOTE: The security context identifier and security context
>        parameters of the security block are not included in the IPPT
>        because these parameters, by definition, are required to verify
>        or accept the security service.  Successful verification at
>        security verifiers and security acceptors implies that these
>        parameters were unchanged since being specified at the security
>        source.
> 
> (related to D1, but distinct...)
> This seems to be only mostly true ... if some other security context was
> created that also used HMAC-SHA2 it seems like the same output might be
> obtained or validated across security contexts ... but this would only cause
> problems if there were different semantics afforded to the different security
> contexts in some way.  Any attack would also be stymied if the honest
> participants were rigorous in separation of keys, with any given key being
> used with only one security context.

C2: Concur.   Address in a -09.

As part of the comments on D3, we should require that KEKs using in these security contexts not be reused in any other security context.

> Section 3.3.3
> 
>    Integrity scope flags that are unrecognized MUST be ignored, as
>    future definitions of additional flags might not be integrated
>    simultaneously into security context implementations operating at all
>    nodes.
> 
> It's not entirely clear how useful a mandate to ignore unrecognized flags will
> be, if the flags would change the plaintext input and thus the value of the
> HMAC output.  I guess maybe the intent is just that intermediate notes
> processing a bundle but not acting as security acceptor should pass through
> such flags unchanged?

C3: Concur. Address in a -09.

Agree this is ambiguous. Particularly if the scope flags are now part of the IPPT/AAD, they cannot be changed in-transit.

> Section 3.3.4
> 
>                      BIB-HMAC-SHA2 Security Parameters
> 
>     +----+-----------------------+--------------------+---------------+
>     | Id |          Name         | CBOR Encoding Type | Default Value |
>     +----+-----------------------+--------------------+---------------+
>     | 1  |      SHA Variant      |        UINT        |       6       |
>     | 2  |      Wrapped Key      |    Byte String     |      NONE     |
>     | 4  | Integrity Scope Flags |        UINT        |      0x7      |
>     +----+-----------------------+--------------------+---------------+
> 
> The examples use a parameter Id of '3' for the Integrity Scope Flags, not the
> '4' shown here.  Since the parameter Ids are just CBOR integers, the
> sequential assignment with '3' would seem to make the most sense.

C4: Concur. Address in a -09.

> Section 3.7.1
> 
>       Any local flags used to generate the IPPT SHOULD be placed in the
>       integrity scope flags security parameter for the BIB unless these
>       flags are expected to be correctly configured at security
>       verifiers and acceptors in the network.
> 
> Just to confirm: do we want "SHOULD ... unless" or "MUST ... unless"?

C5: Concur. Address in a -09.

Agree - this is a MUST...unless. 

> Section 4.2
> 
>        NOTE: The security context identifier and security context
>        parameters of the security block are not included as additional
>        authenticated data because these parameters, by definition, are
>        those needed to verify or accept the security service.
>        Therefore, it is expected that changes to these values would
>        result in failures at security verifiers and security acceptors.
> 
> As for the BIB, this note seems only mostly true.

C6: Concur. Address in a -09.

Based on D1, update the text to clarify this note.

> Section 4.3.2
> 
>    When not provided, implementations SHOULD assume a value of 3
>    (indicating use of A256GCM), unless an alternate default is
> 
> [same comment about "SHOULD ... unless" as above]

C7: Concur. Address in a -09.

> Section 4.5
> 
>    The security provided by block ciphers is reduced as more data is
>    processed with the same key.  The total number of bytes processed
>    with a single key for AES-GCM is recommended to be less than 2^64, as
>    described in Appendix B of [AES-GCM].
> 
> The treatment in Appendix B seems to be specific to the authentication
> functionality (and seems to refer to number of *blocks*, not number of
> *bytes*); section 8.3 of that document has additional guidance on overall
> invocations of the authenticated encryption function, that include smaller
> limits that apply to the number of invocations of the function.

C8: Concur. Address in a -09.

The value of placing pointers to, and repeating information from, other documents is to help implementers, and agree this is additional information that would help implementers.

> Section 4.6
> 
>       The pairing of an IV and a security key MUST be unique.  An IV
>       MUST NOT be used with a security key more than one time.  If an IV
> 
> I believe that the use with the *encryption* function is the risky one.
> This is good, because if IV reuse with decryption was problematic, our use of
> AES-KW would induce pretty strong requirements for state-keeping on
> verifiers/recipients in order to meet this requirement that could be
> operationally challenging!

C9: Concur! Address in a -09.

This should be clarified.

>       As discussed in Appendix B of [AES-GCM], implementations SHOULD
>       limit the number of unsuccessful verification attempts for each
>       key to reduce the likelihood of guessing tag values.
> 
> This type of check would also have potential issues with state-keeping when
> AES-KW is used, since an attacker could cause a large number of keys to have
> been used at least once.

C10: No issue. Address in a -09.

I can add this text to the paragraph discussing this, but do not have implementation guidance associated with it.

> Section 4.8.2
> 
>       The authentication tag MUST be present in the BCB security context
>       parameters field if additional authenticated data are defined for
>       the BCB (either in the AAD scope flags parameter or as specified
>       by local policy).  This tag MUST be 128 bits in length.
> 
> I think the authentication tag always needs to be present, not "just"
> when there is AAD used.

C11: Concur! Address in a -09.

Thank you for this catch.

> Section 7
> 
> I see that RFC 5649 (or really, RFC 3537) is also available as a reference for an
> AES-KW algorithm, and that the NIST reference indicates that the RFCs are
> "essentially equivalent" to what is specified in the NIST reference.  My
> understanding is that we have a mild preference for IETF references over
> external references when a choice is available.
> 
> Likewise, RFC 2104 is available as an HMAC reference.

C12: No issue. Address in a -09.

Fine to reference IETF references over others if they are equivalent.

> Section A.1.4
> 
> Is the final "full output bundle" supposed to include the payload block after
> the BIB block?  I do not see it...

C13: That can be added in a -09.

> NITS
> 
> Section 3.1
> 
>    The BIB-HMAC-SHA2 security context provides a keyed hash over a set
>    of plain text information.  This context uses the Secure Hash
>    Algorithm 2 (SHA-2) discussed in [SHS] combined with the HMAC keyed
>    hash discussed in [HMAC].
> 
> I think s/keyed hash/keyed-hash Message Authentication Code (MAC)/.

C14: Concur. Address in a -09.

> Section 3.3.3
> 
>    Bits in this field represent additional information to be included
>    when generating an integrity signature over the security target.
> 
> My preference is to avoid using the word "signature" to describe a MAC, but
> I recognize that there is some precedent for using "signature" in this manner.
> (I won't mention it again for this document.)

C15: No Change.

> Section 3.5
> 
>    HMAC keys used with this context MUST be symmetric and MUST have a
>    key length equal to the output of the HMAC.  For this reason, HMAC
>    keys will be integer divisible by 8 bytes and special padding-aware
>    AES key wrap algorithms are not needed.
> 
> s/HMAC Keys will be/HMAC key lengths will be/

C16: Concur! Address in a -09.

> Section 4.2
> 
>    Keys used with this context MUST be symmetric and MUST have a key
>    length equal to the key length defined in the security context
>    parameters or as defined by local security policy at security
>    verifiers and acceptors.  For this reason, content-encrypting keys
>    will be integer divisible by 8 bytes and special padding-aware AES
>    key wrap algorithms are not needed.
> 
> Similarly, s/content-encrypting keys will be/content-encrypting key lengths
> will be/.

C17: Concur! Address in a -09.

> Section 4.7.1
> 
>    The plain text used during encryption MUST be calculated as the
>    single, definite-length CBOR byte string representing the block-type-
>    specific data field of the security target excluding the CBOR byte
>    string identifying byte and optional CBOR byte string length field.
> 
> I don't think I see why the CBOR byte string length field is "optional"
> in this scenario.  (Likewise for decryption.)
> 
>    |             18ED             |    18   |            ED            |
>    +------------------------------+---------+--------------------------+
>    | C24CDEADBEEFDEADBEEFDEADBEEF |   C24C  |
> DEADBEEFDEADBEEFDEADBEEF |
> 
> I'm having trouble decoding the "CBOR part"s here; in particular, I expected
> to see the major type 2 and CBOR "argument" with the length of the byte
> string, which '4C' seems to match up with for the second example, but not be
> present for the first example.  Are '18' and 'C2'
> supposed to be CBOR tags?

C18: No Issue. Address in a -09.

We can clean this up. C2 is the tag for a byte string holding a big num. Agree that as byte strings we should see 0x41ED and 0x4CDEADBEEF...

> Section 4.8.1
> 
>       The key length used by this security context MUST be considered
>       when setting the AES variant security parameter for the BCB if it
>       differs from the default AES variant.  Otherwise, the AES variant
> 
> The "MUST be considered" language feels odd to me (what does
> "considered" mean?); the "SHOULD be added as the [parameter] if it differs
> from the default" languaged used for the SHA2 output length feels more
> natural to me.

C19: Agree. Address in a -09.

> Section 4.8.2
> 
>    During encryption, five inputs are prepared for input to the AES/GCM
>    cipher: the decryption key, the IV, the security target cipher text
> 
> s/encryption/decryption/

C20: Concur! Address in a -09.

> Section 6.2
> 
>       In the event that a key is compromised, any security operations
>       using a security context associated with that key SHOULD also be
>       considered compromised.  This means that the BIB-HMAC-SHA2
>       security context SHOULD NOT provide integrity when used with a
>       compromised key and BCB-AES-GCM SHOULD NOT provide confidentiality
>       when used with a compromised key.
> 
> I'm a little confused by the phrasing "SHOULD NOT provide
> [confidentiality/integrity]", since the reality is more along the lines of that
> they will not do so, factually speaking.  Perhaps the intent is that the "SHOUL
> NOT be treated as providing" the indicated properties?

C21: Concur. Adjust to say "SHOULD NOT be treated as providing..." in a -09.

> Section 6.4
> 
>    The AES key wrap (AES-KW) algorithm used by the security contexts in
>    this document does not use an initialization vector and does not
>    require any key padding.  [...]
> 
> Very pedantically, I think that AES-KW does not use a per-invocation IV
> (there is a single globally constant IV used as input to the AES cipher
> operation).

C22: Concur. Address in a -09.

We can make that clarification.

> Section A.1.4
> 
> I suggest using lowercase hex digits consistently.

C23: Concur. Address in a -09.

> Section A.3.1.2
> 
> What do the chevrons around "300" indicate?

C24: No change.

<< >> indicate CBOR items embedded in a byte string, defined in G.3 of RFC8610.

> Section A.3.5, A.4.5
> 
> I suggest consistently using lowercase hex digits here as well.  (It shows up
> when I try to diff a manually assembled bundle against the encoded output
> shown here.)

C23: Concur. Address in a -09.

---

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 30 Jun 2021 20:58:03 -0700

Hi Ed,

Skimming quickly, your proposals all seem reasonable at first read.
In particular your proposed resolutions will address the Discuss points.

I will plan to make a closer read when I am not under deadline pressure,
in case there is anything where I want to provide any further remarks or
clarification.

Thanks!

-Ben