Re: [Hipsec] Benjamin Kaduk's Discuss on draft-ietf-hip-native-nat-traversal-30: (with DISCUSS and COMMENT)

[Miika Komu <[e-mail]>]

Hi Benjamin,

ti, 2020-03-03 kello 20:09 -0800, Benjamin Kaduk via Datatracker
kirjoitti:
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-hip-native-nat-traversal-30: 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 IESG DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-hip-native-nat-traversal/
> 
> 
> 
> -------------------------------------------------------------------
> ---
> DISCUSS:
> -------------------------------------------------------------------
> ---
> 
> One fairly minor point to start: Section 4.3 says that we define a
> new
> mode (ICE-HIP-UDP) for the NAT_TRAVERSAL_MODE parameter type, but
> then
> goes on to say that "the presence of the parameter in a HIP base
> exchange means that the end-host supports NAT traversal extensions
> defined in this document".  If I undrestand correctly, only the
> specific
> presence of the ICE-HIP-UDP mode of the NAT_TRAVERSAL_MODE parameter
> does so, and so to say that the presence of "the [NAT_TRAVERSAL_MODE]
> parameter" indicates support for this document would be backwards
> incompatible with RFC 5770.
> 
> I'd also like to delve a little further into the potential
> "cross-protocol" attack (same protocol, really, but the same attack)
> that Ekr raised, between RVS_HMAC and RELAY_HMAC.  This is probably a
> "discuss discuss", so let's see where it leads...
> 
> The semantics for either type of HMAC is that it is an HMAC over the
> HIP
> packet excluding itself and subsequent parameters.  Pulling up the
> HIP
> packet format from RFC 7401, that looks like:
> 
>     0                   1                   2                   3
>     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    | Next Header   | Header Length |0| Packet Type |Version| RES.|1|
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |          Checksum             |           Controls            |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                Sender's Host Identity Tag (HIT)               |
>    |                                                               |
>    |                                                               |
>    |                                                               |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |               Receiver's Host Identity Tag (HIT)              |
>    |                                                               |
>    |                                                               |
>    |                                                               |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                                                               |
>    /                        HIP Parameters                         /
>    /                                                               /
>    |                                                               |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> The HMAC key is the integrity key for that direction of traffic
> between
> HITs, so the "cross-protocol" part can only come in by confusing the
> packet recipient into confusion as to whether it is processing an
> RVS_HMAC or a RELAY_HMAC (but any other entity will reject the packet
> by
> virtue of it using the wrong key).  Modern best practices are to go
> through a key derivation step that incorporates as much information
> as
> possible about what the derived key will be used for, which would in
> this case include the TLV type of the HMAC parameter and presumably
> the
> HITs in question as well.
> 
> In particular, the TLV type of the HMAC parameter is *not* input into
> the HMAC calculation (at least for RVS_HMAC), so the trivial
> discriminator is not present.  The "packet type" in the header in the
> header is potentially going to differ across usages, so I think
> that's a
> good place to focus discussion.  Unfortunately, Section 4.2.1 of RFC
> 8004 suggests that RVS_HMAC is going to be present a lot of the time,
> so
> it's not really clear to me what packet types either RVS_HMAC and/or
> REPLAY_HMAC are expected to occur in.  Could a HIP expert please jump
> in
> and help clarify?

let's iterate this further. I think the security concern here is
generic but no specific attack scenario. At this point of
standardization,
I am bit hesitant to do any drastic changes to this unless there is
some
clear attack.

So the parameters can be in the following messages:

* RELAY_HMAC parameter can be in I1 or I2, UPDATE or CLOSE message
* RVS_HMAC parameter can be in I1 (or UPDATE)

Relay or RVS server appends the corresponding parameter when it passes
a packet to a
registered client (the reverse does not apply) as follows:

      1. MSG                  2. MSG+HMAC
Peer -----> RVS/ Control Relay ------> RVS/Control Relay Client

I don't see how the peer attacker could benefit, e.g., by replaying
those packets and trying to confuse the rvs/relay client about whether
it was RVS or RELAY HMAC.

A client might register both to RVS and Control Relay service for the
same HIP middlebox. However, if the peer the above figure initiates a
base exchange with I1 message over UDP, then you get Control Relay
functionality for the remainder of the session. If the peer sends the
I1 message without UDP encapsulation, the remainder of the session will
employ RVS. And you cannot switch from non-UDP mode to UDP or vice
versa.

