linux ttyname and ttyname_r: return link if appropriate
Commit Message
Quoting Dmitry V. Levin (ldv@altlinux.org):
> On Sat, Aug 06, 2016 at 10:00:02AM -0500, Serge E. Hallyn wrote:
> [...]
> > Subject: [PATCH] linux ttyname and ttyname_r: return link if appropriate
>
> Please compare
>
> > @@ -170,12 +184,17 @@ ttyname (int fd)
> > #ifdef _STATBUF_ST_RDEV
> > && S_ISCHR (st1.st_mode)
> > && st1.st_rdev == st.st_rdev
> > -#else
> > - && st1.st_ino == st.st_ino
> > - && st1.st_dev == st.st_dev
> > #endif
> > + && st1.st_dev == st.st_dev
> > + && st1.st_ino == st.st_ino
>
> with
>
> > @@ -152,12 +166,20 @@ __ttyname_r (int fd, char *buf, size_t buflen)
> > #ifdef _STATBUF_ST_RDEV
> > && S_ISCHR (st1.st_mode)
> > && st1.st_rdev == st.st_rdev
> > -#else
> > +#endif
> > && st1.st_ino == st.st_ino
> > && st1.st_dev == st.st_dev
> > -#endif
>
> Wouldn't it be better if both functions implemented these comparisons
> exactly the same way?
Subject: [PATCH] linux ttyname and ttyname_r: return link if appropriate
The current ttyname does the wrong thing in two cases:
1. If the passed-in link (say /proc/self/fd/0) points to a
device, say /dev/pts/2, in a parent mount namespace, and a
/dev/pts/2 exists (in a different devpts) in the current
namespace, then it returns /dev/pts/2. But /dev/pts/2 is
NOT the current tty, it is a different file and device.
2. If the passed-in link (say /proc/self/fd/0) points to
a device, say /dev/pts/2, in a parent mount namespace, and
/dev/pts/2 does not exist in the current namespace, it
returns success but an empty name. As far as I can tell,
there is no reason for it to not return /proc/self/fd/0.
http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
does not say anything about not returning a link.
---
sysdeps/unix/sysv/linux/ttyname.c | 23 +++++++++++++++++++++--
sysdeps/unix/sysv/linux/ttyname_r.c | 26 ++++++++++++++++++++++++--
2 files changed, 45 insertions(+), 4 deletions(-)
Comments
On Tue, Aug 09, 2016 at 04:39:37PM -0500, Serge E. Hallyn wrote:
[...]
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
[...]
> + /* If the link doesn't exist, then it points to a device in another
> + namespace. If it is a UNIX98 pty, then return the /proc/self
> + fd, as it points to a name unreachable in our namespace. */
> + if (is_pty (&st) && strlen (procname) < buflen - 1)
> + return strcpy (ttyname_buf, procname);
With buflen == 4095 and sizeof(procname) == 30, this buflen check looks
redundant. To keep the safe side, I'd rather add a static assert instead,
e.g.
#define TTYNAME_BUFLEN 4095
...
buflen = TTYNAME_BUFLEN;
...
_Static_assert (sizeof (procname) < TTYNAME_BUFLEN, "buflen too small");
[...]
> --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
[...]
> + /* If the link doesn't exist, then it points to a device in another
> + namespace. If it is a UNIX98 pty, then return the /proc/self
> + fd, as it points to a name unreachable in our namespace. */
> + if (is_pty (&st) && strlen (procname) < buflen - 1)
> + {
> + strcpy (buf, procname);
> + return 0;
> + }
Unlike ttyname.c, here buflen might be quite small, and this code skips
strlen (procname) == buflen - 1. Shouldn't it rather be
strlen (procname) < buflen ?
If is_pty(&st) is true but buflen is too small for procname, is there any
chance of finding the device using getttyname_r? Shouldn't ttyname_r fail
with ERANGE instead?
On 08/09/2016 11:39 PM, Serge E. Hallyn wrote:
> 2. If the passed-in link (say /proc/self/fd/0) points to
> a device, say /dev/pts/2, in a parent mount namespace, and
> /dev/pts/2 does not exist in the current namespace, it
> returns success but an empty name. As far as I can tell,
> there is no reason for it to not return /proc/self/fd/0.
I'm still concerned that returning something which is not a stable
reference to a fixed PTY could introduce security vulnerabilities
because some applications may assume that if ttyname succeeds, the
returned name is safe to chmod/chown (even if the descriptor is closed
beforehand).
Maybe this is small risk and is worth the change. It seems rather
unlikely that the process would close the descriptor and open a new one
in its place before the file system change.
Florian
Quoting Florian Weimer (fweimer@redhat.com):
> On 08/09/2016 11:39 PM, Serge E. Hallyn wrote:
> >2. If the passed-in link (say /proc/self/fd/0) points to
> >a device, say /dev/pts/2, in a parent mount namespace, and
> >/dev/pts/2 does not exist in the current namespace, it
> >returns success but an empty name. As far as I can tell,
> >there is no reason for it to not return /proc/self/fd/0.
>
> I'm still concerned that returning something which is not a stable
> reference to a fixed PTY could introduce security vulnerabilities
> because some applications may assume that if ttyname succeeds, the
> returned name is safe to chmod/chown
which (I think you know, but just to state it) it can be, but
> (even if the descriptor is
> closed beforehand).
Indeed, not, good point.
> Maybe this is small risk and is worth the change. It seems rather
> unlikely that the process would close the descriptor and open a new
> one in its place before the file system change.
... not inconceivable though, but i would expect most cases of ttyname
plus chown would be using 0/1/2, not a newly opened fd. This might be
worth a code search.
But, even if we decide that part is dangerous, the part where we do not
return /dev/pts/N when /proc/self/fd/M is from a different devpts mount
than the /dev/pts/N in caller's namespace is I think very important, and
should at least be separately applied.
On Wed, Aug 10, 2016 at 06:03:51PM -0500, Serge E. Hallyn wrote:
[...]
> But, even if we decide that part is dangerous, the part where we do not
> return /dev/pts/N when /proc/self/fd/M is from a different devpts mount
> than the /dev/pts/N in caller's namespace is I think very important, and
> should at least be separately applied.
I agree.
In that case, what should ttyname/ttyname_r set errno to? ENOTTY?
Quoting Dmitry V. Levin (ldv@altlinux.org):
> On Tue, Aug 09, 2016 at 04:39:37PM -0500, Serge E. Hallyn wrote:
> [...]
> > --- a/sysdeps/unix/sysv/linux/ttyname.c
> > +++ b/sysdeps/unix/sysv/linux/ttyname.c
> [...]
> > + /* If the link doesn't exist, then it points to a device in another
> > + namespace. If it is a UNIX98 pty, then return the /proc/self
> > + fd, as it points to a name unreachable in our namespace. */
> > + if (is_pty (&st) && strlen (procname) < buflen - 1)
> > + return strcpy (ttyname_buf, procname);
>
> With buflen == 4095 and sizeof(procname) == 30, this buflen check looks
> redundant. To keep the safe side, I'd rather add a static assert instead,
> e.g.
>
> #define TTYNAME_BUFLEN 4095
> ...
> buflen = TTYNAME_BUFLEN;
> ...
> _Static_assert (sizeof (procname) < TTYNAME_BUFLEN, "buflen too small");
>
> [...]
> > --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> > +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> [...]
> > + /* If the link doesn't exist, then it points to a device in another
> > + namespace. If it is a UNIX98 pty, then return the /proc/self
> > + fd, as it points to a name unreachable in our namespace. */
> > + if (is_pty (&st) && strlen (procname) < buflen - 1)
> > + {
> > + strcpy (buf, procname);
> > + return 0;
> > + }
>
> Unlike ttyname.c, here buflen might be quite small, and this code skips
> strlen (procname) == buflen - 1. Shouldn't it rather be
> strlen (procname) < buflen ?
Hm, yeah that's right.
> If is_pty(&st) is true but buflen is too small for procname, is there any
> chance of finding the device using getttyname_r? Shouldn't ttyname_r fail
> with ERANGE instead?
So you mean
if (is_pty (&st)) {
if (strlen (procname) < buflen) {
strcpy (buf, procname);
return 0;
}
return -ERANGE;
}
?
Quoting Dmitry V. Levin (ldv@altlinux.org):
> On Wed, Aug 10, 2016 at 06:03:51PM -0500, Serge E. Hallyn wrote:
> [...]
> > But, even if we decide that part is dangerous, the part where we do not
> > return /dev/pts/N when /proc/self/fd/M is from a different devpts mount
> > than the /dev/pts/N in caller's namespace is I think very important, and
> > should at least be separately applied.
>
> I agree.
> In that case, what should ttyname/ttyname_r set errno to? ENOTTY?
Ideally something like EXDEV, which updated userspace could recognize
as "shucks i'll just use /proc/self/fd/N", but that probably isn't
realistically do-able?
On Wed, Aug 10, 2016 at 06:24:30PM -0500, Serge E. Hallyn wrote:
> Quoting Dmitry V. Levin (ldv@altlinux.org):
> > On Tue, Aug 09, 2016 at 04:39:37PM -0500, Serge E. Hallyn wrote:
> > [...]
> > > --- a/sysdeps/unix/sysv/linux/ttyname.c
> > > +++ b/sysdeps/unix/sysv/linux/ttyname.c
> > [...]
> > > + /* If the link doesn't exist, then it points to a device in another
> > > + namespace. If it is a UNIX98 pty, then return the /proc/self
> > > + fd, as it points to a name unreachable in our namespace. */
> > > + if (is_pty (&st) && strlen (procname) < buflen - 1)
> > > + return strcpy (ttyname_buf, procname);
> >
> > With buflen == 4095 and sizeof(procname) == 30, this buflen check looks
> > redundant. To keep the safe side, I'd rather add a static assert instead,
> > e.g.
> >
> > #define TTYNAME_BUFLEN 4095
> > ...
> > buflen = TTYNAME_BUFLEN;
> > ...
> > _Static_assert (sizeof (procname) < TTYNAME_BUFLEN, "buflen too small");
> >
> > [...]
> > > --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> > > +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> > [...]
> > > + /* If the link doesn't exist, then it points to a device in another
> > > + namespace. If it is a UNIX98 pty, then return the /proc/self
> > > + fd, as it points to a name unreachable in our namespace. */
> > > + if (is_pty (&st) && strlen (procname) < buflen - 1)
> > > + {
> > > + strcpy (buf, procname);
> > > + return 0;
> > > + }
> >
> > Unlike ttyname.c, here buflen might be quite small, and this code skips
> > strlen (procname) == buflen - 1. Shouldn't it rather be
> > strlen (procname) < buflen ?
>
> Hm, yeah that's right.
>
> > If is_pty(&st) is true but buflen is too small for procname, is there any
> > chance of finding the device using getttyname_r? Shouldn't ttyname_r fail
> > with ERANGE instead?
>
> So you mean
>
> if (is_pty (&st)) {
> if (strlen (procname) < buflen) {
> strcpy (buf, procname);
> return 0;
> }
> return -ERANGE;
> }
>
> ?
Yes, something like that. In fact, it has to be
__set_errno (ERANGE);
return ERANGE;
@@ -25,6 +25,7 @@
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
+#include <sys/sysmacros.h>
#include <_itoa.h>
@@ -33,6 +34,19 @@
char *__ttyname;
#endif
+/* Return true if this is a UNIX98 pty device, as defined in
+ linux/Documentation/devices.txt. */
+static int
+is_pty (struct stat64 *sb)
+{
+#ifdef _STATBUF_ST_RDEV
+ int m = major (sb->st_rdev);
+ return (136 <= m && m <= 143);
+#else
+ return false;
+#endif
+}
+
static char *getttyname (const char *dev, dev_t mydev,
ino64_t myino, int save, int *dostat)
internal_function;
@@ -170,12 +184,17 @@ ttyname (int fd)
#ifdef _STATBUF_ST_RDEV
&& S_ISCHR (st1.st_mode)
&& st1.st_rdev == st.st_rdev
-#else
+#endif
&& st1.st_ino == st.st_ino
&& st1.st_dev == st.st_dev
-#endif
)
return ttyname_buf;
+
+ /* If the link doesn't exist, then it points to a device in another
+ namespace. If it is a UNIX98 pty, then return the /proc/self
+ fd, as it points to a name unreachable in our namespace. */
+ if (is_pty (&st) && strlen (procname) < buflen - 1)
+ return strcpy (ttyname_buf, procname);
}
if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
@@ -25,6 +25,7 @@
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
+#include <sys/sysmacros.h>
#include <_itoa.h>
@@ -32,6 +33,19 @@ static int getttyname_r (char *buf, size_t buflen,
dev_t mydev, ino64_t myino, int save,
int *dostat) internal_function;
+/* Return true if this is a UNIX98 pty device, as defined in
+ linux/Documentation/devices.txt. */
+static int
+is_pty (struct stat64 *sb)
+{
+#ifdef _STATBUF_ST_RDEV
+ int m = major (sb->st_rdev);
+ return (136 <= m && m <= 143);
+#else
+ return false;
+#endif
+}
+
static int
internal_function attribute_compat_text_section
getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
@@ -152,12 +166,20 @@ __ttyname_r (int fd, char *buf, size_t buflen)
#ifdef _STATBUF_ST_RDEV
&& S_ISCHR (st1.st_mode)
&& st1.st_rdev == st.st_rdev
-#else
+#endif
&& st1.st_ino == st.st_ino
&& st1.st_dev == st.st_dev
-#endif
)
return 0;
+
+ /* If the link doesn't exist, then it points to a device in another
+ namespace. If it is a UNIX98 pty, then return the /proc/self
+ fd, as it points to a name unreachable in our namespace. */
+ if (is_pty (&st) && strlen (procname) < buflen - 1)
+ {
+ strcpy (buf, procname);
+ return 0;
+ }
}
/* Prepare the result buffer. */