kern/132104: kenv buffer overflow

[Dylan Cochran]

>Number:         132104
>Category:       kern
>Synopsis:       kenv buffer overflow
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Wed Feb 25 17:30:01 UTC 2009
>Closed-Date:
>Last-Modified:
>Originator:     Dylan Cochran
>Release:        7.1-RELEASE
>Organization:
Evoke Project
>Environment:
FreeBSD  7.1-RELEASE-p3 FreeBSD 7.1-RELEASE-p3 #0: Wed Dec 31 19:00:00 EST 1969     root at dummy:/root/evoke-head/obj/obj/13a419ec44df0f8e7392ecf9be07334a/i386/root/evoke-head/obj/13a419ec44df0f8e7392ecf9be07334a/usr/src/sys/kernel  i386
>Description:
The kenv syscall, when called with the KENV_GET action, first allocates a static size buffer, holds the kenv mutex, copies the data in the pointer to the buffer. It then releases the mutex, and runs strlen over the buffer, malloc's a return buffer the size of strlen's return value, and copies from the initial buffer to the return buffer. This usage case only works with environment variables defined by the KENV_SET action, which restricts the length of a value to 128 bytes.
>How-To-Repeat:
loader has no such restriction, and attempting to KENV_GET a variable set by loader that is longer then 128bytes causes an immediate page fault. Add a long string value to /boot/loader.conf and then kenv the name of the variable.
>Fix:
Remove the statically allocated buffer, and move the mutex back to the point where the return buffer is allocated and the data moved. This prevents the panic condition, but also increases the amount of time the mutex is held. Comments?

Patch attached with submission follows:

--- sys/kern/kern_environment.c	2009-02-20 12:31:36.000000000 -0500
+++ sys/kern/kern_environment.c	2009-02-24 23:26:43.000000000 -0500
@@ -293,7 +293,6 @@
 char *
 getenv(const char *name)
 {
-	char buf[KENV_MNAMELEN + 1 + KENV_MVALLEN + 1];
 	char *ret, *cp;
 	int len;
 
@@ -301,11 +300,10 @@
 		mtx_lock(&kenv_lock);
 		cp = _getenv_dynamic(name, NULL);
 		if (cp != NULL) {
-			strcpy(buf, cp);
-			mtx_unlock(&kenv_lock);
-			len = strlen(buf) + 1;
+			len = strlen(cp) + 1;
 			ret = malloc(len, M_KENV, M_WAITOK);
-			strcpy(ret, buf);
+			strcpy(ret, cp);
+			mtx_unlock(&kenv_lock);
 		} else {
 			mtx_unlock(&kenv_lock);
 			ret = NULL;


>Release-Note:
>Audit-Trail:
>Unformatted: