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

Prot exec fixes #136

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Prot exec fixes #136

wants to merge 2 commits into from

Conversation

minipli-oss
Copy link
Contributor

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.

@zherczeg
Copy link
Owner

zherczeg commented Nov 8, 2022

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.

@minipli-oss
Copy link
Contributor Author

The problem with allocating rwx memory is that it'll trigger a violation leading to logs. Using mmap(rw-) + mprotect(rwx) avoids that.

@minipli-oss
Copy link
Contributor Author

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:

minipli@x1:~/src/pcre2 (master)$ LD_LIBRARY_PATH=.libs/ git grep beer
fatal: Couldn't JIT the PCRE2 pattern 'beer', got '-48'

That's the reason why I tried to provide a fix :)

@zherczeg
Copy link
Owner

zherczeg commented Nov 8, 2022

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.

@zherczeg
Copy link
Owner

zherczeg commented Nov 8, 2022

And I fail to see where pcre2 would do this kind of detection.

https://github.com/PCRE2Project/pcre2/blob/master/src/pcre2_jit_compile.c#L14450

@minipli-oss
Copy link
Contributor Author

Well, currently git grep is just failing hard and that's a bad thing, at least for me.
Getting a kernel log entry each time a user is running git grep isn't nice either.
Enabling another allocator is no option either, as that's the distributor's choice -- hard for me to change as an end user.

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.

@minipli-oss
Copy link
Contributor Author

And I fail to see where pcre2 would do this kind of detection.

https://github.com/PCRE2Project/pcre2/blob/master/src/pcre2_jit_compile.c#L14450

https://github.com/PCRE2Project/pcre2/blob/master/src/pcre2_jit_compile.c#L14464-L14465

Yeah, that's probably the error I see with git grep. Not exactly what I'd call user friendly ;)

@minipli-oss
Copy link
Contributor Author

My change to pcre2 avoids running into this error by making pcre2_config(PCRE2_CONFIG_JIT, &jit) return 0, so users of prec2 won't try to make use of JIT.

@zherczeg
Copy link
Owner

zherczeg commented Nov 8, 2022

That error should be silently ignored. I don't get why it prevents git grep from running.

@minipli-oss
Copy link
Contributor Author

That error should be silently ignored. I don't get why it prevents git grep from running.

That's because git tests the availability of JIT support first and relies on it working: https://github.com/git/git/blob/master/grep.c#L317-L321

@zherczeg
Copy link
Owner

zherczeg commented Nov 8, 2022

To me it seems p->pcre2_jit_on = 0; should happen when jitret is not 0, not die.

@zherczeg
Copy link
Owner

zherczeg commented Nov 8, 2022

The truth is @carenas is our allocator expert, but for this would be the right solution:
a) the normal allocator should work in a way that does not add kernel logs
b) there should be another allocator which dynamically selects a working one
c) git should not die on error, just set "jit is not available"

@minipli-oss
Copy link
Contributor Author

To me it seems p->pcre2_jit_on = 0; should happen when jitret is not 0, not die.

There have been a few attempts in the past to get it fixed in git, but nothing has actually landed upstream: :(
https://public-inbox.org/git/[email protected]/

So, arguably, no changes to pcre2 or sljit are needed either. All users of git grep simply need to prefix their searches with "(*NO_JIT)" on affected systems. But that's hardly usable, so my attempt on fixing this at the core and make it "just work" for all possibly affected downstream projects.

@minipli-oss
Copy link
Contributor Author

The truth is @carenas is our allocator expert, but for this would be the right solution:
a) the normal allocator should work in a way that does not add kernel logs
b) there should be another allocator which dynamically selects a working one
c) git should not die on error, just set "jit is not available"

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?

@minipli-oss
Copy link
Contributor Author

To me it seems p->pcre2_jit_on = 0; should happen when jitret is not 0, not die.

https://public-inbox.org/git/[email protected]/ would be the closest, but wasn't merged for whatever reason :/

@carenas
Copy link
Contributor

carenas commented Nov 8, 2022

To me it seems p->pcre2_jit_on = 0; should happen when jitret is not 0, not die.

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)

@zherczeg
Copy link
Owner

zherczeg commented Nov 9, 2022

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.

@minipli-oss
Copy link
Contributor Author

To me it seems p->pcre2_jit_on = 0; should happen when jitret is not 0, not die.

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, 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)

While I agree that git should just handle the error and fall back to interpreter mode, I don't see why it should run into the error in the first place. If we already know we won't be able to generate JIT code on a given system, why would we want users attempt to do it anyway and put the burden on them to handle the error? Wouldn't it be much more sensible to actually stop users trying to make use of the JIT engine by telling them it won't work?

@minipli-oss
Copy link
Contributor Author

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.

