Locator/ID Separation Protocol Security (LISP-SEC)
draft-ietf-lisp-sec-29

# ART AD Review of draft-ietf-lisp-sec-26

cc @fpalombini

Thank you for the work on this document.

I only have a couple of very minor comments, easy to fix - feel free to take them or leave them.

Francesca

## Comments

### DTLS reference

Please add a reference to RFC 9147 on the first occurrence of DTLS in the text.

### + undefined

Section 6.5:
```
       *  per-msg-key = KDF( nonce + s + PSK[Key ID] )
```
It should be explicitly stated in the text what operations "+" denotes in this specification.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues.

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

I generally found this a well-written and pleasant to read document. Thanks for the conversation around my earlier DISCUSS. I have a number of additional questions, comments, and suggestions, below.

1. In §6.4, 

   The ITR MAY use the KDF ID field to indicate the recommended KDF
   algorithm, according to local policy.  
   
I guess the use of MAY is there because the ITR could also put 0 in that field which we are later told (five paragraphs down) means "no preference" (and not literally no KDF at all, as a plain reading of the string "NONE" in Table 6 would imply).

A few questions and suggestions arise from this:

a. If the ITR puts 0 in that field, and the Map-Server updates it, how should this text in §6.9 be interpreted?

                      If the KDF ID in the Map-Reply does not match the
   KDF ID requested in the Map-Request, the ITR MUST discard the Map-
   Reply
   
I guess the ITR didn't "request" any particular KDF ID when it sent 0, so I guess I can disregard the text I've quoted -- but it's not very clear. Certainly a plain field comparison without special-casing 0 would fail this test. I think there should be some update to clarify this case.

b. But what happens if the Map-Server puts in a KDF ID for a KDF the ITR doesn't support, even if the ITR did send 0? I presume that even though the ITR said "no preference" it still can't process the packet, so it has to discard the Map-Reply just as if it had gotten back a genuinely non-matching KDF ID. If this is so, I think there should be some update to clarify.

c. It seems like the string "No Preference" would be better than "NONE" in Table 6, similar for Table 4.

Similar comments apply to "The Requested HMAC ID field contains the suggested HMAC algorithm" (§6.4) and "If the EID HMAC ID field does not match the Requested HMAC ID" (§6.9).

2. Related to the previous, in Section 6.4, there are two fairly widely separated paragraphs:

   The ITR MAY use the KDF ID field to indicate the recommended KDF
   algorithm, according to local policy.  The Map-Server can overwrite
   the KDF ID if it does not support the KDF ID recommended by the ITR
   (see Section 6.7).

And

   The KDF ID field specifies the suggested key derivation function to
   be used by the Map-Server to derive the MS-OTK.  A KDF Value of NONE
   (0) may be used to specify that the ITR has no preferred KDF ID.

I think it would be better if these were combined, as they seem to be redundant and in any case cover the same subject. 

3. For each of the fields whose value is taken from an IANA registry, I think it would be helpful to reference the registry where the field is defined, for example in Section 6.1,

      KDF ID: Identifier of the Key Derivation Function used to derive
      the MS-OTK.  Refer to Section 6.7 for more details.

Could be

      KDF ID: Identifier of the Key Derivation Function used to derive
      the MS-OTK.  Permitted values are registered in the Key 
      Derivation Function registry (see Section 8.5). Refer to 
      Section 6.7 for more details.

Similarly for all five registries defined here and their respective fields.

4. In §6.5,

   It's important to note that, to prevent ETR's overclaiming attacks,
   the ITR/Map-Resolver pre-shared secret MUST be different from the
   Map-Server/ETR pre-shared secret.

This is a bit picky, but wouldn’t “independent from” be more accurate? Strictly speaking to guarantee that it be different, they’d have to collude (to check for uniqueness), which is exactly what you want to prevent, right?

5. In §6.9, this paragraph is problematic in a few ways,

   In response to an encapsulated Map-Request that has the S-bit set, an
   ITR MUST receive a Map-Reply with the S-bit set, that includes an
   EID-AD and a PKT-AD.  If the Map-Reply does not include both ADs, the
   ITR MUST discard it.  In response to a Protected Map-Request, an ITR
   expects a Map-Reply with the S-bit set to 1 including an EID-AD and a
   PKT-AD.  The ITR MUST discard the Map-Reply otherwise.

The MUST in the second line is misused -- you aren't telling the ITR what it MUST do, you're expressing an expectation that if the other entities are forming their messages per-spec, the Map-Reply will be formed as you state. You already have more actionable language later in the paragraph to cover this case, the "MUST discard" part.

Beyond that, the paragraph is redundant, it says everything twice. If you throw away the redundant text, I think the problem I'm complaining of goes away, for example you could cut it down to:

   In response to a Protected Map-Request, an ITR
   expects a Map-Reply with the S-bit set to 1 including an EID-AD and a
   PKT-AD.  The ITR MUST discard the Map-Reply otherwise.

