Subject: Re: [secdir] Secdir last call review of draft-ietf-babel-information-model-11

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 2 Nov 2020 17:27:14 -0800

Hi Valery,

Thanks for this review -- a lot of good points raised.
I filed a Discuss ballot to continue to discuss several of them, especially
to make sure that we are clear and precise about what cryptographic
operations are performed (for the "operation"s), and to discuss a few
places (such as key lengths) where the information model seems to be
imposing restrictions not present in the core protocol without providing a
justification for the divergence.

-Ben

On Mon, Oct 26, 2020 at 07:54:39AM -0700, Valery Smyslov via Datatracker wrote:
> Reviewer: Valery Smyslov
> 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.
> 
> The document defines an informational model for Babel routing protocol. This
> informational model can be used as a basis for creating various data models
> (e.g. YANG). The document is properly structured and well written, however I
> think that it has some issues concerned with security.
> 
> Issues.
> 
> 1. Section 3.1:
> 
>    babel-mac-algorithms:  List of supported MAC computation algorithms.
>       Possible values include "HMAC-SHA256", "BLAKE2s".
> 
> BLAKE2s can produce MACs of different sizes from 1 to 32 bytes and the desired
> size of the MAC is a parameter for it. Where the size of MAC is specified? For
> HMAC with SHA256 I can at least imagine that full 256 bits output is used as a
> MAC...
> 
> 2. Section 3.9:
> 
>    babel-cert-test:  An operation that allows a hash of the provided
>       input string to be created using the certificate public key and
>       the SHA-256 hash algorithm.  Input to this operation is a binary
>       string.  The output of this operation is the resulting hash, as a
>       binary string.
> 
> I failed to understand what this operation should do. Literally reading it is
> intended to produce SHA2-256 hash of public key and some arbitrary string
> (concatenated? in what order?). But then I failed to understand the purpose of
> this test. I would have understood if this operation provides signing of the
> arbitrary string using private key and SHA2-256 as a hash function (similarly
> to babel-mac-key-test), but it in not what is written...
> 
> 3. Section 5 (Security Considerations):
> 
> I think that text about keys (their length and properties) needs some
> expansion. First, there are no any RFC2119 words discouraging using short and
> weak keys (there is some text, but without RFC2119 words and with no references
> it's just hand waving). Note, that draft-ietf-babel-hmac-12 has some text about
> the properties of the keys, so I believe at least it must be referenced here. I
> also suspect that explicitly allowing zero-length and short keys will lead to
> situations when some network operators will use them (because they are not
> prohibited), thus subverting security properties of MAC...
> 
> Nits.
> 
> 1. Section 1 (Introduction):
> 
>    [I-D.ietf-babel-hmac] defines a
>    security mechanism that allows Babel packets to be cryptographically
>    authenticated, and [I-D.ietf-babel-dtls] defines a security mechanism
>    that allows Babel packets to be encrypted.
> 
> DTLS provides both confidentiality and authentication of data, so to be
> formally correct, please add "both authenticated and" before "encrypted".
> 
> 2. Section 2 (Overview) at the very end:
> 
>    Note that this overview is intended simply to be informative and is
>    not normative.  If there is any discrepancy between this overview and
>    the detailed information model definitions in subsequent sections,
>    the error is in this overview.
> 
> This phrase makes me puzzled. The tree-like description of the information
> model in the Overview is very useful, however this phrase seems to discourage
> using it, because authors are not sure it is correct. I think it would be
> better for readers if authors double check that no discrepancy exists between
> the overview and the subsequent detailed description and remove this phrase.
> 
> 3. Section 3.3:
> 
>    babel-mac-verify  A Boolean flag indicating whether MAC hashes in
>       incoming Babel packets are required to be present and are
>       verified.  If this parameter is "true", incoming packets are
>       required to have a valid MAC hash.  An implementation MAY choose
>       to expose this parameter as read-only ("ro").
> 
> "MAC hashes" is strange wording, "MAC values" or just "MACs" are better in my
> opinion.
> 
> 4. Section 3.8:
> 
>    babel-mac-key-use-sign:  Indicates whether this key value is used to
>       sign sent Babel packets.  Sent packets are signed using this key
>       if the value is "true".  If the value is "false", this key is not
>       used to sign sent Babel packets.  An implementation MAY choose to
>       expose this parameter as read-only ("ro").
> 
> "Sign" is not a good word when you describe symmetric key operations (which
> computing MAC belongs to). Although it is often used informally, I think that
> RFC should be more meticulous in selecting words. I'd rather replace it with
> "compute MAC" and rename the entry to babel-mac-key-use-compute or
> babel-mac-key-use-mac (if it is possible). Note, that using "verify MAC" is OK.
> 
> 5. Section 3.8:
> 
>    babel-mac-key-value:
>        ...
>       This value is of a length suitable for the associated babel-mac-key-
>       algorithm.  If the algorithm is based on the HMAC construction
>       [RFC2104], the length MUST be between 0 and the block size of the
>       underlying hash inclusive (where "HMAC-SHA256" block size is 64
>       bytes as described in [RFC4868]).  If the algorithm is "BLAKE2s",
>       the length MUST be between 0 and 32 bytes inclusive, as described
>       in [RFC7693].
> 
> I wonder of the rationale for imposing the above restrictions on HMAC key
> length. HMAC can use keys of any length, but if the key is greater than block
> size of underlying hash function, then it's first hashed (small performance
> penalty). So I imagine that the rationale is to avoid this penalty. However, as
> RFC2104 states, key sizes greater than output length of the underlying hash
> function (32 bytes in case of SHA2-256) would not significantly increase the
> function strength, so it's just a waste of space. See also Issue 3 above.
> 
> 6.    Section 3.8:
> 
>    babel-mac-key-test:  An operation that allows the MAC key and hash
>       algorithm to be tested to see if they produce an expected outcome.
>       Input to this operation is a binary string.  The implementation is
>       expected to create a hash of this string using the babel-mac-key-
>       value and the babel-mac-key-algorithm.  The output of this
>       operation is the resulting hash, as a binary string.
> 
> The text mixes up words "hash" and "MAC". MAC is not a hash (even if MAC
> algorithm is often based on hash function). As with my previous grunt, I'd like
> RFC to be more meticulous in selecting words. Please, avoid using "hash" here.
> 
> 7. Section 5 (Security Considerations):
> 
>    ... This model requires that private keys never be
>    exposed.  The Babel security mechanisms that make use of these
>    credentials (e.g., [I-D.ietf-babel-dtls], [I-D.ietf-babel-hmac])
>    identify what credentials can be used with those mechanisms.
> 
> Please add "and MAC keys" after "private keys" since formally private keys are
> only defined for public key cryptography.
> 
> 8. Section 5 (Security Considerations):
> 
>    MAC keys are allowed to be as short as zero-length.  This is useful
>    for testing.
> 
> I wonder what's benefit of allowing zero-length keys for testing purposes. What
> is intended to be tested in this case? Implementation of MAC? Is it really
> needed outside test lab? Am I missing something?
> 
> 9. Section 5 (Security Considerations):
> 
>     Short (and zero-length) keys and
>    keys that make use of only alphanumeric characters are highly
>    susceptible to brute force attacks.
> 
> Formally, brute force attack with zero-length keys is not defined, since there
> is no key to find and all is in clear.
> 
> Comments.
> 
> 1. The document contains an entry in the Informational model defining which
> hash functions can be used with HMAC authentication. However, there is no
> corresponding entry of which ciphersuites can be used with DTLS. Is it up to
> DTLS library to select ciphersuites?
> 
> 
> 
> _______________________________________________
> secdir mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/secdir
> wiki: http://tools.ietf.org/area/sec/trac/wiki/SecDirReview

