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

Asterisk Bug Tracker noreply at bugs.digium.com
Tue Jan 26 10:31:31 CST 2010


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:                     closed
Target Version:             1.6.1.14
Asterisk Version:           SVN 
JIRA:                       SWP-451 
Regression:                 No 
Reviewboard Link:           https://reviewboard.asterisk.org/r/469/ 
SVN Branch (only for SVN checkouts, not tarball releases):  trunk 
SVN Revision (number only!):  
Request Review:              
Resolution:                 fixed
Fixed in Version:           
====================================================================== 
Date Submitted:             2009-11-21 15:03 CST
Last Modified:              2010-01-26 10:31 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.
====================================================================== 

---------------------------------------------------------------------- 
 (0117194) svnbot (reporter) - 2010-01-26 10:31
 https://issues.asterisk.org/view.php?id=16299#c117194 
---------------------------------------------------------------------- 
Repository: asterisk
Revision: 243201

_U  branches/1.6.2/

------------------------------------------------------------------------
r243201 | dvossel | 2010-01-26 10:31:30 -0600 (Tue, 26 Jan 2010) | 61
lines

Blocked revisions 243200 via svnmerge

........
  r243200 | dvossel | 2010-01-26 10:30:08 -0600 (Tue, 26 Jan 2010) | 56
lines
  
  RFC compliant uri and display-name encode/decode
  
  1.  URI Encoding
  This patch changes ast_uri_encode()'s behavior when doreserved is
enabled.
  Previously when doreserved was enabled only a small set of reserved
  characters were encoded.  This set was comprised primarily of the
reserved
  characters defined in RFC3261 section 25.1, but contained other
characters as
  well.  Rather than only escaping the reserved set, doreserved now
escapes
  all characters not within the unreserved set as defined by RFC 3261 and
  RFC 2396.  Also, the 'doreserved' variable has been renamed to
'do_special_char'
  in attempts to avoid confusion.
  
  When doreserve is not enabled, the previous logic of only encoding the
  characters <= 0X1F and > 0X7f remains, except for the '%' character,
which
  must always be encoded as it signifies a HEX escaped character during
the decode
  process.
  
  2. URI Decoding: Break up URI before decode.
  In chan_sip.c ast_uri_decode is called on the entire URI instead of it's
  individual parts after it is parsed.  This is not good as ast_uri_decode
  can introduce special characters back into the URI which can mess up
parsing.
  This patch resolves this by not decoding a URI until parsing is
completely
  done.  There are many instances where we check to see if pedantic
checking
  is enabled before we decode a URI.  In these cases a new macro,
  SIP_PEDANTIC_DECODE, is used on the individual parsed segments of the
URI
  rather than constantly putting if (pedantic) { decode() } checks
everywhere
  in the code.  In the areas where ast_uri_decode is not dependent upon
  pedantic checking this macro is not used, but decoding is still moved to
  each individual part of the URI.  The only behavior that should change
from
  this patch is the time at which decoding occurs.
  
  Since I had to look over every place URI parsing occurs to create this
  patch, I found several places where we use duplicate code for parsing.
  To consolidate the code, those areas have updated to use the parse_uri()
  function where possible.
  
  3. SIP display-name decoding according to RFC3261 section 25.
  To properly decode the display-name portion of a FROM header, chan_sip's
  get_calleridname() function required a complete re-write.  More
information
  about this change can be found in the comments at the beginning of this
function.
  
  4. Unit Tests.
  Unit tests for ast_uri_encode, ast_uri_decode, and get_calleridname()
have been
  written.  This involved the addition of the test_utils.c file for
testing the
  utils api.
  
  (closes issue https://issues.asterisk.org/view.php?id=16299)
  Reported by: wdoekes
  Patches:
        astsvn-16299-get_calleridname.diff uploaded by wdoekes (license
717)
        get_calleridname_rewrite.diff uploaded by dvossel (license 671)
  Tested by: wdoekes, dvossel, Nick_Lewis
  
  Review: https://reviewboard.asterisk.org/r/469/
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=243201 

Issue History 
Date Modified    Username       Field                    Change               
====================================================================== 
2010-01-26 10:31 svnbot         Checkin                                      
2010-01-26 10:31 svnbot         Note Added: 0117194                          
======================================================================




More information about the asterisk-bugs mailing list