* [PATCH] netfilter: nf_conntrack_sip: CSeq 0 is a valid CSeq
@ 2016-07-01 9:48 Christophe Leroy
2016-07-04 5:48 ` Liping Zhang
0 siblings, 1 reply; 5+ messages in thread
From: Christophe Leroy @ 2016-07-01 9:48 UTC (permalink / raw)
To: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik, David S. Miller
Cc: netfilter-devel, coreteam, netdev, linux-kernel
Do not drop packet when CSeq is 0 as 0 is also a valid value for CSeq.
In order to do so, we replace obsolete simple_strtoul() which
returns 0 on error by kstrtouint(). As kstrtouint() requires a
NULL terminated string, we need to use a temporary buffer
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
net/netfilter/nf_conntrack_sip.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index f72ba55..e770477 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1368,6 +1368,7 @@ static int process_sip_response(struct sk_buff *skb, unsigned int protoff,
struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
unsigned int matchoff, matchlen, matchend;
unsigned int code, cseq, i;
+ char buf[21];
if (*datalen < strlen("SIP/2.0 200"))
return NF_ACCEPT;
@@ -1382,8 +1383,13 @@ static int process_sip_response(struct sk_buff *skb, unsigned int protoff,
nf_ct_helper_log(skb, ct, "cannot parse cseq");
return NF_DROP;
}
- cseq = simple_strtoul(*dptr + matchoff, NULL, 10);
- if (!cseq) {
+ if (matchlen > sizeof(buf) - 1) {
+ nf_ct_helper_log(skb, ct, "cannot parse cseq (too big)");
+ return NF_DROP;
+ }
+ memcpy(buf, *dptr + matchoff, matchlen);
+ buf[matchlen] = 0;
+ if (kstrtouint(buf, 10, &cseq)) {
nf_ct_helper_log(skb, ct, "cannot get cseq");
return NF_DROP;
}
@@ -1432,6 +1438,7 @@ static int process_sip_request(struct sk_buff *skb, unsigned int protoff,
for (i = 0; i < ARRAY_SIZE(sip_handlers); i++) {
const struct sip_handler *handler;
+ char buf[21];
handler = &sip_handlers[i];
if (handler->request == NULL)
@@ -1445,8 +1452,13 @@ static int process_sip_request(struct sk_buff *skb, unsigned int protoff,
nf_ct_helper_log(skb, ct, "cannot parse cseq");
return NF_DROP;
}
- cseq = simple_strtoul(*dptr + matchoff, NULL, 10);
- if (!cseq) {
+ if (matchlen > sizeof(buf) - 1) {
+ nf_ct_helper_log(skb, ct, "cannot parse cseq(too big)");
+ return NF_DROP;
+ }
+ memcpy(buf, *dptr + matchoff, matchlen);
+ buf[matchlen] = 0;
+ if (kstrtouint(buf, 10, &cseq)) {
nf_ct_helper_log(skb, ct, "cannot get cseq");
return NF_DROP;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] netfilter: nf_conntrack_sip: CSeq 0 is a valid CSeq
2016-07-01 9:48 [PATCH] netfilter: nf_conntrack_sip: CSeq 0 is a valid CSeq Christophe Leroy
@ 2016-07-04 5:48 ` Liping Zhang
2016-07-04 6:14 ` Christophe Leroy
0 siblings, 1 reply; 5+ messages in thread
From: Liping Zhang @ 2016-07-04 5:48 UTC (permalink / raw)
To: Christophe Leroy
Cc: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, netfilter-devel, coreteam, netdev, linux-kernel
2016-07-01 17:48 GMT+08:00 Christophe Leroy <christophe.leroy@c-s.fr>:
> Do not drop packet when CSeq is 0 as 0 is also a valid value for CSeq.
>
> --- a/net/netfilter/nf_conntrack_sip.c
> +++ b/net/netfilter/nf_conntrack_sip.c
> @@ -1368,6 +1368,7 @@ static int process_sip_response(struct sk_buff *skb, unsigned int protoff,
> struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
> unsigned int matchoff, matchlen, matchend;
> unsigned int code, cseq, i;
> + char buf[21];
>
> if (*datalen < strlen("SIP/2.0 200"))
> return NF_ACCEPT;
> @@ -1382,8 +1383,13 @@ static int process_sip_response(struct sk_buff *skb, unsigned int protoff,
> nf_ct_helper_log(skb, ct, "cannot parse cseq");
> return NF_DROP;
> }
> - cseq = simple_strtoul(*dptr + matchoff, NULL, 10);
> - if (!cseq) {
I think there is no need to convert simple_strtoul to kstrtouint, add
a further check seems better?
Like this:
- if (!cseq) {
+ if (!cseq && *(*dptr + matchoff) != '0') {
> + if (matchlen > sizeof(buf) - 1) {
> + nf_ct_helper_log(skb, ct, "cannot parse cseq (too big)");
> + return NF_DROP;
> + }
> + memcpy(buf, *dptr + matchoff, matchlen);
> + buf[matchlen] = 0;
> + if (kstrtouint(buf, 10, &cseq)) {
> nf_ct_helper_log(skb, ct, "cannot get cseq");
> return NF_DROP;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] netfilter: nf_conntrack_sip: CSeq 0 is a valid CSeq
2016-07-04 5:48 ` Liping Zhang
@ 2016-07-04 6:14 ` Christophe Leroy
2016-07-04 11:43 ` Liping Zhang
0 siblings, 1 reply; 5+ messages in thread
From: Christophe Leroy @ 2016-07-04 6:14 UTC (permalink / raw)
To: Liping Zhang
Cc: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, netfilter-devel, coreteam, netdev, linux-kernel
Le 04/07/2016 à 07:48, Liping Zhang a écrit :
> 2016-07-01 17:48 GMT+08:00 Christophe Leroy <christophe.leroy@c-s.fr>:
>> Do not drop packet when CSeq is 0 as 0 is also a valid value for CSeq.
>>
>> --- a/net/netfilter/nf_conntrack_sip.c
>> +++ b/net/netfilter/nf_conntrack_sip.c
>> @@ -1368,6 +1368,7 @@ static int process_sip_response(struct sk_buff *skb, unsigned int protoff,
>> struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
>> unsigned int matchoff, matchlen, matchend;
>> unsigned int code, cseq, i;
>> + char buf[21];
>>
>> if (*datalen < strlen("SIP/2.0 200"))
>> return NF_ACCEPT;
>> @@ -1382,8 +1383,13 @@ static int process_sip_response(struct sk_buff *skb, unsigned int protoff,
>> nf_ct_helper_log(skb, ct, "cannot parse cseq");
>> return NF_DROP;
>> }
>> - cseq = simple_strtoul(*dptr + matchoff, NULL, 10);
>> - if (!cseq) {
>
> I think there is no need to convert simple_strtoul to kstrtouint, add
> a further check seems better?
> Like this:
> - if (!cseq) {
> + if (!cseq && *(*dptr + matchoff) != '0') {
>
And what about an invalid CSeq that would look like CSeq: 0abzk852 ?
Should we check it is 0 + space instead ?
>> + if (matchlen > sizeof(buf) - 1) {
>> + nf_ct_helper_log(skb, ct, "cannot parse cseq (too big)");
>> + return NF_DROP;
>> + }
>> + memcpy(buf, *dptr + matchoff, matchlen);
>> + buf[matchlen] = 0;
>> + if (kstrtouint(buf, 10, &cseq)) {
>> nf_ct_helper_log(skb, ct, "cannot get cseq");
>> return NF_DROP;
>> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] netfilter: nf_conntrack_sip: CSeq 0 is a valid CSeq
2016-07-04 6:14 ` Christophe Leroy
@ 2016-07-04 11:43 ` Liping Zhang
0 siblings, 0 replies; 5+ messages in thread
From: Liping Zhang @ 2016-07-04 11:43 UTC (permalink / raw)
To: Christophe Leroy
Cc: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, netfilter-devel, coreteam, netdev, linux-kernel
2016-07-04 14:14 GMT+08:00 Christophe Leroy <christophe.leroy@c-s.fr>:
>> I think there is no need to convert simple_strtoul to kstrtouint, add
>> a further check seems better?
>> Like this:
>> - if (!cseq) {
>> + if (!cseq && *(*dptr + matchoff) != '0') {
>>
>
> And what about an invalid CSeq that would look like CSeq: 0abzk852 ?
> Should we check it is 0 + space instead ?
In this case, i.e. some stupid sip clients set CSeq to "0abzk852",
your patch will also fail to detect this "error".
Because for "Cseq", int (*match_len)(...) point to digits_len(see
struct sip_header ct_sip_hdrs definition).
So in this case match_len will just be setted to ONE (not
sizeof("0abzk852")-1), then cseq will be parsed
as 0 by kstrtouint, not as an error.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] netfilter: nf_conntrack_sip: CSeq 0 is a valid CSeq
@ 2016-07-01 9:45 chleroy
0 siblings, 0 replies; 5+ messages in thread
From: chleroy @ 2016-07-01 9:45 UTC (permalink / raw)
To: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik, David S. Miller
Cc: netfilter-devel, coreteam, netdev, linux-kernel
Do not drop packet when CSeq is 0 as 0 is also a valid value for CSeq.
In order to do so, we replace obsolete simple_strtoul() which
returns 0 on error by kstrtouint(). As kstrtouint() requires a
NULL terminated string, we need to use a temporary buffer
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
net/netfilter/nf_conntrack_sip.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index f72ba55..e770477 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1368,6 +1368,7 @@ static int process_sip_response(struct sk_buff *skb, unsigned int protoff,
struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
unsigned int matchoff, matchlen, matchend;
unsigned int code, cseq, i;
+ char buf[21];
if (*datalen < strlen("SIP/2.0 200"))
return NF_ACCEPT;
@@ -1382,8 +1383,13 @@ static int process_sip_response(struct sk_buff *skb, unsigned int protoff,
nf_ct_helper_log(skb, ct, "cannot parse cseq");
return NF_DROP;
}
- cseq = simple_strtoul(*dptr + matchoff, NULL, 10);
- if (!cseq) {
+ if (matchlen > sizeof(buf) - 1) {
+ nf_ct_helper_log(skb, ct, "cannot parse cseq (too big)");
+ return NF_DROP;
+ }
+ memcpy(buf, *dptr + matchoff, matchlen);
+ buf[matchlen] = 0;
+ if (kstrtouint(buf, 10, &cseq)) {
nf_ct_helper_log(skb, ct, "cannot get cseq");
return NF_DROP;
}
@@ -1432,6 +1438,7 @@ static int process_sip_request(struct sk_buff *skb, unsigned int protoff,
for (i = 0; i < ARRAY_SIZE(sip_handlers); i++) {
const struct sip_handler *handler;
+ char buf[21];
handler = &sip_handlers[i];
if (handler->request == NULL)
@@ -1445,8 +1452,13 @@ static int process_sip_request(struct sk_buff *skb, unsigned int protoff,
nf_ct_helper_log(skb, ct, "cannot parse cseq");
return NF_DROP;
}
- cseq = simple_strtoul(*dptr + matchoff, NULL, 10);
- if (!cseq) {
+ if (matchlen > sizeof(buf) - 1) {
+ nf_ct_helper_log(skb, ct, "cannot parse cseq(too big)");
+ return NF_DROP;
+ }
+ memcpy(buf, *dptr + matchoff, matchlen);
+ buf[matchlen] = 0;
+ if (kstrtouint(buf, 10, &cseq)) {
nf_ct_helper_log(skb, ct, "cannot get cseq");
return NF_DROP;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-07-04 11:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-01 9:48 [PATCH] netfilter: nf_conntrack_sip: CSeq 0 is a valid CSeq Christophe Leroy
2016-07-04 5:48 ` Liping Zhang
2016-07-04 6:14 ` Christophe Leroy
2016-07-04 11:43 ` Liping Zhang
-- strict thread matches above, loose matches on Subject: below --
2016-07-01 9:45 chleroy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.