[PATCH] netfilter: nf_conntrack_sip: fix parsing error

[PATCH] netfilter: nf_conntrack_sip: fix parsing error

[Tong Zhang]

ct_sip_parse_numerical_param can only return 0 or 1, but the caller is
checking parsing error using < 0

Signed-off-by: Tong Zhang <ztong0001@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 b83dc9bf0a5d..08873694120a 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1269,7 +1269,7 @@ static int process_register_request(struct sk_buff *skb, unsigned int protoff,
 
 	if (ct_sip_parse_numerical_param(ct, *dptr,
 					 matchoff + matchlen, *datalen,
-					 "expires=", NULL, NULL, &expires) < 0) {
+					 "expires=", NULL, NULL, &expires) == 0) {
 		nf_ct_helper_log(skb, ct, "cannot parse expires");
 		return NF_DROP;
 	}
@@ -1375,7 +1375,7 @@ static int process_register_response(struct sk_buff *skb, unsigned int protoff,
 						   matchoff + matchlen,
 						   *datalen, "expires=",
 						   NULL, NULL, &c_expires);
-		if (ret < 0) {
+		if (ret == 0) {
 			nf_ct_helper_log(skb, ct, "cannot parse expires");
 			return NF_DROP;
 		}
-- 
2.25.1

Re: [PATCH] netfilter: nf_conntrack_sip: fix parsing error

[Florian Westphal]

Tong Zhang <ztong0001@gmail.com> wrote:
> ct_sip_parse_numerical_param can only return 0 or 1, but the caller is
> checking parsing error using < 0

Reviewed-by: Florian Westphal <fw@strlen.de>

Re: [PATCH] netfilter: nf_conntrack_sip: fix parsing error

[Pablo Neira Ayuso]

On Sat, Aug 15, 2020 at 12:50:30PM -0400, Tong Zhang wrote:
> ct_sip_parse_numerical_param can only return 0 or 1, but the caller is
> checking parsing error using < 0

Is this are real issue in your setup or probably some static analysis
tool is reporting?

You are right that ct_sip_parse_numerical_param() never returns < 0,
however, looking at:

https://tools.ietf.org/html/rfc3261 see Page 161

expires is optional, my understanding is that your patch is making
this option mandatory.

Re: [PATCH] netfilter: nf_conntrack_sip: fix parsing error

[Tong Zhang]

Hi Pablo,
I'm not an expert in this networking stuff.
But from my point of view there's no point in checking if this
condition is always true.
There's also no need of returning anything from the
ct_sip_parse_numerical_param()
if they are all being ignored like this.

On Fri, Aug 28, 2020 at 2:07 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Is this are real issue in your setup or probably some static analysis
> tool is reporting?
>
> You are right that ct_sip_parse_numerical_param() never returns < 0,
> however, looking at:
>
> https://tools.ietf.org/html/rfc3261 see Page 161
>
> expires is optional, my understanding is that your patch is making
> this option mandatory.

Re: [PATCH] netfilter: nf_conntrack_sip: fix parsing error

[Pablo Neira Ayuso]

On Fri, Aug 28, 2020 at 02:14:48PM -0400, Tong Zhang wrote:
> Hi Pablo,
> I'm not an expert in this networking stuff.
> But from my point of view there's no point in checking if this
> condition is always true.

Understood.

> There's also no need of returning anything from the
> ct_sip_parse_numerical_param()
> if they are all being ignored like this.

Then probably update this code to ignore the return value?

Re: [PATCH] netfilter: nf_conntrack_sip: fix parsing error

[Tong Zhang]

I think the original code complaining parsing error is there for a reason,
A better way is to modify ct_sip_parse_numerical_param() and let it return
a real parsing error code instead of returning FOUND(1) and NOT FOUND(0)
if deemed necessary
Once again I'm not an expert and I'm may suggest something stupid,
please pardon my ignorance --
- Tong

On Fri, Aug 28, 2020 at 2:19 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Then probably update this code to ignore the return value?