| Message ID | 20160701094833.B281E1A23A2@localhost.localdomain |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
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; > } -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; >> } -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)