13 ms·

Answer: https://git.tukaani.org/?p=xz.git;a=commitdiff;h=f9cf4c05edd14dedfe63833f8ccbe41b55823b00 https://git.tukaani.org/?p=xz.git;a=commitdiff;h=f9cf4c05edd..

by dhx 3y ago

Answer: 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 ago
What does the dot do?
- Aurornis 3y ago
  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.
  - astrange 3y ago
    configure would print that it's not enabled, so it seems like the kind of thing people would eventually notice.
    - masklinn 3y ago
      Maybe, eventually, but how many people read the reams of garbage autoconf spouts out until the feature they wanted fails to materialize?
      - astrange 3y ago
        I 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 ago
        The 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 ago
      Looks like he only introduced that "bug" in CMake. Was there migration in progress from autoconf to cmake?
  - ak39 3y ago
    Shouldn'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 ago
      This 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 ago
        The 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 ago
  Fails the build, so that Landlock support is never enabled.
  - usr1106 3y ago
    I 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 ago
      It 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 ago
      Problem 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 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 ago
  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 3y ago
  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 3y ago
    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 3y ago
      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
undebuggable 3y ago
This 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 ago
It seems like Lasse Collin is back on the scene and maybe pissed?
- Dalewyn 3y ago
  If 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 ago
    Truly 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 ago
      It 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 ago
        The fact GitHub suspended his account too suggests that they might have info saying he is involved.
        cricalix 3y ago
        Or you know, they just reacted with a ban hammer on all accounts related to xz and to heck with innocence.
        sReinwald 3y ago
        Personally, I doubt that. I would assume that GitHub just banned all accounts that were directly associated with the project as a precaution.
Aeolun 3y ago
I feel like my version control system is better about highlighting the changed characters than these solid green or red strings.
- TheRealPomax 3y ago
  It's git, so it can show a vastly better diff, just not from a URL with hardcoded diff settings.
  - Aeolun 3y ago
    Yeah, but I’m wondering where the code review was done. In a different context, would this be easier to spot?
- XorNot 3y ago
  I 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 ago
    I love unified diffs for many applications. You are right that side by side diffs have their uses, too.
- masklinn 3y ago
  It’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 ago
This 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 ago
  the 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 ago
    I'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 ago
    Not me. Maybe someone is using an editor with a . key mapped to something. It's in a pretty convenient place. :wq!
  - throwaway2037 3y ago
    I 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 ago
      I dont use mailing lists for patches, I use gerrit that would put a great big highlight over that character
  - Tuna-Fish 3y ago
    I 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.

by dhx 3y ago

jstanley 3y ago

What does the dot do?

Aurornis 3y ago
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.
- astrange 3y ago
  configure would print that it's not enabled, so it seems like the kind of thing people would eventually notice.
  - masklinn 3y ago
    Maybe, eventually, but how many people read the reams of garbage autoconf spouts out until the feature they wanted fails to materialize?
    - astrange 3y ago
      I 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 ago
      The 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 ago
    Looks like he only introduced that "bug" in CMake. Was there migration in progress from autoconf to cmake?
- ak39 3y ago
  Shouldn'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 ago
    This 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 ago
      The 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 ago
Fails the build, so that Landlock support is never enabled.
- usr1106 3y ago
  I 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 ago
    It 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 ago
    Problem 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 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 ago
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 3y ago
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 3y ago
  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 3y ago
    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

undebuggable 3y ago

This 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 ago

It seems like Lasse Collin is back on the scene and maybe pissed?

Dalewyn 3y ago
If 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 ago
  Truly 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 ago
    It 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 ago
      The fact GitHub suspended his account too suggests that they might have info saying he is involved.
      - cricalix 3y ago
        Or you know, they just reacted with a ban hammer on all accounts related to xz and to heck with innocence.
      - sReinwald 3y ago
        Personally, I doubt that. I would assume that GitHub just banned all accounts that were directly associated with the project as a precaution.

Aeolun 3y ago

I feel like my version control system is better about highlighting the changed characters than these solid green or red strings.

TheRealPomax 3y ago
It's git, so it can show a vastly better diff, just not from a URL with hardcoded diff settings.
- Aeolun 3y ago
  Yeah, but I’m wondering where the code review was done. In a different context, would this be easier to spot?
XorNot 3y ago
I 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 ago
  I love unified diffs for many applications. You are right that side by side diffs have their uses, too.
masklinn 3y ago
It’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 ago

This 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 ago
the 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 ago
  I'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 ago
  Not me. Maybe someone is using an editor with a . key mapped to something. It's in a pretty convenient place. :wq!
- throwaway2037 3y ago
  I 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 ago
    I dont use mailing lists for patches, I use gerrit that would put a great big highlight over that character
- Tuna-Fish 3y ago
  I 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.