Subject: Re: [Hipsec] rfc5201-bis-04 review

From: Tobias Heer <[email-masked]>
Date: Thu, 03 Feb 2011 14:55:39 +0100

Hello Ari,

thanks a lot for the review. I'll comment below.

Am 27.01.2011 um 18:13 schrieb Ari Keranen:

> Hi,
> 
> I reviewed part of the -04 draft (version dated Jan 27th), and here's some comments, questions, and suggestions. 
> 
> 
> Since we are specifying the version 2 of the protocol, should we say that in the title and abstract?
> 

I think we should. I guess we would have to add "Version 2" to the title at least. Other comments? 
> 
> 1.  Introduction
> 
>  As a result, all cryptographic protocols need to be agile.
>  That is, it should be a part of the protocol to switch from one
>  cryptographic primitive to another, and reasonable effort should be
>  done to support a reasonable set of mainstream algorithms.
> 
> s/to switch/to be able to switch/
Ok. 

> 
> Instead of "reasonable effort should be done [...]" one could say something like: "[...] it is important to support a reasonable set of mainstream algorithms to cater for different use cases and allow moving away from algorithms that are later discovered to be vulnerable".
> 
Sounds good. Thanks for the suggestion.

> 
> 1.1.  A New Namespace and Identifiers
> 
>  The HI is a
>  public key and directly represents the Identity.
> 
> s/Identity/identity of a host/
> 
Done.
> 
>  Since there are
>  different public key algorithms that can be used with different key
>  lengths, the HI is unsuitable for use as a packet identifier
> 
> s/HI is unsuitable/HI, as such, is unsuitable/
> 
Done.

> 
>  Consequently, a hash of the HI, the Host
>  Identity Tag (HIT), is the operational representation.
> 
> s/is/is used as/
> 
> 
Done.

> 1.2.  The HIP Base Exchange (BEX)
> 
>  It
>  should be noted, however, that both the Initiator's and the
>  Responder's HITs are transported as such (in cleartext) in the
>  packets, allowing an eavesdropper with a priori knowledge about the
>  parties to identify them by their HITs.  Hence, encrypting the HI of
>  any party does not provide privacy against such attacker.
> 
> This text goes fairly deep into a specific attack and could better fit to the Security Considerations section than in the introduction.
> 
> 
I think it needs to be there to understand the reasons and implications of using HIP before mechanisms like the ENCRYPTED parameter are introduced. I would prefer it to be there rather than in the SC section.

>  Data packets start to flow after the 4th packet.  The 3rd and 4th HIP
>  packets may carry a data payload in the future.  However, the details
>  of this may be defined later.
> 
> s/may be defined later/are not defined for the time being/
> 
May be defined foe HIP Version 2 later? So far the data packets are defined for HIPv1.

> 
>  Thus, HIP is not a
>  complete replacement for IKE.
> 
> This begs for a question "why would I use HIP instead of IKE". Perhaps a section (to appendix?) on HIP's relationship to IKE (and a reference to that section here) would be helpful.
> 
It probably would be. I'll see what I can do. Maybe Bob is the best person to write a few lines about it,

> 
> 2.3.  Definitions
> 
> All the definitions repeat the "defined word" in the beginning; that's a bit redundant. That is, instead of:
> 
>  HIP Base Exchange (BEX)  The HIP Base Exchange is the handshake for
>     establishing a new HIP association.
> 
> One could say:
> 
>  HIP Base Exchange (BEX): the handshake for
>     establishing a new HIP association
> 
Done.

> 
> Could also add "Initiator", "Responder", "HIP association" and "signed" to the definitions.
> 
Good idea. I added those terms. Check if you like it in the next revision, please.

> 
> 3.  Host Identity (HI) and its Structure
> 
>  HIP implementations MUST support the Rivest Shamir Adelman (RSA)
>  [RFC3110] public key algorithm, and SHOULD support the Digital
>  Signature Algorithm (DSA) [RFC2536] algorithms, and Elliptic Curve
>  Digital Signature Algorithm (ECDSA) defined in Section 5.2.8. 
> 
> Must support [...] for generating the Host Identity, as defined in [...]

Done.
> 
> 
>  Other
>  algorithms MAY be supported.
> 
> s/Other/Additional/
> 
> 
Ack.

> 3.1.  Host Identity Tag (HIT)
> 
> This section repeats quite a bit the material in the previous section. Maybe some of the text related to HITs in the previous section could be removed and discussed only here.
> 
> 
>  This document extends [RFC5201] with measures to support crypto
>  agility.
> 
> s/extends [RFC5201]/extends the original, experimental HIP specification [RFC5201]/ or "HIP v1" or something.
> 
Good idea. Done.
> 
> 3.2.  Generating a HIT from an HI
> 
>  The HIT MUST be generated according to the ORCHID generation method
>  described in [I-D.ietf-hip-rfc4843-bis] using a context ID value of
>  0xF0EF F02F BFF4 3D0F E793 0C3C 6E61 74EA
> 
> Should one say something about the byte order of the value?
> 
Is there a byte order problem here at all? These are all single-byte values concatenated to a multi-byte string. Byte order is a problem for multi-byte values. We don't have these here. Am I missing something here?

