| Message ID | 20160415152929.GA7932@ubuntumail |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 --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))
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(+)