Mime-Version: 1.0 (Apple Message framework v1082)
Content-Type: text/plain; charset=us-ascii
From: Ari Keranen <ari.keranen@nomadiclab.com>
In-Reply-To: <0DF41C4B-5B59-4766-AE72-61D64C8EFA9F@cs.rwth-aachen.de>
Date: Tue, 5 Apr 2011 15:45:10 +0300
Content-Transfer-Encoding: quoted-printable
Message-Id: <2391E49D-B962-48FF-B276-5DE11564739C@nomadiclab.com>
References: <B3E13881-1543-447B-B011-D5394EF086BB@nomadiclab.com>
 <8B48650A-E9CA-4C74-90FA-4BE3860DB3A7@nomadiclab.com>
 <0DF41C4B-5B59-4766-AE72-61D64C8EFA9F@cs.rwth-aachen.de>
To: Tobias Heer <heer@cs.rwth-aachen.de>
Cc: "hipsec@ietf.org" <hipsec@ietf.org>
Subject: Re: [Hipsec] rfc5201-bis-04 review
Precedence: list

Hi Tobi,

On Mar 13, 2011, at 9:19 PM, Tobias Heer wrote:
> Hello Ari,
>=20
> ok, this took a while. Finally I am through. Most of your comments did =
not require second thoughts. On some I made more detailed comments. =
Please check if I always got your point or if I misunderstood something.

OK, see inline (clipping away parts that seem to be clear by now).

> Also. A BIG thank you for providing alternative text when appropriate. =
That did speed up addressing the comments quite much. Excellent work!

You're welcome!

> Am 03.02.2011 um 18:47 schrieb Ari Keranen:
>>    This control is set in packets R1 and/or
>>    I2.  The peer receiving an anonymous HI may choose to refuse it.
>>=20
>> How about using anonymous HIs with other packets? Say, NOTIFYs or =
UPDATEs?
>=20
> It doesn't make much sense because the anonymous HI is only important =
for end-hosts when they bootstrap a new HIP association.

So, if one sends HOST_ID in NOTIFY, even if it's the same anonymous =
HOST_ID one used in the BeX (with the A-bit), the host would not set the =
A-bit here? This sounds just a bit inconsistent. Would it be easier to =
just say that the A-bit is (always) set in packets where the HI is =
anonymous? That is, not say anything about R1/I2.

> For middleboxes, things may be different because after an UPDATE a =
middlebox may suddenly be on the primary communication path between two =
HIP hosts that previously established a connection.
>=20
> However, middleboxes are only addressed to a minimal degree in BEX. =
The semantics and function of the HIT for middleboxes is undefined so =
far. If we start adding things for middleboxes here we might have to =
delve much deeper.
>=20
> Any opinion?

I'd leave middlebox considerations, as long as we're not breaking =
anything, for another draft.

>> 5.1.3.  HIP Fragmentation Support
>>=20
[...]
>> HIP-aware NAT
>> devices MUST perform any IPv4 reassembly/fragmentation.
>>=20
>> 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.
>=20
> It should definitely be a SHOULD. The box may or may not be useful =
without fragmentation support. BEX is not the document to decide that.

Agree.

> I also agree that only HIP packets are to be considered. However, the =
distinction may be difficult if the fragments arrive out of order. How =
will the box figure out if the fragment is a HIP control packet or not =
if the HIP header is in another fragment?

True, but I'd leave that up to the box. That is, if/when it can figure =
it out, it can stop to attempt reassembly.

>>=20
>> 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.
>>=20
>> 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.
>>=20
> Alternative text:
>=20
>   Certificate chains can cause the packet to be fragmented and
>   fragmentation can open implementations to denial-of-service attacks
>   [KAU03].  "Hash and URL" schemes as defined in [I-D.ietf-hip-cert]
>   for HIP version 1 may be used to avoid fragmentation and mitigate
>   resulting DoS attacks.
>=20
> The implications and security considerations for certficate chains and =
hash and url are discussed there. Hence, I see no reason to put a MUST =
or SHOULD here.

Sounds good to me.

