3 ms·

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 lan

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

raimue 3y ago
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.
- viraptor 3y ago
  You're right. I didn't see SYS... symbols being actually used, but they are: https://git.tukaani.org/?p=xz.git;a=blob;f=src/xz/sandbox.c;h=9e30a07a2cbce353456b9475d038af0034624c53;hb=de4337fd89ca7db5feb97b5c40143404f6e22986#l218 https://git.tukaani.org/?p=xz.git;a=blob;f=src/xz/sandbox.c;... This doesn't change my opinion in general - that version should be exposed through a library call and knowing about the specific syscalls shouldn't be needed in xv.
  - Denvercoder9 3y ago
    I see your point, but suggesting adding an additional library dependency while we're discussing a supply chain attack is quite ironic.
    - viraptor 3y ago
      Should've said function call not library call. My bad. Basically if you already have the linux/landlock.h, that should provide everything you need to do without explicit references to SYS...
      - raimue 3y ago
        Now we are running in circles. As you see in the git commit, the compile check was added because the existance of linux/landlock.h alone was not enough to check that the feature can be used. This header defines the data types for the Linux kernel interface, but not how the syscall landlock_create_ruleset(2) will be issued. That is provided by libc either as a separate wrapper function (does not exist in glibc) or the implementation needs to use syscall(SYS_landlock_create_ruleset, ...), with the constant also being provided by libc. That is how it works for all syscalls and you won't be able to change this.
        SAI_Peregrinus 3y ago
        The only source of the claim that the existence of.linux/landlock.h is insufficient is (AFAICT) the malicious git commit. Why trust the comment, written by the attacker, to explain away a malicious change?
        raimue 3y ago
        I already explained above why the existence of linux/landlock.h is not sufficient. Why do you still question it? If you know a bit about system programming and how configure checks work, the change in itself is totally reasonable.

raimue 3y ago

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.

viraptor 3y ago
You're right. I didn't see SYS... symbols being actually used, but they are: https://git.tukaani.org/?p=xz.git;a=blob;f=src/xz/sandbox.c;h=9e30a07a2cbce353456b9475d038af0034624c53;hb=de4337fd89ca7db5feb97b5c40143404f6e22986#l218 https://git.tukaani.org/?p=xz.git;a=blob;f=src/xz/sandbox.c;... This doesn't change my opinion in general - that version should be exposed through a library call and knowing about the specific syscalls shouldn't be needed in xv.
- Denvercoder9 3y ago
  I see your point, but suggesting adding an additional library dependency while we're discussing a supply chain attack is quite ironic.
  - viraptor 3y ago
    Should've said function call not library call. My bad. Basically if you already have the linux/landlock.h, that should provide everything you need to do without explicit references to SYS...
    - raimue 3y ago
      Now we are running in circles. As you see in the git commit, the compile check was added because the existance of linux/landlock.h alone was not enough to check that the feature can be used. This header defines the data types for the Linux kernel interface, but not how the syscall landlock_create_ruleset(2) will be issued. That is provided by libc either as a separate wrapper function (does not exist in glibc) or the implementation needs to use syscall(SYS_landlock_create_ruleset, ...), with the constant also being provided by libc. That is how it works for all syscalls and you won't be able to change this.
      - SAI_Peregrinus 3y ago
        The only source of the claim that the existence of.linux/landlock.h is insufficient is (AFAICT) the malicious git commit. Why trust the comment, written by the attacker, to explain away a malicious change?
        raimue 3y ago
        I already explained above why the existence of linux/landlock.h is not sufficient. Why do you still question it? If you know a bit about system programming and how configure checks work, the change in itself is totally reasonable.