6. In §6.9, the phrase "the first opportunity it needs to" is used several times, for example,

   If the EID HMAC ID field does not match the Requested HMAC ID the ITR
   MUST discard the Map-Reply and send, at the first opportunity it
   needs to, a new Map-Request with a different Requested HMAC ID field,
   according to ITR's local policy.  
   
I don't understand the "needs to" part, I suspect there is some nuance going on here, of trying to shoehorn LISP-SEC into the control plane in some backward-compatible way, but if so the chosen language is too subtle to be unambiguous.

To try to work through some of the possibilities --

- Leaving out the entire clause "at the first opportunity it needs to" would be clear. It would mean that the ITR is supposed to retry right away. Which seems fine, but is probably not what you're going for (else why have that clause at all).

- Leaving out the words "it needs to" would be the same as the above.

- So, maybe what's being said here is something like, "we expect that an event on the ITR will trigger it to retry anyway, and we're just piggybacking onto that event"?

If that's right, I guess what the ITR is expected to do is something like:

- When it needs to look up a given EID, launch a Map-Request.
- If it gets back a Map-Reply with a different HMAC ID or KDF ID from what it sent (except, maybe not in the case where it sent 0, see my earlier question), do something like keep a notation in a local cache, that says "next time you send a Map-Request for this EID, try this other KDF ID".
- Presumably in the mean time it might have dropped some packets destined to that EID, depending on other details of the deployment.
- At some point it needs to look up that EID again (maybe because it has another packet to deliver) and before launching a new Map-Request it consults its cache and says "aha, I need to use a HMAC ID/KDF ID other than my default one".

The fact that I'm guessing about all this suggests that the spec may not be clear enough on these points. (Or that I'm not reading carefully enough, of course, in which case please point me to the parts I've missed that make it clear.)

7. Nit, §6.9,

   [I-D.ietf-lisp-rfc6833bis] allows ETRs to send Solicited-Map-Requests

Should be "Solicit", not "Solicited".

8. In §6.9,

                                           If an ITR accepts Map-Replies
   piggybacked in Map-Requests and its content is not already present in
   its EID-to-RLOC cache, it MUST send a Map-Request over the mapping
   system in order to verify its content with a secured Map-Reply.
   
Does this mean,

a. The piggybacked Map-Reply will be put into use immediately by the ITR, but a verification will be initiated with a secured Map-Reply? How long is the piggybacked one to be used? What happens if the secured transaction never completes? If this is the intention, it needs to be clarified and probably also discussed in the Security Considerations, because it's not hard to imagine an attacker abusing it. Or is it,

