New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't run tmux from `lxc exec ... bash` #936

Closed
cmars opened this Issue Jul 31, 2015 · 18 comments

Comments

6 participants
@cmars
Contributor

cmars commented Jul 31, 2015

When I try to run tmux from an interactive shell in a container with the LXD client, the process exits (rc=1) immediately.

Steps to reproduce (with a trusty lxc container named foo):

cmars@flatland:~$ lxc exec foo bash
root@foo:~# tmux
root@foo:~# echo $?
1

Version info:

cmars@flatland:~$ dpkg -l | grep lxd
ii  lxd                                         0.14-0ubuntu3~ubuntu14.04.1~ppa1        amd64        Container hypervisor based on LXC - daemon
ii  lxd-client                                  0.14-0ubuntu3~ubuntu14.04.1~ppa1        amd64        Container hypervisor based on LXC - client
@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Jul 31, 2015

Member

The reason for this is that the terminal owner is a tty which was created on the host, so outside of the scope of the container's devpts. It's not obvious how to fix this and even whether we should attempt to fix this.

We can't just pass the pts inside the container since we're crossing devpts instances.
And we can't create the pts pair inside the container because a malicious user could provide a fake /dev/pts inside the container, returning us fds they have control over and so having tty control over the host.

So as it stands, it looks like the easiest fix for this would be to teach tmux and screen that not being able to reach the pts device isn't very important and that they can just do their ioctls directly on stdout and not care where it's pointing at.

Member

stgraber commented Jul 31, 2015

The reason for this is that the terminal owner is a tty which was created on the host, so outside of the scope of the container's devpts. It's not obvious how to fix this and even whether we should attempt to fix this.

We can't just pass the pts inside the container since we're crossing devpts instances.
And we can't create the pts pair inside the container because a malicious user could provide a fake /dev/pts inside the container, returning us fds they have control over and so having tty control over the host.

So as it stands, it looks like the easiest fix for this would be to teach tmux and screen that not being able to reach the pts device isn't very important and that they can just do their ioctls directly on stdout and not care where it's pointing at.

@cmars

This comment has been minimized.

Show comment
Hide comment
@cmars

cmars Aug 3, 2015

Contributor

That last bit got me to thinking.. there's got to be a way to trick tmux into doing the ioctls... and it turns out there is!

script -qfc tmux /dev/null

Launches tmux just fine in the LXD client.

Contributor

cmars commented Aug 3, 2015

That last bit got me to thinking.. there's got to be a way to trick tmux into doing the ioctls... and it turns out there is!

script -qfc tmux /dev/null

Launches tmux just fine in the LXD client.

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Aug 27, 2015

Member

@hallyn any idea of how we could solve this in a sane way?

Member

stgraber commented Aug 27, 2015

@hallyn any idea of how we could solve this in a sane way?

@stgraber stgraber added the Bug label Aug 27, 2015

@stgraber stgraber added this to the later milestone Aug 27, 2015

@hackerfriendly

This comment has been minimized.

Show comment
Hide comment
@hackerfriendly

hackerfriendly Sep 18, 2015

Why not do what script does and generate a new pts when requested? It's not important that the original pts gets passed, just that one is allocated inside the container.

Check out how it's done in script.c. It looks pretty straightforward:

http://packages.ubuntu.com/trusty/utils/bsdutils

OpenSSH offers similar functionality with 'ssh -t' to force allocation of a pseudo tty.

hackerfriendly commented Sep 18, 2015

Why not do what script does and generate a new pts when requested? It's not important that the original pts gets passed, just that one is allocated inside the container.

Check out how it's done in script.c. It looks pretty straightforward:

http://packages.ubuntu.com/trusty/utils/bsdutils

OpenSSH offers similar functionality with 'ssh -t' to force allocation of a pseudo tty.

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Sep 18, 2015

Member

Because allocating a pts inside a container means trusting /dev/pts inside that container which we can't since it's under the user's control.

Member

stgraber commented Sep 18, 2015

Because allocating a pts inside a container means trusting /dev/pts inside that container which we can't since it's under the user's control.

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Sep 18, 2015

