Re: [LTP] [PATCH v2] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Using sprintf without length checking in tst_tmpdir may lead to buffer overflow.
> So in this patch use openat() instead of open().
> 
> Fixes: https://github.com/linux-test-project/ltp/issues/1241
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  lib/tst_tmpdir.c | 122 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 78 insertions(+), 44 deletions(-)
> 
> diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
> index 6ed2367b9..bcb9534c5 100644
> --- a/lib/tst_tmpdir.c
> +++ b/lib/tst_tmpdir.c
> @@ -101,7 +101,7 @@ static char test_start_work_dir[PATH_MAX];
>  /* lib/tst_checkpoint.c */
>  extern futex_t *tst_futexes;
>  
> -static int rmobj(const char *obj, char **errmsg);
> +static int rmobjat(int dir_fd, const char *obj, char **errmsg);
>  
>  int tst_tmpdir_created(void)
>  {
> @@ -149,8 +149,8 @@ static int purge_dir(const char *path, char **errptr)
>  	int ret_val = 0;
>  	DIR *dir;
>  	struct dirent *dir_ent;
> -	char dirobj[PATH_MAX];
>  	static char err_msg[PATH_MAX + 1280];
> +	int dir_fd = -1;
>  
>  	/* Do NOT perform the request if the directory is "/" */
>  	if (!strcmp(path, "/")) {
> @@ -167,7 +167,7 @@ static int purge_dir(const char *path, char **errptr)
>  	/* Open the directory to get access to what is in it */
>  	if (!(dir = opendir(path))) {

>  		if (errptr) {
> -			sprintf(err_msg,
> +			snprintf(err_msg, sizeof(err_msg),
>  				"Cannot open directory %s; errno=%d: %s",
>  				path, errno, tst_strerrno(errno));
>  			*errptr = err_msg;
> @@ -175,6 +175,68 @@ static int purge_dir(const char *path, char **errptr)
>  		return -1;
>  	}
>  
> +	dir_fd = dirfd(dir);
> +	if (dir_fd == -1) {
> +		closedir(dir);
> +		if (errptr) {
> +			snprintf(err_msg, sizeof(err_msg),
> +				"Cannot get file descriptor for directory %s; errno=%d: %s",
> +				path, errno, tst_strerrno(errno));
> +			*errptr = err_msg;
> +		}
> +		return -1;
> +	}
> +
> +	/* Loop through the entries in the directory, removing each one */
> +	for (dir_ent = readdir(dir); dir_ent; dir_ent = readdir(dir)) {
> +		/* Don't remove "." or ".." */
> +		if (!strcmp(dir_ent->d_name, ".")
> +		    || !strcmp(dir_ent->d_name, ".."))
> +			continue;
> +
> +		/* Recursively remove the current entry */
> +		if (rmobjat(dir_fd, dir_ent->d_name, errptr) != 0)
> +			ret_val = -1;
> +	}
> +
> +	closedir(dir);
> +	return ret_val;
> +}
> +
> +static int purge_dirat(int dir_fd, const char *path, char **errptr)
> +{
> +	int ret_val = 0;
> +	DIR *dir;
> +	struct dirent *dir_ent;
> +	static char err_msg[PATH_MAX + 1280];
> +	int subdir_fd;
> +
> +	errno = 0;
> +
> +	/* Open the subdirectory using openat */
> +	subdir_fd = openat(dir_fd, path, O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
> +	if (subdir_fd < 0) {
> +		if (errptr) {
> +			snprintf(err_msg, sizeof(err_msg),
> +				 "Cannot open subdirectory %s (via fd %d); errno=%d: %s",
> +				 path, dir_fd, errno, tst_strerrno(errno));
> +			*errptr = err_msg;
> +		}
> +		return -1;
> +	}
> +
> +	dir = fdopendir(subdir_fd);
> +	if (!dir) {
> +		if (errptr) {
> +			snprintf(err_msg, sizeof(err_msg),
> +				 "Cannot open directory stream for %s (via fd %d); errno=%d: %s",
> +				 path, dir_fd, errno, tst_strerrno(errno));
> +			*errptr = err_msg;
> +		}
> +		close(subdir_fd);
> +		return -1;
> +	}
> +
>  	/* Loop through the entries in the directory, removing each one */
>  	for (dir_ent = readdir(dir); dir_ent; dir_ent = readdir(dir)) {
>  		/* Don't remove "." or ".." */
> @@ -183,8 +245,7 @@ static int purge_dir(const char *path, char **errptr)
>  			continue;
>  
>  		/* Recursively remove the current entry */
> -		sprintf(dirobj, "%s/%s", path, dir_ent->d_name);
> -		if (rmobj(dirobj, errptr) != 0)
> +		if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0)
>  			ret_val = -1;
>  	}
>  
> @@ -192,63 +253,36 @@ static int purge_dir(const char *path, char **errptr)
>  	return ret_val;
>  }

Again, there shouldn't be two function for the same job. There should be
only purge_dirat() and the tst_purge_dir() should call purge_dirat()
with AT_FDCWD as the dirfd.

> -static int rmobj(const char *obj, char **errmsg)
> +static int rmobjat(int dir_fd, const char *obj, char **errmsg)
>  {
>  	int ret_val = 0;
> -	struct stat statbuf;
>  	static char err_msg[PATH_MAX + 1280];
>  	int fd;
>  
> -	fd = open(obj, O_DIRECTORY | O_NOFOLLOW);
> +	fd = openat(dir_fd, obj, O_DIRECTORY | O_NOFOLLOW);
>  	if (fd >= 0) {
>  		close(fd);
> -		ret_val = purge_dir(obj, errmsg);
> +		ret_val = purge_dirat(dir_fd, obj, errmsg);
>  
>  		/* If there were problems removing an entry, don't attempt to
>  		   remove the directory itself */
>  		if (ret_val == -1)
>  			return -1;
>  
> -		/* Get the link count, now that all the entries have been removed */
> -		if (lstat(obj, &statbuf) < 0) {
> +		if (unlinkat(dir_fd, obj, AT_REMOVEDIR) < 0) {
>  			if (errmsg != NULL) {
> -				sprintf(err_msg,
> -					"lstat(%s) failed; errno=%d: %s", obj,
> -					errno, tst_strerrno(errno));
> -				*errmsg = err_msg;
> -			}
> -			return -1;
> -		}
> -
> -		/* Remove the directory itself */
> -		if (statbuf.st_nlink >= 3) {
> -			/* The directory is linked; unlink() must be used */
> -			if (unlink(obj) < 0) {
> -				if (errmsg != NULL) {
> -					sprintf(err_msg,
> -						"unlink(%s) failed; errno=%d: %s",
> -						obj, errno, tst_strerrno(errno));
> -					*errmsg = err_msg;
> -				}
> -				return -1;
> -			}
> -		} else {
> -			/* The directory is not linked; remove() can be used */
> -			if (remove(obj) < 0) {
> -				if (errmsg != NULL) {
> -					sprintf(err_msg,
> +				snprintf(err_msg, sizeof(err_msg),
>  						"remove(%s) failed; errno=%d: %s",
>  						obj, errno, tst_strerrno(errno));
> -					*errmsg = err_msg;
> -				}
> -				return -1;
> +				*errmsg = err_msg;
>  			}
> +			return -1;
>  		}

I do not think that we can remove the part where we check the number of
links to the directory. I suppose that the new code should look like:

	int flags = AT_REMOVEDIR;

	if (lstatat(dir_fd, &statbuf)) {
		...
	}

	if (statbuf.st_nlink >= 3)
		flags = 0;

	if (unlinkat(dir_fd, obj, flags)) {
		...
	}


>  	} else {
> -		if (unlink(obj) < 0) {
> +		if (unlinkat(dir_fd, obj, 0) < 0) {
>  			if (errmsg != NULL) {
> -				sprintf(err_msg,
> -					"unlink(%s) failed; errno=%d: %s", obj,
> +				snprintf(err_msg, sizeof(err_msg),
> +					"unlinkat(%s) failed; errno=%d: %s", obj,
>  					errno, tst_strerrno(errno));
>  				*errmsg = err_msg;
>  			}
> @@ -305,7 +339,7 @@ void tst_tmpdir(void)
>  		tst_resm(TERRNO, "%s: chdir(%s) failed", __func__, TESTDIR);
>  
>  		/* Try to remove the directory */
> -		if (rmobj(TESTDIR, &errmsg) == -1) {
> +		if (rmobjat(0, TESTDIR, &errmsg) == -1) {
>  			tst_resm(TWARN, "%s: rmobj(%s) failed: %s",
>  				 __func__, TESTDIR, errmsg);
>  		}
> @@ -343,7 +377,7 @@ void tst_rmdir(void)
>  	/*
>  	 * Attempt to remove the "TESTDIR" directory, using rmobj().
>  	 */
> -	if (rmobj(TESTDIR, &errmsg) == -1) {
> +	if (rmobjat(0, TESTDIR, &errmsg) == -1) {
>  		tst_resm(TWARN, "%s: rmobj(%s) failed: %s",
>  			 __func__, TESTDIR, errmsg);
>  	}

We should pass AT_FDCWD to the two rmobjat() here since it's possible to
pass relative path in the TMPDIR environment variable. Otherwise the
code will not work with e.g. TMPDIR="." ./test_foo

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp