[PATCH 0/3] Fixes for CVE-2021-3998 and CVE-2021-3999

[PATCH 0/3] Fixes for CVE-2021-3998 and CVE-2021-3999

[Siddhesh Poyarekar]

Add functions to make directory trees with paths longer than PATH_MAX
and use them to test fixes for CVE-2021-3998 and CVE-2021-3999.

Tested on x86_64.  Hurd build seems to already been broken, so I was
unable to test it.

Siddhesh Poyarekar (3):
  support: Add helpers to create paths longer than PATH_MAX
  realpath: Set errno to ENAMETOOLONG for result larger than PATH_MAX
    [BZ #28770]
  getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

 NEWS                                          |  10 +
 stdlib/Makefile                               |   1 +
 stdlib/canonicalize.c                         |  12 +-
 stdlib/tst-realpath-toolong.c                 |  48 ++++
 support/temp_file.c                           | 157 ++++++++++-
 support/temp_file.h                           |  11 +
 sysdeps/posix/getcwd.c                        |   7 +
 sysdeps/unix/sysv/linux/Makefile              |   7 +-
 sysdeps/unix/sysv/linux/getcwd.c              |   7 +
 .../unix/sysv/linux/tst-getcwd-smallbuff.c    | 245 ++++++++++++++++++
 10 files changed, 494 insertions(+), 11 deletions(-)
 create mode 100644 stdlib/tst-realpath-toolong.c
 create mode 100644 sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c

-- 
2.34.1

[PATCH 1/3] support: Add helpers to create paths longer than PATH_MAX

[Siddhesh Poyarekar]

Add new helpers support_create_and_chdir_toolong_temp_directory and
support_chdir_toolong_temp_directory to create and descend into
directory trees longer than PATH_MAX.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 support/temp_file.c | 157 +++++++++++++++++++++++++++++++++++++++++---
 support/temp_file.h |  11 ++++
 2 files changed, 160 insertions(+), 8 deletions(-)

diff --git a/support/temp_file.c b/support/temp_file.c
index e7bb8aadb9..c3e83912ac 100644
--- a/support/temp_file.c
+++ b/support/temp_file.c
@@ -36,14 +36,31 @@ static struct temp_name_list
   struct temp_name_list *next;
   char *name;
   pid_t owner;
+  bool toolong;
 } *temp_name_list;
 
 /* Location of the temporary files.  Set by the test skeleton via
    support_set_test_dir.  The string is not be freed.  */
 static const char *test_dir = _PATH_TMP;
 
-void
-add_temp_file (const char *name)
+/* Name of subdirectories in a too long temporary directory tree.  */
+static char toolong_subdir[NAME_MAX + 1];
+
+/* Return the maximum size of path on the target.  */
+static inline size_t
+get_path_max (void)
+{
+#ifdef PATH_MAX
+  return PATH_MAX;
+#else
+  size_t path_max = pathconf ("/", _PC_PATH_MAX);
+  return (path_max < 0 ? 1024
+	  : path_max <= PTRDIFF_MAX ? path_max : PTRDIFF_MAX);
+#endif
+}
+
+static void
+add_temp_file_internal (const char *name, bool toolong)
 {
   struct temp_name_list *newp
     = (struct temp_name_list *) xcalloc (sizeof (*newp), 1);
@@ -53,12 +70,19 @@ add_temp_file (const char *name)
       newp->name = newname;
       newp->next = temp_name_list;
       newp->owner = getpid ();
+      newp->toolong = toolong;
       temp_name_list = newp;
     }
   else
     free (newp);
 }
 
+void
+add_temp_file (const char *name)
+{
+  add_temp_file_internal (name, false);
+}
+
 int
 create_temp_file_in_dir (const char *base, const char *dir, char **filename)
 {
@@ -90,8 +114,8 @@ create_temp_file (const char *base, char **filename)
   return create_temp_file_in_dir (base, test_dir, filename);
 }
 
-char *
-support_create_temp_directory (const char *base)
+static char *
+create_temp_directory_internal (const char *base, bool toolong)
 {
   char *path = xasprintf ("%s/%sXXXXXX", test_dir, base);
   if (mkdtemp (path) == NULL)
@@ -99,16 +123,124 @@ support_create_temp_directory (const char *base)
       printf ("error: mkdtemp (\"%s\"): %m", path);
       exit (1);
     }
-  add_temp_file (path);
+  add_temp_file_internal (path, toolong);
   return path;
 }
 
-/* Helper functions called by the test skeleton follow.  */
+char *
+support_create_temp_directory (const char *base)
+{
+  return create_temp_directory_internal (base, false);
+}
+
+static void
+ensure_toolong_subdir_initialized (void)
+{
+  for (size_t i = 0; i < NAME_MAX; i++)
+    if (toolong_subdir[i] != 'X')
+      {
+	printf ("uninitialized toolong directory tree\n");
+	exit (1);
+      }
+}
+
+char *
+support_create_and_chdir_toolong_temp_directory (const char *basename)
+{
+  size_t path_max = get_path_max ();
+
+  char *base = create_temp_directory_internal (basename, true);
+
+  if (chdir (base) != 0)
+    {
+      printf ("error creating toolong base: chdir (\"%s\"): %m", base);
+      exit (1);
+    }
+
+  memset (toolong_subdir, 'X', sizeof (toolong_subdir) - 1);
+  toolong_subdir[NAME_MAX] = '\0';
+
+  /* Create directories and descend into them so that the final path is larger
+     than PATH_MAX.  */
+  for (size_t i = 0; i <= path_max / sizeof (toolong_subdir); i++)
+    {
+      if (mkdir (toolong_subdir, S_IRWXU) != 0)
+	{
+	  printf ("error creating toolong subdir: mkdir (\"%s\"): %m",
+		  toolong_subdir);
+	  exit (1);
+	}
+      if (chdir (toolong_subdir) != 0)
+	{
+	  printf ("error creating toolong subdir: chdir (\"%s\"): %m",
+		  toolong_subdir);
+	  exit (1);
+	}
+    }
+  return base;
+}
 
 void
-support_set_test_dir (const char *path)
+support_chdir_toolong_temp_directory (const char *base)
 {
-  test_dir = path;
+  size_t path_max = get_path_max ();
+  ensure_toolong_subdir_initialized ();
+
+  if (chdir (base) != 0)
+    {
+      printf ("error: chdir (\"%s\"): %m", base);
+      exit (1);
+    }
+
+  for (size_t i = 0; i <= path_max / sizeof (toolong_subdir); i++)
+    if (chdir (toolong_subdir) != 0)
+      {
+	printf ("error subdir: chdir (\"%s\"): %m", toolong_subdir);
+	exit (1);
+      }
+}
+
+/* Helper functions called by the test skeleton follow.  */
+
+static void
+remove_toolong_subdirs (const char *base)
+{
+  size_t path_max = get_path_max ();
+
+  ensure_toolong_subdir_initialized ();
+
+  if (chdir (base) != 0)
+    {
+      printf ("warning: toolong cleanup base failed: chdir (\"%s\"): %m\n",
+	      base);
+      return;
+    }
+
+  /* Descend.  */
+  int levels = 0;
+  for (levels = 0; levels <= path_max / sizeof (toolong_subdir); levels++)
+    if (chdir (toolong_subdir) != 0)
+      {
+	printf ("warning: toolong cleanup failed: chdir (\"%s\"): %m\n",
+		toolong_subdir);
+	return;
+      }
+
+  /* Ascend and remove.  */
+  while (--levels >= 0)
+    {
+      if (chdir ("..") != 0)
+	{
+	  printf ("warning: toolong cleanup failed: chdir (\"..\"): %m\n");
+	  return;
+	}
+      if (remove (toolong_subdir) != 0)
+	{
+	  printf ("warning: could not remove subdirectory: %s: %m\n",
+		  toolong_subdir);
+	  return;
+	}
+    }
 }
 
 void
@@ -123,6 +255,9 @@ support_delete_temp_files (void)
 	 around, to prevent PID reuse.)  */
       if (temp_name_list->owner == pid)
 	{
+	  if (temp_name_list->toolong)
+	    remove_toolong_subdirs (temp_name_list->name);
+
 	  if (remove (temp_name_list->name) != 0)
 	    printf ("warning: could not remove temporary file: %s: %m\n",
 		    temp_name_list->name);
@@ -147,3 +282,9 @@ support_print_temp_files (FILE *f)
       fprintf (f, ")\n");
     }
 }
+
+void
+support_set_test_dir (const char *path)
+{
+  test_dir = path;
+}
diff --git a/support/temp_file.h b/support/temp_file.h
index 50a443abe4..de01fbbceb 100644
--- a/support/temp_file.h
+++ b/support/temp_file.h
@@ -19,6 +19,8 @@
 #ifndef SUPPORT_TEMP_FILE_H
 #define SUPPORT_TEMP_FILE_H
 
+#include <limits.h>
+#include <unistd.h>
 #include <sys/cdefs.h>
 
 __BEGIN_DECLS
@@ -44,6 +46,15 @@ int create_temp_file_in_dir (const char *base, const char *dir,
    returns.  The caller should free this string.  */
 char *support_create_temp_directory (const char *base);
 
+/* Create a temporary directory tree that is longer than PATH_MAX and schedule
+   it for deletion.  BASENAME is used as a prefix for the unique directory
+   name, which the function returns.  The caller should free this string.  */
+char *support_create_and_chdir_toolong_temp_directory (const char *basename);
+
+/* Change into the innermost directory of the directory tree BASE, which was
+   created using support_create_and_chdir_toolong_temp_directory.  */
+void support_chdir_toolong_temp_directory (const char *base);
+
 __END_DECLS
 
 #endif /* SUPPORT_TEMP_FILE_H */
-- 
2.34.1

Re: [PATCH 1/3] support: Add helpers to create paths longer than PATH_MAX

[Adhemerval Zanella]

On 18/01/2022 06:07, Siddhesh Poyarekar wrote:
> Add new helpers support_create_and_chdir_toolong_temp_directory and
> support_chdir_toolong_temp_directory to create and descend into
> directory trees longer than PATH_MAX.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> ---
>  support/temp_file.c | 157 +++++++++++++++++++++++++++++++++++++++++---
>  support/temp_file.h |  11 ++++
>  2 files changed, 160 insertions(+), 8 deletions(-)
> 
> diff --git a/support/temp_file.c b/support/temp_file.c
> index e7bb8aadb9..c3e83912ac 100644
> --- a/support/temp_file.c
> +++ b/support/temp_file.c
> @@ -36,14 +36,31 @@ static struct temp_name_list
>    struct temp_name_list *next;
>    char *name;
>    pid_t owner;
> +  bool toolong;
>  } *temp_name_list;
>  
>  /* Location of the temporary files.  Set by the test skeleton via
>     support_set_test_dir.  The string is not be freed.  */
>  static const char *test_dir = _PATH_TMP;
>  
> -void
> -add_temp_file (const char *name)
> +/* Name of subdirectories in a too long temporary directory tree.  */
> +static char toolong_subdir[NAME_MAX + 1];
> +
> +/* Return the maximum size of path on the target.  */
> +static inline size_t
> +get_path_max (void)
> +{
> +#ifdef PATH_MAX
> +  return PATH_MAX;
> +#else
> +  size_t path_max = pathconf ("/", _PC_PATH_MAX);
> +  return (path_max < 0 ? 1024
> +	  : path_max <= PTRDIFF_MAX ? path_max : PTRDIFF_MAX);
> +#endif
> +}
> +
> +static void
> +add_temp_file_internal (const char *name, bool toolong)
>  {
>    struct temp_name_list *newp
>      = (struct temp_name_list *) xcalloc (sizeof (*newp), 1);
> @@ -53,12 +70,19 @@ add_temp_file (const char *name)
>        newp->name = newname;
>        newp->next = temp_name_list;
>        newp->owner = getpid ();
> +      newp->toolong = toolong;
>        temp_name_list = newp;
>      }
>    else
>      free (newp);
>  }
>  
> +void
> +add_temp_file (const char *name)
> +{
> +  add_temp_file_internal (name, false);
> +}
> +
>  int
>  create_temp_file_in_dir (const char *base, const char *dir, char **filename)
>  {
> @@ -90,8 +114,8 @@ create_temp_file (const char *base, char **filename)
>    return create_temp_file_in_dir (base, test_dir, filename);
>  }
>  
> -char *
> -support_create_temp_directory (const char *base)
> +static char *
> +create_temp_directory_internal (const char *base, bool toolong)
>  {
>    char *path = xasprintf ("%s/%sXXXXXX", test_dir, base);
>    if (mkdtemp (path) == NULL)
> @@ -99,16 +123,124 @@ support_create_temp_directory (const char *base)
>        printf ("error: mkdtemp (\"%s\"): %m", path);
>        exit (1);
>      }
> -  add_temp_file (path);
> +  add_temp_file_internal (path, toolong);
>    return path;
>  }
>  
> -/* Helper functions called by the test skeleton follow.  */
> +char *
> +support_create_temp_directory (const char *base)
> +{
> +  return create_temp_directory_internal (base, false);
> +}
> +
> +static void
> +ensure_toolong_subdir_initialized (void)
> +{
> +  for (size_t i = 0; i < NAME_MAX; i++)
> +    if (toolong_subdir[i] != 'X')
> +      {
> +	printf ("uninitialized toolong directory tree\n");
> +	exit (1);
> +      }
> +}

Maybe use FAIL_EXIT1 here?

> +
> +char *
> +support_create_and_chdir_toolong_temp_directory (const char *basename)
> +{
> +  size_t path_max = get_path_max ();
> +
> +  char *base = create_temp_directory_internal (basename, true);
> +
> +  if (chdir (base) != 0)
> +    {
> +      printf ("error creating toolong base: chdir (\"%s\"): %m", base);
> +      exit (1);
> +    }
> +

Use xchdir. 

> +  memset (toolong_subdir, 'X', sizeof (toolong_subdir) - 1);
> +  toolong_subdir[NAME_MAX] = '\0';
> +
> +  /* Create directories and descend into them so that the final path is larger
> +     than PATH_MAX.  */
> +  for (size_t i = 0; i <= path_max / sizeof (toolong_subdir); i++)
> +    {
> +      if (mkdir (toolong_subdir, S_IRWXU) != 0)
> +	{
> +	  printf ("error creating toolong subdir: mkdir (\"%s\"): %m",
> +		  toolong_subdir);
> +	  exit (1);
> +	}

Use xmkdir.

> +      if (chdir (toolong_subdir) != 0)


Use xchdir. 

> +	{
> +	  printf ("error creating toolong subdir: chdir (\"%s\"): %m",
> +		  toolong_subdir);
> +	  exit (1);
> +	}
> +    }
> +  return base;
> +}
>  
>  void
> -support_set_test_dir (const char *path)
> +support_chdir_toolong_temp_directory (const char *base)
>  {
> -  test_dir = path;
> +  size_t path_max = get_path_max ();
> +  ensure_toolong_subdir_initialized ();
> +
> +  if (chdir (base) != 0)
> +    {
> +      printf ("error: chdir (\"%s\"): %m", base);
> +      exit (1);
> +    }
> +
> +  for (size_t i = 0; i <= path_max / sizeof (toolong_subdir); i++)
> +    if (chdir (toolong_subdir) != 0)

Use xchdir.

> +      {
> +	printf ("error subdir: chdir (\"%s\"): %m", toolong_subdir);
> +	exit (1);
> +      }
> +}
> +
> +/* Helper functions called by the test skeleton follow.  */
> +
> +static void
> +remove_toolong_subdirs (const char *base)
> +{
> +  size_t path_max = get_path_max ();
> +
> +  ensure_toolong_subdir_initialized ();
> +
> +  if (chdir (base) != 0)
> +    {
> +      printf ("warning: toolong cleanup base failed: chdir (\"%s\"): %m\n",
> +	      base);
> +      return;
> +    }
> +
> +  /* Descend.  */
> +  int levels = 0;
> +  for (levels = 0; levels <= path_max / sizeof (toolong_subdir); levels++)
> +    if (chdir (toolong_subdir) != 0)
> +      {
> +	printf ("warning: toolong cleanup failed: chdir (\"%s\"): %m\n",
> +		toolong_subdir);
> +	return;
> +      }
> +
> +  /* Ascend and remove.  */
> +  while (--levels >= 0)
> +    {
> +      if (chdir ("..") != 0)
> +	{
> +	  printf ("warning: toolong cleanup failed: chdir (\"..\"): %m\n");
> +	  return;
> +	}
> +      if (remove (toolong_subdir) != 0)
> +	{
> +	  printf ("warning: could not remove subdirectory: %s: %m\n",
> +		  toolong_subdir);
> +	  return;
> +	}
> +    }
>  }

