[PATCH v2] netfilter: nf_conntrack_sip: add bounds-checked port parsing helper

[PATCH v2] netfilter: nf_conntrack_sip: add bounds-checked port parsing helper

[Jenny Guanni Qu]

Replace unsafe port parsing in epaddr_len(), ct_sip_parse_header_uri(),
and ct_sip_parse_request() with a new sip_parse_port() helper that
validates each digit against the buffer limit, eliminating the use of
simple_strtoul() which assumes NUL-terminated strings.

The previous code dereferenced pointers without bounds checks after
sip_parse_addr() and relied on simple_strtoul() on non-NUL-terminated
skb data. A port that reaches the buffer limit without a trailing
character is also rejected as malformed.

Based on a suggestion by Florian Westphal.

Fixes: 05e3ced297fe ("[NETFILTER]: nf_conntrack_sip: introduce SIP-URI parsing helper")
Reported-by: Klaudia Kloc <klaudia@vidocsecurity.com>
Reported-by: Dawid Moczadło <dawid@vidocsecurity.com>
Suggested-by: Florian Westphal <fw@strlen.de>
Tested-by: Jenny Guanni Qu <qguanni@gmail.com>
Signed-off-by: Jenny Guanni Qu <qguanni@gmail.com>
---
 net/netfilter/nf_conntrack_sip.c | 80 +++++++++++++++++++++++---------
 1 file changed, 57 insertions(+), 23 deletions(-)

diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index d0eac27f6ba0..00ecb5df5b5d 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -181,6 +181,57 @@ static int sip_parse_addr(const struct nf_conn *ct, const char *cp,
 	return 1;
 }
 
+/* Parse optional port number after IP address.
+ * Returns false on malformed input, true otherwise.
+ * If port is non-NULL, stores parsed port in network byte order.
+ * If no port is present, sets *port to default SIP port.
+ */
+static bool sip_parse_port(const char *dptr, const char **endp,
+			   const char *limit, __be16 *port)
+{
+	unsigned int p = 0;
+	int len = 0;
+
+	if (dptr >= limit)
+		return false;
+
+	if (*dptr != ':') {
+		if (port)
+			*port = htons(SIP_PORT);
+		if (endp)
+			*endp = dptr;
+		return true;
+	}
+
+	dptr++; /* skip ':' */
+
+	while (dptr < limit && isdigit(*dptr)) {
+		p = p * 10 + (*dptr - '0');
+		dptr++;
+		len++;
+		if (len > 5) /* max "65535" */
+			return false;
+	}
+
+	if (len == 0)
+		return false;
+
+	/* reached limit while parsing port */
+	if (dptr >= limit)
+		return false;
+
+	if (port) {
+		if (p < 1024 || p > 65535)
+			return false;
+		*port = htons(p);
+	}
+
+	if (endp)
+		*endp = dptr;
+
+	return true;
+}
+
 /* skip ip address. returns its length. */
 static int epaddr_len(const struct nf_conn *ct, const char *dptr,
 		      const char *limit, int *shift)
@@ -193,11 +244,8 @@ static int epaddr_len(const struct nf_conn *ct, const char *dptr,
 		return 0;
 	}
 
-	/* Port number */
-	if (*dptr == ':') {
-		dptr++;
-		dptr += digits_len(ct, dptr, limit, shift);
-	}
+	if (!sip_parse_port(dptr, &dptr, limit, NULL))
+		return 0;
 	return dptr - aux;
 }
 
@@ -241,7 +289,6 @@ int ct_sip_parse_request(const struct nf_conn *ct,
 {
 	const char *start = dptr, *limit = dptr + datalen, *end;
 	unsigned int mlen;
-	unsigned int p;
 	int shift = 0;
 
 	/* Skip method and following whitespace */
@@ -267,14 +314,8 @@ int ct_sip_parse_request(const struct nf_conn *ct,
 
 	if (!sip_parse_addr(ct, dptr, &end, addr, limit, true))
 		return -1;
-	if (end < limit && *end == ':') {
-		end++;
-		p = simple_strtoul(end, (char **)&end, 10);
-		if (p < 1024 || p > 65535)
-			return -1;
-		*port = htons(p);
-	} else
-		*port = htons(SIP_PORT);
+	if (!sip_parse_port(end, &end, limit, port))
+		return -1;
 
 	if (end == dptr)
 		return 0;
@@ -509,7 +550,6 @@ int ct_sip_parse_header_uri(const struct nf_conn *ct, const char *dptr,
 			    union nf_inet_addr *addr, __be16 *port)
 {
 	const char *c, *limit = dptr + datalen;
-	unsigned int p;
 	int ret;
 
 	ret = ct_sip_walk_headers(ct, dptr, dataoff ? *dataoff : 0, datalen,
@@ -520,14 +560,8 @@ int ct_sip_parse_header_uri(const struct nf_conn *ct, const char *dptr,
 
 	if (!sip_parse_addr(ct, dptr + *matchoff, &c, addr, limit, true))
 		return -1;
-	if (*c == ':') {
-		c++;
-		p = simple_strtoul(c, (char **)&c, 10);
-		if (p < 1024 || p > 65535)
-			return -1;
-		*port = htons(p);
-	} else
-		*port = htons(SIP_PORT);
+	if (!sip_parse_port(c, &c, limit, port))
+		return -1;
 
 	if (dataoff)
 		*dataoff = c - dptr;
-- 
2.34.1

Re: [PATCH v2] netfilter: nf_conntrack_sip: add bounds-checked port parsing helper

[Florian Westphal]

Jenny Guanni Qu <qguanni@gmail.com> wrote:
> +	/* reached limit while parsing port */
> +	if (dptr >= limit)
> +		return false;
> +
> +	if (port) {
> +		if (p < 1024 || p > 65535)
> +			return false;
> +		*port = htons(p);
> +	}

I like the port range check, but should we make this universal?

	if (p < 1024 || p > 65535)
		return false;
	if (port)
		*port = htons(p);

?

Re: [PATCH v2] netfilter: nf_conntrack_sip: add bounds-checked port parsing helper

[Pablo Neira Ayuso]

Hi,

On Sat, Mar 14, 2026 at 12:36:33AM +0100, Florian Westphal wrote:
> Jenny Guanni Qu <qguanni@gmail.com> wrote:
> > +	/* reached limit while parsing port */
> > +	if (dptr >= limit)
> > +		return false;
> > +
> > +	if (port) {
> > +		if (p < 1024 || p > 65535)
> > +			return false;
> > +		*port = htons(p);
> > +	}
> 
> I like the port range check, but should we make this universal?
> 
> 	if (p < 1024 || p > 65535)
> 		return false;
> 	if (port)
> 		*port = htons(p);
> 
> ?

Can anyone have a look into following up on this?

Otherwise, I will try to have a look for the next PR with fixes.

Re: [PATCH v2] netfilter: nf_conntrack_sip: add bounds-checked port parsing helper

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Jenny Guanni Qu <qguanni@gmail.com> wrote:
> > +	/* reached limit while parsing port */
> > +	if (dptr >= limit)
> > +		return false;
> > +
> > +	if (port) {
> > +		if (p < 1024 || p > 65535)
> > +			return false;
> > +		*port = htons(p);
> > +	}
> 
> I like the port range check, but should we make this universal?
> 
> 	if (p < 1024 || p > 65535)
> 		return false;
> 	if (port)
> 		*port = htons(p);

Ping, will you send a v3 or do you expect us to take over from here?