b. The piggybacked Map-Reply is not permitted to be used until it has been verified by a secured Map-Reply? It would be considerably less problematic in terms of security analysis (it's functionally almost the same as the operation of LISP-SEC without piggybacked replies) but less optimal of course.

In either case the spec needs to be clarified.

9. There are four places in §6.9.1 where you say "a log action MUST be taken". I wonder if these unintentionally open a resource exhaustion attack vector, since an attacker might be able to cause an ITR to log like crazy, and logging is often an expensive operation. I guess it depends on just how nuanced you expect the implementor to be in construing what a "log action" is -- if I decide to construe "log action" to implicitly include "with rate limiting" then that's potentially fine, but if I'm a naive implementor and just syslog every time the spec tells me to take a log action, I may be setting myself up as a soft target.

Maybe SHOULD instead of MUST would be a middle ground?

10. By the way, none of the "Receiving a Map-Reply" text talks about other checks. For example, before doing any of the checks specified in §6.9 one might imagine an implementation would first confirm it's even expecting this Map-Reply (presumably looking for a matching unfulfilled Map-Request), and throw it away if not. I briefly looked to see if that's in rfc6833bis but without success. Is the assumption that the checks in §6.9 are performed after other checks that are specified in the base control plane?

11. In §7.6, Replay Attacks, you say

   In case of replayed Map-Request, the Map-Server, Map-Resolver and ETR
   will have to do a LISP-SEC computation.  This is equivalent, in terms
   of resources, to a valid LISP-SEC computation and an attacker does
   not obtain any benefit, since the corresponding Map-Reply is
   discarded as previously explained.

How does the attaker "not obtain any benefit"? Suppose the benefit the attacker is trying to obtain, is to DoS the mapping system or ETR -- aren't they able to use these replayed Map-Requests to force these entities to do work?

12. In §7.7 you say

   DTLS [RFC9147] SHOULD be used to provide communication privacy and to
   prevent eavesdropping, tampering, or message forgery to the messages
   exchanged between the ITR, Map-Resolver, Map-Server, and ETR, unless
   the OTK is encrypted in another way, e.g. using a pre-shared secret.

Elsewhere in the spec this is a MUST, for example §6.5:

   2.  If the NULL-KEY-WRAP-128 algorithm (see Section 8.4) is selected
       and DTLS is not enabled, the Map-Request MUST be dropped and an
       appropriate log action SHOULD be taken.

Probably in §7.7 you should similarly say MUST, then?

13. There are many Specification Required registries that don't have guidance to designated experts, as requested by RFC 8126 §4.5 (which is referenced by RFC 8126 §4.6, which says Specification Required is just a fancy version of Expert Review). Likewise there's no field for change controller. Please consider adding these.

Thanks for dealing with my IANA-related DISCUSS issues.

I concur with John; this was generally well-done and easy to understand.  Nice work.  A couple of suggestions:

In Section 6.1 has:

   E: ETR-Cant-Sign bit.  This bit is set to 1 to signal ...

I think you mean "If this bit is set to 1, it signals ..." or something similar.  Taken literally, the current text means you always set it to 1, but I don't think that's what you meant to say.

I think the fifth paragraph of Section 6.4 is missing a period or something.  I found it hard to parse toward the end.

My DISCUSSes were addressed in version 28/29

Old DISCUSSES:

Note I support the DISCUSSes and COMMENTS of Roman and Murray, and John's comments, and I won't repeat those issues in this review.


#1

The document keeps talking about generating OTK's which are One Time Key's,
and then "securely transports" these. If we have such a secure transport,
why can't this same mechanism be used by the Map-Server for the Map-Request
and Map-Reply message security instead of OTKs?

Possibly I am not understanding the full architecture? An ascii art diagram
would be useful at the beginning of the document.

This all kind of tastes like Kerberos/GSSAPI. Couldn't that be used instead?

Why not just use mTLS between all parties and get rid of all the custom crypto
with OTK's ?

#2

   LISP-SEC deployments SHOULD use AUTH-HMAC-SHA-
   256-128 HMAC function, unless older implementations using AUTH-HMAC-
   SHA-1-96 are present in the same deployment

It makes be sad that 1 older device downgrades the usable algorithm for
all nodes. Would it be possible to change the protocol slightly so those
with sha2 support could use this and only the sha1-only node uses the
old algorithm?

#3

   or by enabling DTLS [RFC9147] between the ITR and the Map-Resolver.

Should this clarify that the DTLS connection should be mutually authenticated?
eg both peers must identify themselves to the other, unlike the more common
(D)TLS connections where only the client authenticates the server. This applies
to multiple locations where it says DTLS can be used.

#4

The Registry "LISP-SEC Authentication Data HMAC ID" seems to really be
conveying a _preference_. Should this be reflected in the name of the
registry? Additionally, can we leave value 0 as Reserved, and make NOPREF
the value 1, etc.

The "Key Wrap Functions" registry specifies 0 as Reserved, but then
associated a KEY WRAP and KDF with this value. That is, it combines
a "Reserved" with a "NULL wrap" entry. These two should be clearly
split - eg reserve 0  with Key wrap and KDf set to "N/A", and if
needed create a "null-wrap-null if an entry is needed with key wrap
and kdf set to "none".

The "Key Derivation Functions" also mixes up NOPREF and Reserved.



COMMENTS:


#1

       This ITR-OTK is included
       into the Encapsulated Control Message (ECM) that contains the
       Map-Request sent to the Map-Resolver, and encrypted.

Is it "encrypted and included"? Or included and the entire ECM encrypted?
I can't parse the "and encrypted" here.

NITS:

Personal pet peeve:  I dislike using +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
and prefer +------+---------+ style lines as it far less distracting and
christmas tree like.

Thank you to Alexey Melnikov for the SECDIR review.

Thank you for addressing my DISCUSS and COMMENT feedback.

Thanks for working on this document. I have skimmed through the document and haven't notices transport related issues.

# Éric Vyncke, INT AD, review of # Éric Vyncke, INT AD, review of draft-ietf-lisp-sec-26
CC @evyncke

Thank you for the work put into this document.

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education).

Special thanks to Luigi Iannone for the shepherd's detailed write-up including the WG consensus and the justification of the intended status. 

I hope that this helps to improve the document,

Regards,

-éric

## COMMENTS

### Section 5, trusts relationships

This section mentions 'trust relationships', but do not explain how those are created ? A forward reference would be welcome (e.g., to section 7.5 but even this is rather weak).

### Section 5, decrypting something that was not encrypted
```
   1.  The ITR, upon needing to transmit a Map-Request message,
       generates and stores an OTK (ITR-OTK).  This ITR-OTK is included
       into the Encapsulated Control Message (ECM) that contains the
       Map-Request sent to the Map-Resolver.
```

Based on the text following this bullet, should the ITR-OTK also be encrypted (as it is decrypted in step 2) ?

### Section 7.5

Are the shared keys per ITR Map-resolver pair or are they shared by *ALL* ITR and the Map-resolver(s). It is probably the former as the latter would be a huge threat of impersonation among ITR. Should there be some text about this ?

### Performance impact of LISP-SEC

Did the authors have an estimate on the performance impact (crypto operations, increased size of the messages) of LISP-SEC? Should there be a section about this potential impact ?


## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. 

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments