Review Board 1.7.16


chan_sip check_via does a hostname lookup but discards results anyway

Review Request #2400 - Created March 18, 2013 and updated

wdoekes
1.8
Reviewers
asterisk-dev
Asterisk
The sent-by-addr address in the Via is supposed to get ignored and the sent-by-port is only used if we're not doing force_rport.

Nevertheless did check_via do hostname lookups even though it only wanted to get at the port.

I've changed the code around so it uses ast_sockaddr_split_hostport() instead of ast_sockaddr_resolve_first(). This also removes an ERROR notice which isn't worthy of such severity.


Before, when someone sent something resolvable in the Via:

  chan_sip did an DNS A lookup, but ignored the results.

Now:

  chan_sip just looks at the port.


Before, when someone sent a bad via header:

  ERROR[29769]: netsock2.c:269 ast_sockaddr_resolve: getaddrinfo("127.0.1.1", "5061branch=z9hG4bK-11063-1-0", ...): Servname not supported for ai_socktype
  WARNING[29769]: chan_sip.c:16921 check_via: Could not resolve socket address for '127.0.1.1:5061branch=z9hG4bK-11063-1-0'

Now:

  WARNING[10229]: chan_sip.c:16928 check_via: Could not split host/port for '127.0.1.1:5061branch=z9hG4bK-11049-1-0'
57 SIP tests ran successfully with and without the patch.
Posted (March 18, 2013, 7:44 a.m.)
This change makes sense to me.  No need to do a DNS lookup if all we want is the port information.

In 11 and trunk, we use the resolved address in attempting to determine if the UA is behind NAT and set the natdetected flag.  So, I think the patch there would be slightly different from this one.
  1. Good point.
    
       Before a request is sent, the client transport MUST insert a value of
       the "sent-by" field into the Via header field.  This field contains
       an IP address or host name, and port.  The usage of an FQDN is
       RECOMMENDED.  This field is used for sending responses under certain
       conditions, described below.  If the port is absent, the default
       value depends on the transport.  It is 5060 for UDP, TCP and SCTP,
       5061 for TLS.
    
    And since it is recommended, I guess we must leave the resolving in place, even though we won't use it most of the time.
    
    Regard this as a blocked-for-11-and-up patch for now.
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
 
 
Curious if we could use what is in asterisk code for checking if ports are valid, although it is pretty clear as it is.

You could call ast_sockaddr_split_hostport with the flag PARSE_PORT_REQUIRED which would fail immediately without any extra checks needed.

  if (!ast_sockaddr_split_hostport(tmp, &tmphost, &tmpport, PARSE_PORT_REQUIRED)) {
      ast_log(LOG_WARNING, "Could not split host/port for '%s'\n", c);
  }

Upon success of the above, you could also check that the port returned is valid by using ast_parse_arg().

  else if (ast_parse_arg(&tmpport, PARSE_UINT32|PARSE_IN_RANGE, &port, 1024, 65535)) {
    ast_log(LOG_WARNING, "Invalid port number, '%s', in VIA header\n", tmpport);
    port = STANDARD_SIP_PORT;
  }

In other parts of the code, when we check for valid ports we are using the range 1024-65535, not 0-65535.
  1. I meant 1-65535.
These were just some ideas that I had when looking at this review.
  1. Thanks for the review.
    
    PARSE_PORT_REQUIRED is not good, since not supplying a port is valid. 
    
    Using ast_parse_arg() could be an option, I didn't know that existed.
    
    Checking peer ports against >= 1024 is wrong. There is nothing that says a port cannot be below that. (And technically I think port 0 might be legal too.. but using that would be.. strange.)
  2. Ah, you are right... sorry for focusing on the port part of this code and also for temporarily crossing in my head client (peer) vs server on the port range part of this.

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.