[asterisk-bugs] [Asterisk 0016299]: [patch] pedantic sip checking needed to generate valid messages (but broken)

[Asterisk Bug Tracker]

A NOTE has been added to this issue. 
====================================================================== 
https://issues.asterisk.org/view.php?id=16299 
====================================================================== 
Reported By:                wdoekes
Assigned To:                dvossel
====================================================================== 
Project:                    Asterisk
Issue ID:                   16299
Category:                   Channels/chan_sip/General
Reproducibility:            always
Severity:                   trivial
Priority:                   normal
Status:                     ready for review
Target Version:             1.6.1.13
Asterisk Version:           SVN 
JIRA:                       SWP-451 
Regression:                 No 
Reviewboard Link:            
SVN Branch (only for SVN checkouts, not tarball releases):  trunk 
SVN Revision (number only!):  
Request Review:              
====================================================================== 
Date Submitted:             2009-11-21 15:03 CST
Last Modified:              2010-01-06 07:44 CST
====================================================================== 
Summary:                    [patch] pedantic sip checking needed to generate
valid messages (but broken)
Description: 
In function 'initreqprep' in channels/chan_sip.c, the following code can be
found:

if (sip_cfg.pedanticsipchecking) {
  ast_uri_encode(n, tmp_n, sizeof(tmp_n), 0);
  n = tmp_n;
  ast_uri_encode(l, tmp_l, sizeof(tmp_l), 0);
  l = tmp_l;
}
<...snip...>
snprintf(from, sizeof(from), "\"%s\" <sip:%s@%s>;tag=%s", n, l, d,
p->tag);


The function ast_uri_encode encodes chars < 32 and > 127 -- perhaps one
should replace that with ((signed char)*ptr < 32) ;-) -- as %HH hex
escapes.

A couple of problems (all minor):
- ast_uri_encode forgets to escape % and 0x7F (RFC2396 2.4.2 and 2.4.3)
- ast_uri_encode does not escape <, >, @ and some other characters that
'l' would've liked to be escaped
- 'n' is not supposed to be hex-escaped (RFC4475 3.1.1.5 writes """The
display name portion of the To and From header fields is "%Z%45". Note that
this is not the same as %ZE.""")
- 'n' does however like the double-quote to be escaped, by a backslash
- ast_uri_decode is called on entire messages, not on already broken up
parts


Browsing through chan_sip.c, I see pedanticsipchecking used in these
cases:
- allow blanks between the header key and the colon
- allow multiline sip headers
- compare the from-tag/to-tag/branches as well instead of only the
call-id
- check that a packet really is for us (handle_incoming)
- encode/decode reserved characters

In my humble opinion, I don't think creating valid output (correctly
encoding illegal characters) should be enabled only by a flag that is
reported as being 'slow'. And, not as relevant to me in this case, but
decoding valid hex-escapes from peers does not sound like too much to ask,
either.


What to do?
- I can easily write a patch that fixes my minor issue: always -- not
dependent on the pedanticsipchecking -- run a s/"/\\"/g (instead of
ast_uri_encode) on the name part in the From.
- I can also easily fix ast_uri_encode to escape %, 0x7f and the others as
mentioned in RFC2396 2.4.3.
- Fixing all ast_uri_decode to operate first after the data has been
broken up is a bit more tedious, so I can't promise I'll do that.


Regards,
Walter Doekes
OSSO B.V.
====================================================================== 

---------------------------------------------------------------------- 
 (0116111) wdoekes (reporter) - 2010-01-06 07:44
 https://issues.asterisk.org/view.php?id=16299#c116111 
---------------------------------------------------------------------- 
dvossel:

A few patches:
(1) astsvn-16299-testh-compile.diff to make it compile without the test
framework
(2) astsvn-16299-parse_uri-calls.diff fixing typo's
(3) astsvn-16299-ast_str_replace.diff a utility function (see point 5)
(4) astsvn-16299-chansip-encode-from-fully.diff set do_special_char to 1
always (yes.. Nicks version is better but more time consuming to
implement), not only in RPID/PAI. (forget this patch, see point 5's patch
instead)
(5) astsvn-16299-displayname-no-uri-encode.diff do what (4) does and don't
encode/decode the display name using uri-encoding, but by replacing " with
''.

One could continue to debate about (4). I'm not qualified to decide
whether you want this as it could break (broken) communication between
asterisken.

As for (5), as can be seen from my comments at the ast_displayname_encode
macro, I initially thought I'd do it right (s/"/\"/g), but reading the RFC
did not reveal to me whether one would need a s/\/\\/g as well. And to be
kind to parsers that match the double-quotes only (not counting
backslashes), I figured it was easiest to remove (replace with '') the
double-quote altogether. 

Issue History 
Date Modified    Username       Field                    Change               
====================================================================== 
2010-01-06 07:44 wdoekes        Note Added: 0116111                          
======================================================================