>>=20
>> 5.2.  HIP Parameters
>>=20
>> The HIP Parameters are used to carry the public key associated with
>> the sender's HIT, together with related security and other
>> information.
>>=20
>> 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.
>=20
> Let me try ;-)
>=20
>   The HIP parameters carry information that is necessary for
>   establishing and maintaining a HIP association.  For example, the
>   peer's public keys as well as the signaling for negotiating ciphers
>   and payload handling are encapsulated in HIP parameters.  Additional
>   information, meaningful for end-hosts or middleboxes, may also be
>   included in HIP parameters.  The specification of the HIP parameters
>   and their mapping to HIP packets and packet types is flexible to
>   allow HIP extensions to define new parameters and new protocol
>   behavior.
>=20
>   In HIP packets, HIP parameters are ordered according to their =
numeric
>   type number and encoded in TLV format.
>=20

Much better.

>>=20
>> | ECHO_RESPONSE_SIGNED   | 961   | variable | Opaque data echoed    |
>> |                        |       |          | back; under signature |
>>=20
>> s/echoed back/echoed back by request/
>>=20
> Do you think this is necessary? To me it is obvious and pronounced in =
the rest of the text.

Not necessary, but IMHO more clear that way. BTW, -05 revision has now =
different text (regarding "by request") for SIGNED and UNSIGNED.


>>=20
>> |  2048 -  4095 | Parameters related to HIP transport types         |
>>=20
>> s/transport types/transport formats/
>> (to be consistent with the later text)

Missed this one?

>> 5.2.1.  TLV Format
>>=20
>> 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.
>>=20
>> 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).
>=20
> I recently sent this issue to the list and unless there are any =
objections, I'll add the following.
>=20
> I won't be able to get this done before the next version but it will =
be the first thing for the next revision.
>=20
> Option: Give all transforms different type numbers, keep the ordering =
and express preference in a list.
>=20
> Example HIP packet excerpt:
> +------+ +------+---------+ +------+-----+ +------+-----+
> |Header| | 2050 | XYZ, ESP| | 2095 | ESP | | 2096 | XYZ | ...
> +------+ +----------------+ +------+-----+ +------+-----+
>              ^
>              |
> List----------+
>=20

Makes sense.

>> 5.2.2.  Defining New Parameters
>>=20
>>     Implementations operating in a mode
>>     adhering to this specification MUST disable the sending of new
>>     critical parameters.
>>=20
>> This sounds a bit strange. Should that be "MUST allow disabling"?
>>=20
> I think the original meaning was that you have to enable it by hand =
and it is disabled by default. This makes sense because otherwise =
standard compliant implementations will be incompatible by default. To =
make it clearer I changed the sentence to:
>=20
>               Implementations operating in a mode adhering to this
>               specification MUST disable the sending of new critical
>               parameters by default.
>=20

That's better.  =20