Throwing an warning should be fine in such cases.

>  
>  void
> @@ -123,6 +255,9 @@ support_delete_temp_files (void)
>  	 around, to prevent PID reuse.)  */
>        if (temp_name_list->owner == pid)
>  	{
> +	  if (temp_name_list->toolong)
> +	    remove_toolong_subdirs (temp_name_list->name);
> +
>  	  if (remove (temp_name_list->name) != 0)
>  	    printf ("warning: could not remove temporary file: %s: %m\n",
>  		    temp_name_list->name);
> @@ -147,3 +282,9 @@ support_print_temp_files (FILE *f)
>        fprintf (f, ")\n");
>      }
>  }
> +
> +void
> +support_set_test_dir (const char *path)
> +{
> +  test_dir = path;
> +}
> diff --git a/support/temp_file.h b/support/temp_file.h
> index 50a443abe4..de01fbbceb 100644
> --- a/support/temp_file.h
> +++ b/support/temp_file.h
> @@ -19,6 +19,8 @@
>  #ifndef SUPPORT_TEMP_FILE_H
>  #define SUPPORT_TEMP_FILE_H
>  
> +#include <limits.h>
> +#include <unistd.h>
>  #include <sys/cdefs.h>
>  
>  __BEGIN_DECLS
> @@ -44,6 +46,15 @@ int create_temp_file_in_dir (const char *base, const char *dir,
>     returns.  The caller should free this string.  */
>  char *support_create_temp_directory (const char *base);
>  
> +/* Create a temporary directory tree that is longer than PATH_MAX and schedule
> +   it for deletion.  BASENAME is used as a prefix for the unique directory
> +   name, which the function returns.  The caller should free this string.  */
> +char *support_create_and_chdir_toolong_temp_directory (const char *basename);
> +
> +/* Change into the innermost directory of the directory tree BASE, which was
> +   created using support_create_and_chdir_toolong_temp_directory.  */
> +void support_chdir_toolong_temp_directory (const char *base);
> +
>  __END_DECLS
>  
>  #endif /* SUPPORT_TEMP_FILE_H */

Re: [PATCH 1/3] support: Add helpers to create paths longer than PATH_MAX

[Siddhesh Poyarekar]

On 19/01/2022 01:12, Adhemerval Zanella wrote:
>> +      {
>> +	printf ("error subdir: chdir (\"%s\"): %m", toolong_subdir);
>> +	exit (1);
>> +      }
>> +}
>> +
>> +/* Helper functions called by the test skeleton follow.  */
>> +
>> +static void
>> +remove_toolong_subdirs (const char *base)
>> +{
>> +  size_t path_max = get_path_max ();
>> +
>> +  ensure_toolong_subdir_initialized ();
>> +
>> +  if (chdir (base) != 0)
>> +    {
>> +      printf ("warning: toolong cleanup base failed: chdir (\"%s\"): %m\n",
>> +	      base);
>> +      return;
>> +    }
>> +
>> +  /* Descend.  */
>> +  int levels = 0;
>> +  for (levels = 0; levels <= path_max / sizeof (toolong_subdir); levels++)
>> +    if (chdir (toolong_subdir) != 0)
>> +      {
>> +	printf ("warning: toolong cleanup failed: chdir (\"%s\"): %m\n",
>> +		toolong_subdir);
>> +	return;
>> +      }
>> +
>> +  /* Ascend and remove.  */
>> +  while (--levels >= 0)
>> +    {
>> +      if (chdir ("..") != 0)
>> +	{
>> +	  printf ("warning: toolong cleanup failed: chdir (\"..\"): %m\n");
>> +	  return;
>> +	}
>> +      if (remove (toolong_subdir) != 0)
>> +	{
>> +	  printf ("warning: could not remove subdirectory: %s: %m\n",
>> +		  toolong_subdir);
>> +	  return;
>> +	}
>> +    }
>>   }
> 
> Throwing an warning should be fine in such cases.
> 

You mean by using the warning() function?  It looks like printf is the 
common way to do this in support/*

Thanks,
Siddhesh

[PATCH 2/3] realpath: Set errno to ENAMETOOLONG for result larger than PATH_MAX [BZ #28770]

[Siddhesh Poyarekar]

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>
---
 NEWS                          |  4 +++
 stdlib/Makefile               |  1 +
 stdlib/canonicalize.c         | 12 +++++++--
 stdlib/tst-realpath-toolong.c | 48 +++++++++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+), 2 deletions(-)
 create mode 100644 stdlib/tst-realpath-toolong.c

diff --git a/NEWS b/NEWS
index 38802f0673..5c63cef156 100644
--- a/NEWS
+++ b/NEWS
@@ -163,6 +163,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.
+
 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..f10653f05b
--- /dev/null
+++ b/stdlib/tst-realpath-toolong.c
@@ -0,0 +1,48 @@
+/* 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 * 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);
+
+  free (base);
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.34.1

Re: [PATCH 2/3] realpath: Set errno to ENAMETOOLONG for result larger than PATH_MAX [BZ #28770]

[Adhemerval Zanella]

On 18/01/2022 06:07, Siddhesh Poyarekar 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>

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  NEWS                          |  4 +++
>  stdlib/Makefile               |  1 +
>  stdlib/canonicalize.c         | 12 +++++++--
>  stdlib/tst-realpath-toolong.c | 48 +++++++++++++++++++++++++++++++++++
>  4 files changed, 63 insertions(+), 2 deletions(-)
>  create mode 100644 stdlib/tst-realpath-toolong.c
> 
> diff --git a/NEWS b/NEWS
> index 38802f0673..5c63cef156 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -163,6 +163,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.
> +
>  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..f10653f05b
> --- /dev/null
> +++ b/stdlib/tst-realpath-toolong.c
> @@ -0,0 +1,48 @@
> +/* 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 * 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);
> +
> +  free (base);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

[PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Siddhesh Poyarekar]

No valid path returned by getcwd would fit into 1 byte, so reject the
size early and return NULL with errno set to ERANGE.

This resolves BZ #28769.

Signed-off-by: Qualys Security Advisory <qsa@qualys.com>
Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 NEWS                                          |   6 +
 sysdeps/posix/getcwd.c                        |   7 +
 sysdeps/unix/sysv/linux/Makefile              |   7 +-
 sysdeps/unix/sysv/linux/getcwd.c              |   7 +
 .../unix/sysv/linux/tst-getcwd-smallbuff.c    | 245 ++++++++++++++++++
 5 files changed, 271 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c

diff --git a/NEWS b/NEWS
index 5c63cef156..a7f25aa5b1 100644
--- a/NEWS
+++ b/NEWS
@@ -167,6 +167,12 @@ Security related changes:
   function could result in a memory leak and potential access of
   uninitialized memory.
 
+  CVE-2021-3999: Passing a buffer of size exactly 1 byte to the getcwd
+  function may result in an off-by-one buffer underflow and overflow
+  when the current working directory is longer than PATH_MAX and also
+  corresponds to the / directory through an unprivileged mount
+  namespace.
+
 The following bugs are resolved with this release:
 
   [The release manager will add the list generated by
diff --git a/sysdeps/posix/getcwd.c b/sysdeps/posix/getcwd.c
index e147a31a81..9d5787b6f4 100644
--- a/sysdeps/posix/getcwd.c
+++ b/sysdeps/posix/getcwd.c
@@ -187,6 +187,13 @@ __getcwd_generic (char *buf, size_t size)
   size_t allocated = size;
   size_t used;
 
+  /* A size of 1 byte is never useful.  */
+  if (allocated == 1)
+    {
+      __set_errno (ERANGE);
+      return NULL;
+    }
+
 #if HAVE_MINIMALLY_WORKING_GETCWD
   /* If AT_FDCWD is not defined, the algorithm below is O(N**2) and
      this is much slower than the system getcwd (at least on
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 61acc1987d..d54753aae5 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -344,7 +344,12 @@ sysdep_routines += xstatconv internal_statvfs \
 
 sysdep_headers += bits/fcntl-linux.h
 
-tests += tst-fallocate tst-fallocate64 tst-o_path-locks
+tests += \
+  tst-fallocate \
+  tst-fallocate64 \
+  tst-getcwd-smallbuff \
+  tst-o_path-locks \
+# tests
 endif
 
 ifeq ($(subdir),elf)
diff --git a/sysdeps/unix/sysv/linux/getcwd.c b/sysdeps/unix/sysv/linux/getcwd.c
index a6b5a7e8b0..5ff678d674 100644
--- a/sysdeps/unix/sysv/linux/getcwd.c
+++ b/sysdeps/unix/sysv/linux/getcwd.c
@@ -50,6 +50,13 @@ __getcwd (char *buf, size_t size)
   char *path;
   char *result;
 
+  /* A size of 1 byte is never useful.  */
+  if (size == 1)
+    {
+      __set_errno (ERANGE);
+      return NULL;
+    }
+
 #ifndef NO_ALLOCATION
   size_t alloc_size = size;
   if (size == 0)
