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

[Benjamin Kaduk <[e-mail]>]

On Fri, Jul 09, 2021 at 10:11:49PM +0000, Francesca Palombini wrote:
> Hi Ed,
> 
> Thank you for your answers, and for implementing the changes. I have gone through v-09 and I have some additional comments. I still have 2 blocking points open: D3 and D5. Everything else looks good, or was minor so that I don't mind the "no change". 
> 
> I'll CC the security ADs and ask them to take a look at C12 below, just to make sure that the resolution and phrasing is ok.

I don't think we need to fully specify how IVs are selected to ensure
uniqueness, though providing an example construction that is generally safe
is an okay thing to do, if we wanted to put one in.

Reviewing the current security considerations, the "vulnerable to forgery
attacks" is correct but perhaps easy to overlook.  I might add another
sentence along the lines of "because the loss of integrity protection
occurs with even a single IV reuse, reuse of GCM IV is often said to have
'catastrophic' security consequences" to increase its prominence, but to a
large extent that's an editorial concern and not a technical one.

-Ben

> 
> On 30/06/2021, 00:12, "Birrane, Edward J." <[e-mail]> wrote:
> 
> >Francesca,
> >
> >  Thank you for this excellent and very thorough review!
> >
> >  My replies are in-line below. To aid in discussion, I have enumerated DISCUSSes as D# and COMMENTs as C#. This provides a slightly different enumeration than the one you provided, but hopefully it helps to track DISCUSS vs COMMENT.
> >
> >  If we make the recommended changes in a -09 draft, as listed below, would that resolve the DISCUSSes as we understand them now?
> >
> >-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:
> >> ----------------------------------------------------------------------
> >>
> >> 1. -----
> >>
> >> FP: I agree with my colleagues that extensibility should be considered for
> >> algorithms. This document defines BIB-HMAC-SHA2 and BCB-AES-GCM, with
> >> the algorithms these security contexts provide. Adding support for one
> >> algorithm would need to define a new security context. Wouldn't it make
> >> sense to, instead, provide a way to add algorithms to the existing algorithms?
> >> For example, defining an IANA registry for each security context with the IDs
> >> of algorithms supported (taken from COSE).
> >
> >D1: Recommend no change.
> >
> >The intent of these default security contexts was to make them simple and functional with the expectation that other security contexts with the characteristics you describe would come later. In particular, the DTNWG is working on a security context for COSE, the draft of which can be found at: https://datatracker.ietf.org/doc/html/draft-bsipos-dtn-bpsec-cose-06
> >
> 
> FP: I see. Thanks for the pointer to the COSE context, which, if I read correctly, allows for a number of algorithms. I do find strange that new security contexts will have to be defined in the "Security Context Identifiers" registry just so that more algorithms can be added, but since the wg has an extensibility option, I won't insist on this point. It might come up again for the draft-bsipos-dtn-bpsec-cose document, when it reaches IESG evaluation.
> 
> >
> >> 2. -----
> >>
> >>       - Bit 2 (0x0003): Security Header Flag.
> >>
> >> FP: This should be (0x0004) and not (0x0003) (and same in a later section).
> >> Also, this is not wrong, but the bitmaps (here and everywhere else) could
> >> also be represented as 0b0100 in CBOR diagnostic notation, which to me is
> >> clearer.
> >
> >D2: Agreed!
> >
> >Great catch on a typo. Recommend to fix in a -09.
> >
> 
> FP: Thanks for having fixed this in -09.
> 
> >
> >> 3. -----
> >>
> >>       - Bits 8-15 are unassigned.
> >>
> >> FP: I am wondering why the limit on Bit 15, marked as unassigned: I think it
> >> would make sense to say Bits 8 and higher are unassigned. (This change
> >> would need to be reflected in the IANA sections)
> >
> >D3: Recommend no change.
> >
> >To assist with hardware implementations, there is value in allowing implementers to presume an upper-bound to the size of this field.
> >
> 
> FP: I would agree, if such an upper-bound was specified. However this section does not specify any such upper bound, and CBOR integers are not implicitly limited to a certain size either. I see two options here: either 1. specify the upper bound (for example stating that the maximum value of the field is 65535) or 2. remove the limitation to 15 bits, both in this section and in the IANA section, and add a statement that "this field is not expected to have a value higher than 65535". Both these options seem ok to me.
> 
> >
> >> 4. -----
> >>
> >> FP: this might be me missing some fundamental reading from bpsec, but I
> >> see that the blocks are defined as CBOR sequences. However, that is only
> >> mentioned in the appendix (meant to be informative):
> >>
> >>    represented using CBOR structures.  In cases where BP blocks (to
> >>    include BPSec security blocks) are comprised of a sequence of CBOR
> >>    objects, these objects are represented as a CBOR sequence as defined
> >>    in [RFC8742].
> >>
> >> Is this defined somewhere else? If yes, could you add a pointer to the doc
> >> where it is defined? If not, this should be clarified, and specified earlier in the
> >> text, say in sections 3 and 4.
> >
> >D4: Recommend no change.
> >
> >The BPSec document defines the contents of a security block in section 3.6 as a series of fields that must individually be encoded using CBOR.  The reference to RFC8742 here is meant to say that the notation used in the appendix for the ordered, CBOR encoded fields defined by BPSec will use the notation for an unadorned CBOR sequence in diagnostic notation (section 4.2 of RFC8742).
> >
> 
> FP: I see, and I realize I am too late to suggest that the BPSec document explicitly references RFC 8742. I am ok with no change here.
> 
> >
> >> 5. -----
> >>
> >>      [1, b'Twelve121212'] / Initialization Vector /,
> >>
> >> FP: I think the IV value is wrong here and should be
> >> h'5477656c7665313231323132'.
> >
> >D5: Agree.
> >
> >h'5477656c7665313231323132' should be used here for clarity. We can represent this value in this way in an upcoming -09 version of the document.
> >
> 
> FP: Thanks for making the change. However, I noticed that you also made one more change (which I assume must come from another AD?): the scope flag is now encoded as a byte string throughout the Appendix A. However, its definition has not changed, and it is still defined as a CBOR integer in section 3.3.3. That is inconsistent, unless I am missing something, and should either revert back to integer or change to byte string in a number of places where it's still defined as unsigned integer.
> 
> >
> >>
> >> ----------------------------------------------------------------------
> >> COMMENT:
> >> ----------------------------------------------------------------------
> >>
> >> 6. -----
> >>
> >> FP: I believe the reference to RFC 8152 should be replaced with references to
> >> draft-ietf-cose-rfc8152bis-algs.
> >
> >C1: Recommend no change
> >
> >The portions of RFC8152 referenced in this security context document are unchanged in the RFC and the draft bis. There was a minor preference to reference the existing standard.
> 
> FP: Ok. However note that that document will be obsoleted soon, and my preference is always to reference documents that are not obsolete. However this is minor, so won't insist on it if you don't think it's worth updating.
> 
> >
> >> 7. -----
> >>
> >>    BIB-HMAC-SHA2 defines the following security context parameters.
> >>
> >>                      BIB-HMAC-SHA2 Security Parameters
> >>
> >>     +----+-----------------------+--------------------+---------------+
> >>     | Id |          Name         | CBOR Encoding Type | Default Value |
> >>
> >> FP: Id should be defined, and a reference to the correct section of bpsec
> >> where Ids are defined should be provided.
> >
> >C2: Agreed.
> >
> >This would make the mapping of identifier clearer and should be added in a -09 version of the document.
> >
> 
> FP: Thank you, looks good.
> 
> >
> >> 8. -----
> >>
> >>     +----+-----------------------+--------------------+---------------+
> >>     | 1  |      SHA Variant      |        UINT        |       6       |
> >>
> >> FP: the correct terminology for CBOR types is not UINT but unsigned integer.
> >
> >C3: Agreed
> >
> >These instances can be corrected in a -09 version of the document.
> >
> 
> FP: Thanks, looks good.
> 
> >
> >> 9. -----
> >>
> >>     | 2  |      Wrapped Key      |    Byte String     |      NONE     |
> >>
> >> FP: Please explicitly write that NONE means that the key does not have a
> >> default value.
> >
> >C4: Agreed.
> >
> >These instances can be corrected in a -09 version of the document.
> >
> 
> FP: Thanks, looks good.
> 
> >
> >> 10. -----
> >>
> >>     | 4  | Integrity Scope Flags |        UINT        |      0x7      |
> >>     +----+-----------------------+--------------------+---------------+
> >>
> >> FP: This representation seems to be conflicting with the one for SHA Variant,
> >> and although not wrong, usually 0x notation is given for byte strings, so this
> >> might be confusing. I suggest to change the default value to 7, rather than
> >> 0x7.
> >
> >C5: Agreed.
> >
> >This can be corrected in a -09 version of the document.
> 
> FP: Thanks, looks good.
> 
> >
> >
> >> 11. -----
> >>
> >> FP: I think this draft could make good use of a terminology section, where
> >> terms such as security results, identifier, bundle, CRC, canonical form, set,
> >> block type, block number, etc can point to the correct document defining
> >> them.
> >
> >C6: Recommend minor change
> >
> >These terms are defined by the BP and BPSec documents with which the security context is to be used. Recommend that this be made clear in the Introduction.
> >
> 
> FP: This helps, thank you.
> 
> >
> >> 12. -----
> >>
> >>       on this security target, as discussed in Section 3.4).
> >>
> >> FP: nit - one closed parenthesis too much.
> >
> >C7: Agreed.
> >
> >This can be corrected in a -09 version of the document.
> >
> 
> FP: Thanks, looks good.
> 
> >
> >> 13. -----
> >>
> >>       The HMAC key MAY be wrapped using the NIST AES-KW algorithm and
> >>       the results of the wrapping added as the wrapped key security
> >>       parameter for the BIB.
> >>
> >> FP: I think this MAY is misleading: written like this, it seems as if the HMAC
> >> key can also be sent unwrapped (while I understand the reason to add this
> >> MAY was because the key might be retrieved in other ways than being sent).
> >
> >C8: Agreed.
> >
> >Agreed that this can be misunderstood. Recommend clarifying that MAY here indicates that key retrieval may happen using other means.
> >
> 
> FP: Thanks for the clarification, looks good.
> 
> >
> >> 14. -----
> >>
> >>    security block.  This information includes the data included in the
> >>    confidentiality service and MAY include other information (additional
> >>    authenticated data), as follows.
> >>
> >> FP: nit - This sentence is hard to parse, could it be rephrased to remove the
> >> "include" duplication?
> >
> >
> >C9: Agreed.
> >
> >Can rephrase this sentence in a -09 version of the document.
> >
> 
> FP: Thank you, looks good.
> 
> >
> >> 15. -----
> >>
> >>       The authentication tag calculated by the AES/GCM cipher MUST be
> >>       added as a security result for the security target in the BCB
> >>       holding results for this security operation.
> >>
> >> and later on...
> >>
> >>       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.
> >>
> >> FP: I agree with Martin that the text is confusing about the presence of the
> >> tag in the security result. Adding an "unless ..." would help.
> >
> >C10: Agreed.
> >
> >In responding to Martin's DISCUSS we proposed clarifying this language in a -09 version of the document.
> >
> 
> FP: Thank you for clarifying.
> 
> >
> >> 16. -----
> >>
> >>    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.
> >>
> >> FP: I think here you mean to say that:
> >>    The plain text used during encryption is the
> >>    block-type-specific data field value of the security target.
> >>
> >> Is that right? I got a bit confused by the formulation, but please correct me if
> >> this is anything else than just the value extracted from the CBOR encoding.
> >
> >C11: Agreed this is confusing. Recommend rewording in a -09.
> >
> >The block-type-specific data field is encoded as a definite-length CBOR byte string. The CBOR encoding is not considered part of the plain text. We could reword this to make clearer that any CBOR encoding artifacts are not to be considered as part of the plain text.
> >
> >
> 
> FP: Thank you for clarifying, that helps, and the text in v-09 looks good.
> 
> >
> 
> C12:
> >> 17. -----
> >>
> >>       The IV selected MUST be of the appropriate length.  Because
> >>       replaying an IV in counter mode voids the confidentiality of all
> >>       messages encrypted with said IV, this context also requires a
> >>       unique IV for every encryption performed with the same key.  This
> >>       means the same key and IV combination MUST NOT be used more than
> >>       once.
> >>
> >> FP: This makes me uncomfortable, as I would like to see more indications on
> >> how the IV uniqueness is achieved. Also, there should be mention of the
> >> failure to provide IV uniqueness in the security considerations. However, I
> >> won't add this to my discuss because I am sure the Sec ADs will have more
> >> detailed comments.
> >
> >
> >C12: Recommend no change.
> >
> >Section 4.6 "GCM Considerations" notes:
> >
> >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
> >      and key pair are repeated then the GCM implementation is
> >      vulnerable to forgery attacks.  More information regarding the
> >      importance of the uniqueness of the IV value can be found in
> >      Appendix A of [AES-GCM].
> >
> >While there are many potential implementation approaches (generate and wrap a new key for each message, use a stored, monotonically incrementing value, etc..) we did not feel it was appropriate to place implementation design into this document.
> >
> 
> FP: Ok, I won't insist on this.
> 
> >
> >> 18. -----
> >>
> >>    This section presents a series of examples of constructing BPSec
> >>    security blocks (using the security contexts defined in this
> >>    document) and adding those blocks to a sample bundle.
> >>
> >> FP: Thank you for this appendix. It would be good to point to the code used
> >> to build these examples, if that is openly available.
> >
> >C13: Recommend no change.
> >
> >Unfortunately, we are not able to provide the software for generating this.
> >
> 
> FP: Ok, that's fine, just figured I'd ask.
> 
> >
> >> 19. -----
> >>
> >>                           Block                    Block   Block
> >>                         in Bundle                  Type    Number
> >>
> >> +========================================+=======+========
> >> +
> >>         |  Primary Block                         |  N/A  |    0   |
> >>         +----------------------------------------+-------+--------+
> >>         |  Payload Block                         |   0   |    1   |
> >>         +----------------------------------------+-------+--------+
> >>
> >>                     Figure 1: Example 1 Original Bundle
> >>
> >> FP: It would be useful to explain (or point to the document explaining) this
> >> representation.
> >
> >C14: Agreed.
> >
> >This is similar to illustrations from the BPSec document and we can add a reference to that in a -09.
> >
> 
> FP: Thanks, the added text is what I was looking for.
> 
> >
> >> 20. -----
> >>
> >> A.1.4.  Final Bundle
> >>
> >> FP: I think it would be useful to include the CBOR diagnostic notation for the
> >> final bundle as well (several occurrences).
> >
> >C15: Recommend no change.
> >
> >We could provide an overall bundle, but security blocks are generated and consumed on a block basis, so a security implementation might not process an entire bundle.
> 
> FP: I see, that makes sense.
> 
> >
> >> 21. -----
> >>
> >> FP: I guess this is more of a comment on bpsec than on this document, but I
> >> was disappointed that no CDDL was defined for the CBOR encoding.
> >
> >C16: Recommend no change.
> >
> >Agreed this is a comment on the BPSec document.
> >
> >
> 
> FP: No change was requested :)
>