>>=20
>> 5.2.6.  DIFFIE_HELLMAN
>>=20
>> 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).
>>=20
>> Should mention which Group ID the 160-bit ECP is (I'd assume 10, but =
it's not explicit).
>>=20
> I rephrased the sentence to mention the group name explicitly. This =
should clarify it.
>=20
>           A HIP implementation MUST implement Group ID 3. The 160-bit
>           SECP160R1 group can be used when lower security is enough =
...
>=20

That works too.

>> 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.
>>=20
> Hmmm.. I think the words of caution above should avoid that. =
Otherwise, I could write something like:
>=20
> In general stronger groups SHOULD be preferred.
>=20
> Should we explicit state the order here? Stronger groups are of course =
more costly and need more space in the packets. So I wouldn't want to =
propose group 9 as default.

I think the important thing is to tell what is the order of strength =
between the groups. Intuitively one could assume that the higher the =
value the stronger the group, but clearly that's not the case with =
SECP160R1.

>>=20
>> 5.2.7.  HIP_CIPHER
>>=20
>>   Cipher ID      defines the cipher algorithm to be used for
>>                  encrypting parts of the HIP packet
>>=20
>> 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/
>>=20
> Nope it is nowhere else (at least in this document). However other =
documents might use it for something else.
> But well, these documents will define it more explicitly anyway.
>=20
> Agreed, changed.

Makes sense.

>>=20
>> NULL-ENCRYPTION is included
>> for testing purposes.  NULL-ENCRYPTION SHOULD NOT be configurable via
>> the UI.
>>=20
>> There may be no UI for the implementation, so could say "by the user" =
instead of "via the UI".
>>=20
> There is (almost) always a UI, right? Maybe its no GUI, maybe its not =
even a command line tool... but any interface that allows the user to =
interact is a UI, right. Changing it to "by the user" implies the =
existence of some UI as well.

I would not consider a config file as "user interface". Or is it OK to =
config NULL encryption via config file (but not via "UI")?=20

>>=20
>> 5.2.8.  HOST_ID
>>=20
>>  DI-type           type of the following Domain Identifier field
>>  Domain Identifier the identifier of the sender
>>=20
>> 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.
>>=20
> I moved it closer and adjusted the order of the field explanations to =
match the order of the fields in the parameter.
> I hope things are clearer now.

Yes (although the "DI Length" and "Algorithm" field/explanation order =
doesn't still match).=20

>>=20
>> If there is no Domain Identifier, i.e., the DI-type field is zero,
>> the DI Length field is set to zero as well.
>>=20
>> Should there be some normative text on when to have the domain =
identifier?
> It is zero if the HIT does not belong to any domain, I guess. I am not =
fully aware of the reasons that led to the inclusion of the NAI and FQDN =
here. =46rom my perspective it is some nice-to-have information that =
adds some weak hierarchy to the HOST_ID (weak because it is not a input =
for the actual HI or HIT.
>=20
> Maybe someone can shed some light? (I will post this as separate mail =
on the list so that it is not lost in the bulk of the comments and =
replies here).

Good idea.

>> 5.2.13.  HIP_SIGNATURE
>>=20
>> The signature algorithms are defined in Section 5.2.8.
>>=20
>> The algorithm identifier field side for HOST_ID (Section 5.2.8) is 16 =
bits whereas here it is 8 bits. Something wrong here?
>=20
> Woops. That's still a remnant of 5201 (no bis). It should be changed, =
of course. It won't bloat the signature to use a 16 bit field. On the =
other hand, we're limited in the numbers of algorithms anyway so 256 =
seems kind of okay. Well, I'll go for the 16 bit field in HIP_SIGNATURE =
if no one produces valid reasons not to do this.
>=20
> This has to be carefully documented in the changelog - otherwise =
implementers will struggle with this hidden change quite a lot.

Sounds good to me.

>>=20
>> Could also reference to exact table with the algorithms since section =
5.2.8 has 4 different tables.
> I added a introductory sentence to the table in 5.2.8. I hope this =
does the trick.

Yes, that helped.

>> Notification information can be error messages specifying why an SA
>>=20
>> First occurrence of SA. Expand, and maybe add a reference too.
>>=20
> I think this refers to the HIP security association, not IPsec. In =
that regard, the text is blurry (I'll change that) but does not require =
a reference.
>=20
> The new text is:
>=20
>           Notification information can be error messages specifying
>           why an HIP Security Association could not be established.
>           It can also be status data that a HIP implementation
>           wishes to communicate with a peer process.  The table
>           below lists the Notification messages and their
>           corresponding values.

That's better.

>=20
>>=20
>> Overall, this section is fairly inconsistent with the use of =
different ways to refer to "Notify Message Type", some suggestions for =
change:
>>=20
>> The table below lists the Notification messages and their
>> corresponding values.
>>=20
>> s/Notification messages/Notify Message Types/
>>=20
> suggestion: The table
>           below lists the notification messages and their
>           Notification Message Types.
>=20

Sounds good.

>> An
>> implementation that receives a NOTIFY packet with a NOTIFICATION
>> error parameter in response to a request packet
>>=20
>> s/NOTIFICATION error parameter/(something)/
>>=20
>=20
> An implementation that receives a
>           NOTIFY packet with a Notify Message Type that indicates an
>           error in response to a request packet ...
>=20
> is this appropriate for (something)?

Yes!

>>=20
>>   UNSUPPORTED_CRITICAL_PARAMETER_TYPE        1
>>=20
>>     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.
>>=20
>> 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?
>=20
> There may be several notify parameters in one HIP packet in that case. =
I spelled it out explicitly in the introduction:
>=20
>           HIP packets MAY contain
>           multiple notify parameters if several problems exist or
>           several independent pieces of information must be =
transmitted.
>=20

OK, that helps. Just s/notify/NOTIFICATION/

>>=20
>> The sender has to create the Opaque field
>> so that it can later identify and remove the corresponding
>> ECHO_RESPONSE_UNSIGNED parameter.
>>=20
>> Does this apply also to middleboxes, i.e., is "sender" the sender of =
the packet or sender of the parameter?
>>=20
> The creator of the parameter. I'll state this more clearly.
>=20
>           The creator of the
>           ECHO_REQUEST_UNSIGNED (end-host or middlebox) has to
>           create the Opaque field so that it can later identify and
>           remove the corresponding ECHO_RESPONSE_UNSIGNED parameter.
>=20

That's better.
=20
>>=20
>> 5.3.1.  I1 - the HIP Initiator Packet
>>=20
>>   IP ( HIP ( DH_GROUP_LIST ) )
>>=20
>> 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).
>>=20
>>=20
>=20
> I think it ti rather clear here. if it were IP(HIP) I could imagine =
that someone might be confused. All other forms of braces are used up =
anyway ([{}])

Agreed; no point changing this.


>>=20
>> 5.3.2.  R1 - the HIP Responder Packet
>>=20
>> The Initiator's HIT MUST match the one received in the I1 packet.
>>=20
>> ...if the R1 is sent due to I1.
>>=20
> The Initiator's HIT MUST match the one received in the I1
>           packet if the R1 is a response to an I1.
>=20

That's good.

>>=20
>> If a future version of this protocol is considered, we strongly
>> recommend that these issues be studied again.
>>=20
>> Has anyone looked into this?
>>=20
> I thought about it for a while but did not come up with any good =
conclusion.

Perhaps this also deserves a separate thread and/or discussion at the WG =
meeting.

>>=20
>> The signature is calculated over the whole HIP envelope
>>=20
>> "HIP envelope" is not defined anywhere (but used many times after =
this occurrence too). Perhaps add it to the definitions part.
>>=20
>>=20
> We can substitute it with HIP packet. That is more technical and more =
precise.

OK, that helps.

>=20
>> 5.3.3.  I2 - the Second HIP Initiator Packet
>>=20
>> The HITs used MUST match the ones used previously.
>>=20
>> This is ambiguous. Should that be "used in R1" or are there also =
other possibilities?
>>=20
>>=20
> Nope. Changed.
>=20
> (Damn... Man, this review is sooo much work. I am not even close to =
the end. Thanks for doing this in such great detail!)

You're welcome :)

>> 5.3.5.  UPDATE - the HIP Update Packet
>>=20
>> 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.
>=20
> The text above seems shaky in itself. The update should nonetheless =
contain an ACK because otherwise it is not clear which SEQ it =
acknowledges. I'll change that.
>=20
>>=20
>> What about UPDATE without ACK nor SEQ? Unreliable UPDATE or a =
protocol error?
>>=20
> I changed the text to:
>=20
>           The UPDATE packet contains zero or one SEQ parameter.  The
>           presence of a SEQ parameter indicates that the receiver
>           MUST acknowledge the the UPDATE.  An UPDATE that does not
>           contain a SEQ but only an ACK parameter is simply an =
acknowledgment of a
>           previous UPDATE and itself MUST NOT be acknowledged by a
>           separate ACK. UPDATE packets without SEQ parameters do not
>           require an acknowledgment and do not require processing in
>           relative order to other UPDATE packets.
>=20
> Satisfied?

That's more clear. Although, I'm not sure what would you use "unreliable =
UPDATEs" for. If you don't care about reliability, that sounds more like =
a notification, and hence using NOTIFY packet(s) would make more sense, =
right? I could be missing a use case though.

[...]
>> 6.4.1.  HMAC Calculation
>>=20
>> 6.  Set Checksum and Header Length field in the HIP header to
>>     original values.
>>=20
>> 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?
>>=20
> Well.. this is a hint for the implementors so that they don't deal =
with a corrupted packet. I guess its fine.

OK. Could make sense to explicitly make it "an implementor hint" then. =
Something like (to the end) "Note that the checksum and length field =
contain incorrect values after these steps". Not sure if that's more =
clear; I'll leave it up to you.=20

>>=20
>> 6.5.  HIP KEYMAT Generation
>>=20
>>=20
>>    HIP-gl encryption key for HOST_g's outgoing HIP packets
>>    HIP-gl integrity (HMAC) key for HOST_g's outgoing HIP packets
>>=20
>> 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.
>=20
> Changed it to make ENCRYPTED more obvious but I am not sure what you =
mean by "same key name twice"... can you explain?

I assume that you draw here two (well, 4 if you see the whole text) =
different keys -- which both have the name "HIP-gl". Or is that just a =
single key?

[...]
>>=20
>> 6.7.  Processing Incoming I1 Packets
>>=20
>> 2.  If the Responder is in ESTABLISHED state, the Responder MAY
>>     respond to this with an R1 packet, prepare to drop existing SAs,
>>=20
>> Should prepare to drop only the SAs one has with the I1's sender?
>>=20
> New text:
>             If the Responder is in ESTABLISHED state, the Responder
>             MAY respond to this with an R1 packet, prepare to drop
>             an existing HIP security association with the peer, and
>             stay at ESTABLISHED state.

That's better.

>>=20
>>     Other than that, selecting the HIT is a
>>     local policy matter.
>>=20
>> Shouldn't the Responder use the same as HIT the Initiator used for =
it? This would otherwise break section 6.8 step 3.
> Actually that is stated two sentences before the clause you cited:
>=20
>             If the received Responder's HIT in the I1 is NULL, the
>             Responder selects a HIT with a the same HIT Suite as the
>             Initiator's HIT.  If this HIT Suite is not supported by
>             the Responder, it SHOULD select a REQUIRED HIT Suite
>             from <xref target=3D"hit_suite_list"/>, which is currently
>             RSA/DSA/SHA-1. Other than that, selecting the HIT is a
>             local policy matter.
>=20
> The "Other than that, selecting the HIT is a local policy matter" is =
for the case if several HITs (same suite) are possible.

OK, I guess I just missed that one.

[...]
>>=20
>> 6.15.  Processing CLOSE_ACK Packets
>>=20
>> 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.
>>=20
>> s/using/matching/
>> And the end of the sentence seems to be incorrect anyway (REQ is not =
in ACK). Could rephrase the whole sentence.
>>=20
>=20
> New text:
>=20
>         A host can map CLOSE messages to
>         CLOSE_ACK messages by using the included
>         ECHO_RESPONSE_SIGNED that was sent in the CLOSE_ACK packet
>         in response to the ECHO_REQUEST_SIGNED in the CLOSE packet.

That's already better, but still a bit confusing. How about something =
like:

A host can map CLOSE_ACK messages to CLOSE messages by comparing the =
value of ECHO_REQUEST_SIGNED (in the CLOSE packet) to the value of =
ECHO_RESPONSE_SIGNED (in the CLOSE_ACK packet).

[...]
>> 7.  HIP Policies
>>=20
>> 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.
>>=20
>> This sounds a bit ambiguous. Why is the mapping needed? What are the =
lifetimes? Why store the preferred transform?
>>=20
> Okay... The text was blurry and weird. I have some equally blurry but =
clearer alternative here. Blurry because this is really local policy, =
not HIP.
>=20
>       Initiators may use a different HI for different Responders to
>       provide basic privacy. Whether such private HITs are used
>       repeatedly with the same Responder and how long these HITs are
>       used is decided by local policy and depends on the privacy
>       requirements of the Initiator.

Much better. Although, now you have both "HI" and "HITs". I'd use only =
one consistently in the text (even though they have 1:1 mapping).

>>=20
>> 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.
>>=20
>> 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? :)
>=20
> I would remove the text on 20 altogether. The 20 makes assumptions on =
the capabilities of the clients and the attacker, which we should not =
do. The 20 is just an unjustified magic number. The new text says:
>=20
>       The value of K used in the HIP R1 must be chosen with care.
>       Too high numbers of K will exclude clients with weak CPUs
>       beause these devices cannot solve the puzzle within reasonable
>       time.  K should only be raised if a Responder is under high
>       load attack, i.e., it cannot process all incoming HIP
>       handshakes any more. If a responder is not under high load, K
>       SHOULD be 0.

That sounds reasonable.

>> 10.  IANA Considerations
>>=20
>> This document also creates a set of new namespaces.  These are
>> described below.
>>=20
>> Do we create new namespaces for v2 or should we re-use the existing =
ones?
>>=20
> We create new ones because the parameter ranges and the parameter =
definitions change. So I guess no textual change is needed here.

OK.=20

By the way, we seem to have default policy of "IETF Review" for the =
namespaces. I'd recommend changing that into "IETF Review or IESG =
Approval", at least for the namespaces that have plenty of space in them =
(8 bits or more). That way also non-IETF (e.g., IRTF) RFCs or docs from =
other SDOs can be given values if it makes sense.


Cheers,
Ari=