diff --git a/sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c b/sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c
new file mode 100644
index 0000000000..6b2b57f4f7
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c
@@ -0,0 +1,245 @@
+/* Verify that getcwd returns ERANGE for size 1 byte and does not underflow
+   buffer when the CWD is too long and is also a mount target of /.  See bug
+   #28769 or CVE-2021-3999 for more context.
+   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 <fcntl.h>
+#include <limits.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mount.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <support/check.h>
+#include <support/temp_file.h>
+
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
+static char *base;
+#define BASENAME "tst-getcwd-smallbuff"
+#define MOUNT_NAME "mpoint"
+static int sockfd[2];
+
+static void
+do_cleanup (void)
+{
+  support_chdir_toolong_temp_directory (base);
+  TEST_VERIFY_EXIT (rmdir (MOUNT_NAME) == 0);
+  free (base);
+}
+
+static int
+send_fd (const int sock, const int fd)
+{
+  struct msghdr msg;
+  union
+    {
+      struct cmsghdr hdr;
+      char buf[CMSG_SPACE (sizeof (int))];
+    } cmsgbuf;
+  struct cmsghdr *cmsg;
+  struct iovec vec;
+  char ch = 'A';
+  ssize_t n;
+
+  memset (&msg, 0, sizeof (msg));
+  memset (&cmsgbuf, 0, sizeof (cmsgbuf));
+  msg.msg_control = &cmsgbuf.buf;
+  msg.msg_controllen = sizeof (cmsgbuf.buf);
+
+  cmsg = CMSG_FIRSTHDR (&msg);
+  cmsg->cmsg_len = CMSG_LEN (sizeof (int));
+  cmsg->cmsg_level = SOL_SOCKET;
+  cmsg->cmsg_type = SCM_RIGHTS;
+  *(int *) CMSG_DATA (cmsg) = fd;
+
+  vec.iov_base = &ch;
+  vec.iov_len = 1;
+  msg.msg_iov = &vec;
+  msg.msg_iovlen = 1;
+
+  while ((n = sendmsg (sock, &msg, 0)) == -1 && errno == EINTR);
+  if (n != 1)
+    return -1;
+  return 0;
+}
+
+static int
+recv_fd (const int sock)
+{
+  struct msghdr msg;
+  union
+    {
+      struct cmsghdr hdr;
+      char buf[CMSG_SPACE(sizeof(int))];
+    } cmsgbuf;
+  struct cmsghdr *cmsg;
+  struct iovec vec;
+  ssize_t n;
+  char ch = '\0';
+  int fd = -1;
+
+  memset (&msg, 0, sizeof (msg));
+  vec.iov_base = &ch;
+  vec.iov_len = 1;
+  msg.msg_iov = &vec;
+  msg.msg_iovlen = 1;
+
+  memset (&cmsgbuf, 0, sizeof (cmsgbuf));
+  msg.msg_control = &cmsgbuf.buf;
+  msg.msg_controllen = sizeof (cmsgbuf.buf);
+
+  while ((n = recvmsg (sock, &msg, 0)) == -1 && errno == EINTR);
+  if (n != 1 || ch != 'A')
+    return -1;
+
+  cmsg = CMSG_FIRSTHDR (&msg);
+  if (cmsg == NULL)
+    return -1;
+  if (cmsg->cmsg_type != SCM_RIGHTS)
+    return -1;
+  fd = *(const int *) CMSG_DATA (cmsg);
+  if (fd < 0)
+    return -1;
+  return fd;
+}
+
+static int
+child_func (void * const arg)
+{
+    TEST_VERIFY_EXIT (close (sockfd[0]) == 0);
+    const int sock = sockfd[1];
+    char ch;
+
+    TEST_VERIFY_EXIT (read (sock, &ch, 1) == 1);
+    TEST_VERIFY_EXIT (ch == '1');
+
+    if (mount ("/", MOUNT_NAME, NULL, MS_BIND | MS_REC, NULL))
+      FAIL_EXIT1 ("mount failed: %m\n");
+    const int fd = open ("mpoint",
+			 O_RDONLY | O_PATH | O_DIRECTORY | O_NOFOLLOW);
+    TEST_VERIFY_EXIT (fd >= 0);
+
+    TEST_VERIFY_EXIT (send_fd (sock, fd) == 0);
+    TEST_VERIFY_EXIT (close (fd) == 0);
+
+    TEST_VERIFY_EXIT (read (sock, &ch, 1) == 1);
+    TEST_VERIFY_EXIT (ch == 'a');
+
+    TEST_VERIFY_EXIT (close (sock) == 0);
+    return 0;
+}
+
+static void
+update_map (char * const mapping, const char * const map_file)
+{
+    const size_t map_len = strlen (mapping);
+
+    const int fd = open (map_file, O_WRONLY);
+    TEST_VERIFY_EXIT (fd >= 0);
+    TEST_VERIFY_EXIT (write (fd, mapping, map_len) == (ssize_t) map_len);
+    TEST_VERIFY_EXIT (close(fd) == 0);
+}
+
+static void
+proc_setgroups_write (const long child_pid, const char * const str)
+{
+    const size_t str_len = strlen(str);
+
+    char setgroups_path[64];
+    snprintf (setgroups_path, sizeof (setgroups_path),
+	      "/proc/%ld/setgroups", child_pid);
+
+    const int fd = open (setgroups_path, O_WRONLY);
+
+    if (fd < 0)
+      {
+        TEST_VERIFY_EXIT (errno == ENOENT);
+        return;
+      }
+
+    TEST_VERIFY_EXIT (write (fd, str, str_len) == (ssize_t) str_len);
+    TEST_VERIFY_EXIT (close(fd) == 0);
+}
+
+static char child_stack[1024 * 1024];
+
+int
+do_test (void)
+{
+  base = support_create_and_chdir_toolong_temp_directory (BASENAME);
+
+  TEST_VERIFY_EXIT (mkdir (MOUNT_NAME, S_IRWXU) == 0);
+  atexit (do_cleanup);
+
+  TEST_VERIFY_EXIT (socketpair(AF_UNIX, SOCK_STREAM, 0, sockfd) == 0);
+  const long child_pid = clone (child_func, child_stack + sizeof(child_stack),
+				CLONE_NEWUSER | CLONE_NEWNS | SIGCHLD, NULL);
+
+  TEST_VERIFY_EXIT (child_pid > 1);
+  TEST_VERIFY_EXIT (close(sockfd[1]) == 0);
+  const int sock = sockfd[0];
+
+  char map_path[64], map_buf[64];
+  snprintf (map_path, sizeof (map_path), "/proc/%ld/uid_map", child_pid);
+  snprintf (map_buf, sizeof (map_buf), "0 %ld 1", (long) getuid());
+  update_map (map_buf, map_path);
+
+  proc_setgroups_write (child_pid, "deny");
+  snprintf (map_path, sizeof (map_path), "/proc/%ld/gid_map", child_pid);
+  snprintf (map_buf, sizeof (map_buf), "0 %ld 1", (long) getgid());
+  update_map (map_buf, map_path);
+
+  TEST_VERIFY_EXIT (send (sock, "1", 1, MSG_NOSIGNAL) == 1);
+  const int fd = recv_fd (sock);
+  TEST_VERIFY_EXIT (fd >= 0);
+  TEST_VERIFY_EXIT (fchdir(fd) == 0);
+
+  static char buf[2 * 10 + 1];
+  memset (buf, 'A', sizeof(buf));
+
+  /* Finally, call getcwd and check if it resulted in a buffer underflow.  */
+  char * cwd = getcwd (buf + sizeof(buf) / 2, 1);
+  TEST_VERIFY (cwd == NULL && errno == ERANGE);
+
+  for (int i = 0; i < sizeof (buf); i++)
+    if (buf[i] != 'A')
+      {
+	printf ("buf[%d] = %02x\n", i, (unsigned int) buf[i]);
+	support_record_failure ();
+      }
+
+  TEST_VERIFY_EXIT (send (sock, "a", 1, MSG_NOSIGNAL) == 1);
+  TEST_VERIFY_EXIT (close (sock) == 0);
+  TEST_VERIFY_EXIT (waitpid (child_pid, NULL, 0) == child_pid);
+
+  return 0;
+}
+
+#define CLEANUP_HANDLER do_cleanup
+#include <support/test-driver.c>
-- 
2.34.1

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Andreas Schwab]

