Page MenuHomeFreeBSD

Add compat.linux.setid_allowed knob
Needs ReviewPublic

Authored by kib on Jan 14 2021, 1:54 PM.

Details

Summary

PR: 21463

Test Plan

not tested

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kib requested review of this revision.Jan 14 2021, 1:54 PM

Use oracle function instead of plain boolean for sv_setid_allow.

I'm not sure this is all that useful, to be honest. What's the usage scenario? Why would one globally disable suid for Linux binaries?

One thing that might be useful is implementing PR_SET_NO_NEW_PRIVS prctl(2). This disables suid (like the patch above) for the calling process (and I'm guessing also for its descendants).

Motivation is provided by the notes in the referenced PR. Briefly, convincing argument for me was that we do not provide environment that would be expected by the linux setuid binaries, so assumptions made during coding of them could be broken by us unknowingly. More, I should add that it is possible that user break them due to our /compat/linux+normal freebsd fs fallback setup.

I do not know what is prctl(2). WRT making the settings more granular, this patch is required pre-requisite for any further progress, because you do need hook into execve(2) to avoid doing setid. This is the main part of the change (and also I did it with a function instead of just bool exactly to ease future improvements, see the patch update history).

I'm not convinced about the environment, tbh. Can you give some specific example of what we don't provide, that could affect security of suid binaries?

Now, regarding prctl(2): think of it as Linux equivalent of procctl(2); in Linux, one can use PR_SET_NO_NEW_PRIVS to set a per-process flag, inherited by children, to do what the sysctl does, except per-process. It would also be useful for native FreeBSD processes, so we'd probably implement it natively first, not just in Linuxulator. The flag is used by various real-world software, so we'll need to implement it anyway - and to me it renders the sysentvec-related parts of this patch redundant.

Also: I wonder if we could start allowing chroot(2) for non-root users, as long as the calling process has the NO_NEW_PRIVS flag set.

Fully support this change and motivation, propose to add a short description to the linux(4) manual about sysctl

Note that I'm not in any way opposed to this change.

Also, there's now a review which shows what I've meant with NO_NEW_PRIVS flag: https://reviews.freebsd.org/D30130. It reuses a part of this patch, but is otherwise orthogonal.

Note that I'm not in any way opposed to this change.

Also, there's now a review which shows what I've meant with NO_NEW_PRIVS flag: https://reviews.freebsd.org/D30130. It reuses a part of this patch, but is otherwise orthogonal.

Could you please test it with Linux binaries?

Rebase.
Add some explanation to man page.

I've tested this patch for both 64, and 32-bit Linux binaries, for both suid and sgid bit, and it works just fine.

One nit, though: I'd strongly prefer the sysctl to be enabled by default, ie to permit suid/sgid by default. Otherwise it will break some pretty common use cases.

Switch to allow by default

I've tested this patch for both 64, and 32-bit Linux binaries, for both suid and sgid bit, and it works just fine.

One nit, though: I'd strongly prefer the sysctl to be enabled by default, ie to permit suid/sgid by default. Otherwise it will break some pretty common use cases.

                                                                         could you olease be more specific here^^^^^^^^^, ie which cases?

All kinds of cases, tbh - from sudo apt update, to Chromium, which (for some reason) depends on a setuid helper.