-
Notifications
You must be signed in to change notification settings - Fork 75
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
Prot exec fixes #136
base: master
Are you sure you want to change the base?
Prot exec fixes #136
Conversation
Currently allocating 1 byte of executable memory then releasing it should do the same thing, isn't it? That is used by pcre2 to detect the availability of jit. |
The problem with allocating rwx memory is that it'll trigger a violation leading to logs. Using mmap(rw-) + mprotect(rwx) avoids that. |
And I fail to see where pcre2 would do this kind of detection. In fact, I'm still able to trigger the bug with pcre2's current HEAD:
That's the reason why I tried to provide a fix :) |
Isn't that a good thing? If you check logs, you are aware that something is wrong (and enable another allocator). You would not notice that otherwise. Maybe dynamic selection of the right allocator could be a good idea. |
https://github.com/PCRE2Project/pcre2/blob/master/src/pcre2_jit_compile.c#L14450 |
Well, currently By implementing support for detecting the problematic case and handling it (instead of failing hard) all options for changes will be handled. If a distro will switch to a different allocator, JIT will be working. As long as it doesn't, users won't be left with broken applications. But distros will eventually update their packages so this support will be available for users sometime in the future, if this PR gets merged. |
https://github.com/PCRE2Project/pcre2/blob/master/src/pcre2_jit_compile.c#L14464-L14465 Yeah, that's probably the error I see with |
My change to pcre2 avoids running into this error by making |
That error should be silently ignored. I don't get why it prevents git grep from running. |
That's because |
To me it seems |
The truth is @carenas is our allocator expert, but for this would be the right solution: |
There have been a few attempts in the past to get it fixed in git, but nothing has actually landed upstream: :( So, arguably, no changes to pcre2 or sljit are needed either. All users of |
My proposed solution fulfills goals a) and c). b) is a worthwhile goal but needs some kind of "middle layer" and should be done by a developer that's much more familiar with the intimate details of the code base than I am. It also needs downstream projects to switch over to this new allocator which they maybe don't want to? |
https://public-inbox.org/git/[email protected]/ would be the closest, but wasn't merged for whatever reason :/ |
this problem was caused by git using the JIT fastpath API, and therefore should be IMHO fixed there by adding the missing detection as shown by the above patch (which I didn't push hard enough to merge at that time) |
The patch adds a hidden api function to detect a special case, which needs to be known by those, who uses a compiler built on top of sljit. This is the difficult requirement. If the normal allocator is rewritten in a way that it does not add logs lines, that sounds like a good way to fix this. |
While I agree that |
It's an internal function for users that need it -- like pcre2: PCRE2Project/pcre2@17e45b0.
I fully agree. Having sljit implement the fallback itself, like tying execalloc first, then protexec, then interpreter mode would be a nice solution. That would, however, still create problems for downstream users like pcre2. Like, what should be the answer to This sounds like API changes to sort out with some more thought. In the meantime, my proposes change provides a solution for the broken use cases that actually fixes things and makes |
If git (or another program using pcre2) would be using the standard API then the error is not triggered because pcre2 fallbacks internally to not using JIT in these cases. the error was only visible because git uses an API that specifically avoids that check and is therefore lacking the error handler.
I was not arguing against expanding pcre2_config so it can report that JIT is not available, indeed I had some similar code that I use in SE BSD systems that does that and I just hadn't cleaned up, but that is similar and would certainly complement and enhance this. I would argue though that avoiding "logs" is misguided (and that also includes the way the test is done, so it is accurate). avoiding RWX maps can be done through 2 different policies and if yours doesn't alert through mprotect, doesn't imply that will prevent mmap. FWIW, Fedora/RHEL does provide SELinux policies that allow exceptions for git to use pcre2 with JIT in their default packages and something similar could be added to Debian/Ubuntu as an alternative. |
The error was visible, because pcre2 told
So you're saying this change should go into pcre2 only? I'm fine with that too, just wanted to provide the necessary infrastructure for other downstream projects. But if these should (re-)implement runtime support tests themselves, well, than it should be this way.
Spamming logs should be avoided as well, especially if this is for code that tries to implement a fallback. And that's what the test tries to do: detect the problematic situation without unnecessarily creating bogus log entries (because we very much handle the case of not being allowed to create rwx mappings). That's why the test is written the way it is.
It's an accurate test for PaX's MPROTECT feature. It tracks the possible transitions of setting
Not really. I just did a test on a CentOS 8 VM and if I enable [user@centos8 ~]$ git clone https://github.com/zherczeg/sljit.git
Klone nach 'sljit' ...
remote: Enumerating objects: 5039, done.
remote: Counting objects: 100% (431/431), done.
remote: Compressing objects: 100% (130/130), done.
remote: Total 5039 (delta 351), reused 349 (delta 301), pack-reused 4608
Empfange Objekte: 100% (5039/5039), 3.21 MiB | 9.00 MiB/s, fertig.
Löse Unterschiede auf: 100% (3974/3974), fertig.
[user@centos8 ~]$ cd sljit/
[user@centos8 sljit]$ git grep beer
fatal: Couldn't JIT the PCRE2 pattern 'beer', got '-48'
[user@centos8 sljit]$ su -
Passwort:
[root@centos8 ~]# semanage boolean -l -C
SELinux boolean State Default Description
deny_execmem (on , on) Allow deny to execmem
virt_sandbox_use_all_caps (on , on) Allow virt to sandbox use all caps
virt_use_nfs (on , on) Allow virt to use nfs So PaX implements a mechanism to disable the MPROTECT feature on a per-binary basis, which would allow to create rwx mappings and make the JIT work. However, that's a user's decision if that's really wanted. Such a policy decision shouldn't be buried in code but left to the user to decide -- just as it is with SELinux's |
I just noticed, I outsmarted myself with the 'mmap(rw-)+mprotect(rwx)' test, as that will also generate a log under grsecurity, as correctly noticed by @bspengler-oss. I just didn't see it in my tests because the I'll change it to a mmap(rwx) test which should address @carenas concern about testing the "wrong thing". |
e316ab7
to
bf78b1c
Compare
With the idea of automatically selecting an allocator being backed out, "b)" of #136 (comment) seems to be no longer relevant? Are there any other obstacles to merge this PR? It otherwise seems to fulfill the remaining goals and fixes existing use cases (PaX/grsecurity's MPROTECT and SELinux's deny_execmem). |
The main problem is adding new api is a burden to everybody, and that does not goes away. I have a question btw: jit is disabled by default, so if they enabled it, why they enabled it with a bad allocator? |
I already proposed that this new API function can be carried by downstream projects like PCRE2 only. I'm fine with that. However, other downstream projects would need to implement a similar functionality or use a different allocator, as you suggest, to handle the problem at hand. From my understanding is this project (sljit) just the bare JIT implementation which needs required plumping anyway. Providing an API function to test the availability of allocating JIT memory for the exec allocator on a given system is, IMHO, a tool that's needed by that plumbing layer. As for PCRE2, there is already an API around testing the availability of certain features. One on them is testing the availability of JIT via
So it only felt natural to hook the runtime testing API function there to prevent users of attempting (and run into errors) when making use of the JIT code generation. That's what commit PCRE2Project/pcre2@9dee7e8 does. (Please have a look if you haven't done so yet.) It makes use of the new sljit API function to test if runtime code generation is possible. It cannot be a compile time decision, nor a distro specific one, as that availability depends on the possibility to create rwx mappings and that can change on a per-binary or even on a per-process basis.
You're asking the wrong guy. I'm neither the maintainer of libpcre2 in Debian (or any of its derivatives) nor the maintainer of it in other distributions (as I showed above, CentOS with its SELinux
Btw, the "disk space" part is probably a benign problem, but the fact that Another reason might be, that it's not as robust as the exec allocator. The very first commit of this PR fixes an out-of-bounds stack read (and write). So it has (hopefully soon had) other security issues as well. |
sljit_src/sljitExecAllocator.c
Outdated
|
||
/* | ||
* Create a temporary mapping and try to make it writable to probe for rwx | ||
* support without generating a log message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "without generating a log message" part is not accurate, as that is the intention of the function but will be still happening unless PaX is being used and the logic above returns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! My tests where flawed, as /proc
was mounted so the test returned early. It will generate a log entry under grsecurity if /poc/self/status
cannot be read and the PaX flags looked up. I'll update the comment accordingly.
However, for SELinux no log message was generated for me -- at least when testing this on CentOS 8 with deny_execmem
enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
The prot allocator is a system exploit (a security hole). If you disable executable file mapping, you literally cannot run any application, because the Anyway, I think a system needs to be able to decide it wants jit or it does not want. Compiling a lot of dead code is pointless and a waste of disk space. Maybe we should just tell the distro to disable jit, because it is not usable. |
As the wx allocator seems to be BSD-specific, this just leaves the exec allocator for Linux, right? And that's the one I'm mucking with.
It's not, as I already mentioned above. It's a runtime property if JIT can be used or not. Let me quote myself: """ |
bf78b1c
to
bd57bff
Compare
This sounds bad. If you can switch its status during runtime any number of times, then testing does not seem useful. Now I remember something, that on apple you can allocate an executable page only once, and if you run out of that space, you cannot create more code. So the current, allocator returns null on error seems the most generic solution for this problem. |
That's not what I said. It's either per-binary or per-process but there's no runtime toggling while a process is running. So the state for a given process would be stable. |
Trying to move forward with this, as the current behavior isn't the preferred state (I want From what I got, @carenas seems to be in favor of either merging this PR (#136 (comment)) or adding the functionality to PCRE2 (#136 (comment)). @zherczeg, can you please decide what's your preferred option? |
As far as I understand there is a relatively simple bug in git, and this should be fixed there. I worry about the compatibility issues for this code, and the api is hidden and its name is not descriptive. |
It's two bugs, actually:
I can try to push getting the proposed fix into git. However, IMO the underlying bug is really that pcre2 gives the wrong result for |
I can also add documentation on the intended usage, if that's a problem. I just thought it's obvious. But sure, having an example using the function can't hurt other projects adopting the pattern. And if naming is an issue, we can surely go with a different name for |
The The executable allocators are not part of the jit compiler, they are just utility examples. Any project uses the compiler, should provide the following macros: They can provide them by using an example provided by sljit, or they can provide whatever they want. So other projects, such as pcre cannot expect the existence of your new function, i.e. it cannot use it. Also compatibility and temporary checks are also a problem. The mmap you call might fail because there is no memory, not because it cannot allocate the memory type. The solution is freeing some memory, and call the function again. But in that case the function gives a random result, not a static value, as it should. The current code returns with I understand you have a specific problem, but the picture is quite big unfortunately. |
my incomplete but similar in concept implementation in carenas/pcre2@83a8560 I think the added visibility that JIT was dynamically disabled through pcre2test is important, as it allows a conversation to be initiated to realize that an exception would be worth considering if keeping the speed up provided by JIT is desired. Tangentially, it would be IMHO interesting to consider a mechanism that could be added so that such exception could be enabled at a library level too. right now, if the application uses the system provided pcre2 library and it has by default a policy to deny RWX maps (like OpenBSD or NetBSD), then the only option (and the one that is taken) is to compile pcre2 without JIT (which is the default, anyway). |
IMHO there are already API extensions that are allocator specific (ex: which reminds me, there is still a patch pending that fixes that bug, but that also probably shows that most people using pcre2 do not provide their own allocator (which they definitely should if serious about security) |
[Sorry for the late reply but I was sick]
No, it does not. It claims to support JIT but when trying to use it, it'll fail because the exec allocator cannot map memory as needed. So please, why should an application attempt to make use of JIT (because
It's funny that you mention that. You should be very well aware of PCRE2 not providing its own allocator. In fact, you seem to be the de facto maintainer of SLJIT in PCRE2, as the commit history shows you as (almost) only contributor, updating it to the state as it's here. So if you have no intention do provide a proper allocator in PCRE2, probably no-one does and every other project also simply takes what's provided by SLJIT. Regarding the existence of the new function, please stop telling PCRE2 (or any other project) cannot be aware of it. That's simply not true. In fact, there's a PR eagerly waiting for this function to land, because it is very well aware of that function: PCRE2Project/pcre2#157. You should be aware of that one as well, I simply don't get why you keep on ignoring that?
So, this is all Linux only, right? Linux does memory over commitment by default, making a call to What might happen in the memory constraint case is that the application might get killed if over commitment hits and there's really no more memory left to demand page the Another pathological case for
Not
I think this "might succeed later" scenario can be ignored as an impossible case, as explained above -- the process likely gets killed in the OOM case. However, I'm happy to change the code to additionally test
That's why the
I wasn't aware that Chomium makes use of SLJIT, that's news to me. No hits in a code search, though.
Right, it's not only PaX's MPROTECT but SELinux's |
While visibility about the runtime disable is nice from a developer point of view, it probably doesn't matter much from a user's point of view -- as long as the application is still working. And that's what this PR is all about: The current state leads to broken applications and that's not acceptable from a user's point of view.
Unfortunately that's not what Debian and a lot of other Linux distributions do. It's, admitted, probably a Linux-specific problem as system packages can be mixed freely, unlike on *BSD where there's a common source for everything. But SLJIT does support Linux too, right? So it makes sense to fix this bug. |
In create_tempfile() we look for a suitable place to put the temporary file into and, among others, look at $TMPDIR. If the value of this environment variable exceeds the bounds of the local tmp_name[] buffer, we ignore it. However, we still change the value of 'tmp_name_len' which leads to follow-up errors. On debug builds this can lead to hitting the assertion as can be seen below: $ TMPDIR=$(perl -e 'print "A"x1024') ./bin/array_access Assertion failed at sljit_src/sljitProtExecAllocator.c:147 Aborted For non-debug builds, however, this can lead to a memory corruption, by abusing the fact that we change a trailing '/' to '\0' later on. With a sufficiently high enough value for 'tmp_name_len' this can corrupt stack frames up in the call chain. Fix this by setting 'tmp_name_len' only if value it is based on is found to be valid -- just like it was prior to commit 98323bd. Fixes: 98323bd ("protexec: refactor create_tempfile() (zherczeg#37)") Signed-off-by: Mathias Krause <[email protected]>
SELinux or PaX/grsecurity based kernels may deny creating writable and executable mappings, leading to errors when trying to allocate JIT memory, even though JIT support is generally available. Provide a function to probe for the runtime availability of rwx maps to support users like libpcre2 which can use it to announce the lack of JIT and fall back to the interpreter instead. This function is only needed for Linux and only if we're using the default JIT memory allocator, as all others implement workarounds via double mappings. Signed-off-by: Mathias Krause <[email protected]>
bd57bff
to
8e46492
Compare
I updated the code accordingly (testing the error code of the failed |
This PR fixes an assertion / memory corruption in the protexec allocator as well as adding a function to detect missing support to create rwx mappings for the exec allocator. The latter is intended to be used by downstream users to fall back to the interpreter mode in this case.
Arguably, the proper solution would be to use the protexec allocator instead, but, unfortunately, not all distros do that. Case in point would be Debian configuring libpcre2 with the exec allocator only, making use cases like
git grep
fail hard on such hardened systems.I also sent a PR to
pcre2
to make use of the runtime detection function to implement the mentioned fallback functionality.