linux-kernel - Re: [REVIEW][PATCH] exec: Don't exec files the userns root can not read.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Oct 2016 11:38:18 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Jann Horn <jann@...jh.net>, Michal Hocko <mhocko@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux Containers <containers@...ts.linux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Linux FS Devel <linux-fsdevel@...r.kernel.org>
Subject: Re: [REVIEW][PATCH] exec: Don't exec files the userns root can not read.

On Wed, Oct 19, 2016 at 10:55 AM, Eric W. Biederman
<ebiederm@...ssion.com> wrote:
> Andy Lutomirski <luto@...capital.net> writes:
>
>> On Wed, Oct 19, 2016 at 10:29 AM, Jann Horn <jann@...jh.net> wrote:
>>> On Wed, Oct 19, 2016 at 11:52:50AM -0500, Eric W. Biederman wrote:
>>>> Andy Lutomirski <luto@...capital.net> writes:
>>>> > Simply ptrace yourself, exec the
>>>> > program, and then dump the program out.  A program that really wants
>>>> > to be unreadable should have a stub: the stub is setuid and readable,
>>>> > but all the stub does is to exec the real program, and the real
>>>> > program should have mode 0500 or similar.
>>>> >
>>>> > ISTM the "right" check would be to enforce that the program's new
>>>> > creds can read the program, but that will break backwards
>>>> > compatibility.
>>>>
>>>> Last I looked I had the impression that exec of a setuid program kills
>>>> the ptrace.
>>>>
>>>> If we are talking about a exec of a simple unreadable executable (aka
>>>> something that sets undumpable but is not setuid or setgid).  Then I
>>>> agree it should break the ptrace as well and since those programs are as
>>>> rare as hens teeth I don't see any problem with changing the ptrace behavior
>>>> in that case.
>>>
>>> Nope. check_unsafe_exec() sets LSM_UNSAFE_* flags in bprm->unsafe, and then
>>> the flags are checked by the LSMs and cap_bprm_set_creds() in commoncap.c.
>>> cap_bprm_set_creds() just degrades the execution to a non-setuid-ish one,
>>> and e.g. ptracers stay attached.
>>
>> I think you're right.  I ought to be completely sure because I rewrote
>> that code back in 2005 or so back when I thought kernel programming
>> was only for the cool kids.  It was probably my first kernel patch
>> ever and it closed an awkward-to-exploit root hole.  But it's been a
>> while.  (Too bad my second (IIRC) kernel patch was more mundane and
>> fixed the mute button on "new" Lenovo X60-era laptops and spend
>> several years in limbo...)
>
> Ah yes and this is only a problem if the ptracer does not have
> CAP_SYS_PTRACE.
>
> If the tracer does not have sufficient permissions any opinions on
> failing the exec or kicking out the ptracer?  I am leaning towards failing
> the exec as it is more obvious if someone cares.  Dropping the ptracer
> could be a major mystery.

I would suggest leaving it alone.  Changing it could break enough
things that a sysctl would be needed, and I just don't see how this is
a significant issue, especially since it's been insecure forever.
Anyone who cares should do the stub executable trick:

/sbin/foo: 04755, literally just does execve("/sbin/foo-helper");

/sbin/foo-helper: 0500.

--Andy