[pacman-dev,v2] pacman: fix possible buffer overflow
diff mbox

Message ID 20180922223013.14333-1-morganamilo@gmail.com
State Changes Requested
Delegated to: Andrew Gregory
Headers show

Commit Message

morganamilo Sept. 22, 2018, 10:30 p.m. UTC
in the function query_fileowner, if the user enters a string longer
than PATH_MAX then rpath will buffer overflow when lrealpath tries to
strcat everything together.

So make sure to bail early if the generated path is going to be bigger
than PATH_MAX.

This also fixes the compiler warning:
query.c: In function ‘query_fileowner’:
query.c:192:4: warning: ‘strncpy’ specified bound 4096 equals destination size [-Wstringop-truncation]
    strncpy(rpath, filename, PATH_MAX);

Signed-off-by: morganamilo <morganamilo@gmail.com>

Comments

Andrew Gregory Oct. 17, 2018, 2:09 a.m. UTC | #1
On 09/22/18 at 11:30pm, morganamilo wrote:
> in the function query_fileowner, if the user enters a string longer
> than PATH_MAX then rpath will buffer overflow when lrealpath tries to
> strcat everything together.
> 
> So make sure to bail early if the generated path is going to be bigger
> than PATH_MAX.
> 
> This also fixes the compiler warning:
> query.c: In function ‘query_fileowner’:
> query.c:192:4: warning: ‘strncpy’ specified bound 4096 equals destination size [-Wstringop-truncation]
>     strncpy(rpath, filename, PATH_MAX);
> 
> Signed-off-by: morganamilo <morganamilo@gmail.com>
> 
> diff --git a/src/pacman/query.c b/src/pacman/query.c
> index 00c39638..c661fafb 100644
> --- a/src/pacman/query.c
> +++ b/src/pacman/query.c
> @@ -102,7 +102,7 @@ static char *lrealpath(const char *path, char *resolved_path)
>  {
>  	const char *bname = mbasename(path);
>  	char *rpath = NULL, *dname = NULL;
> -	int success = 0;
> +	int success = 0, len;
>  
>  	if(strcmp(bname, ".") == 0 || strcmp(bname, "..") == 0) {
>  		/* the entire path needs to be resolved */
> @@ -115,8 +115,14 @@ static char *lrealpath(const char *path, char *resolved_path)
>  	if(!(rpath = realpath(dname, NULL))) {
>  		goto cleanup;
>  	}
> +
> +	len = strlen(rpath) + strlen(bname) + 2;
> +	if (len > PATH_MAX) {
> +		errno = ENAMETOOLONG;
> +		goto cleanup;
> +	}
>  	if(!resolved_path) {
> -		if(!(resolved_path = malloc(strlen(rpath) + strlen(bname) + 2))) {
> +		if(!(resolved_path = malloc(len))) {
>  			goto cleanup;
>  		}
>  	}
> @@ -187,11 +193,16 @@ static int query_fileowner(alpm_list_t *targets)
>  			}
>  		}
>  
> +		errno = 0;

No need to explicitly set errno here.  lrealpath should always set it
on failure.

>  		if(!lrealpath(filename, rpath)) {
> +			if (errno == ENAMETOOLONG) {
> +				pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), filename);
> +				goto targcleanup;
> +			}
>  			/* Can't canonicalize path, try to proceed anyway */
> -			strncpy(rpath, filename, PATH_MAX);
> +			strncpy(rpath, filename, PATH_MAX - 1);
> +			rpath[PATH_MAX - 1] = '\0';

This silences the warning, but filename could still be longer than
PATH_MAX, in which case the results will be incorrect.  I think the
best thing to do is treat ENAMETOOLONG the same as any other lrealpath
error, and explicitly check if strlen(filename) >= PATH_MAX instead.

>  		}
> -

Please leave this empty line to separate the two blocks.

>  		if(strncmp(rpath, root, rootlen) != 0) {
>  			/* file is outside root, we know nothing can own it */
>  			pm_printf(ALPM_LOG_ERROR, _("No package owns %s\n"), filename);
> -- 
> 2.19.0

Patch
diff mbox

diff --git a/src/pacman/query.c b/src/pacman/query.c
index 00c39638..c661fafb 100644
--- a/src/pacman/query.c
+++ b/src/pacman/query.c
@@ -102,7 +102,7 @@  static char *lrealpath(const char *path, char *resolved_path)
 {
 	const char *bname = mbasename(path);
 	char *rpath = NULL, *dname = NULL;
-	int success = 0;
+	int success = 0, len;
 
 	if(strcmp(bname, ".") == 0 || strcmp(bname, "..") == 0) {
 		/* the entire path needs to be resolved */
@@ -115,8 +115,14 @@  static char *lrealpath(const char *path, char *resolved_path)
 	if(!(rpath = realpath(dname, NULL))) {
 		goto cleanup;
 	}
+
+	len = strlen(rpath) + strlen(bname) + 2;
+	if (len > PATH_MAX) {
+		errno = ENAMETOOLONG;
+		goto cleanup;
+	}
 	if(!resolved_path) {
-		if(!(resolved_path = malloc(strlen(rpath) + strlen(bname) + 2))) {
+		if(!(resolved_path = malloc(len))) {
 			goto cleanup;
 		}
 	}
@@ -187,11 +193,16 @@  static int query_fileowner(alpm_list_t *targets)
 			}
 		}
 
+		errno = 0;
 		if(!lrealpath(filename, rpath)) {
+			if (errno == ENAMETOOLONG) {
+				pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), filename);
+				goto targcleanup;
+			}
 			/* Can't canonicalize path, try to proceed anyway */
-			strncpy(rpath, filename, PATH_MAX);
+			strncpy(rpath, filename, PATH_MAX - 1);
+			rpath[PATH_MAX - 1] = '\0';
 		}
-
 		if(strncmp(rpath, root, rootlen) != 0) {
 			/* file is outside root, we know nothing can own it */
 			pm_printf(ALPM_LOG_ERROR, _("No package owns %s\n"), filename);