Re: [PATCH] netfilter: nf_conntrack_sip: fix OOB read in SIP URI port parsing

[Florian Westphal <fw@strlen.de>]

Jenny Guanni Qu <qguanni@gmail.com> wrote:
> In epaddr_len() and ct_sip_parse_header_uri(), after sip_parse_addr()
> parses an IP address, the pointer (dptr or c) may point at or past
> limit. The subsequent check for a ':' port separator dereferences the
> pointer without a bounds check, causing a 1-byte out-of-bounds read.
> 
> Add bounds checks before the dereference in both locations.

I'm not sure.

This bug is real, but I suspect there are many many more bugs in this
code.

> 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>
> Tested-by: Jenny Guanni Qu <qguanni@gmail.com>
> Signed-off-by: Jenny Guanni Qu <qguanni@gmail.com>
> ---
>  net/netfilter/nf_conntrack_sip.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
> index d0eac27f6ba0..a232054d7919 100644
> --- a/net/netfilter/nf_conntrack_sip.c
> +++ b/net/netfilter/nf_conntrack_sip.c
> @@ -194,7 +194,7 @@ static int epaddr_len(const struct nf_conn *ct, const char *dptr,
>  	}
>  
>  	/* Port number */
> -	if (*dptr == ':') {
> +	if (dptr < limit && *dptr == ':') {
>  		dptr++;
>  		dptr += digits_len(ct, dptr, limit, shift);
>  	}
> @@ -520,7 +520,7 @@ 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 == ':') {
> +	if (c < limit && *c == ':') {
>  		c++;
>  		p = simple_strtoul(c, (char **)&c, 10);

I'm not sure this check is enough.  simple_strtoul() assumes
a c-string.  Are we dealing with a c-string here?

I suspect the anser is: 'no' and that we depend on 0 bytes appearing in
skb_shinfo at end of network buffer for this to 'work'.

I would prefer to add much stricter constraints everywhere.

Untested example:
static bool sip_parse_port(const char *dptr, const char **endp, const char *limit)
{
        static const unsigned int max = strlen("65535");
        int len = 0;

        /* port is optional, but dptr >= limit indicates malformed
         * sip message.
         */
        if (dptr >= limit)
                return false;

        if (*dptr != ':') /* this is fine, no port provided. */
                return true;

        while (dptr < limit && isdigit(*dptr)) {
                dptr++;
                len++;

                if (len > max)
                        return false;
        }

        /* reached limit while parsing port */
        if (dptr >= limit)
                return false;

        if (endp)
                *endp = dptr;

        return true;
}

@@ -193,11 +225,9 @@ 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))
+               return 0;
+
        return dptr - aux;
 }

Whats your take?