[IPsec] Paul Wouters' Yes on draft-ietf-ipsecme-rfc8229bis-07: (with COMMENT)

[Paul Wouters via Datatracker <[e-mail]>]

Paul Wouters has entered the following ballot position for
draft-ietf-ipsecme-rfc8229bis-07: Yes

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ 
for more information about how to handle DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-ipsecme-rfc8229bis/



----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

# SEC AD comments for draft-ietf-ipsecme-rfc8229bis-07
CC @paulwouters


## Comments

I have a number of comments and some small fixes. While the appendix fixes technically
might be a DISCUSS, I have faith the authors will pick it up from the comment section too :)



###

The Length field plus non-ESP marker plus IKE packet is called "message"
at the start in Section 3, but in Section 3.1 it is called an "IKE Header Format"
and "IKE message" is used to denote the non-ESP marker plus IKE Header _without_
the Length field. And then it uses "IKE Packet" in the Length field description to
mean the thing without the Length and non-esp marker. And then the figure uses
"IKE header" were it should probably say "IKE packet".

These names should be made more consistent :)

###

Section 3.1 and Section 3.2 state:

        The value in the Length field MUST NOT be 0 or 1.

In fact, a lot more values are clearly wrong, like anything
under 4 which cannot contain the non-esp marker. Then there
is the size of the minimum UDP IKE or ESP packet as well.
Why are these two values singled out?

###

Section 3.1 states:

        The IKE header is preceded by a 16-bit Length field in network byte
        order that specifies the length of the IKE message

Section 3.2 states:

        The ESP header is preceded by a 16-bit Length field in network byte
        order that specifies the length of the ESP packet

Why don't both say either "message" or "packet"? I would say "packet" for both.

###

Section 4:

        at the beginning of any stream of IKE and ESP messages.

I would s/any/the TCP/ to avoid people thinking of the inner streams and thinking
they need to repeat the IKETCP prefix for each burst of traffic - this mistake was
made once in the first version of the Linux kernel code.

###

Section 5:

        when it has been configured to be used with specific IKE peers.

I would say "when it has been configured to be optionally used with specific IKE peers.
Otherwise, implementers might think it needs to be an on/off setting instead of a
"may be used when udp is blocked" setting.

Similarly:

        If a Responder is configured to use TCP encapsulation,

I would say "is configured to accept TCP encapsulation"

(nit: "it is recommended that Initiators should only use TCP encapsulation" can be
written more clearly by omitting the "should")

###

        If TCP encapsulation is being used for a specific IKE SA, all
        messages for that IKE SA and its Child SAs MUST be sent over a TCP
        connection

I think "messages" here is unclear. It might be confused for control messages
(IKE) only and not mean data (ESP) packets. I would use:

        If TCP encapsulation is being used for a specific IKE SA, all
        IKE and ESP packets for that IKE SA and its Child SAs MUST be
        encapsulated and sent over this TCP connection


Note that technically, this is not true because if you want to see if
UDP becomes available again, you have to send an IKE packet over UDP,
but it is probaly clearer not to mention that here.
###

Section 6.1

         using the configured TCP port.

I would say "using the preconfigured Responder's TCP port"

###

        The first bytes sent on the stream MUST be the stream prefix value

Maybe again say "TCP stream" ?

This also made me realize that Section 4. could use a diagram to ensure people do it
right, eg:

        Initiator                       Responder

        Prefix|Length|non-ESP marker|IKE packet ->
                                <- Length|non-ESP marker|IKE packet
        Length|non-ESP marker|IKE packet ->
                                <- Length|non-ESP marker|IKE packet
                                [...]
        Length|ESP packet ->
                           <- Length|ESP packet

###

        If a TCP connection is being used to resume a previous IKE session,

I would use "continue" instead of "resume" to avoid confusion with Session Resumption.
Also instead of "previous IKE session" maybe say "previous encapsulation session"?

Note: at this point it has also not been made clear in the draft whether multiple
IKE SAs can use the same encapsulation session. That information should probably
have been conveyed earlier in the document. This is only stated at the end of Section 6.1

###

        Implementations
        SHOULD NOT tear down a connection if only a single ESP message has an
        unknown SPI, since the SPI databases may be momentarily out of sync.

This advise might be difficult to follow. If this out of sync really happens,
it would be likely that a number of ESP packets would be seen before the IKE
packet comes in that syncs up the SPI. (assuming this out of sync issue happens
when the TCP responder is also the IKE responder to a rekey and once it rekeyed
and installed a new IPsec SA it starts sending ESP packets before it sends its
IKE rekey reply, or a number of smaller ESP packets arrive sooner somehow?)

###

nit:

        if the Responder chooses to send Cookie request

add " a ", eg "a Cookie request".

###

Section 6.3 talks a lot about how COOKIE stuff is both useless for TCP
and can fail in mysterious new ways. Why not simplify things and say
"cookies must (should?)  not be verified and fully ignored when over TCP,
and only puzzles should be verified"

###

Section 6.3.1 assumes the responder knows the rough CPU power of its
clients and that these are all in the same ballpark. Is this really
the case?

###

section 6.4

nit: in case it receives error notification -> in case it receives an error notification

###

section 6.5:

        When negotiating over UDP port 500, IKE_SA_INIT packets include
        NAT_DETECTION_SOURCE_IP and NAT_DETECTION_DESTINATION_IP payloads