Member

If the user does it instead of LXD, it's fine because by that point we're into the LXC code which will have dropped all capabilities making this safe, but trusting a user controlled filesystem from a root process on the host (lxd) is a big no-no.

Member

stgraber commented Sep 18, 2015

If the user does it instead of LXD, it's fine because by that point we're into the LXC code which will have dropped all capabilities making this safe, but trusting a user controlled filesystem from a root process on the host (lxd) is a big no-no.

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Sep 18, 2015

Member

It may be possible to do this inside liblxc by adding an extra attach flag for this which would cause liblxc to attempt to open a pts pair after it's done attaching to the container and has dropped privileges. That would likely require an API change to liblxc though so not something we can very easily do.

Member

stgraber commented Sep 18, 2015

It may be possible to do this inside liblxc by adding an extra attach flag for this which would cause liblxc to attempt to open a pts pair after it's done attaching to the container and has dropped privileges. That would likely require an API change to liblxc though so not something we can very easily do.

@hackerfriendly

This comment has been minimized.

Show comment
Hide comment
@hackerfriendly

hackerfriendly Sep 18, 2015

Ah, I see. Thanks for the context.

hackerfriendly commented Sep 18, 2015

Ah, I see. Thanks for the context.

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Apr 25, 2016

Member

Closing this, the real fix is to fix glibc which @hallyn is doing upstream.

Member

stgraber commented Apr 25, 2016

Closing this, the real fix is to fix glibc which @hallyn is doing upstream.

@xliiv

This comment has been minimized.

Show comment
Hide comment
@xliiv

xliiv Dec 30, 2016

Is this fixed, because I've got similar problem:

[I] [20:08:49] xliiv@alef /tmp (0) 
> lxc --version
2.4.1
[I] [20:08:54] xliiv@alef /tmp (0) 
> lxc exec tmux-test -- bash
root@tmux-test:~# tmux
root@tmux-test:~# echo $?
1
root@tmux-test:~# 

xliiv commented Dec 30, 2016

Is this fixed, because I've got similar problem:

[I] [20:08:49] xliiv@alef /tmp (0) 
> lxc --version
2.4.1
[I] [20:08:54] xliiv@alef /tmp (0) 
> lxc exec tmux-test -- bash
root@tmux-test:~# tmux
root@tmux-test:~# echo $?
1
root@tmux-test:~# 
@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Dec 30, 2016

Member

The change hasn't been merged in glibc yet and once it is, it will most likely take a year or so for new Linux distribution releases to pick up the fix.

Member

stgraber commented Dec 30, 2016

The change hasn't been merged in glibc yet and once it is, it will most likely take a year or so for new Linux distribution releases to pick up the fix.

@techtonik

This comment has been minimized.

Show comment
Hide comment
@techtonik

techtonik Dec 30, 2016

Contributor

Is there a link to track?

Contributor

techtonik commented Dec 30, 2016

Is there a link to track?

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Dec 30, 2016

Member

Not really, the patch was written by @hallyn and further updated by @brauner and is currently on hold while signature of the FSF CLA is being done. Discussions can be found on the glibc development mailing-list (libc-alpha).

Member

stgraber commented Dec 30, 2016

Not really, the patch was written by @hallyn and further updated by @brauner and is currently on hold while signature of the FSF CLA is being done. Discussions can be found on the glibc development mailing-list (libc-alpha).

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Mar 26, 2017

Member

So just to keep people up to date. We managed to get a version of this patch merged with glibc as of a couple of days ago. The problem is that the patch in its current version is not sufficient. What is required are patches to literally all downstream programs that error out !ttyname(fd). All of these now need to be updated to handle

char *tty_name = ttyname(fd)
if (!tty_name && errno == ENODEV)
       // do stuff

Programs that do not actually perform any checks on the returned tty name should be fine and can just proceed without caring about the empty path returned by ttyname(). Other programs need to be updated to operate on the symbolic link itself. This obviously needs some care but it should certainly possible. I'm currently in the process of handling a couple of those programs and I hope that in a few months this will all be sorted out. I say "months" because I can't predict how fast the various upstreams will be in reviewing and merging these patches and how much (valid) arguments we need to have around operating on the symlink itself. In any case, I'm tracking this.