It's an internal function for users that need it -- like pcre2: PCRE2Project/pcre2@17e45b0.
But it can also be part of pcre2 only, not sljit. I just thought other downstream projects might benefit from it as well.

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.

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 pcre2_config(PCRE2_CONFIG_JIT, &jit) be if sljit would fallback to interpreter mode because it's unable to allocate JIT memory? What if only execalloc is supported, like it is right now in Debian? It either would still fail as it is right now (and thereby not fix the problem) or basically do what the current patch at hand does: falling back to interpreter mode.

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 git grep work again.

@carenas
Copy link
Contributor

carenas commented Nov 9, 2022

To me it seems p->pcre2_jit_on = 0; should happen when jitret is not 0, not die.

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, 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)

While I agree that git should just handle the error and fall back to interpreter mode, I don't see why it should run into the error in the first place.

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.

If we already know we won't be able to generate JIT code on a given system, why would we want users attempt to do it anyway and put the burden on them to handle the error? Wouldn't it be much more sensible to actually stop users trying to make use of the JIT engine by telling them it won't work?

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.

@minipli-oss
Copy link
Contributor Author

While I agree that git should just handle the error and fall back to interpreter mode, I don't see why it should run into the error in the first place.

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.

The error was visible, because pcre2 told git that the JIT would be available while it's non-functional on the affected systems.

If we already know we won't be able to generate JIT code on a given system, why would we want users attempt to do it anyway and put the burden on them to handle the error? Wouldn't it be much more sensible to actually stop users trying to make use of the JIT engine by telling them it won't work?

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.

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.

I would argue though that avoiding "logs" is misguided (and that also includes the way the test is done, so it is accurate).

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.

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.

It's an accurate test for PaX's MPROTECT feature. It tracks the possible transitions of setting PROT_WRITE / PROT_EXEC for a given mapping through the kernel internal VM_MAY* flags and ensures they're excluding each other with the type of a mapping ((possibly) writable or executable) set with the initial mmap(). The type of a mapping can be only one of these and cannot change during its lifetime, that's why calls to mmap(…,PROT_WRITE | PROT_EXEC,…) are outright denied.

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.

Not really. I just did a test on a CentOS 8 VM and if I enable deny_execmem it fails in the same way:

[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 git's use of pcre2's JIT only works with the default setting, where deny_execmem is disabled, i.e. rwx mappings are allowed. If it's enabled, git grep is as broken as it is under PaX/grsecurity.

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 deny_execmem.

@minipli-oss
Copy link
Contributor Author

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 /proc/self/status check made sljit_get_runtime_support() return early and skip the fallback test.

I'll change it to a mmap(rwx) test which should address @carenas concern about testing the "wrong thing".

@zherczeg
Copy link
Owner

zherczeg commented Nov 9, 2022

#137

@minipli-oss
Copy link
Contributor Author

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).

@zherczeg
Copy link
Owner

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?

@minipli-oss
Copy link
Contributor Author

The main problem is adding new api is a burden to everybody, and that does not goes away.

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 pcre2_config(PCRE2_CONFIG_JIT, ...). It's documentation states:

  PCRE2_CONFIG_JIT             Availability of just-in-time compiler support (1=yes 0=no)

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.

I have a question btw: jit is disabled by default, so if they enabled it, why they enabled it with a bad allocator?

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 deny_execmem toggle runs into the same issue, so Fedora and RHEL derivatives are affected as well). But one reason that even distros build with SELinux in mind still don't enable the protexec allocator might be, that it's disabled by default and marked as "experimental". You might want to ask the PCRE2 maintainers why that's the case .... if it wouldn't have been you that added the "experimental" part of the description in commit PCRE2Project/pcre2@70b0debf1091c. Thankfully you also gave a good reason yourself, why it's not enabled by default:

                             [...] Warning: this allocator is experimental!
  It does not support fork() operation and may crash when no disk space is
  available. This option has no effect if JIT is disabled.

Btw, the "disk space" part is probably a benign problem, but the fact that alloc_chunk() will consume and bind a file descriptor is much more of a concern, at least for me. That might disturb a threaded application that relies on the predictability/continuity of file descriptor numbers which can be invalidated with sljit allocating files too.

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.


