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:
- 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