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 <[email protected]>
>
> 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
