Comment by phoe-krk

3 years 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.

111 comments

phoe-krk

Reply
kevincox  3 years ago

It 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.)

dathinab  3 years ago

It'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  3 years ago

    It'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.

patrakov  3 years ago

Even more evil would have been to replace this line

    (void)SYS_landlock_create_ruleset;

with this:

    (void)SYS_landloсk_create_ruleset;

  • saagarjha  3 years ago

    Putting random Unicode confusables in source code would be far easier to consider malicious

    • mgaunard  3 years ago

      Except many people have a policy of ASCII only in source code and therefore would catch it immediately.

  • bschwindHN  3 years ago

    Another point where I appreciate the Rust compiler:

        warning: the usage of Script Group `Cyrillic` in this crate consists solely of mixed script confusables
     --> src/lib.rs:1:4
      |
    1 | fn SYS_landloсk_create_ruleset() {
      |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
      |
      = note: the usage includes 'с' (U+0441)
      = note: please recheck to make sure their usages are indeed what you want
      = note: `#[warn(mixed_script_confusables)]` on by default

    https://play.rust-lang.org/?version=stable&mode=debug&editio...

    • nebulous1  3 years ago

      The character was in a string, not directly in what was being compiled. The contents of the string failing to compile was the point, as landlock was then disabled.

      From what I understand, this landlock disabling wasn't relevant to the sshd attack. It appears it was setting up for something else.

  • dathinab  3 years ago

    After the concept of such attacks had blow up idk. 1?2?3? years ago a lot of places now warn at least when mixing "languages"/ranges or sometimes on any usage of non us-ascii printable characters.

    So stuff like that is increasingly more reliable caught.

    Including by 3rd parties doing any ad-hoc scanning.

    Through adding `compiles check` fail with syntax errors definitely should be added tot he list of auto scan checks, at least in C/C++ (it's not something you would do in most languages tbh. even in C it seems to be an antipatters).

  • Etheryte  3 years ago

    Nearly every modern IDE and diff viewer instantly highlights this though? I doubt this would get far.

082349872349872  3 years 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  3 years 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.

  • yegle  3 years ago

    In 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  3 years ago

      No, 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.

      3 replies →

  • philsnow  3 years ago

    AST 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.

    • eru  3 years ago

      > 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.

      Not sure, you could also just forbid code that's too complex to analyse without going down a rabbit hole. Instead of trying to go down the rabbit hole.

      In general, it's hard to analyse arbitrary code. But we don't have to allow arbitrarily complex code when doing code review.

    • mewpmewp2  3 years ago

      I think main issue was that it was embedded in the file itself like that. Would have preferred to have it in a separate valid C file with syntax highlighting etc and being parsed from that file.

      5 replies →

sidewndr46  3 years 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