This implementation has a trivial buffer overflow, ROFLMAO
Geniunely curious.
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.
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
[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.
[1]: https://marc.info/?l=openbsd-misc&m=151233210523661&w=2
Look, I think a lot of programmers and managers fail to understand the number of factors one needs to consider and how it scales with the complexity of your codebase and what it interacts with. If you want to rewrite your video processing service which you wrote together with five or so contributors and is say 20,000 lines of C++ into Rust, that is one thing. Take a step back and consider the number of users and what you interact with, it all seems rather manageable and you can probably be backwards compatible with respect to your users. In terms of time, maybe a few months? Maybe even six? For a single programmer that now needs to learn proper Rust.
Now, instead consider what an operating system is and the surface with which it interacts. The absolutely metric ton of hardware, the heap of standards, the massive load of hacks that are documented and undocumented, the large amount of users, the number of contributors and their experience, the platforms that you support, all the software that is written for your operating system to make it useful, etc. OpenBSD is famous (infamous?) for being willing to break things to do "the right thing". But they are also famous (infamous, again?) for being very conservative, which I think is understandable given their security focus and (relatively) low amount of manpower. Rust has certainly been considered, but it is far from the only consideration. This is akin to walking into a multi-million dollar company that has a fully functioning service or piece of software that they have been selling for decades and suggest to management that maybe we should start moving it all from C# to Rust next week, while being blissfully unaware of everything else the company is beholden to. Spoiler, it will not work out that way.
Furthermore, it is somewhat tiring that Rust keeps being touted as the final revelation when it comes to writing safer code. Guess what, there were plenty of projects before Rust that introduced safety in various forms (and at various costs) and there will be plenty of projects after Rust that will do the same. Yes, it is an amazing piece of technology, but it is equally plausible that its influence may not end up in it eating the world; rather give birth to something else or bring some of its thinking into other languages. Only time will tell and worse may end up being better yet again.
So to me, what any operating system (security focused or not) should do is to consider their (limited) options given their own goals and situation. Which is something I have seen plenty of evidence that a project of the age of OpenBSD is doing and successfully so.
Personally, I am keeping an eye on Redox and look forward to see what lessons will be learnt from their take of what an operating system can be. But for my servers and desktop I have and continue (for now?) to run a healthy mix of Linux and BSD while getting work done.
I don't see a lot of nuance. The primary argument in that thread boils down to any programmer who isn't using C isn't a serious programmer. The dominant technical analyses are a) adding more compiler toolchains make builds take a lot longer and b) memory safety doesn't protect against everything, so why bother? That's not nuance.
Nuance would be pointing out that Rust provides benefits over just memory safety: that Rust tends to push you towards a parse-don't-validate memory, or maybe Rust having a greater emphasis on error checking correctly [1]. But then again, Rust isn't necessary to do that, so you could also analyze how existing software development practices are sufficient to bring those into play.
You can also discuss Rust's failures. Rust, after all, didn't get uninitialized memory right. There's also some uneasiness about the details of the borrow checker to point to, or you can throw some remarks about the &mut-is-noalias and the difficulty it takes to actually ensure that you never create two &mut to the same location in unsafe code.
Nuance might also discuss how safety features do and don't percolate in mixed-programming-language environments, or how a rewrite may or may not improve security. There's definitely items on both sides of the cost-benefit ledger there!
But that's not what we got. The thread starts with an (incorrect [2]) gatekeeping moment of "it's not serious if you don't rewrite coreutils in it," and mostly devolves into a general theme of "anyone who needs their programming language to provide safety wheels is a terrible programmer who shouldn't be allowed anywhere near systems programming". The irony of that viewpoint coming from an OS well known for its love of just-in-case security precautions is not lost on me.
[1] I was recently writing something parsing diffs, and as part of that, I was making sure that the arithmetic on line numbers didn't overflow. Integer overflow (whether signed or unsigned) is frequently ignored as an error source in most programming languages!
[2] At the time the post was written, people were in fact working on doing a rewrite of this stuff in Rust.
As for the linked thread itself, I just ignored the "Real Programmers Don't Use Pascal" parts (it gets old really fast) and what I arrived it is largely reflected in what I already wrote in the parent and ancestors.
I think that's a success story because while Rust 1.0 shipped with the hopelessly broken std::mem::unitialized, Rust 1.36 shipped with std::mem::MaybeUninit that fixed the situation. (it's true that they can't definitely remove the broken API, but it can be aggressively linted against nonetheless)
That is, it's a flaw present in the first version of the language but a success of the development process and language evolution, without breaking backwards compatibility
No, not at all. OpenBSD may consider Rust all the time and it may still be not the right language for them.
But Rust might be the right language for some greenfield projects (which means, no rewrite), in ways that doesn't apply to a decades-old OS
If they had to stabilize some parts, probably the answer would be different.
I'm all for dropping C in favor of Rust for new projects, but killing C developers is going a bit too far, don't you think?
https://play.rust-lang.org/?version=stable&mode=debug&editio...
Clang and gcc have flags that can make integer overflow trap.
Though, these would also be triggered by statements like:
pins[SYS_kbind] = -1;
And the signature totally explains their childish reply, over there and here.
People make mistakes in every project. So grow up
(E.g. Valgrind, Flexelint, cppcheck, clang static analyzer, etc.)
If yes, then why aren't code analyzers used on all OpenBSD code submissions, given their stance on having correct code & security focused.
So this comparison will be unsigned and will allow to set `npins` to any value, even negative:
npins = MAX(npins, syscalls[i].sysno)
npins = MAX(npins, SYS_kbind)
pins[syscalls[i].sysno] = syscalls[i].offset
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.
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;
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).
npins = MAX(npins, SYS_kbind)
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.
int
elf_read_pintable(struct proc *p, Elf_Phdr *pp, struct vnode *vp,
Elf_Ehdr *eh, uint **pinp)
{
struct pinsyscalls {
u_int offset;
u_int sysno;
} *syscalls = NULL;
int i, npins = 0, nsyscalls;
uint *pins = NULL;
[1] nsyscalls = pp->p_filesz / sizeof(*syscalls);
if (pp->p_filesz != nsyscalls * sizeof(*syscalls))
goto bad;
[2] syscalls = malloc(pp->p_filesz, M_PINSYSCALL, M_WAITOK);
[3] if (elf_read_from(p, vp, pp->p_offset, syscalls,
pp->p_filesz) != 0) {
goto bad;
}
[4] for (i = 0; i < nsyscalls; i++)
[5] npins = MAX(npins, syscalls[i].sysno);
[6] npins = MAX(npins, SYS_kbind); /* XXX see ld.so/loader.c */
[7] npins++;
[8] pins = mallocarray(npins, sizeof(int), M_PINSYSCALL, M_WAITOK|M_ZERO);
for (i = 0; i < nsyscalls; i++) {
[9] if (pins[syscalls[i].sysno])
[10] pins[syscalls[i].sysno] = -1; /* duplicated */
else
[11] pins[syscalls[i].sysno] = syscalls[i].offset;
}
pins[SYS_kbind] = -1; /* XXX see ld.so/loader.c */
*pinp = pins;
pins = NULL;
bad:
free(syscalls, M_PINSYSCALL, nsyscalls * sizeof(*syscalls));
free(pins, M_PINSYSCALL, npins * sizeof(uint));
return npins;
}
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.
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 */
};
if (pins[syscalls[i].sysno])
pins = mallocarray(npins, sizeof(int), M_PINSYSCALL, M_WAITOK|M_ZERO);
for (i = 0; i < nsyscalls; i++) {
pins[syscalls[i].sysno] = syscalls[i].offset;
}
Hang on - npins is already checked in the loop before, and incremented with ++
syscalls[i].sysno can't be larger than what is allocated with:
pins = mallocarray(npins, sizeof(int), M_PINSYSCALL, M_WAITOK|M_ZERO);
So I still can't find the problem
struct pinsyscall entries[] = {
{ .sysno = 1, .offset = 0x1234 },
{ .sysno = 2, .offset = 0x5678 },
{ .sysno = 1, .offset = 0x9abc }
};
When we loop through, we'll set:
1. `pin[syscalls[0].sysno] = 0x1234` => `pin[1] = 0x1234`
2. `pin[syscalls[1].sysno] = 0x5678` => `pin[2] = 0x5678`
#define MAX(a, b) ((a) > (b) ? (a) : (b))
Referring back to the implementation, the first use of `MAX` (inside the `for` loop) is this:
...
for (i = 0; i < nsyscalls; i++)
npins = MAX(npins, syscalls[i].sysno);
...
...
npins = MAX((u_int)npins, syscalls[i].sysno);
...
Once we're out of the loop, `MAX` is used again here:
...
npins = MAX(npins, SYS_kbind);
...
...
#define SYS_kbind 86
...
With `npins = 86`, an array of 86+1 will be allocated, but the `syscalls[i].sysno` in the next loop could of course easily be greater than 86 -- thus leading to out-of-bounds array access.
So then it depends on if whatever code loaded the elf section does any validation of the data it reads. I can't help but thinking the whole code could use some structs and/or "getter setters" to talk dirty objective oriented speak. It needn't be that though, it could be as low level as some macros which helps doing the right thing with signedness and such. But my main impression is that a lot of the data structure semantics is kept in the heads of programmers instead of being formalised in the code.
In C there is always the opportunity to run with scissors in the middle of the road, but you can do a lot to protect yourself too, without loosing much, if any, performance.