[HN Gopher] Xz: Can you spot the single character that disabled ...
___________________________________________________________________
Xz: Can you spot the single character that disabled Linux landlock?
Author : dhx
Score : 67 points
Date : 2024-03-30 12:56 UTC (10 hours ago)
(HTM) web link (git.tukaani.org)
(TXT) w3m dump (git.tukaani.org)
| dhx wrote:
| Answer:
| 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
|
| xz official (maybe...) incident response page:
| https://tukaani.org/xz-backdoor/
| jstanley wrote:
| What does the dot do?
| Aurornis wrote:
| The 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.
| thrtythreeforty wrote:
| Fails the build, so that Landlock support is never enabled.
| phoe-krk wrote:
| 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 wrote:
| I 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 wrote:
| I 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 wrote:
| Yeah, 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 wrote:
| Mixing 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 wrote:
| Yep. 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.
| undebuggable wrote:
| This is so vile that even if caught red-handed during PR one
| could shrug off "oh, my IDE's auto formatting did this".
| GrayShade wrote:
| Only in CMake, this time. Not in the autotools version.
| pxx wrote:
| What 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 wrote:
| The 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.
| danieldk wrote:
| Commit that introduced this:
|
| https://git.tukaani.org/?p=xz.git;a=commit;h=328c52da8a2bbb8...
| karmakaze wrote:
| I 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 wrote:
| Well, do we know if that commented code you quoted is accurate?
| karmakaze wrote:
| It's the content of TFA, so flag it if you believe it isn't.
| kardos wrote:
| Yes that is what I mean -- the commit in TFA is from the
| bad actor -- so the quoted comment is suspect..
| legobmw99 wrote:
| I'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 wrote:
| That'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.
| JSDevOps wrote:
| Yeah, I wasn't sure what the diff is alluding to here but I
| assume "mysandbox" could remain undetected (enabled/disabled) for
| whatever reason.
| jijijijij wrote:
| I 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 wrote:
| > 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 wrote:
| Is this by design, or by legacy? I mean, is there a better
| way to do this? Seems really flawed to me.
___________________________________________________________________
(page generated 2024-03-30 23:00 UTC)