cmd/snap-seccomp: only compare the bottom 32-bits of the flags arg of copy_file_range

[jhenstridge]

I ran into a puzzling problem with some copy_file_range calls from the Steam snap failing even though it looked like the seccomp profile should have allowed them. While chatting with @bboozzoo on IRC, he pointed me at a forum thread where other people had run into the same problem.

The underlying cause is that the sixth argument to copy_file_range is of type unsigned int, which is a 32-bit value. Arguments to a system call are passed in six 64-bit registers, with 32-bit values occupying the low half of their register. The high half of the register will contain whatever the program stored in it prior to the syscall.

While the kernel implementation of copy_file_range will only read the lower half of the register to get the flags argument, the seccomp code doesn't know this and just passes the six 64-bit registers to the BPF program. If the top half of the register was not cleared, then the arg6 == 0 test will fail. What we really want is to check arg6 & 0xFFFFFFFF == 0.

We've already got code in snap-seccomp to implement this kind of comparison in the code to handle signed 32-bit comparisons, so I was able to fix the filter by adding copy_file_range to the syscallsWithNegArgsMaskHi32 map. And that's what this initial version of the PR does.

I realise that this is not the cleanest option, as not all copy_file_range arguments are 32-bit, and the one we're actually checking is unsigned. So maybe the right option is to extend the snap-seccomp syntax a bit instead.

[mardy]

Wow, thanks for the investigation! As you wrote, applying the 32 bit logic to all arguments can open the door to security issues, if some arguments are 64bits wide. On the other hand, currently the problem is only theoretical, because currently the only filter that snapd applies is on the 6th parameter. As for the other parameters:

• Two are file descriptors, and being dynamic I find it unlikely that we'll ever need to hardcode them in a rule (and in any case, they are 32 bits and signed).
• Two other arguments are pointers, so we'll probably never filter on them anyway.
• The other argument is a size_t for the length. It's hard, but one might imagine a use-case for filtering on this.

As for the signedness, the only valid value for the flags is currently 0, and it will take some time before Linux finds use for 32 flags :-)

In other words, currently this PR should not pose any practical risks; but since in the long run it's not impossible that this code will become invalid for some use cases, I'd recommend adding a comment next to the newly added entry in the list, explaining the limitations.

[alexmurray]

LGTM, I wonder if there is some way we could try and watch for new syscalls of this type when they get added to the kernel/libseccomp to avoid such whackamole situations in the future.

Thanks for digging into this.

[alexmurray]

Ummm I can't seem to remove the needs security review label from my mobile but please consider the security review done 🙂

[jhenstridge]

For what it is worth, I think there are two orthogonal properties for each argument here:

1. is 32-bit or 64-bit? If it is 32-bit, we need to mask the high bits, and the only operation we have is CompareMaskedEqual, so all other operators are unsupported.
2. is it signed or unsigned? For signed negative values, we need to know 32 bs 64 too to get the sign extension correct. It looks like libseccomp generates unsigned greater than/less than op codes, so using them on signed arguments is questionable.'

We've definitely got a few rules that look problematic in the base template:

• nice <=19 is working with a signed 32-bit int argument. So it's not masking the argument, and will reject negative values.
• third argument of setpriority PRIO_PROCESS 0 <=19 is also a signed 32-bit int.

There may be more hiding in the individual interfaces.

[jhenstridge]

Some of this might be better done within libseccomp itself: it looks like BPF has support for 32-bit comparisons (BPF_JMP vs BPF_JMP32), which would allow the 32-bit cases to be handled without masking. It also has signed vs. unsigned comparisons (e.g. BPF_JGT vs BPF_JSGT). That'd basically remove most of the restrictions mentioned in my last comment.