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

[Jaco Kroon]

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.

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:

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

Kind Regards,
Jaco