[PATCH 0/2] Fix handling of ICMP flows with 0 id

[PATCH 0/2] Fix handling of ICMP flows with 0 id

[David Gibson]

Well, that's the impetus, but it's done as a more general changing of
where we sanity check flow endpoints.

Link: https://bugs.passt.top/show_bug.cgi?id=105

David Gibson (2):
  udp: Improve detail of UDP endpoint sanity checking
  flow: Remove over-zealous sanity checks in flow_sidx_hash()

 flow.c     |  7 +------
 udp_flow.c | 32 ++++++++++++++++++++++++--------
 2 files changed, 25 insertions(+), 14 deletions(-)

-- 
2.47.0

[PATCH 1/2] udp: Improve detail of UDP endpoint sanity checking

[David Gibson]

In udp_flow_new() we reject a flow if the endpoint isn't unicast, or it has
a zero endpoint port.  Those conditions aren't strictly illegal, but we
can't safely handle them at present:
 * Multicast UDP endpoints are certainly possible, but our current flow
   tracking only makes sense for simple unicast flows - we'll need
   different handling if we want to handle multicast flows in future
 * It's not entirely clear if port 0 is RFC-ishly correct, but for socket
   interfaces port 0 sometimes has a special meaning such as "pick the port
   for me, kernel".  That makes flows on port 0 unsafe to forward in the
   usual way.

For the same reason we also can't safely handle port 0 as our port.  In
principle that's also true for our address, however in the case of flows
initiated from a socket, we may not know our address since the socket
could be bound to 0.0.0.0 or ::, so we can only verify that our address
is unicast for flows initiated from the tap side.

Refine the current check in udp_flow_new() to slightly more detailed checks
in udp_flow_from_sock() and udp_flow_from_tap() to make what is and isn't
handled clearer.  This makes this checking more similar to what we do for
TCP connections.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp_flow.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/udp_flow.c b/udp_flow.c
index b81be2c..c8fdb5f 100644
--- a/udp_flow.c
+++ b/udp_flow.c
@@ -75,16 +75,10 @@ void udp_flow_close(const struct ctx *c, struct udp_flow *uflow)
 static flow_sidx_t udp_flow_new(const struct ctx *c, union flow *flow,
 				int s_ini, const struct timespec *now)
 {
-	const struct flowside *ini = &flow->f.side[INISIDE];
 	struct udp_flow *uflow = NULL;
 	const struct flowside *tgt;
 	uint8_t tgtpif;
 
-	if (!inany_is_unicast(&ini->eaddr) || ini->eport == 0) {
-		flow_trace(flow, "Invalid endpoint to initiate UDP flow");
-		goto cancel;
-	}
-
 	if (!(tgt = flow_target(c, flow, IPPROTO_UDP)))
 		goto cancel;
 	tgtpif = flow->f.pif[TGTSIDE];
@@ -189,6 +183,7 @@ flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
 			       const union sockaddr_inany *s_in,
 			       const struct timespec *now)
 {
+	const struct flowside *ini;
 	struct udp_flow *uflow;
 	union flow *flow;
 	flow_sidx_t sidx;
@@ -210,7 +205,19 @@ flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
 		return FLOW_SIDX_NONE;
 	}
 
-	flow_initiate_sa(flow, ref.udp.pif, s_in, ref.udp.port);
+	ini = flow_initiate_sa(flow, ref.udp.pif, s_in, ref.udp.port);
+
+	if (!inany_is_unicast(&ini->eaddr) ||
+	    ini->eport == 0 || ini->oport == 0) {
+		/* In principle ini->oddr also must be unicast, but when we've
+		 * been initiated from a socket bound to 0.0.0.0 or ::, we don't
+		 * know our address, so we have to leave it unpopulated.
+		 */
+		flow_err(flow, "Invalid endpoint on UDP recvfrom()");
+		flow_alloc_cancel(flow);
+		return FLOW_SIDX_NONE;
+	}
+
 	return udp_flow_new(c, flow, ref.fd, now);
 }
 
@@ -233,6 +240,7 @@ flow_sidx_t udp_flow_from_tap(const struct ctx *c,
 			      in_port_t srcport, in_port_t dstport,
 			      const struct timespec *now)
 {
+	const struct flowside *ini;
 	struct udp_flow *uflow;
 	union flow *flow;
 	flow_sidx_t sidx;
@@ -256,7 +264,15 @@ flow_sidx_t udp_flow_from_tap(const struct ctx *c,
 		return FLOW_SIDX_NONE;
 	}
 
-	flow_initiate_af(flow, PIF_TAP, af, saddr, srcport, daddr, dstport);
+	ini = flow_initiate_af(flow, PIF_TAP, af, saddr, srcport,
+			       daddr, dstport);
+
+	if (!inany_is_unicast(&ini->eaddr) || ini->eport == 0 ||
+	    !inany_is_unicast(&ini->oaddr) || ini->oport == 0) {
+		flow_dbg(flow, "Invalid endpoint on UDP packet");
+		flow_alloc_cancel(flow);
+		return FLOW_SIDX_NONE;
+	}
 
 	return udp_flow_new(c, flow, -1, now);
 }
-- 
@@ -75,16 +75,10 @@ void udp_flow_close(const struct ctx *c, struct udp_flow *uflow)
 static flow_sidx_t udp_flow_new(const struct ctx *c, union flow *flow,
 				int s_ini, const struct timespec *now)
 {
-	const struct flowside *ini = &flow->f.side[INISIDE];
 	struct udp_flow *uflow = NULL;
 	const struct flowside *tgt;
 	uint8_t tgtpif;
 
-	if (!inany_is_unicast(&ini->eaddr) || ini->eport == 0) {
-		flow_trace(flow, "Invalid endpoint to initiate UDP flow");
-		goto cancel;
-	}
-
 	if (!(tgt = flow_target(c, flow, IPPROTO_UDP)))
 		goto cancel;
 	tgtpif = flow->f.pif[TGTSIDE];
@@ -189,6 +183,7 @@ flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
 			       const union sockaddr_inany *s_in,
 			       const struct timespec *now)
 {
+	const struct flowside *ini;
 	struct udp_flow *uflow;
 	union flow *flow;
 	flow_sidx_t sidx;
@@ -210,7 +205,19 @@ flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
 		return FLOW_SIDX_NONE;
 	}
 
-	flow_initiate_sa(flow, ref.udp.pif, s_in, ref.udp.port);
+	ini = flow_initiate_sa(flow, ref.udp.pif, s_in, ref.udp.port);
+
+	if (!inany_is_unicast(&ini->eaddr) ||
+	    ini->eport == 0 || ini->oport == 0) {
+		/* In principle ini->oddr also must be unicast, but when we've
+		 * been initiated from a socket bound to 0.0.0.0 or ::, we don't
+		 * know our address, so we have to leave it unpopulated.
+		 */
+		flow_err(flow, "Invalid endpoint on UDP recvfrom()");
+		flow_alloc_cancel(flow);
+		return FLOW_SIDX_NONE;
+	}
+
 	return udp_flow_new(c, flow, ref.fd, now);
 }
 
@@ -233,6 +240,7 @@ flow_sidx_t udp_flow_from_tap(const struct ctx *c,
 			      in_port_t srcport, in_port_t dstport,
 			      const struct timespec *now)
 {
+	const struct flowside *ini;
 	struct udp_flow *uflow;
 	union flow *flow;
 	flow_sidx_t sidx;
@@ -256,7 +264,15 @@ flow_sidx_t udp_flow_from_tap(const struct ctx *c,
 		return FLOW_SIDX_NONE;
 	}
 
-	flow_initiate_af(flow, PIF_TAP, af, saddr, srcport, daddr, dstport);
+	ini = flow_initiate_af(flow, PIF_TAP, af, saddr, srcport,
+			       daddr, dstport);
+
+	if (!inany_is_unicast(&ini->eaddr) || ini->eport == 0 ||
+	    !inany_is_unicast(&ini->oaddr) || ini->oport == 0) {
+		flow_dbg(flow, "Invalid endpoint on UDP packet");
+		flow_alloc_cancel(flow);
+		return FLOW_SIDX_NONE;
+	}
 
 	return udp_flow_new(c, flow, -1, now);
 }
-- 
2.47.0

[PATCH 2/2] flow: Remove over-zealous sanity checks in flow_sidx_hash()

[David Gibson]

In flow_sidx_hash() we verify that the flow we're hashing doesn't have an
unspecified endpoint address, or zero for either port.  The hash table only
works if we're looking for exact matches of address and port, and this is
attempting to catch any cases where we might have left address or port
unpopulated or filled with a wildcard.

This doesn't really work though, because there are cases where unspecified
addresses or zero ports are correct:
 * We already use unspecified addresses for our address in cases where we
   don't know the specific local address for that side, and exclude the
   obvious extra check on side->oaddr for that reason.
 * Zero port numbers aren't strictly forbidden over the wire.  We forbid
   them for TCP & UDP because they can't safely be handled on the socket
   side.  However for ICMP a zero id, which goes in the port field is
   valid.
 * Possible future flow types (for example, for multicast protocols) might
   legitimately have an unspecified address.

Although it makes them easier to miss, these sorts of sanity checks really
have to be done at the protocol / flow type layer, and we already do so.
Remove the checks in flow_sidx_hash() other than checking that the pif
is specified.

Link: https://bugs.passt.top/show_bug.cgi?id=105

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 flow.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/flow.c b/flow.c
index 1ea112b..ee1221b 100644
--- a/flow.c
+++ b/flow.c
@@ -597,12 +597,7 @@ static uint64_t flow_sidx_hash(const struct ctx *c, flow_sidx_t sidx)
 	const struct flowside *side = &f->side[sidx.sidei];
 	uint8_t pif = f->pif[sidx.sidei];
 
-	/* For the hash table to work, entries must have complete endpoint
-	 * information, and at least a forwarding port.
-	 */
-	ASSERT(pif != PIF_NONE && !inany_is_unspecified(&side->eaddr) &&
-	       side->eport != 0 && side->oport != 0);
-
+	ASSERT(pif != PIF_NONE);
 	return flow_hash(c, FLOW_PROTO(f), pif, side);
 }
 
-- 
@@ -597,12 +597,7 @@ static uint64_t flow_sidx_hash(const struct ctx *c, flow_sidx_t sidx)
 	const struct flowside *side = &f->side[sidx.sidei];
 	uint8_t pif = f->pif[sidx.sidei];
 
-	/* For the hash table to work, entries must have complete endpoint
-	 * information, and at least a forwarding port.
-	 */
-	ASSERT(pif != PIF_NONE && !inany_is_unspecified(&side->eaddr) &&
-	       side->eport != 0 && side->oport != 0);
-
+	ASSERT(pif != PIF_NONE);
 	return flow_hash(c, FLOW_PROTO(f), pif, side);
 }
 
-- 
2.47.0

Re: [PATCH 0/2] Fix handling of ICMP flows with 0 id

[Stefano Brivio]

On Thu,  5 Dec 2024 15:26:00 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Well, that's the impetus, but it's done as a more general changing of
> where we sanity check flow endpoints.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=105
> 
> David Gibson (2):
>   udp: Improve detail of UDP endpoint sanity checking
>   flow: Remove over-zealous sanity checks in flow_sidx_hash()

Applied.

-- 
Stefano