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

[Simon Perreault]

> 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.
> 
> Terry Wilson wrote:
>     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?

"The better is the enemy of the good." (Voltaire)

Ship it.


- Simon


-----------------------------------------------------------
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/20101119/1a32404a/attachment-0001.htm