[Chilli] Bug in password handling - Rev 257 PATCH

[David Bird]

committed as rev 258

thanks

On Tue, 2009-12-15 at 13:46 +0100, Alberto Bellettato wrote:
> Long passwords still did not work with the latest svn rev. 257, so I dug into the code and I found a real problem.
> 
> In 3 code's rows, the length of the password string is calculated through strlen(), that considers the first NUL as the end of the string, as you surely know.
> 
> But in function redir_hextochar() (redir.c row 78), sscanf() is used to transform the password from hex string to char (row 87).
> 
> And here is the big problem:
> the hex password string is the result of the md5() encryption so it _can_ contain the hex '00', that will be converted to NUL by sscanf().
> 
> You can easily test it with this example:
> 
> --------------------------------------------
> #include <stdio.h> 
> 
> main () {
>   char x[3];
>   int y;
>   char ds[7];
> 
>   x[0] = '0';
>   x[1] = '0';
>   x[2] = 0;
>   if (sscanf(x, "%2x", &y) != 1) 
>     printf("\nError\n");
>   ds[0]='s';
>   ds[1]='t';
>   ds[2]='r';
>   ds[3]=(char)y;
>   ds[4]='n';
>   ds[5]='g';
>   ds[6]='\0';
>   printf("\n%s\n",ds);
> }
> ----------------------------------------------
> 
> The output will be 'str' and NOT 'str0ng' !!
> 
> Obviously this problem shows more frequently as the password's length increase because the probability of finding a HEX '00' increases with it.
> 
> So I patched the code to remove all the strlen() use with password strings, and I optimized some parts too.
> I tested it with Rev 257 configured with --enable-chilliredir and it works fine with every password length.
> NOTE: if password's length (sent by UAM service) is more than 128 chars (RFC2865 maximum length) it is truncated to the first 128 chars.
> 
> Here is the patch for Rev 257:
> 
> Index: src/redir.c
> ===================================================================
> --- src/redir.c	(revisione 257)
> +++ src/redir.c	(copia locale)
> @@ -85,10 +85,8 @@
>      x[1] = src[n*2+1];
>      x[2] = 0;
>      if (sscanf(x, "%2x", &y) != 1) {
> -      /*log_err(0, "HEX conversion failed!");
> -	return -1;*/
> -      dst[n] = 0;
> -      break;
> +      log_err(0, "HEX conversion failed!");
> +      return -1;
>      }
>      dst[n] = (unsigned char) y;
>    }
> @@ -405,7 +403,7 @@
>    
>    hex[0]=0;
>    for (i=0; i<16; i++)
> -    sprintf(hex+strlen(hex), "%.2X", cksum[i]);
> +    sprintf(hex+2*i, "%.2X", cksum[i]);
>    
>    bcatcstr(str, amp);
>    bcatcstr(str, "md=");
> @@ -1570,8 +1568,11 @@
>  	conn->password[0] = 0;
>        }
>        else if (!redir_getparam(redir, httpreq->qs, "password", bt)) {
> -	redir_hextochar(bt->data, conn->password, RADIUS_PWSIZE);
> -	conn->password[RADIUS_PWSIZE-1]=0;
> +	conn->password_len = bt->slen/2;
> +	log_dbg("Password length: %d",conn->password_len);
> +	if (conn->password_len > RADIUS_PWSIZE)
> +	  conn->password_len = RADIUS_PWSIZE;
> +	redir_hextochar(bt->data, conn->password, conn->password_len);
>  	conn->chap = 0;
>  	conn->chappassword[0] = 0;
>        } else {
> @@ -1798,7 +1799,7 @@
>        for (n=0; n < REDIR_MD5LEN; m++, n++)
>  	user_password[m] = conn->password[m] ^ chap_challenge[n];
>      
> -    user_password[strlen((char *)conn->password)] = 0;
> +    user_password[conn->password_len] = 0;
>  
>  #ifdef HAVE_OPENSSL
>      if (_options.mschapv2) {
> @@ -1810,7 +1811,7 @@
>        
>        GenerateNTResponse(chap_challenge, /*peer*/chap_challenge,
>  			 conn->s_state.redir.username, strlen(conn->s_state.redir.username),
> -			 user_password, strlen(user_password),
> +			 user_password, conn->password_len,
>  			 ntresponse);
>        
>        /* peer challenge - same as auth challenge */
> @@ -1831,7 +1832,7 @@
>  #endif
>        
>        radius_addattr(radius, &radius_pack, RADIUS_ATTR_USER_PASSWORD, 0, 0, 0,
> -		     user_password, strlen((char*)user_password));
> +		     user_password, conn->password_len);
>  
>  #ifdef HAVE_OPENSSL
>      }
> Index: src/redir.h
> ===================================================================
> --- src/redir.h	(revisione 257)
> +++ src/redir.h	(copia locale)
> @@ -79,6 +79,7 @@
>    int response; /* 0: No radius response yet; 1:Reject; 2:Accept; 3:Timeout */
>    uint8_t chappassword[RADIUS_CHAPSIZE];
>    uint8_t password[RADIUS_PWSIZE];
> +  int password_len;
>    uint8_t chap_ident;
>    
>    /* 
> Index: src/radius.c
> ===================================================================
> --- src/radius.c	(revisione 257)
> +++ src/radius.c	(copia locale)
> @@ -1064,7 +1064,7 @@
>      *dstlen = srclen;                 /* No padding */
>  
>    /* Is dstsize too small ? */
> -  if (dstsize <= *dstlen) {
> +  if (dstsize < *dstlen) {
>      *dstlen = 0;
>      return -1;
>    }
> 
> 
> 
> _______________________________________________
> Chilli mailing list
> Chilli at coova.org
> http://lists.coova.org/cgi-bin/mailman/listinfo/chilli