-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Make some parts of _zend_mm_heap read-only at runtime #14570
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
Conversation
I've talked to @bwoebi about this before and some profilers like DataDog depend on this. |
Unfortunately this will break some useful functionality:
Instead of disabling the functionality we could mangle the function pointers with a secret. I believe that the glibc does this to protect Alternatively, we could store the custom callbacks in a separate readonly memory region. In zend_mm_init(), allocate two pages for the zend_mm_heap + the function pointers. mprotect() the second page. Unprotect when necessary, e.g. in zend_mm_set_custom_handlers(). As an additional benefit this page would double as a guard page. We might want to protect |
I thought about having some parts of But I agree, having function pointers as well as other read-only fields like |
I don't think disabling ZEND_MM_CUSTOM is a good thing for all the reasons Arnaud outlined. Overriding the _emalloc/_efree needs platform dependent work and is not easy. The proposal to make the parts of the head readonly sounds more reasonable. After all, these need to only be initializaed once per GINIT, so that sounds pretty okay I think. |
I think if we only store We can leave the |
The Datadog profiler relies on the |
I changed the code to go the read-only way :) |
d91c592
to
120967f
Compare
120967f
to
96cbe36
Compare
We have various other function pointers not living in .rodata. Examples are |
@iluuu1994 These pointers in zend_mm_heap are a particularly interesting for attackers because they can learn their address trivially. We can protect them very effectively and without overhead, so it seems beneficial to do so. An other dangerous one, similar to object handlers, is the array dtor function, as attackers have plenty of ways to place arrays on the heap. It would be great if we could protect those too. I'm not sure how to, either. Mangling/xor would increase difficulty a bit, but I expect a performance regression especially for objects. |
96cbe36
to
d25588c
Compare
I suppose, this can't be compatible with |
Why not? It'll simply use a bit more memory. |
d25588c
to
25a3663
Compare
c1dd1ee
to
44bc6fd
Compare
3aca506
to
9044d3e
Compare
9044d3e
to
7bd296d
Compare
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.
This LGTM apart from a few minor comments
As [presented at OffensiveCon 2024](https://youtu.be/dqKFHjcK9hM?t=1622), having trivially callable writeable function pointers at the top of the heap makes it straightforward to turn a limited write into an arbitrary code execution. Disabling ZEND_MM_HEAP by default isn't doable, as it's used by a couple of profilers, so we're making some parts of `_zend_mm_heap` read-only at runtime instead: this will prevent the custom heap functions pointers from being hijacked, as well as the custom storage ones. We don't put the shadow_key there, since it has a performance impact, and an attacker able to precisely overwrite it is likely already able to read it anyway.
7bd296d
to
7d35e89
Compare
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.
LGTM!
@dstogov do you want to take an other look?
zend_alloc.c was especially optimized for performance. It's especially stored This patch changes some key design ideas. At least it should be benchmarked with callgrind (with cache simulation) and on real hardware. From a quick review, I don't see anything terrible. (I'm going to be on vacation next two weeks). |
Any recommendation on how to properly benchmark php? |
For valgrind, check how https://github.com/php/php-src/blob/master/benchmark/benchmark.php does it. The symfony demo and wordpress benchmarks are probably the most relevant in this case. |
quick'n'dirty testing doesn't show anything incredible performance-wise |
As the person who has created the Valgrind benchmark: It's shown not to be super reliable in the past. Some changes were more or much less profitable/expensive than Valgrind suggested. I would suggest running a benchmark with |
Can this be documented somewhere? Ideally in a README.md file in the |
More (ghetto) benchmarks, impact in the noise levels:
|
@jvoisin Thanks! For the warmup to be effective, you should look at the time printed by php-cgi itself (it goes to stderr). But it doesn't look like there will be a big change either way, which is good! |
With a bigger warmup time:
|
@jvoisin I think you misunderstood. You should not use |
the last benchmark shows 0.6% real time slowdown, including initialization and warm-up. Without them the slowdown should be probably even more significant. |
@dstogov These slowdowns come to a point where I wonder whether we should make these And possibly not enable by default? :-/ |
I would agree. We already introduced |
IMO, opt-in security is useless. |
@iluuu1994 I do agree, but all we're doing here is some hardening after an exploit was already found, nothing insurmountable for attackers. It's not actually fixing gaping holes in PHPs security or fully preventing exploit classes. |
@dstogov can you please share how you did your benchmarks so I can them them here as well and try to reduce the performance impact? |
I didn't bench this. I used your numbers. 3m44.438s / 3m43.036s = 224.438 / 223.036 = 1.00628 ~= 0.6% slowdown It's better to use execution time provided by php-cgi. The same command, just see "Execution Time: ..." at stderr tail. It excludes startup/shutdown and warm up time. |
PHP's heap implementation is the one that virtually everybody uses: it's fast, it's there by default, it works, …The only major ever I've found of custom heap implementation is phpdbg but it looks dispensable at best. Some other debuggers and profilers might use it, and that's alright, but I don't think that this feature should be enabled by default.
Disabling ZEND_MM_CUSTOM will allow to save a couple of bytes (yay), but the main goal is to close a low-hanging exploitation vector: as presented at OffensiveCon 2024, having trivially callable writeable function pointers at the top of the heap makes it straightforward to turn a limited write into an arbitrary code execution.As presented at OffensiveCon 2024, having trivially
callable writeable function pointers at the top of the heap makes it straightforward to turn a limited write into an arbitrary code execution.
Disabling ZEND_MM_HEAP by default isn't doable, as it's used by a couple of profilers, so we're making some parts of
_zend_mm_heap
read-only at runtime instead: this will prevent the custom heap functions pointers from being hijacked.cc @arnaud-lb