[PATCH 1/2] pacman: fix possible buffer overflow

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] pacman: fix possible buffer overflow

morganamilo
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.

Even if we made sure filename was never longer than PATH_MAX this would
not help because lrealpath may concatenate filename with the current
directory among other things.

So simply let lrealpath calculate the size needed and allocate it for
us.

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);

The warning could have also been Fixed by using PATH_MAX -1 and explicitly
terminating the string. However this would not fix the buffer overflow.

Signed-off-by: morganamilo <[hidden email]>

diff --git a/src/pacman/query.c b/src/pacman/query.c
index 00c39638..a1197cea 100644
--- a/src/pacman/query.c
+++ b/src/pacman/query.c
@@ -155,10 +155,10 @@ static int query_fileowner(alpm_list_t *targets)
 
  for(t = targets; t; t = alpm_list_next(t)) {
  char *filename = NULL;
- char rpath[PATH_MAX], *rel_path;
+ char *rpath = NULL, *rpathsave, *rel_path;
  struct stat buf;
  alpm_list_t *i;
- size_t len;
+ size_t len, rlen;
  unsigned int found = 0;
  int is_dir = 0, is_missing = 0;
 
@@ -187,9 +187,15 @@ static int query_fileowner(alpm_list_t *targets)
  }
  }
 
- if(!lrealpath(filename, rpath)) {
+ if(!(rpath = lrealpath(filename, NULL))) {
  /* Can't canonicalize path, try to proceed anyway */
- strncpy(rpath, filename, PATH_MAX);
+ rpath = strdup(filename);
+ }
+
+ rlen = strlen(rpath);
+ if(rlen + 1 >= PATH_MAX) {
+ pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath);
+ goto targcleanup;
  }
 
  if(strncmp(rpath, root, rootlen) != 0) {
@@ -201,11 +207,14 @@ static int query_fileowner(alpm_list_t *targets)
  rel_path = rpath + rootlen;
 
  if((is_missing && is_dir) || (!is_missing && (is_dir = S_ISDIR(buf.st_mode)))) {
- size_t rlen = strlen(rpath);
  if(rlen + 2 >= PATH_MAX) {
  pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath);
  goto targcleanup;
  }
+ if ((rpathsave = realloc(rpath, rlen + 2)) == NULL) {
+ goto targcleanup;
+ }
+ rpath = rpathsave;
  strcat(rpath + rlen, "/");
  }
 
--
2.19.0
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] pacman: give path too long error after strcat

morganamilo
The string only becomes longer than PATH_MAX once adding "/" to the end.
The error message should give this version of the path.

Signed-off-by: morganamilo <[hidden email]>

diff --git a/src/pacman/query.c b/src/pacman/query.c
index a1197cea..ecf8d148 100644
--- a/src/pacman/query.c
+++ b/src/pacman/query.c
@@ -207,15 +207,15 @@ static int query_fileowner(alpm_list_t *targets)
  rel_path = rpath + rootlen;
 
  if((is_missing && is_dir) || (!is_missing && (is_dir = S_ISDIR(buf.st_mode)))) {
- if(rlen + 2 >= PATH_MAX) {
- pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath);
- goto targcleanup;
- }
  if ((rpathsave = realloc(rpath, rlen + 2)) == NULL) {
  goto targcleanup;
  }
  rpath = rpathsave;
  strcat(rpath + rlen, "/");
+ if(rlen + 2 >= PATH_MAX) {
+ pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath);
+ goto targcleanup;
+ }
  }
 
  for(i = packages; i && (!found || is_dir); i = alpm_list_next(i)) {
--
2.19.0
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] pacman: give path too long error after strcat

Andrew Gregory
On 09/22/18 at 09:16pm, morganamilo wrote:
> The string only becomes longer than PATH_MAX once adding "/" to the end.
> The error message should give this version of the path.
>
> Signed-off-by: morganamilo <[hidden email]>

NACK.  The trailing slash is in the format string; your version now
has two trailing slashes..

> diff --git a/src/pacman/query.c b/src/pacman/query.c
> index a1197cea..ecf8d148 100644
> --- a/src/pacman/query.c
> +++ b/src/pacman/query.c
> @@ -207,15 +207,15 @@ static int query_fileowner(alpm_list_t *targets)
>   rel_path = rpath + rootlen;
>  
>   if((is_missing && is_dir) || (!is_missing && (is_dir = S_ISDIR(buf.st_mode)))) {
> - if(rlen + 2 >= PATH_MAX) {
> - pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath);
> - goto targcleanup;
> - }
>   if ((rpathsave = realloc(rpath, rlen + 2)) == NULL) {
>   goto targcleanup;
>   }
>   rpath = rpathsave;
>   strcat(rpath + rlen, "/");
> + if(rlen + 2 >= PATH_MAX) {
> + pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath);
> + goto targcleanup;
> + }
>   }
>  
>   for(i = packages; i && (!found || is_dir); i = alpm_list_next(i)) {
> --
> 2.19.0
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] pacman: give path too long error after strcat

