| Message ID | 20221202013404.163143-7-jeffxu@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kees Cook |
| Headers | show |
| Series | [v3] mm/memfd: Add write seals when apply SEAL_EXEC to executable memfd | expand |
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
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 --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); }