[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:30:13 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: feedback
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:
======================================================================
Date Submitted: 2009-11-21 15:03 CST
Last Modified: 2010-01-26 10:30 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.
======================================================================
----------------------------------------------------------------------
(0117193) svnbot (reporter) - 2010-01-26 10:30
https://issues.asterisk.org/view.php?id=16299#c117193
----------------------------------------------------------------------
Repository: asterisk
Revision: 243200
U trunk/channels/chan_sip.c
U trunk/include/asterisk/utils.h
U trunk/main/test.c
U trunk/main/utils.c
A trunk/tests/test_utils.c
------------------------------------------------------------------------
r243200 | dvossel | 2010-01-26 10:30:09 -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=243200
Issue History
Date Modified Username Field Change
======================================================================
2010-01-26 10:30 svnbot Note Added: 0117193
======================================================================
More information about the asterisk-bugs
mailing list