Date: Sat, 12 Dec 2020 11:23:53 +0000 (UTC) From: =?UTF-8?Q?Stefan_E=c3=9fer?= <se@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r368577 - head/lib/libutil Message-ID: <202012121123.0BCBNrdD003535@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: se Date: Sat Dec 12 11:23:52 2020 New Revision: 368577 URL: https://svnweb.freebsd.org/changeset/base/368577 Log: Change getlocalbase() to not allocate any heap memory After the commit of the current version, Scott Long pointed out, that an attacker might be able to cause a use-after-free access if this function returned the value of the sysctl variable "user.localbase" by freeing the allocated memory without the cached address being cleared in the library function. To resolve this issue, I have proposed the originally suggested version with a statically allocated buffer in a review (D27370). There was no feedback on this review and after waiting for more than 2 weeks, the potential security issue is fixed by this commit. (There was no security risk in practice, since none of the programs converted to use this function attempted to free the buffer. The address could only have pointed into the heap if user.localbase was set to a non-default value, into r/o data or the environment, else.) This version uses a static buffer of size LOCALBASE_CTL_LEN, which defaults to MAXPATHLEN. This does not increase the memory footprint of the library at this time, since its data segment grows from less than 7 KB to less than 8 KB, i.e. it will get two 4 KB pages on typical architectures, anyway. Compiling with LOCALBASE_CTL_LEN defined as 0 will remove the code that accesses the sysctl variable, values between 1 and MAXPATHLEN-1 will limit the maximum size of the prefix. When built with such a value and if too large a value has been configured in user.localbase, the value defined as ILLEGAL_PREFIX will be returned to cause any file operations on that result to fail. (Default value is "/dev/null/", the review contained "/\177", but I assume that "/dev/null" exists and can not be accessed as a directory. Any other string that can be assumed not be a valid path prefix could be used.) I do suggest to use LOCALBASE_CTL_LEN to size the in-kernel buffer for the user.localbase variable, too. Doing this would guarantee that the result always fit into the buffer in this library function (unless run on a kernel built with a different buffer size.) The function always returns a valid string, and only in case it is built with a small static buffer and run on a system with too large a value in user.localbase, the ILLEGAL_PREFIX will be returned, effectively causing the created path to be non-existent. Differential Revision: https://reviews.freebsd.org/D27370 Modified: head/lib/libutil/getlocalbase.3 head/lib/libutil/getlocalbase.c Modified: head/lib/libutil/getlocalbase.3 ============================================================================== --- head/lib/libutil/getlocalbase.3 Sat Dec 12 07:22:38 2020 (r368576) +++ head/lib/libutil/getlocalbase.3 Sat Dec 12 11:23:52 2020 (r368577) @@ -27,7 +27,7 @@ .\" .\" $FreeBSD$ .\" -.Dd November 18, 2020 +.Dd November 25, 2020 .Dt GETLOCALBASE 3 .Os .Sh NAME @@ -59,7 +59,7 @@ If that is undefined then the default of .Pa /usr/local is used. .Pp -The value returned by the +The contents of the string returned by the .Fn getlocalbase function shall not be modified. .Sh IMPLEMENTATION NOTES @@ -67,13 +67,34 @@ Calls to .Fn getlocalbase will perform a setugid check on the running binary before checking the environment. +.Pp +The address returned by +.Fn getlocalbase +will point into the executing processes environment if it is the result of +.Fn getenv "LOCALBASE" , +to a static buffer if it is the result of +.Fn sysctl "user.localbase" , +and to a constant string if the compiled in default value is returned. +.Pp +The same value will be returned on successive calls during the run-time +of the program, ignoring any changes to the environment variable or the +sysctl value that might have been made. +.Pp +The +.Fn getlocalbase +function can be compiled with a non-default value of LOCALBASE_CTL_LEN. +A value of 0 will disable fetching of the sysctl value, a value less than +MAXPATHLEN will put a limit on the maximum string length supported for +this sysctl value. +If built with a non-default value of LOCALBASE_CTL_LEN, a value of the +user.localbase sysctl variable longer than this value will make +.Fn getlocalbase +return a valid string that is not a valid path prefix in any filesystem. .Sh RETURN VALUES The .Fn getlocalbase -function always succeeds and returns a pointer to a string, whose length -may exceed MAXPATHLEN if it has been derived from the environment variable -LOCALBASE. -No length checks are performed on the result. +function returns a pointer to a string, whose length may exceed MAXPATHLEN, +if it has been obtained from the environment. .Sh ENVIRONMENT The .Fn getlocalbase @@ -83,7 +104,7 @@ environment variable. .Sh ERRORS The .Fn getlocalbase -function always succeeds. +function always succeeds and returns a valid pointer to a string. .Sh SEE ALSO .Xr env 1 , .Xr src.conf 5 , Modified: head/lib/libutil/getlocalbase.c ============================================================================== --- head/lib/libutil/getlocalbase.c Sat Dec 12 07:22:38 2020 (r368576) +++ head/lib/libutil/getlocalbase.c Sat Dec 12 11:23:52 2020 (r368577) @@ -31,6 +31,7 @@ __FBSDID("$FreeBSD$"); #include <sys/param.h> #include <sys/sysctl.h> #include <sys/limits.h> +#include <errno.h> #include <stdlib.h> #include <paths.h> #include <libutil.h> @@ -40,35 +41,50 @@ __FBSDID("$FreeBSD$"); #define _PATH_LOCALBASE "/usr/local" #endif +#ifndef LOCALBASE_CTL_LEN +#define LOCALBASE_CTL_LEN MAXPATHLEN +#endif + +/* Any prefix guaranteed to not be the start of a valid path name */ +#define ILLEGAL_PREFIX "/dev/null/" + const char * getlocalbase(void) { - static const int localbase_oid[2] = {CTL_USER, USER_LOCALBASE}; +#if LOCALBASE_CTL_LEN > 0 + int localbase_oid[2] = {CTL_USER, USER_LOCALBASE}; + static char localpath[LOCALBASE_CTL_LEN]; + size_t localpathlen = LOCALBASE_CTL_LEN; +#endif char *tmppath; - size_t tmplen; static const char *localbase = NULL; + if (localbase != NULL) + return (localbase); + if (issetugid() == 0) { tmppath = getenv("LOCALBASE"); - if (tmppath != NULL && tmppath[0] != '\0') - return (tmppath); - } - if (sysctl(localbase_oid, 2, NULL, &tmplen, NULL, 0) == 0 && - (tmppath = malloc(tmplen)) != NULL && - sysctl(localbase_oid, 2, tmppath, &tmplen, NULL, 0) == 0) { - /* - * Check for some other thread already having - * set localbase - this should use atomic ops. - * The amount of memory allocated above may leak, - * if a parallel update in another thread is not - * detected and the non-NULL pointer is overwritten. - */ - if (tmppath[0] != '\0' && - (volatile const char*)localbase == NULL) + if (tmppath != NULL && tmppath[0] != '\0') { localbase = tmppath; + return (localbase); + } + } + +#if LOCALBASE_CTL_LEN > 0 + if (sysctl(localbase_oid, 2, localpath, &localpathlen, NULL, 0) != 0) { + if (errno != ENOMEM) + localbase = _PATH_LOCALBASE; else - free((void*)tmppath); - return (localbase); + localbase = ILLEGAL_PREFIX; + } else { + if (localpath[0] != '\0') + localbase = localpath; + else + localbase = _PATH_LOCALBASE; } - return (_PATH_LOCALBASE); +#else + localbase = _PATH_LOCALBASE; +#endif + + return (localbase); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202012121123.0BCBNrdD003535>