Member

brauner commented Mar 26, 2017

So just to keep people up to date. We managed to get a version of this patch merged with glibc as of a couple of days ago. The problem is that the patch in its current version is not sufficient. What is required are patches to literally all downstream programs that error out !ttyname(fd). All of these now need to be updated to handle

char *tty_name = ttyname(fd)
if (!tty_name && errno == ENODEV)
       // do stuff

Programs that do not actually perform any checks on the returned tty name should be fine and can just proceed without caring about the empty path returned by ttyname(). Other programs need to be updated to operate on the symbolic link itself. This obviously needs some care but it should certainly possible. I'm currently in the process of handling a couple of those programs and I hope that in a few months this will all be sorted out. I say "months" because I can't predict how fast the various upstreams will be in reviewing and merging these patches and how much (valid) arguments we need to have around operating on the symlink itself. In any case, I'm tracking this.

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Apr 4, 2017

Member

GLIBC

The required patch is already merged.

commit 15e9a4f378c8607c2ae1aa465436af4321db0e23
Author: Christian Brauner <christian.brauner@canonical.com>
Date:   Fri Jan 27 15:59:59 2017 +0100

    linux ttyname and ttyname_r: do not return wrong results

    If a link (say /proc/self/fd/0) pointing to a device, say /dev/pts/2, in a
    parent mount namespace is passed to ttyname, 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.

    Detect this case and return ENODEV.  Userspace can choose to take this as a hint
    that the fd points to a tty device but to act on the fd rather than the link.

    Signed-off-by: Serge Hallyn <serge@hallyn.com>
    Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

SCREEN

The patches needed for screen can now be tracked upstream:

TMUX

The patches needed for tmux can now be tracked upstream:

Member

brauner commented Apr 4, 2017

GLIBC

The required patch is already merged.

commit 15e9a4f378c8607c2ae1aa465436af4321db0e23
Author: Christian Brauner <christian.brauner@canonical.com>
Date:   Fri Jan 27 15:59:59 2017 +0100

    linux ttyname and ttyname_r: do not return wrong results

    If a link (say /proc/self/fd/0) pointing to a device, say /dev/pts/2, in a
    parent mount namespace is passed to ttyname, 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.

    Detect this case and return ENODEV.  Userspace can choose to take this as a hint
    that the fd points to a tty device but to act on the fd rather than the link.

    Signed-off-by: Serge Hallyn <serge@hallyn.com>
    Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

SCREEN

The patches needed for screen can now be tracked upstream:

TMUX

The patches needed for tmux can now be tracked upstream:

@techtonik

This comment has been minimized.

Show comment
Hide comment
@techtonik

techtonik Apr 16, 2017

Contributor

Is there any good reading with picture that can explain this fd/ttyname stuff and why tmux is failing?

Contributor

techtonik commented Apr 16, 2017

Is there any good reading with picture that can explain this fd/ttyname stuff and why tmux is failing?

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Apr 16, 2017

Member

tmux should work out of the box now. Ubuntu Zesty (17.04) already has a fixed version. This tmux/tmux@9b28200 is required. Otherwise I suggest manpages and patience. :)

Member

brauner commented Apr 16, 2017

tmux should work out of the box now. Ubuntu Zesty (17.04) already has a fixed version. This tmux/tmux@9b28200 is required. Otherwise I suggest manpages and patience. :)

@techtonik

This comment has been minimized.

Show comment
Hide comment
@techtonik

techtonik Apr 16, 2017

Contributor

=) I was trying to fix this bug https://bitbucket.org/techtonik/python-pager/issues/7 for 4 years. Man pages are not edible without pictures.

Contributor

techtonik commented Apr 16, 2017

=) I was trying to fix this bug https://bitbucket.org/techtonik/python-pager/issues/7 for 4 years. Man pages are not edible without pictures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment