| Message ID | 20200723171227.446711-1-mic@digikod.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for O_MAYEXEC | expand |
On Thu, Jul 23, 2020 at 07:12:20PM +0200, Mickaël Salaün wrote: > This patch series can be applied on top of v5.8-rc5 . v5.8-rc6, actually. > Previous version: > https://lore.kernel.org/lkml/20200505153156.925111-1-mic@digikod.net/ This is v5. v6 is at https://lore.kernel.org/lkml/20200714181638.45751-1-mic@digikod.net/
I think this looks good now. Andrew, since you're already carrying my exec clean-ups (repeated here in patch 1-3), can you pick the rest of this series too? Thanks! -Kees On Thu, Jul 23, 2020 at 07:12:20PM +0200, Mickaël Salaün wrote: > Hi, > > This seventh patch series do not set __FMODE_EXEC for the sake of > simplicity. A notification feature could be added later if needed. The > handling of all file types is now well defined and tested: by default, > when opening a path, access to a directory is denied (with EISDIR), > access to a regular file depends on the sysctl policy, and access to > other file types (i.e. fifo, device, socket) is denied if there is any > enforced policy. There is new tests covering all these cases (cf. > test_file_types() ). > > As requested by Mimi Zohar, I completed the series with one of her > patches for IMA. I also picked Kees Cook's patches to consolidate exec > permission checking into do_filp_open()'s flow. > > > # Goal of O_MAYEXEC > > The goal of this patch series is to enable to control script execution > with interpreters help. A new O_MAYEXEC flag, usable through > openat2(2), is added to enable userspace script interpreters to delegate > to the kernel (and thus the system security policy) the permission to > interpret/execute scripts or other files containing what can be seen as > commands. > > A simple system-wide security policy can be enforced by the system > administrator through a sysctl configuration consistent with the mount > points or the file access rights. The documentation patch explains the > prerequisites. > > Furthermore, the security policy can also be delegated to an LSM, either > a MAC system or an integrity system. For instance, the new kernel > MAY_OPENEXEC flag closes a major IMA measurement/appraisal interpreter > integrity gap by bringing the ability to check the use of scripts [1]. > Other uses are expected, such as for magic-links [2], SGX integration > [3], bpffs [4] or IPE [5]. > > > # Prerequisite of its use > > Userspace needs to adapt to take advantage of this new feature. For > example, the PEP 578 [6] (Runtime Audit Hooks) enables Python 3.8 to be > extended with policy enforcement points related to code interpretation, > which can be used to align with the PowerShell audit features. > Additional Python security improvements (e.g. a limited interpreter > withou -c, stdin piping of code) are on their way [7]. > > > # Examples > > The initial idea comes from CLIP OS 4 and the original implementation > has been used for more than 12 years: > https://github.com/clipos-archive/clipos4_doc > Chrome OS has a similar approach: > https://chromium.googlesource.com/chromiumos/docs/+/master/security/noexec_shell_scripts.md > > Userland patches can be found here: > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC > Actually, there is more than the O_MAYEXEC changes (which matches this search) > e.g., to prevent Python interactive execution. There are patches for > Bash, Wine, Java (Icedtea), Busybox's ash, Perl and Python. There are > also some related patches which do not directly rely on O_MAYEXEC but > which restrict the use of browser plugins and extensions, which may be > seen as scripts too: > https://github.com/clipos-archive/clipos4_portage-overlay/tree/master/www-client > > An introduction to O_MAYEXEC was given at the Linux Security Summit > Europe 2018 - Linux Kernel Security Contributions by ANSSI: > https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s > The "write xor execute" principle was explained at Kernel Recipes 2018 - > CLIP OS: a defense-in-depth OS: > https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s > See also an overview article: https://lwn.net/Articles/820000/ > > > This patch series can be applied on top of v5.8-rc5 . This can be tested > with CONFIG_SYSCTL. I would really appreciate constructive comments on > this patch series. > > Previous version: > https://lore.kernel.org/lkml/20200505153156.925111-1-mic@digikod.net/ > > > [1] https://lore.kernel.org/lkml/1544647356.4028.105.camel@linux.ibm.com/ > [2] https://lore.kernel.org/lkml/20190904201933.10736-6-cyphar@cyphar.com/ > [3] https://lore.kernel.org/lkml/CALCETrVovr8XNZSroey7pHF46O=kj_c5D9K8h=z2T_cNrpvMig@mail.gmail.com/ > [4] https://lore.kernel.org/lkml/CALCETrVeZ0eufFXwfhtaG_j+AdvbzEWE0M3wjXMWVEO7pj+xkw@mail.gmail.com/ > [5] https://lore.kernel.org/lkml/20200406221439.1469862-12-deven.desai@linux.microsoft.com/ > [6] https://www.python.org/dev/peps/pep-0578/ > [7] https://lore.kernel.org/lkml/0c70debd-e79e-d514-06c6-4cd1e021fa8b@python.org/ > > Regards, > > Kees Cook (3): > exec: Change uselib(2) IS_SREG() failure to EACCES > exec: Move S_ISREG() check earlier > exec: Move path_noexec() check earlier > > Mickaël Salaün (3): > fs: Introduce O_MAYEXEC flag for openat2(2) > fs,doc: Enable to enforce noexec mounts or file exec through O_MAYEXEC > selftest/openat2: Add tests for O_MAYEXEC enforcing > > Mimi Zohar (1): > ima: add policy support for the new file open MAY_OPENEXEC flag > > Documentation/ABI/testing/ima_policy | 2 +- > Documentation/admin-guide/sysctl/fs.rst | 49 +++ > fs/exec.c | 23 +- > fs/fcntl.c | 2 +- > fs/namei.c | 36 +- > fs/open.c | 12 +- > include/linux/fcntl.h | 2 +- > include/linux/fs.h | 3 + > include/uapi/asm-generic/fcntl.h | 7 + > kernel/sysctl.c | 12 +- > security/integrity/ima/ima_main.c | 3 +- > security/integrity/ima/ima_policy.c | 15 +- > tools/testing/selftests/kselftest_harness.h | 3 + > tools/testing/selftests/openat2/Makefile | 3 +- > tools/testing/selftests/openat2/config | 1 + > tools/testing/selftests/openat2/helpers.h | 1 + > .../testing/selftests/openat2/omayexec_test.c | 325 ++++++++++++++++++ > 17 files changed, 470 insertions(+), 29 deletions(-) > create mode 100644 tools/testing/selftests/openat2/config > create mode 100644 tools/testing/selftests/openat2/omayexec_test.c > > -- > 2.27.0 >
On Fri, Jul 24, 2020 at 12:06:53PM -0700, Kees Cook wrote: > I think this looks good now. > > Andrew, since you're already carrying my exec clean-ups (repeated here > in patch 1-3), can you pick the rest of this series too? Al, Not sure if you have already re-surfaced from your csum()/raw_copy_from_user() work completely yet but you had thoughts about this series iirc. Aleksa, thoughts? Thanks! Christian
It seems that there is no more complains nor questions. Do you want me to send another series to fix the order of the S-o-b in patch 7? On 24/07/2020 21:06, Kees Cook wrote: > I think this looks good now. > > Andrew, since you're already carrying my exec clean-ups (repeated here > in patch 1-3), can you pick the rest of this series too? > > Thanks! > > -Kees > > On Thu, Jul 23, 2020 at 07:12:20PM +0200, Mickaël Salaün wrote: >> Hi, >> >> This seventh patch series do not set __FMODE_EXEC for the sake of >> simplicity. A notification feature could be added later if needed. The >> handling of all file types is now well defined and tested: by default, >> when opening a path, access to a directory is denied (with EISDIR), >> access to a regular file depends on the sysctl policy, and access to >> other file types (i.e. fifo, device, socket) is denied if there is any >> enforced policy. There is new tests covering all these cases (cf. >> test_file_types() ). >> >> As requested by Mimi Zohar, I completed the series with one of her >> patches for IMA. I also picked Kees Cook's patches to consolidate exec >> permission checking into do_filp_open()'s flow. >> >> >> # Goal of O_MAYEXEC >> >> The goal of this patch series is to enable to control script execution >> with interpreters help. A new O_MAYEXEC flag, usable through >> openat2(2), is added to enable userspace script interpreters to delegate >> to the kernel (and thus the system security policy) the permission to >> interpret/execute scripts or other files containing what can be seen as >> commands. >> >> A simple system-wide security policy can be enforced by the system >> administrator through a sysctl configuration consistent with the mount >> points or the file access rights. The documentation patch explains the >> prerequisites. >> >> Furthermore, the security policy can also be delegated to an LSM, either >> a MAC system or an integrity system. For instance, the new kernel >> MAY_OPENEXEC flag closes a major IMA measurement/appraisal interpreter >> integrity gap by bringing the ability to check the use of scripts [1]. >> Other uses are expected, such as for magic-links [2], SGX integration >> [3], bpffs [4] or IPE [5]. >> >> >> # Prerequisite of its use >> >> Userspace needs to adapt to take advantage of this new feature. For >> example, the PEP 578 [6] (Runtime Audit Hooks) enables Python 3.8 to be >> extended with policy enforcement points related to code interpretation, >> which can be used to align with the PowerShell audit features. >> Additional Python security improvements (e.g. a limited interpreter >> withou -c, stdin piping of code) are on their way [7]. >> >> >> # Examples >> >> The initial idea comes from CLIP OS 4 and the original implementation >> has been used for more than 12 years: >> https://github.com/clipos-archive/clipos4_doc >> Chrome OS has a similar approach: >> https://chromium.googlesource.com/chromiumos/docs/+/master/security/noexec_shell_scripts.md >> >> Userland patches can be found here: >> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC >> Actually, there is more than the O_MAYEXEC changes (which matches this search) >> e.g., to prevent Python interactive execution. There are patches for >> Bash, Wine, Java (Icedtea), Busybox's ash, Perl and Python. There are >> also some related patches which do not directly rely on O_MAYEXEC but >> which restrict the use of browser plugins and extensions, which may be >> seen as scripts too: >> https://github.com/clipos-archive/clipos4_portage-overlay/tree/master/www-client >> >> An introduction to O_MAYEXEC was given at the Linux Security Summit >> Europe 2018 - Linux Kernel Security Contributions by ANSSI: >> https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s >> The "write xor execute" principle was explained at Kernel Recipes 2018 - >> CLIP OS: a defense-in-depth OS: >> https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s >> See also an overview article: https://lwn.net/Articles/820000/ >> >> >> This patch series can be applied on top of v5.8-rc5 . This can be tested >> with CONFIG_SYSCTL. I would really appreciate constructive comments on >> this patch series. >> >> Previous version: >> https://lore.kernel.org/lkml/20200505153156.925111-1-mic@digikod.net/ >> >> >> [1] https://lore.kernel.org/lkml/1544647356.4028.105.camel@linux.ibm.com/ >> [2] https://lore.kernel.org/lkml/20190904201933.10736-6-cyphar@cyphar.com/ >> [3] https://lore.kernel.org/lkml/CALCETrVovr8XNZSroey7pHF46O=kj_c5D9K8h=z2T_cNrpvMig@mail.gmail.com/ >> [4] https://lore.kernel.org/lkml/CALCETrVeZ0eufFXwfhtaG_j+AdvbzEWE0M3wjXMWVEO7pj+xkw@mail.gmail.com/ >> [5] https://lore.kernel.org/lkml/20200406221439.1469862-12-deven.desai@linux.microsoft.com/ >> [6] https://www.python.org/dev/peps/pep-0578/ >> [7] https://lore.kernel.org/lkml/0c70debd-e79e-d514-06c6-4cd1e021fa8b@python.org/ >> >> Regards, >> >> Kees Cook (3): >> exec: Change uselib(2) IS_SREG() failure to EACCES >> exec: Move S_ISREG() check earlier >> exec: Move path_noexec() check earlier >> >> Mickaël Salaün (3): >> fs: Introduce O_MAYEXEC flag for openat2(2) >> fs,doc: Enable to enforce noexec mounts or file exec through O_MAYEXEC >> selftest/openat2: Add tests for O_MAYEXEC enforcing >> >> Mimi Zohar (1): >> ima: add policy support for the new file open MAY_OPENEXEC flag >> >> Documentation/ABI/testing/ima_policy | 2 +- >> Documentation/admin-guide/sysctl/fs.rst | 49 +++ >> fs/exec.c | 23 +- >> fs/fcntl.c | 2 +- >> fs/namei.c | 36 +- >> fs/open.c | 12 +- >> include/linux/fcntl.h | 2 +- >> include/linux/fs.h | 3 + >> include/uapi/asm-generic/fcntl.h | 7 + >> kernel/sysctl.c | 12 +- >> security/integrity/ima/ima_main.c | 3 +- >> security/integrity/ima/ima_policy.c | 15 +- >> tools/testing/selftests/kselftest_harness.h | 3 + >> tools/testing/selftests/openat2/Makefile | 3 +- >> tools/testing/selftests/openat2/config | 1 + >> tools/testing/selftests/openat2/helpers.h | 1 + >> .../testing/selftests/openat2/omayexec_test.c | 325 ++++++++++++++++++ >> 17 files changed, 470 insertions(+), 29 deletions(-) >> create mode 100644 tools/testing/selftests/openat2/config >> create mode 100644 tools/testing/selftests/openat2/omayexec_test.c >> >> -- >> 2.27.0 >> >
On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: > It seems that there is no more complains nor questions. Do you want me > to send another series to fix the order of the S-o-b in patch 7? There is a major question regarding the API design and the choice of hooking that stuff on open(). And I have not heard anything resembling a coherent answer.
> On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: > > It seems that there is no more complains nor questions. Do you want me > > to send another series to fix the order of the S-o-b in patch 7? > > There is a major question regarding the API design and the choice of > hooking that stuff on open(). And I have not heard anything resembling > a coherent answer. To me O_MAYEXEC is just the wrong name. The bit would be (something like) O_INTERPRET to indicate what you want to do with the contents. The kernel 'policy' then decides whether that needs 'r-x' access or whether 'r--' access in enough. I think that is what you 100 line comment in 0/n means. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Aug 10, 2020 at 10:09:09PM +0000, David Laight wrote: > > On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: > > > It seems that there is no more complains nor questions. Do you want me > > > to send another series to fix the order of the S-o-b in patch 7? > > > > There is a major question regarding the API design and the choice of > > hooking that stuff on open(). And I have not heard anything resembling > > a coherent answer. > > To me O_MAYEXEC is just the wrong name. > The bit would be (something like) O_INTERPRET to indicate > what you want to do with the contents. ... which does not answer the question - name of constant is the least of the worries here. Why the hell is "apply some unspecified checks to file" combined with opening it, rather than being an independent primitive you apply to an already opened file? Just in case - "'cuz that's how we'd done it" does not make a good answer...
On 10/08/2020 22:21, Al Viro wrote: > On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: >> It seems that there is no more complains nor questions. Do you want me >> to send another series to fix the order of the S-o-b in patch 7? > > There is a major question regarding the API design and the choice of > hooking that stuff on open(). And I have not heard anything resembling > a coherent answer. Hooking on open is a simple design that enables processes to check files they intend to open, before they open them. From an API point of view, this series extends openat2(2) with one simple flag: O_MAYEXEC. The enforcement is then subject to the system policy (e.g. mount points, file access rights, IMA, etc.). Checking on open enables to not open a file if it does not meet some requirements, the same way as if the path doesn't exist or (for whatever reasons, including execution permission) if access is denied. It is a good practice to check as soon as possible such properties, and it may enables to avoid (user space) time-of-check to time-of-use (TOCTOU) attacks (i.e. misuse of already open resources). It is important to keep in mind that the use cases we are addressing consider that the (user space) script interpreters (or linkers) are trusted and unaltered (i.e. integrity/authenticity checked). These are similar sought defensive properties as for SUID/SGID binaries: attackers can still launch them with malicious inputs (e.g. file paths, file descriptors, environment variables, etc.), but the binaries can then have a way to check if they can extend their trust to some file paths. Checking file descriptors may help in some use cases, but not the ones motivating this series. Checking (already) opened resources could be a *complementary* way to check execute permission, but it is not in the scope of this series.
On 11/08/2020 00:28, Al Viro wrote: > On Mon, Aug 10, 2020 at 10:09:09PM +0000, David Laight wrote: >>> On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: >>>> It seems that there is no more complains nor questions. Do you want me >>>> to send another series to fix the order of the S-o-b in patch 7? >>> >>> There is a major question regarding the API design and the choice of >>> hooking that stuff on open(). And I have not heard anything resembling >>> a coherent answer. >> >> To me O_MAYEXEC is just the wrong name. >> The bit would be (something like) O_INTERPRET to indicate >> what you want to do with the contents. The properties is "execute permission". This can then be checked by interpreters or other applications, then the generic O_MAYEXEC name. > > ... which does not answer the question - name of constant is the least of > the worries here. Why the hell is "apply some unspecified checks to > file" combined with opening it, rather than being an independent primitive > you apply to an already opened file? Just in case - "'cuz that's how we'd > done it" does not make a good answer... > That is not the case, see https://lore.kernel.org/lkml/917bb071-8b1a-3ba4-dc16-f8d7b4cc849f@digikod.net/
On Tue, Aug 11, 2020 at 12:43 AM Mickaël Salaün <mic@digikod.net> wrote: > On 10/08/2020 22:21, Al Viro wrote: > > On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: > >> It seems that there is no more complains nor questions. Do you want me > >> to send another series to fix the order of the S-o-b in patch 7? > > > > There is a major question regarding the API design and the choice of > > hooking that stuff on open(). And I have not heard anything resembling > > a coherent answer. > > Hooking on open is a simple design that enables processes to check files > they intend to open, before they open them. From an API point of view, > this series extends openat2(2) with one simple flag: O_MAYEXEC. The > enforcement is then subject to the system policy (e.g. mount points, > file access rights, IMA, etc.). > > Checking on open enables to not open a file if it does not meet some > requirements, the same way as if the path doesn't exist or (for whatever > reasons, including execution permission) if access is denied. You can do exactly the same thing if you do the check in a separate syscall though. And it provides a greater degree of flexibility; for example, you can use it in combination with fopen() without having to modify the internals of fopen() or having to use fdopen(). > It is a > good practice to check as soon as possible such properties, and it may > enables to avoid (user space) time-of-check to time-of-use (TOCTOU) > attacks (i.e. misuse of already open resources). The assumption that security checks should happen as early as possible can actually cause security problems. For example, because seccomp was designed to do its checks as early as possible, including before ptrace, we had an issue for a long time where the ptrace API could be abused to bypass seccomp filters. Please don't decide that a check must be ordered first _just_ because it is a security check. While that can be good for limiting attack surface, it can also create issues when the idea is applied too broadly. I don't see how TOCTOU issues are relevant in any way here. If someone can turn a script that is considered a trusted file into an untrusted file and then maliciously change its contents, you're going to have issues either way because the modifications could still happen after openat(); if this was possible, the whole thing would kind of fall apart. And if that isn't possible, I don't see any TOCTOU. > It is important to keep > in mind that the use cases we are addressing consider that the (user > space) script interpreters (or linkers) are trusted and unaltered (i.e. > integrity/authenticity checked). These are similar sought defensive > properties as for SUID/SGID binaries: attackers can still launch them > with malicious inputs (e.g. file paths, file descriptors, environment > variables, etc.), but the binaries can then have a way to check if they > can extend their trust to some file paths. > > Checking file descriptors may help in some use cases, but not the ones > motivating this series. It actually provides a superset of the functionality that your existing patches provide. > Checking (already) opened resources could be a > *complementary* way to check execute permission, but it is not in the > scope of this series.
On Tue, Aug 11, 2020 at 12:43:52AM +0200, Mickaël Salaün wrote: > Hooking on open is a simple design that enables processes to check files > they intend to open, before they open them. Which is a good thing, because...? > From an API point of view, > this series extends openat2(2) with one simple flag: O_MAYEXEC. The > enforcement is then subject to the system policy (e.g. mount points, > file access rights, IMA, etc.). That's what "unspecified" means - as far as the kernel concerned, it's "something completely opaque, will let these hooks to play, semantics is entirely up to them". > Checking on open enables to not open a file if it does not meet some > requirements, the same way as if the path doesn't exist or (for whatever > reasons, including execution permission) if access is denied. It is a > good practice to check as soon as possible such properties, and it may > enables to avoid (user space) time-of-check to time-of-use (TOCTOU) > attacks (i.e. misuse of already open resources). ????? You explicitly assume a cooperating caller. If it can't be trusted to issue the check between open and use, or can be manipulated (ptraced, etc.) into not doing so, how can you rely upon the flag having been passed in the first place? And TOCTOU window is definitely not wider that way. If you want to have it done immediately after open(), bloody well do it immediately after open. If attacker has subverted your control flow to the extent that allows them to hit descriptor table in the interval between these two syscalls, you have already lost - they'll simply prevent that flag from being passed. What's the point of burying it inside openat2()? A convenient multiplexor to hook into? We already have one - it's called do_syscall_...
> On 11/08/2020 00:28, Al Viro wrote: > > On Mon, Aug 10, 2020 at 10:09:09PM +0000, David Laight wrote: > >>> On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: > >>>> It seems that there is no more complains nor questions. Do you want me > >>>> to send another series to fix the order of the S-o-b in patch 7? > >>> > >>> There is a major question regarding the API design and the choice of > >>> hooking that stuff on open(). And I have not heard anything resembling > >>> a coherent answer. > >> > >> To me O_MAYEXEC is just the wrong name. > >> The bit would be (something like) O_INTERPRET to indicate > >> what you want to do with the contents. > > The properties is "execute permission". This can then be checked by > interpreters or other applications, then the generic O_MAYEXEC name. The english sense of MAYEXEC is just wrong for what you are trying to check. > > ... which does not answer the question - name of constant is the least of > > the worries here. Why the hell is "apply some unspecified checks to > > file" combined with opening it, rather than being an independent primitive > > you apply to an already opened file? Just in case - "'cuz that's how we'd > > done it" does not make a good answer... Maybe an access_ok() that acts on an open fd would be more appropriate. Which might end up being an fcntrl() action. That would give you a full 32bit mask of options. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 11/08/2020 01:03, Jann Horn wrote: > On Tue, Aug 11, 2020 at 12:43 AM Mickaël Salaün <mic@digikod.net> wrote: >> On 10/08/2020 22:21, Al Viro wrote: >>> On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: >>>> It seems that there is no more complains nor questions. Do you want me >>>> to send another series to fix the order of the S-o-b in patch 7? >>> >>> There is a major question regarding the API design and the choice of >>> hooking that stuff on open(). And I have not heard anything resembling >>> a coherent answer. >> >> Hooking on open is a simple design that enables processes to check files >> they intend to open, before they open them. From an API point of view, >> this series extends openat2(2) with one simple flag: O_MAYEXEC. The >> enforcement is then subject to the system policy (e.g. mount points, >> file access rights, IMA, etc.). >> >> Checking on open enables to not open a file if it does not meet some >> requirements, the same way as if the path doesn't exist or (for whatever >> reasons, including execution permission) if access is denied. > > You can do exactly the same thing if you do the check in a separate > syscall though. > > And it provides a greater degree of flexibility; for example, you can > use it in combination with fopen() without having to modify the > internals of fopen() or having to use fdopen(). > >> It is a >> good practice to check as soon as possible such properties, and it may >> enables to avoid (user space) time-of-check to time-of-use (TOCTOU) >> attacks (i.e. misuse of already open resources). > > The assumption that security checks should happen as early as possible > can actually cause security problems. For example, because seccomp was > designed to do its checks as early as possible, including before > ptrace, we had an issue for a long time where the ptrace API could be > abused to bypass seccomp filters. > > Please don't decide that a check must be ordered first _just_ because > it is a security check. While that can be good for limiting attack > surface, it can also create issues when the idea is applied too > broadly. I'd be interested with such security issue examples. I hope that delaying checks will not be an issue for mechanisms such as IMA or IPE: https://lore.kernel.org/lkml/1544699060.6703.11.camel@linux.ibm.com/ Any though Mimi, Deven, Chrome OS folks? > > I don't see how TOCTOU issues are relevant in any way here. If someone > can turn a script that is considered a trusted file into an untrusted > file and then maliciously change its contents, you're going to have > issues either way because the modifications could still happen after > openat(); if this was possible, the whole thing would kind of fall > apart. And if that isn't possible, I don't see any TOCTOU. Sure, and if the scripts are not protected in some way there is no point to check anything. > >> It is important to keep >> in mind that the use cases we are addressing consider that the (user >> space) script interpreters (or linkers) are trusted and unaltered (i.e. >> integrity/authenticity checked). These are similar sought defensive >> properties as for SUID/SGID binaries: attackers can still launch them >> with malicious inputs (e.g. file paths, file descriptors, environment >> variables, etc.), but the binaries can then have a way to check if they >> can extend their trust to some file paths. >> >> Checking file descriptors may help in some use cases, but not the ones >> motivating this series. > > It actually provides a superset of the functionality that your > existing patches provide. It also brings new issues with multiple file descriptor origins (e.g. memfd_create). > >> Checking (already) opened resources could be a >> *complementary* way to check execute permission, but it is not in the >> scope of this series.
On 11/08/2020 01:05, Al Viro wrote: > On Tue, Aug 11, 2020 at 12:43:52AM +0200, Mickaël Salaün wrote: > >> Hooking on open is a simple design that enables processes to check files >> they intend to open, before they open them. > > Which is a good thing, because...? > >> From an API point of view, >> this series extends openat2(2) with one simple flag: O_MAYEXEC. The >> enforcement is then subject to the system policy (e.g. mount points, >> file access rights, IMA, etc.). > > That's what "unspecified" means - as far as the kernel concerned, it's > "something completely opaque, will let these hooks to play, semantics is > entirely up to them". I see it as an access controls mechanism; access may be granted or denied, as for O_RDONLY, O_WRONLY or (non-Linux) O_EXEC. Even for common access controls, there are capabilities to bypass them (i.e. CAP_DAC_OVERRIDE), but multiple layers may enforce different complementary policies. > >> Checking on open enables to not open a file if it does not meet some >> requirements, the same way as if the path doesn't exist or (for whatever >> reasons, including execution permission) if access is denied. It is a >> good practice to check as soon as possible such properties, and it may >> enables to avoid (user space) time-of-check to time-of-use (TOCTOU) >> attacks (i.e. misuse of already open resources). > > ????? You explicitly assume a cooperating caller. As said in the below (removed) reply, no, quite the contrary. > If it can't be trusted > to issue the check between open and use, or can be manipulated (ptraced, > etc.) into not doing so, how can you rely upon the flag having been passed > in the first place? And TOCTOU window is definitely not wider that way. OK, I guess it would be considered a bug in the application (e.g. buggy resource management between threads). > > If you want to have it done immediately after open(), bloody well do it > immediately after open. If attacker has subverted your control flow to the > extent that allows them to hit descriptor table in the interval between > these two syscalls, you have already lost - they'll simply prevent that > flag from being passed. > > What's the point of burying it inside openat2()? A convenient multiplexor > to hook into? We already have one - it's called do_syscall_... > To check as soon as possible without opening something that should not be opened in the first place. Isn't a dedicated syscall a bit too much for this feature? What about adding a new command/flag to fcntl(2)?
On 11/08/2020 10:09, David Laight wrote: >> On 11/08/2020 00:28, Al Viro wrote: >>> On Mon, Aug 10, 2020 at 10:09:09PM +0000, David Laight wrote: >>>>> On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: >>>>>> It seems that there is no more complains nor questions. Do you want me >>>>>> to send another series to fix the order of the S-o-b in patch 7? >>>>> >>>>> There is a major question regarding the API design and the choice of >>>>> hooking that stuff on open(). And I have not heard anything resembling >>>>> a coherent answer. >>>> >>>> To me O_MAYEXEC is just the wrong name. >>>> The bit would be (something like) O_INTERPRET to indicate >>>> what you want to do with the contents. >> >> The properties is "execute permission". This can then be checked by >> interpreters or other applications, then the generic O_MAYEXEC name. > > The english sense of MAYEXEC is just wrong for what you are trying > to check. We think it reflects exactly what it's purpose is. > >>> ... which does not answer the question - name of constant is the least of >>> the worries here. Why the hell is "apply some unspecified checks to >>> file" combined with opening it, rather than being an independent primitive >>> you apply to an already opened file? Just in case - "'cuz that's how we'd >>> done it" does not make a good answer... > > Maybe an access_ok() that acts on an open fd would be more > appropriate. > Which might end up being an fcntrl() action. > That would give you a full 32bit mask of options. I previously talk about fcntl(2): https://lore.kernel.org/lkml/eaf5bc42-e086-740b-a90c-93e67c535eee@digikod.net/
On Tue, 2020-08-11 at 10:48 +0200, Mickaël Salaün wrote: > On 11/08/2020 01:03, Jann Horn wrote: > > On Tue, Aug 11, 2020 at 12:43 AM Mickaël Salaün <mic@digikod.net> wrote: > > > On 10/08/2020 22:21, Al Viro wrote: > > > > On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: > > > > > It seems that there is no more complains nor questions. Do you want me > > > > > to send another series to fix the order of the S-o-b in patch 7? > > > > > > > > There is a major question regarding the API design and the choice of > > > > hooking that stuff on open(). And I have not heard anything resembling > > > > a coherent answer. > > > > > > Hooking on open is a simple design that enables processes to check files > > > they intend to open, before they open them. From an API point of view, > > > this series extends openat2(2) with one simple flag: O_MAYEXEC. The > > > enforcement is then subject to the system policy (e.g. mount points, > > > file access rights, IMA, etc.). > > > > > > Checking on open enables to not open a file if it does not meet some > > > requirements, the same way as if the path doesn't exist or (for whatever > > > reasons, including execution permission) if access is denied. > > > > You can do exactly the same thing if you do the check in a separate > > syscall though. > > > > And it provides a greater degree of flexibility; for example, you can > > use it in combination with fopen() without having to modify the > > internals of fopen() or having to use fdopen(). > > > > > It is a > > > good practice to check as soon as possible such properties, and it may > > > enables to avoid (user space) time-of-check to time-of-use (TOCTOU) > > > attacks (i.e. misuse of already open resources). > > > > The assumption that security checks should happen as early as possible > > can actually cause security problems. For example, because seccomp was > > designed to do its checks as early as possible, including before > > ptrace, we had an issue for a long time where the ptrace API could be > > abused to bypass seccomp filters. > > > > Please don't decide that a check must be ordered first _just_ because > > it is a security check. While that can be good for limiting attack > > surface, it can also create issues when the idea is applied too > > broadly. > > I'd be interested with such security issue examples. > > I hope that delaying checks will not be an issue for mechanisms such as > IMA or IPE: > https://lore.kernel.org/lkml/1544699060.6703.11.camel@linux.ibm.com/ > > Any though Mimi, Deven, Chrome OS folks? One of the major gaps, defining a system wide policy requiring all code being executed to be signed, is interpreters. The kernel has no context for the interpreter's opening the file. From an IMA perspective, this information needs to be conveyed to the kernel prior to ima_file_check(), which would allow IMA policy rules to be defined in terms of O_MAYEXEC. > > > I don't see how TOCTOU issues are relevant in any way here. If someone > > can turn a script that is considered a trusted file into an untrusted > > file and then maliciously change its contents, you're going to have > > issues either way because the modifications could still happen after > > openat(); if this was possible, the whole thing would kind of fall > > apart. And if that isn't possible, I don't see any TOCTOU. > > Sure, and if the scripts are not protected in some way there is no point > to check anything. The interpreter itself would be signed. Mimi > > > > It is important to keep > > > in mind that the use cases we are addressing consider that the (user > > > space) script interpreters (or linkers) are trusted and unaltered (i.e. > > > integrity/authenticity checked). These are similar sought defensive > > > properties as for SUID/SGID binaries: attackers can still launch them > > > with malicious inputs (e.g. file paths, file descriptors, environment > > > variables, etc.), but the binaries can then have a way to check if they > > > can extend their trust to some file paths. > > > > > > Checking file descriptors may help in some use cases, but not the ones > > > motivating this series. > > > > It actually provides a superset of the functionality that your > > existing patches provide. > > It also brings new issues with multiple file descriptor origins (e.g. > memfd_create). > > > > Checking (already) opened resources could be a > > > *complementary* way to check execute permission, but it is not in the > > > scope of this series.
On Tue, Aug 11, 2020 at 09:56:50AM -0400, Mimi Zohar wrote: > On Tue, 2020-08-11 at 10:48 +0200, Mickaël Salaün wrote: > > On 11/08/2020 01:03, Jann Horn wrote: > > > On Tue, Aug 11, 2020 at 12:43 AM Mickaël Salaün <mic@digikod.net> wrote: > > > > On 10/08/2020 22:21, Al Viro wrote: > > > > > On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: > > > > > > It seems that there is no more complains nor questions. Do you want me > > > > > > to send another series to fix the order of the S-o-b in patch 7? > > > > > > > > > > There is a major question regarding the API design and the choice of > > > > > hooking that stuff on open(). And I have not heard anything resembling > > > > > a coherent answer. > > > > > > > > Hooking on open is a simple design that enables processes to check files > > > > they intend to open, before they open them. From an API point of view, > > > > this series extends openat2(2) with one simple flag: O_MAYEXEC. The > > > > enforcement is then subject to the system policy (e.g. mount points, > > > > file access rights, IMA, etc.). > > > > > > > > Checking on open enables to not open a file if it does not meet some > > > > requirements, the same way as if the path doesn't exist or (for whatever > > > > reasons, including execution permission) if access is denied. > > > > > > You can do exactly the same thing if you do the check in a separate > > > syscall though. > > > > > > And it provides a greater degree of flexibility; for example, you can > > > use it in combination with fopen() without having to modify the > > > internals of fopen() or having to use fdopen(). > > > > > > > It is a > > > > good practice to check as soon as possible such properties, and it may > > > > enables to avoid (user space) time-of-check to time-of-use (TOCTOU) > > > > attacks (i.e. misuse of already open resources). > > > > > > The assumption that security checks should happen as early as possible > > > can actually cause security problems. For example, because seccomp was > > > designed to do its checks as early as possible, including before > > > ptrace, we had an issue for a long time where the ptrace API could be > > > abused to bypass seccomp filters. > > > > > > Please don't decide that a check must be ordered first _just_ because > > > it is a security check. While that can be good for limiting attack > > > surface, it can also create issues when the idea is applied too > > > broadly. > > > > I'd be interested with such security issue examples. > > > > I hope that delaying checks will not be an issue for mechanisms such as > > IMA or IPE: > > https://lore.kernel.org/lkml/1544699060.6703.11.camel@linux.ibm.com/ > > > > Any though Mimi, Deven, Chrome OS folks? > > One of the major gaps, defining a system wide policy requiring all code > being executed to be signed, is interpreters. The kernel has no > context for the interpreter's opening the file. From an IMA > perspective, this information needs to be conveyed to the kernel prior > to ima_file_check(), which would allow IMA policy rules to be defined > in terms of O_MAYEXEC. This is kind of evading the point -- Mickaël is proposing a new flag to open() to tell IMA to measure the file being opened before the fd is returned to userspace, and Al is suggesting a new syscall to allow a previously-obtained fd to be measured. I think what you're saying is that you don't see any reason to prefer one over the other.
On Tue, 2020-08-11 at 15:02 +0100, Matthew Wilcox wrote: > On Tue, Aug 11, 2020 at 09:56:50AM -0400, Mimi Zohar wrote: > > On Tue, 2020-08-11 at 10:48 +0200, Mickaël Salaün wrote: > > > On 11/08/2020 01:03, Jann Horn wrote: > > > > On Tue, Aug 11, 2020 at 12:43 AM Mickaël Salaün <mic@digikod.net> wrote: > > > > > On 10/08/2020 22:21, Al Viro wrote: > > > > > > On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: > > > > > > > It seems that there is no more complains nor questions. Do you want me > > > > > > > to send another series to fix the order of the S-o-b in patch 7? > > > > > > > > > > > > There is a major question regarding the API design and the choice of > > > > > > hooking that stuff on open(). And I have not heard anything resembling > > > > > > a coherent answer. > > > > > > > > > > Hooking on open is a simple design that enables processes to check files > > > > > they intend to open, before they open them. From an API point of view, > > > > > this series extends openat2(2) with one simple flag: O_MAYEXEC. The > > > > > enforcement is then subject to the system policy (e.g. mount points, > > > > > file access rights, IMA, etc.). > > > > > > > > > > Checking on open enables to not open a file if it does not meet some > > > > > requirements, the same way as if the path doesn't exist or (for whatever > > > > > reasons, including execution permission) if access is denied. > > > > > > > > You can do exactly the same thing if you do the check in a separate > > > > syscall though. > > > > > > > > And it provides a greater degree of flexibility; for example, you can > > > > use it in combination with fopen() without having to modify the > > > > internals of fopen() or having to use fdopen(). > > > > > > > > > It is a > > > > > good practice to check as soon as possible such properties, and it may > > > > > enables to avoid (user space) time-of-check to time-of-use (TOCTOU) > > > > > attacks (i.e. misuse of already open resources). > > > > > > > > The assumption that security checks should happen as early as possible > > > > can actually cause security problems. For example, because seccomp was > > > > designed to do its checks as early as possible, including before > > > > ptrace, we had an issue for a long time where the ptrace API could be > > > > abused to bypass seccomp filters. > > > > > > > > Please don't decide that a check must be ordered first _just_ because > > > > it is a security check. While that can be good for limiting attack > > > > surface, it can also create issues when the idea is applied too > > > > broadly. > > > > > > I'd be interested with such security issue examples. > > > > > > I hope that delaying checks will not be an issue for mechanisms such as > > > IMA or IPE: > > > https://lore.kernel.org/lkml/1544699060.6703.11.camel@linux.ibm.com/ > > > > > > Any though Mimi, Deven, Chrome OS folks? > > > > One of the major gaps, defining a system wide policy requiring all code > > being executed to be signed, is interpreters. The kernel has no > > context for the interpreter's opening the file. From an IMA > > perspective, this information needs to be conveyed to the kernel prior > > to ima_file_check(), which would allow IMA policy rules to be defined > > in terms of O_MAYEXEC. > > This is kind of evading the point -- Mickaël is proposing a new flag > to open() to tell IMA to measure the file being opened before the fd > is returned to userspace, and Al is suggesting a new syscall to allow > a previously-obtained fd to be measured. > > I think what you're saying is that you don't see any reason to prefer > one over the other. Being able to define IMA appraise and measure file open (func=FILE_CHECK) policy rules to prevent the interpreter from executing unsigned files would be really nice. Otherwise, the file would be measured and appraised multiple times, once on file open and again at the point of this new syscall. Mimi
On 8/11/2020 1:48 AM, Mickaël Salaün wrote: [...snip] >>> It is a >>> good practice to check as soon as possible such properties, and it may >>> enables to avoid (user space) time-of-check to time-of-use (TOCTOU) >>> attacks (i.e. misuse of already open resources). >> >> The assumption that security checks should happen as early as possible >> can actually cause security problems. For example, because seccomp was >> designed to do its checks as early as possible, including before >> ptrace, we had an issue for a long time where the ptrace API could be >> abused to bypass seccomp filters. >> >> Please don't decide that a check must be ordered first _just_ because >> it is a security check. While that can be good for limiting attack >> surface, it can also create issues when the idea is applied too >> broadly. > > I'd be interested with such security issue examples. > > I hope that delaying checks will not be an issue for mechanisms such as > IMA or IPE: > https://lore.kernel.org/lkml/1544699060.6703.11.camel@linux.ibm.com/ > > Any though Mimi, Deven, Chrome OS folks? > I don't see an issue with IPE. As long as the hypothetical new syscall and associated security hook have the file struct available in the hook, it should integrate fairly easily. [...snip]