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.
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.
>
> > ----------------------------------------------------------------------
> > 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?
>
> > 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.
>
_______________________________________________
IPsec mailing list
IPsec@ietf.org
https://www.ietf.org/mailman/listinfo/ipsec
