Message ID | 20160415152929.GA7932@ubuntumail |
---|---|
State | Superseded |
Headers |
Received: (qmail 56103 invoked by alias); 15 Apr 2016 15:29:47 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 56083 invoked by uid 89); 15 Apr 2016 15:29:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 spammy= X-HELO: youngberry.canonical.com Date: Fri, 15 Apr 2016 15:29:29 +0000 From: Serge Hallyn <serge.hallyn@ubuntu.com> To: libc-alpha@sourceware.org Cc: Serge Hallyn <serge.hallyn@ubuntu.com> Subject: [PATCH 1/1] linux ttyname: return link if appropriate Message-ID: <20160415152929.GA7932@ubuntumail> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) |
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
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))