[asterisk-dev] chap_sip guest over tcp + insecure=port

[Matt Fredrickson]

Hey,

Responses inline.

On Tue, Aug 14, 2018 at 6:16 AM, Jaco Kroon <jaco at uls.co.za> wrote:
> Hi All,
>
> The following bugs refers:
>
> ASTERISK-27457 - chan_sip: Guests disallowed via TCP (or TLS) if
> existing peer from same IP
> commit b2c4e8660a9c89d07041271371151779b7ec75f6
>
> ASTERISK-27881 - PBX calls via chan_sip TCP trunk now get
> authentification error
> change set https://gerrit.asterisk.org/#/c/asterisk/+/9858/
>
> The first change seems to be part of a potentially larger patch set, I
> only looked at the specific commit I referenced.
>
> The original code assumed that TCP|TLS peers from the same IP was the
> same peer.  I don't think this applies to authenticated connections, but
> I could be wrong.
>
> The peer_ipcmp_cb{,_full} is used to find peers by IP, so I'm *guessing*
> to authenticate them based on IP address.
>
> Given that assumption I believe the old behaviour was correct in that
> any TCP based protocol can't control the source port.  The check should
> probably also have included WS and WSS protocols since those are TCP
> based too.
>
> The effect was (as I understand) that the original code, given a peer like:
>
> [peer1]
> host=a.b.c.d
> transport=udp,tcp
>
> Really could either accept a new "call" from IP a.b.c.d over a TCP
> connection with any source port, or over UDP but with source port 5060
> (note, no insecure=port).
>
> After the change, the incoming tcp connection would need to originate
> from port 5060 (not going to happen).  So the logical workaround is
> insecure=port but this now has the effect of loosening things for udp as
> well, which may or may not be unwanted.  And currently doesn't work.  My
> change set solves that particular problem, but I'm less and less
> convinced it's the right solution.

Seems like the new behavior is buggy.  TCP connections (and other
connection oriented protocols) could come from any source port.

> Looking at the logic of peer_ipcmp_cb_full:
>
> if source address doesn't match
>    => no match
>
> originally, if tcp|tls
>    => match
>
> originally, elseif peer2 has port=insecure
> now, if udp|ws|wss and peer2 has port=insecure
>    => match if peer also has port=insecure else no match
>
> if ports match
>   => match
>
> Right, so my thinking is that insecure=port only really makes sense for
> udp based transports and should be implied for non-udp based protocols,
> specifically with host!=dynamic.  Based on that I'd say the code should
> probably do the following:

That would jive with how I understand the world as well.

>
> 1.  if source addresses doesn't match => no match
>
> (2.) if peer2 transport isn't allowed (subset of?) peer transport => no
> match
>
> 3.  if transport is not udp (and peer2 is not dynamic?) => match
>
> 4.  if peer2 transport is udp, and has insecure=port:
> 4.1 if peer also has insecure=port, match, else no match.
>
> 5.  if ports match, match, else, no match.
>
> Based on the comments a further optimization may apply:
>
>  * This callback will be used twice when doing peer matching.  There is
> a first
>  * pass for full IP+port matching, and a second pass in case there is a
> match
>  * that meets the insecure=port criteria.
>
> Based on the above we can skip the second pass for non-udp peers (In
> sip_find_peer_full function), in which case point 4 can drop the test
> for transport udp too.  This second pass is also the reason why 4.1 can
> assume no-match as the first iteration would already have done the port
> comparison.
>
> With this when a transport!=udp client registers to us, both host and
> port will be implicitly set to that connection, and so will match
> specific, and since we only match non-udp host if it's not dynamic, this
> should also cover the sip/!udp guest use-case from an IP from where
> there is also a valid registration (which is what I believe tripped up
> previously).
>
> Point three partially restores the previous behaviour.  If we have a
> peer with an explicit address (in other words, !peer->host_dynamic) we
> can match here, which basically has the same effect as setting
> insecure=port implicitly for all non-UDP transports (and by implication
> negates the need for insecure=port for non-udp transports).
>
> Point two is to allow something like:
>
> [peer1]
> host=a.b.c.d
> transport=udp
>
> [peer2]
> host=a.b.c.d
> transport=tcp
>
> To function correctly, I suspect currently given the above order if the
> tcp variant comes in, the udp peer will match, resulting in transport
> not allowed.
>
> Interestingly, I'm not sure

If you'd like to try to fix it, feel free to do so.  Otherwise, we can
revert the patch that originally broke things.

-- 
Matthew Fredrickson
Digium, Inc. | Asterisk Project Lead
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA