Martin Duke <martin.h.d...@gmail.com> writes:
Comments inline.
On Tue, Aug 9, 2022 at 8:56 PM Christian Hopps <cho...@chopps.org>
wrote:
Thanks for the thorough review! Comments inline..
Martin Duke via Datatracker <nore...@ietf.org> writes:
> (6) As malformed packets are sometimes an attack vector, it
would be good to
> specify behavior in response to pathological BlockOffsets, for
instance:
>
> - What if two BlockOffset fields disagree? e.g., with 500 byte
outer packets,
> what if the sequence of block offsets is {0, 750, 100}? Does
the third packet
> have 250 or 100 bytes of the first data block? Drop the packet,
kill the SA,
> ignore one and accept the other, or something else?
The block offset is pointing at the start of the next packet
(which may be beyond the current packets boundary). So it also
represents what is left in the current inner packet being
reassembled. When the offset doesn't agree with the known length
of the inner being reassembled, the inner is simply dropped and
you move to the start of the next packet (which is what the block
offset points to).
It should be noted that these values are in the cipher text (i.e.
they are encrypted inside the ESP wrapper), so getting bad values
here is almost for sure due to a bug/corruption on a validated
sender rather than an attack. :)
Do I understand correctly that the inner packet's native length field
is the ground truth, rather than the block offset? I actually don't
care how these conflicts are resolved, just that the text resolves
them.
That's correct, it's the only place the actual length is, no duplication. The block offset always points at the start of the next packet. From 2.2.1: Likewise, the length of the data block is extracted from the encapsulated IPv4's Total Length or IPv6's Payload Length fields. From 2.2: [.. diagram showing "DataBlocks" and "BlockOffset" ..] If the BlockOffset value is zero it means that the DataBlocks data begins with a new data block. Conversely, if the BlockOffset value is non-zero it points to the start of the new data block, and the initial DataBlocks data belongs to the data block that is still being re-assembled.
I am not an expert on these attacks, nor do I have a well-thought-out
threat model, but IIUC these sorts of problems usually manifest as
buffer overflows and the like, not as injected packets. In any case,
it's better to have well-defined protocol behavior on unexpected
input.
> - What if a pad block is in a packet with a BlockOffset greater
than the packet
> length? Would the receiver skip over the specified bytes in the
subsequent
> packet, even though padding is supposed to only be at the end
of packets?
This situation can't occur as pad blocks are very simple and hard
to mess up. :) Pad blocks start with 4 0-bits and their length is
"the rest of the packet". By definition if the block offset
points past the end of the outer packet, there is no pad and the
payload is entirely made up of the current inner packet being
reassembled.
OK. The document seems to define a pad block as a kind of data block,
and the BlockOffset field applies to data blocks. So it would be
legal to have an all-padding packet with a BlockOffset > outer packet
size, IIUC.
No, pad blocks are always from their start to the end of the outer packet. You would
never be fragmenting (thus "continuing" in the next packet) a pad block.
Again from 2.2:
Conversely, if the BlockOffset value is non-zero it points to the
start of the new data block, and the initial DataBlocks data belongs
to the data block that is still being re-assembled.
Pad blocks are never fragmented or reassembled.
From 6.1.3.3: Pad Data Block
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
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| 0x0 | Padding ...
+-+-+-+-+-+-+-+-+-+-+-
Figure 9: Pad Data Block format
Type:
A 4-bit value of 0x0 indicating a padding data block.
Padding:
Extends to end of the encapsulating packet.
>
----------------------------------------------------------------------
> COMMENT:
>
----------------------------------------------------------------------
>
> Thanks to Joe Touch for 2 TSVART reviews, and for addressing
his comments. Also
> thanks for the very literate discussion of congestion control.
>
> (2.2.3) It would be nice to at least suggest a default number
for the
> reordering window. In TCP, we traditionally use 3, but really
any suggestion
> for the clueless is fine.
We could add the text "TCP traditionally uses 3" if you'd like.
:)
Sure.
> (3) Please clarify: is TsVal the actual tranmission time, or
the time the
> packet is queued for the next transmission opportunity?
It has to be when when queued as the value is set prior to ESP
encryption.
OK, please clarify in the text.
> (3) This probably just needs a bit more explanation, but
reading this document,
> and not knowing much about ESP, I could not figure out the case
where the
> return path does not support AGGFRAG_PAYLOAD. IIUC, IKEv2
negotiates this for
> the pair explicitly, so this case cannot arise. Otherwise, how
is this
> negotiated? Why would a tunnel endpoint support just AGGFRAG
without payloads
> but not with?
The most common case (for this admittedly uncommon scenario)
would be static configuration of the SAs, where only one side is
configured to use IP-TFS.
I guess my confusion is that this case is not about interacting with
legacy devices; they still have to be updated to support AGGFRAG
without payloads. is there
really that big of a win to implement just the headers without
supporting payloads?
I believe the case that people have in mind is when they only want IPTFS in one direction, not b/c the other direction doesn't support IP-TFS (it has to to handle the no payload case), but b/c they don't want/need to secure that second SA with TFS. Thanks, Chris.
> NITS
> (2.4.1) update the [RFC8229] reference to RFC8229bis?
We wouldn't want to block on this. The normal "updates/replaces"
pointers should take care of things if/when RFC8229bis gets
published, right?
The general practice is to prefer the more up-to-date reference, and
as 8229bis is going through it shouldn't really block. But I'm not
going to insist.
> (6.1) "The value 5 was chosen to not conflict with other used
values." IIUC the
> values here are just Protocol numbers from the registry. So
maybe it's better
> to be more explicit and say that this cannot be used with
RFC1819 streams?
They are specific to ESP, but have traditionally been drawn from
IP protocol numbers. This isn't a requirement though. If you feel
strong we could add that explicit text, but I think it's pretty
obvious this is only for ESP payloads.
I don't feel strongly.
Thanks again for your thorough review!!
Chris.
signature.asc
Description: PGP signature
_______________________________________________ IPsec mailing list IPsec@ietf.org https://www.ietf.org/mailman/listinfo/ipsec