> 
> 4.1.  Creating a HIP Association
> 
>  By definition, the system initiating a HIP exchange is the Initiator,
>  and the peer is the Responder.  This distinction is forgotten once
>  the base exchange completes, and either party can become the
>  Initiator in future communications.
> 
> Would one call a host making a HIP transaction (say, UPDATE) during an active HIP association an Initiator? The text seems to imply that, but at least I would not have used that term in that case. 
> 
it would be the initiator of the transaction but not the Initiator (note the capital I). 

> Also, there are cases where one may need to remember the distinction after the BEX (e.g., with NAT traversal the Initiator becomes the active ICE agent; and I vaguely remember discussing some time ago some other use case too..), so I would say, e.g., that the "distinction is typically forgotten [...]".
> 
Point taken. I changed it to "typically..."

> 
>  The R2 packet acknowledges the receipt of the I2 packet and completes
>  the base exchange.  The packet is signed by the Responder.
> 
> s/The packet/The R2 packet/

Okay.
> 
> 
>  The base exchange is illustrated below
> 
> The figure should be numbered (,center-aligned), and referenced with the number. Bit surprisingly, none of the figures in the draft are numbered.

I'll see how to get this done. There are only two real figures anyway - this should be a quick fix. 

> 
> 
> 4.1.1.  HIP Puzzle Mechanism
> 
>  The puzzle mechanism has been explicitly designed to give space for
>  various implementation options.  First, it allows a Responder
>  implementation to completely delay session-specific state creation
>  until a valid I2 packet is received.  In such a case, a correctly
>  formatted I2 packet can be rejected immediately once the Responder
>  has checked its validity by computing only one hash function.
> 
> Why is a correctly formatted I2 rejected? Because of incorrect solution?
> 
Yes... the correct packet format is not the main point here:

          In such a case, a correctly formatted I2 packet
          without valid puzzle solution can be rejected immediately
          once the Responder has checked the solution by computing
          only one hash function. 

> 
>  Second, the design also allows a Responder implementation to keep
>  state about received I1 packets, and match the received I2 packets
>  against the state, thereby allowing the implementation to avoid the
>  computational cost of the hash function. 
> 
> It's not clear what this means (how one avoids the cost by matching).
> 
I think it misses the point of proving that the Initiator or attacker has churned cycles.
Optimizing away the singular hash function application does not seem worth the effort anyway.

I would rephrase or remove the text completely.

We could write something like:

          The puzzle allows a Responder implementation to completely
          delay session-specific state creation until a valid I2
          packet is received. An I2 packet without valid puzzle
          solution can be rejected immediately once the Responder
          has checked the solution by computing only one hash
          function before state is created and CPU-intensive
          public-key signature verification and Diffie-Hellman key
          generation are performed. By varying the difficulty of the
          puzzle, the Responder can frustrate CPU or memory targeted
          DoS attacks.

Any opinion?

> 
> 4.1.2.  Puzzle Exchange
> 
>  [...] the Receiver can verify that it has itself sent the I to the
> 
> s/Receiver/Responder/
> 
> 
Done.

>  NOTE: The protocol developers explicitly considered whether a memory
>  bound function should be used for the puzzle instead of a CPU-bound
>  function.  The decision was not to use memory-bound functions.
> 
> A few words of rationale for not using memory-bound functions would be nice.
> 
The rationale was that the first document deferred the decision and no good points _for_ memory-bound puzzles were presented.
I think writing that down here will not help much.

> 
> 4.1.3.  Authenticated Diffie-Hellman Protocol with DH Group Negotiation
> 
>  However, since it is precomputed and therefore does not cover
>  association-specific information in the I1 packet [...]
> 
> s/it is/the signature is/ or /the R1 is/ ?

Done. "the R1",

> 
> 
>  The Initiator continues the BEX only
>  if the Group ID of the public DH value of the Responder intersects
>  the preferences of both Initiator and Responder. 
> 
> s/intersects the preferences of/is the most preferred of the IDs supported by/
> 
> 
Done.

>  Otherwise, the
>  communication is subject of a downgrade attack and the Initiator MUST
>  either restart the key exchange with a new I1 packet or abort the key
>  exchange.
> 
> s/key exchange/base exchange/g
> 
Done.

> 
>  The signature of the I2
>  message covers all signed parameters of the packet without exceptions
>  as in the R1.
> 
> Saying "all signed parameters are signed" sounds a bit redundant. And what would be an exception to this rule?
> 
I'd change it into:

          The
          signature of the I2 message covers all parameters of the
          signed parameter ranges (see [ref]) in
          the packet without exceptions as in the R1.

The exceptions are named when the SIGNATURE_2 and HIP_MAC2 are discussed. Discussing the specific exceptions here would distract the reader, I think.

> 
> 4.1.4.  HIP Replay Protection
> 
>  The scope of the
>  counter MAY be system-wide but SHOULD be applied per Host Identity,
>  if there is more than one local host identity.
> 
> What "applied per Host Identity" means is not clear. Could be "every HI must have its own counter" or "every HI must use the same counter and increase it at every usage".
> 

The first one. I'll change it to:

          The scope of the counter MAY be system-wide but there SHOULD
          be a separate counter for each Host Identity, if
          there is more than one local host identity. 

> 
>  Implementations MUST accept puzzles from the current
>  generation and MAY accept puzzles from earlier generations.
> 
> Could give some recommendation on how many earlier generations SHOULD be accepted (otherwise one may conclude that any earlier is always OK).
> 
Any earlier may be okay.... however this really depends on the choice of the duration for which the R1 counter is current and the probability or possibility of misuse (including the choice of I and the opaque data field in the puzzle).

Giving some exact value is difficulty without knowing the implementation details here.

> 
>  When
>  sending multiple I1 packets, an Initiator SHOULD wait for a small
>  amount of time (a reasonable time may be 2 * expected RTT) after the
>  first R1 reception to allow possibly multiple R1s to arrive
> 
> s/multiple I1 packets/multiple I1 packets to the same host/
> 
Done. Thanks.

> 
> 4.1.5.  Refusing a HIP Exchange
> 
>  A HIP-aware host may choose not to accept a HIP exchange.  If the
>  host's policy is to only be an Initiator, it should begin its own HIP
>  exchange.  A host MAY choose to have such a policy since only the
>  privacy of the Initiator's HI is protected in the exchange.  It
>  should be noted that such behavior can introduce the risk of a race
>  condition if each host's policy is to only be an Initiator, at which
>  point the HIP exchange will fail.
> 
> s/HIP Exchange/HIP Base Exchange/g (title and text) -- unless this applies to some other exchanges too? Same with Section 4.1.6.
> 
Done for the whole document.

> Also, should there be some mechanism to detect and prevent the race situation continuing forever?
> 
At this point, the hosts have incompatible policies and cannot communicate. The text concludes that the HIP base exchange will fail in that case. Do you think this needs further elaboration in the text?

> 
> 4.1.7.  HIP Downgrade Protection
> 
>  In a downgrade attack, an attacker manipulates the packets of an
>  Initiator and/or a Responder to unnoticeably influence the result of
>  the cryptographic negotiations in the BEX to its favor.
> 
> s/an attacker manipulates/an attacker attempts to unnoticeably manipulate/
> (and remove the 2nd unnoticeably)
> 
Done. Thanks for providing the alternative text.

> 
>  In contrast to the first version of HIP [RFC5201], this version
>  begins the negotiation of the DH Groups already in the first BEX
>  packet, the I1.
> 
> s/this version/the version 2 of HIP defined in this document/
> 

Done.
> 
>  If the choice of the DH Group in the
>  R1 packet does not equal to the best match of the two lists (the
>  highest priority of the Responder that is present in the Initiator's
>  DH list), 
> 
> s/highest priority/highest priority DH ID/
> 
Done.
> 
> 4.1.8.  HIP Opportunistic Mode
> 
>  Since the Responder's HIT Suite is not determined by the
>  destination HIT of the I1 packet, the Responder can freely select a
>  HIT of any HIT Suite.
> 
> s/is not determined/in the opportunistic mode is not determined/
> 
Done.

> 
>  the Initiator MAY restart the
>  BEX with an I1 packet with a source HIT that is contained in the list
>  of the Responder's HIT Suites in the R1 packet.
> 
> s/is contained in/is compatible with one of the Suite IDs in/
> 
Done.

> 4.4.3.  HIP State Processes
> 
>  | Receive I2, process | If successful, send R2 and go to R2-SENT    |
> 
> What does "process" mean here? (same in the other tables) Should that be "receive and process"? And what is the difference compared to triggers without "process" (e.g., with receiving I1s and in Table 6)?

We could remove the process and say "If processing is successful, send R2 and go to R2-SENT"... That might make it clearer. Agreed?

> 
> 
> | Receive ANYOTHER    | Drop and stay at UNASSOCIATED               |
> 
> "ANYOTHER" is not defined. Could just say "Receive something else" or define ANYOTHER somewhere (say, change 4.4.1 into "State Machine Terminology" and have ANYOTHER there).
> 
> 
Good idea. I'll make the change.

> Table 3 (I1-SENT)
> 
>  | Receive I1          | If the local HIT is smaller than the peer   |
>  |                     | HIT, drop I1 and stay at I1-SENT            |
> 
> s/Receive I1/Receive I1 from the peer/
> Or maybe could explain before the table that "Receive X" means here receiving from a peer one is setting up a HIP SA with -- or does it?
I think "Receive I1 from Responder" would be best. Because from the perspective of the Initiator (who is in state I1-sent) the Responder sends an I1.

