Message ID | 20220120093252.1911498-3-siddhesh@sourceware.org |
---|---|
State | New |
Headers | show |
Series | Fixes for CVE-2021-3998 and CVE-2021-3999 | expand |
On Thu, 20 Jan 2022, Siddhesh Poyarekar via Libc-alpha wrote: > diff --git a/stdlib/tst-realpath-toolong.c b/stdlib/tst-realpath-toolong.c > + char buf[PATH_MAX + 1]; This has broken the testsuite build for Hurd. tst-realpath-toolong.c: In function 'do_test': tst-realpath-toolong.c:37:12: error: 'PATH_MAX' undeclared (first use in this function)
On 22/01/2022 04:52, Joseph Myers wrote: > On Thu, 20 Jan 2022, Siddhesh Poyarekar via Libc-alpha wrote: > >> diff --git a/stdlib/tst-realpath-toolong.c b/stdlib/tst-realpath-toolong.c > >> + char buf[PATH_MAX + 1]; > > This has broken the testsuite build for Hurd. > > tst-realpath-toolong.c: In function 'do_test': > tst-realpath-toolong.c:37:12: error: 'PATH_MAX' undeclared (first use in this function) > Sorry I missed that, I'll post a fix. I was getting a different build error on hurd which prevented me from seeing this. I'm rebuilding my build-many-glibcs to see if that fixes it. Thanks, Siddhesh
On Jan 20 2022, Siddhesh Poyarekar via Libc-alpha wrote: > 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); > + } > + } Shouldn't that preserve any preexisting error? I think the result should only be copied if !failed.
On 24/01/2022 19:15, Andreas Schwab wrote: > On Jan 20 2022, Siddhesh Poyarekar via Libc-alpha wrote: > >> 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); >> + } >> + } > > Shouldn't that preserve any preexisting error? like this? diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c index 732dc7ea46..6caed9e70e 100644 --- a/stdlib/canonicalize.c +++ b/stdlib/canonicalize.c @@ -404,7 +404,7 @@ error: { if (dest - rname <= get_path_max ()) rname = strcpy (resolved, rname); - else + else if (!failed) { failed = true; __set_errno (ENAMETOOLONG); > I think the result > should only be copied if !failed. > Looks like tests in test-canon.c depend on buf contents having being copied in despite failure. Siddhesh
On Jan 24 2022, Siddhesh Poyarekar wrote: > Looks like tests in test-canon.c depend on buf contents having being > copied in despite failure. That is not supported by POSIX.
On 24/01/2022 20:18, Andreas Schwab wrote: > On Jan 24 2022, Siddhesh Poyarekar wrote: > >> Looks like tests in test-canon.c depend on buf contents having being >> copied in despite failure. > > That is not supported by POSIX. > OK, I'll file a separate bug and fix for it then. Is the fix I posted sufficient for your other concern, i.e. not overriding preexisting error? Thanks, Siddhesh
On Jan 24 2022, Siddhesh Poyarekar wrote: > Is the fix I posted sufficient for your other concern, i.e. not > overriding preexisting error? Yes.
diff --git a/NEWS b/NEWS index 6ed9fa9787..4c392a445e 100644 --- a/NEWS +++ b/NEWS @@ -166,6 +166,10 @@ Security related changes: CVE-2022-23218: Passing an overlong file name to the svcunix_create legacy function could result in a stack-based buffer overflow. + CVE-2021-3998: Passing a path longer than PATH_MAX to the realpath + function could result in a memory leak and potential access of + uninitialized memory. Reported by Qualys. + The following bugs are resolved with this release: [The release manager will add the list generated by diff --git a/stdlib/Makefile b/stdlib/Makefile index 1e81f98fac..8236741984 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -109,6 +109,7 @@ tests := \ 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..8bed772460 --- /dev/null +++ b/stdlib/tst-realpath-toolong.c @@ -0,0 +1,49 @@ +/* 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." + +int +do_test (void) +{ + char *base = support_create_and_chdir_toolong_temp_directory (BASENAME); + + char buf[PATH_MAX + 1]; + const char *res = realpath (".", buf); + + /* canonicalize.c states that if the real path is >= PATH_MAX, then + realpath returns NULL and sets ENAMETOOLONG. */ + TEST_VERIFY (res == NULL); + TEST_VERIFY (errno == ENAMETOOLONG); + + free (base); + return 0; +} + +#include <support/test-driver.c>