I've recently gotten into the kick of inspecting source code on my system for bugs and vulnerabilities. Picked conky to start with, basically just because I thought it would be smaller and easier to understand. Anyway, I ran flawfinder on it, and the first thing that popped up was this:
Code:
./core.c:99: [5] (race) readlink:
This accepts filename arguments; if an attacker can move those files
or change the link content, a race condition results. Also, it does not
terminate with ASCII NUL. Reconsider approach.
Of course, not all flawfinder reports are real problems, so I went to the source, and found it in the function
dev_name:
Code:
/* strip a leading /dev/ if any, following symlinks first
*
* BEWARE: this function returns a pointer to static content
* which gets overwritten in consecutive calls. I.e.:
* this function is NOT reentrant.
*/
const char *dev_name(const char *path)
{
static char buf[255]; /* should be enough for pathnames */
ssize_t buflen;
if (!path)
return NULL;
#define DEV_NAME(x) \
x != NULL && strlen(x) > 5 && strncmp(x, "/dev/", 5) == 0 ? x + 5 : x
if ((buflen = readlink(path, buf, 254)) == -1)
return DEV_NAME(path);
buf[buflen] = '\0';
return DEV_NAME(buf);
#undef DEV_NAME
}
So, it appears that the function
dev_name is used in the rest of the code to pre-process a file path argument that is supposed to point to a
dev file. For the most part all the function is supposed to do is strip "/dev/" from the file name; however, it also calls the
readlink function to determine if the path refers to a symbolic link, in which case
dev_name will return the file the link points toward rather than simply the modified version of the original path.
So, a few concerns come to mind:
1. Race condition? Is this a good place to be calling
readlink?
2. Necessary? What is wrong with simply using the path to the symbolic link?
3. Bad error handling? Notice that, if
readlink returns
-1 (error) then
dev_name simply forgets about the symbolic link and returns the modified version of the original path. However, READLINK(2) indicates that there are a quite a number of reasons that a
readlink call might fail:
Code:
RETURN VALUE
On success, readlink() returns the number of bytes placed in buf. On error, -1 is returned and errno is set to indi‐
cate the error.
ERRORS
EACCES Search permission is denied for a component of the path prefix. (See also path_resolution(7).)
EFAULT buf extends outside the process's allocated address space.
EINVAL bufsiz is not positive.
EINVAL The named file is not a symbolic link.
EIO An I/O error occurred while reading from the file system.
ELOOP Too many symbolic links were encountered in translating the pathname.
ENAMETOOLONG
A pathname, or a component of a pathname, was too long.
ENOENT The named file does not exist.
ENOMEM Insufficient kernel memory was available.
ENOTDIR
A component of the path prefix is not a directory.
So it seems like we are just "moving on" under the assumption that the file is not a symbolic link, when there could be some other problem. Maybe would should be exiting the program, or logging an error, or something?
Anyway, before I go throwing my ignorant bug reports at the developer, I was curious for feedback.