[asterisk-dev] tilghman: branch 1.4 r129803 - /branches/1.4/channels/chan_iax2.c

[Russell Bryant]

SVN commits to the Digium repositories wrote:
> Author: tilghman
> Date: Thu Jul 10 16:57:05 2008
> New Revision: 129803
> 
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=129803
> Log:
> Correctly deal with duplicate NEW frames (due to retransmission).  Also, fixup
> the destination call number matching to be more strict and reliable.
> (closes issue #12963)
>  Reported by: jpgrayson
>  Patches: 
>        chan_iax2_dup_new_fix3.patch uploaded by jpgrayson (license 492)
>  Tested by: jpgrayson, Corydon76
> 
> Modified:
>     branches/1.4/channels/chan_iax2.c
> 
> Modified: branches/1.4/channels/chan_iax2.c
> URL: http://svn.digium.com/view/asterisk/branches/1.4/channels/chan_iax2.c?view=diff&rev=129803&r1=129802&r2=129803
> ==============================================================================
> --- branches/1.4/channels/chan_iax2.c (original)
> +++ branches/1.4/channels/chan_iax2.c Thu Jul 10 16:57:05 2008
> @@ -1438,13 +1438,13 @@
>  #define NEW_ALLOW 	1
>  #define NEW_FORCE 	2
>  
> -static int match(struct sockaddr_in *sin, unsigned short callno, unsigned short dcallno, struct chan_iax2_pvt *cur, int check_dcallno)
> +static int match(struct sockaddr_in *sin, unsigned short callno, unsigned short dcallno, struct chan_iax2_pvt *cur)
>  {
>  	if ((cur->addr.sin_addr.s_addr == sin->sin_addr.s_addr) &&
>  		(cur->addr.sin_port == sin->sin_port)) {
>  		/* This is the main host */
>  		if ( (cur->peercallno == 0 || cur->peercallno == callno) &&
> -			 (check_dcallno ? dcallno == cur->callno : 1) ) {
> +				(dcallno == 0 || cur->callno == dcallno) ) {
>  			/* That's us.  Be sure we keep track of the peer call number */
>  			return 1;
>  		}

I'm concerned that this change to match() is not the right thing to do. 
  It reverts a change that we made to address a security issue, 
AST-2008-006.

It looks like the code has been changed back to how it used to behave. 
Someone can send any FULL frames with a destination call number of 0, 
and they will always be accepted (dcallno == 0).  So, we're back to the 
problem of making it trivial to use IAX2 in a traffic amplification 
attack by no longer requiring a proper handshake.

I think the proper thing to do for the issue that was reported is to 
adjust the logic that sets check_dcallno, and make sure that it is 0 in 
the case that we are processing a NEW, LAGRQ, or PING.

-- 
Russell Bryant
Senior Software Engineer
Open Source Team Lead
Digium, Inc.