> 
> Could also add a reference to the Section 6.5 on how to compare HITs
> 
Done.

> 
> Table 5: R2-SENT - Waiting to finish HIP
> and
> Table 6: ESTABLISHED - HIP association established
> 
> What to do if some other HIP message arrives? Probably with message types not specified in this document that's extension-specific, but could mention it somewhere here. Also, Table 5 doesn't have CLOSE_ACK.
I changed the sentence in the introduction of the state machine from:

New states may be introduced by mechanisms in other
specifications (such as mobility and multihoming).

to

New states and state transitions may be introduced by mechanisms in other
specifications (such as mobility and multihoming).

I guess that covers additional procedures for extensions quite well.

> Table 5: R2-SENT - Waiting to finish HI
it should contain "CLOSE_ACK -> Drop and stay at R2-SENT" because no CLOSE, and therefore, no echo request was sent. Validating the echo will fail anyway.

> 
> 
> Table 7: CLOSING - HIP association has not been used for UAL minutes/CLOSING - HIP association has not been used for UAL minutes
> 
>  | Timeout, increment  | If timeout sum is less than UAL+MSL         |
>  | timeout sum, reset  | minutes, retransmit CLOSE and stay at       |
>  | timer               | CLOSING                                     |
> 
> This trigger's definition is not particularly clear.
> 
This is because it intermingles the action (increment timeout sum, reset timer) into the trigger. I'll separate these:

   | Timeout             | Increment timeout sum and reset timer. If   |
   |                     | timeout sum is less than UAL+MSL minutes,   |
   |                     | retransmit CLOSE and stay at CLOSING        |
   |                     |                                             |
   |                     | If timeout sum is greater than UAL+MSL      |
   |                     | minutes, go to UNASSOCIATED                 |
   +---------------------+---------------------------------------------+

> 
> 4.5.1.  TCP and UDP Pseudo-Header Computation for User Data
> 
>  When computing TCP and UDP checksums on user data packets that flow
>  through sockets bound to HITs, the IPv6 pseudo-header format
>  [RFC2460] MUST be used, even if the actual addresses on the packet
>  are IPv4 addresses.
> 
> s/on the packet/on the IP header of the packet/
> 
> 
> 4.5.2.  Sending Data on HIP Packets
> 
>  A future version of this document may define how to include user data
>  in various HIP packets.  However, currently the HIP header is a
>  terminal header, and not followed by any other headers.
> 
> Do we plan to do this in a future version of *this* document or would that be better left for some other document?
> 
I think other is more accurate given the developments regarding the sending of user data in overlays, etc.

> 
> 4.6.  Certificate Distribution
> 
>  This document does not define how to use certificates or how to
>  transfer them between hosts.  These functions are expected to be
>  defined in a future specification.
> 
> Informational reference to the CERT draft could be appropriate.
> 
Good point.

> 
> I'll send additional nits off-list.
> 
Thanks.

> 
> Cheers,
> Ari
> _______________________________________________
> Hipsec mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/hipsec

---

From: Ari Keranen <[email-masked]>
Date: Thu, 3 Feb 2011 19:47:58 +0200

Hi,

Here's comments, suggestions, and questions of the rest of the rfc5201-bis-04 document. 

BTW, don't be scared of the amount of comments; I just tried to do a thorough review and weed out all the remaining errors, ambiguities, and style issues -- even the smaller ones.

5.1.  Payload Format

   | Next Header   | Header Length |0| Packet Type |  VER. | RES.|1|

s/  VER. /Version/
(if fits there, so no need to abbreviate?)

   document does not describe processing for Next Header values other
   than decimal 59, IPPROTO_NONE, the IPv6 'no next header' value.
   Future documents MAY do so. 

s/do so/define behavior for also other values/

   The Header Length field contains the length of the HIP Header and HIP
   parameters in 8-byte units, excluding the first 8 bytes. 

s/the length/the combined length/

  The HIP Version is four bits.  The current version is 2. 

s/Version is/Version field is/
s/current version/version defined in this document/

   For example, in the case
   that the forthcoming SHIM6 protocol happens to be compatible with
   this specification

Since SHIM6 is already an RFC, this text could be updated.

5.1.2.  HIP Controls

   The HIP Controls section conveys information about the structure of

s/section/field/

   The following fields have been defined:

s/fields/sub-fields/

      This control is set in packets R1 and/or
      I2.  The peer receiving an anonymous HI may choose to refuse it.

How about using anonymous HIs with other packets? Say, NOTIFYs or UPDATEs?

5.1.3.  HIP Fragmentation Support

   In IPv4 networks, HIP packets may encounter low MTUs along their
   routed path.  Since HIP does not provide a mechanism to use multiple
   IP datagrams for a single HIP packet

