Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sandbox-seccomp-filter: allow mprotect syscall #142

Closed
wants to merge 1 commit into from
Closed

sandbox-seccomp-filter: allow mprotect syscall #142

wants to merge 1 commit into from

Conversation

This allows to use openssh-portable with OpenBSD malloc and GrapheneOS hardened_malloc.
@thestinger
Copy link

thestinger commented Aug 22, 2019

The mprotect call you're linking in OpenBSD malloc is technically only called during initialization, before the sandbox is entered, but these are called at runtime:

For malloc_freeunmap (U):

https://github.com/openbsd/src/blob/df69c215c7c66baf660f3f65414fd34796c96152/lib/libc/stdlib/malloc.c#L719-L720
https://github.com/openbsd/src/blob/df69c215c7c66baf660f3f65414fd34796c96152/lib/libc/stdlib/malloc.c#L783-L784
https://github.com/openbsd/src/blob/df69c215c7c66baf660f3f65414fd34796c96152/lib/libc/stdlib/malloc.c#L800-L801

For malloc(0):

https://github.com/openbsd/src/blob/df69c215c7c66baf660f3f65414fd34796c96152/lib/libc/stdlib/malloc.c#L899-L901
https://github.com/openbsd/src/blob/df69c215c7c66baf660f3f65414fd34796c96152/lib/libc/stdlib/malloc.c#L1112-L1113

For malloc_guard (G):

https://github.com/openbsd/src/blob/df69c215c7c66baf660f3f65414fd34796c96152/lib/libc/stdlib/malloc.c#L1149-L1154

The hardened_malloc implementation is mostly using mmap with MAP_FIXED to both purge and protect regions at the same time, but it uses mprotect to make regions readable/writable to allocate new slabs, allocate metadata and to allocate the usable portion of the large allocation mapping after mapping a PROT_NONE region with guards. It could technically use mmap with MAP_FIXED for all of these cases but mprotect makes more sense and is probably faster.

@thestinger
Copy link

I think glibc malloc also uses mprotect, but it just isn't getting triggered by OpenSSH at the moment. It might only be used with threads.

@brynet
Copy link

brynet commented Aug 22, 2019

It's also worth clarifying that there is no portable version of OpenBSD malloc(3), and that people using it on platforms other than OpenBSD are kind of doing their own thing.

OpenSSH on OpenBSD wouldn't be using the sandbox-seccomp-filter code, but instead sandbox-pledge, which doesn't have this problem.

Note: I'm not stating an opinion on whether adding mprotect to the Linux seccomp-filter sandbox makes sense or not for OpenSSH. I will say that on OpenBSD, mprotect(2) is permitted by the "stdio" pledge promise. However, like mmap(2), the PROT_EXEC prot isn't allowed by default.

@lukateras
Copy link
Author

lukateras commented Aug 23, 2019

It's also worth clarifying that there is no portable version of OpenBSD malloc(3), and that people using it on platforms other than OpenBSD are kind of doing their own thing.

There is a really old unofficial port, that said, you're right. However, it does make a point that other malloc implementations might also want to use mprotect(2) for the same reasons OpenBSD implementation does.

However, like mmap(2), the PROT_EXEC flag isn't allowed by default.

As far as I can tell, seccomp filter will currently pass through PROT_EXEC flag for mmap(2):

#ifdef __NR_mmap
SC_ALLOW(__NR_mmap),
#endif

In which case, passing through PROT_EXEC for mprotect(2) seems fair.

djmdjm added a commit that referenced this pull request Aug 23, 2019
Used by some hardened heap allocators. Requested by Yegor
Timoshenko in #142
@lukateras
Copy link
Author

Looks like this has been addressed in f6906f9. Thank you!

@djmdjm
Copy link
Contributor

djmdjm commented Aug 23, 2019

Yeah, I don't want to allow PROT_EXEC mappings as these seem useful in bootstrapping a memory fault exploit to full RCE.

I've added a SC_ALLOW_ARG_MASK() to allow permission of syscalls whose arguments match a particular mask. In this case mprotect() with only PROT_READ|PROT_WRITE|PROT_NONE is permitted.

@djmdjm djmdjm closed this Aug 23, 2019
@lukateras
Copy link
Author

lukateras commented Aug 23, 2019

@djmdjm: Somewhat offtopic, would it make sense to limit PROT_EXEC for mmap(2) in that case?

@djmdjm
Copy link
Contributor

djmdjm commented Aug 23, 2019 via email

@lukateras lukateras deleted the patch-1 branch August 23, 2019 00:24
@adrelanos
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants