netdev - Re: [PATCH] netfilter: nf_conntrack_sip: fix the ct_sip_parse_numerical

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 May 2023 11:43:19 +0000
From:   Gavrilov Ilia <Ilia.Gavrilov@...otecs.ru>
To:     Simon Horman <simon.horman@...igine.com>
CC:     Pablo Neira Ayuso <pablo@...filter.org>,
        Jozsef Kadlecsik <kadlec@...filter.org>,
        Florian Westphal <fw@...len.de>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Patrick McHardy <kaber@...sh.net>,
        "netfilter-devel@...r.kernel.org" <netfilter-devel@...r.kernel.org>,
        "coreteam@...filter.org" <coreteam@...filter.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>
Subject: Re: [PATCH] netfilter: nf_conntrack_sip: fix the
 ct_sip_parse_numerical_param() return value.

On 4/28/23 22:24, Simon Horman wrote:
> On Wed, Apr 26, 2023 at 03:04:31PM +0000, Gavrilov Ilia wrote:
>> ct_sip_parse_numerical_param() returns only 0 or 1 now.
>> But process_register_request() and process_register_response() imply
>> checking for a negative value if parsing of a numerical header parameter
>> failed. Let's fix it.
>>
>> Found by InfoTeCS on behalf of Linux Verification Center
>> (linuxtesting.org) with SVACE.
>>
>> Fixes: 0f32a40fc91a ("[NETFILTER]: nf_conntrack_sip: create signalling expectations")
>> Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@...otecs.ru>
> 
> Hi Gavrilov,
> 

Hi Simon, thank you for your answer.

> although it is a slightly unusual convention for kernel code,
> I believe the intention is that this function returns 0 when
> it fails (to parse) and 1 on success. So I think that part is fine.
> 
> What seems a bit broken is the way that callers use the return value.
> 
> 1. The call in process_register_response() looks like this:
> 
> 	ret = ct_sip_parse_numerical_param(...)
> 	if (ret < 0) {
> 		nf_ct_helper_log(skb, ct, "cannot parse expires");
> 		return NF_DROP;
> 	}
> 
>      But ret can only be 0 or 1, so the error handling is never inoked,
>      and a failure to parse is ignored. I guess failure doesn't occur in
>      practice.
> 
>      I suspect this should be:
> 
> 	ret = ct_sip_parse_numerical_param(...)
> 	if (!ret) {
> 		nf_ct_helper_log(skb, ct, "cannot parse expires");
> 		return NF_DROP;
> 	}
> 

ct_sip_parse_numerical_param() returns 0 in to cases 1) when the 
parameter 'expires=' isn't found in the header or 2) it's incorrectly set.
In the first case, the return value should be ignored, since this is a 
normal situation
In the second case, it's better to write to the log and return NF_DROP, 
or ignore it too, then checking the return value can be removed as 
unnecessary.


> 2. The callprocess_register_request() looks like this:
> 
>          if (ct_sip_parse_numerical_param(...)) {
>                  nf_ct_helper_log(skb, ct, "cannot parse expires");
>                  return NF_DROP;
>          }
> 
>     But this seems to treat success as an error and vice versa.
> 
>          if (!ct_sip_parse_numerical_param(...)) {
>                  nf_ct_helper_log(skb, ct, "cannot parse expires");
>                  return NF_DROP;
>          }
> 
>    Or, better:
> 
>          ret = ct_sip_parse_numerical_param(...);
> 	if (!ret) {
> 		...
> 	}
> 

Here is the same as in process_register_response()

ret = ct_sip_parse_numerical_param(...);
if (ret < 0) {
    ...
    return NF_DROP;
}

Maybe it's better to remove the check altogether?



> 
> 3. The invocation in nf_nat_sip() looks like this:
> 
> 	if (ct_sip_parse_numerical_param(...) > 0 &&
> 	    ...)
> 	    ...
> 
>     This seems correct to me.

I agree, everything seems correct here

> 
>> ---
>>   net/netfilter/nf_conntrack_sip.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
>> index 77f5e82d8e3f..d0eac27f6ba0 100644
>> --- a/net/netfilter/nf_conntrack_sip.c
>> +++ b/net/netfilter/nf_conntrack_sip.c
>> @@ -611,7 +611,7 @@ int ct_sip_parse_numerical_param(const struct nf_conn *ct, const char *dptr,
>>   	start += strlen(name);
>>   	*val = simple_strtoul(start, &end, 0);
>>   	if (start == end)
>> -		return 0;
>> +		return -1;
>>   	if (matchoff && matchlen) {
>>   		*matchoff = start - dptr;
>>   		*matchlen = end - start;
>> -- 
>> 2.30.2
>>