Message-ID: <CABRcYm+RquNGYUyfCEmLrcaoMSKagzwXgto-24ZCaLAdFUKUjg@mail.gmail.com>
Date:   Mon, 8 May 2023 14:12:21 +0200
From:   Florent Revest <revest@...omium.org>
To:     Peter Xu <peterx@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        akpm@...ux-foundation.org, catalin.marinas@....com,
        anshuman.khandual@....com, joey.gouly@....com, mhocko@...e.com,
        keescook@...omium.org, david@...hat.com, izbyshev@...ras.ru,
        nd@....com, broonie@...nel.org, szabolcs.nagy@....com,
        toiwoton@...il.com, lennart@...ttering.net
Subject: Re: [PATCH 0/4] MDWE without inheritance

On Mon, May 8, 2023 at 3:29 AM Peter Xu <peterx@...hat.com> wrote:
>
> On Fri, May 05, 2023 at 06:42:08PM +0200, Florent Revest wrote:
> > On Thu, May 4, 2023 at 10:06 PM Peter Xu <peterx@...hat.com> wrote:
> > >
> > > On Thu, May 04, 2023 at 07:09:38PM +0200, Florent Revest wrote:
> > > > Joey recently introduced a Memory-Deny-Write-Executable (MDWE) prctl which tags
> > > > current with a flag that prevents pages that were previously not executable from
> > > > becoming executable.
> > > > This tag always gets inherited by children tasks. (it's in MMF_INIT_MASK)
> > > >
> > > > At Google, we've been using a somewhat similar downstream patch for a few years
> > > > now. To make the adoption of this feature easier, we've had it support a mode in
> > > > which the W^X flag does not propagate to children. For example, this is handy if
> > > > a C process which wants W^X protection suspects it could start children
> > > > processes that would use a JIT.
> > > >
> > > > I'd like to align our features with the upstream prctl. This series proposes a
> > > > new NO_INHERIT flag to the MDWE prctl to make this kind of adoption easier. It
> > > > sets a different flag in current that is not in MMF_INIT_MASK and which does not
> > > > propagate.
> > >
> > > I don't think I have enough context, so sorry if I'm going to ask a naive
> > > question..
> >
> > Not at all! :) You're absolutely right, it's important to address these points.
> >
> > > I can understand how current MDWE helps on not allowing any modifi-able
> > > content from becoming executable.  How could NO_INHERIT help if it won't
> > > inherit and not in MMF_INIT_MASK?
> >
> > The way I see it, enabling MDWE is just a small step towards hardening
> > a binary anyway. It can possibly make exploitation a bit harder in the
> > case where the attacker has _just_: a write primitive they can use to
> > write a shellcode somewhere and a primitive to make that page
> > executable later. It's a fairly narrow protection already and I think
> > it only really helps as part of a broader "defense in depth" strategy.
> >
> > > IIUC it means the restriction will only apply to the current process.  Then
> > > I assume the process can escape from this rule simply by a fork().  If so,
> > > what's the point to protect at all?
> >
> > If we assume enough control from the attacker, then MDWE is already
> > useless since it can be bypassed by writing to a file and then
> > mmapping that file with PROT_EXEC. I think that's a good example of
> > how "perfect can be the enemy of good" in security hardening. MDWE
> > isn't a silver-bullet but it's a cheap trick and it makes a small dent
> > in reducing the attack surface so it seems worth having anyway ?
> >
> > But indeed, to address your question, if you choose to use this
> > NO_INHERIT flag: you're no longer protected if the attacker can fork()
> > as part of their exploitation. I think it's been a useful trade-off
> > for our internal users since, on the other hand, it also makes
> > adoption a lot easier: our C++ services developers can trivially opt
> > into a potpourri of hardening features without having to think too
> > much about how they work under-the-hood. The default behavior has been
> > to use a NO_INHERIT strategy so users don't get bad surprises the day
> > when they try to spawn a JITted subcommand. In the meantime, their C++
> > service still gets a little bit of extra protection.
> >
> > > And, what's the difference of this comparing to disabling MDWE after being
> > > enabled (which seems to be forbidden for now, but it seems fork() can play
> > > a similar role of disabling it)?
> >
> > That would be functionally somewhat similar, yes. I think it mostly
> > comes down to ease of adoption. I imagine that users who would opt
> > into NO_INHERIT are those who are interested in MDWE for the binary
> > they are writing but aren't 100% confident in what subprocesses they
> > will run and so they don't have to think about disabling it after
> > every fork.
>
> Okay, that makes sense to me.  Thanks.
>
> Since the original MDWE was for systemd, I'm wondering what will happen if
> some program like what you said is invoked by systemd and with MDWE enabled
> already.

Good question

> Currently in your patch IIUC MDWE_NO_INHERIT will fail directly on MDWE
> enabled process,

Yes, I tried to stay close to the spirit of the existing logic (which
doesn't allow any sort of privilege gains) but this is not
particularly a requirement on our side so I'm quite flexible here.
Maybe Joey has an input here ?

> but then it makes me think whether it makes more sense to
> allow converting MDWE->MDWE_NO_INHERIT in this case.  It seems to provide a
> most broad coverage on system daemons using MDWE starting from systemd
> initial process, meanwhile allows specific daemons to fork into anything
> like a JIT process so it can make itself NO_INHERIT.  Attackers won't
> leverage this because MDWE_NO_INHERIT also means MDWE enabled.

I should have cc-ed systemd folks who could have opinions on this. I
will for v2.

+ cc Topi & Lennart

Powered by blists - more mailing lists