13 ms·
This implementation has a trivial buffer overflow, ROFLMAO
by MyMonkeyBalls 3y ago
This implementation has a trivial buffer overflow, ROFLMAO
- tiffanyh 3y agoWould you mind sharing how and where in the code, specifically. Geniunely curious.
- ninjin 3y agoThis just went out to tech@: https://marc.info/?l=openbsd-tech&m=170234892604404&w=2 https://marc.info/?l=openbsd-tech&m=170234892604404&w=2
- hedora 3y agoOne 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.
- abound 3y ago> 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/26197104b7bb9a5a35db243d639aee6e46d35d75/text/0560-integer-overflow.md https://github.com/rust-lang/rfcs/blob/26197104b7bb9a5a35db2... [2] https://stackoverflow.com/a/60238510 https://stackoverflow.com/a/60238510
- ninjin 3y agoLook, 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 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.
- nextaccountic 3y agoThis is all true, but this just means that any security focused OS for the next 40 years should consider Rust
- bayindirh 3y agoRust brings niceties, but this implies no requirement for considering Rust.
- ninjin 3y agoImplying that OpenBSD has not considered Rust? There is plenty of discussion on misc@ already (for example [1]) and while it certainly can be "ranty", I am sure if one reads it in good faith you get a fairly nuanced picture. [1]: https://marc.info/?l=openbsd-misc&m=151233210523661&w=2 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.
- yakubin 3y agoIn 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&edition=2021&gist=73ab2fde7f46f5d7e94b5156aa8951dd https://play.rust-lang.org/?version=stable&mode=debug&editio...
- yakubin 3y agoA similar warning could be enabled in GCC using the flag -Wsign-compare.
- 1over137 3y ago>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.
- piss_n_chips 3y agoThere 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.
- petee 3y ago> Stonks only go up And the signature totally explains their childish reply, over there and here.
- MyMonkeyBalls 3y agoYeah even a child like me is better at secure programming than Theo De Raadt
- petee 3y agoThen report the bug like an adult instead of acting like you found some huge published vulnerability in code that was posted for peer review. Thats why the code is there, so others can identify issues; congrats, you did. People make mistakes in every project. So grow up
- MyMonkeyBalls 3y ago[flagged]
- tiffanyh 3y agoWould a code analyzer have detected this bug? (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.
- ori_b 3y agoNo, probably not. It requires a crafted binary to be executed.
- qzzi 3y agoI 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.
- tedunangst 3y agoThat seems about right.
- LukeShu 3y ago> 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#L193 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#L269 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.
- trealira 3y ago> 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: 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).
- trealira 3y ago
- accessvector 3y agoOut-of-bounds heap write happens in this function: 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; } 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.
- accessvector 3y agoRe-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`.
- lmm 3y agoImagine trying to write secure code in C by hand.
- MyMonkeyBalls 3y agoYeah and imagine advertising yourself as the "most secure OS in the world" at the same time
- gkbrk 3y agoAnyone else with their track record?
- MyMonkeyBalls 3y agoWhich track record exactly? Their slogan is known to be a complete lie
- IntelMiner 3y ago[citation needed]
- SSLy 3y ago> Two remote public holes in the default install, as in “unauthenticated remote code execution”. How many local privilege escalation? How many remote/local denial of service? How many remote code execution in software not present in the non-default install? How many private ones that were never disclosed? Other operating systems didn’t have many unauthenticated RCE either
- MyMonkeyBalls 3y agoHere are a lot of citations: https://isopenbsdsecu.re/quotes/ https://isopenbsdsecu.re/quotes/
- deleted 3y ago
- trealira 3y agoHave you managed to trigger this? You never ended up explaining how the heap overflow occurs, and I cannot determine whether the other person who was guessing how it might happen is right, because I am not very familiar with OpenBSD's code.