[PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

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

[PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Dmitry V. Levin
As the underlying getcwd syscall, starting with linux commit
v2.6.36-rc1~96^2~2, may succeed without returning an absolute path,
check for the path returned by syscall and fail with EACCES if the path
is not absolute.

[BZ #22679]
* sysdeps/unix/sysv/linux/getcwd.c (__getcwd): Set errno to EACCES and
return NULL if the path returned by getcwd syscall is not absolute.
* sysdeps/unix/sysv/linux/tst-getcwd-abspath.c: New test.
* sysdeps/unix/sysv/linux/Makefile (tests): Add tst-getcwd-abspath.
---
 ChangeLog                                    |  8 ++++
 sysdeps/unix/sysv/linux/Makefile             |  2 +-
 sysdeps/unix/sysv/linux/getcwd.c             | 10 +++++
 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c | 59 ++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 8f19e0e..34c5c39 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -45,7 +45,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
 tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
  tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
  test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
- tst-rlimit-infinity
+ tst-rlimit-infinity tst-getcwd-abspath
 
 # Generate the list of SYS_* macros for the system calls (__NR_*
 # macros).  The file syscall-names.list contains all possible system
diff --git a/sysdeps/unix/sysv/linux/getcwd.c b/sysdeps/unix/sysv/linux/getcwd.c
index f545106..2a4320d 100644
--- a/sysdeps/unix/sysv/linux/getcwd.c
+++ b/sysdeps/unix/sysv/linux/getcwd.c
@@ -78,6 +78,16 @@ __getcwd (char *buf, size_t size)
   retval = INLINE_SYSCALL (getcwd, 2, path, alloc_size);
   if (retval >= 0)
     {
+      if (retval == 0 || path[0] != '/')
+ {
+#ifndef NO_ALLOCATION
+  if (buf == NULL && size == 0)
+    free (path);
+#endif
+  __set_errno (EACCES);
+  return NULL;
+ }
+
 #ifndef NO_ALLOCATION
       if (buf == NULL && size == 0)
  /* Ensure that the buffer is only as large as necessary.  */
diff --git a/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
new file mode 100644
index 0000000..2a81bbd
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
@@ -0,0 +1,59 @@
+/* BZ #22679 getcwd(3) does not succeed without returning an absolute path.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   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 <stdio.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+static char *chroot_dir;
+
+/* The actual test.  Run it in a subprocess, so that the test harness
+   can remove the temporary directory in --direct mode.  */
+static void
+getcwd_callback (void *closure)
+{
+  xchroot (chroot_dir);
+  errno = 0;
+  char *cwd = getcwd (NULL, 0);
+  TEST_VERIFY (cwd == NULL);
+  TEST_VERIFY (errno == EACCES);
+  _exit (0);
+}
+
+static int
+do_test (void)
+{
+  support_become_root ();
+  if (!support_can_chroot ())
+    return EXIT_UNSUPPORTED;
+
+  chroot_dir = support_create_temp_directory ("tst-getcwd-abspath-");
+  support_isolate_in_subprocess (getcwd_callback, NULL);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
--
ldv
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Andreas Schwab-2
On Jan 07 2018, "Dmitry V. Levin" <[hidden email]> wrote:

> As the underlying getcwd syscall, starting with linux commit
> v2.6.36-rc1~96^2~2, may succeed without returning an absolute path,
> check for the path returned by syscall and fail with EACCES if the path
> is not absolute.

When we still used /proc we just fell through to the generic getcwd in
that case.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Dmitry V. Levin
On Sun, Jan 07, 2018 at 09:23:26AM +0100, Andreas Schwab wrote:
> On Jan 07 2018, "Dmitry V. Levin" <[hidden email]> wrote:
>
> > As the underlying getcwd syscall, starting with linux commit
> > v2.6.36-rc1~96^2~2, may succeed without returning an absolute path,
> > check for the path returned by syscall and fail with EACCES if the path
> > is not absolute.
>
> When we still used /proc we just fell through to the generic getcwd in
> that case.

Sure, but the generic getcwd sets errno to ENOENT in this case.
Do you suppose that ENOENT is more appropriate than EACCES?


--
ldv

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Andreas Schwab-2
On Jan 07 2018, "Dmitry V. Levin" <[hidden email]> wrote:

> Sure, but the generic getcwd sets errno to ENOENT in this case.
> Do you suppose that ENOENT is more appropriate than EACCES?

Yes, the anchor no longer exists.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Dmitry V. Levin
On Sun, Jan 07, 2018 at 01:20:42PM +0100, Andreas Schwab wrote:
> On Jan 07 2018, "Dmitry V. Levin" <[hidden email]> wrote:
>
> > Sure, but the generic getcwd sets errno to ENOENT in this case.
> > Do you suppose that ENOENT is more appropriate than EACCES?
>
> Yes, the anchor no longer exists.

OK, I'll submit the second edition shortly.


--
ldv

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Dmitry V. Levin
As the underlying getcwd syscall, starting with linux commit
v2.6.36-rc1~96^2~2, may succeed without returning an absolute path,
check for the path returned by syscall and fall back to generic_getcwd
if the path is not absolute.

[BZ #22679]
* sysdeps/unix/sysv/linux/getcwd.c (__getcwd): Fall back to
generic_getcwd if the path returned by getcwd syscall is not absolute.
* sysdeps/unix/sysv/linux/tst-getcwd-abspath.c: New test.
* sysdeps/unix/sysv/linux/Makefile (tests): Add tst-getcwd-abspath.
---
 ChangeLog                                    |  8 ++++
 sysdeps/unix/sysv/linux/Makefile             |  2 +-
 sysdeps/unix/sysv/linux/getcwd.c             |  8 ++--
 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c | 58 ++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+), 5 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index b1fe960..88ecb08 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -51,7 +51,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
 
 tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
  tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
- test-errno-linux
+ test-errno-linux tst-getcwd-abspath
 
 # Generate the list of SYS_* macros for the system calls (__NR_* macros).
 
diff --git a/sysdeps/unix/sysv/linux/getcwd.c b/sysdeps/unix/sysv/linux/getcwd.c
index 3b556fd..f485de8 100644
--- a/sysdeps/unix/sysv/linux/getcwd.c
+++ b/sysdeps/unix/sysv/linux/getcwd.c
@@ -76,7 +76,7 @@ __getcwd (char *buf, size_t size)
   int retval;
 
   retval = INLINE_SYSCALL (getcwd, 2, path, alloc_size);
-  if (retval >= 0)
+  if (retval > 0 && path[0] == '/')
     {
 #ifndef NO_ALLOCATION
       if (buf == NULL && size == 0)
@@ -92,10 +92,10 @@ __getcwd (char *buf, size_t size)
       return buf;
     }
 
-  /* The system call cannot handle paths longer than a page.
-     Neither can the magic symlink in /proc/self.  Just use the
+  /* The system call either cannot handle paths longer than a page
+     or can succeed without returning an absolute path.  Just use the
      generic implementation right away.  */
-  if (errno == ENAMETOOLONG)
+  if (retval >= 0 || errno == ENAMETOOLONG)
     {
 #ifndef NO_ALLOCATION
       if (buf == NULL && size == 0)
diff --git a/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
new file mode 100644
index 0000000..69c5366
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
@@ -0,0 +1,58 @@
+/* BZ #22679 getcwd(3) does not succeed without returning an absolute path.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   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 <stdio.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+static char *chroot_dir;
+
+/* The actual test.  Run it in a subprocess, so that the test harness
+   can remove the temporary directory in --direct mode.  */
+static void
+getcwd_callback (void *closure)
+{
+  xchroot (chroot_dir);
+  errno = 0;
+  char *cwd = getcwd (NULL, 0);
+  TEST_VERIFY_EXIT (cwd == NULL && errno == ENOENT);
+  _exit (0);
+}
+
+static int
+do_test (void)
+{
+  support_become_root ();
+  if (!support_can_chroot ())
+    return EXIT_UNSUPPORTED;
+
+  chroot_dir = support_create_temp_directory ("tst-getcwd-abspath-");
+  support_isolate_in_subprocess (getcwd_callback, NULL);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
--
ldv
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Florian Weimer
* Dmitry V. Levin:

> +  TEST_VERIFY_EXIT (cwd == NULL && errno == ENOENT);

Using

  TEST_COMPARE (errno, ENOENT);

for the second check would make it easier to diagnose test failures
without recompiling the test.
Reply | Threaded
Open this post in threaded view
|

[PATCH v3] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Dmitry V. Levin
As the underlying getcwd syscall, starting with linux commit
v2.6.36-rc1~96^2~2, may succeed without returning an absolute path,
check for the path returned by syscall and fall back to generic_getcwd
if the path is not absolute.

[BZ #22679]
* sysdeps/unix/sysv/linux/getcwd.c (__getcwd): Fall back to
generic_getcwd if the path returned by getcwd syscall is not absolute.
* sysdeps/unix/sysv/linux/tst-getcwd-abspath.c: New test.
* sysdeps/unix/sysv/linux/Makefile (tests): Add tst-getcwd-abspath.
---
 ChangeLog                                    |  8 ++++
 sysdeps/unix/sysv/linux/Makefile             |  2 +-
 sysdeps/unix/sysv/linux/getcwd.c             |  8 ++--
 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c | 59 ++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 5 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 8f19e0e..34c5c39 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -45,7 +45,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
 tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
  tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
  test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
- tst-rlimit-infinity
+ tst-rlimit-infinity tst-getcwd-abspath
 
 # Generate the list of SYS_* macros for the system calls (__NR_*
 # macros).  The file syscall-names.list contains all possible system
diff --git a/sysdeps/unix/sysv/linux/getcwd.c b/sysdeps/unix/sysv/linux/getcwd.c
index f545106..866b9d2 100644
--- a/sysdeps/unix/sysv/linux/getcwd.c
+++ b/sysdeps/unix/sysv/linux/getcwd.c
@@ -76,7 +76,7 @@ __getcwd (char *buf, size_t size)
   int retval;
 
   retval = INLINE_SYSCALL (getcwd, 2, path, alloc_size);
-  if (retval >= 0)
+  if (retval > 0 && path[0] == '/')
     {
 #ifndef NO_ALLOCATION
       if (buf == NULL && size == 0)
@@ -92,10 +92,10 @@ __getcwd (char *buf, size_t size)
       return buf;
     }
 
-  /* The system call cannot handle paths longer than a page.
-     Neither can the magic symlink in /proc/self.  Just use the
+  /* The system call either cannot handle paths longer than a page
+     or can succeed without returning an absolute path.  Just use the
      generic implementation right away.  */
-  if (errno == ENAMETOOLONG)
+  if (retval >= 0 || errno == ENAMETOOLONG)
     {
 #ifndef NO_ALLOCATION
       if (buf == NULL && size == 0)
diff --git a/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
new file mode 100644
index 0000000..ea3562c
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
@@ -0,0 +1,59 @@
+/* BZ #22679 getcwd(3) does not succeed without returning an absolute path.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   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 <stdio.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+static char *chroot_dir;
+
+/* The actual test.  Run it in a subprocess, so that the test harness
+   can remove the temporary directory in --direct mode.  */
+static void
+getcwd_callback (void *closure)
+{
+  xchroot (chroot_dir);
+  errno = 0;
+  char *cwd = getcwd (NULL, 0);
+  TEST_COMPARE (errno, ENOENT);
+  TEST_VERIFY (cwd == NULL);
+  _exit (0);
+}
+
+static int
+do_test (void)
+{
+  support_become_root ();
+  if (!support_can_chroot ())
+    return EXIT_UNSUPPORTED;
+
+  chroot_dir = support_create_temp_directory ("tst-getcwd-abspath-");
+  support_isolate_in_subprocess (getcwd_callback, NULL);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
--
ldv
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Zack Weinberg-2
In reply to this post by Andreas Schwab-2
On Sun, Jan 7, 2018 at 7:20 AM, Andreas Schwab <[hidden email]> wrote:
> On Jan 07 2018, "Dmitry V. Levin" <[hidden email]> wrote:
>
>> Sure, but the generic getcwd sets errno to ENOENT in this case.
>> Do you suppose that ENOENT is more appropriate than EACCES?
>
> Yes, the anchor no longer exists.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
does not license getcwd to fail with ENOENT.

zw
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Andreas Schwab-2
On Jan 07 2018, Zack Weinberg <[hidden email]> wrote:

> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
> does not license getcwd to fail with ENOENT.

That's not true.  It doesn't specify any condition for ENOENT, thus we
can use it for our purpose.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Zack Weinberg-2
On Sun, Jan 7, 2018 at 11:07 AM, Andreas Schwab <[hidden email]> wrote:
> On Jan 07 2018, Zack Weinberg <[hidden email]> wrote:
>
>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
>> does not license getcwd to fail with ENOENT.
>
> That's not true.  It doesn't specify any condition for ENOENT, thus we
> can use it for our purpose.

I thought the ERRORS sections were meant to be exhaustive - no other
errors are allowed.

zw
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Dmitry V. Levin
In reply to this post by Andreas Schwab-2
On Sun, Jan 07, 2018 at 05:07:25PM +0100, Andreas Schwab wrote:
> On Jan 07 2018, Zack Weinberg <[hidden email]> wrote:
>
> > http://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
> > does not license getcwd to fail with ENOENT.
>
> That's not true.  It doesn't specify any condition for ENOENT, thus we
> can use it for our purpose.

In fact, we already use ENOENT when the current directory is unlinked,
and making getcwd(3) fail with ENOENT when it cannot obtain an absolute
path would be consistent with that traditional case.


--
ldv

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Dmitry V. Levin
In reply to this post by Zack Weinberg-2
On Sun, Jan 07, 2018 at 11:21:19AM -0500, Zack Weinberg wrote:

> On Sun, Jan 7, 2018 at 11:07 AM, Andreas Schwab <[hidden email]> wrote:
> > On Jan 07 2018, Zack Weinberg <[hidden email]> wrote:
> >
> >> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
> >> does not license getcwd to fail with ENOENT.
> >
> > That's not true.  It doesn't specify any condition for ENOENT, thus we
> > can use it for our purpose.
>
> I thought the ERRORS sections were meant to be exhaustive - no other
> errors are allowed.
Just the otherwise: "Implementations may support additional errors not
included in this list, may generate errors included in this list under
circumstances other than those described here, or may contain extensions
or limitations that prevent some errors from occurring."


--
ldv

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Zack Weinberg-2
On Sun, Jan 7, 2018 at 12:07 PM, Dmitry V. Levin <[hidden email]> wrote:
> On Sun, Jan 07, 2018 at 11:21:19AM -0500, Zack Weinberg wrote:
>>
>> I thought the ERRORS sections were meant to be exhaustive - no other
>> errors are allowed.
>
> Just the otherwise: "Implementations may support additional errors not
> included in this list, may generate errors included in this list under
> circumstances other than those described here, or may contain extensions
> or limitations that prevent some errors from occurring."

OK, objection withdrawn.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Dmitry V. Levin
In reply to this post by Dmitry V. Levin
I think this edition of the fix addresses all comments raised so far.
Is it OK to commit, or does anybody else have any more comments?

On Sun, Jan 07, 2018 at 04:39:05PM +0300, Dmitry V. Levin wrote:

> As the underlying getcwd syscall, starting with linux commit
> v2.6.36-rc1~96^2~2, may succeed without returning an absolute path,
> check for the path returned by syscall and fall back to generic_getcwd
> if the path is not absolute.
>
> [BZ #22679]
> * sysdeps/unix/sysv/linux/getcwd.c (__getcwd): Fall back to
> generic_getcwd if the path returned by getcwd syscall is not absolute.
> * sysdeps/unix/sysv/linux/tst-getcwd-abspath.c: New test.
> * sysdeps/unix/sysv/linux/Makefile (tests): Add tst-getcwd-abspath.
> ---
>  ChangeLog                                    |  8 ++++
>  sysdeps/unix/sysv/linux/Makefile             |  2 +-
>  sysdeps/unix/sysv/linux/getcwd.c             |  8 ++--
>  sysdeps/unix/sysv/linux/tst-getcwd-abspath.c | 59 ++++++++++++++++++++++++++++
>  4 files changed, 72 insertions(+), 5 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
>
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 8f19e0e..34c5c39 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -45,7 +45,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
>  tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
>   tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
>   test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
> - tst-rlimit-infinity
> + tst-rlimit-infinity tst-getcwd-abspath
>  
>  # Generate the list of SYS_* macros for the system calls (__NR_*
>  # macros).  The file syscall-names.list contains all possible system
> diff --git a/sysdeps/unix/sysv/linux/getcwd.c b/sysdeps/unix/sysv/linux/getcwd.c
> index f545106..866b9d2 100644
> --- a/sysdeps/unix/sysv/linux/getcwd.c
> +++ b/sysdeps/unix/sysv/linux/getcwd.c
> @@ -76,7 +76,7 @@ __getcwd (char *buf, size_t size)
>    int retval;
>  
>    retval = INLINE_SYSCALL (getcwd, 2, path, alloc_size);
> -  if (retval >= 0)
> +  if (retval > 0 && path[0] == '/')
>      {
>  #ifndef NO_ALLOCATION
>        if (buf == NULL && size == 0)
> @@ -92,10 +92,10 @@ __getcwd (char *buf, size_t size)
>        return buf;
>      }
>  
> -  /* The system call cannot handle paths longer than a page.
> -     Neither can the magic symlink in /proc/self.  Just use the
> +  /* The system call either cannot handle paths longer than a page
> +     or can succeed without returning an absolute path.  Just use the
>       generic implementation right away.  */
> -  if (errno == ENAMETOOLONG)
> +  if (retval >= 0 || errno == ENAMETOOLONG)
>      {
>  #ifndef NO_ALLOCATION
>        if (buf == NULL && size == 0)
> diff --git a/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
> new file mode 100644
> index 0000000..ea3562c
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
> @@ -0,0 +1,59 @@
> +/* BZ #22679 getcwd(3) does not succeed without returning an absolute path.
> +
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   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 <stdio.h>
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/namespace.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/test-driver.h>
> +#include <support/xunistd.h>
> +#include <unistd.h>
> +
> +static char *chroot_dir;
> +
> +/* The actual test.  Run it in a subprocess, so that the test harness
> +   can remove the temporary directory in --direct mode.  */
> +static void
> +getcwd_callback (void *closure)
> +{
> +  xchroot (chroot_dir);
> +  errno = 0;
> +  char *cwd = getcwd (NULL, 0);
> +  TEST_COMPARE (errno, ENOENT);
> +  TEST_VERIFY (cwd == NULL);
> +  _exit (0);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  support_become_root ();
> +  if (!support_can_chroot ())
> +    return EXIT_UNSUPPORTED;
> +
> +  chroot_dir = support_create_temp_directory ("tst-getcwd-abspath-");
> +  support_isolate_in_subprocess (getcwd_callback, NULL);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> --
> ldv
--
ldv

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

[PATCH v4] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Dmitry V. Levin
Currently getcwd(3) can succeed without returning an absolute path
because the underlying getcwd syscall, starting with linux commit
v2.6.36-rc1~96^2~2, may succeed without returning an absolute path.

This is a conformance issue because "The getcwd() function shall
place an absolute pathname of the current working directory
in the array pointed to by buf, and return buf".

This is also a security issue because a non-absolute path returned
by getcwd(3) causes a buffer underflow in realpath(3).

Fix this by checking the path returned by getcwd syscall and falling
back to generic_getcwd if the path is not absolute, effectively making
getcwd(3) fail with ENOENT.  The error code is chosen for consistency
with the case when the current directory is unlinked.

[BZ #22679]
CVE-2018-1000001
* sysdeps/unix/sysv/linux/getcwd.c (__getcwd): Fall back to
generic_getcwd if the path returned by getcwd syscall is not absolute.
* sysdeps/unix/sysv/linux/tst-getcwd-abspath.c: New test.
* sysdeps/unix/sysv/linux/Makefile (tests): Add tst-getcwd-abspath.
---

This is the same patch as v3; I've just added information about
security implications.

 ChangeLog                                    |  9 +++++
 NEWS                                         |  4 ++
 sysdeps/unix/sysv/linux/Makefile             |  2 +-
 sysdeps/unix/sysv/linux/getcwd.c             |  8 ++--
 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c | 59 ++++++++++++++++++++++++++++
 5 files changed, 77 insertions(+), 5 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c

diff --git a/NEWS b/NEWS
index 75bf467..79736db 100644
--- a/NEWS
+++ b/NEWS
@@ -199,6 +199,10 @@ Security related changes:
   for AT_SECURE or SUID binaries could be used to load libraries from the
   current directory.
 
+  CVE-2018-1000001: Buffer underflow in realpath function when getcwd function
+  succeeds without returning an absolute path due to unexpected behaviour
+  of the Linux kernel getcwd syscall.  Reported by halfdog.
+
 The following bugs are resolved with this release:
 
   [The release manager will add the list generated by
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 8f19e0e..34c5c39 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -45,7 +45,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
 tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
  tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
  test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
- tst-rlimit-infinity
+ tst-rlimit-infinity tst-getcwd-abspath
 
 # Generate the list of SYS_* macros for the system calls (__NR_*
 # macros).  The file syscall-names.list contains all possible system
diff --git a/sysdeps/unix/sysv/linux/getcwd.c b/sysdeps/unix/sysv/linux/getcwd.c
index f545106..866b9d2 100644
--- a/sysdeps/unix/sysv/linux/getcwd.c
+++ b/sysdeps/unix/sysv/linux/getcwd.c
@@ -76,7 +76,7 @@ __getcwd (char *buf, size_t size)
   int retval;
 
   retval = INLINE_SYSCALL (getcwd, 2, path, alloc_size);
-  if (retval >= 0)
+  if (retval > 0 && path[0] == '/')
     {
 #ifndef NO_ALLOCATION
       if (buf == NULL && size == 0)
@@ -92,10 +92,10 @@ __getcwd (char *buf, size_t size)
       return buf;
     }
 
-  /* The system call cannot handle paths longer than a page.
-     Neither can the magic symlink in /proc/self.  Just use the
+  /* The system call either cannot handle paths longer than a page
+     or can succeed without returning an absolute path.  Just use the
      generic implementation right away.  */
-  if (errno == ENAMETOOLONG)
+  if (retval >= 0 || errno == ENAMETOOLONG)
     {
 #ifndef NO_ALLOCATION
       if (buf == NULL && size == 0)
diff --git a/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
new file mode 100644
index 0000000..ea3562c
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
@@ -0,0 +1,59 @@
+/* BZ #22679 getcwd(3) does not succeed without returning an absolute path.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   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 <stdio.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+static char *chroot_dir;
+
+/* The actual test.  Run it in a subprocess, so that the test harness
+   can remove the temporary directory in --direct mode.  */
+static void
+getcwd_callback (void *closure)
+{
+  xchroot (chroot_dir);
+  errno = 0;
+  char *cwd = getcwd (NULL, 0);
+  TEST_COMPARE (errno, ENOENT);
+  TEST_VERIFY (cwd == NULL);
+  _exit (0);
+}
+
+static int
+do_test (void)
+{
+  support_become_root ();
+  if (!support_can_chroot ())
+    return EXIT_UNSUPPORTED;
+
+  chroot_dir = support_create_temp_directory ("tst-getcwd-abspath-");
+  support_isolate_in_subprocess (getcwd_callback, NULL);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
--
ldv
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Florian Weimer-5
On 01/11/2018 11:03 PM, Dmitry V. Levin wrote:
> +  char *cwd = getcwd (NULL, 0);
> +  TEST_COMPARE (errno, ENOENT);
> +  TEST_VERIFY (cwd == NULL);

Maybe also add this?

   cwd = realpath (".", NULL);
   TEST_VERIFY (cwd == NULL);
   TEST_COMPARE (errno, ENOENT);

I assume that we expect to fail realpath with ENOENT as well.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Dmitry V. Levin
On Fri, Jan 12, 2018 at 12:44:17AM +0100, Florian Weimer wrote:
> On 01/11/2018 11:03 PM, Dmitry V. Levin wrote:
> > +  char *cwd = getcwd (NULL, 0);
> > +  TEST_COMPARE (errno, ENOENT);
> > +  TEST_VERIFY (cwd == NULL);
>
> Maybe also add this?
>
>    cwd = realpath (".", NULL);

I don't mind adding this, but where do we stop?
This is a test of getcwd, after all.

>    TEST_VERIFY (cwd == NULL);
>    TEST_COMPARE (errno, ENOENT);

The check for errno should go first because TEST_VERIFY potentially
clobbers errno (it invokes printf).

> I assume that we expect to fail realpath with ENOENT as well.

Sure.  Would you be happy with the following amendment to the test?

@@ -36,10 +36,19 @@ static void
 getcwd_callback (void *closure)
 {
   xchroot (chroot_dir);
+
   errno = 0;
   char *cwd = getcwd (NULL, 0);
   TEST_COMPARE (errno, ENOENT);
   TEST_VERIFY (cwd == NULL);
+  free (cwd);
+
+  errno = 0;
+  cwd = realpath (".", NULL);
+  TEST_COMPARE (errno, ENOENT);
+  TEST_VERIFY (cwd == NULL);
+  free (cwd);
+
   _exit (0);
 }
 

--
ldv

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4] linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

Florian Weimer-5
In reply to this post by Dmitry V. Levin
On 01/11/2018 11:03 PM, Dmitry V. Levin wrote:
> [BZ #22679]
> CVE-2018-1000001
> * sysdeps/unix/sysv/linux/getcwd.c (__getcwd): Fall back to
> generic_getcwd if the path returned by getcwd syscall is not absolute.
> * sysdeps/unix/sysv/linux/tst-getcwd-abspath.c: New test.
> * sysdeps/unix/sysv/linux/Makefile (tests): Add tst-getcwd-abspath.

The patch as such looks good to me.  The test case should go into the
top-level io directory, where getcwd resides.  I don't think it is
Linux-specific.  Can you move it and commit it?

I still think we should have a realpath test as well, but that should
delaying committing this fix.

Thanks,
Florian