On Jan 18 2022, Siddhesh Poyarekar via Libc-alpha wrote:

> diff --git a/sysdeps/unix/sysv/linux/getcwd.c b/sysdeps/unix/sysv/linux/getcwd.c
> index a6b5a7e8b0..5ff678d674 100644
> --- a/sysdeps/unix/sysv/linux/getcwd.c
> +++ b/sysdeps/unix/sysv/linux/getcwd.c
> @@ -50,6 +50,13 @@ __getcwd (char *buf, size_t size)
>    char *path;
>    char *result;
>  
> +  /* A size of 1 byte is never useful.  */
> +  if (size == 1)
> +    {
> +      __set_errno (ERANGE);
> +      return NULL;
> +    }
> +

This is not needed, since the getcwd syscall does the check already and
returns the correct error.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Siddhesh Poyarekar]

On 18/01/2022 17:22, Andreas Schwab wrote:
> On Jan 18 2022, Siddhesh Poyarekar via Libc-alpha wrote:
> 
>> diff --git a/sysdeps/unix/sysv/linux/getcwd.c b/sysdeps/unix/sysv/linux/getcwd.c
>> index a6b5a7e8b0..5ff678d674 100644
>> --- a/sysdeps/unix/sysv/linux/getcwd.c
>> +++ b/sysdeps/unix/sysv/linux/getcwd.c
>> @@ -50,6 +50,13 @@ __getcwd (char *buf, size_t size)
>>     char *path;
>>     char *result;
>>   
>> +  /* A size of 1 byte is never useful.  */
>> +  if (size == 1)
>> +    {
>> +      __set_errno (ERANGE);
>> +      return NULL;
>> +    }
>> +
> 
> This is not needed, since the getcwd syscall does the check already and
> returns the correct error.
> 

Not quite; this is a very specific bug that goes beyond just a simple 
range issue.  If the buffer is insufficient the syscall does return 
ERANGE.  However if the returned name is too long, it does that check 
first and returns ENAMETOOLONG instead.  We then process it to try and 
get the cwd anyway by using the posix variant.

Now if the path also has an unprivileged mount (see reproducer) of '/' 
on it, it ends up writing outside of the single byte buffer bound.

That said, we could get away with fixing only sysdeps/posix/getcwd.c. 
Is that what you suggest I do?

Thanks,
Siddhesh

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Andreas Schwab]

On Jan 18 2022, Siddhesh Poyarekar wrote:

> We then process it to try and get the cwd anyway by using the posix
> variant.

Which returns the appropriate error.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Siddhesh Poyarekar]

On 18/01/2022 18:43, Andreas Schwab wrote:
> On Jan 18 2022, Siddhesh Poyarekar wrote:
> 
>> We then process it to try and get the cwd anyway by using the posix
>> variant.
> 
> Which returns the appropriate error.
> 

In the specific case of an unprivileged mount on the same directory, it 
ends up underflowing the buffer before returning.  Whether it returns 
the right error or not becomes irrelevant then.  Please see the reproducer.

Siddhesh

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Andreas Schwab]

On Jan 18 2022, Siddhesh Poyarekar wrote:

> On 18/01/2022 18:43, Andreas Schwab wrote:
>> On Jan 18 2022, Siddhesh Poyarekar wrote:
>> 
>>> We then process it to try and get the cwd anyway by using the posix
>>> variant.
>> Which returns the appropriate error.
>> 
>
> In the specific case of an unprivileged mount on the same directory, it
> ends up underflowing the buffer before returning.

No, it returns with ERANGE.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Siddhesh Poyarekar]

On 18/01/2022 19:00, Andreas Schwab wrote:
> On Jan 18 2022, Siddhesh Poyarekar wrote:
> 
>> On 18/01/2022 18:43, Andreas Schwab wrote:
>>> On Jan 18 2022, Siddhesh Poyarekar wrote:
>>>
>>>> We then process it to try and get the cwd anyway by using the posix
>>>> variant.
>>> Which returns the appropriate error.
>>>
>>
>> In the specific case of an unprivileged mount on the same directory, it
>> ends up underflowing the buffer before returning.
> 
> No, it returns with ERANGE.
> 

