Message-ID: <20230626.0a8f70d4228e@gnoack.org>
Date:   Mon, 26 Jun 2023 20:13:59 +0200
From:   Günther Noack <gnoack3000@...il.com>
To:     Mickaël Salaün <mic@...ikod.net>
Cc:     Günther Noack <gnoack@...gle.com>,
        linux-security-module@...r.kernel.org, Jeff Xu <jeffxu@...gle.com>,
        Jorge Lucangeli Obes <jorgelo@...omium.org>,
        Allen Webb <allenwebb@...gle.com>,
        Dmitry Torokhov <dtor@...gle.com>,
        Paul Moore <paul@...l-moore.com>,
        Konstantin Meskhidze <konstantin.meskhidze@...wei.com>,
        linux-fsdevel@...r.kernel.org, Kees Cook <keescook@...omium.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Simon Brand <simon.brand@...tadigitale.de>,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 0/6] Landlock: ioctl support

Hello!

On Fri, Jun 23, 2023 at 06:37:25PM +0200, Mickaël Salaün wrote:
> On 23/06/2023 17:20, Günther Noack wrote:
> > ***Proposal***:
> > 
> >    I am in favor of *disabling* TIOCSTI in all Landlocked processes,
> >    if the Landlock policy handles the LANDLOCK_ACCESS_FS_IOCTL right.
> > 
> > If we want to do it in a backwards-compatible way, now would be the time to add
> > it to the patch set. :)
> 
> What would that not be backward compatible?

What I meant is that disabling TIOCSTI for Landlocked processes would
only be backwards compatible for Landlock users if they did a
conscious step for opting in to that feature, such as specifying that
"ioctl" should be a handled right.


> > As far as I can tell, there are no good legitimate use cases for TIOCSTI, and it
> > would fix the aforementioned sandbox escaping trick for a Landlocked process.
> > With the patch set as it is now, the only way to prevent that sandbox escape is
> > unfortunately to either (1) close the TTY file descriptors when enabling
> > Landlock, or (2) rely on an outside process to pass something else than a TTY
> > for FDs 0, 1 and 2.
> 
> What about calling setsid()?
> 
> Alternatively, seccomp could be used, even if it could block overlapping
> IOCTLs as well…

The possible approaches that I have seen kicked around are:

setsid()

  This does not work reliably from all processes, unfortunately.
  In particular, it does not work in the common case where a process
  gets started from a shell, without pipes or other bells and
  whistles.
  
  From the man page:

    setsid() creates a new session if the calling process is not a
    process group leader.

  Shells run subprocesses in their own process groups and if it is
  only one, it is its own process group leader (see credentials(7)).

  Such a process *could* of course run a subprocess and do it there,
  but it would potentially require architectural changes in some
  programs to do that, which de

seccomp-bpf

  That's a fallback solution, yes, although I am still hoping in the
  long run that we can get away without it.  The problem of tracking
  the available syscall numbers as I previously discussed it at [1]
  still exists: If the compiled program runs on a new kernel with a
  new syscall number, and is linked against a new version of glibc,
  that program might start doing this syscall but it can't tell apart
  in the seccomp filter whether this is to be blocked or not.

  It's a fundamentally flawed approach when linking against shared
  objects and running on unknown (future) Linux versions.

  [1] https://blog.gnoack.org/post/pledge-on-linux/

creating a new pseudo-terminal

  The cleaner solution suggested by Alan Cox in [2] is to create a new
  pseudo terminal and run the program within that pseudo terminal.

  This is also the technique used by programs like su (with the --pty
  flag) and sudo.  The man pages of both programs talk about it.

  Implementing this approach unfortunately also requires some
  architectural changes to the program doing that - it would also
  involve two processes again, one which keeps a reference to the tty
  which shovels data between the old and new terminal, and one which
  is sandboxed which only sees the pseudo-terminal.  The details are
  described in "Advanced Programming in the UNIX Environment", Chapter
  11.

  [2] https://lore.kernel.org/all/20170510212920.7f6bc5e6@alans-desktop/


I dislike all three of them for the Landlock use case:

 - Involving additional processes is a bigger change that the programs
   using Landlock would have to deal with.

 - Seccomp-BPF is a hack as well, due to the problem of having to
   track syscall numbers across platforms.

It might not be worth doing these things just to bridge the gap until
we have a more proper solution in Landlock.

For more entertaining/scary background reading, this search query in
the CVE database has links where you can see how the issue has been
dealt with in the past:

https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=TIOCSTI

The most recent one of these is in opendoas, the discussion in that
thread also has some interesting pointers (and the team ended up stuck
with the same three potential fixes which have similar problems for
them and which they have not implemented so far):
https://github.com/Duncaen/OpenDoas/issues/106



> > Does that sound reasonable?  If yes, I'd send an update to this patch set which
> > forbids TIOCSTI.
> 
> I agree that TIOCSTI is dangerous, but I don't see the rationale to add an
> exception for this specific IOCTL. I'm sure there are a lot of potentially
> dangerous IOCTLs out there, but from a kernel point of view, why should
> Landlock handle this one in a specific way?
> 
> Landlock should not define specific policies itself but let users manage
> that. Landlock should only restrict kernel features that *directly* enable
> to bypass its own restrictions (e.g. ptrace scope, FS topology changes when
> FS restrictions are in place).
> 
> I think we should instead focus on adding something like a
> landlock_inode_attr rule type to restrict IOCTLs defined by
> users/developers, and then extend it to make it possible to restrict already
> opened FDs as well.

After researching all the stuff above, I believe you are right - it's
better to leave this up to userspace and let them define the ioctls
that they actually need with an allow-listing approach, to reduce the
risk from obscure TTY ioctls.

Another point that also came up in the mail thread in [2] above was:
TIOCSTI is not the only way to do harm through the tty FD.  Another
one is TIOCLINUX (ioctl_console(2)) through its cut&paste
functionality, and who knows what other ioctls there might be.

So in that sense, I'm coming around to your approach of letting the
user define it in the next iteration of this ioctl patch set -- but
it will really be necessary to do that. o_O

–Günther

Powered by blists - more mailing lists