An IKE peer is allowed to skip port 500 and use 4500 directly. It would
still send the NAT payloads. I would write "When negotiating over UDP,
IKE_SA_INIT packets include".

###

How about sharing an alternative to the transport mode checksum fixups:

        Implementations MAY use the information that a NAT is present to omit
        sending USE_TRANSPORT over TCP, and thus enforce tunnel mode IPsec SA's
        to avoid the need for checksum fixups for encapsulated packets inside TCP.

###

I would personally split out NAT-T and keepalive into its own seperate sections.
People already confuse them too often and it is really two completely different
things.

###

Section 6.6

        NAT keep-alive packets over a TCP-
        encapsulated IPsec connection will be sent as an ESP message with a
        one-octet-long payload with the value 0xFF.

I don't think you can call it an ESP message? I understand you say this so
implementers will know there is no non-ESP marker, but its really not an ESP message.

Maybe something like:

        NAT keep-alive packets over a TCP-
        encapsulated IPsec connection will be sent as a one-octet-long payload
        with the value 0xFF, preceded by the two byte Length specifying a length of 1.

###

        for which purpose the standard IKEv2 mechanism of
        exchanging empty INFORMATIONAL messages is used

I believe these INFORMATIONALs are not neccessarilly empty, even though they started
out that way. I would just leave out the word "empty".

###

Section 6.7 mentions QoS, but we are also working on per-CPU IPsec SA's, which would
have the same problem. Perhaps that can be worked into the existing text? I would
perhaps also state here that the effects on performance are not very important, as doing
TCP encapsulation in itself is already reducing performance significantly.

###

nit: Section 7.4 starts with a broken reference to [RFC6311]

###

Section 8:

        TCP encapsulation of IKE should therefore use common TCP behaviors to
        avoid being dropped by middleboxes.

I'm not sure why this text is here? Perhaps you mean to say:

        If the IKE implementation has its own minimal implementation of TCP,
        it SHOULD still use common TCP behaviors to avoid being dropped by middleboxes.

That at least clarifies that one needs to do nothing if one uses the OS TCP stack.

###

Section 9:

        Implementations SHOULD favor using direct ESP
        or UDP encapsulation over TCP encapsulation whenever possible.

I understand the SHOULD relates to "whenever possible", but since we are talking
about "SHOULD favor", I think we can say "MUST favor". It's only favoring after all :)

###

Section 10:

nit: [RFC5961] is a broken reference.

nit: it will be re-created by TCP Originator [add "the"]

###

        Alternatively, implementations MAY try to re-sync after they receive
        unknown SPIs by searching the TCP stream [...]

This is an odd paragraph. "You can do this, but really it is futile". I would
remove it.

###

        An attacker capable of blocking UDP traffic can force peers to use
        TCP encapsulation, thus degrading the performance and making the
        connection more vulnerable to DoS attacks.  Note, that attacker
        capable to modify packets on the wire or to block them can prevent
        peers to communicate regardless of the transport being used.

I don't think this paragraph is useful either. There is nothing an implementer
can do with this information.

###

        TCP Responders should be careful to ensure that (1) the stream prefix
        "IKETCP" uniquely identifies incoming streams as streams that use the
        TCP encapsulation protocol and (2) they are not running any other
        protocols on the same listening port (to avoid potential conflicts).

I thought (2) was actually a good thing. One can run a HTTPS server while also
acting as a VPN server and demultiplex based on the prefix. So I don't think the
advise in (2) is appropriate and just limits the deployment possibilities.

###

        The successful delivery of valid IKE or ESP messages over a new TCP connection is used

I think this should say "valid new IKE or ESP messages" (as explained right below it)

Though if an attacker can block packets, they can take a valid new message, and stuff it
in their own source IP packet and send it to cause the TCP stream to be deviated. Perhaps
recommend sending a liveness probe once a TCP connection is redirected? (although even
then, we only detect we are dead - we cannot go back to the original stream)

###

Section B.4


     1)  ----------------- IKE_SA_INIT Exchange -----------------
         (IP_I1:UDP4500 -> IP_R:UDP4500)
         Non-ESP Marker           ----------->
         Initial IKE_AUTH

The marker ------ foo ------ has been used to describe what follows, but what
follows here is an IKE_AUTH exchange, not an IKE_SA_INIT


     6)  ------------ Retransmit Message from step 2 -------------
         Length + Non-ESP Marker  ----------->
         INFORMATIONAL
         HDR, SK { N(UPDATE_SA_ADDRESSES),
         N(NAT_DETECTION_SOURCE_IP),
         N(NAT_DETECTION_DESTINATION_IP) }
                                  <------- Length + Non-ESP Marker
                             HDR, SK { N(NAT_DETECTION_SOURCE_IP),
                                 N(NAT_DETECTION_DESTINATION_IP) }

This seems to miss the line "INFORMATIONAL" on the responder side. Same
for step 7.

   1.  During the IKE_SA_INIT exchange, the client and server exchange
       MOBIKE_SUPPORTED notify payloads to indicate support for MOBIKE.

See above, step 1 shows an IKE_AUTH, not IKE_SA_INIT so does not match this description.


   6.  The client sends the UPDATE_SA_ADDRESSES notify payload on the
       TCP-encapsulated connection.

I find this wording misleading. The UPDATE_SA_ADDRESSES payload is not send on the TCP connection,
but a whole IKE message containing this payload is sent.