diff mbox series

[v3] mm/memfd: Add write seals when apply SEAL_EXEC to executable memfd

Message ID 20221202013404.163143-7-jeffxu@google.com (mailing list archive)
State New
Headers show
Series [v3] mm/memfd: Add write seals when apply SEAL_EXEC to executable memfd | expand

Commit Message

Jeff Xu Dec. 2, 2022, 1:34 a.m. UTC 
From: Jeff Xu <jeffxu@chromium.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@chromium.org>
---
 mm/memfd.c                                 |  3 +++
 tools/testing/selftests/memfd/memfd_test.c | 25 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)

Comments

Daniel Verkamp Dec. 2, 2022, 11:23 p.m. UTC | #1 
On Thu, Dec 1, 2022 at 5:36 PM <jeffxu@chromium.org> wrote:
>
> From: Jeff Xu <jeffxu@chromium.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@chromium.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
Jeff Xu Dec. 3, 2022, 2:29 a.m. UTC | #2 
Hi Daniel

Thanks for your review.

On Fri, Dec 2, 2022 at 3:24 PM Daniel Verkamp <dverkamp@chromium.org> wrote:
>
> On Thu, Dec 1, 2022 at 5:36 PM <jeffxu@chromium.org> wrote:
> >
> > From: Jeff Xu <jeffxu@chromium.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@chromium.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.
>
If we take error out approach, application need to add
F_SEAL_SHRINK|F_SEAL_GROW|F_SEAL_WRITE|F_SEAL_FUTURE_WRITE
when F_SEAL_EXEC is used.
Personally I think it is a bit long. From an API point of view, we can
think of this as
sealing the whole executable instead of just "X" bit.

If there is a new type of write SEAL in future, all applications need
to be updated, that is much harder,
and updating the kernel is easier.

Maybe I should remove F_SEAL_SEAL, so this code is still correct if a
new type of "Non-Write" seal  is added in future.

> 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.
>
There are also step 4.
4. call exec on the memfd,
In confused deputy attack, attacker wants to inject content into memfd
before step 4,
because step 4 is by a privilege process, attackers can gain root
escalation this way.

Ideally step 2 rewrites the whole memfd,  (injecting  content between
1 and 2 won't work), and
step 3 is the next line after 2, making the process to stop  exactly
between 2 and 3 is not easy.

So enforcing W^X can reduce the attack surface. It also defines the
most secure way for dev,
or else, dev might:
- forget to apply the W seal.
- choose to apply X and W seal in multiple calls, thus adding a gap.

> Thanks,
> -- Daniel

Thanks
Jeff
diff mbox series

Patch

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;
 
diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
index 775c9e6c061e..0731e5b3cdce 100644
--- a/tools/testing/selftests/memfd/memfd_test.c
+++ b/tools/testing/selftests/memfd/memfd_test.c
@@ -32,6 +32,13 @@ 
 #define F_SEAL_EXEC	0x0020
 #endif
 
+#define F_ALL_SEALS (F_SEAL_SEAL | \
+		     F_SEAL_SHRINK | \
+		     F_SEAL_GROW | \
+		     F_SEAL_WRITE | \
+		     F_SEAL_FUTURE_WRITE | \
+		     F_SEAL_EXEC)
+
 #ifndef MAX_PATH
 #define MAX_PATH 256
 #endif
@@ -1006,6 +1013,7 @@  static void test_exec_seal(void)
 
 	printf("%s SEAL-EXEC\n", memfd_str);
 
+	printf("%s	Apply SEAL_EXEC\n", memfd_str);
 	fd = mfd_assert_new("kern_memfd_seal_exec",
 			    mfd_def_size,
 			    MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_EXEC);
@@ -1024,7 +1032,24 @@  static void test_exec_seal(void)
 	mfd_fail_chmod(fd, 0700);
 	mfd_fail_chmod(fd, 0100);
 	mfd_assert_chmod(fd, 0666);
+	mfd_assert_write(fd);
+	close(fd);
+
+	printf("%s	Apply ALL_SEALS\n", memfd_str);
+	fd = mfd_assert_new("kern_memfd_seal_exec",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_EXEC);
 
+	mfd_assert_mode(fd, 0777);
+	mfd_assert_chmod(fd, 0700);
+
+	mfd_assert_has_seals(fd, 0);
+	mfd_assert_add_seals(fd, F_SEAL_EXEC);
+	mfd_assert_has_seals(fd, F_ALL_SEALS);
+
+	mfd_fail_chmod(fd, 0711);
+	mfd_fail_chmod(fd, 0600);
+	mfd_fail_write(fd);
 	close(fd);
 }