| Message ID | 20161019082402.GA4575@gir |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
Hi Ulrich, Cc'ing Marco Angaroni. On Wed, Oct 19, 2016 at 10:24:02AM +0200, Ulrich Weber wrote: > on SIP requests, so a fragmented TCP SIP packet starting with > INVITE,NOTIFY,OPTIONS,REFER,REGISTER,UPDATE,SUBSCRIBE > Content-Length: 0 > > will not bet interpreted as an INVITE request. > > Confirm with RFC 3261 > Request-Line = Method SP Request-URI SP SIP-Version CRLF Marco has missed your email, so I'm copying and pasting what he told me wrt. to your patch: My understanding is that Ulrich is referring to something like an "Allow" header of a SIP message where the header name is part of a TCP segment and the header value is part of the following TCP segment: Allow: INVITE,ACK,CANCEL,OPTIONS,BYE So I think the fix (check for a space after the method name) is correct. However maybe the check could be made more robust, because if you have the fragmented "Allow" header containing the method names with a space before comma (it's permitted by SIP grammar) like you see below, you would again think that it's the first line of the SIP message: Allow: INVITE , ACK , CANCEL , OPTIONS , BYE Maybe it's worth checking two bytes after method name; the first byte must be a space and the second must a alphanumeric character: the URI following the method name has to start with "sip:" or "sips:" or some other scheme, which starts for sure with an ALPHA character. Request-Line = Method SP Request-URI SP SIP-Version CRLF Request-URI = SIP-URI / SIPS-URI / absoluteURI absoluteURI = scheme ":" ( hier-part / opaque-part ) scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." ) -- 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
Hi Pablo, good point, will send a new version! Thanks Ulrich On 20.10.2016 20:17, Pablo Neira Ayuso wrote: > Hi Ulrich, > > Cc'ing Marco Angaroni. > > On Wed, Oct 19, 2016 at 10:24:02AM +0200, Ulrich Weber wrote: >> on SIP requests, so a fragmented TCP SIP packet starting with >> INVITE,NOTIFY,OPTIONS,REFER,REGISTER,UPDATE,SUBSCRIBE >> Content-Length: 0 >> >> will not bet interpreted as an INVITE request. >> >> Confirm with RFC 3261 >> Request-Line = Method SP Request-URI SP SIP-Version CRLF > > Marco has missed your email, so I'm copying and pasting what he told > me wrt. to your patch: > > My understanding is that Ulrich is referring to something like an > "Allow" header of a SIP message where the header name is part of a TCP > segment and the header value is part of the following TCP segment: > > Allow: INVITE,ACK,CANCEL,OPTIONS,BYE > > So I think the fix (check for a space after the method name) is > correct. > However maybe the check could be made more robust, because if you have > the fragmented "Allow" header containing the method names with a space > before comma (it's permitted by SIP grammar) like you see below, you > would again think that it's the first line of the SIP message: > > Allow: INVITE , ACK , CANCEL , OPTIONS , BYE > > Maybe it's worth checking two bytes after method name; the first byte > must be a space and the second must a alphanumeric character: the URI > following the method name has to start with "sip:" or "sips:" or some > other scheme, which starts for sure with an ALPHA character. > > Request-Line = Method SP Request-URI SP SIP-Version CRLF > Request-URI = SIP-URI / SIPS-URI / absoluteURI > absoluteURI = scheme ":" ( hier-part / opaque-part ) > scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." ) > -- > 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 > -- 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 7700556..e37f8da 100644 --- a/net/netfilter/nf_conntrack_sip.c +++ b/net/netfilter/nf_conntrack_sip.c @@ -1436,9 +1436,11 @@ static int process_sip_request(struct sk_buff *skb, unsigned int protoff, handler = &sip_handlers[i]; if (handler->request == NULL) continue; - if (*datalen < handler->len || + if (*datalen < handler->len + 1 || strncasecmp(*dptr, handler->method, handler->len)) continue; + if ((*dptr)[handler->len] != ' ') + continue; if (ct_sip_get_header(ct, *dptr, 0, *datalen, SIP_HDR_CSEQ, &matchoff, &matchlen) <= 0) {
on SIP requests, so a fragmented TCP SIP packet starting with INVITE,NOTIFY,OPTIONS,REFER,REGISTER,UPDATE,SUBSCRIBE Content-Length: 0 will not bet interpreted as an INVITE request. Confirm with RFC 3261 Request-Line = Method SP Request-URI SP SIP-Version CRLF Signed-off-by: Ulrich Weber <ulrich.weber@riverbed.com> --- net/netfilter/nf_conntrack_sip.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)