From: Jeremy Tan Date: Thu, 3 Oct 2013 06:19:38 +0000 (+0800) Subject: Remove misleading bounds check X-Git-Url: https://git.ucc.asn.au/?a=commitdiff_plain;h=e2c333035c595ef6e48ff66d46a811ca17f97a26;p=matches%2FMCTX3420.git Remove misleading bounds check (1) Length of user is not guaranteed (and in this case definitely < BUFSIZ) (2) Because of (1), a buffer overflow is possible anyway (3) Sure, a BUFSIZ limit will prevent an 'infinite' loop, but this may just make it more difficult to track down the buffer overflow. --- diff --git a/server/login.c b/server/login.c index a616af2..2aa702d 100644 --- a/server/login.c +++ b/server/login.c @@ -168,9 +168,8 @@ void Login_Handler(FCGIContext * context, char * params) return; } - char * user = ""; // The username supplied through CGI - char * pass = ""; // The password supplied through CGI - //TODO: Make sure these are passed through HTTPS, *not* HTTP .... otherwise people can eavesdrop on the passwords + char * user; // The username supplied through CGI + char * pass; // The password supplied through CGI FCGIValue values[] = { {"user", &user, FCGI_REQUIRED(FCGI_STRING_T)}, @@ -191,17 +190,14 @@ void Login_Handler(FCGIContext * context, char * params) return; } - - // Trim leading whitespace (the BUFSIZ check is to make sure incorrectly terminated strings don't cause an infinite loop) + // Trim leading whitespace int i = 0; - for (i = 0; i < BUFSIZ && isspace(user[0]) && user[0] != '\0'; ++i,++user); + for (i = 0; isspace(user[0]) && user[0] != '\0'; ++i, ++user); // Truncate string at first non alphanumeric character - for (i = 0; i < BUFSIZ && isalnum(user[i]) && user[i] != '\0'; ++i); + for (i = 0; isalnum(user[i]) && user[i] != '\0'; ++i); user[i] = '\0'; - - bool authenticated = true; @@ -260,5 +256,5 @@ void Login_Handler(FCGIContext * context, char * params) // Give the user a cookie FCGI_PrintRaw("Content-type: text\r\n"); FCGI_PrintRaw("Set-Cookie: %s\r\n\r\n", context->control_key); - + FCGI_PrintRaw("Logged in"); }