HIP over HIP does provide mechanism for this, so I'd change:
s/HIP does not/basic HIP, as defined in this document, does not/ (or something)

   HIP-aware NAT
   devices MUST perform any IPv4 reassembly/fragmentation.

Despite the warning in the next paragraph, this would potentially make all HIP-aware IPv4 NAT devices vulnerable for fragment-attacks. Could change the MUST into SHOULD? Also, could clarify that reassembly/fragmentation is a MUST/SHOULD for HIP packets, not for any random packet.

   it is strongly recommended that the separate document
   specifying the certificate usage in the HIP Base Exchange defines the
   usage of "Hash and URL" formats rather than including certificates in
   exchanges. 

This already exists in the CERT draft, so this text could be removed/re-worded. The reference to fragmentation DoS attack is still useful though.

5.2.  HIP Parameters

   The HIP Parameters are used to carry the public key associated with
   the sender's HIT, together with related security and other
   information.

Considering the amount of different things carried in HIP Parameters today, this section could start with a more generic description of the use of the parameters ("and other information" sounds a bit understatement). Can't come up with the right words for now, but would be good to have something that would explain that Parameters are used to carry the security stuff, but are also one of the main ways for extending HIP.

   | R1_COUNTER             | 128   | 12       | System Boot Counter   |

The data description does not seem to match other descriptions of R1_COUNTER.

   | HIP_CIPHER             | 579   | variable | HIP encryption        |
   |                        |       |          | algorithm             |

s/HIP encryption algorithm/List of HIP encryption algorithms/

   | ENCRYPTED              | 641   | variable | Encrypted part of I2  |
   |                        |       |          | packet                |

ENCRYPTED could be also in other packets than just I2, right?

   | CERT                   | 768   | variable | HI Certificate; used  |

Also this text regarding certificates should be updated.

   | ECHO_RESPONSE_SIGNED   | 961   | variable | Opaque data echoed    |
   |                        |       |          | back; under signature |

s/echoed back/echoed back by request/

Also "covered by signature", or "signed" could be better than "under signature" (also in ECHO_REQ_SIGNED).

   |  2048 -  4095 | Parameters related to HIP transport types         |

s/transport types/transport formats/
(to be consistent with the later text)

5.2.1.  TLV Format

   The type-field value also describes the order of these
   fields in the packet, except for type values from 2048 to 4095 which
   are reserved for new transport forms.

This is a bit strange exception. I wonder if it would make sense to make this consistent with the other ordered lists, i.e., have one parameter that lists the supported transport formats and then, depending on which format was chosen, the corresponding parameter(s) from this range are used (and others ignored). 

Also:
s/order of these fields/order of these parameters/
s/transport forms/transport formats/

   If the parameter can exist multiple times in the packet, the
   type value may be the same in consecutive parameters. 

This could rather be formatted as a normative requirement (and then it would be also implicit for the responder). Something like:

   If the parameter can exist multiple times in the packet, the
   parameters with the same type MUST be consecutive in the packet.

5.2.2.  Defining New Parameters

       Implementations operating in a mode
       adhering to this specification MUST disable the sending of new
       critical parameters.

This sounds a bit strange. Should that be "MUST allow disabling"?

5.2.3.  R1_COUNTER

   The sender is supposed to increment this counter
   periodically.

s/is supposed to increment/increments/ or "SHOULD increment"

5.2.6.  DIFFIE_HELLMAN

   The 160-bit ECP
   group can be used when lower security is enough (e.g., web surfing)
   and when the equipment is not powerful enough (e.g., some PDAs).

Should mention which Group ID the 160-bit ECP is (I'd assume 10, but it's not explicit).

Also, some general recommendation on the order of proposed group IDs would be helpful so that implementations don't end up (unnecessarily) proposing the weakest groups by default.

5.2.7.  HIP_CIPHER

     Cipher ID      defines the cipher algorithm to be used for
                    encrypting parts of the HIP packet

Is HIP_CIPHER used anywhere else except in the ENCRYPTED parameter? If not, the explanation above could say that explicitly. That is, for example,
s/parts of the HIP packet/the contents of the ENCRYPTED parameter/

   NULL-ENCRYPTION is included
   for testing purposes.  NULL-ENCRYPTION SHOULD NOT be configurable via
   the UI.

There may be no UI for the implementation, so could say "by the user" instead of "via the UI".

5.2.8.  HOST_ID

    DI-type           type of the following Domain Identifier field
    Domain Identifier the identifier of the sender

The actual content of these fields is not clear from the text. Could at least move the DI-types table closer to the figure. Also, "DI Length" could simply say "length (in octets) of the Domain Identifier field"; the FQDN and NAI part just confuses here.

   If there is no Domain Identifier, i.e., the DI-type field is zero,
   the DI Length field is set to zero as well.

Should there be some normative text on when to have the domain identifier?