Can you tell me where the reproducer is wrong then?  It clearly shows 
the buffer being overwritten where it shouldn't be.

Thanks,
Siddhesh

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Andreas Schwab]

On Jan 18 2022, Siddhesh Poyarekar wrote:

> Can you tell me where the reproducer is wrong then?

Is it?

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Siddhesh Poyarekar]

On 18/01/2022 19:11, Andreas Schwab wrote:
> On Jan 18 2022, Siddhesh Poyarekar wrote:
> 
>> Can you tell me where the reproducer is wrong then?
> 
> Is it?
> 

I'm unable to parse your one-liners, can you please elaborate?  I can't 
even tell for sure what part of the patch you're objecting to.

Without the patch, the test fails like so:

error: ../sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c:228: not true: 
cwd == NULL && errno == ERANGE
buf[9] = 2f
buf[10] = 2f
buf[11] = 00
error: 4 test failures

where buf[10] is the single byte that is passed.  Note that buf[9] as 
well as buf[11] get overwritten.  Not only that, neither getcwd returns 
a non-NULL value nor is errno ERANGE; I split out the TEST_VERIFY to 
confirm that both are false.

Siddhesh

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Siddhesh Poyarekar]

On 18/01/2022 19:15, Siddhesh Poyarekar via Libc-alpha wrote:
> well as buf[11] get overwritten.  Not only that, neither getcwd returns 
> a non-NULL value nor is errno ERANGE; I split out the TEST_VERIFY to 

correction: neither getcwd returns NULL nor is errno ERANGE.

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Andreas Schwab]

On Jan 18 2022, Siddhesh Poyarekar wrote:

> Without the patch

What's your point?  I don't understand what you are trying to say.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Siddhesh Poyarekar]

On 18/01/2022 19:27, Andreas Schwab wrote:
> On Jan 18 2022, Siddhesh Poyarekar wrote:
> 
>> Without the patch
> 
> What's your point?  I don't understand what you are trying to say.
> 

Let me start over.

getcwd in its current form may underflow and overflow the user supplied 
buffer when *all* of the following conditions are met:

- The buffer size (i.e. the second argument of getcwd) is 1 byte
- The current working directory is too long
- '/' is also mounted on the current working directory

Sequence of events:

- In sysdeps/unix/sysv/linux/getcwd.c, the syscall returns ENAMETOOLONG 
because the kernel checks for name length before it checks buffer size

- The code falls back to the generic getcwd in sysdeps/posix

- In the generic func, the buf[0] is set to '\0' on line 250

- this while loop on line 262 is bypassed:

while (!(thisdev == rootdev && thisino == rootino))

since the rootfs (/) is bind mounted onto the directory and the flow 
goes on to line 449, where it puts a '/' in the byte before the buffer.

- Finally on line 458, it moves 2 bytes (the underflowed byte and the 
'\0') to the buf[0] and buf[1], resulting in a 1 byte buffer overflow.

- buf is returned on line 469 and errno is not set.

This fix avoids the underflow+overflow by shortcircuiting early and 
returns NULL, setting errno to ERANGE for 1 byte buffers because they 
can never be reasonably used to return a valid path.

Siddhesh

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Adhemerval Zanella]

On 18/01/2022 10:45, Siddhesh Poyarekar via Libc-alpha wrote:
> On 18/01/2022 19:11, Andreas Schwab wrote:
>> On Jan 18 2022, Siddhesh Poyarekar wrote:
>>
>>> Can you tell me where the reproducer is wrong then?
>>
>> Is it?
>>
> 
> I'm unable to parse your one-liners, can you please elaborate?  I can't even tell for sure what part of the patch you're objecting to.
> 
> Without the patch, the test fails like so:
> 
> error: ../sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c:228: not true: cwd == NULL && errno == ERANGE
> buf[9] = 2f
> buf[10] = 2f
> buf[11] = 00
> error: 4 test failures
> 
> where buf[10] is the single byte that is passed.  Note that buf[9] as well as buf[11] get overwritten.  Not only that, neither getcwd returns a non-NULL value nor is errno ERANGE; I split out the TEST_VERIFY to confirm that both are false.


Shouldn't we fix it on posix generic implementation then?

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Siddhesh Poyarekar]

On 18/01/2022 19:29, Adhemerval Zanella via Libc-alpha wrote:
> 
> Shouldn't we fix it on posix generic implementation then?
> 

I added the shortcircuit in the generic as well as linux 
implementations.  Should I only restrict it to the posix one? 
Technically the posix implementation is the only one that writes beyond 
buffer bounds, but the linux target is the only one that has the 
reproducer due to the linux-specific features used to get the 
underflow+overflow going.

Thanks,
Siddhesh

Re: [PATCH 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999)

[Adhemerval Zanella]

On 18/01/2022 11:44, Siddhesh Poyarekar wrote:
> On 18/01/2022 19:29, Adhemerval Zanella via Libc-alpha wrote:
>>
>> Shouldn't we fix it on posix generic implementation then?
>>
> 
> I added the shortcircuit in the generic as well as linux implementations.  Should I only restrict it to the posix one? Technically the posix implementation is the only one that writes beyond buffer bounds, but the linux target is the only one that has the reproducer due to the linux-specific features used to get the underflow+overflow going.

I think it would make sense only to fix on posix one, since the syscall 
already handle it correctly.