This isn't the first time this has happened (https://www.openwall.com/lists/oss-security/2019/01/09/3). Uninformed opinion, but can we just like not stack allocate things, especially if their size is attacker-controlled? Linux has already moved away from VLAs for security reasons, I don't see why systemd shouldn't either.
I mean, if we avoid stack allocation, then perhaps we need to instead allocate on the heap. What if malloc fails then? Can systemd return an error code? Or do we instead have a DoS problem?
The critical point is unboundedly attacker-controlled sizes. A reasonable limit, specified and then carried into the implementation, would've prevented this bug easily.
Speaking of limits, this is something that I think deserves more attention in education --- does the algorithm or code you create work for all inputs, especially those at the limits? Are you even able to determine those limits? Why does a path in D-Bus need to have a 32-bit length? Is 16 (64K) or even 8 (255) bits sufficient? If not, why? Can it be made sufficient?
Asking the important questions, learning, and understanding is ultimately going to be a far more useful than the current "just cut off the limb" attitude toward security that seems so prevalent today. Those questions should've all been asked and clearly answered at the design stage.
Rather, I'd say the critical points are that systemd has critical issues it absolutely should not be having, with the state of tooling we have today (taint analysis, heck, even a good linter might have been able to find this!). When the last thing came out someone I know told me that they had just glanced through the codebase and were not surprised that it had bugs like this; I think his exact words were something like "you don't have code like this in 2019".
> Why does a path in D-Bus need to have a 32-bit length? Is 16 (64K) or even 8 (255) bits sufficient?
I'm not sure what path is in this context but if I assume file path (the rest of my post will look stupid if it's not) then 16 seems more than enough, but 8 has bitten many people in the windows world before. I'm also unsure of the context, is it always a path or is it a generic message that will be a path in this context? If it's the latter then there are many instances where 32-bit makes sense and then regardless of what is suitable for a path the 32-bit length is the contract.
Weirdly, having this stack-allocated probably saved the day a bit. If it was on the heap, there's a good chance that instead of an unmapped page, you'd hit something actually available.
which results in the stack pointer moving outside of the bounds of the currently mapped stack region, jumping over the stack guard pages.
They don't generate stack probes? I'm used to this behaviour on Windows, where the compiler will generate code to explicitly touch the guard pages if it knows/suspects you may allocate more than a page:
Yes, I know if you try to get it to allocate too much it will still crash, but that would be an "out of stack" error, so I'm a little surprised at that description.
I mean, he is a good programmer, but I’ve seen him (even in other projects) optimizing codes prematurely, which allows vulnerabilities to slip in. I understand the temptation, but many of his projects are mostly not performance critical. Not a good habit.
This isn't an optimization. It is pretending that C is like managed code and using the stack as a glorified garbage collector to avoid the responsibility of freeing memory. He did the same with his alloca() misuse. I would hope that they turn on -Wvla to find any more of these landmines.
I don't do much C, or any at all. What would happen? Would the allocation fail and you'd end up with prefix pointing to NULL? Or would it overwrite something else on the stack?
As far as I know if there is not enough space it should trigger a page fault, which in turn triggers an exception, which should probably result in a segmentation fault.
Traditionally, the allocation would nevertheless be made. Effect would be that the stack and the memory allocator shared memory.
That typically doesn’t go well for long. The moment you write to that local variable (which technically need not happen, but if you don’t write to it, why have the variable at all?), you overwrite data in the heap and/or heap data structures. That will become a problem when the program reads that data or when the memory allocator walks the heap.
Debugging this kind of problem is hard, as the buggy code often will run fine. It ‘just’ triggers problems at some unspecified later time, likely in a completely unrelated part of the program.
So, guard pages ‘below’ the stack were added. They trigger exceptions that kill the offending program when it tries to write to or read from those pages. That doesn’t fully prevent this problem; large stack objects may jump over the protected area (especially in 32-bit code, you don’t want to give up lots of virtual address space to such guard pages, and the amount of address space ‘lost’ can add up fast in multi-threaded programs)
Another way to detect this is to periodically (say in an interrupt handler, or whenever a call to the memory allocator is made) check that the stack pointer has a valid value. That isn’t sure-fire, either. Firstly, it can only detect problems after the fact and secondly, it may not see short violations of the rules.
How do large stack objects jump over the protected area? Even if the OS allocates so much stack space that the stack pointer overlaps with the heap area, if the stack is written sequentially I would think that it's going to write to the protected area before it writes to memory beyond it.
EDIT: maybe I got this. Basically, if my stack pointer jumps the protected page and ends up in the heap, I can address prefix[last_pos] without triggering any page fault, thus corrupting memory. Is this correct? I guess guard pages are a very weak protection mechanism, the OS should check the position of the stack pointer at every memory allocation.
Yes, that’s correct. For example, if the stack grows downwards (which, AFAIK, is the case on most current architectures):
- stack pointer is at S
- protected area starts at B, ends at E (with B<E<S)
- stack is decreased by something larger than (S-B), making it point somewhere before the start of the protected area
Compilers won’t allocate large local variables on the stack, so guard pages work fine for accidental stack overflows, such as when a call stack gets too deep. They also work reasonably well against intentional (by an attacker, not by the programmer who wrote the code) ones, but indeed aren’t perfect. That’s why some compilers have compiler flags that, when allocating a stack variable that they don’t know to be ‘small’ will insert extra code that will hit the protected area, even if the allocation jumps over it.
28 comments
[ 1.3 ms ] story [ 75.6 ms ] thread> the length of this is attacker controlled
This isn't the first time this has happened (https://www.openwall.com/lists/oss-security/2019/01/09/3). Uninformed opinion, but can we just like not stack allocate things, especially if their size is attacker-controlled? Linux has already moved away from VLAs for security reasons, I don't see why systemd shouldn't either.
> if we avoid stack allocation, then perhaps we need to instead allocate on the heap
That's what the fix does: https://github.com/systemd/systemd/commit/f519a19bcd5afe674a...
Speaking of limits, this is something that I think deserves more attention in education --- does the algorithm or code you create work for all inputs, especially those at the limits? Are you even able to determine those limits? Why does a path in D-Bus need to have a 32-bit length? Is 16 (64K) or even 8 (255) bits sufficient? If not, why? Can it be made sufficient?
Asking the important questions, learning, and understanding is ultimately going to be a far more useful than the current "just cut off the limb" attitude toward security that seems so prevalent today. Those questions should've all been asked and clearly answered at the design stage.
I'm not sure what path is in this context but if I assume file path (the rest of my post will look stupid if it's not) then 16 seems more than enough, but 8 has bitten many people in the windows world before. I'm also unsure of the context, is it always a path or is it a generic message that will be a path in this context? If it's the latter then there are many instances where 32-bit makes sense and then regardless of what is suitable for a path the 32-bit length is the contract.
https://security-tracker.debian.org/tracker/CVE-2019-6454
They don't generate stack probes? I'm used to this behaviour on Windows, where the compiler will generate code to explicitly touch the guard pages if it knows/suspects you may allocate more than a page:
https://geidav.wordpress.com/tag/stack-probing/
Yes, I know if you try to get it to allocate too much it will still crash, but that would be an "out of stack" error, so I'm a little surprised at that description.
https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.h...
I mean, he is a good programmer, but I’ve seen him (even in other projects) optimizing codes prematurely, which allows vulnerabilities to slip in. I understand the temptation, but many of his projects are mostly not performance critical. Not a good habit.
That typically doesn’t go well for long. The moment you write to that local variable (which technically need not happen, but if you don’t write to it, why have the variable at all?), you overwrite data in the heap and/or heap data structures. That will become a problem when the program reads that data or when the memory allocator walks the heap.
Debugging this kind of problem is hard, as the buggy code often will run fine. It ‘just’ triggers problems at some unspecified later time, likely in a completely unrelated part of the program.
So, guard pages ‘below’ the stack were added. They trigger exceptions that kill the offending program when it tries to write to or read from those pages. That doesn’t fully prevent this problem; large stack objects may jump over the protected area (especially in 32-bit code, you don’t want to give up lots of virtual address space to such guard pages, and the amount of address space ‘lost’ can add up fast in multi-threaded programs)
Another way to detect this is to periodically (say in an interrupt handler, or whenever a call to the memory allocator is made) check that the stack pointer has a valid value. That isn’t sure-fire, either. Firstly, it can only detect problems after the fact and secondly, it may not see short violations of the rules.
EDIT: maybe I got this. Basically, if my stack pointer jumps the protected page and ends up in the heap, I can address prefix[last_pos] without triggering any page fault, thus corrupting memory. Is this correct? I guess guard pages are a very weak protection mechanism, the OS should check the position of the stack pointer at every memory allocation.
- stack pointer is at S
- protected area starts at B, ends at E (with B<E<S)
- stack is decreased by something larger than (S-B), making it point somewhere before the start of the protected area
Compilers won’t allocate large local variables on the stack, so guard pages work fine for accidental stack overflows, such as when a call stack gets too deep. They also work reasonably well against intentional (by an attacker, not by the programmer who wrote the code) ones, but indeed aren’t perfect. That’s why some compilers have compiler flags that, when allocating a stack variable that they don’t know to be ‘small’ will insert extra code that will hit the protected area, even if the allocation jumps over it.