Message ID | 20220113055920.3155918-1-siddhesh@sourceware.org |
---|---|
State | New |
Headers | show |
Series | [v2] realpath: Set errno to ENAMETOOLONG for result larger than PATH_MAX [BZ #28770] | expand |
On 13/01/2022 02:59, Siddhesh Poyarekar via Libc-alpha wrote: > realpath returns an allocated string when the result exceeds PATH_MAX, > which is unexpected when its second argument is not NULL. This results > in the second argument (resolved) being uninitialized and also results > in a memory leak since the caller expects resolved to be the same as the > returned value. > > Return NULL and set errno to ENAMETOOLONG if the result exceeds > PATH_MAX. This fixes [BZ #28770], which is CVE-2021-3998. > > Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> CCing Paul because I believe he might want to fix it on gnulib as well. Looks good, it just need to fix the Hurd build below. > --- > stdlib/Makefile | 1 + > stdlib/canonicalize.c | 12 ++++- > stdlib/tst-realpath-toolong.c | 84 +++++++++++++++++++++++++++++++++++ > 3 files changed, 95 insertions(+), 2 deletions(-) > create mode 100644 stdlib/tst-realpath-toolong.c > > diff --git a/stdlib/Makefile b/stdlib/Makefile > index 52e4d8cf19..897d4ea79d 100644 > --- a/stdlib/Makefile > +++ b/stdlib/Makefile > @@ -108,6 +108,7 @@ tests := bug-fmtmsg1 \ > tst-random \ > tst-random2 \ > tst-realpath \ > + tst-realpath-toolong \ > tst-secure-getenv \ > tst-setcontext \ > tst-setcontext2 \ Ok. > diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c > index f36bdf4c76..732dc7ea46 100644 > --- a/stdlib/canonicalize.c > +++ b/stdlib/canonicalize.c > @@ -400,8 +400,16 @@ realpath_stk (const char *name, char *resolved, > > error: > *dest++ = '\0'; > - if (resolved != NULL && dest - rname <= get_path_max ()) > - rname = strcpy (resolved, rname); > + if (resolved != NULL) > + { > + if (dest - rname <= get_path_max ()) > + rname = strcpy (resolved, rname); > + else > + { > + failed = true; > + __set_errno (ENAMETOOLONG); > + } > + } > > error_nomem: > scratch_buffer_free (&extra_buffer); Ok. > diff --git a/stdlib/tst-realpath-toolong.c b/stdlib/tst-realpath-toolong.c > new file mode 100644 > index 0000000000..71c0829a80 > --- /dev/null > +++ b/stdlib/tst-realpath-toolong.c > @@ -0,0 +1,84 @@ > +/* Verify that realpath returns NULL with ENAMETOOLONG if the result exceeds > + NAME_MAX. > + Copyright The GNU Toolchain Authors. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <limits.h> > +#include <stdlib.h> > +#include <string.h> > +#include <unistd.h> > +#include <support/check.h> > +#include <support/temp_file.h> > +#include <sys/types.h> > +#include <sys/stat.h> > + > +#define BASENAME "tst-realpath-toolong." > +size_t nest_count; > +char dir[NAME_MAX + 1]; > +char *base; > + > +static void > +delete_subdirectories (const char *name) > +{ > + TEST_VERIFY (chdir (name) == 0); > + if (--nest_count > 0) > + delete_subdirectories (dir); > + TEST_VERIFY (chdir ("..") == 0); > + TEST_VERIFY (rmdir (name) == 0); > +} > + > +static void > +do_cleanup (void) > +{ > + TEST_VERIFY (chdir (base) == 0); > + if (nest_count > 0) > + delete_subdirectories (dir); > +} > + > +int > +do_test (void) > +{ > + base = support_create_temp_directory (BASENAME); > + > + TEST_VERIFY_EXIT (chdir (base) == 0); > + > + memset (dir, 'X', sizeof (dir) - 1); > + dir[NAME_MAX] = '\0'; > + > + /* Create directories and descend into them so that the final path is larger > + than PATH_MAX. */ > + for (nest_count = 0; nest_count <= PATH_MAX / sizeof (dir); nest_count++) This fails to build on Hurd, so I am not sure if it would be better to just set it as UNSUPPORTED in this case or use the assumption on realpath: #ifdef PATH_MAX size_t path_max = PATH_MAX; #else size_t path_max = pathconf ("/", _PC_PATH_MAX); path_max = path_max < 0 ? 1024 : path_max < ? path_max : PTRDIFF_MAX; #endif > + { > + TEST_VERIFY_EXIT (mkdir (dir, S_IRWXU) == 0); > + TEST_VERIFY_EXIT (chdir (dir) == 0); > + } > + > + char buf[PATH_MAX + 1]; > + const char * const res = realpath (".", buf); > + > + /* canonicalize.c states that if the real path is >= PATH_MAX, then > + realpath returns NULL and sets ENAMETOOLONG. */ > + TEST_VERIFY_EXIT (res == NULL && errno == ENAMETOOLONG); > + > + do_cleanup (); > + free (base); > + return 0; > +} > + > +#define CLEANUP_HANDLER do_cleanup > +#include <support/test-driver.c>
On 13/01/2022 19:30, Adhemerval Zanella wrote: > > > On 13/01/2022 02:59, Siddhesh Poyarekar via Libc-alpha wrote: >> realpath returns an allocated string when the result exceeds PATH_MAX, >> which is unexpected when its second argument is not NULL. This results >> in the second argument (resolved) being uninitialized and also results >> in a memory leak since the caller expects resolved to be the same as the >> returned value. >> >> Return NULL and set errno to ENAMETOOLONG if the result exceeds >> PATH_MAX. This fixes [BZ #28770], which is CVE-2021-3998. >> >> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > > CCing Paul because I believe he might want to fix it on gnulib as well. > > Looks good, it just need to fix the Hurd build below. > >> --- >> stdlib/Makefile | 1 + >> stdlib/canonicalize.c | 12 ++++- >> stdlib/tst-realpath-toolong.c | 84 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 95 insertions(+), 2 deletions(-) >> create mode 100644 stdlib/tst-realpath-toolong.c >> >> diff --git a/stdlib/Makefile b/stdlib/Makefile >> index 52e4d8cf19..897d4ea79d 100644 >> --- a/stdlib/Makefile >> +++ b/stdlib/Makefile >> @@ -108,6 +108,7 @@ tests := bug-fmtmsg1 \ >> tst-random \ >> tst-random2 \ >> tst-realpath \ >> + tst-realpath-toolong \ >> tst-secure-getenv \ >> tst-setcontext \ >> tst-setcontext2 \ > > Ok. > >> diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c >> index f36bdf4c76..732dc7ea46 100644 >> --- a/stdlib/canonicalize.c >> +++ b/stdlib/canonicalize.c >> @@ -400,8 +400,16 @@ realpath_stk (const char *name, char *resolved, >> >> error: >> *dest++ = '\0'; >> - if (resolved != NULL && dest - rname <= get_path_max ()) >> - rname = strcpy (resolved, rname); >> + if (resolved != NULL) >> + { >> + if (dest - rname <= get_path_max ()) >> + rname = strcpy (resolved, rname); >> + else >> + { >> + failed = true; >> + __set_errno (ENAMETOOLONG); >> + } >> + } >> >> error_nomem: >> scratch_buffer_free (&extra_buffer); > > Ok. > >> diff --git a/stdlib/tst-realpath-toolong.c b/stdlib/tst-realpath-toolong.c >> new file mode 100644 >> index 0000000000..71c0829a80 >> --- /dev/null >> +++ b/stdlib/tst-realpath-toolong.c >> @@ -0,0 +1,84 @@ >> +/* Verify that realpath returns NULL with ENAMETOOLONG if the result exceeds >> + NAME_MAX. >> + Copyright The GNU Toolchain Authors. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + <https://www.gnu.org/licenses/>. */ >> + >> +#include <errno.h> >> +#include <limits.h> >> +#include <stdlib.h> >> +#include <string.h> >> +#include <unistd.h> >> +#include <support/check.h> >> +#include <support/temp_file.h> >> +#include <sys/types.h> >> +#include <sys/stat.h> >> + >> +#define BASENAME "tst-realpath-toolong." >> +size_t nest_count; >> +char dir[NAME_MAX + 1]; >> +char *base; >> + >> +static void >> +delete_subdirectories (const char *name) >> +{ >> + TEST_VERIFY (chdir (name) == 0); >> + if (--nest_count > 0) >> + delete_subdirectories (dir); >> + TEST_VERIFY (chdir ("..") == 0); >> + TEST_VERIFY (rmdir (name) == 0); >> +} >> + >> +static void >> +do_cleanup (void) >> +{ >> + TEST_VERIFY (chdir (base) == 0); >> + if (nest_count > 0) >> + delete_subdirectories (dir); >> +} >> + >> +int >> +do_test (void) >> +{ >> + base = support_create_temp_directory (BASENAME); >> + >> + TEST_VERIFY_EXIT (chdir (base) == 0); >> + >> + memset (dir, 'X', sizeof (dir) - 1); >> + dir[NAME_MAX] = '\0'; >> + >> + /* Create directories and descend into them so that the final path is larger >> + than PATH_MAX. */ >> + for (nest_count = 0; nest_count <= PATH_MAX / sizeof (dir); nest_count++) > > This fails to build on Hurd, so I am not sure if it would be better to just > set it as UNSUPPORTED in this case or use the assumption on realpath: > > #ifdef PATH_MAX > size_t path_max = PATH_MAX; > #else > size_t path_max = pathconf ("/", _PC_PATH_MAX); > path_max = path_max < 0 ? 1024 : path_max < ? path_max : PTRDIFF_MAX; > #endif Ahh yes, I'll fix up the test for hurd using this snippet. Thanks, Siddhesh
diff --git a/stdlib/Makefile b/stdlib/Makefile index 52e4d8cf19..897d4ea79d 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -108,6 +108,7 @@ tests := bug-fmtmsg1 \ tst-random \ tst-random2 \ tst-realpath \ + tst-realpath-toolong \ tst-secure-getenv \ tst-setcontext \ tst-setcontext2 \ diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c index f36bdf4c76..732dc7ea46 100644 --- a/stdlib/canonicalize.c +++ b/stdlib/canonicalize.c @@ -400,8 +400,16 @@ realpath_stk (const char *name, char *resolved, error: *dest++ = '\0'; - if (resolved != NULL && dest - rname <= get_path_max ()) - rname = strcpy (resolved, rname); + if (resolved != NULL) + { + if (dest - rname <= get_path_max ()) + rname = strcpy (resolved, rname); + else + { + failed = true; + __set_errno (ENAMETOOLONG); + } + } error_nomem: scratch_buffer_free (&extra_buffer); diff --git a/stdlib/tst-realpath-toolong.c b/stdlib/tst-realpath-toolong.c new file mode 100644 index 0000000000..71c0829a80 --- /dev/null +++ b/stdlib/tst-realpath-toolong.c @@ -0,0 +1,84 @@ +/* Verify that realpath returns NULL with ENAMETOOLONG if the result exceeds + NAME_MAX. + Copyright The GNU Toolchain Authors. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <limits.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <support/check.h> +#include <support/temp_file.h> +#include <sys/types.h> +#include <sys/stat.h> + +#define BASENAME "tst-realpath-toolong." +size_t nest_count; +char dir[NAME_MAX + 1]; +char *base; + +static void +delete_subdirectories (const char *name) +{ + TEST_VERIFY (chdir (name) == 0); + if (--nest_count > 0) + delete_subdirectories (dir); + TEST_VERIFY (chdir ("..") == 0); + TEST_VERIFY (rmdir (name) == 0); +} + +static void +do_cleanup (void) +{ + TEST_VERIFY (chdir (base) == 0); + if (nest_count > 0) + delete_subdirectories (dir); +} + +int +do_test (void) +{ + base = support_create_temp_directory (BASENAME); + + TEST_VERIFY_EXIT (chdir (base) == 0); + + memset (dir, 'X', sizeof (dir) - 1); + dir[NAME_MAX] = '\0'; + + /* Create directories and descend into them so that the final path is larger + than PATH_MAX. */ + for (nest_count = 0; nest_count <= PATH_MAX / sizeof (dir); nest_count++) + { + TEST_VERIFY_EXIT (mkdir (dir, S_IRWXU) == 0); + TEST_VERIFY_EXIT (chdir (dir) == 0); + } + + char buf[PATH_MAX + 1]; + const char * const res = realpath (".", buf); + + /* canonicalize.c states that if the real path is >= PATH_MAX, then + realpath returns NULL and sets ENAMETOOLONG. */ + TEST_VERIFY_EXIT (res == NULL && errno == ENAMETOOLONG); + + do_cleanup (); + free (base); + return 0; +} + +#define CLEANUP_HANDLER do_cleanup +#include <support/test-driver.c>
realpath returns an allocated string when the result exceeds PATH_MAX, which is unexpected when its second argument is not NULL. This results in the second argument (resolved) being uninitialized and also results in a memory leak since the caller expects resolved to be the same as the returned value. Return NULL and set errno to ENAMETOOLONG if the result exceeds PATH_MAX. This fixes [BZ #28770], which is CVE-2021-3998. Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> --- stdlib/Makefile | 1 + stdlib/canonicalize.c | 12 ++++- stdlib/tst-realpath-toolong.c | 84 +++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 stdlib/tst-realpath-toolong.c