So perhaps the problem might a "rogue" implementation offering both RVS
and Control Relay service over UDP. To make sure this is not
acceptable, I have added the following statement in the end of the
following section:

4.1.  Relay Registration

   [..]

   To avoid cross-protocol attacks associated with RELAY_HMAC and
   RVS_HMAC, a Control Relay Server MUST NOT offer RENDEZVOUS
   registration service [RFC8004] to any Control Relay Client.

Does this address your concern?

> -------------------------------------------------------------------
> ---
> COMMENT:
> -------------------------------------------------------------------
> ---
> 
> [My latest reply to the ballot thread for my previous ballot
> position,
> 
https://mailarchive.ietf.org/arch/msg/hipsec/iP_of3P2RfxJIbeVDz2qGonO8nY/
> , seems to have not gotten a response]

I did:


https://mailarchive.ietf.org/arch/msg/hipsec/38L3_sKwOPIJs-otFUbelyzEwAo/

> Why did some rfc5245bis references get converted to RFC5245 and
> others
> to RFC8445?

The first and only RFC5245 reference is associated with RFC5770.
This is correct because RFC8445 did not exist back when RFC5770 was
published.

> The phrase "Control Data Relay" occurs just five times in the
> document
> (vs. over a hundred for "Control Relay") and seems to needlessly
> conflate "Control Relay" and "Data Relay".  I'd suggest rewording to
> avoid this phrase.

good catch. No such thing as "Control Data Relay". This should be
"Control Relay Server" in all but the first case that I changed as
follows:

- The Relay Client registers for the Control Data Relay
+ The Relay Client registers for the Control/Data Relay

> Abstract
> 
>    and UDP encapsulation of data and signaling traffic.  The main
>    difference from the previously specified modes is the use of HIP
>    messages instead of ICE for all NAT traversal procedures due to
> its
>    kernel-space dependencies.
> 
> It might be worth disambiguating what "its" refers to.

this is now changed now to "...due to the kernel-space dependencies of
HIP".

> Section 1
> 
>    messages tunneled over a single UDP flow.  The benefit of using
> ICE
>    and its STUN/TURN messaging formats is that one can re-use the NAT
>    traversal infrastructure already available in the Internet, such
> as
>    STUN and TURN servers.  Also, some middleboxes may be STUN-aware
> and
>    may be able to do something "smart" when they see STUN being used
> for
>    NAT traversal.
> 
> Don't we go on to say that we can't actually use stock TURN
> infrastructure since we'd need extra encapsulation for the HIP bits?

I am not sure how to fit this detail fluently here to the text but
Appendix B. already already mentions this:

"Relaying of ESP packets via TURN relays was not considered that simple
because TURN relays require adding and removing extra TURN framing for
the relayed packets."

> Section 2
> 
>    HIP offer:
>       Before two end-hosts can establish a communication channel
> using
>       the NAT traversal procedures defined in this document, they
> need
>       exchange their locators (i.e. candidates) with each other.  In
>       ICE, this procedure is called Candidate Exchange and it does
>       specify how the candidates are exchanged but Session
> Description
> 
> nit: s/does specify/does not specify/

thanks, fixed.

>       handover.  Following [RFC5770] and Session Description Protocol
>       (SDP) [RFC3264] naming conventions, "HIP offer" is the the
> 
> nit: we just expanded SDP a few lines previously; we probably don't
> need
> to do it again.

thanks, fixed.

> Section 3
> 
>    connected to the public Internet.  To be contacted from behind a
> NAT,
>    at least the Responder must be registered with is own Control
> Relay
>    Server reachable on the public Internet.  The Responder may have
> also
> 
> nit: "is own" is not right; I'm not sure whether "his own" or "its
> own"
> was intended (but the "a Control Relay Server in the -28 didn't seem
> wrong to me either).

changed to "a Control Relay Server"

> Section 4.1
> 
>    In step 3, the Relay Client selects the services for which it
>    registers and lists them in the REG_REQ parameter.  The Relay
> Client
>    registers for the Control Data Relay service by listing the
>    RELAY_UDP_HIP value in the request parameter.  If the Relay Client
>    requires also ESP relaying over UDP, it lists also RELAY_UDP_ESP.
> 
> (This is one of the five "Control Data Relay"s I mention previously.)
> I'd suggest using "Data Relay Service" in combination with
> RELAY_UDP_ESP
> to reinforce the link between the two terms.  The current "mix and
> match" of "Control [...] Relay" and "RELAY_UDP_ESP" requires more
> effort
> to understand than using a uniform set of terminology would.