/*
* Create a temporary mapping and try to make it writable to probe for rwx
* support without generating a log message.
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@zherczeg
Copy link
Owner

The prot allocator is a system exploit (a security hole). If you disable executable file mapping, you literally cannot run any application, because the ld program loader will simply not work. The prot allocator exploits this fact, and maps a tmp file as executable. I don't know how tmp files work, they might never been written to disk. Regardless, forks does not duplicate them with copy on write.

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.

@minipli-oss
Copy link
Contributor Author

The prot allocator is a system exploit (a security hole). If you disable executable file mapping, you literally cannot run any application, because the ld program loader will simply not work. The prot allocator exploits this fact, and maps a tmp file as executable. I don't know how tmp files work, they might never been written to disk. Regardless, forks does not duplicate them with copy on write.

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.

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.

It's not, as I already mentioned above. It's a runtime property if JIT can be used or not. Let me quote myself:

"""
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.
"""

@zherczeg
Copy link
Owner

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.

@minipli-oss
Copy link
Contributor Author

This sounds bad. If you can switch its status during runtime any number of times, then testing does not seem useful.

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.

@minipli-oss
Copy link
Contributor Author

Trying to move forward with this, as the current behavior isn't the preferred state (I want git grep ... to be functional even with PaX MPROTECT or SELinux systems with deny_execmem enabled).

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?

@zherczeg
Copy link
Owner

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.

@minipli-oss
Copy link
Contributor Author

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:

  1. git should handle errors of pcre2_jit_compile() and simply fall back to interpreter mode.
  2. pcre2 shouldn't announce that it supports JIT when it won't be able to allocate executable memory for the JIT code.

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 pcre2_config(PCRE2_CONFIG_JIT, ...). Fixing that would automatically fix the bug in git as well. That's what PCRE2Project/pcre2#157 is all about. It's just the question if the supporting parts should be included in sljit or not.

@minipli-oss
Copy link
Contributor Author

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 sljit_get_runtime_support(). Any preference?

@zherczeg
Copy link
Owner

The pcre2_config is a static check, it works as intended.

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:
https://github.com/zherczeg/sljit/blob/master/sljit_src/sljitConfigInternal.h#L616

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 NULL if it cannot allocate memory, but it may allocate memory later, even if it returned with NULL before. Also you might not be able to access that file. The application might have serious i/o restrictions. Just think about chromium javascript jit processes. They do allocate executable memory, but cannot access anything (especially files) to reduce the chance of security breaks.

I understand you have a specific problem, but the picture is quite big unfortunately.

@carenas
Copy link
Contributor

carenas commented Nov 30, 2022

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)).

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).

@carenas
Copy link
Contributor

carenas commented Nov 30, 2022

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.

IMHO there are already API extensions that are allocator specific (ex: sljit_free_unused_memory_exec()) and that are therefore not expected to be implemented by any external allocator, the same should be applied here.

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)

@minipli-oss
Copy link
Contributor Author

[Sorry for the late reply but I was sick]

The pcre2_config is a static check, it works as intended.

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 pcre2_config() signaled support for it) when we know it won't work, no matter what? It's like letting someone pull the trigger of a gun, him assuming it's not loaded, when we know it is, just to have him fix up any damage that have caused. While instead we could have told him the truth before and therefore could have prevented the damage from happening.

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: https://github.com/zherczeg/sljit/blob/master/sljit_src/sljitConfigInternal.h#L616

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.

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?

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.

So, this is all Linux only, right? Linux does memory over commitment by default, making a call to mmap() not fail just because there's only little memory left. That'll only happen in the extreme case when there's no memory left to even allocate the required kernel objects for tracking the new memory range (vma, page tables,...). But the system has much more serious problems in this pathological case, so we can ignore it and assume the mmap() call succeeds.

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 mmap()ed memory on first access. In this case the OOM killer will free up memory by killing processes.

Another pathological case for mmap() failing is address space exhaustion. But, again, that's such an extreme case, a JIT library shouldn't bother about.

The solution is freeing some memory, and call the function again.

Not free()ing memory but unmap()ing pages instead. But, really, there's little an application can do when it hits the out-of-memory case but calling _exit() -- or simply let the OOM killer do its job and hope for the best.

But in that case the function gives a random result, not a static value, as it should. The current code returns with NULL if it cannot allocate memory, but it may allocate memory later, even if it returned with NULL before.

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 errno for EPERM to ensure sljit_get_runtime_support() gives a correct result even for the pathological cases.

Also you might not be able to access that file. The application might have serious i/o restrictions.

That's why the /proc/PID/status test is followed by a runtime test that'll provide an answer to the question if WX memory can be allocated or not. It'll generate a log warning on grsecurity / PaX systems, though. That's why the proc based lookup gets done first. But yes, I thought of a fall-back for such restricted processes and that's what the mmap() test is about.

Just think about chromium javascript jit processes. They do allocate executable memory, but cannot access anything (especially files) to reduce the chance of security breaks.

I wasn't aware that Chomium makes use of SLJIT, that's news to me. No hits in a code search, though.

I understand you have a specific problem, but the picture is quite big unfortunately.

Right, it's not only PaX's MPROTECT but SELinux's deny_execmem as well. But I already mentioned that a few times before. So yes, the impact of this bug is quite big. Time to get it fixed!

@minipli-oss
Copy link
Contributor Author

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)).

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.

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.

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).

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]>
@minipli-oss
Copy link
Contributor Author

I updated the code accordingly (testing the error code of the failed mmap() call), added "e" to the flags of the fopen() call to have the procfs file opened with O_CLOEXEC and rebased the branch.

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.

3 participants