---

From: "STARK, BARBARA H" <[email-masked]>
Date: Wed, 4 Nov 2020 23:34:04 +0000

Hi Valery,
Thanks for your review. Taking into account Juliusz' response to your email, I'm proposing the following resolutions. For the first issue raised, I'm disagreeing slightly with Juliusz, so that may need further discussion. Please let me know if you disagree. I've copied Juliusz' responses here, so I can include my responses to your complete feedback and his in one email.
Roman specifically mentioned he agreed with your "recommendations about precise language around the names of the parameters". I think perhaps this was in reference to item 4 under nits? I didn't completely agree with your recommendation, so I'd appreciate some additional discussion on that.
Thx,
Barbara

> -----Original Message-----
> 
> Reviewer: Valery Smyslov
> (e.g. YANG). The document is properly structured and well written, however
> I think that it has some issues concerned with security.
> 
> Issues.
> 
> 1. Section 3.1:
> 

Juliusz said:
> Right.  The intent is that Blake2s is used with 32-octet keys and 16-octet
> hashes (collision-resistance is not a concern for Babel-MAC while
> dictionary attacks are).  Barbara, I think that you should explicitly
> state that Blake2s implies 128-bit hashes.  (You may also consider
> renaming BLAKE2s to BLAKE2s-128.)

