FW: external mobike-protocol-02 review (technical)

(Forwarding this because Pekka is not a subscriber on the list.
And breaking to smaller pieces to get by the 10K limit...)

---------- Forwarded message ----------
Date: Fri, 16 Sep 2005 14:22:53 +0300 (EEST)
From: Pekka Savola <[email protected]>
To: [email protected]
Subject: external mobike-protocol-02 review

Hi,

(Note: I'm not subscribed)

Jari asked me to review the MOBIKE protocol.  Beware, I'm not really
familiar with IKE, so there are probably things I have wrong or have
missed.

In any case, I thought the document was in a good shape and written in a
manner where it should be easy to get interoperable implementations.  It's
also short -- good.  What troubled me a bit was a significant number of
forward references to other places in the spec, but this is understandable
in the interest of avoiding text duplication.


meta-issues
-----------
                                                           It is assumed
   that issues such as transport mode (updating traffic selectors),
   PFKEY extensions, and tunnel overhead reduction will be handled in
   separate documents.

==> is transport mode even in the scope of the WG?  I thought not, but the
charter doesn't seem conclusive either way..

substantial
-----------

   Note that if some of the initiator's interfaces are behind a NAT
   (from the responder's point of view), the addresses received by the
   responder will be incorrect.  This means the procedure for changing
   responder addresses described in Section 2.4 does not fully work when
   the initiator is behind a NAT. [...]

==> this is probably something that needs more air time.  What does "does
not fully work" mean?  This also probably applies to the responder, though
in different ways.  This may also have security implications (exposing
internal topology), and it may affect address selection (if for example
smaller scope addresses are preferable, but actually belong to different
sites).  This probably needs a bunch of more analysis...

   This specification requires the use of return routability tests
   (under certain conditions) to limit the duration of any "third party
   bombing" attacks by off-path (relative to the victim) attackers.

==> this is not true.  The spec only says RR _SHOULD_ be done before
updating the SAs.  There is no "MUST" for RR (whether before or after), but
a "MAY skip" under certain conditions.  The normativeness requirements need
to be reviewed.  Further, the procedures for handling the payloads
(sections
2.3 and 2.4) should include a bullet point on RR tests and a pointer to
2.5.

  When the initiator is behind a NAT, it SHOULD include these payloads
   in DPD messages, and compare the received ...

  When MOBIKE is in use, the host not behind a NAT SHOULD NOT use the
   dynamic updates specified ...

==> How does the MOBIKE code know whether you're behind a NAT?  Is this
done
automatically as part of IKEv2 NAT detection?  Note that the spec doesn't
seem to require running NAT presence detection so with current spec this
can't be done.

   To give the initiator enough time to detect the error, the responder
   SHOULD use relatively long timeout intervals when, for instance,
   retransmitting IKEv2 requests or deciding whether to initiate dead
   peer detection.

==> "relatively long timeout intervals" is pretty ambiguous.  While this
may
not affect the interoperability, shouldn't we be a bit more precise what
kind of order of magnitude we're talking about here?  Seconds?  Ten
seconds?
30 sec?  Minutes?  10 minutes? Hours?

   The data associated with this notification is the SHA-1 hash
   [FIPS180-2] of the following data:

==> this spec is not algorithm-agile. (I can't find the ref right now
where this is encouraged.)  In this case, maybe it doesn't need to be
considering the impact (I don't take a stance on that) -- but this
probably deserves at least brief discussion in the security
considerations section.

semi-editorial
--------------

==> there seem to be a number of assumptions scattered throughout the
document (e.g., relating to the responder not being behind a NAT) -- maybe
it would be useful to put these as a new subsection of the Introduction?

==> I would also consider splitting out 2.3 in subsections for readability
and more exact references.  For example, initiator's first steps could
be 2.3.1,
Responder's reply 2.3.2, initiator's reply 2.3.3, and the exception 2.3.4.

==> It might make sense to spell out, probably in section 1.2, that
apparently (AFAICS) the initiator never tells the responder which addresses
it takes to use.  It just sends UPDATE_SA_ADDRESSES with the updated
addresses (inmplicit update).

     HDR, SK { IDi, [CERT], [IDr], AUTH,
                [CP(CFG_REQUEST)]
                SAi2, TSi, TSr,
                N(MOBIKE_SUPPORTED),
                [N(ADDITIONAL_*_ADDRESS)+]  -->

==> this raises a minor issue of the normativeness of this spec with regard
to the non-MOBIKE -related IKE payloads.  The above exchange is just an
example; some other payloads could be included as well, or some listed
above
could be omitted.  Also the optional/mandatory status might change.
Perhaps
the document could give a short disclaimer about the IKE exchange examples.

   o  The initiator receives a NAT_DETECTION_DESTINATION_IP payload that
      does not match the previous UPDATE_SA_ADDRESSES response (see
      Section 2.6 for a more detailed description).

==> In some cases, you use NAT_DETECTION_*_IP and in some
NAT_DETECTION_DESTINATION_IP.  I guess this is intentional, but I'd still
review the cases carefully..

   o  Determines whether it has already received a newer
      UPDATE_SA_ADDRESSES request than this one (if the responder uses a
      window size greater than one, it is possible that requests are
      received out of order).  If it has, a response message is sent,
      but no other action is taken.

==> at first, it was not clear to me what "a response message" referred
to. An unspecified response?  Probably you meant the normal response
specified a
few bullets down but this was not unambiguous.

   There is one additional issue that must be taken into account.  If
   the INFORMATIONAL request has been sent to several different
   addresses (i.e., the destination address in the IKE_SA has been
   updated after the request was first sent), ...

==> s/If the/If the same/ (?)

==> this is actually a good issue in more general.  Is it worth the
effort to do so in the first place? That is, couldn't we just specify
that the requests be sent with different cookies for each destination. I
don't think a hash is computationally all that expensive.  That would
simplify the spec and eliminate an extra roundtrip in some cases.

   IKEv2 requires that if an IPsec endpoint discovers a NAT between it
   and its correspondent, it MUST send all subsequent traffic to and
   from port 4500.  To simplify things, implementations that support
   both this specification and NAT Traversal MUST change to port 4500 if
   the correspondent also supports both, even if no NAT was detected
   between them (this way, there is no need to change the ports later).

==> While this seems to be a logical place to talk about this, I'm
troubled. The title is "NAT Prohibition".  What about MOBIKE
implementations which
don't implement NAT prohibition (I guess that's OK).  The above requirement
is more general though -- not really anything to with NAT prohibition.
These kind of more generic requirements should be placed in a much more
prominent place in the spec.

   NO_NATS_ALLOWED payloads can also be included when changing the
   addresses of IPsec SAs (see Section 2.3) and updating the additional
   addresses (see Section 2.4).  An initiator using this "NAT
   prohibition" feature includes a NO_NATS_ALLOWED payload in all
   address update messages.

==> s/includes/MUST include/ or..?

3.1  MOBIKE_SUPPORTED Notification Payload

==> note that the IKEv2 spec refers these as "Notify Payloads" (at least
in some places of the spec; "Notification" is also mentioned though)..?

   The Notify Message Type for MOBIKE_SUPPORTED is TBD-BY-
   IANA(16396..40959).

==> per
http://www.ietf.org/internet-drafts/draft-narten-iana-considerations-rfc2434bis-02.txt,

thse types should be named TBD-BY-IANA1, ...., TBD-BY-IANAN (or something
like that), so we know IANA has assigned a separate value for each and it
can be easily search-n-replaced.

I'd also suggest dropping the assignment range from here, and giving that
guidance in IANA Considerations.

   of the stack, such as properly dealing with ICMP errors
   [ICMPAttacks]

==> some folks may argue that what ICMPattacks proposes may not be
"proper". So, maybe find a different word for "proper", or use something
like "as
validating ICMP errors" which takes no stance on whether it's useful or
not.

   [IPsecArch]
              Kent, S. and K. Seo, "Security Architecture for the
              Internet Protocol", draft-ietf-ipsec-rfc2401bis-06 (work
              in progress), March 2005.

==> this should probably be under normative refs?

-- 
Pekka Savola                 "You each name yourselves king, yet the
Netcore Oy                    kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings