One of the more under-appreciated features of Rust is that it traps on integer overflow / underflow by default.
It’s too bad OpenBSD doesn’t have a good Rust story for the core system. (I understand their reasoning, but it’s still too bad.)
I wonder how hard it would be to backport the integer behavior to C. (C++ would be easy: Use templates.) Perhaps they could add a compiler directive that causes vanilla integer overflow/wraparound to trap, then add annotations or a special library call that supports modulo arithmetic as expected.
Look, I get as much as the next guy that Rust brings a lot of niceties. But what we are talking about here is a project that clocks in at just over 19,000,000 lines of C (`wc -l $(find src -name '*.c' -or -name '*.h')`), code heritage going back more than 40 years, an explicit commitment to a very rich set of platforms [1], and a very limited amount of manpower compared to projects such as Linux with their insane level of corporate backing. "Just rewrite it in and/or integrate Rust" is neither easy nor safe in that it overturns everything that is already there and tested.
As for improving upon C. I can not speak for OpenBSD as a project, but I am sure that there would be ample excitement to produce a minimal, solid C compiler with experimental security features to then serve as the default compiler for the project (heck, OpenBSD already ships with a number of less common security-related compiler flags from what I recall). Sadly, I doubt there is either funding or the hands to make that a reality.
There are compiler flags that OpenBSD could be using but aren't, which would have caught this bug without needing to convert the codebase to Rust. Using -Wconversion would have warned on the mismatched signedness of the MAX macro argument (the unsigned integer, sysno) and its result (being assigned to npins, a signed integer). Alternatively, adding -fsanitize=implicit-integer-sign-change, or a UBSan flag that includes this, would detect this at runtime for the actual range of values that end up causing a change of sign.
Though, these would also be triggered by statements like:
pins[SYS_kbind] = -1;
Due to the pins array being of unsigned int, so all this sort of code would need to be fixed too.
In this case more important than any runtime overflow/underflow checks is the fact that the compiler will check that comparison operands are of the same type instead of inserting an implicit cast. Instead the programmer is forced to insert an explicit conversion like .try_into().unwrap(), which clearly suggests the possibility of an error. And if the error isn't handled, it will panic.
I don't know how they define `MAX`, but I'm guessing it's a typical "a>b?a:b". In function `elf_read_pintable` the `npins` is defined as signed int and `sysno` as unsigned int.
So this comparison will be unsigned and will allow to set `npins` to any value, even negative:
npins = MAX(npins, syscalls[i].sysno)
Then `SYS_kbind` seems to be a signed int. So this comparison will be signed and "fix" the negative `npins` to `SYS_kbind`:
npins = MAX(npins, SYS_kbind)
And finally the `sysno` index might be out of bounds here:
pins[syscalls[i].sysno] = syscalls[i].offset
But maybe I'm completely wrong, I'm not interested in researching it too much.
I believe your whole analysis is correct, that running an elf file with an openbsd.syscalls entry with .sysno > INT_MAX will allow an out-of-bounds write.
Pure decimal integer literals (like 86) are typed as "int" in C, rather than being typeless and triggering type inference. This is a pain when you accidentally write something like this:
uint64_t n = 1 << 32;
On modern desktop platforms, an int is 32 bits, so 1 << 32 is 0, not 2^32, even though a 64-bit integer is wide enough to support that.
Regardless, it's not relevant here, because if an integer and an unsigned integer of the same size are compared the integer is implicitly cast to unsigned integer, and 86 is fine for both signed and unsigned integers (so "MAX(npins, SYS_kbind)" is safe).
> Then `SYS_kbind` seems to be a signed int. So this comparison will be signed and "fix" the negative `npins` to `SYS_kbind`:
npins = MAX(npins, SYS_kbind)
No, the comparison is unsigned here. They're integers of the same "conversion rank", so the unsigned type wins and the signed integer is interpreted as unsigned.
This isn't correct, as npins is a signed int and SYS_kbind is just a macro for the integer 86 (as defined in sys/syscall.h). So it will be a signed comparison between the value of npins and 86.
So first of all we calculate the number of syscalls in the pin section [1], allocate some memory for it [2] and read it in [3].
At [4], we want to figure out how big to make our pin array, so we loop over all of the syscall entries and record the largest we've seen so far [5]. (Note: the use of `MAX` here is fine since `sysno` is unsigned -- see near the top of the function).
With the maximum `sysno` found, we then crucially go on to clamp the value to `SYS_kbind` [6] and +1 at [7].
This clamped maximum value is used for the array allocation at [8].
We now loop through the syscall list again, but now take the unclamped `sysno` as the index into the array to read at [9] and write at [10] and [11]. This is essentially the vulnerability right here.
Through heap grooming, there's a good chance you could arrange for a useful structure to be placed within range of the write at [11] -- and `offset` is essentially an arbitrary value you can write. So it looks like it would be relatively easy to exploit.
Re-reading this, my analysis is slightly incorrect: the `MAX` at [5] with an unsigned arg means we can make `npins` an arbitrary `int` using the loop at [4].
Choosing to make `npins` negative using that loop means we'll end up allocating an array of 87 (`SYS_kbind + 1`) `int`s at [8] and continue with the OOB accesses described.
You'd set up your `pinsyscall` entries like this:
struct pinsyscall entries[] = {
{ .sysno = 0x1111, .offset = 0xdeadbeef }, /* first oob write */
{ .sysno = 0x2222, .offset = 0xf000f000 }, /* second oob write */
{ .sysno = 0xffffffff } /* sets npins to 0xffffffff so we under-allocate */
};
`npins` would be `0xffffffff` after the loop and then the `MAX` at [6] would then return `86`, since `MAX(-1, 86) == 86`.
This just went out to tech@:
https://marc.info/?l=openbsd-tech&m=170234892604404&w=2
One of the more under-appreciated features of Rust is that it traps on integer overflow / underflow by default.
It’s too bad OpenBSD doesn’t have a good Rust story for the core system. (I understand their reasoning, but it’s still too bad.)
I wonder how hard it would be to backport the integer behavior to C. (C++ would be easy: Use templates.) Perhaps they could add a compiler directive that causes vanilla integer overflow/wraparound to trap, then add annotations or a special library call that supports modulo arithmetic as expected.
Look, I get as much as the next guy that Rust brings a lot of niceties. But what we are talking about here is a project that clocks in at just over 19,000,000 lines of C (`wc -l $(find src -name '*.c' -or -name '*.h')`), code heritage going back more than 40 years, an explicit commitment to a very rich set of platforms [1], and a very limited amount of manpower compared to projects such as Linux with their insane level of corporate backing. "Just rewrite it in and/or integrate Rust" is neither easy nor safe in that it overturns everything that is already there and tested.
[1]: https://www.openbsd.org/plat.html
As for improving upon C. I can not speak for OpenBSD as a project, but I am sure that there would be ample excitement to produce a minimal, solid C compiler with experimental security features to then serve as the default compiler for the project (heck, OpenBSD already ships with a number of less common security-related compiler flags from what I recall). Sadly, I doubt there is either funding or the hands to make that a reality.
11 replies →
> One of the more under-appreciated features of Rust is that it traps on integer overflow / underflow by default.
I think this is only true on debug builds, with --release (which most Rust binaries an end-user uses should be compiled with) it just wraps [1] [2]
[1] https://github.com/rust-lang/rfcs/blob/26197104b7bb9a5a35db2... [2] https://stackoverflow.com/a/60238510
There are compiler flags that OpenBSD could be using but aren't, which would have caught this bug without needing to convert the codebase to Rust. Using -Wconversion would have warned on the mismatched signedness of the MAX macro argument (the unsigned integer, sysno) and its result (being assigned to npins, a signed integer). Alternatively, adding -fsanitize=implicit-integer-sign-change, or a UBSan flag that includes this, would detect this at runtime for the actual range of values that end up causing a change of sign.
Though, these would also be triggered by statements like:
Due to the pins array being of unsigned int, so all this sort of code would need to be fixed too.
In this case more important than any runtime overflow/underflow checks is the fact that the compiler will check that comparison operands are of the same type instead of inserting an implicit cast. Instead the programmer is forced to insert an explicit conversion like .try_into().unwrap(), which clearly suggests the possibility of an error. And if the error isn't handled, it will panic.
https://play.rust-lang.org/?version=stable&mode=debug&editio...
1 reply →
>I wonder how hard it would be to backport the integer behavior to C.
Clang and gcc have flags that can make integer overflow trap.
> Stonks only go up
And the signature totally explains their childish reply, over there and here.
Yeah even a child like me is better at secure programming than Theo De Raadt
4 replies →
I don't know how they define `MAX`, but I'm guessing it's a typical "a>b?a:b". In function `elf_read_pintable` the `npins` is defined as signed int and `sysno` as unsigned int.
So this comparison will be unsigned and will allow to set `npins` to any value, even negative:
Then `SYS_kbind` seems to be a signed int. So this comparison will be signed and "fix" the negative `npins` to `SYS_kbind`:
And finally the `sysno` index might be out of bounds here:
But maybe I'm completely wrong, I'm not interested in researching it too much.
> I don't know how they define `MAX`, but I'm guessing it's a typical "a>b?a:b"
Indeed: https://github.com/openbsd/src/blob/master/sys/sys/param.h#L...
> Then `SYS_kbind` seems to be a signed int.
It's an untyped #define: https://github.com/openbsd/src/blob/master/sys/sys/syscall.h...
I believe your whole analysis is correct, that running an elf file with an openbsd.syscalls entry with .sysno > INT_MAX will allow an out-of-bounds write.
> It's an untyped #define
Pure decimal integer literals (like 86) are typed as "int" in C, rather than being typeless and triggering type inference. This is a pain when you accidentally write something like this:
On modern desktop platforms, an int is 32 bits, so 1 << 32 is 0, not 2^32, even though a 64-bit integer is wide enough to support that.
Regardless, it's not relevant here, because if an integer and an unsigned integer of the same size are compared the integer is implicitly cast to unsigned integer, and 86 is fine for both signed and unsigned integers (so "MAX(npins, SYS_kbind)" is safe).
> Then `SYS_kbind` seems to be a signed int. So this comparison will be signed and "fix" the negative `npins` to `SYS_kbind`:
No, the comparison is unsigned here. They're integers of the same "conversion rank", so the unsigned type wins and the signed integer is interpreted as unsigned.
https://en.cppreference.com/w/c/language/conversion
I can never remember the integer conversion rules and end up looking up this link whenever necessary.
This isn't correct, as npins is a signed int and SYS_kbind is just a macro for the integer 86 (as defined in sys/syscall.h). So it will be a signed comparison between the value of npins and 86.
1 reply →
No.
That seems about right.
Out-of-bounds heap write happens in this function:
So first of all we calculate the number of syscalls in the pin section [1], allocate some memory for it [2] and read it in [3].
At [4], we want to figure out how big to make our pin array, so we loop over all of the syscall entries and record the largest we've seen so far [5]. (Note: the use of `MAX` here is fine since `sysno` is unsigned -- see near the top of the function).
With the maximum `sysno` found, we then crucially go on to clamp the value to `SYS_kbind` [6] and +1 at [7].
This clamped maximum value is used for the array allocation at [8].
We now loop through the syscall list again, but now take the unclamped `sysno` as the index into the array to read at [9] and write at [10] and [11]. This is essentially the vulnerability right here.
Through heap grooming, there's a good chance you could arrange for a useful structure to be placed within range of the write at [11] -- and `offset` is essentially an arbitrary value you can write. So it looks like it would be relatively easy to exploit.
Re-reading this, my analysis is slightly incorrect: the `MAX` at [5] with an unsigned arg means we can make `npins` an arbitrary `int` using the loop at [4].
Choosing to make `npins` negative using that loop means we'll end up allocating an array of 87 (`SYS_kbind + 1`) `int`s at [8] and continue with the OOB accesses described.
You'd set up your `pinsyscall` entries like this:
`npins` would be `0xffffffff` after the loop and then the `MAX` at [6] would then return `86`, since `MAX(-1, 86) == 86`.
I must misunderstand something very basic about this code.
but pins is newly allocated and should just zero or "empty". Why dereference it right after allocation?
6 replies →