5.2.10.  DH_GROUP_LIST

   The information in the
   DH_GROUP_LIST allows the Responder to select the DH group preferred
   by itself and the Initiator.

s/and the Initiator/and supported by the Initiator/

5.2.13.  HIP_SIGNATURE

  The signature algorithms are defined in Section 5.2.8.

The algorithm identifier field side for HOST_ID (Section 5.2.8) is 16 bits whereas here it is 8 bits. Something wrong here? 

Could also reference to exact table with the algorithms since section 5.2.8 has 4 different tables.

Sections 5.2.15 and 5.2.16 (SEQ and ACK) 

Figures, and their Length descriptions are not consistent although, AFAIK, the format of the parameters is identical.

5.2.16.  ACK

   The Length field identifies the number of
   peer Update IDs that are present in the parameter.

This is a bit misleading. Could say something like "number of peer Update IDs can be inferred from the length by dividing it by 4"

5.2.17.  ENCRYPTED

     Encrypted      The data is encrypted using an encryption algorithm
       data         as defined in the HIP_CIPHER parameter.

s/an encryption algorithm as defined in/the encryption algorithm defined by/

5.2.18.  NOTIFICATION

     Padding        any Padding, if necessary, to make the parameter a
                    multiple of 8 bytes.

This is the first parameter to explicitly define padding after the figure (not consistent). Also, base text says that padding MUST be zero, but here it says "anything will do". Would suggest removing this altogether.

   Notification information can be error messages specifying why an SA

First occurrence of SA. Expand, and maybe add a reference too.

Overall, this section is fairly inconsistent with the use of different ways to refer to "Notify Message Type", some suggestions for change:

   The table below lists the Notification messages and their
   corresponding values.

s/Notification messages/Notify Message Types/

   Types in the range 0-16383 are intended for reporting errors and in

s/Types/Notify Message Types/

   An
   implementation that receives a NOTIFY packet with a NOTIFICATION
   error parameter in response to a request packet

s/NOTIFICATION error parameter/(something)/

   Notify payloads with status types MUST be ignored if not recognized.

s/Notify payloads with status types/Notification Data in NOTIFICATION parameters with status Notify Message Types/

     NOTIFICATION PARAMETER - ERROR TYPES     Value

s/NOTIFICATION PARAMETER - ERROR TYPES/Notify Message Types - Errors/
(or why not in all-caps, if that looks better)

     NOTIFY MESSAGES - STATUS TYPES           Value

s/NOTIFY MESSAGES - STATUS TYPES/Notify Message Types - Status/

     UNSUPPORTED_CRITICAL_PARAMETER_TYPE        1

       Sent if the parameter type has the "critical" bit set and the
       parameter type is not recognized.  Notification Data contains the
       two-octet parameter type.

What if there are multiple unsupported critical parameter types? Should send multiple NOTIFICATIONs of this Message Type or would it be better to concatenate the types?

     INVALID_SYNTAX                             7

       Indicates that the HIP message received was invalid because some
       type, length, or value was out of range or because the request
       was rejected for policy reasons. 

Rejecting message with "invalid syntax" code due to policy reasons sounds strange. Isn't the type "BLOCKED_BY_POLICY" for that purpose? Could change that policy text to: "[...] or the message was otherwise malformed".

5.2.19.  ECHO_REQUEST_SIGNED

   A HIP
   packet can contain only one ECHO_REQUEST_SIGNED or
   ECHO_REQUEST_UNSIGNED parameter.  

s/can/MUST/ ?

5.2.20.  ECHO_REQUEST_UNSIGNED

   HIP packet can contain one or more ECHO_REQUEST_UNSIGNED parameters.
   It is possible that middleboxes add ECHO_REQUEST_UNSIGNED parameters
   in HIP packets passing by. 

This seems to contradict with the previous section (see above).

   The sender has to create the Opaque field
   so that it can later identify and remove the corresponding
   ECHO_RESPONSE_UNSIGNED parameter.

Does this apply also to middleboxes, i.e., is "sender" the sender of the packet or sender of the parameter?

5.3.1.  I1 - the HIP Initiator Packet

     IP ( HIP ( DH_GROUP_LIST ) )

This is not strictly consistent with the notation definition "X(y)   indicates that y is a parameter of X" (HIP stuff is not really a parameter of IP packet). I'm not entirely sure how to fix this though (or if it even needs to be fixed).

   The Initiator receives the Responder's HIT either from a DNS lookup
   of the Responder's FQDN, 

Add here a reference to 5205-bis.

5.3.2.  R1 - the HIP Responder Packet

   The Initiator's HIT MUST match the one received in the I1 packet. 

...if the R1 is sent due to I1. 

   Re-using Diffie-Hellman public keys opens up the potential security
   risk of more than one Initiator ending up with the same keying

s/public keys/public values/ ?
(same in next paragraph)

   If a future version of this protocol is considered, we strongly
   recommend that these issues be studied again.

Has anyone looked into this?

   The signature is calculated over the whole HIP envelope

