Message-ID: <20211226150310.GA992@1wt.eu>
Date:   Sun, 26 Dec 2021 16:03:10 +0100
From:   Willy Tarreau <w@....eu>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Waiman Long <longman@...hat.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Kees Cook <keescook@...omium.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Laurent Vivier <laurent@...ier.eu>,
        YunQiang Su <ysu@...ecomp.com>, Helge Deller <deller@....de>
Subject: Re: [PATCH] exec: Make suid_dumpable apply to SUID/SGID binaries
 irrespective of invoking users

Hi Eric,

I've experimented a bit with this and am not completely convinced we're
on the right track.

First, resetting the not-dumpable status on setuid()/setsid() and friends
means that sudo itself resets it, and as such that the executable that it
launches may very well crash on CPU limits for example, thus allowing a
sudoer to produce a root core in a random directory. That's the current
state of affairs after the first attached patch.

This made me think that since we want to protect the called program and
not sudo itself, we ought to verify that the called program performs the
action itself. I.e. only programs that are marked as dumpable may reset
their not-dumpable status on various actions. That's what the second patch
does.

Doing this worked a lot better and initially made me think it solved the
issue: a user becoming root via sudo could regularly dump cores, but
crashes during startup would not work. That was until I tested with
"sudo ping" and saw root cores again, because ping, like many setuid
enabled programs, takes care of its permissions via setuid(0).

So this makes me think that trying to infer a program's intents via
snooping a few syscalls isn't going very far. Earlier in this
conversation there were a few other proposals around just playing with
RLIMIT_CORE and PR_SET_DUMPABLE.

I tend to think that if we combine the principle above (only monitor
dumpable programs) with the only two actions that *really* act on the
ability to produce a core (rlimit and prctl) then it might actually
reasonably work, because then a program could explicitly want to enable
core dumps and be allowed to do that. That's what the last patch does
and I couldn't find a case where it doesn't work. I.e. switching from
a user to root via sudo allows me to dump a core from a shell as the
shell sets RLIMIT_CORE, but will not let a program such as "ping" dump
a core by default for example. I've put that in the last patch which
replaces the first two ones.

I'm still wondering if adding a 4th value to suid_dumpable wouldn't
solve all of this: the value would automatically change when setting
RLIMIT_CORE. It could just be a bit more confusing to configure.

Other (orthogonal) approaches could consist in forcefully resetting a
few limits on suidexec. At least memory limits and CPU limits could be
reset to default (unlimited) values since there doesn't seem to be any
valid reason for an unprivileged process to change them for a privileged
one. And the core dump limits could be set to zero for the same reason.
It could be decided that we'd never dump a core on user-addressable
signals (SIGQUIT, maybe others?) and that could be even cleaner than
the currently discussed solutions.

Please let me know what you think.

Thanks,
Willy

View attachment "0001-WIP-only-remove-non-dumpable-in-the-non-suid-program.patch" of type "text/plain" (3293 bytes)

View attachment "0002-WIP-only-change-not_dumpable-via-prctl-and-setrlimit.patch" of type "text/plain" (2305 bytes)

View attachment "0003-coredump-disable-core-dumps-when-transitionning-via-.patch" of type "text/plain" (3662 bytes)

View attachment "dumpable.c" of type "text/plain" (747 bytes)

Powered by blists - more mailing lists