* Re: [RFC 00/20] ns: Introduce Time Namespace
From: Thomas Gleixner @ 2018-10-02 6:15 UTC (permalink / raw)
To: Andrey Vagin
Cc: Eric W. Biederman, Dmitry Safonov, linux-kernel@vger.kernel.org,
Dmitry Safonov, Adrian Reber, Andy Lutomirski, Christian Brauner,
Cyrill Gorcunov, H. Peter Anvin, Ingo Molnar, Jeff Dike,
Oleg Nesterov, Pavel Emelianov, Shuah Khan,
containers@lists.linux-foundation.org, criu@openvz.org,
linux-api@vger.kernel.org, x86@kernel.org, Alexey
In-Reply-To: <20181001232033.GA31324@outlook.office365.com>
On Mon, 1 Oct 2018, Andrey Vagin wrote:
> On Thu, Sep 27, 2018 at 11:41:49PM +0200, Thomas Gleixner wrote:
> > On Thu, 27 Sep 2018, Thomas Gleixner wrote:
> > > Add time skew via NTP/PTP into the picture and you might have to adjust
> > > timers as well, because you need to guarantee that they are not expiring
> > > early.
> > >
> > > I haven't looked through Dimitry's patches yet, but I don't see how this
> > > can work at all without introducing subtle issues all over the place.
> >
> > And just a quick scan tells me that this is broken. Timers will expire
> > early or late. The latter is acceptible to some extent, but larger delays
> > might come with surprise. Expiring early is an absolute nono.
>
> Do you mean that we have to adjust all timers after changing offset for
> CLOCK_MONOTONIC or CLOCK_BOOTTIME? Our idea is that offsets for
> monotonic and boot times will be set immediately after creating a time
> namespace before using any timers.
I explained that in detail in this thread, but it's not about the initial
setting of clock mono/boot before any timers have been armed.
It's about setting the offset or clock realtime (via settimeofday) when
timers are already armed. Also having a entirely different time domain,
e.g. separate NTP adjustments, makes that necessary.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC 00/20] ns: Introduce Time Namespace
From: Andrey Vagin @ 2018-10-01 23:20 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Eric W. Biederman, Dmitry Safonov, linux-kernel@vger.kernel.org,
Dmitry Safonov, Adrian Reber, Andy Lutomirski, Christian Brauner,
Cyrill Gorcunov, H. Peter Anvin, Ingo Molnar, Jeff Dike,
Oleg Nesterov, Pavel Emelianov, Shuah Khan,
containers@lists.linux-foundation.org, criu@openvz.org,
linux-api@vger.kernel.org, x86@kernel.org, Alexey
In-Reply-To: <alpine.DEB.2.21.1809272335430.8118@nanos.tec.linutronix.de>
On Thu, Sep 27, 2018 at 11:41:49PM +0200, Thomas Gleixner wrote:
> On Thu, 27 Sep 2018, Thomas Gleixner wrote:
> > Add time skew via NTP/PTP into the picture and you might have to adjust
> > timers as well, because you need to guarantee that they are not expiring
> > early.
> >
> > I haven't looked through Dimitry's patches yet, but I don't see how this
> > can work at all without introducing subtle issues all over the place.
>
> And just a quick scan tells me that this is broken. Timers will expire
> early or late. The latter is acceptible to some extent, but larger delays
> might come with surprise. Expiring early is an absolute nono.
Do you mean that we have to adjust all timers after changing offset for
CLOCK_MONOTONIC or CLOCK_BOOTTIME? Our idea is that offsets for
monotonic and boot times will be set immediately after creating a time
namespace before using any timers.
It is interesting to think what a use-case for changing these offsets
after creating timers. It may be useful for testing needs. A user sets a
timer in an hour and then change a clock offset forward and check that a
test application handles the timer properly.
>
> Thanks,
>
> tglx
>
^ permalink raw reply
* Re: Setting monotonic time?
From: Andrey Vagin @ 2018-10-01 20:51 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Thomas Gleixner, Dmitry Safonov, linux-kernel@vger.kernel.org,
Dmitry Safonov, Adrian Reber, Andy Lutomirski, Christian Brauner,
Cyrill Gorcunov, H. Peter Anvin, Ingo Molnar, Jeff Dike,
Oleg Nesterov, Pavel Emelianov, Shuah Khan,
containers@lists.linux-foundation.org, criu@openvz.org,
linux-api@vger.kernel.org, x86@kernel.org
In-Reply-To: <877ej2xc23.fsf_-_@xmission.com>
On Mon, Oct 01, 2018 at 11:15:32AM +0200, Eric W. Biederman wrote:
>
> In the context of process migration there is a simpler subproblem that I
> think it is worth exploring if we can do something about.
>
> For a cluster of machines all running with synchronized
> clocks. CLOCK_REALTIME matches. CLOCK_MONOTNIC does not match between
> machines. Not having a matching CLOCK_MONOTONIC prevents successful
> process migration between nodes in that cluster.
>
> Would it be possible to allow setting CLOCK_MONOTONIC at the very
> beginning of time? So that all of the nodes in a cluster can be in
> sync?
Here is a question about how to synchronize clocks between nodes. It
looks like we will need to have a working network for this, but a
network configuration may be non-trivial and it can require to run a few
processes which can use CLOCK_MONOTNIC...
>
> No change in skew just in offset for CLOCK_MONOTONIC.
>
> There are also dragons involved in coordinating things so that
> CLOCK_MONOTONIC gets set before CLOCK_MONOTONIC gets used. So I don't
> know if allowing CLOCK_MONOTONIC to be set would be practical but it
> seems work exploring all on it's own.
>
> Dmitry would setting CLOCK_MONOTONIC exactly once at boot time solve
> your problem that is you are looking at a time namespace to solve?
Process migration is only one of use-cases. Another use-case is
restoring from snapshots. It may be even more popular than process
migration. We can't guarantee that all snapshots will be done in one
cluster. For example, a user meets a bug, does a container snapshot and
attaches it to a bug report.
>
> Eric
^ permalink raw reply
* Re: [PATCH 0/3] namei: implement various scoping AT_* flags
From: James Morris @ 2018-10-01 20:14 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Jann Horn, cyphar, jlayton, Bruce Fields, Al Viro, Arnd Bergmann,
shuah, David Howells, Andy Lutomirski, christian,
Eric W. Biederman, Tycho Andersen, kernel list, linux-fsdevel,
linux-arch, linux-kselftest, dev, containers,
linux-security-module, Kees Cook, Linux API
In-Reply-To: <0ca12a6e-a86b-5d50-40b9-e76c1a4bc6a0@digikod.net>
[-- Attachment #1: Type: text/plain, Size: 6502 bytes --]
On Mon, 1 Oct 2018, Mickaël Salaün wrote:
> Another way to apply a security policy could be to tied it to a file
> descriptor, similarly to Capsicum, which could enable to create
> programmable (real) capabilities. This way, it would be possible to
> "wrap" a file descriptor with a Landlock program and use it with
> FD-based syscalls or pass it to other processes. This would not require
> changes to the FS subsystem, but only the Landlock LSM code. This isn't
> done yet but I plan to add this new way to restrict operations on file
> descriptors.
Very interesting!
This could possibly be an LSM which stacks/integrates with other LSMs to
enforce MAC of object capabilities.
>
> Anyway, for the use case you mentioned, the AT_BENEATH flag(s) should be
> simple to use and enough for now. We must be careful of the hardcoded
> policy though.
>
>
> >
> >> On 9/29/18 12:34, Aleksa Sarai wrote:
> >>> The need for some sort of control over VFS's path resolution (to avoid
> >>> malicious paths resulting in inadvertent breakouts) has been a very
> >>> long-standing desire of many userspace applications. This patchset is a
> >>> revival of Al Viro's old AT_NO_JUMPS[1] patchset with a few additions.
> >>>
> >>> The most obvious change is that AT_NO_JUMPS has been split as dicussed
> >>> in the original thread, along with a further split of AT_NO_PROCLINKS
> >>> which means that each individual property of AT_NO_JUMPS is now a
> >>> separate flag:
> >>>
> >>> * Path-based escapes from the starting-point using "/" or ".." are
> >>> blocked by AT_BENEATH.
> >>> * Mountpoint crossings are blocked by AT_XDEV.
> >>> * /proc/$pid/fd/$fd resolution is blocked by AT_NO_PROCLINKS (more
> >>> correctly it actually blocks any user of nd_jump_link() because it
> >>> allows out-of-VFS path resolution manipulation).
> >>>
> >>> AT_NO_JUMPS is now effectively (AT_BENEATH|AT_XDEV|AT_NO_PROCLINKS). At
> >>> Linus' suggestion in the original thread, I've also implemented
> >>> AT_NO_SYMLINKS which just denies _all_ symlink resolution (including
> >>> "proclink" resolution).
> >>>
> >>> An additional improvement was made to AT_XDEV. The original AT_NO_JUMPS
> >>> path didn't consider "/tmp/.." as a mountpoint crossing -- this patch
> >>> blocks this as well (feel free to ask me to remove it if you feel this
> >>> is not sane).
> >>>
> >>> Currently I've only enabled these for openat(2) and the stat(2) family.
> >>> I would hope we could enable it for basically every *at(2) syscall --
> >>> but many of them appear to not have a @flags argument and thus we'll
> >>> need to add several new syscalls to do this. I'm more than happy to send
> >>> those patches, but I'd prefer to know that this preliminary work is
> >>> acceptable before doing a bunch of copy-paste to add new sets of *at(2)
> >>> syscalls.
> >>>
> >>> One additional feature I've implemented is AT_THIS_ROOT (I imagine this
> >>> is probably going to be more contentious than the refresh of
> >>> AT_NO_JUMPS, so I've included it in a separate patch). The patch itself
> >>> describes my reasoning, but the shortened version of the premise is that
> >>> continer runtimes need to have a way to resolve paths within a
> >>> potentially malicious rootfs. Container runtimes currently do this in
> >>> userspace[2] which has implicit race conditions that are not resolvable
> >>> in userspace (or use fork+exec+chroot and SCM_RIGHTS passing which is
> >>> inefficient). AT_THIS_ROOT allows for per-call chroot-like semantics for
> >>> path resolution, which would be invaluable for us -- and the
> >>> implementation is basically identical to AT_BENEATH (except that we
> >>> don't return errors when someone actually hits the root).
> >>>
> >>> I've added some selftests for this, but it's not clear to me whether
> >>> they should live here or in xfstests (as far as I can tell there are no
> >>> other VFS tests in selftests, while there are some tests that look like
> >>> generic VFS tests in xfstests). If you'd prefer them to be included in
> >>> xfstests, let me know.
> >>>
> >>> [1]: https://lore.kernel.org/patchwork/patch/784221/
> >>> [2]: https://github.com/cyphar/filepath-securejoin
> >>>
> >>> Aleksa Sarai (3):
> >>> namei: implement O_BENEATH-style AT_* flags
> >>> namei: implement AT_THIS_ROOT chroot-like path resolution
> >>> selftests: vfs: add AT_* path resolution tests
> >>>
> >>> fs/fcntl.c | 2 +-
> >>> fs/namei.c | 158 ++++++++++++------
> >>> fs/open.c | 10 ++
> >>> fs/stat.c | 15 +-
> >>> include/linux/fcntl.h | 3 +-
> >>> include/linux/namei.h | 8 +
> >>> include/uapi/asm-generic/fcntl.h | 20 +++
> >>> include/uapi/linux/fcntl.h | 10 ++
> >>> tools/testing/selftests/Makefile | 1 +
> >>> tools/testing/selftests/vfs/.gitignore | 1 +
> >>> tools/testing/selftests/vfs/Makefile | 13 ++
> >>> tools/testing/selftests/vfs/at_flags.h | 40 +++++
> >>> tools/testing/selftests/vfs/common.sh | 37 ++++
> >>> .../selftests/vfs/tests/0001_at_beneath.sh | 72 ++++++++
> >>> .../selftests/vfs/tests/0002_at_xdev.sh | 54 ++++++
> >>> .../vfs/tests/0003_at_no_proclinks.sh | 50 ++++++
> >>> .../vfs/tests/0004_at_no_symlinks.sh | 49 ++++++
> >>> .../selftests/vfs/tests/0005_at_this_root.sh | 66 ++++++++
> >>> tools/testing/selftests/vfs/vfs_helper.c | 154 +++++++++++++++++
> >>> 19 files changed, 707 insertions(+), 56 deletions(-)
> >>> create mode 100644 tools/testing/selftests/vfs/.gitignore
> >>> create mode 100644 tools/testing/selftests/vfs/Makefile
> >>> create mode 100644 tools/testing/selftests/vfs/at_flags.h
> >>> create mode 100644 tools/testing/selftests/vfs/common.sh
> >>> create mode 100755 tools/testing/selftests/vfs/tests/0001_at_beneath.sh
> >>> create mode 100755 tools/testing/selftests/vfs/tests/0002_at_xdev.sh
> >>> create mode 100755 tools/testing/selftests/vfs/tests/0003_at_no_proclinks.sh
> >>> create mode 100755 tools/testing/selftests/vfs/tests/0004_at_no_symlinks.sh
> >>> create mode 100755 tools/testing/selftests/vfs/tests/0005_at_this_root.sh
> >>> create mode 100644 tools/testing/selftests/vfs/vfs_helper.c
> >>>
> >>
> >
> >
>
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 0/3] bpf: allow zero-initialising hash map seed
From: Daniel Borkmann @ 2018-10-01 19:12 UTC (permalink / raw)
To: Lorenz Bauer, ast; +Cc: netdev, linux-api
In-Reply-To: <20181001104509.24211-1-lmb@cloudflare.com>
On 10/01/2018 12:45 PM, Lorenz Bauer wrote:
> This patch set adds a new flag BPF_F_ZERO_SEED, which allows
> forcing the seed used by hash maps to zero. This makes
> it possible to write deterministic tests.
>
> Based on an off-list conversation with Alexei Starovoitov and
> Daniel Borkmann.
>
> Lorenz Bauer (3):
> bpf: allow zero-initializing hash map seed
> tools: sync linux/bpf.h
> tools: add selftest for BPF_F_ZERO_SEED
>
> include/uapi/linux/bpf.h | 2 +
> kernel/bpf/hashtab.c | 8 ++-
> tools/include/uapi/linux/bpf.h | 2 +
> tools/testing/selftests/bpf/test_maps.c | 67 +++++++++++++++++++++----
> 4 files changed, 66 insertions(+), 13 deletions(-)
>
Please respin with proper SoB for each patch and non-empty commit
description. I think patch 1 should also have a more elaborate
commit description on the use case for BPF_F_ZERO_SEED, and I
think also a better comment in the uapi header that this is only
meant for testing and not production use.
Thanks,
Daniel
^ permalink raw reply
* Re: Setting monotonic time?
From: Thomas Gleixner @ 2018-10-01 18:52 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Andrey Vagin, Dmitry Safonov, linux-kernel@vger.kernel.org,
Dmitry Safonov, Adrian Reber, Andy Lutomirski, Christian Brauner,
Cyrill Gorcunov, H. Peter Anvin, Ingo Molnar, Jeff Dike,
Oleg Nesterov, Pavel Emelianov, Shuah Khan,
containers@lists.linux-foundation.org, criu@openvz.org,
linux-api@vger.kernel.org, x86@kernel.org,
Alexey Dobriyan <ado>
In-Reply-To: <877ej2xc23.fsf_-_@xmission.com>
On Mon, 1 Oct 2018, Eric W. Biederman wrote:
> In the context of process migration there is a simpler subproblem that I
> think it is worth exploring if we can do something about.
>
> For a cluster of machines all running with synchronized
> clocks. CLOCK_REALTIME matches. CLOCK_MONOTNIC does not match between
> machines. Not having a matching CLOCK_MONOTONIC prevents successful
> process migration between nodes in that cluster.
>
> Would it be possible to allow setting CLOCK_MONOTONIC at the very
> beginning of time? So that all of the nodes in a cluster can be in
> sync?
>
> No change in skew just in offset for CLOCK_MONOTONIC.
>
> There are also dragons involved in coordinating things so that
> CLOCK_MONOTONIC gets set before CLOCK_MONOTONIC gets used. So I don't
> know if allowing CLOCK_MONOTONIC to be set would be practical but it
> seems work exploring all on it's own.
It's used very early on in the kernel, so that would be a major surprise
for many things including user space which has expectations on clock
monotonic.
It would be reasonably easy to add CLOCK_MONONOTIC_SYNC which can be set in
the way you described and then in name spaces make it possible to magically
map CLOCK_MONOTONIC to CLOCK_MONOTONIC_SYNC.
It still wouldn't allow to have different NTP/PTP time domains, but might
be a good start to address the main migration headaches.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Aleksa Sarai @ 2018-10-01 16:18 UTC (permalink / raw)
To: Jann Horn
Cc: Eric W. Biederman, Al Viro, jlayton, Bruce Fields, Arnd Bergmann,
shuah, David Howells, Andy Lutomirski, christian, Tycho Andersen,
kernel list, linux-fsdevel, linux-arch, linux-kselftest, dev,
containers, Linux API
In-Reply-To: <CAG48ez3ApZC5cGzTxyt1ej0CyhYPD4PUEf5nbkqPmNbO+daOXQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]
On 2018-10-01, Jann Horn <jannh@google.com> wrote:
> > If this is an issue for AT_THIS_ROOT, I believe this might also be an
> > issue for AT_BENEATH since they are effectively both using the same
> > nd->root trick (so you could similarly trick AT_BENEATH to not error
> > out). So we'd need to figure out how to solve this problem in order for
> > AT_BENEATH to be safe.
>
> Oh, wait, what? I think I didn't notice that the semantics of
> AT_BENEATH changed like that since the original posting of David
> Drysdale's O_BENEATH_ONLY patch
> (https://lore.kernel.org/lkml/1439458366-8223-2-git-send-email-drysdale@google.com/).
> David's patch had nice, straightforward semantics, blocking any form
> of upwards traversal. Why was that changed? Does anyone actually want
> to use paths that contain ".." with AT_BENEATH? I would strongly
> prefer something that blocks any use of "..".
>
> @Al: It looks like this already changed back when you posted
> https://lore.kernel.org/lkml/20170429220414.GT29622@ZenIV.linux.org.uk/
> ?
Yes, I copied the semantics from Al's patchset. I don't know why he felt
strongly that this was the best idea, but in my opinion allowing paths
like "a/../b/../c" seems like it's quite useful because otherwise you
wouldn't be able to operate on most distribution root filesystems (many
have symlinks that have ".." components). Looking at my own (openSUSE)
machine there are something like 100k such symlinks (~37% of symlinks on
my machine).
While I do understand that the easiest way of solving this problem is to
disallow ".." entirely with AT_BENEATH, given that support ".." has
utility, I would like to know whether it's actually not possible to have
this work safely.
> > Speaking naively, doesn't it make sense to invalidate the walk if a path
> > component was modified? Or is this something that would be far too
> > costly with little benefit? What if we do more aggressive nd->root
> > checks when resolving with AT_BENEATH or AT_THIS_ROOT (or if nd->root !=
> > current->mnt_ns->root)?
>
> It seems to me like doing that would basically require looking at each
> node in the path walk twice? And it'd be difficult to guarantee
> forward progress unless you're willing to do some fairly heavy
> locking.
I had another idea since I wrote my previous mail -- since the issue (at
least the way I understand it) is that ".." can "skip" over nd->root
because of the rename, what if we had some sort of is_descendant() check
within follow_dotdot()? (FWIW, ".." already has some pretty interesting
"hand-over-hand" locking semantics.) This should be effectively similar
to how prepend_path() deals with a path that is not reachable from @root
(I'm not sure if the locking is acceptable for the namei path though).
Since ".." with AT_THIS_ROOT (or AT_BENEATH) is not going to be the most
common component type (and we only need to do these checks for those
flags), I would think that the extra cost would not be _that_ awful.
Would this work?
> > You're right about this -- for C runtimes. In Go we cannot do a raw
> > clone() or fork() (if you do it manually with RawSyscall you'll end with
> > broken runtime state). So you're forced to do fork+exec (which then
> > means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
> > for CLONE_VFORK.
>
> If you insist on implementing every last bit of your code in Go, I guess.
Fair enough, though I believe this would affect most multi-threaded
programs as well (regardless of the language they're written in).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Andy Lutomirski @ 2018-10-01 14:28 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Jann Horn, Eric W. Biederman, jlayton, Bruce Fields, Al Viro,
Arnd Bergmann, shuah, David Howells, Andy Lutomirski, christian,
Tycho Andersen, kernel list, linux-fsdevel, linux-arch,
linux-kselftest, dev, containers, Linux API
In-Reply-To: <20181001054246.gfinmx3api7kjhmc@ryuk>
>>> Currently most container runtimes try to do this resolution in
>>> userspace[1], causing many potential race conditions. In addition, the
>>> "obvious" alternative (actually performing a {ch,pivot_}root(2))
>>> requires a fork+exec which is *very* costly if necessary for every
>>> filesystem operation involving a container.
>>
>> Wait. fork() I understand, but why exec? And actually, you don't need
>> a full fork() either, clone() lets you do this with some process parts
>> shared. And then you also shouldn't need to use SCM_RIGHTS, just keep
>> the file descriptor table shared. And why chroot()/pivot_root(),
>> wouldn't you want to use setns()?
>
> You're right about this -- for C runtimes. In Go we cannot do a raw
> clone() or fork() (if you do it manually with RawSyscall you'll end with
> broken runtime state). So you're forced to do fork+exec (which then
> means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
> for CLONE_VFORK.
I must admit that I’m not very sympathetic to the argument that “Go’s runtime model is incompatible with the simpler solution.”
Anyway, it occurs to me that the real problem is that setns() and chroot() are both overkill for this use case. What’s needed is to start your walk from /proc/pid-in-container/root, with two twists:
1. Do the walk as though rooted at a directory. This is basically just your AT_THIS_ROOT, but the footgun is avoided because the dirfd you use is from a foreign namespace, and, except for symlinks to absolute paths, no amount of .. racing is going to escape the *namespace*.
2. Avoid /proc. It’s not just the *links* — you really don’t want to walk into /proc/self. *Maybe* procfs is already careful enough when mounted in a namespace?
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Christian Brauner @ 2018-10-01 14:00 UTC (permalink / raw)
To: Jann Horn
Cc: cyphar, Eric W. Biederman, jlayton, Bruce Fields, Al Viro,
Arnd Bergmann, shuah, David Howells, Andy Lutomirski,
Tycho Andersen, kernel list, linux-fsdevel, linux-arch,
linux-kselftest, dev, containers, Linux API
In-Reply-To: <CAG48ez30WJhbsro2HOc_DR7V91M+hNFzBP5ogRMZaxbAORvqzg@mail.gmail.com>
On Sat, Sep 29, 2018 at 06:35:17PM +0200, Jann Horn wrote:
> +cc linux-api; please keep them in CC for future versions of the patch
>
> On Sat, Sep 29, 2018 at 4:29 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > The primary motivation for the need for this flag is container runtimes
> > which have to interact with malicious root filesystems in the host
> > namespaces. One of the first requirements for a container runtime to be
> > secure against a malicious rootfs is that they correctly scope symlinks
> > (that is, they should be scoped as though they are chroot(2)ed into the
> > container's rootfs) and ".."-style paths. The already-existing AT_XDEV
> > and AT_NO_PROCLINKS help defend against other potential attacks in a
> > malicious rootfs scenario.
>
> So, I really like the concept for patch 1 of this series (but haven't
> read the code yet); but I dislike this patch because of its footgun
> potential.
>
> If this patch landed as-is, the manpage would need some big warning
> labels. chroot() basically provides no security guarantees at all; and
> yes, that includes that if you do `chroot(...); chdir("/");
> open(attacker_controlled_path, ...);`, you can potentially end up
> opening a file outside the chroot. See
> https://github.com/QubesOS/qubes-secpack/blob/master/QSBs/qsb-014-2015.txt
> for an example, where Qubes OS did pretty much that, and ended up with
> a potentially exploitable security bug because of that, where one VM,
> while performing a file transfer into another VM, could write outside
> of the transfer target directory.
> The problem is what happens if a folder you are walking through is
> concurrently moved out of the chroot. Consider the following scenario:
>
> You attempt to open "C/../../etc/passwd" under the root "/A/B".
> Something else concurrently moves /A/B/C to /A/C. This can result in
> the following:
>
> 1. You start the path walk and reach /A/B/C.
> 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> 3. Your path walk follows the first ".." up into /A. This is outside
> the process root, but you never actually encountered the process root,
> so you don't notice.
> 4. Your path walk follows the second ".." up to /. Again, this is
> outside the process root, but you don't notice.
> 5. Your path walk walks down to /etc/passwd, and the open completes
> successfully. You now have an fd pointing outside your chroot.
>
> If the root of your walk is below an attacker-controlled directory,
> this of course means that you lose instantly. If you point the root of
> the walk at a directory out of which a process in the container
> wouldn't be able to move the file, you're probably kinda mostly fine -
> as long as you know, for certain, that nothing else on the system
> would ever do that. But I still wouldn't feel good about that.
>
> (Yes, this means that if you run an SFTP server with OpenSSH's
> ChrootDirectory directive, you have to be very careful about these
> things.)
>
> I believe that the only way to robustly use this would be to point the
> dirfd at a mount point, such that you know that being moved out of the
> chroot is impossible because the mount point limits movement of
> directories under it. (Well, technically, it doesn't, but it ensures
> that if a directory does dangerously move away, the syscall fails.) It
> might make sense to hardcode this constraint in the implementation of
> AT_THIS_ROOT, to keep people from shooting themselves in the foot.
I'm very much in favor of dropping AT_THIS_ROOT from this patch series
at least for now and only land the first patch. The first patch is
something that we really want and that it seems we can find a good
design for.
If AT_THIS_ROOT is a feature that still makes sense we can revisit it.
>
> > Currently most container runtimes try to do this resolution in
> > userspace[1], causing many potential race conditions. In addition, the
> > "obvious" alternative (actually performing a {ch,pivot_}root(2))
> > requires a fork+exec which is *very* costly if necessary for every
> > filesystem operation involving a container.
>
> Wait. fork() I understand, but why exec? And actually, you don't need
> a full fork() either, clone() lets you do this with some process parts
> shared. And then you also shouldn't need to use SCM_RIGHTS, just keep
> the file descriptor table shared. And why chroot()/pivot_root(),
> wouldn't you want to use setns()? I think something like this should
> work (except that you should add some error handling - omitted here
> because I'm lazy), assuming that the container runtime does NOT have
> CAP_SYS_ADMIN in the init namespace (otherwise it's easier). Of
> course, this is entirely untested, and probably won't compile because
> I screwed something up. :P But you should get the idea...
>
> // Ensure that we are non-dumpable. Together with
> // commit bfedb589252c, this ensures that container root
> // can't trace our child once it enters the container.
> // My patch
> // https://lore.kernel.org/lkml/1451098351-8917-1-git-send-email-jann@thejh.net/
> // would make this unnecessary, but that patch didn't
> // land because Eric nacked it (for political reasons,
> // because people incorrectly claimed that this was a
> // security fix):
> // https://lore.kernel.org/lkml/8760z7fope.fsf@x220.int.ebiederm.org/
> // Note that dumpability is per-mm, not per-process,
> // so this hack has the unfortunate side effect of preventing
> // unprivileged debugging of the container runtime.
> // Oh well.
> prctl(PR_SET_DUMPABLE, SUID_DUMP_DISABLE);
> // Inform gcc that this particular syscall will effectively
> // return twice, just like vfork() or setjmp().
> __attribute__((returns_twice)) long syscall_(long sysno, ...) = (void*)syscall;
> int result_fd = -1;
> // CLONE_FILES means we don't need to do fd passing,
> // we share the file descriptor table.
> // CLONE_VM means we don't have the cost of duplicating
> // our VMAs and page tables, and we don't have to mark
> // all our pagetable entries as readonly for copy-on-write.
> // CLONE_VFORK is a dirty hack to avoid having to
> // allocate a child stack.
> // Lack of SIGCHLD means we don't want to have to wait()
> // for the child.
> int child_pid = syscall_(__NR_clone, CLONE_FILES|CLONE_VM|CLONE_VFORK,
> 0, 0, 0, 0);
> if (child_pid == 0) {
> // Enter the container's user namespace; this allows us
> // to afterwards join its mount namespace even if we're
> // not capable in the init namespace.
> // (I believe that it should be possible to change the kernel
> // such that this is not required if you have set the
> // no-new-privs flag.)
> setns(container_user_ns_fd, CLONE_NEWUSER);
> // Entering the filesystem namespace automatically
> // moves us to that namespace's filesystem root.
> setns(container_fs_ns_fd, CLONE_NEWNS);
> result_fd = open(untrusted_container_path, ...);
> syscall(__NR_exit, 0);
> }
>
> > The most significant change in semantics with AT_THIS_ROOT is that
> > *at(2) syscalls now no longer have the property that an absolute
> > pathname causes the dirfd to be ignored completely (if LOOKUP_CHROOT is
> > specified). The reasoning behind this is that AT_THIS_ROOT necessarily
> > has to chroot-scope symlinks with absolute paths to dirfd, and so doing
> > it for the base path seems to be the most consistent behaviour (and also
> > avoids foot-gunning users who want to chroot-scope paths that might be
> > absolute).
> >
> > Currently this is only enabled for the stat(2) and openat(2) family (the
> > latter has its own flag O_THISROOT with the same semantics). Ideally
> > this flag would be supported by all *at(2) syscalls, but this will
> > require adding flags arguments to many of them (and will be done in a
> > separate patchset).
> >
> > [1]: https://github.com/cyphar/filepath-securejoin
> >
> > Cc: Eric Biederman <ebiederm@xmission.com>
> > Cc: Christian Brauner <christian@brauner.io>
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> > fs/fcntl.c | 2 +-
> > fs/namei.c | 121 +++++++++++++++++--------------
> > fs/open.c | 2 +
> > fs/stat.c | 4 +-
> > include/linux/fcntl.h | 2 +-
> > include/linux/namei.h | 1 +
> > include/uapi/asm-generic/fcntl.h | 3 +
> > include/uapi/linux/fcntl.h | 2 +
> > 8 files changed, 81 insertions(+), 56 deletions(-)
> >
> > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > index e343618736f7..4c36c5b9fdb9 100644
> > --- a/fs/fcntl.c
> > +++ b/fs/fcntl.c
> > @@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
> > * Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
> > * is defined as O_NONBLOCK on some platforms and not on others.
> > */
> > - BUILD_BUG_ON(25 - 1 /* for O_RDONLY being 0 */ !=
> > + BUILD_BUG_ON(26 - 1 /* for O_RDONLY being 0 */ !=
> > HWEIGHT32(
> > (VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
> > __FMODE_EXEC | __FMODE_NONOTIFY));
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 757dd783771c..1b984f0dbbb4 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2193,9 +2193,64 @@ static int link_path_walk(const char *name, struct nameidata *nd)
> > }
> > }
> >
> > +/*
> > + * Configure nd->path based on the nd->dfd. This is only used as part of
> > + * path_init().
> > + */
> > +static inline int dirfd_path_init(struct nameidata *nd)
> > +{
> > + if (nd->dfd == AT_FDCWD) {
> > + if (nd->flags & LOOKUP_RCU) {
> > + struct fs_struct *fs = current->fs;
> > + unsigned seq;
> > +
> > + do {
> > + seq = read_seqcount_begin(&fs->seq);
> > + nd->path = fs->pwd;
> > + nd->inode = nd->path.dentry->d_inode;
> > + nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
> > + } while (read_seqcount_retry(&fs->seq, seq));
> > + } else {
> > + get_fs_pwd(current->fs, &nd->path);
> > + nd->inode = nd->path.dentry->d_inode;
> > + }
> > + } else {
> > + /* Caller must check execute permissions on the starting path component */
> > + struct fd f = fdget_raw(nd->dfd);
> > + struct dentry *dentry;
> > +
> > + if (!f.file)
> > + return -EBADF;
> > +
> > + dentry = f.file->f_path.dentry;
> > +
> > + if (*nd->name->name && unlikely(!d_can_lookup(dentry))) {
> > + fdput(f);
> > + return -ENOTDIR;
> > + }
> > +
> > + nd->path = f.file->f_path;
> > + if (nd->flags & LOOKUP_RCU) {
> > + nd->inode = nd->path.dentry->d_inode;
> > + nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
> > + } else {
> > + path_get(&nd->path);
> > + nd->inode = nd->path.dentry->d_inode;
> > + }
> > + fdput(f);
> > + }
> > + if (unlikely(nd->flags & (LOOKUP_CHROOT | LOOKUP_BENEATH))) {
> > + nd->root = nd->path;
> > + if (!(nd->flags & LOOKUP_RCU))
> > + path_get(&nd->root);
> > + }
> > + return 0;
> > +}
> > +
> > /* must be paired with terminate_walk() */
> > static const char *path_init(struct nameidata *nd, unsigned flags)
> > {
> > + int error;
> > const char *s = nd->name->name;
> >
> > if (!*s)
> > @@ -2230,65 +2285,25 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
> > nd->path.dentry = NULL;
> >
> > nd->m_seq = read_seqbegin(&mount_lock);
> > + if (unlikely(flags & LOOKUP_CHROOT)) {
> > + error = dirfd_path_init(nd);
> > + if (unlikely(error))
> > + return ERR_PTR(error);
> > + }
> > if (*s == '/') {
> > - int error;
> > - set_root(nd);
> > + if (likely(!nd->root.mnt))
> > + set_root(nd);
> > error = nd_jump_root(nd);
> > if (unlikely(error))
> > s = ERR_PTR(error);
> > return s;
> > - } else if (nd->dfd == AT_FDCWD) {
> > - if (flags & LOOKUP_RCU) {
> > - struct fs_struct *fs = current->fs;
> > - unsigned seq;
> > -
> > - do {
> > - seq = read_seqcount_begin(&fs->seq);
> > - nd->path = fs->pwd;
> > - nd->inode = nd->path.dentry->d_inode;
> > - nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
> > - } while (read_seqcount_retry(&fs->seq, seq));
> > - } else {
> > - get_fs_pwd(current->fs, &nd->path);
> > - nd->inode = nd->path.dentry->d_inode;
> > - }
> > - if (unlikely(flags & LOOKUP_BENEATH)) {
> > - nd->root = nd->path;
> > - if (!(flags & LOOKUP_RCU))
> > - path_get(&nd->root);
> > - }
> > - return s;
> > - } else {
> > - /* Caller must check execute permissions on the starting path component */
> > - struct fd f = fdget_raw(nd->dfd);
> > - struct dentry *dentry;
> > -
> > - if (!f.file)
> > - return ERR_PTR(-EBADF);
> > -
> > - dentry = f.file->f_path.dentry;
> > -
> > - if (*s && unlikely(!d_can_lookup(dentry))) {
> > - fdput(f);
> > - return ERR_PTR(-ENOTDIR);
> > - }
> > -
> > - nd->path = f.file->f_path;
> > - if (flags & LOOKUP_RCU) {
> > - nd->inode = nd->path.dentry->d_inode;
> > - nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
> > - } else {
> > - path_get(&nd->path);
> > - nd->inode = nd->path.dentry->d_inode;
> > - }
> > - if (unlikely(flags & LOOKUP_BENEATH)) {
> > - nd->root = nd->path;
> > - if (!(flags & LOOKUP_RCU))
> > - path_get(&nd->root);
> > - }
> > - fdput(f);
> > - return s;
> > }
> > + if (likely(!nd->path.mnt)) {
> > + error = dirfd_path_init(nd);
> > + if (unlikely(error))
> > + return ERR_PTR(error);
> > + }
> > + return s;
> > }
> >
> > static const char *trailing_symlink(struct nameidata *nd)
> > diff --git a/fs/open.c b/fs/open.c
> > index 80f5f566a5ff..81d148f626cd 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -996,6 +996,8 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
> > lookup_flags |= LOOKUP_NO_PROCLINKS;
> > if (flags & O_NOSYMLINKS)
> > lookup_flags |= LOOKUP_NO_SYMLINKS;
> > + if (flags & O_THISROOT)
> > + lookup_flags |= LOOKUP_CHROOT;
> > op->lookup_flags = lookup_flags;
> > return 0;
> > }
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 791e61b916ae..e8366e4812c3 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -172,7 +172,7 @@ int vfs_statx(int dfd, const char __user *filename, int flags,
> >
> > if (flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT | AT_EMPTY_PATH |
> > KSTAT_QUERY_FLAGS | AT_BENEATH | AT_XDEV |
> > - AT_NO_PROCLINKS | AT_NO_SYMLINKS))
> > + AT_NO_PROCLINKS | AT_NO_SYMLINKS | AT_THIS_ROOT))
> > return -EINVAL;
> >
> > if (flags & AT_SYMLINK_NOFOLLOW)
> > @@ -189,6 +189,8 @@ int vfs_statx(int dfd, const char __user *filename, int flags,
> > lookup_flags |= LOOKUP_NO_PROCLINKS;
> > if (flags & AT_NO_SYMLINKS)
> > lookup_flags |= LOOKUP_NO_SYMLINKS;
> > + if (flags & AT_THIS_ROOT)
> > + lookup_flags |= LOOKUP_CHROOT;
> >
> > retry:
> > error = user_path_at(dfd, filename, lookup_flags, &path);
> > diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
> > index ad5bba4b5b12..95480cd4c09d 100644
> > --- a/include/linux/fcntl.h
> > +++ b/include/linux/fcntl.h
> > @@ -10,7 +10,7 @@
> > O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
> > FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
> > O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_BENEATH | O_XDEV | \
> > - O_NOPROCLINKS | O_NOSYMLINKS)
> > + O_NOPROCLINKS | O_NOSYMLINKS | O_THISROOT)
> >
> > #ifndef force_o_largefile
> > #define force_o_largefile() (BITS_PER_LONG != 32)
> > diff --git a/include/linux/namei.h b/include/linux/namei.h
> > index 5ff7f3362d1b..7ec9e2d84649 100644
> > --- a/include/linux/namei.h
> > +++ b/include/linux/namei.h
> > @@ -53,6 +53,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
> > #define LOOKUP_NO_PROCLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
> > #define LOOKUP_NO_SYMLINKS 0x080000 /* No symlink crossing *at all*.
> > Implies LOOKUP_NO_PROCLINKS. */
> > +#define LOOKUP_CHROOT 0x100000 /* Treat dirfd as %current->fs->root. */
> >
> > extern int path_pts(struct path *path);
> >
> > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
> > index c2bf5983e46a..11206b0e927c 100644
> > --- a/include/uapi/asm-generic/fcntl.h
> > +++ b/include/uapi/asm-generic/fcntl.h
> > @@ -113,6 +113,9 @@
> > #ifndef O_NOSYMLINKS
> > #define O_NOSYMLINKS 01000000000
> > #endif
> > +#ifndef O_THISROOT
> > +#define O_THISROOT 02000000000
> > +#endif
> >
> > #define F_DUPFD 0 /* dup */
> > #define F_GETFD 1 /* get close_on_exec */
> > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > index 551a9e2166a8..ea978457b68f 100644
> > --- a/include/uapi/linux/fcntl.h
> > +++ b/include/uapi/linux/fcntl.h
> > @@ -99,6 +99,8 @@
> > #define AT_NO_PROCLINKS 0x40000 /* No /proc/$pid/fd/... "symlinks". */
> > #define AT_NO_SYMLINKS 0x80000 /* No symlinks *at all*.
> > Implies AT_NO_PROCLINKS. */
> > +#define AT_THIS_ROOT 0x100000 /* Path resolution acts as though
> > + it is chroot-ed into dirfd. */
> >
> >
> > #endif /* _UAPI_LINUX_FCNTL_H */
> > --
> > 2.19.0
> >
> >
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Bruce Fields @ 2018-10-01 13:55 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Jann Horn, Eric W. Biederman, jlayton, Al Viro, Arnd Bergmann,
shuah, David Howells, Andy Lutomirski, christian, Tycho Andersen,
kernel list, linux-fsdevel, linux-arch, linux-kselftest, dev,
containers, Linux API
In-Reply-To: <20181001054246.gfinmx3api7kjhmc@ryuk>
On Mon, Oct 01, 2018 at 03:44:28PM +1000, Aleksa Sarai wrote:
> On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> > The problem is what happens if a folder you are walking through is
> > concurrently moved out of the chroot. Consider the following scenario:
> >
> > You attempt to open "C/../../etc/passwd" under the root "/A/B".
> > Something else concurrently moves /A/B/C to /A/C. This can result in
> > the following:
> >
> > 1. You start the path walk and reach /A/B/C.
> > 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> > 3. Your path walk follows the first ".." up into /A. This is outside
> > the process root, but you never actually encountered the process root,
> > so you don't notice.
> > 4. Your path walk follows the second ".." up to /. Again, this is
> > outside the process root, but you don't notice.
> > 5. Your path walk walks down to /etc/passwd, and the open completes
> > successfully. You now have an fd pointing outside your chroot.
> >
> > If the root of your walk is below an attacker-controlled directory,
> > this of course means that you lose instantly. If you point the root of
> > the walk at a directory out of which a process in the container
> > wouldn't be able to move the file, you're probably kinda mostly fine -
> > as long as you know, for certain, that nothing else on the system
> > would ever do that. But I still wouldn't feel good about that.
>
> Please correct me if I'm wrong here (this is the first patch I've
> written for VFS). Isn't the retry/LOOKUP_REVAL code meant to handle this
No.
...
> Speaking naively, doesn't it make sense to invalidate the walk if a path
> component was modified? Or is this something that would be far too
> costly with little benefit?
Lookups and renames can definitely proceed in parallel, and yes I
suspect it would be difficult to get good performance and guaranteed
forward progress if you required lookup of the full path to be atomic
with respect to renames.
--b.
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Christian Brauner @ 2018-10-01 12:35 UTC (permalink / raw)
To: Jann Horn
Cc: cyphar, Eric W. Biederman, jlayton, Bruce Fields, Al Viro,
Arnd Bergmann, shuah, David Howells, Andy Lutomirski,
Tycho Andersen, kernel list, linux-fsdevel, linux-arch,
linux-kselftest, dev, containers, Linux API
In-Reply-To: <CAG48ez32xKKdtNWreLUMqB+tOfMy-bUK2Bo=vujOTb3qK9NwBg@mail.gmail.com>
On Mon, Oct 01, 2018 at 01:29:16PM +0200, Jann Horn wrote:
> On Mon, Oct 1, 2018 at 12:42 PM Christian Brauner <christian@brauner.io> wrote:
> > On Mon, Oct 01, 2018 at 03:44:28PM +1000, Aleksa Sarai wrote:
> > > On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> > > > The problem is what happens if a folder you are walking through is
> > > > concurrently moved out of the chroot. Consider the following scenario:
> > > >
> > > > You attempt to open "C/../../etc/passwd" under the root "/A/B".
> > > > Something else concurrently moves /A/B/C to /A/C. This can result in
> > > > the following:
> > > >
> > > > 1. You start the path walk and reach /A/B/C.
> > > > 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> > > > 3. Your path walk follows the first ".." up into /A. This is outside
> > > > the process root, but you never actually encountered the process root,
> > > > so you don't notice.
> > > > 4. Your path walk follows the second ".." up to /. Again, this is
> > > > outside the process root, but you don't notice.
> > > > 5. Your path walk walks down to /etc/passwd, and the open completes
> > > > successfully. You now have an fd pointing outside your chroot.
> > > >
> > > > If the root of your walk is below an attacker-controlled directory,
> > > > this of course means that you lose instantly. If you point the root of
> > > > the walk at a directory out of which a process in the container
> > > > wouldn't be able to move the file, you're probably kinda mostly fine -
> > > > as long as you know, for certain, that nothing else on the system
> > > > would ever do that. But I still wouldn't feel good about that.
> > >
> > > Please correct me if I'm wrong here (this is the first patch I've
> > > written for VFS). Isn't the retry/LOOKUP_REVAL code meant to handle this
> > > -- or does that only handle if a particular path component changes
> > > *while* it's being walked through? Is it possible for a path walk to
> > > succeed after a path component was unmounted (obviously you can't delete
> > > a directory path component since you'd get -ENOTEMPTY)?
> > >
> > > If this is an issue for AT_THIS_ROOT, I believe this might also be an
> > > issue for AT_BENEATH since they are effectively both using the same
> > > nd->root trick (so you could similarly trick AT_BENEATH to not error
> > > out). So we'd need to figure out how to solve this problem in order for
> > > AT_BENEATH to be safe.
> > >
> > > Speaking naively, doesn't it make sense to invalidate the walk if a path
> > > component was modified? Or is this something that would be far too
> > > costly with little benefit? What if we do more aggressive nd->root
> > > checks when resolving with AT_BENEATH or AT_THIS_ROOT (or if nd->root !=
> > > current->mnt_ns->root)?
> > >
> > > Regarding chroot attacks, I was aware of the trivial
> > > chroot-open-chroot-fchdir attack but I was not aware that there was a
> > > rename attack for chroot. Thanks for bringing this up!
> > >
> > > > I believe that the only way to robustly use this would be to point the
> > > > dirfd at a mount point, such that you know that being moved out of the
> > > > chroot is impossible because the mount point limits movement of
> > > > directories under it. (Well, technically, it doesn't, but it ensures
> > > > that if a directory does dangerously move away, the syscall fails.) It
> > > > might make sense to hardcode this constraint in the implementation of
> > > > AT_THIS_ROOT, to keep people from shooting themselves in the foot.
> > >
> > > Unless I'm missing something, would this not also affect using a
> > > mountpoint as a dirfd-root (with MS_MOVE of an already-walked-through
> > > path component) -- or does MS_MOVE cause a rewalk in a way that rename
> > > does not?
> > >
> > > I wouldn't mind tying AT_THIS_ROOT to only work on mountpoints (I
> > > thought that bind-mounts would be an issue but you also get -EXDEV when
> > > trying to rename across bind-mounts even if they are on the same
> > > underlying filesystem). But AT_BENEATH might be a more bitter pill to
> > > swallow. I'm not sure.
> > >
> > > In the usecase of container runtimes, we wouldn't generally be doing
> > > resolution of attacker-controlled paths but it still definitely doesn't
> > > hurt to consider this part of the threat model -- to avoid foot-gunning
> > > as you've said. (There also might be some nested-container cases where
> > > you might want to do that.)
> > >
> > > > > Currently most container runtimes try to do this resolution in
> > > > > userspace[1], causing many potential race conditions. In addition, the
> > > > > "obvious" alternative (actually performing a {ch,pivot_}root(2))
> > > > > requires a fork+exec which is *very* costly if necessary for every
> > > > > filesystem operation involving a container.
> > > >
> > > > Wait. fork() I understand, but why exec? And actually, you don't need
> > > > a full fork() either, clone() lets you do this with some process parts
> > > > shared. And then you also shouldn't need to use SCM_RIGHTS, just keep
> > > > the file descriptor table shared. And why chroot()/pivot_root(),
> > > > wouldn't you want to use setns()?
> > >
> > > You're right about this -- for C runtimes. In Go we cannot do a raw
> > > clone() or fork() (if you do it manually with RawSyscall you'll end with
> > > broken runtime state). So you're forced to do fork+exec (which then
> > > means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
> > > for CLONE_VFORK.
> > >
> > > (It should be noted that multi-threaded C runtimes have somewhat similar
> > > issues -- AFAIK you can technically only use AS-Safe glibc functions
> > > after a fork() but that's more of a theoretical concern here. If you
> > > just use raw syscalls there isn't an issue.)
> > >
> > > As for why use setns() rather than pivot_root(), there are cases where
> > > you're operating on a container's image without a running container
> > > (think image extraction or snapshotting tools). In those cases, you
> > > would need to set up a dummy container process in order to setns() into
> > > its namespaces. You are right that setns() would be a better option if
> > > you want the truthful state of what mounts the container sees.
> > >
> > > [I also don't like the idea of joining the user namespace of a malicious
> > > container unless it's necessary but that's probably just needless
> > > paranoia more than anything -- since you're not joining the pidns you
> > > aren't trivially addressable by a malicious container.]
> > >
> > > > // Ensure that we are non-dumpable. Together with
> > > > // commit bfedb589252c, this ensures that container root
> > > > // can't trace our child once it enters the container.
> > > > // My patch
> > > > // https://lore.kernel.org/lkml/1451098351-8917-1-git-send-email-jann@thejh.net/
> > > > // would make this unnecessary, but that patch didn't
> > > > // land because Eric nacked it (for political reasons,
> > > > // because people incorrectly claimed that this was a
> > > > // security fix):
> > >
> > > Unless I'm very much mistaken this was fixed by bfedb589252c ("mm: Add a
> > > user_ns owner to mm_struct and fix ptrace permission checks"). If you
> > > join a user namespace then processes within that user namespace won't
> > > have ptrace_may_access() permissions because your mm is owned by an
> > > ancestor user namespace -- only after exec() will you be traceable.
> >
> > That is not _completely_ true.
> > Iirc (Please someone do yell at me if I'm wrong!), this is as follows.
> > You will in fact be dumpable as long as you don't set{g,u}id() to an
> > effective uid that is different from the effective uid of the process
> > that created the task. For example, if you clone(CLONE_NEWUSER) as an
> > unprivileged user with uid and euid 1000 you are in fact dumpable and
> > thus traceable *but* if you do a setuid(0) in the new task then you will
> > end up with old->euid = 1000 and new->euid = 0 at which point the kernel
> > will remove the dumpable flag and the creating process cannot trace you
> > anymore (which has funny consequences for lsm isolation and sending fds
> > around). Iiuc, The same logic applies when you do a setns() to another
> > user namespace.
>
> (Note that this is only true if your un-namespaced UID actually
> changes. If you create a user namespace and then write to its uid_map
> such that your namespaced UID is zero, that won't trigger this logic.)
The way I figured this works is that it actually only applies to the
case where you're creating a user namespace as an unprivileged user. And
even in that case you will retain dumpability in two cases:
1. mapping userns id 0 to <my-unpriv-host-uid>
and any identity mapping, i.e.
2. mapping <my-unpriv-host-uid> to <my-unpriv-host-uid>
If you're creating a user namespace as root and set up mappings you
should always retain the dumpable flag (I might not be remembering all
corner-cases atm.) if you don't muck with capability sets and so on.
^ permalink raw reply
* Re: [RFC 0/2] ns: introduce binfmt_misc namespace
From: Dmitry Safonov @ 2018-10-01 12:26 UTC (permalink / raw)
To: Laurent Vivier, Andy Lutomirski
Cc: LKML, Linux FS Devel, James Bottomley, Al Viro, Linux API,
Eric W. Biederman, Andrey Vagin, Linux Containers
In-Reply-To: <d1b6b52b-dd7f-fa27-eafa-7a6a43a03b92@vivier.eu>
Hi Laurent, thanks for Cc,
On Mon, 2018-10-01 at 09:13 +0200, Laurent Vivier wrote:
> Le 01/10/2018 à 06:45, Andy Lutomirski a écrit :
> > On Sun, Sep 30, 2018 at 4:47 PM Laurent Vivier <laurent@vivier.eu>
> > wrote:
> > >
> > > This series introduces a new namespace for binfmt_misc.
> > >
> >
> > This seems conceptually quite reasonable, but I'm wondering if the
> > number of namespace types is getting out of hand given the current
> > API. Should we be considering whether we need a new set of
> > namespace
> > creation APIs that scale better to larger numbers of namespace
> > types?
> >
>
> Yes, we need something to increase the maximum number of namespace
> types
> because this is the last bit in the clone() flags and the time
> namespace
> has already preempted it.
Yeah, there is this last CLONE_* flag..
I tried to use that 0x1000 flag for something like CLONE_EXTENDED with
all parameters on the stack, but not sure that's reasonable and maybe
someone will suggest a better solution.
All those different clone() ABI (how many parameters to supply and in
which order do not help much).
--
Thanks,
Dmitry
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Jann Horn @ 2018-10-01 11:29 UTC (permalink / raw)
To: christian
Cc: cyphar, Eric W. Biederman, jlayton, Bruce Fields, Al Viro,
Arnd Bergmann, shuah, David Howells, Andy Lutomirski,
Tycho Andersen, kernel list, linux-fsdevel, linux-arch,
linux-kselftest, dev, containers, Linux API
In-Reply-To: <20181001104202.f6tz54s3fbvld56g@brauner.io>
On Mon, Oct 1, 2018 at 12:42 PM Christian Brauner <christian@brauner.io> wrote:
> On Mon, Oct 01, 2018 at 03:44:28PM +1000, Aleksa Sarai wrote:
> > On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> > > The problem is what happens if a folder you are walking through is
> > > concurrently moved out of the chroot. Consider the following scenario:
> > >
> > > You attempt to open "C/../../etc/passwd" under the root "/A/B".
> > > Something else concurrently moves /A/B/C to /A/C. This can result in
> > > the following:
> > >
> > > 1. You start the path walk and reach /A/B/C.
> > > 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> > > 3. Your path walk follows the first ".." up into /A. This is outside
> > > the process root, but you never actually encountered the process root,
> > > so you don't notice.
> > > 4. Your path walk follows the second ".." up to /. Again, this is
> > > outside the process root, but you don't notice.
> > > 5. Your path walk walks down to /etc/passwd, and the open completes
> > > successfully. You now have an fd pointing outside your chroot.
> > >
> > > If the root of your walk is below an attacker-controlled directory,
> > > this of course means that you lose instantly. If you point the root of
> > > the walk at a directory out of which a process in the container
> > > wouldn't be able to move the file, you're probably kinda mostly fine -
> > > as long as you know, for certain, that nothing else on the system
> > > would ever do that. But I still wouldn't feel good about that.
> >
> > Please correct me if I'm wrong here (this is the first patch I've
> > written for VFS). Isn't the retry/LOOKUP_REVAL code meant to handle this
> > -- or does that only handle if a particular path component changes
> > *while* it's being walked through? Is it possible for a path walk to
> > succeed after a path component was unmounted (obviously you can't delete
> > a directory path component since you'd get -ENOTEMPTY)?
> >
> > If this is an issue for AT_THIS_ROOT, I believe this might also be an
> > issue for AT_BENEATH since they are effectively both using the same
> > nd->root trick (so you could similarly trick AT_BENEATH to not error
> > out). So we'd need to figure out how to solve this problem in order for
> > AT_BENEATH to be safe.
> >
> > Speaking naively, doesn't it make sense to invalidate the walk if a path
> > component was modified? Or is this something that would be far too
> > costly with little benefit? What if we do more aggressive nd->root
> > checks when resolving with AT_BENEATH or AT_THIS_ROOT (or if nd->root !=
> > current->mnt_ns->root)?
> >
> > Regarding chroot attacks, I was aware of the trivial
> > chroot-open-chroot-fchdir attack but I was not aware that there was a
> > rename attack for chroot. Thanks for bringing this up!
> >
> > > I believe that the only way to robustly use this would be to point the
> > > dirfd at a mount point, such that you know that being moved out of the
> > > chroot is impossible because the mount point limits movement of
> > > directories under it. (Well, technically, it doesn't, but it ensures
> > > that if a directory does dangerously move away, the syscall fails.) It
> > > might make sense to hardcode this constraint in the implementation of
> > > AT_THIS_ROOT, to keep people from shooting themselves in the foot.
> >
> > Unless I'm missing something, would this not also affect using a
> > mountpoint as a dirfd-root (with MS_MOVE of an already-walked-through
> > path component) -- or does MS_MOVE cause a rewalk in a way that rename
> > does not?
> >
> > I wouldn't mind tying AT_THIS_ROOT to only work on mountpoints (I
> > thought that bind-mounts would be an issue but you also get -EXDEV when
> > trying to rename across bind-mounts even if they are on the same
> > underlying filesystem). But AT_BENEATH might be a more bitter pill to
> > swallow. I'm not sure.
> >
> > In the usecase of container runtimes, we wouldn't generally be doing
> > resolution of attacker-controlled paths but it still definitely doesn't
> > hurt to consider this part of the threat model -- to avoid foot-gunning
> > as you've said. (There also might be some nested-container cases where
> > you might want to do that.)
> >
> > > > Currently most container runtimes try to do this resolution in
> > > > userspace[1], causing many potential race conditions. In addition, the
> > > > "obvious" alternative (actually performing a {ch,pivot_}root(2))
> > > > requires a fork+exec which is *very* costly if necessary for every
> > > > filesystem operation involving a container.
> > >
> > > Wait. fork() I understand, but why exec? And actually, you don't need
> > > a full fork() either, clone() lets you do this with some process parts
> > > shared. And then you also shouldn't need to use SCM_RIGHTS, just keep
> > > the file descriptor table shared. And why chroot()/pivot_root(),
> > > wouldn't you want to use setns()?
> >
> > You're right about this -- for C runtimes. In Go we cannot do a raw
> > clone() or fork() (if you do it manually with RawSyscall you'll end with
> > broken runtime state). So you're forced to do fork+exec (which then
> > means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
> > for CLONE_VFORK.
> >
> > (It should be noted that multi-threaded C runtimes have somewhat similar
> > issues -- AFAIK you can technically only use AS-Safe glibc functions
> > after a fork() but that's more of a theoretical concern here. If you
> > just use raw syscalls there isn't an issue.)
> >
> > As for why use setns() rather than pivot_root(), there are cases where
> > you're operating on a container's image without a running container
> > (think image extraction or snapshotting tools). In those cases, you
> > would need to set up a dummy container process in order to setns() into
> > its namespaces. You are right that setns() would be a better option if
> > you want the truthful state of what mounts the container sees.
> >
> > [I also don't like the idea of joining the user namespace of a malicious
> > container unless it's necessary but that's probably just needless
> > paranoia more than anything -- since you're not joining the pidns you
> > aren't trivially addressable by a malicious container.]
> >
> > > // Ensure that we are non-dumpable. Together with
> > > // commit bfedb589252c, this ensures that container root
> > > // can't trace our child once it enters the container.
> > > // My patch
> > > // https://lore.kernel.org/lkml/1451098351-8917-1-git-send-email-jann@thejh.net/
> > > // would make this unnecessary, but that patch didn't
> > > // land because Eric nacked it (for political reasons,
> > > // because people incorrectly claimed that this was a
> > > // security fix):
> >
> > Unless I'm very much mistaken this was fixed by bfedb589252c ("mm: Add a
> > user_ns owner to mm_struct and fix ptrace permission checks"). If you
> > join a user namespace then processes within that user namespace won't
> > have ptrace_may_access() permissions because your mm is owned by an
> > ancestor user namespace -- only after exec() will you be traceable.
>
> That is not _completely_ true.
> Iirc (Please someone do yell at me if I'm wrong!), this is as follows.
> You will in fact be dumpable as long as you don't set{g,u}id() to an
> effective uid that is different from the effective uid of the process
> that created the task. For example, if you clone(CLONE_NEWUSER) as an
> unprivileged user with uid and euid 1000 you are in fact dumpable and
> thus traceable *but* if you do a setuid(0) in the new task then you will
> end up with old->euid = 1000 and new->euid = 0 at which point the kernel
> will remove the dumpable flag and the creating process cannot trace you
> anymore (which has funny consequences for lsm isolation and sending fds
> around). Iiuc, The same logic applies when you do a setns() to another
> user namespace.
(Note that this is only true if your un-namespaced UID actually
changes. If you create a user namespace and then write to its uid_map
such that your namespaced UID is zero, that won't trigger this logic.)
^ permalink raw reply
* [PATCH 3/3] tools: add selftest for BPF_F_ZERO_SEED
From: Lorenz Bauer @ 2018-10-01 10:45 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181001104509.24211-1-lmb@cloudflare.com>
---
tools/testing/selftests/bpf/test_maps.c | 67 +++++++++++++++++++++----
1 file changed, 56 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index 9b552c0fc47d..2a1cc00475f6 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -257,23 +257,35 @@ static void test_hashmap_percpu(int task, void *data)
close(fd);
}
+static int helper_fill_hashmap(int max_entries)
+{
+ int i, fd, ret;
+ long long key, value;
+
+ fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
+ max_entries, map_flags);
+ CHECK(
+ fd < 0,
+ "failed to create hashmap",
+ "err: %s, flags: 0x%x\n", strerror(errno), map_flags
+ );
+
+ for (i = 0; i < max_entries; i++) {
+ key = i; value = key;
+ ret = bpf_map_update_elem(fd, &key, &value, BPF_NOEXIST);
+ CHECK(ret != 0, "can't update hashmap", "err: %s\n", strerror(ret));
+ }
+
+ return fd;
+}
+
static void test_hashmap_walk(int task, void *data)
{
int fd, i, max_entries = 1000;
long long key, value, next_key;
bool next_key_valid = true;
- fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
- max_entries, map_flags);
- if (fd < 0) {
- printf("Failed to create hashmap '%s'!\n", strerror(errno));
- exit(1);
- }
-
- for (i = 0; i < max_entries; i++) {
- key = i; value = key;
- assert(bpf_map_update_elem(fd, &key, &value, BPF_NOEXIST) == 0);
- }
+ fd = helper_fill_hashmap(max_entries);
for (i = 0; bpf_map_get_next_key(fd, !i ? NULL : &key,
&next_key) == 0; i++) {
@@ -305,6 +317,38 @@ static void test_hashmap_walk(int task, void *data)
close(fd);
}
+static void test_hashmap_zero_seed(void)
+{
+ int i, first, second, old_flags;
+ long long key, next_first, next_second;
+
+ old_flags = map_flags;
+ map_flags |= BPF_F_ZERO_SEED;
+
+ first = helper_fill_hashmap(3);
+ second = helper_fill_hashmap(3);
+
+ for (i = 0; ; i++) {
+ void *key_ptr = !i ? NULL : &key;
+
+ if (bpf_map_get_next_key(first, key_ptr, &next_first) != 0)
+ break;
+
+ CHECK(bpf_map_get_next_key(second, key_ptr, &next_second) != 0,
+ "next_key for second map must succeed",
+ "key_ptr: %p", key_ptr);
+ CHECK(next_first != next_second,
+ "keys must match",
+ "i: %d first: %lld second: %lld\n", i, next_first, next_second);
+
+ key = next_first;
+ }
+
+ map_flags = old_flags;
+ close(first);
+ close(second);
+}
+
static void test_arraymap(int task, void *data)
{
int key, next_key, fd;
@@ -1417,6 +1461,7 @@ static void run_all_tests(void)
test_hashmap(0, NULL);
test_hashmap_percpu(0, NULL);
test_hashmap_walk(0, NULL);
+ test_hashmap_zero_seed();
test_arraymap(0, NULL);
test_arraymap_percpu(0, NULL);
--
2.17.1
^ permalink raw reply related
* [PATCH 2/3] tools: sync linux/bpf.h
From: Lorenz Bauer @ 2018-10-01 10:45 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181001104509.24211-1-lmb@cloudflare.com>
---
tools/include/uapi/linux/bpf.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index aa5ccd2385ed..9d15c8f179ac 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -252,6 +252,8 @@ enum bpf_attach_type {
#define BPF_F_NO_COMMON_LRU (1U << 1)
/* Specify numa node during map creation */
#define BPF_F_NUMA_NODE (1U << 2)
+/* Zero-initialize hash function seed */
+#define BPF_F_ZERO_SEED (1U << 6)
/* flags for BPF_PROG_QUERY */
#define BPF_F_QUERY_EFFECTIVE (1U << 0)
--
2.17.1
^ permalink raw reply related
* [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Lorenz Bauer @ 2018-10-01 10:45 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181001104509.24211-1-lmb@cloudflare.com>
Add a new flag BPF_F_ZERO_SEED, which forces a hash map
to initialize the seed to zero.
---
include/uapi/linux/bpf.h | 2 ++
kernel/bpf/hashtab.c | 8 ++++++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index aa5ccd2385ed..9d15c8f179ac 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -252,6 +252,8 @@ enum bpf_attach_type {
#define BPF_F_NO_COMMON_LRU (1U << 1)
/* Specify numa node during map creation */
#define BPF_F_NUMA_NODE (1U << 2)
+/* Zero-initialize hash function seed */
+#define BPF_F_ZERO_SEED (1U << 6)
/* flags for BPF_PROG_QUERY */
#define BPF_F_QUERY_EFFECTIVE (1U << 0)
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 2c1790288138..a79e123dae62 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -23,7 +23,7 @@
#define HTAB_CREATE_FLAG_MASK \
(BPF_F_NO_PREALLOC | BPF_F_NO_COMMON_LRU | BPF_F_NUMA_NODE | \
- BPF_F_RDONLY | BPF_F_WRONLY)
+ BPF_F_RDONLY | BPF_F_WRONLY | BPF_F_ZERO_SEED)
struct bucket {
struct hlist_nulls_head head;
@@ -373,7 +373,11 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
if (!htab->buckets)
goto free_htab;
- htab->hashrnd = get_random_int();
+ if (htab->map.map_flags & BPF_F_ZERO_SEED)
+ htab->hashrnd = 0;
+ else
+ htab->hashrnd = get_random_int();
+
for (i = 0; i < htab->n_buckets; i++) {
INIT_HLIST_NULLS_HEAD(&htab->buckets[i].head, i);
raw_spin_lock_init(&htab->buckets[i].lock);
--
2.17.1
^ permalink raw reply related
* [PATCH 0/3] bpf: allow zero-initialising hash map seed
From: Lorenz Bauer @ 2018-10-01 10:45 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
This patch set adds a new flag BPF_F_ZERO_SEED, which allows
forcing the seed used by hash maps to zero. This makes
it possible to write deterministic tests.
Based on an off-list conversation with Alexei Starovoitov and
Daniel Borkmann.
Lorenz Bauer (3):
bpf: allow zero-initializing hash map seed
tools: sync linux/bpf.h
tools: add selftest for BPF_F_ZERO_SEED
include/uapi/linux/bpf.h | 2 +
kernel/bpf/hashtab.c | 8 ++-
tools/include/uapi/linux/bpf.h | 2 +
tools/testing/selftests/bpf/test_maps.c | 67 +++++++++++++++++++++----
4 files changed, 66 insertions(+), 13 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Christian Brauner @ 2018-10-01 10:42 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Jann Horn, Eric W. Biederman, jlayton, Bruce Fields, Al Viro,
Arnd Bergmann, shuah, David Howells, Andy Lutomirski,
Tycho Andersen, kernel list, linux-fsdevel, linux-arch,
linux-kselftest, dev, containers, Linux API
In-Reply-To: <20181001054246.gfinmx3api7kjhmc@ryuk>
On Mon, Oct 01, 2018 at 03:44:28PM +1000, Aleksa Sarai wrote:
> On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> > The problem is what happens if a folder you are walking through is
> > concurrently moved out of the chroot. Consider the following scenario:
> >
> > You attempt to open "C/../../etc/passwd" under the root "/A/B".
> > Something else concurrently moves /A/B/C to /A/C. This can result in
> > the following:
> >
> > 1. You start the path walk and reach /A/B/C.
> > 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> > 3. Your path walk follows the first ".." up into /A. This is outside
> > the process root, but you never actually encountered the process root,
> > so you don't notice.
> > 4. Your path walk follows the second ".." up to /. Again, this is
> > outside the process root, but you don't notice.
> > 5. Your path walk walks down to /etc/passwd, and the open completes
> > successfully. You now have an fd pointing outside your chroot.
> >
> > If the root of your walk is below an attacker-controlled directory,
> > this of course means that you lose instantly. If you point the root of
> > the walk at a directory out of which a process in the container
> > wouldn't be able to move the file, you're probably kinda mostly fine -
> > as long as you know, for certain, that nothing else on the system
> > would ever do that. But I still wouldn't feel good about that.
>
> Please correct me if I'm wrong here (this is the first patch I've
> written for VFS). Isn't the retry/LOOKUP_REVAL code meant to handle this
> -- or does that only handle if a particular path component changes
> *while* it's being walked through? Is it possible for a path walk to
> succeed after a path component was unmounted (obviously you can't delete
> a directory path component since you'd get -ENOTEMPTY)?
>
> If this is an issue for AT_THIS_ROOT, I believe this might also be an
> issue for AT_BENEATH since they are effectively both using the same
> nd->root trick (so you could similarly trick AT_BENEATH to not error
> out). So we'd need to figure out how to solve this problem in order for
> AT_BENEATH to be safe.
>
> Speaking naively, doesn't it make sense to invalidate the walk if a path
> component was modified? Or is this something that would be far too
> costly with little benefit? What if we do more aggressive nd->root
> checks when resolving with AT_BENEATH or AT_THIS_ROOT (or if nd->root !=
> current->mnt_ns->root)?
>
> Regarding chroot attacks, I was aware of the trivial
> chroot-open-chroot-fchdir attack but I was not aware that there was a
> rename attack for chroot. Thanks for bringing this up!
>
> > I believe that the only way to robustly use this would be to point the
> > dirfd at a mount point, such that you know that being moved out of the
> > chroot is impossible because the mount point limits movement of
> > directories under it. (Well, technically, it doesn't, but it ensures
> > that if a directory does dangerously move away, the syscall fails.) It
> > might make sense to hardcode this constraint in the implementation of
> > AT_THIS_ROOT, to keep people from shooting themselves in the foot.
>
> Unless I'm missing something, would this not also affect using a
> mountpoint as a dirfd-root (with MS_MOVE of an already-walked-through
> path component) -- or does MS_MOVE cause a rewalk in a way that rename
> does not?
>
> I wouldn't mind tying AT_THIS_ROOT to only work on mountpoints (I
> thought that bind-mounts would be an issue but you also get -EXDEV when
> trying to rename across bind-mounts even if they are on the same
> underlying filesystem). But AT_BENEATH might be a more bitter pill to
> swallow. I'm not sure.
>
> In the usecase of container runtimes, we wouldn't generally be doing
> resolution of attacker-controlled paths but it still definitely doesn't
> hurt to consider this part of the threat model -- to avoid foot-gunning
> as you've said. (There also might be some nested-container cases where
> you might want to do that.)
>
> > > Currently most container runtimes try to do this resolution in
> > > userspace[1], causing many potential race conditions. In addition, the
> > > "obvious" alternative (actually performing a {ch,pivot_}root(2))
> > > requires a fork+exec which is *very* costly if necessary for every
> > > filesystem operation involving a container.
> >
> > Wait. fork() I understand, but why exec? And actually, you don't need
> > a full fork() either, clone() lets you do this with some process parts
> > shared. And then you also shouldn't need to use SCM_RIGHTS, just keep
> > the file descriptor table shared. And why chroot()/pivot_root(),
> > wouldn't you want to use setns()?
>
> You're right about this -- for C runtimes. In Go we cannot do a raw
> clone() or fork() (if you do it manually with RawSyscall you'll end with
> broken runtime state). So you're forced to do fork+exec (which then
> means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
> for CLONE_VFORK.
>
> (It should be noted that multi-threaded C runtimes have somewhat similar
> issues -- AFAIK you can technically only use AS-Safe glibc functions
> after a fork() but that's more of a theoretical concern here. If you
> just use raw syscalls there isn't an issue.)
>
> As for why use setns() rather than pivot_root(), there are cases where
> you're operating on a container's image without a running container
> (think image extraction or snapshotting tools). In those cases, you
> would need to set up a dummy container process in order to setns() into
> its namespaces. You are right that setns() would be a better option if
> you want the truthful state of what mounts the container sees.
>
> [I also don't like the idea of joining the user namespace of a malicious
> container unless it's necessary but that's probably just needless
> paranoia more than anything -- since you're not joining the pidns you
> aren't trivially addressable by a malicious container.]
>
> > // Ensure that we are non-dumpable. Together with
> > // commit bfedb589252c, this ensures that container root
> > // can't trace our child once it enters the container.
> > // My patch
> > // https://lore.kernel.org/lkml/1451098351-8917-1-git-send-email-jann@thejh.net/
> > // would make this unnecessary, but that patch didn't
> > // land because Eric nacked it (for political reasons,
> > // because people incorrectly claimed that this was a
> > // security fix):
>
> Unless I'm very much mistaken this was fixed by bfedb589252c ("mm: Add a
> user_ns owner to mm_struct and fix ptrace permission checks"). If you
> join a user namespace then processes within that user namespace won't
> have ptrace_may_access() permissions because your mm is owned by an
> ancestor user namespace -- only after exec() will you be traceable.
That is not _completely_ true.
Iirc (Please someone do yell at me if I'm wrong!), this is as follows.
You will in fact be dumpable as long as you don't set{g,u}id() to an
effective uid that is different from the effective uid of the process
that created the task. For example, if you clone(CLONE_NEWUSER) as an
unprivileged user with uid and euid 1000 you are in fact dumpable and
thus traceable *but* if you do a setuid(0) in the new task then you will
end up with old->euid = 1000 and new->euid = 0 at which point the kernel
will remove the dumpable flag and the creating process cannot trace you
anymore (which has funny consequences for lsm isolation and sending fds
around). Iiuc, The same logic applies when you do a setns() to another
user namespace.
/* dumpability changes */
if (!uid_eq(old->euid, new->euid) ||
!gid_eq(old->egid, new->egid) ||
!uid_eq(old->fsuid, new->fsuid) ||
!gid_eq(old->fsgid, new->fsgid) ||
!cred_cap_issubset(old, new)) {
if (task->mm)
set_dumpable(task->mm, suid_dumpable); <<< suid_dumpable == 0 at this point
task->pdeath_signal = 0;
smp_wmb();
}
>
> We still use PR_SET_DUMPABLE in runc but that's because we support older
> kernels (and people don't use user namespaces under Docker) but with
> user namespaces this should not be required anymore.
>
> --
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Jann Horn @ 2018-10-01 10:13 UTC (permalink / raw)
To: cyphar, Eric W. Biederman, Al Viro
Cc: jlayton, Bruce Fields, Arnd Bergmann, shuah, David Howells,
Andy Lutomirski, christian, Tycho Andersen, kernel list,
linux-fsdevel, linux-arch, linux-kselftest, dev, containers,
Linux API
In-Reply-To: <20181001054246.gfinmx3api7kjhmc@ryuk>
On Mon, Oct 1, 2018 at 7:44 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> > The problem is what happens if a folder you are walking through is
> > concurrently moved out of the chroot. Consider the following scenario:
> >
> > You attempt to open "C/../../etc/passwd" under the root "/A/B".
> > Something else concurrently moves /A/B/C to /A/C. This can result in
> > the following:
> >
> > 1. You start the path walk and reach /A/B/C.
> > 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> > 3. Your path walk follows the first ".." up into /A. This is outside
> > the process root, but you never actually encountered the process root,
> > so you don't notice.
> > 4. Your path walk follows the second ".." up to /. Again, this is
> > outside the process root, but you don't notice.
> > 5. Your path walk walks down to /etc/passwd, and the open completes
> > successfully. You now have an fd pointing outside your chroot.
> >
> > If the root of your walk is below an attacker-controlled directory,
> > this of course means that you lose instantly. If you point the root of
> > the walk at a directory out of which a process in the container
> > wouldn't be able to move the file, you're probably kinda mostly fine -
> > as long as you know, for certain, that nothing else on the system
> > would ever do that. But I still wouldn't feel good about that.
>
> Please correct me if I'm wrong here (this is the first patch I've
> written for VFS). Isn't the retry/LOOKUP_REVAL code meant to handle this
> -- or does that only handle if a particular path component changes
> *while* it's being walked through?
Eric Biederman should be able to talk about all this much better than
me, but as far as I know, the LOOKUP_REVAL path is only for dealing
with some special filesystems like procfs.
> Is it possible for a path walk to
> succeed after a path component was unmounted (obviously you can't delete
> a directory path component since you'd get -ENOTEMPTY)?
I don't think so, but I'm not exactly a VFS expert.
> If this is an issue for AT_THIS_ROOT, I believe this might also be an
> issue for AT_BENEATH since they are effectively both using the same
> nd->root trick (so you could similarly trick AT_BENEATH to not error
> out). So we'd need to figure out how to solve this problem in order for
> AT_BENEATH to be safe.
Oh, wait, what? I think I didn't notice that the semantics of
AT_BENEATH changed like that since the original posting of David
Drysdale's O_BENEATH_ONLY patch
(https://lore.kernel.org/lkml/1439458366-8223-2-git-send-email-drysdale@google.com/).
David's patch had nice, straightforward semantics, blocking any form
of upwards traversal. Why was that changed? Does anyone actually want
to use paths that contain ".." with AT_BENEATH? I would strongly
prefer something that blocks any use of "..".
@Al: It looks like this already changed back when you posted
https://lore.kernel.org/lkml/20170429220414.GT29622@ZenIV.linux.org.uk/
?
> Speaking naively, doesn't it make sense to invalidate the walk if a path
> component was modified? Or is this something that would be far too
> costly with little benefit? What if we do more aggressive nd->root
> checks when resolving with AT_BENEATH or AT_THIS_ROOT (or if nd->root !=
> current->mnt_ns->root)?
It seems to me like doing that would basically require looking at each
node in the path walk twice? And it'd be difficult to guarantee
forward progress unless you're willing to do some fairly heavy
locking.
> Regarding chroot attacks, I was aware of the trivial
> chroot-open-chroot-fchdir attack but I was not aware that there was a
> rename attack for chroot. Thanks for bringing this up!
>
> > I believe that the only way to robustly use this would be to point the
> > dirfd at a mount point, such that you know that being moved out of the
> > chroot is impossible because the mount point limits movement of
> > directories under it. (Well, technically, it doesn't, but it ensures
> > that if a directory does dangerously move away, the syscall fails.) It
> > might make sense to hardcode this constraint in the implementation of
> > AT_THIS_ROOT, to keep people from shooting themselves in the foot.
>
> Unless I'm missing something, would this not also affect using a
> mountpoint as a dirfd-root (with MS_MOVE of an already-walked-through
> path component) -- or does MS_MOVE cause a rewalk in a way that rename
> does not?
Hmm. Good point.
It looks to me like you probably wouldn't be able to walk up through a
mountpoint in RCU mode after the mount hierarchy has changed (see
follow_dotdot_rcu()), but it might work in refwalk mode.
Eric?
> I wouldn't mind tying AT_THIS_ROOT to only work on mountpoints (I
> thought that bind-mounts would be an issue but you also get -EXDEV when
> trying to rename across bind-mounts even if they are on the same
> underlying filesystem). But AT_BENEATH might be a more bitter pill to
> swallow. I'm not sure.
Which is part of why I strongly prefer the semantics from David
Drysdale's O_BENEATH_ONLY.
> In the usecase of container runtimes, we wouldn't generally be doing
> resolution of attacker-controlled paths but it still definitely doesn't
> hurt to consider this part of the threat model -- to avoid foot-gunning
> as you've said. (There also might be some nested-container cases where
> you might want to do that.)
>
> > > Currently most container runtimes try to do this resolution in
> > > userspace[1], causing many potential race conditions. In addition, the
> > > "obvious" alternative (actually performing a {ch,pivot_}root(2))
> > > requires a fork+exec which is *very* costly if necessary for every
> > > filesystem operation involving a container.
> >
> > Wait. fork() I understand, but why exec? And actually, you don't need
> > a full fork() either, clone() lets you do this with some process parts
> > shared. And then you also shouldn't need to use SCM_RIGHTS, just keep
> > the file descriptor table shared. And why chroot()/pivot_root(),
> > wouldn't you want to use setns()?
>
> You're right about this -- for C runtimes. In Go we cannot do a raw
> clone() or fork() (if you do it manually with RawSyscall you'll end with
> broken runtime state). So you're forced to do fork+exec (which then
> means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
> for CLONE_VFORK.
If you insist on implementing every last bit of your code in Go, I guess.
> (It should be noted that multi-threaded C runtimes have somewhat similar
> issues -- AFAIK you can technically only use AS-Safe glibc functions
> after a fork() but that's more of a theoretical concern here. If you
> just use raw syscalls there isn't an issue.)
>
> As for why use setns() rather than pivot_root(), there are cases where
> you're operating on a container's image without a running container
> (think image extraction or snapshotting tools). In those cases, you
> would need to set up a dummy container process in order to setns() into
> its namespaces. You are right that setns() would be a better option if
> you want the truthful state of what mounts the container sees.
>
> [I also don't like the idea of joining the user namespace of a malicious
> container unless it's necessary but that's probably just needless
> paranoia more than anything -- since you're not joining the pidns you
> aren't trivially addressable by a malicious container.]
>
> > // Ensure that we are non-dumpable. Together with
> > // commit bfedb589252c, this ensures that container root
> > // can't trace our child once it enters the container.
> > // My patch
> > // https://lore.kernel.org/lkml/1451098351-8917-1-git-send-email-jann@thejh.net/
> > // would make this unnecessary, but that patch didn't
> > // land because Eric nacked it (for political reasons,
> > // because people incorrectly claimed that this was a
> > // security fix):
>
> Unless I'm very much mistaken this was fixed by bfedb589252c ("mm: Add a
> user_ns owner to mm_struct and fix ptrace permission checks"). If you
> join a user namespace then processes within that user namespace won't
> have ptrace_may_access() permissions because your mm is owned by an
> ancestor user namespace -- only after exec() will you be traceable.
Nope. The code added in bfedb589252c only applies if `get_dumpable(mm)
!= SUID_DUMP_USER`.
Looking at the current version, you can see that
`ptrace_has_cap(tcred->user_ns, mode)` is enough to ptrace a process
that is not nondumpable.
> We still use PR_SET_DUMPABLE in runc but that's because we support older
> kernels (and people don't use user namespaces under Docker) but with
> user namespaces this should not be required anymore.
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Aleksa Sarai @ 2018-10-01 9:46 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jann Horn, Eric W. Biederman, jlayton, Bruce Fields, Al Viro,
Arnd Bergmann, shuah, David Howells, Andy Lutomirski, christian,
Tycho Andersen, kernel list, linux-fsdevel, linux-arch,
linux-kselftest, dev, containers, Linux API
In-Reply-To: <F0E08B90-F10B-4897-913D-CA18E99A987D@amacapital.net>
[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]
On 2018-09-29, Andy Lutomirski <luto@amacapital.net> wrote:
> >> On Sat, Sep 29, 2018 at 4:29 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >> The primary motivation for the need for this flag is container runtimes
> >> which have to interact with malicious root filesystems in the host
> >> namespaces. One of the first requirements for a container runtime to be
> >> secure against a malicious rootfs is that they correctly scope symlinks
> >> (that is, they should be scoped as though they are chroot(2)ed into the
> >> container's rootfs) and ".."-style paths. The already-existing AT_XDEV
> >> and AT_NO_PROCLINKS help defend against other potential attacks in a
> >> malicious rootfs scenario.
> >
> > So, I really like the concept for patch 1 of this series (but haven't
> > read the code yet); but I dislike this patch because of its footgun
> > potential.
> >
>
> The code could do it differently: do the path walk and then, before
> accepting the result, walk back up and make sure the result is under
> the starting point.
>
> This is *not* a full solution, though, since a walk above the root gas
> side effects on timing, various caches, and possibly network traffic,
> so it’s open to Spectre-like attacks in which a malicious container
> could use a runtime-initiated AT_THIS_ROOT to infer the existence of
> directories outside the container.
I think that one way to solve this problem might be to have more strict
checks on nd->root in follow_dotdot(). The problem here (as far as I can
tell) is that ".." could end up skipping past the root because of a
rename, however walking *down* into a path shouldn't be a problem (even
absolute symlinks shouldn't be a problem because they will nd_jump_root
and will land back in the root).
However, I'm not entirely sure what happens to nd->root if it gets
renamed -- can you still safely do checks against it (we'd need to do
some sort of is_descendant() check on the current path before we handle
".." in follow_dotdot).
That way, we wouldn't shouldn't have the spectre-like attack problem
(since the attack would be halted at the ".." stage -- before the path
walk can proceed into host paths). Would this be sufficient or is there
a more serious issue I'm missing?
> But what’s the container usecase? Any sane container is based on
> pivot_root or similar, so the runtime can just do the walk in the
> container context. IOW I’m a bit confused as to the exact intended use
> of the whole series. Can you elaborate?
I went into this in my response to Jann.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Setting monotonic time?
From: Eric W. Biederman @ 2018-10-01 9:15 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrey Vagin, Dmitry Safonov, linux-kernel@vger.kernel.org,
Dmitry Safonov, Adrian Reber, Andy Lutomirski, Christian Brauner,
Cyrill Gorcunov, H. Peter Anvin, Ingo Molnar, Jeff Dike,
Oleg Nesterov, Pavel Emelianov, Shuah Khan,
containers@lists.linux-foundation.org, criu@openvz.org,
linux-api@vger.kernel.org, x86@kernel.org, Alexey
In-Reply-To: <alpine.DEB.2.21.1809282023350.1432@nanos.tec.linutronix.de>
In the context of process migration there is a simpler subproblem that I
think it is worth exploring if we can do something about.
For a cluster of machines all running with synchronized
clocks. CLOCK_REALTIME matches. CLOCK_MONOTNIC does not match between
machines. Not having a matching CLOCK_MONOTONIC prevents successful
process migration between nodes in that cluster.
Would it be possible to allow setting CLOCK_MONOTONIC at the very
beginning of time? So that all of the nodes in a cluster can be in
sync?
No change in skew just in offset for CLOCK_MONOTONIC.
There are also dragons involved in coordinating things so that
CLOCK_MONOTONIC gets set before CLOCK_MONOTONIC gets used. So I don't
know if allowing CLOCK_MONOTONIC to be set would be practical but it
seems work exploring all on it's own.
Dmitry would setting CLOCK_MONOTONIC exactly once at boot time solve
your problem that is you are looking at a time namespace to solve?
Eric
^ permalink raw reply
* Re: [RFC 00/20] ns: Introduce Time Namespace
From: Eric W. Biederman @ 2018-10-01 9:05 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrey Vagin, Dmitry Safonov, linux-kernel@vger.kernel.org,
Dmitry Safonov, Adrian Reber, Andy Lutomirski, Christian Brauner,
Cyrill Gorcunov, H. Peter Anvin, Ingo Molnar, Jeff Dike,
Oleg Nesterov, Pavel Emelianov, Shuah Khan,
containers@lists.linux-foundation.org, criu@openvz.org,
linux-api@vger.kernel.org, x86@kernel.org, Alexey
In-Reply-To: <alpine.DEB.2.21.1809282023350.1432@nanos.tec.linutronix.de>
Thomas Gleixner <tglx@linutronix.de> writes:
> Eric,
>
> On Fri, 28 Sep 2018, Eric W. Biederman wrote:
>> Thomas Gleixner <tglx@linutronix.de> writes:
>> > On Wed, 26 Sep 2018, Eric W. Biederman wrote:
>> >> At the same time using the techniques from the nohz work and a little
>> >> smarts I expect we could get the code to scale.
>> >
>> > You'd need to invoke the update when the namespace is switched in and
>> > hasn't been updated since the last tick happened. That might be doable, but
>> > you also need to take the wraparound constraints of the underlying
>> > clocksources into account, which again can cause walking all name spaces
>> > when they are all idle long enough.
>>
>> The wrap around constraints being how long before the time sources wrap
>> around so you have to read them once per wrap around? I have not dug
>> deeply enough into the code to see that yet.
>
> It's done by limiting the NOHZ idle time when all CPUs are going into deep
> sleep for a long time, i.e. we make sure that at least one CPU comes back
> sufficiently _before_ the wraparound happens and invokes the update
> function.
>
> It's not so much a problem for TSC, but not every clocksource the kernel
> supports has wraparound times in the range of hundreds of years.
>
> But yes, your idea of keeping track of wraparounds might work. Tricky, but
> looks feasible on first sight, but we should be aware of the dragons.
Oh. Yes. Definitely. A key enabler of any namespace implementation is
figuring out how to tame the dragons.
>> Please pardon me for thinking out load.
>>
>> There are one or more time sources that we use to compute the time
>> and for each time source we have a conversion from ticks of the
>> time source to nanoseconds.
>>
>> Each time source needs to be sampled at least once per wrap-around
>> and something incremented so that we don't loose time when looking
>> at that time source.
>>
>> There are several clocks presented to userspace and they all share the
>> same length of second and are all fundamentally offsets from
>> CLOCK_MONOTONIC.
>
> Yes. That's the readout side. This one is doable. But now look at timers.
>
> If you arm the timer from a name space, then it needs to be converted to
> host time in order to sort it into the hrtimer queue and at some point arm
> the clockevent device for it. This works as long as host and name space
> time have a constant offset and the same skew.
>
> Once the name space time has a different skew this falls apart because the
> armed timer will either expire late or early.
>
> Late might be acceptable, early violates the spec. You could do an extra
> check for rescheduling it, if it's early, but that requires to store the
> name space time accessor in the hrtimer itself because not every timer
> expiry happens so that it can be checked in the name space context (think
> signal based timers). We need to add this extra magic right into
> __hrtimer_run_queues() which is called from the hard and soft interrupt. We
> really don't want to touch all relevant callbacks or syscalls. The latter
> is not sufficient anyway for signal based timer delivery.
>
> That's going to be interesting in terms of synchronization and might also
> cause substantial overhead at least for the timers which belong to name
> spaces.
>
> But that also means that anything which is early can and probably will
> cause rearming of the timer hardware possibly for a very short delta. We
> need to think about whether this can be abused to create interrupt storms.
>
> Now if you accept a bit late, which I'm not really happy about, then you
> surely won't accept very late, i.e. hours, days. But that can happen when
> settimeofday() comes into play. Right now with a single time domain, this
> is easy. When settimeofday() or adjtimex() makes time jump, we just go and
> reprogramm the hardware timers accordingly, which might also result in
> immediate expiry of timers.
>
> But this does not help for time jumps in name spaces because the timer is
> enqueued on the host time base.
>
> And no, we should not think about creating per name space hrtimer queues
> and then have to walk through all of them for finding the first expiring
> timer in order to arm the hardware. That cannot scale.
>
> Walking all hrtimer bases on all CPUs and check all queued timers whether
> they belong to the affected name space does not scale either.
>
> So we'd need to keep track of queued timers belonging to a name space and
> then just handle them. Interesting locking problem and also a scalability
> issue because this might need to be done on all online CPUs. Haven't
> thought it through, but it makes me shudder.
Yes. I can see how this is a dragon that we need to figure out how to
tame. It already exist somewhat for CLOCK_MONOTONIC vs CLOCK_REALTIME
but still.
>> I see two fundamental driving cases for a time namespace.
>
> <SNIP>
>
> I completely understand the problem you are trying to solve and yes, the
> read out of time should be a solvable problem.
There is simplified subproblem that I want to ask about but I will reply
separately for that.
>> Not that I think a final implementation would necessary look like what I
>> have described. I just think it is possible with extreme care to evolve
>> the current code base into something that can efficiently handle
>> multiple time domains with slightly different lenghts of second.
>
> Yes, it really needs some serious thoughts and timekeeping is a really
> complex place especially with NTP/PTP in play. We had quite some quality
> time to make it work correctly and reliably, now you come along and want to
> transform it into a multidimensional puzzle. :)
I thought it was Einstein who pointed out what a puzzle timekeeping is,
with the rest of us just playing catch up. ;-)
>> It does though sound like it is going to take some serious digging
>> through the code to understand how what everything does and how and why
>> everthing works the way it does. Not something grafted on top with just
>> a cursory understanding of how the code works.
>
> I fully agree and I'm happy to help with explanations and ideas and being
> the one who shoots holes into yours.
Sounds good.
Eric
^ permalink raw reply
* Re: [RFC 0/2] ns: introduce binfmt_misc namespace
From: Eric W. Biederman @ 2018-10-01 8:56 UTC (permalink / raw)
To: Laurent Vivier
Cc: Andy Lutomirski, LKML, Linux FS Devel, James Bottomley, Al Viro,
Linux API, Dmitry Safonov, Andrey Vagin, Linux Containers
In-Reply-To: <5b4f0798-eb22-2684-775c-c038c104e190@vivier.eu>
Laurent Vivier <laurent@vivier.eu> writes:
> Le 01/10/2018 à 09:21, Eric W. Biederman a écrit :
>> Andy Lutomirski <luto@kernel.org> writes:
>>
>>> On Sun, Sep 30, 2018 at 4:47 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>>>
>>>> This series introduces a new namespace for binfmt_misc.
>>>>
>>>
>>> This seems conceptually quite reasonable, but I'm wondering if the
>>> number of namespace types is getting out of hand given the current
>>> API. Should we be considering whether we need a new set of namespace
>>> creation APIs that scale better to larger numbers of namespace types?
>>
>> I would rather encourage a way to make this part of an existing
>> namespace or find a way to make a mount of binfmt_misc control this.
>>
>> Hmm. This looks like something that can be very straight forwardly be
>> made part of the user namespace. If you ever mount binfmt_misc in the
>> user namespace you get the new behavior. Otherwise you get the existing
>> behavior.
>
> Thank you. I'll do that.
>
>> A user namespace will definitely be required, as otherwise you run the
>> risk of confusing root (and suid root exectuables0 by being able to
>> change the behavior of executables.
>>
>> What is the motivation for this? My impression is that very few people
>> tweak binfmt_misc.
>
> I think more and more people are using an interpreter like qemu
> linux-usermode to have a cross-compilation environment: they bootstrap a
> distro filesystems (with something like debootstrap), and then use
> binfmt_misc to run the compiler inside this environment (see for
> instance [1] [2] [3] or [4] [5]). This is interesting because you have
> more than a cross-compiler with that: you have also all the libraries of
> the target system, you can select exactly which target release you want
> to build to, with the exact same compiler and libraries versions (and
> you can re-use it you want to do maintenance on your project 10 years
> later...)
>
> The problem with this is you need to be root:
> 1- to chroot
> 2- to configure binfmt_misc
>
> We already can use "unshare --map-root-user chroot" to address the point
> 1, and this series tries to address the point 2.
>
> I think it's also interesting to have a per container configuration for
> binfmt_misc when the server administrator configures it and don't want
> to share each user configuration with all the other user ones (in
> something like docker or a cloud application).
OK. So it sounds like you are already needing a user namespace for
this. If this is your use case then my proposed method above seems to
fit rather well. James Bottomley was doing something similar that
connected to personality(2). That might be worth a look to see if there
is some synergy there.
>> I also don't think this raises to the level where it makes sense to
>> create a new namespace for this.
>
> OK.
>
> Thanks,
> Laurent
>
> [1] https://wiki.debian.org/Arm64Qemu
> [2] https://wiki.debian.org/M68k/sbuildQEMU
> [3] https://wiki.debian.org/RISC-V#Manual_qemu-user_installation
> [4] https://kbeckmann.github.io/2017/05/26/QEMU-instead-of-cross-compiling/
> [5] https://wiki.gentoo.org/wiki/Crossdev_qemu-static-user-chroot
^ permalink raw reply
* Re: [RFC 2/2] binfmt_misc: move data to binfmt_namespace
From: Jann Horn @ 2018-10-01 8:54 UTC (permalink / raw)
To: laurent
Cc: kernel list, linux-fsdevel, James Bottomley, Al Viro, Linux API,
Eric W. Biederman, dima, Andrei Vagin, containers,
Andy Lutomirski
In-Reply-To: <20180930234628.25528-3-laurent@vivier.eu>
On Mon, Oct 1, 2018 at 1:47 AM Laurent Vivier <laurent@vivier.eu> wrote:
> @@ -716,7 +711,8 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> if (!inode)
> goto out2;
>
> - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
> + err = simple_pin_fs(&bm_fs_type, &binfmt_ns(bm_mnt),
> + &binfmt_ns(entry_count));
> if (err) {
> iput(inode);
> inode = NULL;
> @@ -730,7 +726,8 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> if (IS_ERR(f)) {
> err = PTR_ERR(f);
> pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&binfmt_ns(bm_mnt),
> + &binfmt_ns(entry_count));
> iput(inode);
> inode = NULL;
> goto out2;
> @@ -743,9 +740,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> inode->i_fop = &bm_entry_operations;
>
> d_instantiate(dentry, inode);
> - write_lock(&entries_lock);
> - list_add(&e->list, &entries);
> - write_unlock(&entries_lock);
> + write_lock(&binfmt_ns(entries_lock));
> + list_add(&e->list, &binfmt_ns(entries));
> + write_unlock(&binfmt_ns(entries_lock));
This looks wrong. A write handler's behavior should not depend on the
namespace of the process that is using it.
Ideally, the affected namespace should depend on the file you're writing to.
If that's not possible, the affected namespace should at least be the
namespace of the process that opened the file.
^ permalink raw reply
* Re: [RFC 0/2] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2018-10-01 8:45 UTC (permalink / raw)
To: Eric W. Biederman, Andy Lutomirski
Cc: LKML, Linux FS Devel, James Bottomley, Al Viro, Linux API,
Dmitry Safonov, Andrey Vagin, Linux Containers
In-Reply-To: <874le63zei.fsf@xmission.com>
Le 01/10/2018 à 09:21, Eric W. Biederman a écrit :
> Andy Lutomirski <luto@kernel.org> writes:
>
>> On Sun, Sep 30, 2018 at 4:47 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>>
>>> This series introduces a new namespace for binfmt_misc.
>>>
>>
>> This seems conceptually quite reasonable, but I'm wondering if the
>> number of namespace types is getting out of hand given the current
>> API. Should we be considering whether we need a new set of namespace
>> creation APIs that scale better to larger numbers of namespace types?
>
> I would rather encourage a way to make this part of an existing
> namespace or find a way to make a mount of binfmt_misc control this.
>
> Hmm. This looks like something that can be very straight forwardly be
> made part of the user namespace. If you ever mount binfmt_misc in the
> user namespace you get the new behavior. Otherwise you get the existing
> behavior.
Thank you. I'll do that.
> A user namespace will definitely be required, as otherwise you run the
> risk of confusing root (and suid root exectuables0 by being able to
> change the behavior of executables.
>
> What is the motivation for this? My impression is that very few people
> tweak binfmt_misc.
I think more and more people are using an interpreter like qemu
linux-usermode to have a cross-compilation environment: they bootstrap a
distro filesystems (with something like debootstrap), and then use
binfmt_misc to run the compiler inside this environment (see for
instance [1] [2] [3] or [4] [5]). This is interesting because you have
more than a cross-compiler with that: you have also all the libraries of
the target system, you can select exactly which target release you want
to build to, with the exact same compiler and libraries versions (and
you can re-use it you want to do maintenance on your project 10 years
later...)
The problem with this is you need to be root:
1- to chroot
2- to configure binfmt_misc
We already can use "unshare --map-root-user chroot" to address the point
1, and this series tries to address the point 2.
I think it's also interesting to have a per container configuration for
binfmt_misc when the server administrator configures it and don't want
to share each user configuration with all the other user ones (in
something like docker or a cloud application).
> I also don't think this raises to the level where it makes sense to
> create a new namespace for this.
OK.
Thanks,
Laurent
[1] https://wiki.debian.org/Arm64Qemu
[2] https://wiki.debian.org/M68k/sbuildQEMU
[3] https://wiki.debian.org/RISC-V#Manual_qemu-user_installation
[4] https://kbeckmann.github.io/2017/05/26/QEMU-instead-of-cross-compiling/
[5] https://wiki.gentoo.org/wiki/Crossdev_qemu-static-user-chroot
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox