[Home]

Summary:ASTERISK-16597: [patch] contact header does not get ast_uri_encoded value from p->exten, but dialplan does
Reporter:Walter Doekes (wdoekes)Labels:
Date Opened:2010-08-20 04:49:40Date Closed:2010-10-01 11:23:17
Priority:TrivialRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) bug17892-1.patch
( 1) issue17892_too_much_decoding.diff
Description:Hi, I have two asterisken, asterisk1 and asterisk2. (They run 1.6.2.11 and 1.6.2.8 respectively, but I'm fairly sure no sip encoding issues have been fixed between those versions.)

With pedantic mode off, I get (url)decoded INVITE values, but they're not re-encoded in the contact header. (I'm not sure about pedantic mode on, but I'm guessing that will yield wrong results too.)

I was expecting to get no encoding/decoding at all with pedantic mode off, but it seems I'm getting some:

(1) invite asterisk1 to asterisk2, triggered by:
Dial(126680004%40126680004@asterisk2)

INVITE sip:126680004%40126680004@asterisk2 SIP/2.0
Via: SIP/2.0/UDP x.x.x.1:5060;branch=z9hG4bK4a2faaf6
From: "209" <sip:209@x.x.x.1>;tag=as39764467
To: <sip:126680004%40126680004@asterisk2>
Contact: <sip:209@x.x.x.1>

(2) response from asterisk2 to asterisk1, found by:
exten => _[0-9].@[0-9].,1,NoOp(got exten ${EXTEN})

SIP/2.0 100 Trying
Via: SIP/2.0/UDP x.x.x.1:5060;branch=z9hG4bK4a2faaf6;received=x.x.x.1
From: "209" <sip:209@x.x.x.1>;tag=as39764467
To: <sip:126680004%40126680004@asterisk2>
Contact: <sip:126680004@126680004@x.x.x.2>

In asterisk2, with pedantic mode off, I expect 126680004%40126680004 to be found, but it matches 126680004@126680004. This could be a good thing, because that's what I really wanted.

But, as you can see, the Contact header is wrong. This is to be expected, because only with pedantic mode the encoding functions (should) get triggered.

However, when checking static void build_contact(struct sip_pvt *p) {}, I see no logic to ast_uri_encode p->exten if sip_cfg.pedanticsipchecking.


The non-encoded Contact poses a problem when trying to create the ACK on asterisk1 as response to the 200 from asterisk2:

Capabilities: us - 0x8 (alaw), peer - audio=0x8 (alaw)/video=0x0 (nothing)/text=0x0 (nothing), combined - 0x8 (alaw)
Non-codec capabilities (dtmf): us - 0x1 (telephone-event), peer - 0x1 (telephone-event), combined - 0x1 (telephone-event)
Peer audio RTP is at port x.x.x.2:10060
list_route: hop: <sip:126680004@126680004@x.x.x.2>
[2010-08-20 11:08:01] WARNING[1580]: chan_sip.c:12259 __set_address_from_contact: Invalid host name in Contact: (can't resolve in DNS) : '126680004@x.x.x.2'
set_destination: Parsing <sip:126680004@126680004@x.x.x.2> for address/port to send to
[2010-08-20 11:08:01] WARNING[1580]: chan_sip.c:9323 set_destination: Can't find address for host '126680004@x.x.x.2'

(In my case, this isn't a problem, as the Contact destination is the same as the original destination, causing the set_destination to be a harmless no-op.)

****** STEPS TO REPRODUCE ******

asterisk1:

Dial(126680004%40126680004@asterisk2)

asterisk2:

exten => _[0-9].@[0-9].,1,NoOp(got exten ${EXTEN})
exten => _[0-9].@[0-9].,n,Answer()

****** ADDITIONAL INFORMATION ******

I've already mentioned in a ticket once that I don't like it that you need pedantic mode to get proper encoding/decoding. But that's a different matter.

Asterisk should at least be consistent in what it does. I think that means that:
(1) The original uri shouldn't have been decoded with pedantic mode off.
(2) If pedantic mode is on, p->exten should be encoded in build_contact.


Regards,
Walter Doekes
OSSO B.V.


B.t.w. this inactive ticket is a less detailed report of the same problem:
https://issues.asterisk.org/view.php?id=16329
Comments:By: Walter Doekes (wdoekes) 2010-09-21 14:48:21

issue17892_too_much_decoding.diff points to places where things are wrong:

Hunk #2 undoes the Contact being decoded. With that patched (pedantic mode still off), the Contact header is unscathed, but the decoded extension is still matched.

Hunk #1 fixes that the original extension is matched (123%40456 matches exten 123%40456)

I'm unsure about the decode at #3. And #4 is merely cosmetic.


Unpatched and without pedantic mode, asterisk-1.6.2.13 matches exten 123@456 when calling 123%2540456 (that's enc(enc("123@456")))).
Unpatched and with pedantic mode, it's even worse, because the uri is already decoded (line 13443) and then the decoding continues. (enc(enc(enc("123@456"))))


A bit of FYI. Hope it helps.
By: Jeff Peeler (jpeeler) 2010-09-23 16:00:52

This modification takes into consideration the recent commit here:
http://svnview.digium.com/svn/asterisk?view=rev&rev=288113

The dialplan is actually now consulted to determine encoding actions. I believe it makes sense to have the encoded version of the extension in the dialplan since that is what is being sent (which works with this patch). Now, pedantic mode turned on does not work, but why would you expect things to work using prohibited characters in that case?
By: Walter Doekes (wdoekes) 2010-09-26 15:06:48

I'm sorry, I don't quite follow what you mean.

The which-boolean in the changeset by tilghman looks like it could fix the Contact being decoded. I'll go test that patch and see what it does.

I'd say it makes sense to use the encoded version ("123%40456") in the dialplan, IFF pedantic mode is off. If it's on and you use the encoded version, you would be forcing all other protocols to use SIP encoding rules.

And I can't place your last statement about prohibited characters. There is nothing wrong with "sip:123%40456@mydomain", right?
By: Jeff Peeler (jpeeler) 2010-09-30 12:31:00

What I mean is you can't put this in the dialplan (with pedantic turned on):
exten => 5003@12,1,dial(sip/5003,,tTkK)
This is on the remote side.

I can't see why you would do the above with pedantic mode on. Can you test my patch and see if it works as you're expecting?

There just isn't a good way to handle a decoded extension with an illegal character. I think allowing an encoded version in the dialplan should be acceptable, which works with Tilghman's change and my patch.
By: Walter Doekes (wdoekes) 2010-09-30 13:48:27

Okay. Applied both patches and it's looking good. Pedantic mode off => does what I expect. Pedantic mode on => also does what I expect.

Excellent. Thanks :)

(As far as the illegality of the @ goes: RFC3261 mentions the user j%40son being perfectly valid, so I don't know why you would call it "illegal". But this is no issue to me. I'll be running with pedantic mode off anyway.)
By: Jeff Peeler (jpeeler) 2010-09-30 14:49:33

Then I'll go ahead and commit it. I'm referring simply to the extension being illegal in the decoded format with pedantic on in the dialplan, as in j@son.
By: Digium Subversion (svnbot) 2010-10-01 11:20:03

Repository: asterisk
Revision: 289699

U   branches/1.4/channels/chan_sip.c

------------------------------------------------------------------------
r289699 | jpeeler | 2010-10-01 11:20:02 -0500 (Fri, 01 Oct 2010) | 14 lines

Ensure user portion of SIP URI matches dialplan when using encoded characters.

This commit takes a simliar approach to 288112 and checks the dialplan to
determine the proper action for an incoming contact header as to whether or not
it should be decoded or not. sip_new was blindly always decoding the extension,
which also caused the outgoing contact header to be incorrect as well as failing
to match the encoded extension in the dialplan.

(closes issue ASTERISK-16597)
Reported by: wdoekes
Patches:
     bug17892-1.patch uploaded by jpeeler (license 325)
Tested by: wdoekes

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

http://svn.digium.com/view/asterisk?view=rev&revision=289699
By: Digium Subversion (svnbot) 2010-10-01 11:21:05

Repository: asterisk
Revision: 289700

_U  branches/1.6.2/
U   branches/1.6.2/channels/chan_sip.c

------------------------------------------------------------------------
r289700 | jpeeler | 2010-10-01 11:21:04 -0500 (Fri, 01 Oct 2010) | 21 lines

Merged revisions 289699 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r289699 | jpeeler | 2010-10-01 11:20:00 -0500 (Fri, 01 Oct 2010) | 14 lines
 
 Ensure user portion of SIP URI matches dialplan when using encoded characters.
 
 This commit takes a simliar approach to 288112 and checks the dialplan to
 determine the proper action for an incoming contact header as to whether or not
 it should be decoded or not. sip_new was blindly always decoding the extension,
 which also caused the outgoing contact header to be incorrect as well as failing
 to match the encoded extension in the dialplan.
 
 (closes issue ASTERISK-16597)
 Reported by: wdoekes
 Patches:
       bug17892-1.patch uploaded by jpeeler (license 325)
 Tested by: wdoekes
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=289700
By: Digium Subversion (svnbot) 2010-10-01 11:22:20

Repository: asterisk
Revision: 289701

_U  branches/1.8/
U   branches/1.8/channels/chan_sip.c

------------------------------------------------------------------------
r289701 | jpeeler | 2010-10-01 11:22:20 -0500 (Fri, 01 Oct 2010) | 28 lines

Merged revisions 289700 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.6.2

................
 r289700 | jpeeler | 2010-10-01 11:21:04 -0500 (Fri, 01 Oct 2010) | 21 lines
 
 Merged revisions 289699 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r289699 | jpeeler | 2010-10-01 11:20:00 -0500 (Fri, 01 Oct 2010) | 14 lines
   
   Ensure user portion of SIP URI matches dialplan when using encoded characters.
   
   This commit takes a simliar approach to 288112 and checks the dialplan to
   determine the proper action for an incoming contact header as to whether or not
   it should be decoded or not. sip_new was blindly always decoding the extension,
   which also caused the outgoing contact header to be incorrect as well as failing
   to match the encoded extension in the dialplan.
   
   (closes issue ASTERISK-16597)
   Reported by: wdoekes
   Patches:
         bug17892-1.patch uploaded by jpeeler (license 325)
   Tested by: wdoekes
 ........
................

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

http://svn.digium.com/view/asterisk?view=rev&revision=289701
By: Digium Subversion (svnbot) 2010-10-01 11:23:17

Repository: asterisk
Revision: 289702

_U  trunk/
U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r289702 | jpeeler | 2010-10-01 11:23:17 -0500 (Fri, 01 Oct 2010) | 35 lines

Merged revisions 289701 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.8

................
 r289701 | jpeeler | 2010-10-01 11:22:19 -0500 (Fri, 01 Oct 2010) | 28 lines
 
 Merged revisions 289700 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.6.2
 
 ................
   r289700 | jpeeler | 2010-10-01 11:21:04 -0500 (Fri, 01 Oct 2010) | 21 lines
   
   Merged revisions 289699 via svnmerge from
   https://origsvn.digium.com/svn/asterisk/branches/1.4
   
   ........
     r289699 | jpeeler | 2010-10-01 11:20:00 -0500 (Fri, 01 Oct 2010) | 14 lines
     
     Ensure user portion of SIP URI matches dialplan when using encoded characters.
     
     This commit takes a simliar approach to 288112 and checks the dialplan to
     determine the proper action for an incoming contact header as to whether or not
     it should be decoded or not. sip_new was blindly always decoding the extension,
     which also caused the outgoing contact header to be incorrect as well as failing
     to match the encoded extension in the dialplan.
     
     (closes issue ASTERISK-16597)
     Reported by: wdoekes
     Patches:
           bug17892-1.patch uploaded by jpeeler (license 325)
     Tested by: wdoekes
   ........
 ................
................

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

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