[Asterisk-code-review] chan sip: tcp/tls honour insecure=port (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/9858 )

Change subject: chan_sip: tcp/tls honour insecure=port
......................................................................


Patch Set 6:

(2 comments)

https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c
File channels/chan_sip.c:

https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34458
PS6, Line 34458: 	return peer->host_dynamic ? 0 : (CMP_MATCH | CMP_STOP);
> Please refer my comments and explanation in the commit message. […]
A friend is going to attempt name matching first before it attempts IP address matching.

The problem with matching IP addresses using the peer->addr is that it is the contact address of the peer.  It is not necessarily the source address of the transport.  If we believe the contact header during registration we are unlikely to ever match by IP and port.  We believe the contact header during registration if NAT is not configured on the peer and the incoming REGISTER message does not have rport.

How about this to shrink the number of unmatching cases:

if (!peer->host_dynamic) {
   return CMP_MATCH;
}
/* Conditions taken from parse_register_contact() */
if (peer2->transports & (AST_TRANSPORT_WS | AST_TRANSPORT_WSS)) {
   /* The contact address of websockets is always the transport source address and port */
   return 0;
}
if (ast_test_flag(&peer->flags[0], SIP_NAT_FORCE_RPORT)) {
   /* The contact address of NATed peers is always the transport source address and port */
   return 0;
}
/* Have to assume that we used the registered contact header */
return CMP_MATCH;


https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34461
PS6, Line 34461: static int peer_ipcmp_cb(void *obj, void *arg, int flags)
               : {
               : 	return peer_ipcmp_cb_full(obj, arg, NULL, flags);
               : }
> It's still in use in load_module() for the peers_by_ip container (which is used in various places). […]
It is dead.  It is referenced but never called.  That function will only get called if something uses ao2_find() on that container.  Nothing uses ao2_find() on that container so it will never get called.

It is confusing because it isn't clear how peer_ipcmp_cb() gets called nor is it clear how it could do two passes.  Two passes is an implicit assumption built into peer_ipcmp_cb_full().



-- 
To view, visit https://gerrit.asterisk.org/9858
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61a9804e4feba9c7224c481f7a10bf7eb7c7f2a2
Gerrit-Change-Number: 9858
Gerrit-PatchSet: 6
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 23 Aug 2018 18:54:41 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180823/7518fdfe/attachment.html>