[asterisk-dev] [Code Review] Ensure SIP responses only have one Via header

[Terry Wilson]

> On 2010-11-18 14:57:19, Mark Michelson wrote:
> > I think there's an edge case here that's not accounted for. When there are multiple SIP headers of a specific type, they can either be specified like the following:
> > Via: <blah1>
> > Via: <blah2>
> > Via: <blah3>
> > or they can be specified like the following:
> > Via: <blah1>,<blah2>,<blah3>
> > In the second case, I don't *think* that parse_request() will actually treat these as separate Via headers. If you are trying to fail a response due to its having multiple Via headers, then just using __get_header() won't do the trick.
> > 
> > The real fix for this would be to have parse_request recognize the second case as having multiple Via headers, thus allowing you to use __get_header() in order to iterate over each one. A temporary fix would be to use both __get_header() as well as a check to be sure that the Via line you initially found does not have another value on it as well.
> > 
> > Now, given the nature of how Vias are added to requests and removed from responses, it seems like a case where you will never actually see this occur. It's probably safe to just add what you have, especially since it wouldn't be adding any new breakage to the code.

The problem with doing it in parse_request is that there are lots of headers that can be combined with a comma, and lots that can't. So you'd end up having to do string comparisons while parsing, then modify the behavior accordingly. It is a serious pain to actually handle this correctly. I'm a fan of SIP, but why they added things like this that have very little value is beyond me. Yes, compacting with commas can save a few bytes, but if that is the goal, why the hell would you use such a wordy protocol in the first place?

I hoped that I could just check for a comma, since the ABNF seemed to show that I couldn't have one except as the separator...until we get down to via-extension -> generic-param -> token [ EQUAL gen-value] -> gen-value -> token / host /quoted-string -> quoted-string which screws you. So, you instead need to check for "some or no whitespace followed by a comma followed by some or no whitespace followed by 'SIP' (or some other token) followed by a slash followed by another token (protocol version) followed by a slash followed by by a transport (TCP, UDP, SCTP, or just a token).

So, yeah, parse_request is definitely where such a fix should be. It is just a lot of work as it requires making a list of every header that either does or doesn't support the whole join-with-commas thing, then figuring out the "safe way" to actually split on the comma for *each* header. Parsing gets a lot more expensive, but would be correct. And again, lots of work. So it is really just completely writing a parser from scratch to make it "correct".

Of course, check_via() just does a strchr(via, ',') when it does the check, so if there is some whacked via-extension like ;crazy="you,betcha" things will break.

So, I suppose the moral of the story is "we don't have a real SIP message parser." I'm not sure how far I should go in fixing that problem. Anybody have any opinions on the matter?


- Terry


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1019/#review2937
-----------------------------------------------------------


On 2010-11-18 14:34:42, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1019/
> -----------------------------------------------------------
> 
> (Updated 2010-11-18 14:34:42)
> 
> 
> Review request for Asterisk Developers and David Vossel.
> 
> 
> Summary
> -------
> 
> 8.1.3.3 Vias
> 
>    If more than one Via header field value is present in a response, the
>    UAC SHOULD discard the message.
> 
>       The presence of additional Via header field values that precede
>       the originator of the request suggests that the message was
>       misrouted or possibly corrupted.
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_sip.c 295442 
> 
> Diff: https://reviewboard.asterisk.org/r/1019/diff
> 
> 
> Testing
> -------
> 
> Used sipp to send a request with an extra via header; it was ignored. Valid responses still passed normally.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.digium.com/pipermail/asterisk-dev/attachments/20101118/0c3eb23f/attachment.htm