Hacker News Reader

Comment by onedognight | Hacker News Reader

> With the right sandboxing techniques, SELinux and mitigations could prevent the attacker from doing anything with root permissions.

Please review this commit[0] where the sandbox detection was “improved”.

[0] https://git.tukaani.org/?p=xz.git;a=commitdiff;h=328c52da8a2...

View on HN

I can't blame anyone who has missed that dot dissimulated at the beginning of the line.

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

I specifically opened this diff to search for a sneaky dot, knowing it’s there, and wasn’t able to find it until I checked the revert patch

Same, I knew a sneaky dot was in the diff but had to ctrl-f the diff to find it.

For people like me whose C knowledge is poor, can you explain why this dot is significant? What does it do in actuality?

As far as I can tell, the check is to see if a certain program compiles, and if so, disable something. The dot makes it so that it always fails to compile and thus always disables that something.

> if a certain program compiles, and if so, disable something.

Tiny correction: [...] enable something.

The idea is: If that certain program does not compile it is because something is not available on the system and therefore needs to be disabled.

That dot undermines that logic. The program fails because of a syntax error caused by the dot and not because something is missing.

It is easy to overlook because that dot is tiny and there are many such tests.

I had a similar problem with unit testing of a library. Expected failures need to be tested as well. As an example imagine writing a matrix inversion library. Then you need to verify that you get something like a division by zero error if you invert the zero matrix. You write a unit test for that and by mistake you insert a syntax error. Then you run the unit test and it fails as expected but not in the correct way.

It's subtle. It fails as expected but it fails because of unexpected wrong causes.

The solution: Check the errors carefully!

> The solution: Check the errors carefully!

The desire for "does this compile on this platform" checks comes from an era where there was pretty much no way to check the error. Somebody runs it on HP-UX with the "HP-UX Ansi C Compiler" they licensed from HP and the error it spits out isn't going to look like anything you recognize.

It's part of a test program used for feature detection (of a sandboxing functionality), and causes a syntax error. That in turn causes the test program to fail to compile, which makes the configure script assume that the sandboxing function is unavailable, and disables support for it.

You are looking at a makefile, not C. The C code is in a string that is being passed to a function called `check_c_source_compiles()`, and this dot makes that code not compile when it should have -- which sets a boolean incorrectly, which presumably makes the build do something it should not do.

Interesting that validating the failure reason of an autotools compile check could be a security mitigation...

This is something that should have unit/integration tests inside the tooling itself, yeah. If your assertion is that X function is called / in the environment X then the function should return Y then that should be a test especially when it’s load-bearing for security.

And tooling is no exception either. You should have tests that your tooling does the things it says on the tin and that things happen when flags are set and things don’t happen when they’re not set, and that the tooling sets the flags in the way you expect.

These aren’t even controversial statements in the JVM world etc. Just C tooling is largely still living in the 70s apart from abortive attempts to build the jenga tower even taller like autotools/autoconf/cmake/etc (incomprehensible, may god have mercy on your build). At least hand written make files are comprehensible tbh.

It's a "does this compile on this platform" test, not a "does this function return what we expect" test.

Unfortunately in the world of autoconf and multiple platforms and compilers, there was no standard way to understand why the compilation failed.

Cmake actually but yes

That one's a separate attack vector, which is seemingly unused in the sshd attack. It only disables sandboxing of the xzdec(2) utility, which is not used in the sshd attack.

Which strongly suggests that they planned and/or executed more backdoors via Jia Tan’s access.

I guess xzdec was supposed to sandbox itself where possible so they disabled the sandbox feature check in the build system so that future payload exploits passed to xzdec wouldn’t have to escape the sandbox in order to do anything useful?

Sneaky.

Yes, but don't forget that there are different kinds of sandboxes. SELinux never needs the cooperation of any program running on the system in order to correctly sandbox things. No change to Xz could ever make SELinux less effective.

But don't forget that xz is also used as part of dpkg for unpacking packages. The whole purpose of dpkg is to update critical system packages. Any SELinux policy that protects from a backdoored dpkg/xz installing a rootkit during the next kernel security update; will also prevent installing real kernel security updates.

The particular way of attack in this OpenSSH backdoor can maybe be prevented; but we've got to realize that the attacker already had full root permissions and there's no way of protecting from that.

SELinux policies are much more subtle than that. You don’t restrict what xz or liblzma can do, you restrict what the whole process can do. That process is either sshd or dpkg, and you can give them completely different access to the system, so that if dpkg tries to launch an interactive shell it fails, while sshd fails if it tries to overwrite a system file such as /bin/login or whatever. Neither would ordinarily do that, but the payload delivered via the back door might attempt it and wouldn’t succeed. And you would get a report stating what had happened, so if you’re paying attention the back door starts to become obvious.

Also I think dpkg switched to Zstd, didn’t it? Or am I misremembering?

But you’re not wrong; ultimately both sshd and dpkg are critical infrastructure. SELinux can prevent them from doing completely wrong things, but obviously it wouldn’t be useful for it to prevent them from doing their jobs. And those jobs are security critical already. SELinux is not a panacea, merely defense in depth.

Oh, that one is interesting, because it only breaks it in cmake.

I wonder if there is anything else cmake related that should be looked at.

Wasn't cmake support originally added to xz to use with Windows and MSVC?

But that's a check for a Linux feature. So the more interesting question would be, what in the Linux world might be building xz-utils with cmake, I guess using ExternalProject_Add or something similar.

Yes this is Linux.

At this time we don't know exactly how much is affected and what originally drew the attention of the attacker(s).

Well, the definition of "improve" depends on one's goals.

Reply on news.ycombinator.com