[IPsec] Tsvart last call review of draft-ietf-ipsecme-rfc8229bis-06

[Joseph Touch via Datatracker <[e-mail]>]

Reviewer: Joseph Touch
Review result: Ready with Issues

This document has been reviewed as part of the transport area review team's
ongoing effort to review key IETF documents. These comments were written
primarily for the transport area directors, but are copied to the document's
authors and WG to allow them to address any issues raised and also to the IETF
discussion list for information.

When done at the time of IETF Last Call, the authors should consider this
review as part of the last-call comments they receive. Please always CC
[e-mail] if you reply to or forward this review.

Overall, this document adds useful clarifications to the original RFC on
tunneling IPsec over TCP. There are a number of issues that should be addressed
as it proceeds, as noted below. All can be addressed relatively directly (i.e.,
none create new open issues).

General comments:

The document lacks (and would benefit from) a section providing details of the
differences in this update.

Figures should include captions.

Given the new document adds primarily clarifications, it would be preferable if
the header numbering were not gratuitously modified vs. the original. The new
section 2 should be demoted to 1.2 as per the original; this would go a long
way to avoiding unnecessary confusion between the two.

Specific suggestions and concerns:

Section 3 clarifies the meaning of the 16-bit length field as including both
the message and the message length field. This is counterintuitive and
problematic, notably because ESP messages could be up to 65535 bytes long. This
possibility should be addressed (e.g., prohibit tunneling of messages over
65533 bytes).

Section 4.2 claims the length cannot be 0 or 1 bytes; again, this suggests it
might have been better to have the length field no include the length itself.
Regardless, it seems there are other lengths that are equally invalid (isn’t
there a min ESP header size? What about the IP packet header inside)? The true
min should be indicated.

Section 7.1 suggests closing idle TCP connections to clean up resources; this
is inconsistent with TCP’s basic premise (don’t clean it up until those
resources are used for a new connection). There should be a more direct reason
given for this change.

Section 7.1 mentions a keep-alive; it would be useful to explain whether this
is intended to use TCP keepalives or IPsec keepalives or both. It may also be
important to indicate how these keepalives might interact. This might refer to
the more detailed discussion in Section 7.6.

Section 7.2 on retransmissions should explain the need for IPsec to continue to
retransmit messages even though the transport ensures reliable delivery (e.g.,
that messages could be ignored or delayed elsewhere in receiver processing).

Section 7.7 discusses the relation of the IPsec DFs to the outer TCP DFs. As
with all tunnels, there need be no direct relation. The outer TCP header acts
as a link layer protocol and its frag/reassembly need have no correlation to
the inner payload. Additionally, it is nonsensical to relate the two for TCP as
a tunnel because it does not preserve message boundaries of the carried IPsec
traffic anyway. It might be useful to mention this, rather than indicating this
as “not possible”. (i.e., even if it were possible, it would be incorrect to do
so).

Section 10.1 indicates problems with TCP-in-TCP. It would probably be useful to
provide a citation with better treatment of this issue (e.g,,
https://www.spiedigitallibrary.org/conference-proceedings-of-spie/6011/1/Understanding-TCP-over-TCP--effects-of-TCP-tunneling-on/10.1117/12.630496.short?SSO=1,
https://link.springer.com/chapter/10.1007/978-981-13-3329-3_32). This is more
commonly referred to as “TCP meltdown”; bufferbloat is a different phenomenon
with a different cause (in-network queuing that relies on tail-drop and/or
lacks ECN), and does not appear relevant to the issues presented in this
section.

Section 11 (security considerations) mentions the new vulnerability introduced
by the outer TCP layer, but only DoS via SYN-flooding. This connection is also
susceptible to RST and other spoofing attacks attacks (RFC4953), which should
be noted as well. Data injection attacks are not possible, but all the rest of
the TCP machinery remains vulnerable.

--