Re: [Hipsec] Secdir last call review of draft-ietf-hip-dex-06

[Miika Komu <[e-mail]>]

Hi David,

I am stepping in and helping the authors to finish the draft as agreed 
with the authors and HIP WG chair. I am not actively working on the 
topic, so I have very limited time for this but I have earlier 
background in HIP. Please let me know if the following comments and 
edits address your concerns?

On 2/23/18 04:01, David Waltermire wrote:
> Reviewer: David Waltermire
> 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 summary of the review is Ready with issues.
> 
> In general this document is clearly-written and well-organized. It was a fun
> read overall.
> 
> I have the following concerns with the draft:
> -----------------------------------------------------------
> 
> Section 2.1:
> 
> You should use text from RFC8174 to indicate that lowercase versions of the
> keywords are not normative.
> 
> Something like the following would work:
> 
>     The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
>     "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and
>     "OPTIONAL" in this document are to be interpreted as described in BCP
>     14 [RFC2119] [RFC8174] when, and only when, they appear in all
>     capitals, as shown here.

fixed.

> Section 4.1.3.1:
> 
> "it can be long-lived with no need for rekeying" Small is open to
> interpretation. It would be useful to include some guidance on the expected
> amount of data to be exchanged before rekeying would be needed, or why this is
> a practical impossibility.

rekeying is subject to local policies, but a hard limit exists. I added 
the following sentence:

At the latest, the system MUST initiate
rekeying when its incoming ESP sequence counter is going to overflow,
and he system MUST NOT replace its keying material until the rekeying
packet exchange successfully completes as described in section 6.8 in
[RFC7402].

> Section 5.3.2 and 5.3.3:
> 
> In the paragraph on TRANSPORT_FORMAT_LIST, it would be good to document the
> specific ESP parameter value to be used from:
> https://www.iana.org/assignments/hip-parameters/hip-parameters.xml#transport-modes.
> This will remove any ambiguity.

I changed the sentence referring to the parameter value to be more specific:

The different format types are DEFAULT, ESP and ESP-TCP as explained in 
Section 3.1 in [RFC6261].

The actual choice(s) depend on local policy, I don't think we should 
mandate a specific one here.

> Section 6.6:
> 
> In items #5 and #6, what is "an acceptable time span"? Some guidance here would
> be helpful. I believe this is discussed earlier in the draft. Perhaps a
> reference back may provide some clarity?

this extension builds on RFC7401 which does give any explicit time 
values either:

https://tools.ietf.org/html/rfc7401#section-6.8

So I am a bit hesitant to add explicit timeouts here.

> Section 6.9:
> 
> In #1, under what circumstances would the NOTIFY packet not be dropped
> silently? Why is this not a MUST? Some explanation would be useful here. In
> general, many of the SHOULDs in the section 6 subsections, could use further
> justification.

good point, I changed the explicitly mentioned SHOULD to MUST.

Some of SHOULDs in section 6 are inherited from RFC7401. I would like to 
go through all of the SHOULDs in section 6 but it will delay the already 
delayed process for this draft. If we change some of the SHOULDs to 
MUSTs, I believe we need also WG concensus. So, as a quick resolution I 
just added the following note to section 6 intro:

It should be noted that many of the packet processing rules are denoted 
here with "SHOULD" but may be updated to "MUST" when further 
implementation experience provides better guidance.

...which I believe reflects the current reality.

> Section 8:
> 
> What is "a reasonable delta time"? Some guidance here would be useful.

good catch. The situation is actually related to sending of I1 packets, 
so I would actually change the time delta to a packet counter similarly 
as in RFC7401:

As an adversary could also send such an ICMP
packet in a man-in-the-middle attack with the aim to prevent the HIP
DEX handshake from completing, the Initiator SHOULD NOT react to an
ICMP message before retransmission counter reaches I1_RETRIES_MAX in
its state machine (see Table 3 in [RFC7401]).

> Section 9 (Security Considerations):
> 
> "The puzzle mechanism using CMAC may need further study regarding the level of
> difficulty." Study of what? Is the concern here that the impact on constrained
> devices at a higher level of difficulty is not well understood? Or is this
> concern around identifying best practices around raising the difficulty under
> specific conditions? A sentence or two on this would be helpful for the reader
> to better understand the issue.

It means finding puzzle values that don't exhaust computational 
resources with sensors. I changed the sentence to:

"The puzzle mechanism using CMAC may need further study regarding the

level of difficulty in order to establish best practices with current

generation of constrained devices."

> I don't see a mention of the non-protected Host Identity issue from section 1.1
> here.

I added an extra bullet:

Contrary to HIPv2, HIP DEX does not provide for end-point
anonymity for the Initiator or Responder.  Thus, any signaling
that indicates such anonymity should be ignored as explained in
Section 1.1.

> With regards to the 4th bullet, the text in section 3 should be referenced
> regarding HIT collisions. (nit)-> Referencing back to the relevant sections for
> the other bullets may also be useful.

Done!

>>From section 5.3.1, I don't see a mention of the issues around dealing with I1
> storms addressed here.

Added:

    Section 5.3.1 mentions that implementations need to be able to handle
    storms of I1 packets.  Contrary to HIPv2, R1 packets cannot be pre-
    computated in HIP DEX and also the state machine does not include an
    "R1_SENT" state (that would enable caching of R1 packets).
    Therefore, an implementation has to cache information (e.g., at least
    the HITs) from incoming I1 packets and rate control the incoming I1
    packets to avoid unnecessary packet processing during I1 packet
    storms.

> Section 10:
> 
> It can be useful for IANA to reference the specific registry by name and URL in
> the IANA considerations. It can also be useful to include the actual table in
> the IANA considerations section. These are embedded in section 5 in the current
> document. Suggest moving these tables to the IANA consideration section.

I think it would break the flow of the text. Instead, I double checked 
that all new parameters are listed in the IANA section and replicated 
the type values there.

> I found the following nits in the draft:
> -------------------------------------------------
> 
> Section 1.1:
> 
> In the text "any signaling that indicates such anonymity should be ignored" it
> would be useful to provide an example of such signaling.

good point, I modified the description as follows:

...and any signaling (i.e., HOST_ID parameter contained with an 
ENCRYPTED parameter) that...

> /may carry data payload/may carry a data payload/

fixed

> The packets are referred to as 1st, 2nd, 3rd, and 4th here, and as I1, R1, I2,
> and R2 in section 4. Consider use a consistent naming approach throughout this
> document to improve clarity.

Agreed, fixed.

> Section 1.2:
> 
> Section 8 is not included in this summary. Suggest adding a sentence about it.
>
> (nit) Section 10 is not linked as well.

thanks, fixed.

> Section 2:
> 
> /Terms and Definitions/Terms, Notation, and Definitions/

fixed

> Section 3:
> 
> "other methods are used to map the data packets to the corresponding HIs" What
> are these other methods? What if ESP is not used and the SPI is not an option?

I added to the end of this paragraph:

When other user data packet formats are used, the
corresponding extensions need to define a replacement for the
ESP_TRANSFORM [RFC7402] parameter along with associated semantics,
but this procedure is outside the scope of this document.

> Section 4.1.2.3:
> 
> In the diagram, I think there is a missing arrow between I2-SENT and R2-SENT.
> Please double check. Also, this diagram is far from simple. Maybe name this
> section "HIP State Diagram"?

There is an arrow from I2-SENT to R2-SENT and the state diagram is 
identical to RFC7401. I would prefer to keep the title the same as in 
RFC7401, "Simplified State Diagram", although I agree it is not so simple :)

> Section 4.1.3.2:
> 
> "Even though this input" Please clarify the "this" indefinite article.

fixed (...the concatenated input...)

> Section 4.1.4:
> 
> /This will limit state/Using non-volatile storage will limit state/
> 
> This section should reference section 6.11, since some of the content is
> duplicated there.

thanks, fixed as suggested

> Section 5.2.3:
> 
> /It is defined in/The HOST_ID parameter is defined in/

thanks, fixed

> Section 5.2.5:
> 
> /at least 64 bit/at least 64 bits/
> 
> Update the reference "#I and the puzzle solution #J (see [RFC7401])" to point
> to section 4.1.2 in RFC7401.

done

> Section 5.3.2:
> 
> The discussion of difficulty K touches on a local policy issue that is
> discussed in section 7. It could be useful to reference section 7 from here.
> 
> Update "(see [RFC7401])" to point to section 4.1.2 in RFC7401.
> 
> /based on which it chose the ECDH/based on which the Responder chose the ECDH/

fixed both of these.

Thanks for insightful and very technically detailed comments! Please let 
me know if this discussion resolves your concerns.