The defined values for babel-mac-algorithms come directly from draft-ietf-babel-hmac. The defined value names should map closely to the names used for the algorithms in in that draft -- which they currently do. 

If it needs to be explicitly stated somewhere that an implementation of draft-ietf-babel-hmac with BLAKE2s outputs 128-bit MACs, then draft-ietf-babel-hmac (which was already submitted for publication) would be the correct place to say that. The information model is not the right place, unless there's some expectation for the size to be configurable or reportable. I'm not seeing any request for the MAC size to be configured or reported via the information model. 

I'm proposing no change to the defined values of babel-mac-algorithms in order to maintain complete consistency with the names used in draft-ietf-babel-hmac-12.

> to babel-mac-key-test), but it in not what is written...

One of the most common problems in configuring security mechanisms is in the format of the input key (hex, ASCII, base64, hashing that occurs to create "actual key", etc.). When a security mechanism fails to work, it is important for users or device managers to be able to trouble-shoot this specific point of failure. This test allows the user/manager to see if what this device thinks the MAC should be is the same as what another device thinks the MAC should be or is the same as the MAC being sent on the wire. Many ISPs have built a test like this into their ISP-supplied CE routers (invoked using the TR-069 protocol and TR-181 data model) to test various stored key values. It has proven useful.

> prohibited), thus subverting security properties of MAC...

Thanks. I'll add a reference to draft-ietf-babel-hmac Security Considerations.

Zero length and short keys were discussed on the mailing list. The group considered it appropriate to
allow configuration of zero-length keys for testing but to advise people to follow best
current practices. I find the use of normative language to attempt to control the behavior of 
a home network owner (for example) or someone setting up an informal ad hoc mesh
network (for example) to be odd. IMO, the IETF should not seek to control
the choices of people putting together such relatively small-scale networks through 
the use of strong normative language in an information model specification. It's 
impossible to enforce and such people pretty much never read RFCs.

If there is a strong desire for some sort of normative language, then I could suggest
OLD
   MAC keys are allowed to be as short as zero-length.  This is useful
   for testing.  Network operators are advised to follow current best
   practices for key length and generation of keys related to the MAC
   algorithm associated with the key.  Short (and zero-length) keys and
   keys that make use of only alphanumeric characters are highly
   susceptible to brute force attacks.
NEW
   MAC keys are allowed to be as short as zero-length.  This is useful
   for testing.  Network operators are RECOMMENDED to follow current best
   practices for key length and generation of keys related to the MAC
   algorithm associated with the key.  Short (and zero-length) keys and
   keys that make use of only alphanumeric characters are highly
   susceptible to brute force attacks. See the Security Considerations
  section of [ID.draft-ietf-babel-hmac] for additional considerations 
  related to MAC keys.

> formally correct, please add "both authenticated and" before "encrypted".

Thank you. I will make this change.
 
> the overview and the subsequent detailed description and remove this

The authors have double- and triple-checked, but human error is still possible, since these were not auto-generated (like YANG does). Whenever redundant information is presented, it is critical that it be normative in only one place. This language makes it clear which is normative and what to do in the case of discrepancies. I propose no change. If there is a strong push to remove this language, then I believe the correct approach would be to remove the entire section (remove the redundancy) rather than to allow confusion if there is an error. But people have found this section useful, so I would prefer to leave it as is.
 
> "MAC hashes" is strange wording, "MAC values" or just "MACs" are better in

Thank you. I'll change it to MAC.

> babel-mac-key-use-mac (if it is possible). Note, that using "verify MAC" is OK.

I've been thinking through this. I can't speak to the informal nature of "sign", but I can say that simply replacing "sign" with "compute" or "mac" wouldn't convey correctly what this parameter is about. This parameter is primarily concerned with whether or not a MAC is included in the sent packet. The sending is the critical piece, and not the computing (it's possible to compute the MAC without sending it; a MAC in a sent packet is assumed to have been computed). I could change the description to:
       Indicates whether this key value is used to compute a MAC and include that MAC in the
       sent Babel packet.  A MAC for sent packets is computed using this key
       if the value is "true".  If the value is "false", this key is not
       used to compute a MAC to include in sent Babel packets.  An implementation MAY choose to
       expose this parameter as read-only ("ro")

But I struggle with the proposed parameter renaming. I strongly believe the name should concisely describe that the Boolean value indicates whether or not to include a MAC in the sent packet. The term "sign" is one I've commonly seen to indicate that a MAC is included in the sent packet. I'm not aware of a different, similarly short word. "Compute" and "mac" do not convey the sending aspect. And sending is very asymmetric.

Juliusz said:
> This was discussed at length on the mailing list.  It's not about
> performance, it's about making it more difficult to use an unsafe
> procedure for generating keys.
> 
> Since Babel-MAC is vulnerable to dictionary attacks, the key must either
> be drawn randomly or generated using a procedure that is hardened against
> such attacks (scrypt, etc.).  Applying the procedure described in RFC 2104
> to a user-provided passphrase is not safe, and therefore we try to make it
> difficult for a naive user to do so.
> 
> I am opposed to putting the RFC 2104 hashing procedure in the information
> model.  Doing so would be a disservice to our users.

In addition to the rationale Juliusz mentioned, we (babel WG) also noted that implementers
of the babel MAC function were using existing libraries for the HMAC-SHA256 algorithm.
The user interface (UI) that accepted manual key entry was also from an existing library. When
the same longer strings were entered into different UIs of the different implementations, these
strings were treated differently and resulted in non-interoperability. The "actual key" (using
RFC 2104 words) ended up different. Requiring entered keys to be directly usable as "actual
keys" solved this problem. BTW, I have considered UIs for direct management and configuration
to effectively be implementations of the information model. 

I recommend no changes to this text.
 
> RFC to be more meticulous in selecting words. Please, avoid using "hash"

If I understand, the suggestion is to reword as:
   babel-mac-key-test:  An operation that allows the MAC key and MAC
      algorithm to be tested to see if they produce an expected outcome.
      Input to this operation is a binary string.  The implementation is
      expected to create a MAC of this string using the babel-mac-key-
      value and the babel-mac-key-algorithm.  The output of this
      operation is the resulting MAC, as a binary string.

If this is right, I'll make that change.

> only defined for public key cryptography.

Thank you. I'll make this change.
 
> needed outside test lab? Am I missing something?

As with the -test actions, this allows someone to diagnose whether a problem they are having is with the formatting 
of the input key (hex, padded, ASCII, base64, etc.). This is by far one of the most common problems when attempting to
get different implementations to interoperate.

Juliusz said:
> The key length is not carried in the clear by the protocol.  Guessing the
> key length requires a brute-force attack, even when it is zero.

I propose no change. 

Juliusz said:
> Yes.  Babel-DTLS is intended to inherit the security properties of the
> system's DTLS library.  If the DTLS library is unsafe, then Babel-DTLS
> must not be used until the library is fixed.

---

From: "Valery Smyslov" <[email-masked]>
Date: Thu, 5 Nov 2020 11:30:50 +0300

Hi Barbara,

> Hi Valery,
> Thanks for your review. Taking into account Juliusz' response to your email, I'm proposing the following
> resolutions. For the first issue raised, I'm disagreeing slightly with Juliusz, so that may need further discussion.
> Please let me know if you disagree. I've copied Juliusz' responses here, so I can include my responses to your
> complete feedback and his in one email.
> Roman specifically mentioned he agreed with your "recommendations about precise language around the
> names of the parameters". I think perhaps this was in reference to item 4 under nits? I didn't completely agree
> with your recommendation, so I'd appreciate some additional discussion on that.
> Thx,
> Barbara
> 
> The defined values for babel-mac-algorithms come directly from draft-ietf-babel-hmac. The defined value
> names should map closely to the names used for the algorithms in in that draft -- which they currently do.
> 
> If it needs to be explicitly stated somewhere that an implementation of draft-ietf-babel-hmac with BLAKE2s
> outputs 128-bit MACs, then draft-ietf-babel-hmac (which was already submitted for publication) would be the
> correct place to say that. The information model is not the right place, unless there's some expectation for the
> size to be configurable or reportable. I'm not seeing any request for the MAC size to be configured or reported
> via the information model.
> 
> I'm proposing no change to the defined values of babel-mac-algorithms in order to maintain complete
> consistency with the names used in draft-ietf-babel-hmac-12.

My point was that the intent Juliusz mentioned (that Blake2s is used with 32-octet keys and 16-octet
hashes) must be documented somewhere. If you think it must be in the draft-ietf-babel-hmac, 
I'm fine with this, but currently I cannot find any such requirement anywhere.

> One of the most common problems in configuring security mechanisms is in the format of the input key (hex,
> ASCII, base64, hashing that occurs to create "actual key", etc.). When a security mechanism fails to work, it is
> important for users or device managers to be able to trouble-shoot this specific point of failure. This test
> allows the user/manager to see if what this device thinks the MAC should be is the same as what another
> device thinks the MAC should be or is the same as the MAC being sent on the wire. Many ISPs have built a test
> like this into their ISP-supplied CE routers (invoked using the TR-069 protocol and TR-181 data model) to test
> various stored key values. It has proven useful.

I'm still confused. Are we talking about MACs or about certificates for DTLS?
I have no problems with text describing test for MAC keys. The text I'm having problem with 
is about testing certificates for DTLS. The test it describes is not clear for me: 
it suggests to perform SHA2-256 hash of an input string "using the certificate public key".
It is unclear for me how you would use the certificate public key to produce a hash 
of some input string. So I believe the test should be clarified.

>   section of [ID.draft-ietf-babel-hmac] for additional considerations

I would suggest additionally using "SHOULD NOT" for weak keys. 
How about the following new text: 

    MAC keys are allowed to be as short as zero-length.  This is useful
    for testing.  Network operators are RECOMMENDED to follow current best
    practices for key length and generation of keys related to the MAC
    algorithm associated with the key.  Short (and zero-length) keys and
    keys that make use of only alphanumeric characters are highly
    susceptible to brute force attacks and thus SHOULD NOT be used. 
    See the Security Considerations section of [ID.draft-ietf-babel-hmac] 
    for additional considerations related to MAC keys.

(note that "SHOULD NOT" still allows people to shoot in their feet if they want to). 

> Thank you. I will make this change.

Thank you.

> The authors have double- and triple-checked, but human error is still possible, since these were not auto-
> generated (like YANG does). Whenever redundant information is presented, it is critical that it be normative in
> only one place. This language makes it clear which is normative and what to do in the case of discrepancies. I
> propose no change. If there is a strong push to remove this language, then I believe the correct approach
> would be to remove the entire section (remove the redundancy) rather than to allow confusion if there is an
> error. But people have found this section useful, so I would prefer to leave it as is.

OK.

> Thank you. I'll change it to MAC.

Thanks.

> I've been thinking through this. I can't speak to the informal nature of "sign", but I can say that simply
> replacing "sign" with "compute" or "mac" wouldn't convey correctly what this parameter is about. This
> parameter is primarily concerned with whether or not a MAC is included in the sent packet. The sending is the
> critical piece, and not the computing (it's possible to compute the MAC without sending it; a MAC in a sent
> packet is assumed to have been computed). I could change the description to:

I'm fine with this.

> But I struggle with the proposed parameter renaming. I strongly believe the name should concisely describe
> that the Boolean value indicates whether or not to include a MAC in the sent packet. The term "sign" is one
> I've commonly seen to indicate that a MAC is included in the sent packet. I'm not aware of a different,
> similarly short word. "Compute" and "mac" do not convey the sending aspect. And sending is very
> asymmetric.

How about "use"?

> I recommend no changes to this text.

I can live with this if you add "SHOULD NOT" for zero-length keys in the Security Consideration
(as I suggested above).

> If this is right, I'll make that change.

Great, thank you.

> Thank you. I'll make this change.

Thanks.

> get different implementations to interoperate.

OK, but I'd rather still add "SHOULD NOT" for using them as I suggested above.

> > key length requires a brute-force attack, even when it is zero.

OK.

> > must not be used until the library is fixed.

Regards,
Valery.