Subject: Re: [Hipsec] more comments on draft-ietf-hip-rfc5201-bis-04

From: Tobias Heer <[email-masked]>
Date: Fri, 11 Feb 2011 12:24:03 +0100

Hi Jeff, 

thanks for the comments. See my responses in line.

Am 24.01.2011 um 22:31 schrieb Ahrenholz, Jeffrey M:

> Here are some additional comments on the 5201-bis draft.
> 
> Section 5.4.2 says:
> "However, an implementation MUST NOT send an ICMP message if the checksum fails; instead, it MUST silently drop the packet." If we MUST silently drop a packet, why do we have NOTIFY type 26, "CHECKSUM_FAILED"? (Debugging?)

Hmmm good question. This was already that way in RFC5201-bis. I think we have three options here:

a) go for "SHOULD silently drop".
Implementors could use the notify if they have a good reason to do so.

b) remove the NOTIFY type 26

c) state that is for debugging purposes only and sending notifies for malformed checksums MUST be turned off by default.

I don't have a strong preference on either of the solutions. Any comments from the group?
> 
> Section 6.11 item 4: this says the association is considered broken if an
> UPDATE is not ACKed, but one circumstance where the association is NOT broken
> would be when a host is informing its peer of its current address list
> (without changing the current preferred locators used.) In that case, the
> association may continue happily without going to CLOSING.
> 

Well, the payload channel might be still functional in that case. However, the missing ACK on the control channel is certainly a sign that something is wrong with the HIP association.
I think if the problem persists (sender of the update keeps retransmitting the update with the seq and receiver does not acknowledge it) it indicates that the HIP channel is broken.
Trying to re-establish the connection (by first closing it and then doing another BEX (because user data is still coming) might be the best choice here.
Am I missing something here?

> Below are more minor editorial nits...
> 
> -Jeff
> 
> 
> 5.2.4, 5.2.5 - suggest changing "n bytes" in parameter diagram to "n bits"
> the descriptions that follow (for random #I, solution #J) discuss n-bit
> integers, and RHASH_len is stated as a bit length
> 
All parameter descriptions are given in bytes. I think going to bits in that single case would confuse people, don't you think? On the other hand, security-related discussions (as for hash functions, etc.) mostly refer to bits in literature. So I wouldn't want to change that here either. 
We could try to untangle a bit by not reusing the quantity n as bit and bytes, though. We could just refer to RHASH_len/8 bytes for I and J:

      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
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |             Type              |             Length            |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |  K, 1 byte    |    Lifetime   |        Opaque, 2 bytes        |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                  Random #I, RHASH_len/8 bytes                 |
     /                                                               /
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

      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
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |             Type              |             Length            |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     | K, 1 byte     |   Reserved    |        Opaque, 2 bytes        |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                      Random #I, n bytes                       |
     /                                                               /
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |            Puzzle solution #J, RHASH_len/8 bytes              |
     /                                                               /
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

> in 5.3.2
> To support the "<ECHO_REQUEST_UNSIGNED>i" used in the diagram, may want to add
> a note about multiple ECHO_REQUEST_UNSIGNEDs as described in 5.2.20 with text such as:
> "The R1 packet may contain zero or more ECHO_REQUEST_UNSIGNED parameters as
> described in Section 5.2.20."
> 
Thanks, Done.

> Then in 5.3.3 may change s/parameter/parameter(s)/ to support the
> "<ECHO_RESPONSE_UNSIGNED>i" diagram notation.

Done.
> 
> in 5.3.7 and 9
> s/an HIP_MAC/a HIP_MAC/
> (for consistency with "a HIP..." elsewhere in the document)
Done.

> 
> 5.3.8, suggest changing:
> "The receiver peer MUST validate both the HMAC and the signature."
> to:
> "The receiver MUST verify the echo response and validate both the HIP_MAC and the signature."
> (because 5.3.7 says you MUST include the ECHO_REQUEST_SIGNED)
Good catch! Done.
> 
> Section 6.4.1 s/SAH-1/SHA-1/
> 
Oops. Done.

> Section 6.7 s/the Responder a HIT with/the Responder selects a HIT with/
> 
Done.

> Section 6.8 item 6 s/a Initiator/an Initiator/
> 
Fixed it.

> Section 5.3.2 and 6.8 item 7 s/SHOULD not/SHOULD NOT/
> 
Done. I checked for other SHOULD not as well. There were a few.

> Section 6.9 item 12 s/packet I2/I2 packet/
Done.

> Section 6.9 item 13 s/verify the HMAC/verify the HIP_MAC/  for consistency?
> (elsewhere, "verify the HIP_MAC" is used)
Good catch. Done.

> 
> Section 6.11 s/steps below ./steps below./
> 
Done.

> Section 6.11 item 2: support multiple ACKs?
> replace:
> "The UPDATE packet MAY also include an ACK of the peer's Update
> ID found in a received UPDATE SEQ parameter, if any."
> with:
> "The UPDATE packet MAY also include zero or more ACKs of the peer's Update
> ID(s) from previously received UPDATE SEQ parameter(s)."
> 
Done. Thanks for providing alternative text.

> Section 7
> suggest defining the first occurrence of ACL:
> "This access control list (ACL) SHOULD also include..."
> 
Done.

> s/representing which hosts/representing for which hosts/
Done.

> s/for such ACL also/for such ACLs, and also/ ?
Done.

> 
> Appendix C.2 suggest re-wording slightly:
> "The IPv4 checksum value for the example I1 packet equals the IPv6 checksum
> value (since the checksum components due to the IPv4 and IPv6 pseudo-header are the same)."
> 
Done. Again, thanks for the alternative text.

> Appendix D 
> s/Today should be used only when/Today these groups should only be used when/

Kudos for the review!

Tobias

---

From: "Ahrenholz, Jeffrey M" <[email-masked]>
Date: Fri, 11 Feb 2011 07:25:51 -0800

> checksum fails; instead, it MUST silently drop the packet." If we MUST
> silently drop a packet, why do we have NOTIFY type 26,
> "CHECKSUM_FAILED"? (Debugging?)
> 
> c) state that is for debugging purposes only and sending notifies for
> malformed checksums MUST be turned off by default.
> 

(c) seems like a good solution

> Well, the payload channel might be still functional in that case.
> However, the missing ACK on the control channel is certainly a sign
> that something is wrong with the HIP association.
> I think if the problem persists (sender of the update keeps
> retransmitting the update with the seq and receiver does not
> acknowledge it) it indicates that the HIP channel is broken.
> Trying to re-establish the connection (by first closing it and then
> doing another BEX (because user data is still coming) might be the best
> choice here.

I guess I was thinking the SEQ/ACK mechanism was not always required. But yes, if every UPDATE requires a SEQ and ACKs have gone missing, this is probably the best thing to do (close the association).

> All parameter descriptions are given in bytes. I think going to bits in
> that single case would confuse people, don't you think? On the other
> hand, security-related discussions (as for hash functions, etc.) mostly
> refer to bits in literature. So I wouldn't want to change that here
> either.
> We could try to untangle a bit by not reusing the quantity n as bit and
> bytes, though. We could just refer to RHASH_len/8 bytes for I and J:

OK, yes. My comment was how "n" referred to both bits and bytes here; using RHASH_len/8 does untangle that a bit.

-Jeff