Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <7655a573-544f-05a4-36dc-0c84c73ac9ee@gmail.com>
In-Reply-To: <7655a573-544f-05a4-36dc-0c84c73ac9ee@gmail.com>
From:   Jann Horn <jannh@google.com>
Date:   Tue, 27 Oct 2020 13:51:30 +0100
Message-ID: <CAG48ez2W+p2RuAPMyxE+hggNtHsVx3-NHCawk18_F5tonX20LQ@mail.gmail.com>
Subject: Re: Inconsistent capability requirements for prctl_set_mm_exe_file()
To:     "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc:     Nicolas Viennot <Nicolas.Viennot@twosigma.com>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Adrian Reber <areber@redhat.com>,
        Cyrill Gorcunov <gorcunov@gmail.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Kirill Tkhai <ktkhai@virtuozzo.com>,
        Oleg Nesterov <oleg@redhat.com>,
        Pavel Emelyanov <ovzxemul@gmail.com>,
        Kees Cook <keescook@chromium.org>,
        Casey Schaufler <casey@schaufler-ca.com>,
        lkml <linux-kernel@vger.kernel.org>,
        linux-security-module <linux-security-module@vger.kernel.org>,
        Andrei Vagin <avagin@gmail.com>,
        Dmitry Safonov <0x7f454c46@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Oct 27, 2020 at 1:11 PM Michael Kerrisk (man-pages)
<mtk.manpages@gmail.com> wrote:
> @Nicolas, your commit ebd6de6812387a changed the capability
> requirements for the prctl_set_mm_exe_file() operation from
>
>     ns_capable(CAP_SYS_ADMIN)
>
> to
>
>     ns_capable(CAP_SYS_ADMIN) || ns_capable(CAP_CHECKPOINT_RESTORE).
>
> That's fine I guess, but while looking at that change, I found
> an anomaly.
>
> The same prctl_set_mm_exe_file() functionality is also available
> via the prctl() PR_SET_MM_EXE_FILE operation, which was added
> by Cyrill's commit b32dfe377102ce668. However, there the
> prctl_set_mm_exe_file() operation is guarded by a check
>
>     capable(CAP_SYS_RESOURCE).
>
> There are two things I note:
>
> * The capability requirements are different in the two cases.
> * In one case the checks are with ns_capable(), while in the
>   other case the check is with capable().
>
> In both cases, the inconsistencies predate Nicolas's patch,
> and appear to have been introduced in Kirill Tkhai's commit
> 4d28df6152aa3ff.
>
> I'm not sure what is right, but those inconsistencies seem
> seem odd, and presumably unintended. Similarly, I'm not
> sure what fix, if any, should be applied. However, I thought
> it worth mentioning these details, since the situation is odd
> and surprising.

FWIW, as a bit of context here: I believe that these checks are more
driven by "what capabilitiies do we think a typical caller will have"
than by a proper security design of "what capabilities do we have to
require to establish certain security guarantees". As people have
noted elsewhere, on a system without LSMs, a process can point
/proc/self/exe to almost any executable file of its choice anyway (by
executing that file and then replacing the executable code of the
resulting process).

The properly engineered solution would probably be to let LSMs hook
these APIs (if they care) and then remove the capable()/ns_capable()
checks.