[PATCH net v2] inet: frags: prevent acceptance of partially authenticated reassembly

[PATCH net v2] inet: frags: prevent acceptance of partially authenticated reassembly

[Jérémy Jean]

A reassembled skb retains the secpath from its first fragment, which
makes it possible for the final policy check to accept bytes that were
not covered by the transform recorded on the skb. This can affect IPsec
tunnel traffic, including AH and ESP, and allow a packet to be accepted
even though only part of its payload was authenticated.

Reject a fragment when its secpath differs from the fragments already
in the queue, and keep overlap statistics limited to actual overlaps.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
Reviewed-by: Eric Dumazet <edumazet@kernel.org>
---
Changes in v2, after review by Eric Dumazet:
 - Remove check on verified_cnt.

v1: https://lore.kernel.org/all/20261023074515.2347924-2-Jeremy.Jean@oss.cyber.gouv.fr/

 include/net/inet_frag.h  |  1 +
 include/net/xfrm.h       | 17 +++++++++++++++++
 net/ipv4/inet_fragment.c |  5 +++++
 net/ipv4/ip_fragment.c   |  3 ++-
 net/ipv6/reassembly.c    |  3 ++-
 5 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 365925c9d2628..b02d2472b003e 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -171,6 +171,7 @@ extern const u8 ip_frag_ecn_table[16];
 #define IPFRAG_OK	0
 #define IPFRAG_DUP	1
 #define IPFRAG_OVERLAP	2
+#define IPFRAG_MIXED_SECPATH	3
 int inet_frag_queue_insert(struct inet_frag_queue *q, struct sk_buff *skb,
 			   int offset, int end);
 void *inet_frag_reasm_prepare(struct inet_frag_queue *q, struct sk_buff *skb,
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index a6d69aaa6cd2d..298ca9bb00835 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1201,6 +1201,23 @@ secpath_reset(struct sk_buff *skb)
 #endif
 }
 
+static inline bool
+secpath_equal(const struct sk_buff *skb1, const struct sk_buff *skb2)
+{
+	const struct sec_path *sp1 = skb_sec_path(skb1);
+	const struct sec_path *sp2 = skb_sec_path(skb2);
+	int i;
+
+	if (!sp1 || !sp2)
+		return sp1 == sp2;
+	if (sp1->len != sp2->len)
+		return false;
+	for (i = 0; i < sp1->len; i++)
+		if (sp1->xvec[i] != sp2->xvec[i])
+			return false;
+	return true;
+}
+
 static inline int
 xfrm_addr_any(const xfrm_address_t *addr, unsigned short family)
 {
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index b286ee429da8f..c18da91c1f031 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -23,6 +23,7 @@
 #include <net/inet_ecn.h>
 #include <net/ip.h>
 #include <net/ipv6.h>
+#include <net/xfrm.h>
 
 /* Use skb->cb to track consecutive/adjacent fragments coming at
  * the end of the queue. Nodes in the rb-tree queue will
@@ -441,6 +442,10 @@ int inet_frag_queue_insert(struct inet_frag_queue *q, struct sk_buff *skb,
 {
 	struct sk_buff *last = q->fragments_tail;
 
+	/* The reassembled skb keeps the first fragment's secpath. */
+	if (last && !secpath_equal(last, skb))
+		return IPFRAG_MIXED_SECPATH;
+
 	/* An IP fragment is never a GSO packet, but an untrusted source
 	 * (virtio_net_hdr) may have attached GSO metadata to it. Do not let
 	 * that reach the reassembled skb, whose head keeps the first
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index c790d2f494870..36105dd192823 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -378,8 +378,9 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb, int *refs)
 		err = -EINVAL;
 		goto err;
 	}
+	if (err == IPFRAG_OVERLAP)
+		__IP_INC_STATS(net, IPSTATS_MIB_REASM_OVERLAPS);
 	err = -EINVAL;
-	__IP_INC_STATS(net, IPSTATS_MIB_REASM_OVERLAPS);
 discard_qp:
 	inet_frag_kill(&qp->q, refs);
 	__IP_INC_STATS(net, IPSTATS_MIB_REASMFAILS);
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 05c51f669754a..53a685f5c6b7f 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -240,8 +240,9 @@ static int ip6_frag_queue(struct net *net,
 		err = -EINVAL;
 		goto err;
 	}
+	if (err == IPFRAG_OVERLAP)
+		__IP6_INC_STATS(net, idev, IPSTATS_MIB_REASM_OVERLAPS);
 	err = -EINVAL;
-	__IP6_INC_STATS(net, idev, IPSTATS_MIB_REASM_OVERLAPS);
 discard_fq:
 	inet_frag_kill(&fq->q, refs);
 	__IP6_INC_STATS(net, idev, IPSTATS_MIB_REASMFAILS);
-- 
2.47.3

Re: [PATCH net v2] inet: frags: prevent acceptance of partially authenticated reassembly

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2] inet: frags: prevent acceptance of partially authenticated reassembly

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] secpath_equal() (include/net/xfrm.h) treats two secpaths as
  equal only if they have the same len and the same xfrm_state pointer…
- [Low] The new secpath check in inet_frag_queue_insert()
  (net/ipv4/inet_fragment.c:446-447) runs before the overlap/duplicate…

--

inet: frags: prevent acceptance of partially authenticated reassembly

This patch adds secpath_equal(). It makes inet_frag_queue_insert() return
a new IPFRAG_MIXED_SECPATH code when an incoming fragment's secpath
differs from the secpath of the queue tail. The IPv4 and IPv6 reassembly
paths now count IPSTATS_MIB_REASM_OVERLAPS only for real overlaps.

> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index a6d69aaa6cd2d..298ca9bb00835 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1201,6 +1201,23 @@ secpath_reset(struct sk_buff *skb)
>  #endif
>  }
>  
> +static inline bool
> +secpath_equal(const struct sk_buff *skb1, const struct sk_buff *skb2)
> +{
> +	const struct sec_path *sp1 = skb_sec_path(skb1);
> +	const struct sec_path *sp2 = skb_sec_path(skb2);
> +	int i;
> +
> +	if (!sp1 || !sp2)
> +		return sp1 == sp2;
> +	if (sp1->len != sp2->len)
> +		return false;
> +	for (i = 0; i < sp1->len; i++)
> +		if (sp1->xvec[i] != sp2->xvec[i])
> +			return false;
> +	return true;
> +}

[Severity: Medium]
Is comparing xvec[] by xfrm_state pointer stricter than the final policy
check requires?

xfrm_state_ok() in net/xfrm/xfrm_policy.c matches a state against a
template. It does not require one particular SA:

	return	x->id.proto == tmpl->id.proto &&
		(x->id.spi == tmpl->id.spi || !tmpl->id.spi) &&
		(x->props.reqid == tmpl->reqid || !tmpl->reqid) &&
		x->props.mode == tmpl->mode &&
		...
		(if_id == 0 || if_id == x->if_id);

This means several distinct xfrm_state objects can each be valid for the
same flow. Before this patch, a datagram whose fragments were
decapsulated by different but policy-equivalent SAs would reassemble and
pass __xfrm_policy_check(). Would such a datagram now fail reassembly
with IPFRAG_MIXED_SECPATH?

This seems possible in these setups:

  - per-CPU SAs (struct xfrm_state::pcpu_num) on a sending gateway that
    forwards inner fragments without conntrack, where the first and later
    fragments are handled on different CPUs (for example, NIC RSS hashing
    on the UDP 4-tuple)
  - an outbound rekey between fragments of one datagram
  - duplicate CHILD_SAs that share the same reqid

In the per-CPU SA case, the drop could repeat for every fragmented
datagram.

The commit message also says:

    Reject a fragment when its secpath differs from the fragments already
    in the queue, and keep overlap statistics limited to actual overlaps.

The code seems to drop the whole queue, not only the one fragment. In
ip_frag_queue(), IPFRAG_MIXED_SECPATH falls through insert_error into
discard_qp. ip6_frag_queue() does the same with discard_fq:

discard_qp:
	inet_frag_kill(&qp->q, refs);
	__IP_INC_STATS(net, IPSTATS_MIB_REASMFAILS);

nf_ct_frag6_queue() goes to insert_error, which also calls
inet_frag_kill().

The patch is tagged for stable. Should the commit message say that the
whole datagram is discarded, and that legitimate IPsec traffic can now be
dropped this way? If the strict pointer comparison is a deliberate
security trade-off, could that be documented as well?

> diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> index b286ee429da8f..c18da91c1f031 100644
> --- a/net/ipv4/inet_fragment.c
> +++ b/net/ipv4/inet_fragment.c

[ ... ]

> @@ -441,6 +442,10 @@ int inet_frag_queue_insert(struct inet_frag_queue *q, struct sk_buff *skb,
>  {
>  	struct sk_buff *last = q->fragments_tail;
>  
> +	/* The reassembled skb keeps the first fragment's secpath. */
> +	if (last && !secpath_equal(last, skb))
> +		return IPFRAG_MIXED_SECPATH;
> +

[Severity: Low]
This check runs before the overlap and duplicate classification. The
RFC5722 comment further down in inet_frag_queue_insert() still says:

	 * Duplicates, however, should be ignored (i.e. skb dropped, but the
	 * queue/fragments kept for later reassembly).

Take an exact duplicate of a queued fragment that carries a different
secpath, or no secpath at all (for example, a plaintext copy of an inner
fragment that arrived through an ESP tunnel). It now gets
IPFRAG_MIXED_SECPATH instead of IPFRAG_DUP.

In nf_ct_frag6_queue(), that skips the duplicate branch:

	if (err) {
		if (err == IPFRAG_DUP) {
			/* No error for duplicates, pretend they got queued. */
			...
		}
		goto insert_error;
	}

ip_frag_queue() and ip6_frag_queue() treat it like an overlap. All three
paths end in inet_frag_kill(), so the queue is destroyed instead of the
duplicate being dropped quietly.

Should the comment say that a mixed secpath is fatal even for duplicates?
Or should the handling change to match the comment?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023185118.2588450-2-Jeremy.Jean%40oss.cyber.gouv.fr