16 ms·
Xz: Can you spot the single character that disabled Linux landlock?
- dhx 3y agoAnswer: https://git.tukaani.org/?p=xz.git;a=commitdiff;h=f9cf4c05edd14dedfe63833f8ccbe41b55823b00 https://git.tukaani.org/?p=xz.git;a=commitdiff;h=f9cf4c05edd... Description of Linux's Landlock access control system if you are not familiar with it: https://docs.kernel.org/userspace-api/landlock.html https://docs.kernel.org/userspace-api/landlock.html xz official (maybe...) incident response page: https://tukaani.org/xz-backdoor/ https://tukaani.org/xz-backdoor/
- jstanley 3y agoWhat does the dot do?
- Aurornis 3y agoThe function is “check_c_source_compiles”. The comment indicates that the intention is to confirm that the Landlock functionality can be compiled on the system, in which case it will be enabled. The stray dot isn’t valid C, so it will never compile. By ensuring it can never compile, Landlock will never be enabled.
- astrange 3y agoconfigure would print that it's not enabled, so it seems like the kind of thing people would eventually notice.
- masklinn 3y agoMaybe, eventually, but how many people read the reams of garbage autoconf spouts out until the feature they wanted fails to materialize?
- astrange 3y agoI used to and it's actually how I got a large part of my computer skills, but unfortunately I got medicated for ADHD and became normal and can't do it anymore. But I still think reading boring logs is a great way to understand a system. Turning on all the intermediates for a compiler (llvm ghc etc) is very educational too.
- makomk 3y agoThe actual compiler output from the autoconf feature test is one of the things I'd probably look at fairly early on if some feature is disabled when it shouldn't be, but I maybe have a bit more experience running into problems with this than younger folks.
- masklinn 3y ago> if some feature is disabled when it shouldn't be That's my point, you're going to look at autoconf's output if a feature you're expecting is missing. But would you think to have a test or expectation for landlock in xz? How would you even check if landlock is enabled for a process?
- takeda 3y agoLooks like he only introduced that "bug" in CMake. Was there migration in progress from autoconf to cmake?
- ak39 3y agoShouldn't there be a unit test to confirm landlock is on/off? (I mean, this seems a crucial aspect of the code which needs 100% test coverage.)
- patrakov 3y agoThis is not something that a unit test can catch. First, this 100% coverage rule applies to the program/library code, and only to the subset that is not ifdeffed out (otherwise, you will not be able to have, e.g., Windows-specific code), and definitely not to the build system's code. Second, how would you test that landlock works, in a unit test, when this feature is optional and depends on the system headers being recent enough? You can't fail a unit test just because the software is being compiled on an old but still supported system, so it would be a "SKIPPED" result at best, which is not a failure and which is not normally caught.
- account42 3y agoThe proper way would be to have a minimum glibc version (or whatever it depends on) where you expect landlock to be available and then shout loudly if it is not so that you can either fix the check or correct your expectations. This isn't just for malicious users, these checks can be brittle enough that a small change in the library or even compiler update can occasionally break something. Of course this is ideal and does not match common practice. I can't even claim of doing this consistently myself although I did start that practice before this mess.
- thrtythreeforty 3y agoFails the build, so that Landlock support is never enabled.
- usr1106 3y agoI don't think it fails the build. It's part of a test, trying to compile a bit of code. If it compiles the test is true and a certain feature is enabled. If the compilation fails the the feature is disabled. This is quite common in build systems. I had such a test produce incorrect results in the kernel build system recently. The problem is that the tests should really look carefully for an expected error message. If the compilation fails with the expected message the test result is false. If the compilation fails for some other reason the build should fail so the developer is forced to investigate. Disclaimer: I have not studied the xz build system in detail, just what I saw from the diff plus a bit of extrapolation.
- akdev1l 3y agoIt fails that small build that is testing for landlock functionality. Hence it doesn’t build the support for it. It doesn’t fail the overall build.
- jcelerier 3y agoProblem is that every compiler/compiler version will have different messages for the same error. And xz is the kind of project that gets built on really really random homegrown compilers. Maybe we need an option to make the compilers output a standard JSON or whatever...
- MereInterest 3y ago> Maybe we need an option to make the compilers output a standard JSON or whatever... While I like the idea, I fear this would go the same route as UserAgent strings, with everybody giving carefully crafted lies. The main issue is that bugs are only known in retrospect. As an example, suppose Compiler A and Compiler B both implement Feature X. Therefore, they both produce JSON output indicating that they support Feature X. A year later, developers for Compiler A find and fix a bug in its implementation of Feature X. Now, how do we handle that case? * Update the version for Feature X as by Compiler A? Now Compiler B is publishing the same version as the buggy Compiler A did, and would get marked as not having a usable implementation. * Update the spec, forcing a version update for Feature X in both Compiler A and Compiler B? Now a bugfix in one compiler requires code updates in others. * In the build system, override the JSON output from Compiler A, such that Feature X is marked as unavailable for the buggy versions? Now the JSON is no longer trustable, as it requires maintaining a list of exceptions. * Try to trigger the bug that Compiler A just fixed, so that the build system can recognize the buggy versions, regardless of the JSON output? Now we're back to the starting point, with features determined based on attempted compilation.
- phoe-krk 3y agoSo 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
- undebuggable 3y agoThis is so vile that even if caught red-handed during PR one could shrug off "oh, my IDE's auto formatting did this".
- asveikau 3y agoIt seems like Lasse Collin is back on the scene and maybe pissed?
- Dalewyn 3y agoIf he is innocent and a victim as much as everyone else in all this, I won't blame him for wanting blood. Most of us are humans after all, and being social creatures we tend to take violations of trust quite deeply.
- danielhlockard 3y agoTruly seems that way currently. He said he'd really dig in starting next week and just checked his email on vacation and saw this whole mess.
- asveikau 3y agoIt may be hard for him to re-establish trust. Maintaining xz for more than a decade then doing this would be quite a "long con" but if HN threads are any indication, many will still be suspicious. His commits on these links look legit to me. It's a sad situation for him if he wasn't involved.
- londons_explore 3y agoThe fact GitHub suspended his account too suggests that they might have info saying he is involved.
- Aeolun 3y agoI feel like my version control system is better about highlighting the changed characters than these solid green or red strings.
- TheRealPomax 3y agoIt's git, so it can show a vastly better diff, just not from a URL with hardcoded diff settings.
- Aeolun 3y agoYeah, but I’m wondering where the code review was done. In a different context, would this be easier to spot?
- XorNot 3y agoI don't love unified diffs as a rule either. They're very noisy to read in general. A side by side in something like "meld" will highlight changes but also means you're reading the full context as it will exist in the code base. My number one complaint about "code review" is the number of people who simply run their eyes over the diff (in private orgs) because management says "code review" and that's as far as they ever take that in terms of defining outcomes.
- eru 3y agoI love unified diffs for many applications. You are right that side by side diffs have their uses, too.
- masklinn 3y agoIt’s a giant block of new code, what is it supposed to do beyond tell you it’s a giant block of new code? Note that this is C code inside if a cmake string, even if your diff can do highlighting the odds it would highlight that are low, and if it did highlighting is generally just lexing so there wouldn’t be much to show.
- kristopolous 3y agoThis has plausible deniability on it. There's better ways to hide by swapping in Unicode lookalike characters. Some of them even pixel match depending on the font. Maybe I'm out of the loop but is intentionality settled here?
- zzzeek 3y agothe period right there on the left edge? if I saw that in a patch I'd be through the roof, that looks completely intentional
- aeyes 3y agoI'd never suspect this to be intentional if I'd spot it in a patch, even given the consequences in this particular case. I have written and committed things into code instead of writing it into some other window several times. Without a linter I probably wouldn't spot an extra dot when reviewing my own change before sending it out.
- kristopolous 3y agoNot me. Maybe someone is using an editor with a . key mapped to something. It's in a pretty convenient place. :wq!
- throwaway2037 3y agoI love this type of hindsight to 20-10 comment. "If I saw...". That is a BIG if. Plenty of smart people on that mailing also missed it. I missed it myself when I opened the HN lead link. Very subtle.
- zzzeek 3y agoI dont use mailing lists for patches, I use gerrit that would put a great big highlight over that character
- Tuna-Fish 3y agoI distinctly remember having to remove such a superfluous . that I accidentally added into a file on multiple occasions. If you are using vi and your left shift key is dodgy, that can easily happen by accident.
- deleted 3y ago[deleted]
- deleted 3y ago[deleted]
- GrayShade 3y agoOnly in CMake, this time. Not in the autotools version.
- almostnormal 3y agoDoes autotools compile only, or try to link? There's no main().
- jan3000 3y agoThere is a main()...[0] [0] https://git.tukaani.org/?p=xz.git;a=blob;f=CMakeLists.txt;h=d2b1af7ab0ab759b6805ced3dff2555e2a4b3f8e;hb=d2b1af7ab0ab759b6805ced3dff2555e2a4b3f8e#l921 https://git.tukaani.org/?p=xz.git;a=blob;f=CMakeLists.txt;h=...
- jwilk 3y agoThat's not autotools.
- jwilk 3y agoAC_COMPILE_IFELSE indeed only compiles, so there's no need for main(). There's AC_LINK_IFELSE if you want to test linking too.
- pxx 3y agoWhat was even the game here? Eventually even more backdoors, ones that would have more plausible deniability? Afaict neither the oss-fuzz nor this change would actually discover the found backdoor. But why put your backdoor eggs into one basket (library)?
- bayindirh 3y agoThe library is entrenched enough, trusted enough, and its main developer has long internet breaks because of mental health problems. Plus, you do not backdoor the library itself, but the tools using it. "Reflections on trusting trust" style. Sounds like a perfect plan, until it isn't.
- dagmx 3y agoWho says it was just the one library though?
- microtonal 3y agoCommit that introduced this: https://git.tukaani.org/?p=xz.git;a=commit;h=328c52da8a2bbb81307644efdb58db2c422d9ba7 https://git.tukaani.org/?p=xz.git;a=commit;h=328c52da8a2bbb8...
- karmakaze 3y agoI can't believe that system security is dependent on such a loose chain of correctness. Any number of things could have stopped this. + # A compile check is done here because some systems have + # linux/landlock.h, but do not have the syscalls defined + # in order to actually use Linux Landlock. Fix those headers on those systems to explicitly opt-out. What's the point of headers if they don't declare their capabilities? Also why isn't there a single test after a binary blob (even when compiled from open source) is made to ensure security is in-tact? I wouldn't even ship a website checkout without end-to-end tests for core capabilities. There must be a priority misalignment of adding features > stability. Edit: I hope the 'fix' isn't to remove the '.'--I just saw the other post on HN that shows removing the '.'
- kardos 3y agoWell, do we know if that commented code you quoted is accurate?
- pas 3y ago... there's a huge bias in things that get tested (and how they get tested). easy to test things get tested. there's a lot more developer, committer, maintainer for web stuff. there's a cultural issue, it's hard to improve on old ossified processes/projects, etc.
- Hnrobert42 3y agoAs a user of open source who doesn’t know enough to make those suggestions, I would be grateful if you would develop and contribute them.
- legobmw99 3y agoI’m not quite following what the diff here is suggesting - was this some cmake magic to detect if a feature was enabled, but the file had an intentional syntax error?
- bean-weevil 3y agoThat's exactly right. It was checking if the c code compiled to detect the landlock feature, and there was a single period in the middle of the code that made it always fail to compile and thus silently leave the feature disabled.
- deleted 3y ago[deleted]
- db48x 3y agoAutoconf and CMake both compile small test programs to verify that the feature in question actually works. The test programs almost never actually do anything; the just refer to the feature of function that they rely on. If there is a compile or linker error then the feature isn’t available. In this case the compiler always outputs an error because the test program doesn’t have valid C syntax. Of course in practice you shouldn’t be writing each test program by hand. Autoconf has macros that generate them; you would only need to supply an identifier or at most a single line of code and the rest would be created correctly. I’m sure CMake is similar in that regard, so the first red flag is that they wrote a whole test program.
- JSDevOps 3y agoYeah, I wasn't sure what the diff is alluding to here but I assume "mysandbox" could remain undetected (enabled/disabled) for whatever reason.
- jijijijij 3y agoI don't know enough about C and complex builds, but the proposed change appears to be kind of a red flag, even without the breaking dot. - check_include_file(linux/landlock.h HAVE_LINUX_LANDLOCK_H) ... + check_c_source_compiles(" + #include <linux/landlock.h Is a compilation-test a legitimate/common/typical method to go about this? Independently, of the breaking code, to me it seems accidental failing, or even accidentally not failing, would be in the nature of such an assessment... So, this commit seems to raise the question of "why?", even if you missed the dot, doesn't it? If a feature is formally available, but effectively broken somehow, wouldn't you want the compiler to complain, instead of the feature dropped silently? Is the reasoning in the code comment sound? Can you test, if syscalls are defined in another way?
- ronsor 3y ago> Is a compilation-test a legitimate/common/typical method to go about this? Yes—in fact, compilation tests are often the only way you can tell if a feature actually works. It's extremely common for C build systems to detect and work around weird systems.
- jijijijij 3y agoIs this by design, or by legacy? I mean, is there a better way to do this? Seems really flawed to me.
- ninkendo 3y agoIt’s by design. The job of autotools is to find “ground truth” about whatever environment you’re compiling against. It’s meant to discover if you can use a feature by actually seeing whether it works, not just by allow-listing a known set of compiler or library versions. This is because the whole point is to allow porting code to any environment where it’ll work, even on compilers you don’t know about. Think back to a time when there were several dozen Unix vendors, and just as many compilers. You don’t want your build script to report it can’t compile something just because it isn’t aware of your particular Unix vendor… you want it to only fail if the thing it’s trying to do actually doesn’t work. The only way to do this is by just testing if certain code compiles and produces the expected result.
- trelane 3y agoI wonder why it was useful to prevent Landlock from being enabled in xz. Perhaps a later stage was to inject malicious content into xz archives? But then why not just inject malicious activity in xz itself?
- ColonelPhantom 3y agoBecause a deliberate vulnerability is much easier to hide than actual malicious content. One could probably sneak a buffer overflow or use-after-free into a C project they maintain without being noticed. Actually shipping a trojan is much harder, as observed with the xz-to-sshd backdoor.
- trelane 3y agoAh, so the next stage would have been to add a "bug" in xz that would trigger during the supposedly sandboxed execution, when presented with certain input files. Clever.
- puetzk 3y agoWell, is also quite possible that adding such a bug was the previous stage. Or even just having found one that you didn't report/fix...
- A1kmm 3y agoUnless there are more subtle backdoors that target xz itself beyond the targeting of ssh. Clearly the aim was to be subtle.
- clnhlzmn 3y agoWould it be reasonable to expect that this MR comes along with a test that shows that it does the thing it’s claiming to do? I’m not sure how that would work in this case.. have a test that is run on a system that is known to have landlock that does something to ensure that it’s enabled? Even that could be subverted, but it seems like demanding that kind of thing before merging “features” is a good step.
- adrianmonk 3y agoCreate a tiny fake version of landlock with just the features you're testing for. Since it's only checking for 4 #defines in 3 header files, that's easy. Then compile your test program against your fake header files (with -Imy-fake-includes). It should compile without errors even if landlock is missing from your actual system. Then build your test program a second time, this time against the real system headers, to test whether landlock is supported on your system.
- masspro 3y agoI like the idea of testing build-system behaviors like this, and I don’t think it’s ever really done in practice. Scriptable build systems, for lack of a better name for them, exist at a bad intersection of Turing complete, hard to test different cases, hard to reason about, hard to read the build script code, and most of us treating them as “ugh I hope all this stuff works” and if it does “thank god I get to ignore all this stuff for another 6 months”.
- azakai 3y agoIf you mean testing the "disable Landlock if the headers and syscalls are out of sync" functionality then I agree, workarounds for such corner cases are often not fully tested. But it would have been enough here to have a test just to see that Landlock works in general. That test would have broken with this commit, because that's what the commit actually does - break all Landlock support. Based on that it sounds like there wasn't a test for Landlock integration, if I've understood things correctly.
- viraptor 3y ago
- snnn 3y agoSo for each optional feature we may need three build options: 1. Force enable 2. Enable if available 3. Force disable Like, --enable_landlock=always --enable_landlock --disable_landlock
- glandium 3y agoMy rule of thumb is that things should never be disabled automatically. Make the test hard fail and print a message that the feature can be disabled.
- ComputerGuru 3y agoThat’s how you make unusable/uncompilable software. It might be a good rule for something security critical like ssh but not as a general rule.
- glandium 3y agoI'd rather have people complaining about having to run configure a bunch of times to disable several features they don't have the libraries for than complaining that a feature doesn't work (because it ended up being disabled without them knowing). Likewise, I'd rather distros figure out the hard way when a new release has a new feature and needs a new dependency rather than their users complain that a new feature is missing. Principle of least surprises.
- cperciva 3y agoIn my code I have a bunch of routines optimized for different platforms, e.g. using x86 AESNI instructions. Not all compilers support them, and they don't even make sense when compiling for a different CPU architecture. It's much simpler to say "enable this if we can compile it" rather than detecting the compiler and target platform and throwing a mess of conditional compilation into an autoconf script.
- xorcist 3y agoThat's ... how autoconf works? If you explicitly set the enable-landlock flag, configure will fail when the feature doesn't compile.
- rossjudson 3y agoThere I am, scanning carefully, and I see a period where one clearly should not be. "This wasn't so hard", I said to myself. I poked my screen, and the period moved. Curse you, monitor dust particle.
- CGamesPlay 3y agoOn an unrelated note, this malware team has assembled a great dataset for training AIs on identifying security problems. Every commit has some security problem, and the open source community will be going through and identifying them. (Thanks, maintainers, for the cleanup work; definitely not fun!)
- luyu_wu 3y agoOne of the cooler uses of AI I've seen!
- wizzwizz4 3y agoCurrently it doesn't work, but yeah, it'll be really cool when we have tech like that! (It'll still only be able to detect known vulnerabilities, but we don't often invent new ones.)
- chilling 3y agoI hear this stuff for the first time, can you post some info about that?
- CGamesPlay 3y agoLike in this article, we have a patch that introduces a security flaw (disabling landlock). We later have a patch that fixes it, specifically. The job of the LLM is to reproduce the fixing patch given the problem patch. Or at the very least, explain that this patch results in landlock always being disabled. To be clear, this problem is much, much harder than the problems LLMs are solving now, requiring knowledge of autotools behavior that isn’t included in the context (identifying that a failed build disables the feature, and that this build always fails). There was another example where this team submitted a patch that swapped safe_fprintf for fprintf while adding some additional behavior. It was later pointed out that this allows printing invisible characters to the stream, which allows hiding some of the files that are placed when decompressing.
- bawolff 3y agoI doubt it will generalize well. At best its just an arms race.
- fcanesin 3y agoGeez, his last commit is making security reports worse: https://git.tukaani.org/?p=xz.git;a=commitdiff;h=af071ef7702debef4f1d324616a0137a5001c14c;hp=0b99783d63f27606936bb79a16c52d0d70c0b56f https://git.tukaani.org/?p=xz.git;a=commitdiff;h=af071ef7702...
- eacapeisfutuile 3y agoWhy is that accepted? Serious question
- foooorsyth 3y agoBecause nobody’s really paying attention. “LGTM!”
- eacapeisfutuile 3y agoGenerally yes, but ripping all conditions out of SECURITY.md should at least raise an eyebrow?
- indrora 3y agoNobody was watching. Plain and simple. If you have commit access to it, and nobody is there to see, nothing stops you.
- eacapeisfutuile 3y agoYes but if that’s the sentiment how is this not as problematic as the npm ecosystem.
- snazz 3y agoIt’s similarly problematic but on a somewhat smaller scale and with fewer levels of nested dependencies.
- usr1106 3y agoWhere/how was landlock supposed to be used? I guess you cannot really use it in a generic library like compression/decompression. The library has no clue what the program is supposed to do and what should be restricted. For a program it might be clearer. The sshd attack was using liblzma as a library. So disabling landlock seems unrelated? A sign that there is more bad code waiting to be detected / had been planned to be inserted???
- bawolff 3y agoIts weird. Like i would consider doing two unrelated backdoor-esque things in the same project really sloopy. Seems like it just significantly increases the risk of being discovered for minimal gain. Its very confusing. Parts of this sega seem incredibly sophisticated while other parts seem kind of sloppy.
- akdev1l 3y agoPresumably sshd itself used to lock down its own capabilities after a certain point of execution. Removing the landlock means the daemon doesn’t lock itself down and will allow for better payload execution when they get to the exploitation stage. I don’t think these two things are unrelated. I think they already had payloads in mind and realized this would be a hurdle.
- deleted 3y ago[deleted]
- IshKebab 3y agoThis is surprisingly obvious. I mean it's a clever technique to disable the feature and a really plausible commit. But then why did they go with an obvious syntax error instead of just misspelling something. E.g. would you have spotted it if the mistake was `PR_SET_NO_NEW_PRIV`? More plausibly deniable too.
- leni536 3y agoOptional features should not depend on feature detection, ever. Feature detection for a security feature should be suspicious, even if it works as intended. Optional features should always be configured by whoever tries to compile the code. There can be defaults, but they shouldn't depend on the environment.
- acqq 3y agoNow I'd like to have a link to that patch in code used by Apple that also appeared as a typo. Anybody remembers?
- frankjr 3y agoYou mean the infamous "goto fail"? Yeah that was fun too. https://www.imperialviolet.org/2014/02/22/applebug.html https://www.imperialviolet.org/2014/02/22/applebug.html
- Cloudef 3y agoThis is why I really don't like configure style build systems that automatically enable / disable features. When I want something I explicitly opt-in for it. If there's good reason for feature to be default, then instead explicitly allow opt-out.
- kevincox 3y agoThis is a huge pain when packaging things. You set up the package, add dependencies until it builds and think you are done. But they feature X is missing. What? Oh, it was silently disabled because libY isn't available. Ok, go back and add it. Then a user reports that feature Z isn't available... Yeah, just have a default set of features and allow `--enable-a --disable-b` as needed. The fact that it silently swallows bugs in the check is just another problem of it.
- BobbyTables2 3y agoWorse, it enables features based on local system packages whose dependencies aren’t captured by the package dependencies. At least for a time, this was a horrible problem in Yocto.
- SillyHNDorks 3y ago[flagged]
- mjcohen 3y agoThis may be naive, but it seems to me that a failed compile should generate an error worth worrying about.
- kzrdude 3y agoIt's in an autoconf style compile test. The compile or not of the snippet is being tested, unfortunately. This just decides the configuration, doesn't stop the build.