Page MenuHomeFreeBSD

Add compat.linux.setid_allowed knob
ClosedPublic

Authored by kib on Jan 14 2021, 1:54 PM.
Referenced Files
Unknown Object (File)
Thu, Nov 7, 2:13 AM
Unknown Object (File)
Wed, Nov 6, 12:52 AM
Unknown Object (File)
Wed, Nov 6, 12:52 AM
Unknown Object (File)
Tue, Nov 5, 2:00 PM
Unknown Object (File)
Tue, Nov 5, 1:01 PM
Unknown Object (File)
Tue, Oct 29, 10:05 AM
Unknown Object (File)
Sun, Oct 27, 2:39 PM
Unknown Object (File)
Oct 17 2024, 2:39 PM
Subscribers

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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.

Do we have an agreement there?

I think that switching the defaults, if ever, should be done by separate change indeed.

In D28154#680209, @kib wrote:

Do we have an agreement there?

I think that switching the defaults, if ever, should be done by separate change indeed.

if it makes sence to allow setid by default, I have no objection, although it seems strange to me

This revision is now accepted and ready to land.Jun 6 2021, 5:19 PM