12 ms·
So that function checked if the following C code compiled, and only in that situation enabled the landlock? Except that lone period, hard to recognize because
by phoe-krk 3y ago
So that function checked if the following C code compiled, and only in that situation enabled the landlock?
Except that lone period, hard to recognize because of its small size and proximity to the left edge of the diff, caused the C code to become always invalid, hence keeping the landlock always disabled?
That's both vilely impressive and impressively vile. I didn't even spot it on my first read-through.
- sidewndr46 3y agoI agree. The way I read this, I missed it completely until I searched for it. I was looking too closely at the rest of the code around defines
- 082349872349872 3y agoI just got a little more respect for pythonic whitespace-sensitivity EDIT: come to think of it, even that might not have done much here, where well-formedness is the issue :(
- capitainenemo 3y agoYeah, if anything, python worsens the situation. I had a friend DOS our server because he accidentally inserted a tab, causing the illusion that one statement was inside a block but was actually outside it. He swore off python at that point. I personally avoid the language, but I understand due to issues like that these days mixing tabs and spaces is an error (or is it just a warning?) by default. Regardless, still pretty silly to me to have whitespace play such a major significant role, besides the fact that I find it visually harder to read, like text without punctuation or capitalisation.
- secstate 3y agoMixing tabs and spaces usually throws a runtime exception. I'm not gonna make a value judgement about that, but your story doesn't make sense based on how I understand py3 Edit, sorry, shoulda read your whole commebt before replying
- capitainenemo 3y agoYep. It was a few years ago while that was stilled allowed (as I'd noted ;) ) but regardless. Significant whitespace is just annoying.. There's a lot of things that render as whitespace, and source code one might be reviewing could be printed wrapped or copied and pasted in odd ways. Other languages are more robust to this.
- _visgean 3y agoNever had this happen, this is largely eliminated by using tools like black or other autoformatters.
- eru 3y agoAnd/or linters. This issue is no worse that having multiple different variables that look the same and only in their obscure same-looking unicode symbols. A linter can handle this. As can a decent font, or editor settings.
- MrDresden 3y agoThe point still stands that this is an inherit possibility in the language it self.
- eru 3y agoWell, in C the visual indentation and the meaning of the code (given by {}) can diverge. That's even worse, and happens in practice. Many style guidelines for C have evolved specifically to address that problem with 'the language itself'.
- worik 3y ago> Well, in C the visual indentation and the meaning of the code (given by {}) can diverge. How?
- 3y ago
- katbyte 3y agowhite space as a delimiter is why i never use python.
- naikrovek 3y ago> white space as a delimiter is why i never use python. Whitespace is a delimiter in (almost?) all languages humans use. Whitespace determining which scope you’re in is one of the many problems of making whitespace significant, which might be what you meant.
- tomoyoirl 3y ago“The thing that controls scope is the count of certain nonprinting characters, which happen to come in multiple widths” is reasonably insane, yes
- eru 3y agoWhich non-printing characters are you talking about? Whitespace characters are very much printable. Yes, I agree that Python should just forbid tabs. As a second best, you can tell your linter (and/or formatter) to forbid tabs (and any weird unicode whitespace). That's basically the equivalent of compiling your C with -Wall.
- takeda 3y agoThis was an issue in Python 2, where for some dumb reason it allowed mixing tabs and spaces and equated tab to 8 spaces (I think). Python 3 doesn't have that issue.
- capitainenemo 3y agoOf course, if one was coding an exploit, one could still use python2. It is still commonly available due to a long tail of legacy scripts and in some cases (like a script I use routinely but didn't write) the difficulty of porting it to python3 (I've asked over a dozen pythonistas over the years, they kept running into same problems)
- raverbashing 3y agoYes mixing tabs and spaces is a big no no and rightfully throws an error now
- worik 3y agoNot an improvement Tabs still have syntactical meaning and are still invisible Python will never be a success, I predict, because of this Trust me
- fragmede 3y agohumans are weird creatures sometimes. there was this bad thing that happened that won't happen again now, but now I can't use the thing forever because Reasons.
- yegle 3y agoIn Python, you only need to indent the `def test_foo` by an additional whitespace, to make it a locally scoped function instead of a proper test function.
- eru 3y agoNo, not if you have any sane linter or formatter involved. They wouldn't let you get away with indenting by a single space, but only by multiples of whatever you normally use in the rest of your program.
- jrockway 3y agoI mean, some CI system should be checking that "if_code_compiles()" blocks compile somewhere. It should be an error until the CI system has that header and can test both variants. People are really quick to add optionality like this without understanding the maintenance cost. (Every boolean feature flag increases the number of variants you need to test by 2!) Either make a decision, or check that both sides work. Don't let people check in dead code.
- philsnow 3y agoAST diffs instead of textual diffs might have helped here (to spot the `.` making the code un-compilable). Edit: oof, though the stray character in question is inside a perfectly legitimate C string, so to catch this, any such diffs would need to Matroyshka down and that seems unsolvable / intractable.
- raverbashing 3y agoAh yes but thanks to C being cursed due to includes and macros this is harder to do
- uecker 3y agoHuh, the code with a dot is not legal C. It is CMake issue that the test breaks here.
- acdha 3y agoThat’s what makes this so clever: these systems were born in the era where you couldn’t trust anything - compilers sometimes emitted buggy code, operating systems would claim to be Unix but had weird inconsistencies on everything from system calls to command line tool arguments, etc. - so they just try to compile a test script and if it fails assume that the feature isn’t supported. This is extremely easy to miss since the tool is working as it’s designed and since it’s running tons of stuff there’s a ton of noise to sort through to realize that something which failed was supposed to have worked.
- josefx 3y agoYou could just run tests for the feature detection on a known system or a dozen(VMs are cheap). The big problem is that most code is not tested at all or test errors are flat out ignored.
- acdha 3y agoNothing “just” about it. VMs weren’t cheap when the C world started using autoconf - that was a feature mostly known on IBM mainframes – and in any case that couldn’t do what you need. The goal is not to figure out what’s there on systems _you_ control, it’s to figure out what is actually available on the systems other users are building on. Think about just this library, for example, your dozen VMs wouldn’t even be enough to cover modern Linux much less other platforms but even if it was (drop most architectures and only support the latest stable versions for the sake of argument), you’d still need to build some reliable prober for a particular bug or odd configuration. The problem here wasn’t the concept of feature detection but that it was sabotaged by someone trusted and easily missed in the noise of other work. What was needed was someone very carefully reviewing that commit or build output to notice that the lockdown feature was being disabled on systems where it was fully enabled, and we already know that maintainer time was in short supply. Any other approach would likely have failed because the attacker would have used the same plausible sounding language to explain why it needed to be a complicated dynamic check.
- deleted 3y ago[deleted]
- patrakov 3y agoEven more evil would have been to replace this line (void)SYS_landlock_create_ruleset; with this: (void)SYS_landloсk_create_ruleset;
- Terr_ 3y agoFor those squinting, the "landlock" regular "c" is replaced with a Cyrillic U+0441.
- SV_BubbleTime 3y agoIs there a GCC option to error on non-standard English characters?
- Terr_ 3y agoDisclosure: I've got zero C/C++ on my resume. I was asked to diagnose a kernel panic and backport kernel security patches once, but it was very uncomfortable. ("Hey, Terr knows the build system, that's close enough, right?") That said, perhaps something like disabling the default -fextended-identifiers [0], and enabling the -Wbidi-chars [1] warning. [0] https://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html#index-fextended-identifiers https://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html... [1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wbidi-chars_003d https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#inde...
- pajko 3y agoCool, the latter was added to fix CVE-2021-42574: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103026 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103026
- masklinn 3y agoError-ing is the point here and what the period achieved. It’s a feature detection snippet so if it fails to compile the feature is disabled.
- dathinab 3y agoIt's also iffy tbh. that the compilation check functionality: - doesn't force users to differentiate between syntax errors and other errors (e.g. symbols not found). Partially you can't even make it work properly if you want due to C macros. - it seems sensible, tbh. if "does compile" checks for compatibility seems sensible then there is so much wrong with the ecosystem in so many ways
- mysidia 3y agoIt's standard autoconf stuff, BUT... the right thing to do for a security option should be to throw an error at the configure stage If the Option is requested but can't be supported, And not to silently turn it off. That is because a human should have to Look at the build and manually make the decision to switch off a security feature from the build.
- BenFrantzDale 3y agoAgreed. I just opened https://gitlab.kitware.com/cmake/cmake/-/issues/25846 https://gitlab.kitware.com/cmake/cmake/-/issues/25846 although I have no idea how hard it is to address this sort of issue robustly.
- kevincox 3y agoIt also shows how brittle these sorts of checks are in general. I really think that this auto-disable of features based on compilation was a big mistake. It is not only very annoying when packaging things (Why does feature X not work? Because you forgot to include libY in the build environment of course) but they also fail silently even if intended. While this `.` was almost certainly malicious it is easy to imagine an accidental break. Maybe a minor typo or `-Werror`is enabled any one of these functions was marked as must use or something similar. It seems like it is infinitely better to have a default set of features and explicitly require the user to say `--enable-x --disable-y` as needed. Then error out if a required library isn't available. (Preferably saying which feature requires it.)
- TsiCClawOfLight 3y agoThis is what the cargo/rust ecosystem does :)