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

From: Francesca Palombini via Datatracker <[email-masked]>
Date: Tue, 29 Jun 2021 07:39:53 -0700

Francesca Palombini has entered the following ballot position for
draft-ietf-dtn-bpsec-default-sc-08: 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-dtn-bpsec-default-sc/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

Thank you for the work on this document.

I'd like to discuss the following point. I also have some non blocking
comments, which I hope will help improve the document.

Francesca

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).

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.

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)

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.

5. -----

     [1, b'Twelve121212'] / Initialization Vector /,

FP: I think the IV value is wrong here and should be
h'5477656c7665313231323132'.

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

6. -----

FP: I believe the reference to RFC 8152 should be replaced with references to
draft-ietf-cose-rfc8152bis-algs.

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.

8. -----

    +----+-----------------------+--------------------+---------------+
    | 1  |      SHA Variant      |        UINT        |       6       |

FP: the correct terminology for CBOR types is not UINT but unsigned integer.

9. -----

    | 2  |      Wrapped Key      |    Byte String     |      NONE     |

FP: Please explicitly write that NONE means that the key does not have a
default value.

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.

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.

12. -----

      on this security target, as discussed in Section 3.4).

FP: nit - one closed parenthesis too much.

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).

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?

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.

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.

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.

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.

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.

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).

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.

---

From: "Birrane, Edward J." <[email-masked]>
Date: Tue, 29 Jun 2021 22:12:10 +0000

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

> 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

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

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

> 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):
> 
> 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). 

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

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: 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. 

C3: Agreed

These instances can be corrected in a -09 version of the document.

> FP: Please explicitly write that NONE means that the key does not have a

C4: Agreed.

These instances can be corrected in a -09 version of the document.

>     | 4  | Integrity Scope Flags |        UINT        |      0x7      |

C5: Agreed.

This can be corrected in a -09 version of the document.

> block type, block number, etc can point to the correct document defining

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: nit - one closed parenthesis too much.

C7: Agreed.

This can be corrected in a -09 version of the document.

> 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: nit - This sentence is hard to parse, could it be rephrased to remove the

C9: Agreed.

Can rephrase this sentence in a -09 version of the document.

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

> 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: 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: 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.

>                         in Bundle                  Type    Number

C14: Agreed.

This is similar to illustrations from the BPSec document and we can add a reference to that in a -09.

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

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