morganamilo
On Sat, 22 Sep 2018 at 21:54, Andrew Gregory <[hidden email]> wrote:

>
> On 09/22/18 at 09:16pm, morganamilo wrote:
> > The string only becomes longer than PATH_MAX once adding "/" to the end.
> > The error message should give this version of the path.
> >
> > Signed-off-by: morganamilo <[hidden email]>
>
> NACK.  The trailing slash is in the format string; your version now
> has two trailing slashes..
>
> > diff --git a/src/pacman/query.c b/src/pacman/query.c
> > index a1197cea..ecf8d148 100644
> > --- a/src/pacman/query.c
> > +++ b/src/pacman/query.c
> > @@ -207,15 +207,15 @@ static int query_fileowner(alpm_list_t *targets)
> >               rel_path = rpath + rootlen;
> >
> >               if((is_missing && is_dir) || (!is_missing && (is_dir = S_ISDIR(buf.st_mode)))) {
> > -                     if(rlen + 2 >= PATH_MAX) {
> > -                                     pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath);
> > -                                     goto targcleanup;
> > -                     }
> >                       if ((rpathsave = realloc(rpath, rlen + 2)) == NULL) {
> >                               goto targcleanup;
> >                       }
> >                       rpath = rpathsave;
> >                       strcat(rpath + rlen, "/");
> > +                     if(rlen + 2 >= PATH_MAX) {
> > +                                     pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath);
> > +                                     goto targcleanup;
> > +                     }
> >               }
> >
> >               for(i = packages; i && (!found || is_dir); i = alpm_list_next(i)) {
> > --
> > 2.19.0

Oh yeah silly me, missed the / inside the printf. Disregard this patch then.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] pacman: fix possible buffer overflow

Andrew Gregory
In reply to this post by morganamilo
On 09/22/18 at 09:16pm, 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.
>
> Even if we made sure filename was never longer than PATH_MAX this would
> not help because lrealpath may concatenate filename with the current
> directory among other things.
>
> So simply let lrealpath calculate the size needed and allocate it for
> us.
>
> 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);
>
> The warning could have also been Fixed by using PATH_MAX -1 and explicitly
> terminating the string. However this would not fix the buffer overflow.
>
> Signed-off-by: morganamilo <[hidden email]>
>
> diff --git a/src/pacman/query.c b/src/pacman/query.c
> index 00c39638..a1197cea 100644
> --- a/src/pacman/query.c
> +++ b/src/pacman/query.c
> @@ -155,10 +155,10 @@ static int query_fileowner(alpm_list_t *targets)
>  
>   for(t = targets; t; t = alpm_list_next(t)) {
>   char *filename = NULL;
> - char rpath[PATH_MAX], *rel_path;
> + char *rpath = NULL, *rpathsave, *rel_path;
>   struct stat buf;
>   alpm_list_t *i;
> - size_t len;
> + size_t len, rlen;
>   unsigned int found = 0;
>   int is_dir = 0, is_missing = 0;
>  
> @@ -187,9 +187,15 @@ static int query_fileowner(alpm_list_t *targets)
>   }
>   }
>  
> - if(!lrealpath(filename, rpath)) {
> + if(!(rpath = lrealpath(filename, NULL))) {
>   /* Can't canonicalize path, try to proceed anyway */
> - strncpy(rpath, filename, PATH_MAX);
> + rpath = strdup(filename);
> + }
> +
> + rlen = strlen(rpath);
> + if(rlen + 1 >= PATH_MAX) {
> + pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath);
> + goto targcleanup;
>   }
>  
>   if(strncmp(rpath, root, rootlen) != 0) {
> @@ -201,11 +207,14 @@ static int query_fileowner(alpm_list_t *targets)
>   rel_path = rpath + rootlen;
>  
>   if((is_missing && is_dir) || (!is_missing && (is_dir = S_ISDIR(buf.st_mode)))) {
> - size_t rlen = strlen(rpath);
>   if(rlen + 2 >= PATH_MAX) {
>   pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath);
>   goto targcleanup;
>   }
> + if ((rpathsave = realloc(rpath, rlen + 2)) == NULL) {
> + goto targcleanup;
> + }
> + rpath = rpathsave;
>   strcat(rpath + rlen, "/");
>   }
>  
> --
> 2.19.0

