linux-hardening - Re: [PATCH v3] mm/memfd: Add write seals when apply SEAL

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]

Message-ID: <CABVzXAkoGoypAs86EG5RsJZ=CXPu3NtTHb7_2=byQt7A7p7krQ@mail.gmail.com>
Date:   Fri, 2 Dec 2022 15:23:41 -0800
From:   Daniel Verkamp <dverkamp@...omium.org>
To:     jeffxu@...omium.org
Cc:     skhan@...uxfoundation.org, keescook@...omium.org,
        akpm@...ux-foundation.org, dmitry.torokhov@...il.com,
        hughd@...gle.com, jeffxu@...gle.com, jorgelo@...omium.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, mnissler@...omium.org, jannh@...gle.com,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH v3] mm/memfd: Add write seals when apply SEAL_EXEC to
 executable memfd

On Thu, Dec 1, 2022 at 5:36 PM <jeffxu@...omium.org> wrote:
>
> From: Jeff Xu <jeffxu@...omium.org>
>
> When apply F_SEAL_EXEC to an executable memfd, add write seals also to
> prevent modification of memfd.
>
> Signed-off-by: Jeff Xu <jeffxu@...omium.org>
> ---
>  mm/memfd.c                                 |  3 +++
>  tools/testing/selftests/memfd/memfd_test.c | 25 ++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 96dcfbfed09e..3a04c0698957 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -222,6 +222,9 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
>                 }
>         }
>
> +       if (seals & F_SEAL_EXEC && inode->i_mode & 0111)
> +               seals |= F_ALL_SEALS;
> +
>         *file_seals |= seals;
>         error = 0;
>

Hi Jeff,

(Following up on some discussion on the original review, sorry for any
duplicate comments.)

Making F_SEAL_EXEC imply all seals (including F_SEAL_SEAL) seems a bit
confusing. This at least needs documentation to make it clear.

Rather than silently adding other seals, perhaps we could return an
error if the caller requests F_SEAL_EXEC but not the write seals, so
the other seals would have to be explicitly listed in the application
code. This would have the same net effect without making the
F_SEAL_EXEC operation too magical.

Additionally, if the goal is to enforce W^X, I don't think this
completely closes the gap. There will always be a period where it is
both writable and executable with this API:

1. memfd_create(MFD_EXEC). Can't use MFD_NOEXEC since that would seal
chmod(+x), so the memfd is W + X here.
2. write() code to the memfd.
3. fcntl(F_ADD_SEALS, F_SEAL_EXEC) to convert the memfd to !W + X.

I think one of the attack vectors involved the attacker waiting for
another process to create a memfd, pausing/delaying the victim
process, overwriting the memfd with their own code, and calling exec()
on it, which is still possible in the window between steps 1 and 3
with this design.

Thanks,
-- Daniel