Eli Britstein via dev <[email protected]> writes:
> IP reassembly must not accept data past an MF=0 fragment. Track
> last fragments, reject ranges beyond them on insert, and require the
> sorted rightmost fragment to be MF=0 before completing a list.
>
> The accompanying test is added to tests/ofproto-dpif.at (using
> netdev-dummy/receive with variable-built packets and precomputed IPv4
> header checksums) rather than the system-traffic suite.
>
> Assisted-by: composer-2.5-fast, Cursor
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Signed-off-by: Eli Britstein <[email protected]>
> ---
The AI review flagged that this behavior can have a racy kind of DoS
where overlaps will pin a bad frag queue forever. It is probably better
to follow the RFC 5722 behavior closer. See below.
> lib/dpctl.c | 4 ++
> lib/dpif-provider.h | 1 +
> lib/ipf.c | 48 +++++++++++++++++++--
> lib/ipf.h | 1 +
> tests/ofproto-dpif.at | 74 ++++++++++++++++++++++++++++++++
> tests/system-userspace-macros.at | 8 ++++
> 6 files changed, 133 insertions(+), 3 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 48afb8549..b147853ca 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -2629,6 +2629,8 @@ dpctl_ct_ipf_get_status(int argc, const char *argv[],
> dpif_ipf_status.v4.nfrag_too_large);
> dpctl_print(dpctl_p, " v4 frags overlapped: %"PRIu64"\n",
> dpif_ipf_status.v4.nfrag_overlap);
> + dpctl_print(dpctl_p, " v4 frags beyond last: %"PRIu64"\n",
> + dpif_ipf_status.v4.nfrag_beyond_last);
> dpctl_print(dpctl_p, " v4 frags purged: %"PRIu64"\n",
> dpif_ipf_status.v4.nfrag_purged);
>
> @@ -2646,6 +2648,8 @@ dpctl_ct_ipf_get_status(int argc, const char *argv[],
> dpif_ipf_status.v6.nfrag_too_large);
> dpctl_print(dpctl_p, " v6 frags overlapped: %"PRIu64"\n",
> dpif_ipf_status.v6.nfrag_overlap);
> + dpctl_print(dpctl_p, " v6 frags beyond last: %"PRIu64"\n",
> + dpif_ipf_status.v6.nfrag_beyond_last);
> dpctl_print(dpctl_p, " v6 frags purged: %"PRIu64"\n",
> dpif_ipf_status.v6.nfrag_purged);
> } else {
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index b3dd58e9d..6f667fed1 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -131,6 +131,7 @@ struct dpif_ipf_proto_status {
> uint64_t nfrag_too_small;
> uint64_t nfrag_too_large;
> uint64_t nfrag_overlap;
> + uint64_t nfrag_beyond_last;
> uint64_t nfrag_purged;
> unsigned int min_frag_size;
> bool enabled;
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 2b016e60b..1a16908c7 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -84,6 +84,7 @@ enum ipf_counter_type {
> IPF_NFRAGS_TOO_SMALL,
> IPF_NFRAGS_TOO_LARGE,
> IPF_NFRAGS_OVERLAP,
> + IPF_NFRAGS_BEYOND_LAST,
> IPF_NFRAGS_PURGED,
> IPF_NFRAGS_NUM_CNTS,
> };
> @@ -98,6 +99,7 @@ struct ipf_frag {
> struct dp_packet *pkt;
> uint16_t start_data_byte;
> uint16_t end_data_byte;
> + bool last_frag; /* True if this was the MF=0 fragment. */
> };
>
> /* The key for a collection of fragments potentially making up an
> unfragmented
> @@ -397,8 +399,14 @@ ipf_list_complete(const struct ipf_list *ipf_list)
> != ipf_list->frag_list[i].start_data_byte) {
> return false;
> }
> +
> + /* Only the final fragment may have MF=0; data past it is invalid. */
> + if (ipf_list->frag_list[i - 1].last_frag) {
> + return false;
> + }
> }
> - return true;
> +
> + return ipf_list->frag_list[ipf_list->last_inuse_idx].last_frag;
> }
>
> /* Runs O(n) for a sorted or almost sorted list. */
> @@ -859,6 +867,28 @@ ipf_is_frag_duped(const struct ipf_frag *frag_list, int
> last_inuse_idx,
> return false;
> }
>
> +/* Returns true if accepting this fragment would place data past an MF=0
> + * (last) fragment, which is illegal for IP reassembly. */
Instead of 'illegal' please write the RFC that the code is complying
with - that makes it easier to cross check the behavior. Best would be
to include the RFC ID + section.
> +static bool
> +ipf_is_beyond_last_frag(const struct ipf_frag *frag_list, int last_inuse_idx,
> + uint16_t start_data_byte, uint16_t end_data_byte,
> + bool lf)
> + /* OVS_REQUIRES(ipf_lock) */
> +{
> + for (int i = 0; i <= last_inuse_idx; i++) {
> + if (frag_list[i].last_frag
> + && start_data_byte > frag_list[i].end_data_byte) {
> + return true;
> + }
> +
> + if (lf && frag_list[i].end_data_byte > end_data_byte) {
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> /* Adds a fragment to a list of fragments, if the fragment is not a
> * duplicate. If the fragment is a duplicate, the fragment is dropped
> * to avoid the work that conntrack would do to mark the fragment
> @@ -872,14 +902,17 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list
> *ipf_list,
> {
> bool duped_frag = ipf_is_frag_duped(ipf_list->frag_list,
> ipf_list->last_inuse_idx, start_data_byte, end_data_byte);
> + bool beyond_last = ipf_is_beyond_last_frag(ipf_list->frag_list,
> + ipf_list->last_inuse_idx, start_data_byte, end_data_byte, lf);
> int last_inuse_idx = ipf_list->last_inuse_idx;
>
> - if (!duped_frag) {
> + if (!duped_frag && !beyond_last) {
> if (last_inuse_idx < ipf_list->size - 1) {
> struct ipf_frag *frag = &ipf_list->frag_list[last_inuse_idx + 1];
> frag->pkt = pkt;
> frag->start_data_byte = start_data_byte;
> frag->end_data_byte = end_data_byte;
> + frag->last_frag = lf;
> ipf_list->last_inuse_idx++;
> atomic_count_inc(&ipf->nfrag);
> ipf_count(ipf, v6, IPF_NFRAGS_ACCEPTED);
> @@ -887,11 +920,16 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list
> *ipf_list,
> } else {
> OVS_NOT_REACHED();
> }
> - } else {
> + } else if (duped_frag) {
> ipf_count(ipf, v6, IPF_NFRAGS_OVERLAP);
duped != overlap
Pkt:
0 8 16 24
|--------|--------|--------|
AAAAAAAA AAAAAAAA BBBBBBBB
Sender looks like:
Fragment 1 (f1):
offset=0, length=16
+------------------+
| AAAAAAAAAAAAAAAA |
+------------------+
Fragment 2 (f2):
offset=16, length=8
+----------+
| BBBBBBBB |
+----------+
Let's say network delivers frag 1 twice:
f1, f1, f2
f1 has the exact same byte range, offset, etc. This is a duplicate
fragment. RFC 8200 *explicitly* calls this out, and allows discarding
just the fragment in question:
It should be noted that fragments may be duplicated in the
network. Instead of treating these exact duplicate fragments
as overlapping fragments, an implementation may choose to
detect this case and drop exact duplicate fragments while
keeping the other fragments belonging to the same packet.
This is also what kernel does.
OTOH, assume that on the wire comes something like:
Fragment 1:
offset=0, length=16
0 16
+-----------------+
| AAAAAAAAAAAAAAA |
+-----------------+
Fragment 2:
offset=8, length=16
8 24
+-----------------+
| BBBBBBBBBBBBBBB |
+-----------------+
This results in an overlap:
Pkt:
0 8 16 24
|--------|--------|--------|
AAAAAAAA AAAAAAAA <-- f1
f2 --> BBBBBBBB BBBBBBBB
This is what is called an overlap, from my reading of RFC 815 and 8200,
and is expected (in ipv6 case) to completely discard the entire
reassembly queue for the packet in question for security reasons.
> dp_packet_delete(pkt);
> return true;
> + } else {
> + ipf_count(ipf, v6, IPF_NFRAGS_BEYOND_LAST);
> + dp_packet_delete(pkt);
> + return true;
> }
> +
> return true;
> }
>
> @@ -1486,6 +1524,8 @@ ipf_get_status(struct ipf *ipf, struct ipf_status
> *ipf_status)
> &ipf_status->v4.nfrag_too_large);
> atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_OVERLAP],
> &ipf_status->v4.nfrag_overlap);
> + atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_BEYOND_LAST],
> + &ipf_status->v4.nfrag_beyond_last);
> atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_PURGED],
> &ipf_status->v4.nfrag_purged);
>
> @@ -1504,6 +1544,8 @@ ipf_get_status(struct ipf *ipf, struct ipf_status
> *ipf_status)
> &ipf_status->v6.nfrag_too_large);
> atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_OVERLAP],
> &ipf_status->v6.nfrag_overlap);
> + atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_BEYOND_LAST],
> + &ipf_status->v6.nfrag_beyond_last);
> atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_PURGED],
> &ipf_status->v6.nfrag_purged);
> return 0;
> diff --git a/lib/ipf.h b/lib/ipf.h
> index 2ac3c9658..59ae887c3 100644
> --- a/lib/ipf.h
> +++ b/lib/ipf.h
> @@ -29,6 +29,7 @@ struct ipf_proto_status {
> uint64_t nfrag_too_small;
> uint64_t nfrag_too_large;
> uint64_t nfrag_overlap;
> + uint64_t nfrag_beyond_last;
> uint64_t nfrag_purged;
> unsigned int min_frag_size;
> bool enabled;
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 0ff4c50f8..032707dd5 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5842,6 +5842,80 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m \
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - fragment handling - reject fragment beyond last])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 90
> +
> +AT_DATA([flows.txt], [dnl
> +table=0 in_port=90,ip actions=ct(commit),output:1
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow11 replace-flows br0 flows.txt])
> +
> +dnl The minimum fragment size is clamped to 400 bytes, so a non-last fragment
> +dnl must be at least that large to be admitted for reassembly.
> +AT_CHECK([ovs-appctl dpctl/ipf-set-min-frag v4 400], [], [dnl
> +setting minimum fragment size successful
> +])
> +
> +dnl First admit a last (MF=0) fragment carrying bytes 400..799, then reject a
> +dnl new fragment carrying bytes 800..1199, whose data lies entirely beyond
> the
> +dnl end of the last fragment and so must not be reassembled.
> +dnl
> +dnl Packet 1 (admitted). Last fragment carrying bytes 400..799 (MF clear).
> +dnl Ethernet II, Src: 50:54:00:00:00:09, Dst: 50:54:00:00:00:0a
> +dnl Type: IPv4 (0x0800)
> +dnl Internet Protocol Version 4, Src: 10.1.1.1, Dst: 10.1.1.2
> +dnl 0100 .... = Version: 4
> +dnl .... 0101 = Header Length: 20 bytes (5)
> +dnl Differentiated Services Field: 0x00 (DSCP: CS0, ECN: Not-ECT)
> +dnl Total Length: 420
> +dnl Identification: 0x0020 (32)
> +dnl 000. .... = Flags: 0x0 (Last fragment)
> +dnl ...0 0000 0011 0010 = Fragment Offset: 400
> +dnl Time to Live: 64
> +dnl Protocol: UDP (17)
> +dnl Header Checksum: 0x62f3
> +dnl Data (400 bytes)
> +eth="50 54 00 00 00 0a 50 54 00 00 00 09 08 00"
> +ip1="45 00 01 a4 00 20 00 32 40 11 62 f3"
> +addrs="0a 01 01 01 0a 01 01 02"
> +data1=$(printf '%0*d' 800 0)
> +packet1="${eth}${ip1}${addrs}${data1}"
> +
> +dnl Packet 2 (rejected as beyond last). Fragment carrying bytes 800..1199
> +dnl (MF set), starting past packet 1's last-fragment end.
> +dnl Ethernet II, Src: 50:54:00:00:00:09, Dst: 50:54:00:00:00:0a
> +dnl Type: IPv4 (0x0800)
> +dnl Internet Protocol Version 4, Src: 10.1.1.1, Dst: 10.1.1.2
> +dnl 0100 .... = Version: 4
> +dnl .... 0101 = Header Length: 20 bytes (5)
> +dnl Differentiated Services Field: 0x00 (DSCP: CS0, ECN: Not-ECT)
> +dnl Total Length: 420
> +dnl Identification: 0x0020 (32)
> +dnl 001. .... = Flags: 0x1 (More fragments)
> +dnl ...0 0000 0110 0100 = Fragment Offset: 800
> +dnl Time to Live: 64
> +dnl Protocol: UDP (17)
> +dnl Header Checksum: 0x42c1
> +dnl Data (400 bytes)
> +ip2="45 00 01 a4 00 20 20 64 40 11 42 c1"
> +data2=$(printf '%0*d' 800 0)
> +packet2="${eth}${ip2}${addrs}${data2}"
> +
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$packet1"])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$packet2"])
> +
> +AT_CHECK([ovs-appctl dpctl/ipf-get-status -m \
> +| grep -E 'num frag:|v4 frags accepted:|v4 frags completed:|v4 frags beyond
> last:'], [], [dnl
> + num frag: 1
> + v4 frags accepted: 1
> + v4 frags completed: 0
> + v4 frags beyond last: 1
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([ofproto-dpif - handling of malformed TCP packets])
> OVS_VSWITCHD_START
> add_of_ports br0 1 90
> diff --git a/tests/system-userspace-macros.at
> b/tests/system-userspace-macros.at
> index 10ce3e746..3579718f8 100644
> --- a/tests/system-userspace-macros.at
> +++ b/tests/system-userspace-macros.at
> @@ -180,6 +180,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status], [], [dnl
> v4 frags too small: 0
> v4 frags too large: 0
> v4 frags overlapped: 0
> + v4 frags beyond last: 0
> v4 frags purged: 0
> min v6 frag size: 1280
> v6 frags accepted: 0
> @@ -188,6 +189,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status], [], [dnl
> v6 frags too small: 0
> v6 frags too large: 0
> v6 frags overlapped: 0
> + v6 frags beyond last: 0
> v6 frags purged: 0
> ])
> ])
> @@ -212,6 +214,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [],
> [dnl
> v4 frags too small: 0
> v4 frags too large: 0
> v4 frags overlapped: 0
> + v4 frags beyond last: 0
> v4 frags purged: 0
> min v6 frag size: 1280
> v6 frags accepted: 0
> @@ -220,6 +223,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [],
> [dnl
> v6 frags too small: 0
> v6 frags too large: 0
> v6 frags overlapped: 0
> + v6 frags beyond last: 0
> v6 frags purged: 0
>
> Fragment Lists:
> @@ -247,6 +251,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [],
> [dnl
> v4 frags too small: 0
> v4 frags too large: 0
> v4 frags overlapped: 0
> + v4 frags beyond last: 0
> v4 frags purged: 0
> min v6 frag size: 1280
> v6 frags accepted: 30
> @@ -255,6 +260,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [],
> [dnl
> v6 frags too small: 0
> v6 frags too large: 0
> v6 frags overlapped: 0
> + v6 frags beyond last: 0
> v6 frags purged: 0
>
> Fragment Lists:
> @@ -289,6 +295,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m |
> FORMAT_FRAG_LIST()], [], [dnl
> v4 frags too small: 0
> v4 frags too large: 0
> v4 frags overlapped: 0
> + v4 frags beyond last: 0
> v4 frags purged: 0
> min v6 frag size: 1280
> v6 frags accepted: 0
> @@ -297,6 +304,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m |
> FORMAT_FRAG_LIST()], [], [dnl
> v6 frags too small: 0
> v6 frags too large: 0
> v6 frags overlapped: 0
> + v6 frags beyond last: 0
> v6 frags purged: 0
>
> Fragment Lists:
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