"HIP envelope" is not defined anywhere (but used many times after this occurrence too). Perhaps add it to the definitions part.

5.3.3.  I2 - the Second HIP Initiator Packet

   The HITs used MUST match the ones used previously.

This is ambiguous. Should that be "used in R1" or are there also other possibilities?

   The HIP_CIPHER contains the single encryption transform selected by
   the Initiator, that it uses to encrypt the ENCRYPTED parameter.

s/the ENCRYPTED parameter/ENCRYPTED parameters/
(The ENCRYPTED parameter is optional here, but may be sent later, right?)

5.3.5.  UPDATE - the HIP Update Packet

   An UPDATE that does not contain a SEQ parameter is
   simply an acknowledgment of a previous UPDATE and itself MUST NOT be
   acknowledged by a separate ACK.

What about UPDATE without ACK nor SEQ? Unreliable UPDATE or a protocol error?

5.3.7.  CLOSE - the HIP Association Closing Packet

   The receiver peer MUST validate the ECHO_RESPONSE_SIGNED and validate
   both the HIP_MAC and the signature if it has state for a HIP
   association,

The ECHO_RESPONSE_SIGNED is only in the CLOSE_ACK. I assume this is not just a typo, since there's not much to validate in the request, right? Also, could be more explicit about who the state is with.

5.3.8.  CLOSE_ACK - the HIP Closing Acknowledgment Packet

   The receiver peer MUST validate both the HMAC and the signature.

...and the contents of the ECHO_RESPONSE?

5.4.1.  Invalid Version

   pointing to the VER./RES. byte in the HIP header.

If you change the "VER." field (as suggested in the beginning of this mail), remember to change it also here.

6.1.  Processing Outgoing Application Data

   The exact format and method for transferring the data from the source

s/the data/the user data/
(this does not apply to HIP packets)

6.4.1.  HMAC Calculation

   6.  Set Checksum and Header Length field in the HIP header to
       original values.

This could be just an implementation issue, but unless the signatures and MACs are also restored, the length and checksum are invalid after this step. Is this step even needed?

6.5.  HIP KEYMAT Generation

      HIP-gl encryption key for HOST_g's outgoing HIP packets
      HIP-gl integrity (HMAC) key for HOST_g's outgoing HIP packets

It's a bit confusing to use the same "key name" twice (same for -lg). I assume "outgoing HIP packets" means the ENCRYPTED parameter? If so, could say it explicitly.

      HIP-lg encryption key (currently unused) for HOST_l's outgoing HIP
      packets

Why HOST_l's key is not used if either Initiator or Responder may end up being the HOST_l?

6.7.  Processing Incoming I1 Packets

   2.  If the Responder is in ESTABLISHED state, the Responder MAY
       respond to this with an R1 packet, prepare to drop existing SAs,

Should prepare to drop only the SAs one has with the I1's sender?

       Other than that, selecting the HIT is a
       local policy matter.

Shouldn't the Responder use the same as HIT the Initiator used for it? This would otherwise break section 6.8 step 3.

6.8.  Processing Incoming R1 Packets

   7.   The system MUST check that the DH Group ID in the DH parameter

s/DH parameter/DIFFIE_HELLMAN parameter/ 
(twice in this step)

6.9.  Processing Incoming I2 Packets

   Hellman key, decrypting the Initiator's Host Identity, verifying the

s/decrypting/possibly decrypting/
(if it's not within ENCRYPTED parameter; also in step 11)

   7.   If the system's state machine is in any other state than R2-
        SENT, the system SHOULD check that the echoed R1 generation
        counter in the I2 packet is within the acceptable range.

...if the counter is included

6.10.  Processing of Incoming R2 Packets

   If an R2 packet is received in state I2-SENT, it SHOULD be processed.

When would it be appropriate to *not* process the R2 in I2-SENT state? This looks like a MUST to me.

   2.  The system MUST verify that the HITs in use correspond to the
       HITs that were received in the R1 packet.

"..that caused the transition to the I1-SENT state" 
i.e., not just any R1

6.14.  Processing CLOSE Packets

   The HIP association is not discarded before the host moves from the
   UNASSOCIATED state.

s/from/to/?

6.15.  Processing CLOSE_ACK Packets

   A host can map CLOSE messages to CLOSE_ACK messages by using
   the included ECHO_RESPONSE_SIGNED in response to the sent
   ECHO_REQUEST_SIGNED in the CLOSE_ACK.

s/using/matching/
And the end of the sentence seems to be incorrect anyway (REQ is not in ACK). Could rephrase the whole sentence.

   The CLOSE_ACK contains the HIP_MAC and the SIGNATURE for

s/and the SIGNATURE/and SIGNATURE parameters/

7.  HIP Policies

   Initiators may use a different HI for different Responders to provide
   basic privacy.  Such implementations SHOULD keep state for mapping
   Initiator's HIT to Responder's HIT.  This access control list (ACL)
   SHOULD also include preferred transform and local lifetimes.

This sounds a bit ambiguous. Why is the mapping needed? What are the lifetimes? Why store the preferred transform?

   The value of K used in the HIP R1 packet can also vary by policy.  K
   should never be greater than 20, but for trusted partners it could be
   as low as 0.

s/should never/SHOULD NOT/
Also, why 20? Some rough number on "how much is 20" could be helpful. How about, say, 10 years from now, is 20 that big still then or should we comment something on adjusting the maximum value in the future? Or will it be HIP v3 by then? :)

