Re: [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl

[Aleksa Sarai]

When opening the slave end of a PTY, it is not possible for userspace to
safely ensure that /dev/pts/$num is actually a slave (in cases where the
mount namespace in which devpts was mounted is controlled by an
untrusted process). In addition, there are several unresolvable
race conditions if userspace were to attempt to detect attacks through
stat(2) and other similar methods [in addition it is not clear how
userspace could detect attacks involving FUSE].

Resolve this by providing an interface for userpace to safely open the
"peer" end of a PTY file descriptor by using the dentry cached by
devpts. Since it is not possible to have an open master PTY without
having its slave exposed in /dev/pts this interface is safe. This
interface currently does not provide a way to get the master pty (since
it is not clear whether such an interface is safe or even useful).

Cc: Christian Brauner 
Cc: Valentin Rothberg 
Signed-off-by: Aleksa Sarai 


Is this going to be documented anywhere?  Is there a man page update
that also goes along with this?


I will add one, I didn't know where the man-pages project is hosted / where
patches get pushed? What is the ML?


 From the MAINTAINERS file:
   MAN-PAGES: MANUAL PAGES FOR LINUX -- Sections 2, 3, 4, 5, and 7
   M:  Michael Kerrisk 
   W:  http://www.kernel.org/doc/man-pages
   L:  linux-...@vger.kernel.org
   S:  Maintained


Ah, should've looked there first!

Thanks Greg, I'll send it over the weekend.

--
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

Re: [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl

[Greg Kroah-Hartman]

On Fri, Jun 09, 2017 at 07:50:43PM +1000, Aleksa Sarai wrote:
> > > When opening the slave end of a PTY, it is not possible for userspace to
> > > safely ensure that /dev/pts/$num is actually a slave (in cases where the
> > > mount namespace in which devpts was mounted is controlled by an
> > > untrusted process). In addition, there are several unresolvable
> > > race conditions if userspace were to attempt to detect attacks through
> > > stat(2) and other similar methods [in addition it is not clear how
> > > userspace could detect attacks involving FUSE].
> > > 
> > > Resolve this by providing an interface for userpace to safely open the
> > > "peer" end of a PTY file descriptor by using the dentry cached by
> > > devpts. Since it is not possible to have an open master PTY without
> > > having its slave exposed in /dev/pts this interface is safe. This
> > > interface currently does not provide a way to get the master pty (since
> > > it is not clear whether such an interface is safe or even useful).
> > > 
> > > Cc: Christian Brauner 
> > > Cc: Valentin Rothberg 
> > > Signed-off-by: Aleksa Sarai 
> > 
> > Is this going to be documented anywhere?  Is there a man page update
> > that also goes along with this?
> 
> I will add one, I didn't know where the man-pages project is hosted / where
> patches get pushed? What is the ML?

>From the MAINTAINERS file:
  MAN-PAGES: MANUAL PAGES FOR LINUX -- Sections 2, 3, 4, 5, and 7
  M:  Michael Kerrisk 
  W:  http://www.kernel.org/doc/man-pages
  L:  linux-...@vger.kernel.org
  S:  Maintained

> > What userspace program wants to use this?
> 
> LXC (Christian is on Cc) will use this, runC will most likely use it,
> pending on some design discussions (as well as some future container
> runtimes I'm planning on working on). Effectively any container runtime that
> wants to safely create terminals and spawn containers inside an existing
> container's namespaces will likely want to use this.
> 
> [ As an aside, I /would/ argue this is a security fix (it fixes an interface
> problem that made doing certain operations securely possible) but I didn't
> want to Cc stable@ because it's a feature and not a strict bugfix. ]

Yeah, it's a new feature, so stable doesn't really fit here.  And as
people who use containers are all keeping up to date with their kernel
versions, this shouldn't be that big of a deal, not like the Android
kernel mess :)

thanks,

greg k-h

Re: [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl

[Aleksa Sarai]

When opening the slave end of a PTY, it is not possible for userspace to
safely ensure that /dev/pts/$num is actually a slave (in cases where the
mount namespace in which devpts was mounted is controlled by an
untrusted process). In addition, there are several unresolvable
race conditions if userspace were to attempt to detect attacks through
stat(2) and other similar methods [in addition it is not clear how
userspace could detect attacks involving FUSE].

Resolve this by providing an interface for userpace to safely open the
"peer" end of a PTY file descriptor by using the dentry cached by
devpts. Since it is not possible to have an open master PTY without
having its slave exposed in /dev/pts this interface is safe. This
interface currently does not provide a way to get the master pty (since
it is not clear whether such an interface is safe or even useful).

Cc: Christian Brauner 
Cc: Valentin Rothberg 
Signed-off-by: Aleksa Sarai 


Is this going to be documented anywhere?  Is there a man page update
that also goes along with this?


I will add one, I didn't know where the man-pages project is hosted / 
where patches get pushed? What is the ML?



What userspace program wants to use this?


LXC (Christian is on Cc) will use this, runC will most likely use it, 
pending on some design discussions (as well as some future container 
runtimes I'm planning on working on). Effectively any container runtime 
that wants to safely create terminals and spawn containers inside an 
existing container's namespaces will likely want to use this.


[ As an aside, I /would/ argue this is a security fix (it fixes an 
interface problem that made doing certain operations securely possible) 
but I didn't want to Cc stable@ because it's a feature and not a strict 
bugfix. ]


--
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

Re: [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl

[Greg Kroah-Hartman]

On Sun, Jun 04, 2017 at 12:15:15AM +1000, Aleksa Sarai wrote:
> When opening the slave end of a PTY, it is not possible for userspace to
> safely ensure that /dev/pts/$num is actually a slave (in cases where the
> mount namespace in which devpts was mounted is controlled by an
> untrusted process). In addition, there are several unresolvable
> race conditions if userspace were to attempt to detect attacks through
> stat(2) and other similar methods [in addition it is not clear how
> userspace could detect attacks involving FUSE].
> 
> Resolve this by providing an interface for userpace to safely open the
> "peer" end of a PTY file descriptor by using the dentry cached by
> devpts. Since it is not possible to have an open master PTY without
> having its slave exposed in /dev/pts this interface is safe. This
> interface currently does not provide a way to get the master pty (since
> it is not clear whether such an interface is safe or even useful).
> 
> Cc: Christian Brauner 
> Cc: Valentin Rothberg 
> Signed-off-by: Aleksa Sarai 

Is this going to be documented anywhere?  Is there a man page update
that also goes along with this?  What userspace program wants to use
this?

I'm not objecting to this, I just want to know that people will use
this, and that they can find out information about it if they want to.

thanks,

greg k-h

[PATCH v4 2/2] tty: add TIOCGPTPEER ioctl

[Aleksa Sarai]

When opening the slave end of a PTY, it is not possible for userspace to
safely ensure that /dev/pts/$num is actually a slave (in cases where the
mount namespace in which devpts was mounted is controlled by an
untrusted process). In addition, there are several unresolvable
race conditions if userspace were to attempt to detect attacks through
stat(2) and other similar methods [in addition it is not clear how
userspace could detect attacks involving FUSE].

Resolve this by providing an interface for userpace to safely open the
"peer" end of a PTY file descriptor by using the dentry cached by
devpts. Since it is not possible to have an open master PTY without
having its slave exposed in /dev/pts this interface is safe. This
interface currently does not provide a way to get the master pty (since
it is not clear whether such an interface is safe or even useful).

Cc: Christian Brauner 
Cc: Valentin Rothberg 
Signed-off-by: Aleksa Sarai 
---
 arch/alpha/include/uapi/asm/ioctls.h   |  1 +
 arch/mips/include/uapi/asm/ioctls.h|  1 +
 arch/parisc/include/uapi/asm/ioctls.h  |  1 +
 arch/powerpc/include/uapi/asm/ioctls.h |  1 +
 arch/sh/include/uapi/asm/ioctls.h  |  1 +
 arch/sparc/include/uapi/asm/ioctls.h   |  3 +-
 arch/xtensa/include/uapi/asm/ioctls.h  |  1 +
 drivers/tty/pty.c  | 71 --
 include/uapi/asm-generic/ioctls.h  |  1 +
 9 files changed, 76 insertions(+), 5 deletions(-)

diff --git a/arch/alpha/include/uapi/asm/ioctls.h 
b/arch/alpha/include/uapi/asm/ioctls.h
index f30c94ae1bdb..ff67b8373bf7 100644
--- a/arch/alpha/include/uapi/asm/ioctls.h
+++ b/arch/alpha/include/uapi/asm/ioctls.h
@@ -100,6 +100,7 @@
 #define TIOCGPKT   _IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL  _IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER_IOR('T', 0x41, int) /* Safely open the slave */
 
 #define TIOCSERCONFIG  0x5453
 #define TIOCSERGWILD   0x5454
diff --git a/arch/mips/include/uapi/asm/ioctls.h 
b/arch/mips/include/uapi/asm/ioctls.h
index 740219c2c894..68e19b689a00 100644
--- a/arch/mips/include/uapi/asm/ioctls.h
+++ b/arch/mips/include/uapi/asm/ioctls.h
@@ -91,6 +91,7 @@
 #define TIOCGPKT   _IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL  _IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER_IOR('T', 0x41, int) /* Safely open the slave */
 
 /* I hope the range from 0x5480 on is free ... */
 #define TIOCSCTTY  0x5480  /* become controlling tty */
diff --git a/arch/parisc/include/uapi/asm/ioctls.h 
b/arch/parisc/include/uapi/asm/ioctls.h
index b6572f051b67..674c68a5bbd0 100644
--- a/arch/parisc/include/uapi/asm/ioctls.h
+++ b/arch/parisc/include/uapi/asm/ioctls.h
@@ -60,6 +60,7 @@
 #define TIOCGPKT   _IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL  _IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER_IOR('T', 0x41, int) /* Safely open the slave */
 
 #define FIONCLEX   0x5450  /* these numbers need to be adjusted. */
 #define FIOCLEX0x5451
diff --git a/arch/powerpc/include/uapi/asm/ioctls.h 
b/arch/powerpc/include/uapi/asm/ioctls.h
index 49a25796a61a..bfd609a3e928 100644
--- a/arch/powerpc/include/uapi/asm/ioctls.h
+++ b/arch/powerpc/include/uapi/asm/ioctls.h
@@ -100,6 +100,7 @@
 #define TIOCGPKT   _IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL  _IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER_IOR('T', 0x41, int) /* Safely open the slave */
 
 #define TIOCSERCONFIG  0x5453
 #define TIOCSERGWILD   0x5454
diff --git a/arch/sh/include/uapi/asm/ioctls.h 
b/arch/sh/include/uapi/asm/ioctls.h
index c9903e56ccf4..eec7901e9e65 100644
--- a/arch/sh/include/uapi/asm/ioctls.h
+++ b/arch/sh/include/uapi/asm/ioctls.h
@@ -93,6 +93,7 @@
 #define TIOCGPKT   _IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL  _IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER_IOR('T', 0x41, int) /* Safely open the slave */
 
 #define TIOCSERCONFIG  _IO('T', 83) /* 0x5453 */
 #define TIOCSERGWILD   _IOR('T', 84,  int) /* 0x5454 */
diff --git a/arch/sparc/include/uapi/asm/ioctls.h 
b/arch/sparc/include/uapi/asm/ioctls.h
index 06b3f6c3bb9a..6d27398632ea 100644
--- a/arch/sparc/include/uapi/asm/ioctls.h
+++ b/arch/sparc/include/uapi/asm/ioctls.h
@@ -27,7 +27,7 @@
 #define TIOCGRS485 _IOR('T', 0x41, struct serial_rs485)
 #define TIOCSRS485 _IOWR('T', 0x42, struct serial_rs485)
 
-/* Note