Re: [Hipsec] rfc5201-bis-04 review

[Ari Keranen <[e-mail]>]

Hi Tobi,

Responses inline.

On Feb 3, 2011, at 3:55 PM, Tobias Heer wrote:
> Am 27.01.2011 um 18:13 schrieb Ari Keranen:
>> 
>> 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?

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


>> 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.

OK, makes sense.

>> 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.

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

>> 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?

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

>> 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). 

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"?

>> 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. 

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.


>> 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. 

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

>> 
>> 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?

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

> 
>> 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.
> 

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 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.

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

>> 
>> 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. 

Thanks, this is much clearer.

> 
>> 
>> 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.

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

> 
>> 
>> 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.
>> 
> 
[...]
> 
>> 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?

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.

> 
>> 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?

Agreed.

> 
>> 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.

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

> 
>> 
>> 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.

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


>> 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.

Sounds good.

>> 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                 |
>   +---------------------+---------------------------------------------+

Much better.

>> 
>> 
>> 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.

Sounds reasonable; could change the text accordingly.


Cheers,
Ari