diff mbox series

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

Message ID 20220118090728.1825487-4-siddhesh@sourceware.org
State Superseded
Headers show
Series Fixes for CVE-2021-3998 and CVE-2021-3999 | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit fail Patch caused testsuite regressions

Commit Message

Siddhesh Poyarekar Jan. 18, 2022, 9:07 a.m. UTC
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

Comments

Andreas Schwab Jan. 18, 2022, 11:52 a.m. UTC | #1
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.
Siddhesh Poyarekar Jan. 18, 2022, 1:10 p.m. UTC | #2
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
Andreas Schwab Jan. 18, 2022, 1:13 p.m. UTC | #3
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.
Siddhesh Poyarekar Jan. 18, 2022, 1:16 p.m. UTC | #4
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
Andreas Schwab Jan. 18, 2022, 1:30 p.m. UTC | #5
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.
Siddhesh Poyarekar Jan. 18, 2022, 1:33 p.m. UTC | #6
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
Andreas Schwab Jan. 18, 2022, 1:41 p.m. UTC | #7
On Jan 18 2022, Siddhesh Poyarekar wrote:

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

Is it?
Siddhesh Poyarekar Jan. 18, 2022, 1:45 p.m. UTC | #8
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
Siddhesh Poyarekar Jan. 18, 2022, 1:56 p.m. UTC | #9
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.
Andreas Schwab Jan. 18, 2022, 1:57 p.m. UTC | #10
On Jan 18 2022, Siddhesh Poyarekar wrote:

> Without the patch

What's your point?  I don't understand what you are trying to say.
Adhemerval Zanella Jan. 18, 2022, 1:59 p.m. UTC | #11
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?
Siddhesh Poyarekar Jan. 18, 2022, 2:40 p.m. UTC | #12
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
Siddhesh Poyarekar Jan. 18, 2022, 2:44 p.m. UTC | #13
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
Adhemerval Zanella Jan. 18, 2022, 4:30 p.m. UTC | #14
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.
diff mbox series

Patch

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>