10.  IANA Considerations

   This document also creates a set of new namespaces.  These are
   described below.

Do we create new namespaces for v2 or should we re-use the existing ones?

   HIP Version

At least this namespace we shouldn't redefine (but e.g., HIP Suite we should).

   Group ID

      New values either from the reserved or unassigned
      space are assigned through IETF Consensus.

What are these spaces? Is "0" the "reserved space"? Same with CIPHER_ID.

      Notify Message Type values 1-10 are used for informing about
      errors in packet structures, values 11-20 for informing about
      problems in parameters [...]

This text could be more helpful in the NOTIFICATION section.

That's all for now. More nits coming off-lists.

Cheers,
Ari

---

From: Ari Keranen <[email-masked]>
Date: Fri, 4 Feb 2011 15:41:18 +0200

Hi Tobi,

Responses inline.

> I think we should. I guess we would have to add "Version 2" to the title at least. Other comments?

I would probably do like they did for IKEv2: http://tools.ietf.org/html/rfc5996

> I think it needs to be there to understand the reasons and implications of using HIP before mechanisms like the ENCRYPTED parameter are introduced. I would prefer it to be there rather than in the…

OK, makes sense.

> May be defined foe HIP Version 2 later? So far the data packets are defined for HIPv1.

But not for I2/R2, as discussed in the text above, right?

> Is there a byte order problem here at all? These are all single-byte values concatenated to a multi-byte string. Byte order is a problem for multi-byte values. We don't have these here. Am I missin…

OK, if it's just a set of single-byte values, that should work.

> it would be the initiator of the transaction but not the Initiator (note the capital I). 

OK, so the text above should change s/the Initiator in/an initiator in/ ?
Or would it be more clear to say "[...]either party can initiate future communications"?

> I'll see how to get this done. There are only two real figures anyway - this should be a quick fix. 

I tend to label also all "message format figures" Figures because it makes it easier to reference to them in the text. That said, 5201 seems to have survived without that convention, so maybe it's OK.

>          once the Responder has checked the solution by computing

OK, looks better. Just: s/valid puzzle/a valid puzzle/

>          puzzle, the Responder can frustrate CPU or memory targeted

Looks good to me. I would use something else than "frustrate" though; but can't come up with any better text right now.

> I think writing that down here will not help much.

I think even that would be actually helpful. Something like " [...] CPU-bound function, but so far no {compelling arguments | proper uses cases | something else} for using memory-bound functions have been presented." 

Although, I guess memory bound functions would be useful in *some* use cases, so perhaps we could leave that as an option for extension? That is, instead of PUZZLE parameter, there could be PUZZLE_MEM parameter that has different kind of puzzle?

> The exceptions are named when the SIGNATURE_2 and HIP_MAC2 are discussed. Discussing the specific exceptions here would distract the reader, I think.

OK, now I get the point of exceptions. The proposed text looks good to me. Just add commas around "without exceptions".

>          there is more than one local host identity. 

Thanks, this is much clearer.

> Giving some exact value is difficulty without knowing the implementation details here.

OK, if any earlier can be used, this doesn't sound like a big problem.

>> point the HIP exchange will fail.
[...]
> At this point, the hosts have incompatible policies and cannot communicate. The text concludes that the HIP base exchange will fail in that case. Do you think this needs further elaboration in the…

I think so. I'm not concerned of the fact that they can't communicate, but that they will end up trying to Initiate the BeX, and eventually I1-DoS:ing each other, indefinitely.

> We could remove the process and say "If processing is successful, send R2 and go to R2-SENT"... That might make it clearer. Agreed?

Agreed.

> I think "Receive I1 from Responder" would be best. Because from the perspective of the Initiator (who is in state I1-sent) the Responder sends an I1.

Agreed (but just say "the Responder" to make it clear it's not just any host),

> I guess that covers additional procedures for extensions quite well.

OK, but what I actually meant was that there's no ANYOTHER here like in the other tables. 

> it should contain "CLOSE_ACK -> Drop and stay at R2-SENT" because no CLOSE, and therefore, no echo request was sent. Validating the echo will fail anyway.

Sounds good.

>   |                     | minutes, go to UNASSOCIATED                 |

Much better.

> I think other is more accurate given the developments regarding the sending of user data in overlays, etc.

Sounds reasonable; could change the text accordingly.

Cheers,
Ari