[Chilli] Bug in password handling - Rev 257 PATCH

[Alberto Bellettato]

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;
   }