Vulnerability: The wait_for_child_to_die function in main/event.c in FreeRADIUS 2.1.x before 2.1.10, in certain circumstances involving long-term database outages, does not properly handle long queue times for requests, which allows remote attackers to cause a denial of service (daemon crash) by sending many requests.
Vulnerability ID: CVE-2010-3697
CWE: CWE-399
Severity score: 2.9
Project: http://github.com/alandekok/freeradius-server
Published: 2010-10-07
File: src/main/event.c
Commit message: Do not delete "old" requests until they are free.

If the request is in the queue for 30+ seconds, do NOT delete it.
Instead, mark it as "STOP PROCESSING", and do "wait_for_child_to_die",
which waits for a child thread to pick it up, and acknowledge that it's
done.  Once it's marked done, we can finally clean it up.

This may be the underlying issue behind bug #35
Commit author: Alan T. DeKok

Vulnerable version (before fix):
 static void wait_for_child_to_die(void *ctx) 	REQUEST *request = ctx;  	rad_assert(request->magic == REQUEST_MAGIC);  	/* 	 *	If it's still queued (waiting for a thread to pick it static void wait_for_child_to_die(void *ctx) 	     (pthread_equal(request->child_pid, NO_SUCH_CHILD_PID) == 0))) {  		/* 		 *	Cap delay at five minutes. 		 */ 		if (request->delay < (USEC * 60 * 5)) { 			request->delay += (request->delay >> 1); 			radlog(L_INFO, "WARNING: Child is hung for request %u in component %s module %s.", 			       request->number, request->component, request->module); 		} else { 			RDEBUG2("Child is still stuck for request %u", 				request->number); 		} 		tv_add(&request->when, request->delay); static void wait_for_child_to_die(void *ctx) 	}  	RDEBUG2("Child is finally responsive for request %u", request->number); 	remove_from_request_hash(request);  #ifdef WITH_PROXY 	if (request->proxy) { static void wait_a_bit(void *ctx) 	switch (request->child_state) { 	case REQUEST_QUEUED: 	case REQUEST_RUNNING: 		when = request->received; 		when.tv_sec += request->root->max_request_time;  static void wait_a_bit(void *ctx) 		 *	Request still has more time.  Continue 		 *	waiting. 		 */ 		if (timercmp(&now, &when, <) || 		    ((request->listener->type == RAD_LISTEN_DETAIL) && 		     (request->child_state == REQUEST_QUEUED))) { 			if (request->delay < (USEC / 10)) { 				request->delay = USEC / 10; 			} 			request->delay += request->delay >> 1;  #ifdef WITH_DETAIL 			/* 			 *	Cap wait at some sane value for detail 			 *	files. 			 */ 			if ((request->listener->type == RAD_LISTEN_DETAIL) && 			    (request->delay > (request->root->max_request_time * USEC))) { 				request->delay = request->root->max_request_time * USEC; 			} #endif  			request->when = now; 			tv_add(&request->when, request->delay); 			callback = wait_a_bit; 			break; 		}  #if defined(HAVE_PTHREAD_H) 		/* 		 *	A child thread MAY still be running on the 		 *	request.  Ask the thread to stop working on 		 *	the request. 		 */ 		if (have_children && 		    (pthread_equal(request->child_pid, NO_SUCH_CHILD_PID) == 0)) { 			request->master_state = REQUEST_STOP_PROCESSING;  			radlog(L_ERR, "WARNING: Unresponsive child for request %u, in module %s component %s", 			       request->number, 			       request->module ? request->module : "<server core>", 			       request->component ? request->component : "<server core>"); 			 			request->delay = USEC / 4; 			tv_add(&request->when, request->delay); 			callback = wait_for_child_to_die; 			break; 		} #endif  		/* 		 *	Else no child thread is processing the 		 *	request.  We probably should have just marked 		 *	the request as 'done' elsewhere, like in the 		 *	post-proxy-fail handler.  But doing that would 		 *	involve checking for max_request_time in 		 *	multiple places, so this may be simplest. 		 */ 		request->child_state = REQUEST_DONE; 		/* FALL-THROUGH */  		/* 		 *	Mark the request as no longer running, 		 *	and clean it up.