as mentioned earlier, "Control Data Relay" should have been
"Control Relay" here:

   The Relay Client
   registers for the Control Relay service by listing the RELAY_UDP_HIP
   value in the request parameter.  If the Relay Client requires also
   ESP relaying over UDP, it lists also RELAY_UDP_ESP.

I think the reinforcement link is now there?

>    ECHO_REQUEST_SIGNED . In case the Data Relay Server admitted a new
>    relayed UDP port for the Data Relay Client, the REG_RES parameter
>    MUST list RELAY_UDP_ESP as a service and the UPDATE message MUST
> also
>    include a RELAYED_ADDRESS parameter describing the relayed UDP
> port.
> 
> nit: "admitted" is a surprising word choice, to me.

changed to "allocated"

> Section 4.2
> 
>    local preference value MUST be unique.  Dual-stack considerations
> for
>    IPv6 apply also here as defined in [RFC8445] in section 5.1.2.2.
> 
> It looks like this should be 5.1.2.1, not 5.1.2.2.

good catch, corrected (the section numbering changed in some ICE-bis
version)

>    where Ta is the value used for the connectivity check pacing and
> Num-
>    Of-Cands is the sum of server-reflexive and relay candidates.  A
>    smaller value than 500 ms for the RTO MUST NOT be used.
> 
> nit: I don't think addition is defined on (set-of-server-reflexive
> candidates, set-of-relay candidates); presumably there's some missing
> "number of"s here.

I changed this as follows:

"Num-Of-Cands is the number of server-reflexive and relay candidates."

> Section 4.3
> 
>    In step 4, the Responder concludes the base exchange with an R2
>    packet.  If the Initiator chose ICE NAT traversal mode, the
> Responder
> 
> nit(?): s/ICE NAT/ICE-HIP-UDP/?

thanks, corrected as suggested.

> Section 4.5
> 
>    repeated here.  Similarly, the NAT traversal mode negotiation
> process
>    (denoted as NAT_TM in the example) was described in Section 4.3
> and
>    is neither repeated here.  If a Control Relay Server receives an
> R1
> 
> nit: AFAIK this is not a correct usage of "neither".

Changed to "and is also not repeated"

>    It is RECOMMENDED that the Initiator send an I1 packet
> encapsulated
>    in UDP when it is destined to an IPv4 address of the Responder.
> 
>    Respectively, the Responder MUST respond to such an I1 packet with
> a
>    UDP-encapsulated R1 packet, and also the rest of the communication
>    related to the HIP association MUST also use UDP encapsulation.
> 
> Is this Responder behavior also restricted to IPv4?

no, changed this to "IP address"

>    Server.  The Control Relay Server protects the I1 packet with
>    RELAY_HMAC as described in [RFC8004], except that the parameter
> type
>    is different (see Section 5.8).  The Control Relay Server changes
> the
> 
> I suggest dropping the 8004 reference and just using Section 5.8 of
> this
> document.

done

>    In step 3, the Responder receives the I1 packet.  The Responder
>    processes it according to the rules in [RFC7401].  In addition,
> the
>    Responder validates the RELAY_HMAC according to [RFC8004] and
>    silently drops the packet if the validation fails.  The Responder
> 
> [likewise here]

ok

>    In step 7, the Responder receives the I2 packet and processes it
>    according to [RFC7401].  The Responder validates the RELAY_HMAC
>    according to [RFC8004] and silently drops the packet if the
>    validation fails.  It replies with an R2 packet and includes a
> 
> [and here]

ok

>    Hosts MUST include the address of one or more Control Relay
> Servers
>    (including the one that is being used for the initial signaling)
> in
>    the LOCATOR_SET parameter in I2 and R2 if they intend to use such
>    servers for relaying HIP signaling immediately after the base
>    exchange completes.  The traffic type of these addresses MUST be
> "HIP
>    signaling" and they MUST NOT be used as HIP candidates.  If the
> 
> I'm not sure I understand how the recipient will know that a given
> "HIP
> signaling" candidate is intended only for signaling and is not to be
> used as a HIP candidate.  Is that inherent to the "HIP signaling"
> traffic type (vs. ESP)?  (We don't seem to define "HIP candidate"
> specifically", which probably contributes to my confusion.)

the signaling traffic type described in a section in the draft.
I put a pointer there as follows:

   The traffic type of these addresses MUST be "HIP
   signaling" (see Section 5.7) and they MUST NOT be used for the
   connectivity tests described in Section 4.6.

"HIP candidate" was old, confusing terminology from earlier revisions.
I cleared it up, so it means that the address is not tested for
connectivity.

> Section 4.6.2
> 
> 
>    All of the connectivity check packets MUST be protected with HMACs
>    and signatures (even though the illustrations in this
> specification
>    omit them for simplicity).  Each connectivity check sent by a host
> 
> Are these the RELAY_HMACs defined by this document or the normal
> HIP_MAC?

this would be HIP_MAC since it is directly between the two end-hosts.
Clarified as follows:

   All of the connectivity check messages MUST be protected with
   HIP_HMAC and signatures (even though the illustrations in this
   specification omit them for simplicity) according to [RFC7401].

>    [RFC7401] states that UPDATE packets have to include either a SEQ
> or
>    ACK parameter (or both).  According to the RFC, each SEQ parameter
>    should be acknowledged separately.  In the context of NATs, this
> 
> I'm not finding where RFC 7401 says that each SEQ parameter should be
> "acknowledged separately".  (A few sentences later we say that an ACK
> parameter may acknowledge multiple sequence identifiers, too...)

I put a more precise pointer now:

   [RFC7401] section 5.3.5 states that UPDATE packets have to include
   either a SEQ or ACK parameter (but can include both).

RFC7401: "An UPDATE packet without either a SEQ or an ACK parameter is
invalid; such unacknowledged updates MUST instead use a NOTIFY packet."

>    Full ICE procedures for prioritizing candidates, eliminating
>    redundant candidates, forming check lists (including pruning) and
>    triggered check-queues MUST be followed as specified in section
> 6.1
>    [RFC8445], with the exception that the foundation, frozen
> candidates
> 
> nit: missing "of"

thanks, added

> Section 4.7.2
> 
>    traversal mode as one of the supported modes in the R1 packet.  If
>    the Responder has registered to a Control Relay Server in order to
>    discover its address candidates, it MUST also include a
> LOCATOR_SET
>    parameter in R1 that contains a preferred address where the
> Responder
> 
> Previous mentions of LOCATOR_SET have mandated an ENCRYPTED
> container.
> If this instance is notable in its exception from that, I suggest
> calling it out explicitly as not encrypted; otherwise, I think a
> reminder might be in order.

Ok, now this text is as follows:

   If the Responder has registered to a Control Relay Server in order
   to discover its address candidates, it SHOULD also include a
   LOCATOR_SET parameter encapsulated inside an ENCRYPTED parameter
   in R1 message that contains a preferred address where the Responder
   is able to receive UDP-encapsulated ESP and HIP packets.

(note that I changed the "it MUST" to "it SHOULD", see my following
comment)

>    is able to receive UDP-encapsulated ESP and HIP packets.  This
>    locator MUST be of type "Transport address", its Traffic type MUST
> be
>    "both", and it MUST have the "Preferred bit" set (see Table
> 1).  If
>    there is no such locator in R1, the source address of R1 is used
> as
>    the Responder's preferred address.
> 
> Given the last sentence, it seems like the "MUST also include [...]
> preferred address" is unenforceable by the Initiator.

I am not sure what you mean? The MUST (or now SHOULD) is a guideline 
what the Responder sends. Anyway, I changed the wording now so that
this should be more clear and hopefully less ambiguous?

  If there is no such locator in R1, the Initiator MUST use the source
  address of the R1 as the Responder's preferred address.

Note: I changed the "it *MUST* also include a LOCATOR_SET" to
"it *SHOULD* include a LOCATOR" set because otherwise the last sentence
("If there is no such locator in R1") is moot since a MUST is a MUST.
The latter case can be used in scenarios where the Responder has a
public IP address.

> Section 4.9
> 
>    responds with an UPDATE with ECHO_REQ_SIGN as described in step 3
> in
>    Figure 6.  Upon receiving the valid response from host M as
> described
>    in step 6, the peer host MUST restart the connectivity checks with
>    host M.  This way, both hosts start the connectivity checks
> roughly
>    in a synchronized way.  It is also important that peer host does
> not
>    restart the connectivity checks until it has received a valid
> "fresh"
>    confirmation from host M because the UPDATE message carrying
> locators
>    could be replayed by an attacker.
> 
> Can you remind me which step this "fresh" confirmation is in?  Step 6
> doesn't really seem to match that description...

I changed this sentence to be more explicit:

   It is also important that peer host does not
   restart the connectivity checks until the step 6 is successfully
   completed because the UPDATE message carrying locators in step 1
   could be replayed by an attacker.

> Section 5.4
> 
> 
>    The format of NAT traversal mode parameter is borrowed from Legacy
>    ICE-HIP [RFC5770].  The format of the NAT_TRAVERSAL_MODE parameter
> is
> 
> "Borrowed from"?  It had darn well better be *the same as* (i.e.,
> "retained from") the RFC 5770 format, the way we're using the IANA
> registry for NAT traversal modes...
> (The "defined in [RFC5770]. but repeated for completeness" language
> we
> use for TRANSACTION_PACING seems much better.)

Changed to:

   The format of NAT traversal mode parameter is defined in Legacy ICE-
   HIP [RFC5770] but repeated here for completeness.

> Section 5.7
> 
> Should we say that the LOCATOR_SET must always appear within an
> ENCRYPTED wrapper?

Added to the end of this section:

  The LOCATOR parameter MUST be encapsulated inside an ENCRYPTED
parameter.

> Section 6.1
> 
> It's hard to see the phrase "requires further experimentation" in a
> proposed standard.  (Maybe it shouldn't be, given the "proposed"
> part,
> but in practice it seems to be.)  Perhaps we could just say that
> which
> host addresses to exclude from the LOCATOR_SET is a matter of local
> policy?

I don't see why a local policy should be mandated in an RFC? Anyway, I
turned this around as follows:

   For these two local policy reasons, it might be tempting exclude 
   certain host addresses from the LOCATOR_SET parameter of an end-   
   host  but this is NOT RECOMMENDED. For instance, such behavior
   creates non-optimal paths when the hosts are located behind the    
   same NAT.

Are you ok with this?

>    This scenario is referred to as the hairpin problem
> [RFC5128].  With
>    such a legacy NAT, the only option left would be to use a relayed
>    transport address from an Control Relay Server server.
> 
> Data Relay Server, no?  (Also, nit: "Server server".)

thanks, now this is:

"...from an Data Relay Server."

> Section 6.7
> 
>    replay attacks that are difficult to prevent.  The connectivity
>    checks in this protocol are immune against replay attacks because
> a
>    connectivity request includes a random nonce that the recipient
> must
>    sign and send back as a response.
> 
> If I was feeling particularly pedantic I'd request "effectively
> immune",
> since there is the minor risk of collision in the random nonce
> values.

added "effectively"

> Section 10.1
> 
> RFC 5766 could probably be just an informative reference; RFCs 6146
> and
> 6147 as well.

done!

> Section 10.2
> 
> I'm not sure that we still need to reference RFC 5245 (vs. 8445)?

RFC5770 depends on it. RFC5245 is an informative reference.

> The status of RFC 5770 is less clear; in Section 5 we note that "most
> of
> the parameters are shown for the sake of completeness", implying that
> not all are shown, which would in theory leave a normative dependency
> on
> RFC 5770 for those parameters' interpretation.

I compared the draft parameters with RFC5770, and now changed "most" to
"all".

> Similarly, the status of RFC 3948 is also less clear; we do use a
> fair
> bit of machinery from it and it's not trivially clear that we
> reproduce
> everything that is needed.

the UDP encapsulation of ESP is fairly trivial and has been
interoperated successfully by three implementations.

> Could we get "draft-rosenberg-mmusic-ice-nonsip" included in the
> [MMUSIC-ICE] reference, assuming that's the right I-D?

done

> Similarly, a draft name for [HIP-MIDDLE] would be helpful.

done

> Appendix A
> 
> ["further experimentation" here as well]

Changed this to:

   In general, estimating RTT can be difficult and error prone, thus 
   the guidelines for choosing a Ta value in Section 4.4 MUST be
   followed.