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.
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.)
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
Unicode lookalikes would be detected by IDEs and other tools.
There would be plausible deniability in a different situation, but this is the same author who implemented the backdoor and several similar changes that disable security features. I don't think the benefit of doubt is deserved here.
> Maybe I'm out of the loop but is intentionality settled here?
Yes. The exploit used a payload, that was stored in the tests directory as a binary compression test case, but which is very clearly a very intentional exploit payload.
All the sneaky stuff was about deploying that payload without it being obvious.
I worry about his home/personal system, this guy seems to have befriended Lasse in order to get the position, is it possible Lasse himself has been compromised?
I'd be nuking everything, possibly even disposing of hardware and setting up a new verifiable online identity on a clean system/hardware.
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.
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.
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.
There 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.
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 '.'
You are quoting Jia Tan [1]. The malicious actor wrote that comment when deliberately breaking the check in the first place.
Fixing headers or extra tests would not have prevented this, as there is no indication the headers were broken in the first place, and extra tests could have been compromised (or ignored for release tarball) some other way.
Tangentially related, but I've had a case where GCC 9.4.0 shipped a broken arm_acle.h header [https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100985] - code which includes the header would always fail to compile.
Since users could be trying to compile it on GCC 9.4.0, and as the functionality dependent on the header isn't critical to the application, if the build detects the header is broken, it just disables the functionality and emits a warning.
Back to xz: If security isn't your top priority, it seems reasonable to just ignore the failure of an optional feature and just move along. Of course, in hindsight, it's easy to point fingers, but without that, it doesn't sound completely unreasonable to me.
... 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.
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???
It's pretty impossible to lock down sshd. The restrictions are inherited by all ancestors. What would the sysadmin say if they find themselves in a restricted shell?
Its 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.
Dude is a developer just like the rest of us. We all try to write clever code from time to time, but at other times write sloppy crap. Especially if there's a boss insisting on a tight deadline.
You can use it and it's a good idea, although it's meant to stop exploits rather than deliberate backdoors.
The idea is that the library starts a separate thread to do any complex operation and then waits for it in the API call the user code made. The thread landlocks itself and then begins the operation. If something goes wrong the code is confined.
Of course in this case it doesn't work because the sandbox is controlled by the attacker, so they can just turn it off or make it weaker than it should be. But you can also do things other ways.
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)?
I 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?
Because 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.
Ah, 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.
This 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`?
On 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!)
Currently 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.)
Like 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.
This 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.
This 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.
In 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.
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?
Autoconf 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.
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.
Would 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.
I 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”.
If 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.
Create 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.
I'd say this MR is a bad approach in general. The headers say what interfaces are known, not what features are available. You should be able to compile with landlock support on a system which doesn't enable it. Same situation as seccomp and others. Your build machine doesn't have to match the capabilities of the target runner.
But yeah, to test it, you can have a mock version of landlock which responds with the error/success as you want, regardless of what the system would normally do. It relies on the test not being sabotaged too though...
Read the code of the check again. It mostly checks that the required SYS_* constants are defined to be able to use the syscalls. You can compile this on a system that does not have landlock enabled in the running kernel, but the libc (which imports the kernel system call interface) has to provide the syscall numbers.
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?
> 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.
That "check_c_source_compiles" function should first test if the provided code snipped is "valid C code in general" and only then check if it compiles in given system.
Optional 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.
It'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.
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/
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.
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.)
1 reply →
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
5 replies →
Even more evil would have been to replace this line
with this:
42 replies →
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 :(
57 replies →
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
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?
Unicode lookalikes would be detected by IDEs and other tools.
There would be plausible deniability in a different situation, but this is the same author who implemented the backdoor and several similar changes that disable security features. I don't think the benefit of doubt is deserved here.
14 replies →
> Maybe I'm out of the loop but is intentionality settled here?
Yes. The exploit used a payload, that was stored in the tests directory as a binary compression test case, but which is very clearly a very intentional exploit payload.
All the sneaky stuff was about deploying that payload without it being obvious.
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
6 replies →
This is so vile that even if caught red-handed during PR one could shrug off "oh, my IDE's auto formatting did this".
It seems like Lasse Collin is back on the scene and maybe pissed?
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.
8 replies →
I worry about his home/personal system, this guy seems to have befriended Lasse in order to get the position, is it possible Lasse himself has been compromised?
I'd be nuking everything, possibly even disposing of hardware and setting up a new verifiable online identity on a clean system/hardware.
I feel like my version control system is better about highlighting the changed characters than these solid green or red strings.
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.
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.
1 reply →
It's git, so it can show a vastly better diff, just not from a URL with hardcoded diff settings.
1 reply →
What does the dot do?
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.
9 replies →
Fails the build, so that Landlock support is never enabled.
5 replies →
There 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.
I can't believe that system security is dependent on such a loose chain of correctness. Any number of things could have stopped this.
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 '.'
He introduced a whole new testing framework then slowly put a backdoor in it over 2 years
You are quoting Jia Tan [1]. The malicious actor wrote that comment when deliberately breaking the check in the first place.
Fixing headers or extra tests would not have prevented this, as there is no indication the headers were broken in the first place, and extra tests could have been compromised (or ignored for release tarball) some other way.
[1] https://git.tukaani.org/?p=xz.git;a=commit;h=328c52da8a2bbb8...
Should the better fix then to have been to revert the bad commit with the malicious commit message, rather than just deleting the dot (as was done)?
1 reply →
Tangentially related, but I've had a case where GCC 9.4.0 shipped a broken arm_acle.h header [https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100985] - code which includes the header would always fail to compile.
Since users could be trying to compile it on GCC 9.4.0, and as the functionality dependent on the header isn't critical to the application, if the build detects the header is broken, it just disables the functionality and emits a warning.
Back to xz: If security isn't your top priority, it seems reasonable to just ignore the failure of an optional feature and just move along. Of course, in hindsight, it's easy to point fingers, but without that, it doesn't sound completely unreasonable to me.
As 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.
Well, do we know if that commented code you quoted is accurate?
It's the content of TFA, so flag it if you believe it isn't.
1 reply →
I'm pretty certain the comment isn't accurate, and is just more subterfuge.
... 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.
Geez, his last commit is making security reports worse: https://git.tukaani.org/?p=xz.git;a=commitdiff;h=af071ef7702...
Why is that accepted? Serious question
Presumably because they're one of the two maintainers: https://web.archive.org/web/20240329182607/https://xz.tukaan...
He had unfettered access to xz’s git?
19 replies →
Because nobody’s really paying attention.
“LGTM!”
7 replies →
Where/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???
Presumably 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.
No. OpenSSH doesn't use Landlock, and even if it did, this patch wouldn't affect it.
It's pretty impossible to lock down sshd. The restrictions are inherited by all ancestors. What would the sysadmin say if they find themselves in a restricted shell?
Providing answers to things you have no clue about is worse than providing no answer at all.
Its 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.
Dude is a developer just like the rest of us. We all try to write clever code from time to time, but at other times write sloppy crap. Especially if there's a boss insisting on a tight deadline.
1 reply →
You can use it and it's a good idea, although it's meant to stop exploits rather than deliberate backdoors.
The idea is that the library starts a separate thread to do any complex operation and then waits for it in the API call the user code made. The thread landlocks itself and then begins the operation. If something goes wrong the code is confined.
Of course in this case it doesn't work because the sandbox is controlled by the attacker, so they can just turn it off or make it weaker than it should be. But you can also do things other ways.
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)?
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.
Who says it was just the one library though?
waiting for a new bot to scan everyone's repos to find "." and then spam every repo with false positives
1 reply →
I 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?
Because 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.
Ah, 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.
1 reply →
Unless there are more subtle backdoors that target xz itself beyond the targeting of ssh. Clearly the aim was to be subtle.
This 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.
On 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!)
I doubt it will generalize well. At best its just an arms race.
One of the cooler uses of AI I've seen!
Currently 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.)
I hear this stuff for the first time, can you post some info about that?
Like 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.
This 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.
This 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.
Worse, 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.
2 replies →
So for each optional feature we may need three build options:
1. Force enable 2. Enable if available 3. Force disable
Like,
My 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.
In 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.
That’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.
1 reply →
That's ... how autoconf works?
If you explicitly set the enable-landlock flag, configure will fail when the feature doesn't compile.
A cmake option only has two values: ON or OFF. There is no unset. Because it is a boolean.
See: https://cmake.org/cmake/help/latest/command/option.html
2 replies →
Commit that introduced this:
https://git.tukaani.org/?p=xz.git;a=commit;h=328c52da8a2bbb8...
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?
Autoconf 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.
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.
Would 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.
I 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”.
If 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.
Create 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.
I'd say this MR is a bad approach in general. The headers say what interfaces are known, not what features are available. You should be able to compile with landlock support on a system which doesn't enable it. Same situation as seccomp and others. Your build machine doesn't have to match the capabilities of the target runner.
But yeah, to test it, you can have a mock version of landlock which responds with the error/success as you want, regardless of what the system would normally do. It relies on the test not being sabotaged too though...
Read the code of the check again. It mostly checks that the required SYS_* constants are defined to be able to use the syscalls. You can compile this on a system that does not have landlock enabled in the running kernel, but the libc (which imports the kernel system call interface) has to provide the syscall numbers.
6 replies →
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.
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?
> 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.
Is this by design, or by legacy? I mean, is there a better way to do this? Seems really flawed to me.
29 replies →
That "check_c_source_compiles" function should first test if the provided code snipped is "valid C code in general" and only then check if it compiles in given system.
Optional 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.
Yeah, I wasn't sure what the diff is alluding to here but I assume "mysandbox" could remain undetected (enabled/disabled) for whatever reason.
This may be naive, but it seems to me that a failed compile should generate an error worth worrying about.
It'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.
Now I'd like to have a link to that patch in code used by Apple that also appeared as a typo. Anybody remembers?
You mean the infamous "goto fail"? Yeah that was fun too.
https://www.imperialviolet.org/2014/02/22/applebug.html
Only in CMake, this time. Not in the autotools version.
Does autotools compile only, or try to link? There's no main().
AC_COMPILE_IFELSE indeed only compiles, so there's no need for main().
There's AC_LINK_IFELSE if you want to test linking too.
There is a main()...[0]
[0] https://git.tukaani.org/?p=xz.git;a=blob;f=CMakeLists.txt;h=...
1 reply →
[flagged]