Fixed version (after fix):
 static void wait_for_child_to_die(void *ctx) 	REQUEST *request = ctx;  	rad_assert(request->magic == REQUEST_MAGIC); 	remove_from_request_hash(request);  	/* 	 *	If it's still queued (waiting for a thread to pick it static void wait_for_child_to_die(void *ctx) 	     (pthread_equal(request->child_pid, NO_SUCH_CHILD_PID) == 0))) {  		/* 		 *	Cap delay at max_request_time 		 */ 		if (request->delay < (USEC * request->root->max_request_time)) { 			request->delay += (request->delay >> 1); 			radlog(L_INFO, "WARNING: Child is hung for request %u in component %s module %s.", 			       request->number, request->component, request->module); 		} else { 			request->delay = USEC * request->root->max_request_time; 			RDEBUG2("WARNING: Child is still stuck for request %u", 				request->number); 		} 		tv_add(&request->when, request->delay); static void wait_for_child_to_die(void *ctx) 	}  	RDEBUG2("Child is finally responsive for request %u", request->number);  #ifdef WITH_PROXY 	if (request->proxy) { static void wait_a_bit(void *ctx) 	switch (request->child_state) { 	case REQUEST_QUEUED: 	case REQUEST_RUNNING: 		/* 		 *	If we're not thread-capable, OR we're capable, 		 *	but have been told to run without threads, 		 *	complain when the requests is queued for a 		 *	thread, or running in a child thread. 		 */ #ifdef HAVE_PTHREAD_H 		if (!have_children) #endif 		{ 			rad_assert("We do not have threads, but the request is marked as queued or running in a child thread" == NULL); 			break; 		}  #ifdef HAVE_PTHREAD_H 		/* 		 *	If we have threads, wait for the child thread 		 *	to stop. 		 */ 		when = request->received; 		when.tv_sec += request->root->max_request_time;  static void wait_a_bit(void *ctx) 		 *	Request still has more time.  Continue 		 *	waiting. 		 */ 		if (timercmp(&now, &when, <)) { 			if (request->delay < (USEC / 10)) { 				request->delay = USEC / 10; 			} 			request->delay += request->delay >> 1;  			/* 			 *	Cap delays at something reasonable. 			 */ 			if (request->delay > (request->root->max_request_time * USEC)) { 				request->delay = request->root->max_request_time * USEC; 			}  			request->when = now; 			tv_add(&request->when, request->delay); 			callback = wait_a_bit; 			break; 		}  		request->master_state = REQUEST_STOP_PROCESSING;  		/* 		 *	A child thread MAY still be running on the 		 *	request.  Ask the thread to stop working on 		 *	the request. 		 */ 		if (have_children && 		    (pthread_equal(request->child_pid, NO_SUCH_CHILD_PID) == 0)) { 			radlog(L_ERR, "WARNING: Unresponsive child for request %u, in module %s component %s", 			       request->number, 			       request->module ? request->module : "<server core>", 			       request->component ? request->component : "<server core>"); 		} 			 		request->delay = USEC; 		tv_add(&request->when, request->delay); 		callback = wait_for_child_to_die; 		break; #endif  		/* 		 *	Mark the request as no longer running, 		 *	and clean it up.

Patch:
patch: @@ -479,6 +479,7 @@ static void wait_for_child_to_die(void *ctx)
 	REQUEST *request = ctx;
 
 	rad_assert(request->magic == REQUEST_MAGIC);
+	remove_from_request_hash(request);
 
 	/*
 	 *	If it's still queued (waiting for a thread to pick it
@@ -490,14 +491,15 @@ static void wait_for_child_to_die(void *ctx)
 	     (pthread_equal(request->child_pid, NO_SUCH_CHILD_PID) == 0))) {
 
 		/*
-		 *	Cap delay at five minutes.
+		 *	Cap delay at max_request_time
 		 */
-		if (request->delay < (USEC * 60 * 5)) {
+		if (request->delay < (USEC * request->root->max_request_time)) {
 			request->delay += (request->delay >> 1);
 			radlog(L_INFO, "WARNING: Child is hung for request %u in component %s module %s.",
 			       request->number, request->component, request->module);
 		} else {
-			RDEBUG2("Child is still stuck for request %u",
+			request->delay = USEC * request->root->max_request_time;
+			RDEBUG2("WARNING: Child is still stuck for request %u",
 				request->number);
 		}
 		tv_add(&request->when, request->delay);
@@ -507,7 +509,6 @@ static void wait_for_child_to_die(void *ctx)
 	}
 
 	RDEBUG2("Child is finally responsive for request %u", request->number);
-	remove_from_request_hash(request);
 
 #ifdef WITH_PROXY
 	if (request->proxy) {
@@ -1140,6 +1141,25 @@ static void wait_a_bit(void *ctx)
 	switch (request->child_state) {
 	case REQUEST_QUEUED:
 	case REQUEST_RUNNING:
+		/*
+		 *	If we're not thread-capable, OR we're capable,
+		 *	but have been told to run without threads,
+		 *	complain when the requests is queued for a
+		 *	thread, or running in a child thread.
+		 */
+#ifdef HAVE_PTHREAD_H
+		if (!have_children)
+#endif
+		{
+			rad_assert("We do not have threads, but the request is marked as queued or running in a child thread" == NULL);
+			break;
+		}
+
+#ifdef HAVE_PTHREAD_H
+		/*
+		 *	If we have threads, wait for the child thread
+		 *	to stop.
+		 */
 		when = request->received;
 		when.tv_sec += request->root->max_request_time;
 
@@ -1156,64 +1176,46 @@ static void wait_a_bit(void *ctx)
 		 *	Request still has more time.  Continue
 		 *	waiting.
 		 */
-		if (timercmp(&now, &when, <) ||
-		    ((request->listener->type == RAD_LISTEN_DETAIL) &&
-		     (request->child_state == REQUEST_QUEUED))) {
+		if (timercmp(&now, &when, <)) {
 			if (request->delay < (USEC / 10)) {
 				request->delay = USEC / 10;
 			}
 			request->delay += request->delay >> 1;
 
-#ifdef WITH_DETAIL
 			/*
-			 *	Cap wait at some sane value for detail
-			 *	files.
+			 *	Cap delays at something reasonable.
 			 */
-			if ((request->listener->type == RAD_LISTEN_DETAIL) &&
-			    (request->delay > (request->root->max_request_time * USEC))) {
+			if (request->delay > (request->root->max_request_time * USEC)) {
 				request->delay = request->root->max_request_time * USEC;
 			}
-#endif
 
 			request->when = now;
 			tv_add(&request->when, request->delay);
 			callback = wait_a_bit;
 			break;
 		}
 
-#if defined(HAVE_PTHREAD_H)
+		request->master_state = REQUEST_STOP_PROCESSING;
+
 		/*
 		 *	A child thread MAY still be running on the
 		 *	request.  Ask the thread to stop working on
 		 *	the request.
 		 */
 		if (have_children &&
 		    (pthread_equal(request->child_pid, NO_SUCH_CHILD_PID) == 0)) {
-			request->master_state = REQUEST_STOP_PROCESSING;
-
 			radlog(L_ERR, "WARNING: Unresponsive child for request %u, in module %s component %s",
 			       request->number,
 			       request->module ? request->module : "<server core>",
 			       request->component ? request->component : "<server core>");
-			
-			request->delay = USEC / 4;
-			tv_add(&request->when, request->delay);
-			callback = wait_for_child_to_die;
-			break;
 		}
+			
+		request->delay = USEC;
+		tv_add(&request->when, request->delay);
+		callback = wait_for_child_to_die;
+		break;
 #endif
 
-		/*
-		 *	Else no child thread is processing the
-		 *	request.  We probably should have just marked
-		 *	the request as 'done' elsewhere, like in the
-		 *	post-proxy-fail handler.  But doing that would
-		 *	involve checking for max_request_time in
-		 *	multiple places, so this may be simplest.
-		 */
-		request->child_state = REQUEST_DONE;
-		/* FALL-THROUGH */
-
 		/*
 		 *	Mark the request as no longer running,
 		 *	and clean it up.
path: src/main/event.c