diff mbox

[1/1] linux ttyname: return link if appropriate

Message ID 20160415152929.GA7932@ubuntumail
State Superseded
Headers

Commit Message

Serge Hallyn April 15, 2016, 3:29 p.m. UTC 
  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.

Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
 sysdeps/unix/sysv/linux/ttyname.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Florian Weimer April 15, 2016, 4:27 p.m. UTC | #1 
On 04/15/2016 05:29 PM, Serge Hallyn wrote:
> 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.

Is this the first change?

> 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.

Is it safe to drop the verification that ttyname ordinarily would do?

ttyname_r will need a similar change.

Florian
Serge Hallyn April 15, 2016, 4:46 p.m. UTC | #2 
Quoting Florian Weimer (fweimer@redhat.com):
> On 04/15/2016 05:29 PM, Serge Hallyn wrote:
> >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.
> 
> Is this the first change?

Right, it ensures that the filesystem of the two files is
the same.

> >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.
> 
> Is it safe to drop the verification that ttyname ordinarily would do?

Which verification do you mean exactly?

> ttyname_r will need a similar change.

Oh, yeah, it will.

thanks,
-serge
Florian Weimer April 15, 2016, 5:06 p.m. UTC | #3 
On 04/15/2016 06:46 PM, Serge Hallyn wrote:
> Quoting Florian Weimer (fweimer@redhat.com):
>> On 04/15/2016 05:29 PM, Serge Hallyn wrote:
>>> 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.
>>
>> Is this the first change?
>
> Right, it ensures that the filesystem of the two files is
> the same.
>
>>> 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.
>>
>> Is it safe to drop the verification that ttyname ordinarily would do?
>
> Which verification do you mean exactly?

That the file descriptor actually belongs to a PTY device listed under 
/dev/pts.

>> ttyname_r will need a similar change.
>
> Oh, yeah, it will.

Please also fix the stylistic issues (GNU style requires a space in 
function calls, braces on their own lines etc.).

But I don't think we can make the change, considering the security 
implications.

Florian
Serge Hallyn April 15, 2016, 5:42 p.m. UTC | #4 
Quoting Florian Weimer (fweimer@redhat.com):
> On 04/15/2016 06:46 PM, Serge Hallyn wrote:
> >Quoting Florian Weimer (fweimer@redhat.com):
> >>On 04/15/2016 05:29 PM, Serge Hallyn wrote:
> >>>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.
> >>
> >>Is this the first change?
> >
> >Right, it ensures that the filesystem of the two files is
> >the same.
> >
> >>>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.
> >>
> >>Is it safe to drop the verification that ttyname ordinarily would do?
> >
> >Which verification do you mean exactly?
> 
> That the file descriptor actually belongs to a PTY device listed
> under /dev/pts.

Oh, yeah.  I think that adding a chck that this is a pts (using st_rdev)
before returning "/proc/self/fd/N" (in my newly added block) would be good.

> >>ttyname_r will need a similar change.
> >
> >Oh, yeah, it will.
> 
> Please also fix the stylistic issues (GNU style requires a space in
> function calls, braces on their own lines etc.).

Oh, yes I can update those.

> But I don't think we can make the change, considering the security
> implications.

What security implications exactly?  Does ensuring that it is a
device on a devpts filesystem address them?

-serge
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..b0500a9 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -170,12 +170,21 @@  ttyname (int fd)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
+	  && st1.st_dev == st.st_dev
 #else
 	  && 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 dvice in another
+       * namespace.  We've already verified it's a tty.  Just return the
+       * link itself. */
+      if (strlen(procname) < buflen - 1) {
+         memcpy(ttyname_buf, procname, strlen(procname));
+         ttyname_buf[strlen(procname)] = '\0';
+         return ttyname_buf;
+      }
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))