Message-Id: <dadfd8d3-fe83-4aee-b8aa-3b31d4408f66@app.fastmail.com>
Date:   Wed, 02 Aug 2023 09:56:42 +0200
From:   "David Rheinsberg" <david@...dahead.eu>
To:     "Jeff Xu" <jeffxu@...omium.org>
Cc:     "Jeff Xu" <jeffxu@...gle.com>, linux-kernel@...r.kernel.org,
        "Andrew Morton" <akpm@...ux-foundation.org>,
        "Kees Cook" <keescook@...omium.org>,
        "Daniel Verkamp" <dverkamp@...omium.org>, linux-mm@...ck.org,
        "Peter Xu" <peterx@...hat.com>, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] memfd: support MFD_NOEXEC alongside MFD_EXEC

Hi Jeff!

On Tue, Aug 1, 2023, at 9:24 PM, Jeff Xu wrote:
>> My point is, an application might decide to *not* seal a property, because it knows it has to change it later on. But it might still want to disable the executable bit initially, so to avoid having executable pages around that can be exploited.
>>
>
> I understand that.
> My argument was this application can do this in two steps, as in my
> previous email:
> 1> memfd_create(MFD_EXEC)
> 2> chmod
>
> Two system calls back to back isn't too terrible,  and I know this
> might seem to be not optimized for your user case, I will explain it
> later, please read on.

Yes, I agree that MFD_NOEXEC would be rather simple to imitate. So this is mostly a discussion about the intention and side-effects of this workaround, which is also likely why we haven't found an agreement, yet.

[...]
>> I think I didn't get my point across. Imagine an application that does *NOT* use sealing, but uses memfds. This application shares memfds with untrusted clients, and does this in a safe way (SIGBUS protected). Everything works fine, unless someone decides to enable `vm.memfd_noexec=2`. Suddenly, the memfd will have sealing enabled *without* the application ever requesting this. Now any untrusted client that got the memfd can add seals to the memfd, even though the creator of the memfd did not enable sealing. This client can now seal WRITES on the memfd, even though it really should not be able to do that.
>>
>> (This is not an hypothetical setup, we have such setups for data sharing already)
>
> Thanks, this helps me understand your point better.
>
> I'm not convinced that sysctl needs to consider the threat model of
> "someone" changing and breaking an application.  If we follow that
> threat model, there are a lot of other sysctls to worry about.
>
> Also, in the system that you described, if memfd is handled to an
> untrusted process, not only "sealing"  can cause damage, but also
> chmod, arbitrary rw,  imo the right approach is to harden the process
> or mechanism of passing the memfd.

No. The model I describe is carefully designed to hand out file-descriptors to inodes that the clients have *no* access to. They cannot run fchmod(2), unlink(2), etc. All they can do is operate on the open file. And all access to this shared file is properly guarded against possible damage the other concurrent clients can do. The entire model is already hardened against malicious actors.

With the new sysctl, a new attack-vector is introduced, which was not possible before.

I was *explicitly* told to add `MFD_ALLOW_SEALING` for that exact reason when introducing memfd_create(2). So I am a bit baffled why it is now ok to enable sealing behind the users back.

I agree that the new sysctl is a root-only option. But I fail to see *why* it implies `MFD_ALLOW_SEALING`? This behavior is not documented nor is it explained in the original commit-messages, nor mentioned *anywhere*.

>> Thus, setting the security-option `memfd_noexec` *breaks* applications, because it enables sealing. If `MFD_NOEXEC_SEAL` would *not* imply `MFD_ALLOW_SEALING`, this would not be an issue. IOW, why does ´MFD_NOEXEC_SEAL` clear `F_SEAL_SEAL` even if `MFD_ALLOW_SEALING` is not set?
>>
>
> If MFD_NOEXEC_SEAL is not desired, then it should not be used to
> overwrite memfd_create() in this system.
>
> For the question of why the sysctl adding a seal without application
> setting it , the rationale here is, as summary of previous/this
> emails:

I still think we are not talking about the same thing. I completely understand why you add the seal! I am just questioning why you *CLEAR* `F_SEAL_SEAL`? That is, why do you enable `MFD_ALLOW_SEALING` without the user requesting it? You could just set `F_SEAL_EXEC` without clearing `F_SEAL_SEAL`. And then require `MFD_ALLOW_SEALING` on top to clear `F_SEAL_SEAL`.

[...]
>> The downside of `MFD_NOEXEC` is that it might be picked over `MFD_NOEXEC_SEAL` by uneducated users, thus reducing security. But right now, the alternative is that existing code picks `MFD_EXEC` instead and never clears the executable bit, because it is a hassle to do so.
>>
>
> Yes. This is the downside I was thinking about.
>
> I lean to believe the kernel API shouldn't be feature rich, it could
> be simple, optimized towards the majority of user cases, and ideally,
> is self-explained without devs to look through documentation. For
> example, if I had to choose one to implement between MFD_NOEXEC and
> MFD_NOEXEC_SEAL, I would choose MFD_NOEXEC_SEAL because it should be
> what most users care about.

Well, if we were to go back, we would make MFD_NOEXEC(_SEAL) the default and just add `MFD_EXEC` :)

>> Or is there another reason *not* to include `MFD_NOEXEC`? I am not sure I understand fully why you fight it so vehemently?
>>
>
> I wouldn't add it myself, I hope to convince you not to :-).
> If you still think it is beneficial to add MFD_NOEXEC (saving one
> chmod call and making it easy to use), I wouldn't feel bad about that.
> I would suggest going with documentation to help devs to choose
> between those two, i.e. recommend MFD_NOEXEC_SEAL in most cases.

Any application that cannot use `F_SEAL_EXEC` (e.g., because its peers verify for historic reasons that the seal is not set) now has to do an extra dance to get the "safer" behavior, rather than getting the "safer" behavior by default. That is, we make it easier to get the unsafe behavior than to get the safe behavior (in this particular scenario).

Without `MFD_NOEXEC`, it is easier to end up with a 0777 memfd than not. I want the application that desires `S_IXUSR` to jump through hoops, not the application that does *not* require it.

In other words, I would prefer `MFD_ALLOW_EXEC`, which requires fchmod(2)` to set `S_IXUSR`, rather than requiring a call to fchmod(2) to clear it for everyone that does not need it.

Thanks
David

Powered by blists - more mailing lists