Good catch, but this approach allows lrealpath to allocate a buffer
larger than PATH_MAX only to error if it actually does.  lrealpath is
intended to be the same as realpath (aside from not resolving symlinks
obviously), so it should be fixed so that it doesn't write more than
PATH_MAX into a provided buffer.

We could switch to dynamic allocation in addition to fixing lrealpath,
but then the PATH_MAX checks would be pointless and should be removed.
You also never free the malloc'd path.  You can run the entire test
suite under valgrind with `make PY_TEST_FLAGS=--valgrind check` to
catch most memory leaks.

apg
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] pacman: fix possible buffer overflow

morganamilo
On Sat, 22 Sep 2018 at 22:20, Andrew Gregory <[hidden email]> wrote:
> Good catch, but this approach allows lrealpath to allocate a buffer
> larger than PATH_MAX only to error if it actually does.  lrealpath is
> intended to be the same as realpath (aside from not resolving symlinks
> obviously), so it should be fixed so that it doesn't write more than
> PATH_MAX into a provided buffer.

I tried this initially, not really being that familiar with C I
couldn't really figure it out. How exactly would one communicate the
error? I can return null but then that could be anything. I could also
move the alpm_log_error into lrealpath but just seems messy. Maybe
just truncate and don't error?

> We could switch to dynamic allocation in addition to fixing lrealpath,
> but then the PATH_MAX checks would be pointless and should be removed.

I assumed PATH_MAX was an OS limit type of thing, so even if we had a
bigger buffer it would be useless as it would not work properly with
other functions. I'm guessing that's not the case?

> You also never free the malloc'd path.  You can run the entire test
> suite under valgrind with `make PY_TEST_FLAGS=--valgrind check` to
> catch most memory leaks.
>
> apg

I did free it at some point. Must have accidentally checked it out at
some point. Whoops
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] pacman: fix possible buffer overflow

Andrew Gregory
On 09/22/18 at 10:46pm, Morgan Adamiec wrote:

> On Sat, 22 Sep 2018 at 22:20, Andrew Gregory <[hidden email]> wrote:
> > Good catch, but this approach allows lrealpath to allocate a buffer
> > larger than PATH_MAX only to error if it actually does.  lrealpath is
> > intended to be the same as realpath (aside from not resolving symlinks
> > obviously), so it should be fixed so that it doesn't write more than
> > PATH_MAX into a provided buffer.
>
> I tried this initially, not really being that familiar with C I
> couldn't really figure it out. How exactly would one communicate the
> error? I can return null but then that could be anything. I could also
> move the alpm_log_error into lrealpath but just seems messy. Maybe
> just truncate and don't error?

Set errno to ENAMETOOLONG and return NULL, just like realpath.

> > We could switch to dynamic allocation in addition to fixing lrealpath,
> > but then the PATH_MAX checks would be pointless and should be removed.
>
> I assumed PATH_MAX was an OS limit type of thing, so even if we had a
> bigger buffer it would be useless as it would not work properly with
> other functions. I'm guessing that's not the case?

Sort of.  Path limitations are pretty messy, but yes, some functions
will not work correctly with paths longer than PATH_MAX.  We don't
actually use the path after resolving it though; we just do a partial
comparison against paths stored in pacman's database.

> > You also never free the malloc'd path.  You can run the entire test
> > suite under valgrind with `make PY_TEST_FLAGS=--valgrind check` to
> > catch most memory leaks.
>
> I did free it at some point. Must have accidentally checked it out at
> some point. Whoops
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] pacman: fix possible buffer overflow

morganamilo
On Sat, 22 Sep 2018 at 22:57, Andrew Gregory <[hidden email]> wrote:
> Set errno to ENAMETOOLONG and return NULL, just like realpath.

The problem still remains. You can input a filename that's < PATH_MAX
and have it resolve to something > PATH_MAX. You have no way to print
what that resolved path was. You can print the original file name but
that could be misleading.

Although I guess the chances of any one running into that is pretty
tiny. So the solution might just be to not care and print the original
filename.

I'll make a patch for it, see what people think.
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] pacman: fix possible buffer overflow

morganamilo
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 <[hidden email]>

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);
--
2.19.0
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] pacman: fix possible buffer overflow

Andrew Gregory
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 <[hidden email]>
>
> 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