nf_conntrack_sip: check for trailing spaces

PATCH In review Ulrich Weber 3 participants 3 messages Oct 19, 2016 – Oct 24, 2016

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Ulrich Weber

author

2016-10-19 15:50 UTC #0 lore raw

PATCH 1/1 nf_conntrack_sip: check for trailing spaces

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 <[email protected]>

---
 net/netfilter/nf_conntrack_sip.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

net/netfilter/nf_conntrack_sip.c +3 -1

index 7700556..e37f8da 100644
@@ -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) {

Signature · 1 line

2.7.4

Pablo Neira Ayuso

maintainer

2016-10-20 18:17 UTC #1 lore raw

Hi Ulrich,

Cc'ing Marco Angaroni.

Quoted 8 lines · Ulrich Weber

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 / "+" / "-" / "." )

Ulrich Weber

2016-10-24 16:05 UTC #2 lore raw

Hi Pablo,

good point, will send a new version!

Thanks
Ulrich

Quoted 46 lines · Pablo Neira Ayuso

Hi Ulrich,
Cc'ing Marco Angaroni.
Quoted 8 lines · Ulrich Weber
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html