Hi Linus,
This patch series adds a new syscall named trusted_for. It enables user
space to ask the kernel: is this file descriptor's content trusted to be
used for this purpose? The set of usage currently only contains
execution, but other may follow (e.g. configuration, sensitive data).
If the kernel identifies the file descriptor as trustworthy for this
usage, user space should then take this information into account. The
"execution" usage means that the content of the file descriptor is
trusted according to the system policy to be executed by user space,
which means that it interprets the content or (try to) maps it as
executable memory.
A simple system-wide security policy can be set by the system
administrator through a sysctl configuration consistent with the mount
points or the file access rights. The documentation explains the
prerequisites.
It is important to note that this can only enable to extend access
control managed by the kernel. Hence it enables current access control
mechanism to be extended and become a superset of what they can
currently control. Indeed, the security policy could also be delegated
to an LSM, either a MAC system or an integrity system. For instance,
this is required to close a major IMA measurement/appraisal interpreter
integrity gap by bringing the ability to check the use of scripts.
Other uses are expected as well.
For further details, please see the latest cover letter:
https://lore.kernel.org/r/[email protected]
Commit dae71698b6c5 ("printk: Move back proc_dointvec_minmax_sysadmin()
to sysctl.c") was recently added due to the sysctl refactoring.
Commit e674341a90b9 ("selftests/interpreter: fix separate directory
build") will fix some test build cases as explained here:
https://lore.kernel.org/r/[email protected]
Merging this commit without the new KHDR_INCLUDES is not an issue.
The upcoming kselftest pull request is ready:
https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=next
This patch series has been open for review for more than three years and
got a lot of feedbacks (and bikeshedding) which were all considered.
Since I heard no objection, please consider to pull this code for
v5.18-rc1 . These five patches have been successfully tested in the
latest linux-next releases for several weeks.
Regards,
Mickaël
--
The following changes since commit dcb85f85fa6f142aae1fe86f399d4503d49f2b60:
gcc-plugins/stackleak: Use noinstr in favor of notrace (2022-02-03 17:02:21 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git tags/trusted-for-v18
for you to fetch changes up to e674341a90b95c3458d684ae25e6891afc3e03ad:
selftests/interpreter: fix separate directory build (2022-03-04 10:56:25 +0100)
----------------------------------------------------------------
Add the trusted_for system call (v18)
The final goal of this patch series is to enable the kernel to be a
global policy manager by entrusting processes with access control at
their level. To reach this goal, two complementary parts are required:
* user space needs to be able to know if it can trust some file
descriptor content for a specific usage;
* and the kernel needs to make available some part of the policy
configured by the system administrator.
In a nutshell, this is a required building block to control script
execution.
For further details see the latest cover letter:
https://lore.kernel.org/r/[email protected]
----------------------------------------------------------------
Mickaël Salaün (4):
printk: Move back proc_dointvec_minmax_sysadmin() to sysctl.c
fs: Add trusted_for(2) syscall implementation and related sysctl
arch: Wire up trusted_for(2)
selftest/interpreter: Add tests for trusted_for(2) policies
Muhammad Usama Anjum (1):
selftests/interpreter: fix separate directory build
Documentation/admin-guide/sysctl/fs.rst | 50 +++
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 +
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
fs/open.c | 133 ++++++++
fs/proc/proc_sysctl.c | 2 +-
include/linux/syscalls.h | 1 +
include/linux/sysctl.h | 3 +
include/uapi/asm-generic/unistd.h | 5 +-
include/uapi/linux/trusted-for.h | 18 +
kernel/printk/sysctl.c | 9 -
kernel/sysctl.c | 9 +
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/interpreter/.gitignore | 2 +
tools/testing/selftests/interpreter/Makefile | 21 ++
tools/testing/selftests/interpreter/config | 1 +
.../selftests/interpreter/trust_policy_test.c | 362 +++++++++++++++++++++
32 files changed, 625 insertions(+), 12 deletions(-)
create mode 100644 include/uapi/linux/trusted-for.h
create mode 100644 tools/testing/selftests/interpreter/.gitignore
create mode 100644 tools/testing/selftests/interpreter/Makefile
create mode 100644 tools/testing/selftests/interpreter/config
create mode 100644 tools/testing/selftests/interpreter/trust_policy_test.c
On 21/03/2022 18:38, Luis Chamberlain wrote:
> On Mon, Mar 21, 2022 at 05:15:57PM +0100, Mickaël Salaün wrote:
>> Since I heard no objection, please consider to pull this code for
>> v5.18-rc1 . These five patches have been successfully tested in the
>> latest linux-next releases for several weeks.
>
>> kernel/sysctl.c | 9 +
>
> Please don't add more sysctls there. We're slowly trying to move
> all these to their respective places so this does not become a larger
> kitchen sink mess.
It is not a new sysctl but proc_dointvec_minmax_sysadmin(). This helper
is shared between printk and fs subsystems.
On Mon, Mar 21, 2022 at 05:15:57PM +0100, Micka?l Sala?n wrote:
> Since I heard no objection, please consider to pull this code for
> v5.18-rc1 . These five patches have been successfully tested in the
> latest linux-next releases for several weeks.
> kernel/sysctl.c | 9 +
Please don't add more sysctls there. We're slowly trying to move
all these to their respective places so this does not become a larger
kitchen sink mess.
Luis
On Mon, Mar 21, 2022 at 07:05:42PM +0100, Micka?l Sala?n wrote:
>
> On 21/03/2022 18:38, Luis Chamberlain wrote:
> > On Mon, Mar 21, 2022 at 05:15:57PM +0100, Micka?l Sala?n wrote:
> > > Since I heard no objection, please consider to pull this code for
> > > v5.18-rc1 . These five patches have been successfully tested in the
> > > latest linux-next releases for several weeks.
> >
> > > kernel/sysctl.c | 9 +
> >
> > Please don't add more sysctls there. We're slowly trying to move
> > all these to their respective places so this does not become a larger
> > kitchen sink mess.
>
> It is not a new sysctl but proc_dointvec_minmax_sysadmin(). This helper is
> shared between printk and fs subsystems.
That should be good then, thanks!
Luis
Hi,
What is the status of this pull request? Do you need something more?
Regards,
Mickaël
On 21/03/2022 17:15, Mickaël Salaün wrote:
> Hi Linus,
>
> This patch series adds a new syscall named trusted_for. It enables user
> space to ask the kernel: is this file descriptor's content trusted to be
> used for this purpose? The set of usage currently only contains
> execution, but other may follow (e.g. configuration, sensitive data).
> If the kernel identifies the file descriptor as trustworthy for this
> usage, user space should then take this information into account. The
> "execution" usage means that the content of the file descriptor is
> trusted according to the system policy to be executed by user space,
> which means that it interprets the content or (try to) maps it as
> executable memory.
>
> A simple system-wide security policy can be set by the system
> administrator through a sysctl configuration consistent with the mount
> points or the file access rights. The documentation explains the
> prerequisites.
>
> It is important to note that this can only enable to extend access
> control managed by the kernel. Hence it enables current access control
> mechanism to be extended and become a superset of what they can
> currently control. Indeed, the security policy could also be delegated
> to an LSM, either a MAC system or an integrity system. For instance,
> this is required to close a major IMA measurement/appraisal interpreter
> integrity gap by bringing the ability to check the use of scripts.
> Other uses are expected as well.
>
> For further details, please see the latest cover letter:
> https://lore.kernel.org/r/[email protected]
>
> Commit dae71698b6c5 ("printk: Move back proc_dointvec_minmax_sysadmin()
> to sysctl.c") was recently added due to the sysctl refactoring.
>
> Commit e674341a90b9 ("selftests/interpreter: fix separate directory
> build") will fix some test build cases as explained here:
> https://lore.kernel.org/r/[email protected]
> Merging this commit without the new KHDR_INCLUDES is not an issue.
> The upcoming kselftest pull request is ready:
> https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=next
>
> This patch series has been open for review for more than three years and
> got a lot of feedbacks (and bikeshedding) which were all considered.
> Since I heard no objection, please consider to pull this code for
> v5.18-rc1 . These five patches have been successfully tested in the
> latest linux-next releases for several weeks.
>
> Regards,
> Mickaël
>
> --
> The following changes since commit dcb85f85fa6f142aae1fe86f399d4503d49f2b60:
>
> gcc-plugins/stackleak: Use noinstr in favor of notrace (2022-02-03 17:02:21 -0800)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git tags/trusted-for-v18
>
> for you to fetch changes up to e674341a90b95c3458d684ae25e6891afc3e03ad:
>
> selftests/interpreter: fix separate directory build (2022-03-04 10:56:25 +0100)
>
> ----------------------------------------------------------------
> Add the trusted_for system call (v18)
>
> The final goal of this patch series is to enable the kernel to be a
> global policy manager by entrusting processes with access control at
> their level. To reach this goal, two complementary parts are required:
> * user space needs to be able to know if it can trust some file
> descriptor content for a specific usage;
> * and the kernel needs to make available some part of the policy
> configured by the system administrator.
>
> In a nutshell, this is a required building block to control script
> execution.
>
> For further details see the latest cover letter:
> https://lore.kernel.org/r/[email protected]
>
> ----------------------------------------------------------------
> Mickaël Salaün (4):
> printk: Move back proc_dointvec_minmax_sysadmin() to sysctl.c
> fs: Add trusted_for(2) syscall implementation and related sysctl
> arch: Wire up trusted_for(2)
> selftest/interpreter: Add tests for trusted_for(2) policies
>
> Muhammad Usama Anjum (1):
> selftests/interpreter: fix separate directory build
>
> Documentation/admin-guide/sysctl/fs.rst | 50 +++
> arch/alpha/kernel/syscalls/syscall.tbl | 1 +
> arch/arm/tools/syscall.tbl | 1 +
> arch/arm64/include/asm/unistd.h | 2 +-
> arch/arm64/include/asm/unistd32.h | 2 +
> arch/ia64/kernel/syscalls/syscall.tbl | 1 +
> arch/m68k/kernel/syscalls/syscall.tbl | 1 +
> arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
> arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
> arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
> arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
> arch/parisc/kernel/syscalls/syscall.tbl | 1 +
> arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
> arch/s390/kernel/syscalls/syscall.tbl | 1 +
> arch/sh/kernel/syscalls/syscall.tbl | 1 +
> arch/sparc/kernel/syscalls/syscall.tbl | 1 +
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
> fs/open.c | 133 ++++++++
> fs/proc/proc_sysctl.c | 2 +-
> include/linux/syscalls.h | 1 +
> include/linux/sysctl.h | 3 +
> include/uapi/asm-generic/unistd.h | 5 +-
> include/uapi/linux/trusted-for.h | 18 +
> kernel/printk/sysctl.c | 9 -
> kernel/sysctl.c | 9 +
> tools/testing/selftests/Makefile | 1 +
> tools/testing/selftests/interpreter/.gitignore | 2 +
> tools/testing/selftests/interpreter/Makefile | 21 ++
> tools/testing/selftests/interpreter/config | 1 +
> .../selftests/interpreter/trust_policy_test.c | 362 +++++++++++++++++++++
> 32 files changed, 625 insertions(+), 12 deletions(-)
> create mode 100644 include/uapi/linux/trusted-for.h
> create mode 100644 tools/testing/selftests/interpreter/.gitignore
> create mode 100644 tools/testing/selftests/interpreter/Makefile
> create mode 100644 tools/testing/selftests/interpreter/config
> create mode 100644 tools/testing/selftests/interpreter/trust_policy_test.c
On Mon, Apr 04, 2022 at 02:28:19PM -0700, Linus Torvalds wrote:
> Now, what I *think* you mean is
>
> (1) user-space executable loaders want to be able to test the *same*
> policy as the kernel does for execve()
Right. The script interpreter wants to ask "if this file were actually
an ELF going through execve(), would the kernel allow it?"
> (2) access(path, EXECVE_OK) will do the same permission checks as
> "execve()" would do for that path
Maybe. I defer to Micka?l here, but my instinct is to avoid creating an
API that can be accidentally misused. I'd like this to be fd-only based,
since that removes path name races. (e.g. trusted_for() required an fd.)
> (3) if you already have the fd open, use "faccess(fd, NULL,
> F_OK_TO_EXECUTE, AT_EMPTY_PATH)"
Yes, specifically faccessat2(). (And continuing the race thought above,
yes, there could still be races if the content of the file could be
changed, but that case is less problematic under real-world conditions.)
> (4) maybe we want to add a flag for the "euid vs real uid", and that
> would be in the "flags" field, since that changes the actual *lookup*
> semantics
>
> Note that that (4) is something that some normal user space has wanted
> in the past too (GNU libcs has a "eaccess()" thing for "effective uid
> access").
I think this already exists as AT_EACCESS? It was added with
faccessat2() itself, if I'm reading the history correctly.
And I just need to say that the thought of setuid script interpreters
still makes me sad. :)
> - I really want the exact semantics very clearly defined. I think
> it's ok to say "exact same security check as for 'execve()'", but even
> then we need to have that discussion about
>
> (a) "what about suid bits that user space cannot react to"
What do you mean here? Do you mean setid bits on the file itself?
> (b) that whole "effective vs real" discussion
I think this is handled with AT_EACCESS?
--
Kees Cook
On Mon, Mar 21, 2022 at 05:15:57PM +0100, Micka?l Sala?n wrote:
> [...]
> For further details, please see the latest cover letter:
> https://lore.kernel.org/r/[email protected]
>
> Commit dae71698b6c5 ("printk: Move back proc_dointvec_minmax_sysadmin()
> to sysctl.c") was recently added due to the sysctl refactoring.
>
> Commit e674341a90b9 ("selftests/interpreter: fix separate directory
> build") will fix some test build cases as explained here:
> https://lore.kernel.org/r/[email protected]
> Merging this commit without the new KHDR_INCLUDES is not an issue.
> The upcoming kselftest pull request is ready:
> https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=next
>
> This patch series has been open for review for more than three years and
> got a lot of feedbacks (and bikeshedding) which were all considered.
> Since I heard no objection, please consider to pull this code for
> v5.18-rc1 . These five patches have been successfully tested in the
> latest linux-next releases for several weeks.
Hi Linus,
It looks like this didn't get pulled for -rc1 even though it was sent
during the merge window and has been in -next for a while. It would be
really nice to get this landed since userspace can't make any forward
progress without the kernel support.
Was there some issue blocking this from being merged? All the feedback I
can find on prior versions was addressed.
-Kees
--
Kees Cook
On Mon, Apr 4, 2022 at 11:40 AM Kees Cook <[email protected]> wrote:
>
> It looks like this didn't get pulled for -rc1 even though it was sent
> during the merge window and has been in -next for a while. It would be
> really nice to get this landed since userspace can't make any forward
> progress without the kernel support.
Honestly, I need a *lot* better reasoning for random new non-standard
system calls than this had.
And this kind of "completely random interface with no semantics except
for random 'future flags'" I will not pull even *with* good reasoning.
I already told Mickaël in private that I wouldn't pull this.
Honestly, we have a *horrible* history with non-standard system calls,
and that's been true even for well-designed stuff that actually
matters, that people asked for.
Something like this, which adds one very special system call and
where the whole thing is designed for "let's add something random
later because we don't even know what we want" is right out.
What the system call seems to actually *want* is basically a new flag
to access() (and faccessat()). One that is very close to what X_OK
already is.
But that wasn't how it was sold.
So no. No way will this ever get merged, and whoever came up with that
disgusting "trusted_for()" (for WHAT? WHO TRUSTS? WHY?) should look
themselves in the mirror.
If you add a new X_OK variant to access(), maybe that could fly.
Linus
On 04/04/2022 20:47, Linus Torvalds wrote:
> On Mon, Apr 4, 2022 at 11:40 AM Kees Cook <[email protected]> wrote:
>>
>> It looks like this didn't get pulled for -rc1 even though it was sent
>> during the merge window and has been in -next for a while. It would be
>> really nice to get this landed since userspace can't make any forward
>> progress without the kernel support.
>
> Honestly, I need a *lot* better reasoning for random new non-standard
> system calls than this had.
>
> And this kind of "completely random interface with no semantics except
> for random 'future flags'" I will not pull even *with* good reasoning.
I think the semantic is well defined:
"This new syscall enables user space to ask the kernel: is this file
descriptor's content trusted to be used for this purpose?"
See the trusted_for_policy sysctl documentation:
https://lore.kernel.org/all/[email protected]/
There is currently only one defined and implemented purpose: execution
(or script interpretation). There is room for other flags because it is
a good practice to do so, and other purposes were proposed.
>
> I already told Mickaël in private that I wouldn't pull this.
>
> Honestly, we have a *horrible* history with non-standard system calls,
> and that's been true even for well-designed stuff that actually
> matters, that people asked for.
>
> Something like this, which adds one very special system call and
> where the whole thing is designed for "let's add something random
> later because we don't even know what we want" is right out.
>
> What the system call seems to actually *want* is basically a new flag
> to access() (and faccessat()). One that is very close to what X_OK
> already is.
I agree.
>
> But that wasn't how it was sold.
>
> So no. No way will this ever get merged, and whoever came up with that
> disgusting "trusted_for()" (for WHAT? WHO TRUSTS? WHY?) should look
> themselves in the mirror.
Well, naming is difficult, but I'm open to suggestion. :)
As explained in the description, the WHAT is the file descriptor
content, the WHO TRUSTS is the system security policy (e.g. the mount
point options) and the WHY is defined by the usage flag
(TRUSTED_FOR_EXECUTION).
This translates to: is this file descriptor's content trusted to be used
for this specified purpose/usage?
>
> If you add a new X_OK variant to access(), maybe that could fly.
As answered in private, that was the approach I took for one of the
early versions but a dedicated syscall was requested by Al Viro:
https://lore.kernel.org/r/[email protected]
The main reason behind this request was that it doesn't have the exact
same semantic as faccessat(2). The changes for this syscall are
documented here:
https://lore.kernel.org/all/[email protected]/
The whole history is linked in the cover letter:
https://lore.kernel.org/all/[email protected]/
This initial proposal was using a new faccessat2(2) flag:
AT_INTERPRETED, see
https://lore.kernel.org/all/[email protected]/
What do you think about that? I'm happy to get back to this version if
everyone is OK with it.
On Mon, Apr 4, 2022 at 1:29 PM Mickaël Salaün <[email protected]> wrote:
>
> This initial proposal was using a new faccessat2(2) flag:
> AT_INTERPRETED, see
> https://lore.kernel.org/all/[email protected]/
> What do you think about that? I'm happy to get back to this version if
> everyone is OK with it.
I'm certainly happi_er_ with that, but I find that particular patch
odd for other reasons.
In no particular order:
- what's with the insane non-C syntax? Double parentheses have no
actual meaning in C:
if ((flags & AT_INTERPRETED)) {
if ((mode & MAY_EXEC)) {
so I don't understand why you'd use that strance thing.
- why is this an AT_INTERPRETED flag? I don't understand the name, I
don't understand the semantics.
Why would that flag have the same value as AT_SYMLINK_FOLLOW?
Why isn't this just a new _mode_ bit, which is what I feel is
sensible? We only use three bits (with no bits set meaning
"existence"), so we have *tons* of bits left in that namespace, and it
would make much more sense to me to have
#define EXECVE_OK 8
which is the same as the "group exec" bit, so it actually makes
some kind of sense too.
- related to that "I don't understand the semantics", the
"documentation" for that thing doesn't make sense either:
+ The
+ main usage is for script
+ interpreters to enforce a policy
+ consistent with the kernel's one
+ (through sysctl configuration or LSM
+ policy). */
Now, what I *think* you mean is
(1) user-space executable loaders want to be able to test the *same*
policy as the kernel does for execve()
(2) access(path, EXECVE_OK) will do the same permission checks as
"execve()" would do for that path
(3) if you already have the fd open, use "faccess(fd, NULL,
F_OK_TO_EXECUTE, AT_EMPTY_PATH)"
(4) maybe we want to add a flag for the "euid vs real uid", and that
would be in the "flags" field, since that changes the actual *lookup*
semantics
Note that that (4) is something that some normal user space has wanted
in the past too (GNU libcs has a "eaccess()" thing for "effective uid
access").
And yes, we still need to talk details:
- no backwards compatibility issues, because we've happily always
checked 'mode' for being valid, so old kernels will always return
-EINVAL.
- POSIX namespace issues for EXECVE_OK means that the name probably
needs some thinking (maybe we just need to call it __ACCESS_OK_EXECVE
internally or something - the kernel actually doesn't even export the
existing [FRWX]_OK values, because we "know" they map tho the usual
"owner RWX" bits, with F being "no bit set")
- I really want the exact semantics very clearly defined. I think
it's ok to say "exact same security check as for 'execve()'", but even
then we need to have that discussion about
(a) "what about suid bits that user space cannot react to"
(b) that whole "effective vs real" discussion
So to recap: I'm very much ok with some access() extension, but I
think even that very much needs clarification, and the existing patch
is just odd in many many ways.
Linus
On Mon, Apr 4, 2022 at 2:28 PM Linus Torvalds
<[email protected]> wrote:
>
> (4) maybe we want to add a flag for the "euid vs real uid", and that
> would be in the "flags" field, since that changes the actual *lookup*
> semantics
Duh. We already did that long ago, and it's there as AT_EACCESS.
I should have just looked at the code more closely.
But that "you didn't even check what we already do, Linus" thing just
makes it even more obvious that all of this makes perfect sense in the
confines of access() already, and a new "check _these_ protections"
should just be a new mode bit.
Linus
On Mon, Apr 4, 2022 at 3:25 PM Kees Cook <[email protected]> wrote:
>
> Maybe. I defer to Mickaël here, but my instinct is to avoid creating an
> API that can be accidentally misused. I'd like this to be fd-only based,
> since that removes path name races. (e.g. trusted_for() required an fd.)
Some people want pathnames. Think things like just the PATH thing just
to find the right executable in the first place.
For things like that, races don't matter, because you're just trying
to find the right path to begin with.
> I think this already exists as AT_EACCESS? It was added with
> faccessat2() itself, if I'm reading the history correctly.
Yeah, I noticed myself, I just hadn't looked (and I don't do enough
user-space programming to be aware of if that way).
> > (a) "what about suid bits that user space cannot react to"
>
> What do you mean here? Do you mean setid bits on the file itself?
Right.
Maybe we don't care.
Maybe we do.
Is the user-space loader going to honor them? Is it going to ignore
them? I don't know. And it actually interacts with things like
'nosuid', which the kernel does know about, and user space has a hard
time figuring out.
So if the point is "give me an interface so that I can do the same
thing a kernel execve() loader would do", then those sgid/suid bits
actually may be exactly the kind of thing that user space wants the
kernel to react to - should it ignore them, or should it do something
special when it sees that they are set?
I'm not saying that they *should* be something we care about. All I'm
saying is that I want that *discussion* to happen.
Linus
On 05/04/2022 16:54, Theodore Ts'o wrote:
> On Mon, Apr 04, 2022 at 10:30:13PM +0200, Mickaël Salaün wrote:
>>> If you add a new X_OK variant to access(), maybe that could fly.
>>
>> As answered in private, that was the approach I took for one of the early
>> versions but a dedicated syscall was requested by Al Viro:
>> https://lore.kernel.org/r/[email protected]
>> The main reason behind this request was that it doesn't have the exact same
>> semantic as faccessat(2). The changes for this syscall are documented here:
>> https://lore.kernel.org/all/[email protected]/
>> The whole history is linked in the cover letter:
>> https://lore.kernel.org/all/[email protected]/
>
> As a suggestion, something that can be helpful for something which has
> been as heavily bike-sheded as this concept might be to write a
> "legislative history", or perhaps, a "bike shed history".
>
> And not just with links to mailing list discussions, but a short
> summary of why, for example, we moved from the open flag O_MAYEXEC to
> the faccessat(2) approach. I looked, but I couldn't find the
> reasoning while diving into the mail archives. And there was some
> kind of request for some new functionality for IMA and other LSM's
> that I couldn't follow that is why the new AT_INTERETED flag, but at
> this point my time quantuum for mailing list archeology most
> definitely expired. :-)
>
> It might be that when all of this is laid out, we can either revisit
> prior design decisions as "that bike-shed request to support this
> corner case was unreasonable", or "oh, OK, this is why we need as
> fully general a solution as this".
>
> Also, some of examples of potential future use cases such as "magic
> links" that were linked in the cover letter, it's not clear to me
> actually make sense in the context of a "trusted for" system call
> (although might make more sense in the context of an open flag). So
> revisiting some of those other cases to see whether they actually
> *could* be implemented as new "TRUSTED_FOR" flags might be
> instructive.
>
> Personally, I'm a bit skeptical about the prospct of additional use
> cases, since trusted_for(2) is essentially a mother_should_I(2)
That would be an interesting syscall name. ;)
> request where userspace is asking the kernel whether they should go
> ahead and do some particular policy thing. And it's not clear to me
> how many of these policy questions exist where (a) the kernel is in
> the past position to answer that question, and (b) there isn't some
> additional information that the kernel doesn't have that might be
> needed to answer that question.
Script execution is definitely the main use case and the semantic is
already known by the kernel.
>
> For example, "Mother should I use that private key file" might require
> information about whether the SRE is currently on pager duty or not,
> at least for some policies, and the kernel isn't going to have that
> information.
>
> Other examples of TRUSTED_FOR flags that really make sense and would
> be useful might help alleviate my skepticsm. And the "bike shed
> history" would help with my question about why some folks didn't like
> the original O_MAYEXEC flag?
Thanks, I'll do some that.
>
> Cheers,
>
> - Ted
On 05/04/2022 00:25, Kees Cook wrote:
> On Mon, Apr 04, 2022 at 02:28:19PM -0700, Linus Torvalds wrote:
>> Now, what I *think* you mean is
>>
>> (1) user-space executable loaders want to be able to test the *same*
>> policy as the kernel does for execve()
>
> Right. The script interpreter wants to ask "if this file were actually
> an ELF going through execve(), would the kernel allow it?"
The current behavior was a bit more flexible thanks to the sysctl. It
was either the mount exec option check, the file perm check or both. The
rationale is to let sysadmins adapt their system to existing
applications/interpreters without breaking. Only basing the check on
mount exec and file perm could be an issue in the short term, but I
guess it would deter inconsistencies in existing systems… I'm not sure
it is a wise move because if no interpreter want to use this check it
would then be useless. See commit message in
https://lore.kernel.org/all/[email protected]/ and
the trusted_for_policy sysctl documentation:
"Even without enforced security policy, user space interpreters can use
this syscall to try as much as possible to enforce the system policy at
their level, knowing that it will not break anything on running systems
which do not care about this feature. However, on systems which want
this feature enforced, there will be knowledgeable people (i.e. system
administrator who configured fs.trusted_for_policy deliberately) to
manage it. [...]"
>
>> (2) access(path, EXECVE_OK) will do the same permission checks as
>> "execve()" would do for that path
>
> Maybe. I defer to Mickaël here, but my instinct is to avoid creating an
> API that can be accidentally misused. I'd like this to be fd-only based,
> since that removes path name races. (e.g. trusted_for() required an fd.)
The fd-based approach is definitely better from a security point of view
but there is indeed a use case for pathnames. I guess we could highlight
this point in the documentation.
>
>> (3) if you already have the fd open, use "faccess(fd, NULL,
>> F_OK_TO_EXECUTE, AT_EMPTY_PATH)"
>
> Yes, specifically faccessat2(). (And continuing the race thought above,
> yes, there could still be races if the content of the file could be
> changed, but that case is less problematic under real-world conditions.)
I'm not worried about changes in the file once it is opened. This could
be an issue but not in the kernel (e.g. flaky update system).
[...]
On 04/04/2022 23:28, Linus Torvalds wrote:
> On Mon, Apr 4, 2022 at 1:29 PM Mickaël Salaün <[email protected]> wrote:
>>
>> This initial proposal was using a new faccessat2(2) flag:
>> AT_INTERPRETED, see
>> https://lore.kernel.org/all/[email protected]/
>> What do you think about that? I'm happy to get back to this version if
>> everyone is OK with it.
>
> I'm certainly happi_er_ with that, but I find that particular patch
> odd for other reasons.
>
> In no particular order:
>
> - what's with the insane non-C syntax? Double parentheses have no
> actual meaning in C:
>
> if ((flags & AT_INTERPRETED)) {
> if ((mode & MAY_EXEC)) {
>
> so I don't understand why you'd use that strance thing.
I guess it comes from a previous version that ANDed two booleans.
>
> - why is this an AT_INTERPRETED flag? I don't understand the name, I
> don't understand the semantics.
I wasn't sure it was a good idea to add another mode bit, so I ended up
using a flag to not break compatibility of other mode checks but extend
the semantic to interpreted scripts. But I agree that a new mode makes
sense.
>
> Why would that flag have the same value as AT_SYMLINK_FOLLOW?
It was a bug.
>
> Why isn't this just a new _mode_ bit, which is what I feel is
> sensible? We only use three bits (with no bits set meaning
> "existence"), so we have *tons* of bits left in that namespace, and it
> would make much more sense to me to have
>
> #define EXECVE_OK 8
>
> which is the same as the "group exec" bit, so it actually makes
> some kind of sense too.
Looks fine to me. The "EXECVE_" prefix is a bit weird but it will not be
defined in the kernel like X_OK and others anyway, and as you said, it
matches S_IXGRP.
>
> - related to that "I don't understand the semantics", the
> "documentation" for that thing doesn't make sense either:
>
> + The
> + main usage is for script
> + interpreters to enforce a policy
> + consistent with the kernel's one
> + (through sysctl configuration or LSM
> + policy). */
I'll synchronize the documentation with a next series.
>
> Now, what I *think* you mean is
See a following email in reply to Kees.
[...]
>
> And yes, we still need to talk details:
>
> - no backwards compatibility issues, because we've happily always
> checked 'mode' for being valid, so old kernels will always return
> -EINVAL.
>
> - POSIX namespace issues for EXECVE_OK means that the name probably
> needs some thinking (maybe we just need to call it __ACCESS_OK_EXECVE
> internally or something - the kernel actually doesn't even export the
> existing [FRWX]_OK values, because we "know" they map tho the usual
> "owner RWX" bits, with F being "no bit set")
Right, I cannot find a better name for now.
See a following email in reply to Kees.
[...]
>
> So to recap: I'm very much ok with some access() extension, but I
> think even that very much needs clarification, and the existing patch
> is just odd in many many ways.
This v8 was kind of an early version, I'll update everything. Thanks!
On 05/04/2022 01:26, Linus Torvalds wrote:
> On Mon, Apr 4, 2022 at 3:25 PM Kees Cook <[email protected]> wrote:
[...]
>
>> I think this already exists as AT_EACCESS? It was added with
>> faccessat2() itself, if I'm reading the history correctly.
>
> Yeah, I noticed myself, I just hadn't looked (and I don't do enough
> user-space programming to be aware of if that way).
I think AT_EACCESS should be usable with the new EXECVE_OK too.
>
>>> (a) "what about suid bits that user space cannot react to"
>>
>> What do you mean here? Do you mean setid bits on the file itself?
>
> Right.
>
> Maybe we don't care.
I think we don't. I think the only corner case that could be different
is for files that are executable, SUID and non-readable. In this case it
wouldn't matter because userspace could not read the file, which is
required for interpretation/execution. Anyway, S[GU]ID bits in scripts
are just ignored by execve and we want to follow the same semantic.
>
> Maybe we do.
>
> Is the user-space loader going to honor them? Is it going to ignore
> them? I don't know. And it actually interacts with things like
> 'nosuid', which the kernel does know about, and user space has a hard
> time figuring out.
>
> So if the point is "give me an interface so that I can do the same
> thing a kernel execve() loader would do", then those sgid/suid bits
> actually may be exactly the kind of thing that user space wants the
> kernel to react to - should it ignore them, or should it do something
> special when it sees that they are set?
>
> I'm not saying that they *should* be something we care about. All I'm
> saying is that I want that *discussion* to happen.
>
> Linus
On Mon, Apr 04, 2022 at 10:30:13PM +0200, Micka?l Sala?n wrote:
> > If you add a new X_OK variant to access(), maybe that could fly.
>
> As answered in private, that was the approach I took for one of the early
> versions but a dedicated syscall was requested by Al Viro:
> https://lore.kernel.org/r/[email protected]
> The main reason behind this request was that it doesn't have the exact same
> semantic as faccessat(2). The changes for this syscall are documented here:
> https://lore.kernel.org/all/[email protected]/
> The whole history is linked in the cover letter:
> https://lore.kernel.org/all/[email protected]/
As a suggestion, something that can be helpful for something which has
been as heavily bike-sheded as this concept might be to write a
"legislative history", or perhaps, a "bike shed history".
And not just with links to mailing list discussions, but a short
summary of why, for example, we moved from the open flag O_MAYEXEC to
the faccessat(2) approach. I looked, but I couldn't find the
reasoning while diving into the mail archives. And there was some
kind of request for some new functionality for IMA and other LSM's
that I couldn't follow that is why the new AT_INTERETED flag, but at
this point my time quantuum for mailing list archeology most
definitely expired. :-)
It might be that when all of this is laid out, we can either revisit
prior design decisions as "that bike-shed request to support this
corner case was unreasonable", or "oh, OK, this is why we need as
fully general a solution as this".
Also, some of examples of potential future use cases such as "magic
links" that were linked in the cover letter, it's not clear to me
actually make sense in the context of a "trusted for" system call
(although might make more sense in the context of an open flag). So
revisiting some of those other cases to see whether they actually
*could* be implemented as new "TRUSTED_FOR" flags might be
instructive.
Personally, I'm a bit skeptical about the prospct of additional use
cases, since trusted_for(2) is essentially a mother_should_I(2)
request where userspace is asking the kernel whether they should go
ahead and do some particular policy thing. And it's not clear to me
how many of these policy questions exist where (a) the kernel is in
the past position to answer that question, and (b) there isn't some
additional information that the kernel doesn't have that might be
needed to answer that question.
For example, "Mother should I use that private key file" might require
information about whether the SRE is currently on pager duty or not,
at least for some policies, and the kernel isn't going to have that
information.
Other examples of TRUSTED_FOR flags that really make sense and would
be useful might help alleviate my skepticsm. And the "bike shed
history" would help with my question about why some folks didn't like
the original O_MAYEXEC flag?
Cheers,
- Ted
On Mon, Apr 04, 2022 at 04:26:44PM -0700, Linus Torvalds wrote:
> > > (a) "what about suid bits that user space cannot react to"
> >
> > What do you mean here? Do you mean setid bits on the file itself?
>
> Right.
>
> Maybe we don't care.
>
> Maybe we do.
>
> Is the user-space loader going to honor them? Is it going to ignore
> them? I don't know. And it actually interacts with things like
> 'nosuid', which the kernel does know about, and user space has a hard
> time figuring out.
So there *used* to be suidperl which was a setuid version of perl with
some extra security checks. (See [1] for more details.) The suidperl
binary would be used by #!/usr/bin/perl so it could honor setuid bits
on perl scripts, but it was deprecated in Perl 5.8 and removed in Perl
5.12 in 2010[2].
[1] https://mattmccutchen.net/suidperl.html
[2] https://metacpan.org/release/SHAY/perl-5.20.2/view/pod/perl5120delta.pod#Deprecations
So it's possible that the user-space loader might try to honor them,
and if there was such an example "in the field", it might be nice if
there was a way for the kernel to advise userspace about the nosuid.
But I'm not aware of any other shell script interpreter that tried do
what perl did with suidperl.
> So if the point is "give me an interface so that I can do the same
> thing a kernel execve() loader would do", then those sgid/suid bits
> actually may be exactly the kind of thing that user space wants the
> kernel to react to - should it ignore them, or should it do something
> special when it sees that they are set?
>
> I'm not saying that they *should* be something we care about. All I'm
> saying is that I want that *discussion* to happen.
I'm not convinced we should. I suppose *if* the shell script was
suid, *and* the file system was mounted nosuid, then the check could
return false, and that would be mostly harmless even if the script
interpreter didn't support setuid. But it's extra complexity, and in
theory it could break a setuid script, where the setuid bit was
previously a no-op, and it now might cause a problem for that user.
- Ted
On Tue, Apr 5, 2022 at 9:08 AM Mickaël Salaün <[email protected]> wrote:
>
> I think we don't. I think the only corner case that could be different
> is for files that are executable, SUID and non-readable. In this case it
> wouldn't matter because userspace could not read the file, which is
> required for interpretation/execution. Anyway, S[GU]ID bits in scripts
> are just ignored by execve and we want to follow the same semantic.
So I just want to bring up the possibility that somebody wants to just
implement execve() in user space for some reason - not just "script
interpreter".
It's *doable*.
Don't ask me if it's sane or useful, but people have done insane
things before. Things like "emulate other operating systems in user
space" etc
Such a user can trivially see the suid/sgid bit on the file (just do
fstat() on it), but wouldn't necessarily see if that file is then in a
mount that is mounted nosuid.
Now, I think the right thing to do is to just say "we don't support
it", but I do think it should perhaps be mentioned somewhere
explicitly.
Particularly since I can well imagine that a security policy might
have some "no, I don't allow suid exec" and return an actual error for
it, and then the access() call would fail for that case.
(Ok, so the security policies would look at the actual bprm data on a
real exec, not the inode executable, so that's kind of made up and
theoretical, but I just want this issue to be mentioned somewhere so
that people are aware that the "it's the same basic file checking that
execve does, but a _real_ execve might then have _other_ issues going
on, including suid bits etc")
Linus
*thread necromancy*
On Tue, Apr 05, 2022 at 06:09:03PM +0200, Micka?l Sala?n wrote:
>
> On 05/04/2022 01:26, Linus Torvalds wrote:
> > On Mon, Apr 4, 2022 at 3:25 PM Kees Cook <[email protected]> wrote:
>
> [...]
>
> >
> > > I think this already exists as AT_EACCESS? It was added with
> > > faccessat2() itself, if I'm reading the history correctly.
> >
> > Yeah, I noticed myself, I just hadn't looked (and I don't do enough
> > user-space programming to be aware of if that way).
>
> I think AT_EACCESS should be usable with the new EXECVE_OK too.
>
>
> >
> > > > (a) "what about suid bits that user space cannot react to"
> > >
> > > What do you mean here? Do you mean setid bits on the file itself?
> >
> > Right.
> >
> > Maybe we don't care.
>
> I think we don't. I think the only corner case that could be different is
> for files that are executable, SUID and non-readable. In this case it
> wouldn't matter because userspace could not read the file, which is required
> for interpretation/execution. Anyway, S[GU]ID bits in scripts are just
> ignored by execve and we want to follow the same semantic.
Hi Micka?l,
Is there a new version of this being worked on? It would be really nice
to have the O_MAYEXEC/faccessat2() visibility for script execution control
in userspace. It seems like it would be mainly a respin of an earlier
version of this series before trusted_for() was proposed.
-Kees
--
Kees Cook
On 08/02/2023 20:32, Kees Cook wrote:
> *thread necromancy*
>
> On Tue, Apr 05, 2022 at 06:09:03PM +0200, Mickaël Salaün wrote:
>>
>> On 05/04/2022 01:26, Linus Torvalds wrote:
>>> On Mon, Apr 4, 2022 at 3:25 PM Kees Cook <[email protected]> wrote:
>>
>> [...]
>>
>>>
>>>> I think this already exists as AT_EACCESS? It was added with
>>>> faccessat2() itself, if I'm reading the history correctly.
>>>
>>> Yeah, I noticed myself, I just hadn't looked (and I don't do enough
>>> user-space programming to be aware of if that way).
>>
>> I think AT_EACCESS should be usable with the new EXECVE_OK too.
>>
>>
>>>
>>>>> (a) "what about suid bits that user space cannot react to"
>>>>
>>>> What do you mean here? Do you mean setid bits on the file itself?
>>>
>>> Right.
>>>
>>> Maybe we don't care.
>>
>> I think we don't. I think the only corner case that could be different is
>> for files that are executable, SUID and non-readable. In this case it
>> wouldn't matter because userspace could not read the file, which is required
>> for interpretation/execution. Anyway, S[GU]ID bits in scripts are just
>> ignored by execve and we want to follow the same semantic.
>
> Hi Mickaël,
>
> Is there a new version of this being worked on? It would be really nice
> to have the O_MAYEXEC/faccessat2() visibility for script execution control
> in userspace. It seems like it would be mainly a respin of an earlier
> version of this series before trusted_for() was proposed.
Yes, I plan to send a new version in a few weeks.
>
> -Kees
>