[asterisk-dev] [Code Review] Fix SIP directmedia's use of ACL so that the directmedia reachability of peers is checked in a sensible fashion

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1899/#review6200
-----------------------------------------------------------

Ship it!


Aside from the comment below, I think this is good to go for 1.8 and 10.

As unsavory as it is for a channel to reach across the bridge, this is probably the only option for 1.8 and 10 due to ABI restrictions on ast_rtp_glue and ast_udptl_protocol. For trunk, on the other hand, it would be best to add a callback to each of these such that if the two channels being bridged are of the same technology, call a callback so that any technology-specific restrictions can be ironed out to determine if the two are capable of being native bridged.


/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1899/#comment11361>

    One thing you're missing in this is to make sure that opp_chan is a SIP channel. Typical way to do this is to check
    
    if (opp_chan->tech == &sip_tech)
    
    This needs to be done in the other sections where similar code exists.


- Mark


On May 3, 2012, 10:54 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1899/
> -----------------------------------------------------------
> 
> (Updated May 3, 2012, 10:54 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and Matt Jordan.
> 
> 
> Summary
> -------
> 
> Prior to this patch, when comparing the host address to the allowable set of host addresses, the address of a peer would be compared to its own allowable address.  What we wanted with this was for the host to use the opposed peer's address for checking whether or not directmedia should be allowed.  This patch does that by getting the bridged channel's pvt and using that pvt's directmediaha to compare against to compare with.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 364839 
> 
> Diff: https://reviewboard.asterisk.org/r/1899/diff
> 
> 
> Testing
> -------
> 
> I had some test code in here while I was testing to make sure all of this made sense... it would basically print the acls being used for comparisons along with the ip addresses being compared.
> 
> 2 phones with ip addresses 10.24.22.201 and 10.24.22.193 (shortened to 201 and 193).
> 
> Prior to the patch:
> 
> [201]
> directmediadeny = 0.0.0.0
> directmediapermit = 10.24.22.193
> 
> When 193 called 201, directmedia would be prevented because the ip address of 201 didn't match the directmediapermit address.  When 201 called 193, directmedia would work as no directmedia ha would actually be touched.
> 
> After the patch, 193 to 201 results in a connection because two comparisons occured, the first was 201 against 193's directmediaha (which was empty so that check passed).  The second was 193 against 201's directmediaha (which permits 193, so it passed direct media connected).
> 
> In a second scenario, the following was applied:
> 
> [201]
> directmediapermit = 0.0.0.0
> directmediadeny = 10.24.22.193
> 
> and another phone with a different IP address was added.
> 
> With the patch, calling 201 from 193 resulted in no directmedia connection when 193 compared against 201's directmediaha and was rejected.  The new phone passed the check since it used a different IP address.  Calls from 201 to 193 similarly didn't do a directmedia connection because of the same filter on 201 rejecting 193.
> 
> 
> Unfortunately, video, text, and udptl couldn't really be checked, but the concepts are the same so it should probably be ok.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120511/7b664b4d/attachment.htm>