| Message ID | 20180922223013.14333-1-morganamilo@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Andrew Gregory |
| Headers | show |
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
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);
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>