[RFC,v1,3/5] Yama: Enforces noexec mounts or file executability through O_MAYEXEC
diff mbox series

Message ID 20181212081712.32347-4-mic@digikod.net
State New
Headers show
Series
  • Add support for O_MAYEXEC
Related show

Commit Message

Mickaël Salaün Dec. 12, 2018, 8:17 a.m. UTC
Enable to either propagate the mount options from the underlying VFS
mount to prevent execution, or to propagate the file execute permission.
This may allow a script interpreter to check execution permissions
before reading commands from a file.

The main goal is to be able to protect the kernel by restricting
arbitrary syscalls that an attacker could perform with a crafted binary
or certain script languages.  It also improves multilevel isolation
by reducing the ability of an attacker to use side channels with
specific code.  These restrictions can natively be enforced for ELF
binaries (with the noexec mount option) but require this kernel
extension to properly handle scripts (e.g., Python, Perl).

Add a new sysctl kernel.yama.open_mayexec_enforce to control this
behavior.  A following patch adds documentation.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
---
 security/yama/Kconfig    |  3 +-
 security/yama/yama_lsm.c | 82 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 83 insertions(+), 2 deletions(-)

Comments

Mickaël Salaün Dec. 12, 2018, 2:28 p.m. UTC | #1
Le 12/12/2018 à 09:17, Mickaël Salaün a écrit :
> Enable to either propagate the mount options from the underlying VFS
> mount to prevent execution, or to propagate the file execute permission.
> This may allow a script interpreter to check execution permissions
> before reading commands from a file.
> 
> The main goal is to be able to protect the kernel by restricting
> arbitrary syscalls that an attacker could perform with a crafted binary
> or certain script languages.  It also improves multilevel isolation
> by reducing the ability of an attacker to use side channels with
> specific code.  These restrictions can natively be enforced for ELF
> binaries (with the noexec mount option) but require this kernel
> extension to properly handle scripts (e.g., Python, Perl).
> 
> Add a new sysctl kernel.yama.open_mayexec_enforce to control this
> behavior.  A following patch adds documentation.
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
> ---
>  security/yama/Kconfig    |  3 +-
>  security/yama/yama_lsm.c | 82 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 83 insertions(+), 2 deletions(-)
> 
> diff --git a/security/yama/Kconfig b/security/yama/Kconfig
> index 96b27405558a..9457619fabd5 100644
> --- a/security/yama/Kconfig
> +++ b/security/yama/Kconfig
> @@ -5,7 +5,8 @@ config SECURITY_YAMA
>  	help
>  	  This selects Yama, which extends DAC support with additional
>  	  system-wide security settings beyond regular Linux discretionary
> -	  access controls. Currently available is ptrace scope restriction.
> +	  access controls. Currently available are ptrace scope restriction and
> +	  enforcement of the O_MAYEXEC open flag.
>  	  Like capabilities, this security module stacks with other LSMs.
>  	  Further information can be found in
>  	  Documentation/admin-guide/LSM/Yama.rst.
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index ffda91a4a1aa..120664e94ee5 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -1,10 +1,12 @@
>  /*
>   * Yama Linux Security Module
>   *
> - * Author: Kees Cook <keescook@chromium.org>
> + * Authors: Kees Cook <keescook@chromium.org>
> + *          Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
>   *
>   * Copyright (C) 2010 Canonical, Ltd.
>   * Copyright (C) 2011 The Chromium OS Authors.
> + * Copyright (C) 2018 ANSSI
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2, as
> @@ -28,7 +30,14 @@
>  #define YAMA_SCOPE_CAPABILITY	2
>  #define YAMA_SCOPE_NO_ATTACH	3
>  
> +#define YAMA_OMAYEXEC_ENFORCE_NONE	0
> +#define YAMA_OMAYEXEC_ENFORCE_MOUNT	(1 << 0)
> +#define YAMA_OMAYEXEC_ENFORCE_FILE	(1 << 1)
> +#define _YAMA_OMAYEXEC_LAST		YAMA_OMAYEXEC_ENFORCE_FILE
> +#define _YAMA_OMAYEXEC_MASK		((_YAMA_OMAYEXEC_LAST << 1) - 1)
> +
>  static int ptrace_scope = YAMA_SCOPE_RELATIONAL;
> +static int open_mayexec_enforce = YAMA_OMAYEXEC_ENFORCE_NONE;
>  
>  /* describe a ptrace relationship for potential exception */
>  struct ptrace_relation {
> @@ -423,7 +432,40 @@ int yama_ptrace_traceme(struct task_struct *parent)
>  	return rc;
>  }
>  
> +/**
> + * yama_inode_permission - check O_MAYEXEC permission before accessing an inode
> + * @inode: inode structure to check
> + * @mask: permission mask
> + *
> + * Return 0 if access is permitted, -EACCES otherwise.
> + */
> +int yama_inode_permission(struct inode *inode, int mask)
> +{
> +	if (!(mask & MAY_OPENEXEC))
> +		return 0;
> +	/*
> +	 * Match regular files and directories to make it easier to
> +	 * modify script interpreters.
> +	 */
> +	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> +		return 0;

I forgot to mention that these checks do not handle fifos. This is
relevant in a threat model targeting persistent attacks (and with
additional protections/restrictions).

> +
> +	if ((open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_MOUNT) &&
> +			!(mask & MAY_EXECMOUNT))
> +		return -EACCES;
> +
> +	/*
> +	 * May prefer acl_permission_check() instead of generic_permission(),
> +	 * to not be bypassable with CAP_DAC_READ_SEARCH.
> +	 */
> +	if (open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_FILE)
> +		return generic_permission(inode, MAY_EXEC);
> +
> +	return 0;
> +}
> +
>  static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(inode_permission, yama_inode_permission),
>  	LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
>  	LSM_HOOK_INIT(task_prctl, yama_task_prctl),
> @@ -447,6 +489,37 @@ static int yama_dointvec_minmax(struct ctl_table *table, int write,
>  	return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>  }
>  
> +static int yama_dointvec_bitmask_macadmin(struct ctl_table *table, int write,
> +					  void __user *buffer, size_t *lenp,
> +					  loff_t *ppos)
> +{
> +	int error;
> +
> +	if (write) {
> +		struct ctl_table table_copy;
> +		int tmp_mayexec_enforce;
> +
> +		if (!capable(CAP_MAC_ADMIN))
> +			return -EPERM;
> +		tmp_mayexec_enforce = *((int *)table->data);
> +		table_copy = *table;
> +		/* do not erase open_mayexec_enforce */
> +		table_copy.data = &tmp_mayexec_enforce;
> +		error = proc_dointvec(&table_copy, write, buffer, lenp, ppos);
> +		if (error)
> +			return error;
> +		if ((tmp_mayexec_enforce | _YAMA_OMAYEXEC_MASK) !=
> +				_YAMA_OMAYEXEC_MASK)
> +			return -EINVAL;
> +		*((int *)table->data) = tmp_mayexec_enforce;
> +	} else {
> +		error = proc_dointvec(table, write, buffer, lenp, ppos);
> +		if (error)
> +			return error;
> +	}
> +	return 0;
> +}
> +
>  static int zero;
>  static int max_scope = YAMA_SCOPE_NO_ATTACH;
>  
> @@ -466,6 +539,13 @@ static struct ctl_table yama_sysctl_table[] = {
>  		.extra1         = &zero,
>  		.extra2         = &max_scope,
>  	},
> +	{
> +		.procname       = "open_mayexec_enforce",
> +		.data           = &open_mayexec_enforce,
> +		.maxlen         = sizeof(int),
> +		.mode           = 0644,
> +		.proc_handler   = yama_dointvec_bitmask_macadmin,
> +	},
>  	{ }
>  };
>  static void __init yama_init_sysctl(void)
>
Jann Horn Dec. 12, 2018, 5:09 p.m. UTC | #2
On Wed, Dec 12, 2018 at 9:18 AM Mickaël Salaün <mic@digikod.net> wrote:
> Enable to either propagate the mount options from the underlying VFS
> mount to prevent execution, or to propagate the file execute permission.
> This may allow a script interpreter to check execution permissions
> before reading commands from a file.
>
> The main goal is to be able to protect the kernel by restricting
> arbitrary syscalls that an attacker could perform with a crafted binary
> or certain script languages.  It also improves multilevel isolation
> by reducing the ability of an attacker to use side channels with
> specific code.  These restrictions can natively be enforced for ELF
> binaries (with the noexec mount option) but require this kernel
> extension to properly handle scripts (e.g., Python, Perl).
>
> Add a new sysctl kernel.yama.open_mayexec_enforce to control this
> behavior.  A following patch adds documentation.
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
> ---
[...]
> +/**
> + * yama_inode_permission - check O_MAYEXEC permission before accessing an inode
> + * @inode: inode structure to check
> + * @mask: permission mask
> + *
> + * Return 0 if access is permitted, -EACCES otherwise.
> + */
> +int yama_inode_permission(struct inode *inode, int mask)

This should be static, no?

> +{
> +       if (!(mask & MAY_OPENEXEC))
> +               return 0;
> +       /*
> +        * Match regular files and directories to make it easier to
> +        * modify script interpreters.
> +        */
> +       if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> +               return 0;

So files are subject to checks, but loading code from things like
sockets is always fine?

> +       if ((open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_MOUNT) &&
> +                       !(mask & MAY_EXECMOUNT))
> +               return -EACCES;
> +
> +       /*
> +        * May prefer acl_permission_check() instead of generic_permission(),
> +        * to not be bypassable with CAP_DAC_READ_SEARCH.
> +        */
> +       if (open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_FILE)
> +               return generic_permission(inode, MAY_EXEC);
> +
> +       return 0;
> +}
> +
>  static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
> +       LSM_HOOK_INIT(inode_permission, yama_inode_permission),
>         LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
>         LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
>         LSM_HOOK_INIT(task_prctl, yama_task_prctl),
> @@ -447,6 +489,37 @@ static int yama_dointvec_minmax(struct ctl_table *table, int write,
>         return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>  }
>
> +static int yama_dointvec_bitmask_macadmin(struct ctl_table *table, int write,
> +                                         void __user *buffer, size_t *lenp,
> +                                         loff_t *ppos)
> +{
> +       int error;
> +
> +       if (write) {
> +               struct ctl_table table_copy;
> +               int tmp_mayexec_enforce;
> +
> +               if (!capable(CAP_MAC_ADMIN))
> +                       return -EPERM;

Don't put capable() checks in sysctls, it doesn't work.
Mickaël Salaün Dec. 13, 2018, 2:49 p.m. UTC | #3
On 12/12/2018 18:09, Jann Horn wrote:
> On Wed, Dec 12, 2018 at 9:18 AM Mickaël Salaün <mic@digikod.net> wrote:
>> Enable to either propagate the mount options from the underlying VFS
>> mount to prevent execution, or to propagate the file execute permission.
>> This may allow a script interpreter to check execution permissions
>> before reading commands from a file.
>>
>> The main goal is to be able to protect the kernel by restricting
>> arbitrary syscalls that an attacker could perform with a crafted binary
>> or certain script languages.  It also improves multilevel isolation
>> by reducing the ability of an attacker to use side channels with
>> specific code.  These restrictions can natively be enforced for ELF
>> binaries (with the noexec mount option) but require this kernel
>> extension to properly handle scripts (e.g., Python, Perl).
>>
>> Add a new sysctl kernel.yama.open_mayexec_enforce to control this
>> behavior.  A following patch adds documentation.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
>> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
>> ---
> [...]
>> +/**
>> + * yama_inode_permission - check O_MAYEXEC permission before accessing an inode
>> + * @inode: inode structure to check
>> + * @mask: permission mask
>> + *
>> + * Return 0 if access is permitted, -EACCES otherwise.
>> + */
>> +int yama_inode_permission(struct inode *inode, int mask)
> 
> This should be static, no?

Right, it will be in the next series. The previous function
(yama_ptrace_traceme) is not static though.

> 
>> +{
>> +       if (!(mask & MAY_OPENEXEC))
>> +               return 0;
>> +       /*
>> +        * Match regular files and directories to make it easier to
>> +        * modify script interpreters.
>> +        */
>> +       if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
>> +               return 0;
> 
> So files are subject to checks, but loading code from things like
> sockets is always fine?

As I said in a previous email, these checks do not handle fifo either.
This is relevant in a threat model targeting persistent attacks (and
with additional protections/restrictions). We may want to only whitelist
fifo, but I don't get how a socket is relevant here. Can you please clarify?

> 
>> +       if ((open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_MOUNT) &&
>> +                       !(mask & MAY_EXECMOUNT))
>> +               return -EACCES;
>> +
>> +       /*
>> +        * May prefer acl_permission_check() instead of generic_permission(),
>> +        * to not be bypassable with CAP_DAC_READ_SEARCH.
>> +        */
>> +       if (open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_FILE)
>> +               return generic_permission(inode, MAY_EXEC);
>> +
>> +       return 0;
>> +}
>> +
>>  static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
>> +       LSM_HOOK_INIT(inode_permission, yama_inode_permission),
>>         LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
>>         LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
>>         LSM_HOOK_INIT(task_prctl, yama_task_prctl),
>> @@ -447,6 +489,37 @@ static int yama_dointvec_minmax(struct ctl_table *table, int write,
>>         return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>>  }
>>
>> +static int yama_dointvec_bitmask_macadmin(struct ctl_table *table, int write,
>> +                                         void __user *buffer, size_t *lenp,
>> +                                         loff_t *ppos)
>> +{
>> +       int error;
>> +
>> +       if (write) {
>> +               struct ctl_table table_copy;
>> +               int tmp_mayexec_enforce;
>> +
>> +               if (!capable(CAP_MAC_ADMIN))
>> +                       return -EPERM;
> 
> Don't put capable() checks in sysctls, it doesn't work.
> 

I tested it and the root user can indeed open the file even if the
process doesn't have CAP_MAC_ADMIN, however writing in the sysctl file
is denied. Btw there is a similar check in the previous function
(yama_dointvec_minmax).

Thanks
Jann Horn Jan. 3, 2019, 11:17 a.m. UTC | #4
On Thu, Dec 13, 2018 at 3:49 PM Mickaël Salaün
<mickael.salaun@ssi.gouv.fr> wrote:
> On 12/12/2018 18:09, Jann Horn wrote:
> > On Wed, Dec 12, 2018 at 9:18 AM Mickaël Salaün <mic@digikod.net> wrote:
> >> Enable to either propagate the mount options from the underlying VFS
> >> mount to prevent execution, or to propagate the file execute permission.
> >> This may allow a script interpreter to check execution permissions
> >> before reading commands from a file.
> >>
> >> The main goal is to be able to protect the kernel by restricting
> >> arbitrary syscalls that an attacker could perform with a crafted binary
> >> or certain script languages.  It also improves multilevel isolation
> >> by reducing the ability of an attacker to use side channels with
> >> specific code.  These restrictions can natively be enforced for ELF
> >> binaries (with the noexec mount option) but require this kernel
> >> extension to properly handle scripts (e.g., Python, Perl).
> >>
> >> Add a new sysctl kernel.yama.open_mayexec_enforce to control this
> >> behavior.  A following patch adds documentation.
[...]
> >> +{
> >> +       if (!(mask & MAY_OPENEXEC))
> >> +               return 0;
> >> +       /*
> >> +        * Match regular files and directories to make it easier to
> >> +        * modify script interpreters.
> >> +        */
> >> +       if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> >> +               return 0;
> >
> > So files are subject to checks, but loading code from things like
> > sockets is always fine?
>
> As I said in a previous email, these checks do not handle fifo either.
> This is relevant in a threat model targeting persistent attacks (and
> with additional protections/restrictions). We may want to only whitelist
> fifo, but I don't get how a socket is relevant here. Can you please clarify?

I don't think that there's a security problem here. I just think it's
weird to have the extra check when it seems to me like it isn't really
necessary - nobody is going to want to execute a socket or fifo
anyway, right?

> >
> >> +       if ((open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_MOUNT) &&
> >> +                       !(mask & MAY_EXECMOUNT))
> >> +               return -EACCES;
> >> +
> >> +       /*
> >> +        * May prefer acl_permission_check() instead of generic_permission(),
> >> +        * to not be bypassable with CAP_DAC_READ_SEARCH.
> >> +        */
> >> +       if (open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_FILE)
> >> +               return generic_permission(inode, MAY_EXEC);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>  static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
> >> +       LSM_HOOK_INIT(inode_permission, yama_inode_permission),
> >>         LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
> >>         LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
> >>         LSM_HOOK_INIT(task_prctl, yama_task_prctl),
> >> @@ -447,6 +489,37 @@ static int yama_dointvec_minmax(struct ctl_table *table, int write,
> >>         return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
> >>  }
> >>
> >> +static int yama_dointvec_bitmask_macadmin(struct ctl_table *table, int write,
> >> +                                         void __user *buffer, size_t *lenp,
> >> +                                         loff_t *ppos)
> >> +{
> >> +       int error;
> >> +
> >> +       if (write) {
> >> +               struct ctl_table table_copy;
> >> +               int tmp_mayexec_enforce;
> >> +
> >> +               if (!capable(CAP_MAC_ADMIN))
> >> +                       return -EPERM;
> >
> > Don't put capable() checks in sysctls, it doesn't work.
> >
>
> I tested it and the root user can indeed open the file even if the
> process doesn't have CAP_MAC_ADMIN, however writing in the sysctl file
> is denied. Btw there is a similar check in the previous function
> (yama_dointvec_minmax).

It's still wrong. If an attacker without CAP_MAC_ADMIN opens the
sysctl file, then passes the file descriptor to a setcap binary that
has CAP_MAC_ADMIN as stdout/stderr, and the setcap binary writes to
it, then the capable() check is bypassed. (But of course, to open the
sysctl file in the first place, you'd need to be root (uid 0), so the
check doesn't really matter.)
Mickaël Salaün Jan. 8, 2019, 1:29 p.m. UTC | #5
On 03/01/2019 12:17, Jann Horn wrote:
> On Thu, Dec 13, 2018 at 3:49 PM Mickaël Salaün
> <mickael.salaun@ssi.gouv.fr> wrote:
>> On 12/12/2018 18:09, Jann Horn wrote:
>>> On Wed, Dec 12, 2018 at 9:18 AM Mickaël Salaün <mic@digikod.net> wrote:
>>>> Enable to either propagate the mount options from the underlying VFS
>>>> mount to prevent execution, or to propagate the file execute permission.
>>>> This may allow a script interpreter to check execution permissions
>>>> before reading commands from a file.
>>>>
>>>> The main goal is to be able to protect the kernel by restricting
>>>> arbitrary syscalls that an attacker could perform with a crafted binary
>>>> or certain script languages.  It also improves multilevel isolation
>>>> by reducing the ability of an attacker to use side channels with
>>>> specific code.  These restrictions can natively be enforced for ELF
>>>> binaries (with the noexec mount option) but require this kernel
>>>> extension to properly handle scripts (e.g., Python, Perl).
>>>>
>>>> Add a new sysctl kernel.yama.open_mayexec_enforce to control this
>>>> behavior.  A following patch adds documentation.
> [...]
>>>> +{
>>>> +       if (!(mask & MAY_OPENEXEC))
>>>> +               return 0;
>>>> +       /*
>>>> +        * Match regular files and directories to make it easier to
>>>> +        * modify script interpreters.
>>>> +        */
>>>> +       if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
>>>> +               return 0;
>>>
>>> So files are subject to checks, but loading code from things like
>>> sockets is always fine?
>>
>> As I said in a previous email, these checks do not handle fifo either.
>> This is relevant in a threat model targeting persistent attacks (and
>> with additional protections/restrictions). We may want to only whitelist
>> fifo, but I don't get how a socket is relevant here. Can you please clarify?
> 
> I don't think that there's a security problem here. I just think it's
> weird to have the extra check when it seems to me like it isn't really
> necessary - nobody is going to want to execute a socket or fifo
> anyway, right?

Right, the fifo whitelisting should answer your concern then.

> 
>>>
>>>> +       if ((open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_MOUNT) &&
>>>> +                       !(mask & MAY_EXECMOUNT))
>>>> +               return -EACCES;
>>>> +
>>>> +       /*
>>>> +        * May prefer acl_permission_check() instead of generic_permission(),
>>>> +        * to not be bypassable with CAP_DAC_READ_SEARCH.
>>>> +        */
>>>> +       if (open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_FILE)
>>>> +               return generic_permission(inode, MAY_EXEC);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>  static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
>>>> +       LSM_HOOK_INIT(inode_permission, yama_inode_permission),
>>>>         LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
>>>>         LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
>>>>         LSM_HOOK_INIT(task_prctl, yama_task_prctl),
>>>> @@ -447,6 +489,37 @@ static int yama_dointvec_minmax(struct ctl_table *table, int write,
>>>>         return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>>>>  }
>>>>
>>>> +static int yama_dointvec_bitmask_macadmin(struct ctl_table *table, int write,
>>>> +                                         void __user *buffer, size_t *lenp,
>>>> +                                         loff_t *ppos)
>>>> +{
>>>> +       int error;
>>>> +
>>>> +       if (write) {
>>>> +               struct ctl_table table_copy;
>>>> +               int tmp_mayexec_enforce;
>>>> +
>>>> +               if (!capable(CAP_MAC_ADMIN))
>>>> +                       return -EPERM;
>>>
>>> Don't put capable() checks in sysctls, it doesn't work.
>>>
>>
>> I tested it and the root user can indeed open the file even if the
>> process doesn't have CAP_MAC_ADMIN, however writing in the sysctl file
>> is denied. Btw there is a similar check in the previous function
>> (yama_dointvec_minmax).
> 
> It's still wrong. If an attacker without CAP_MAC_ADMIN opens the
> sysctl file, then passes the file descriptor to a setcap binary that
> has CAP_MAC_ADMIN as stdout/stderr, and the setcap binary writes to
> it, then the capable() check is bypassed. (But of course, to open the
> sysctl file in the first place, you'd need to be root (uid 0), so the
> check doesn't really matter.)

I agree with you that a confused deputy attack may uses file
descriptors, but I don't see how the current sysctl API may be used to
check the process capability at open time. Anyway, on a properly
configured system, especially one leveraging Linux capabilities (e.g.
CLIP OS), root processes may not have CAP_SYS_ADMIN. Moreover, SUID or
fcap binaries may not be available to an attacker (e.g. in a container).
Kees Cook Jan. 8, 2019, 11:30 p.m. UTC | #6
On Tue, Jan 8, 2019 at 5:29 AM Mickaël Salaün
<mickael.salaun@ssi.gouv.fr> wrote:
>
>
> On 03/01/2019 12:17, Jann Horn wrote:
> > On Thu, Dec 13, 2018 at 3:49 PM Mickaël Salaün
> > <mickael.salaun@ssi.gouv.fr> wrote:
> >> On 12/12/2018 18:09, Jann Horn wrote:
> >>> On Wed, Dec 12, 2018 at 9:18 AM Mickaël Salaün <mic@digikod.net> wrote:
> >>>> Enable to either propagate the mount options from the underlying VFS
> >>>> mount to prevent execution, or to propagate the file execute permission.
> >>>> This may allow a script interpreter to check execution permissions
> >>>> before reading commands from a file.
> >>>>
> >>>> The main goal is to be able to protect the kernel by restricting
> >>>> arbitrary syscalls that an attacker could perform with a crafted binary
> >>>> or certain script languages.  It also improves multilevel isolation
> >>>> by reducing the ability of an attacker to use side channels with
> >>>> specific code.  These restrictions can natively be enforced for ELF
> >>>> binaries (with the noexec mount option) but require this kernel
> >>>> extension to properly handle scripts (e.g., Python, Perl).

I like this idea, but I think it shouldn't live in Yama (since it is
currently intended to be a ptrace-policy-only LSM). It was
_originally_ designed to do various DAC improvements, but the
agreement was that those should live directly in the VFS instead (i.e.
the symlink, hardlink and now fifo and regular file defenses).

This should likely go in similarly. (But if not, it could also be its own LSM.)
Mickaël Salaün Jan. 9, 2019, 1:41 p.m. UTC | #7
On 09/01/2019 00:30, Kees Cook wrote:
> On Tue, Jan 8, 2019 at 5:29 AM Mickaël Salaün
> <mickael.salaun@ssi.gouv.fr> wrote:
>>
>>
>> On 03/01/2019 12:17, Jann Horn wrote:
>>> On Thu, Dec 13, 2018 at 3:49 PM Mickaël Salaün
>>> <mickael.salaun@ssi.gouv.fr> wrote:
>>>> On 12/12/2018 18:09, Jann Horn wrote:
>>>>> On Wed, Dec 12, 2018 at 9:18 AM Mickaël Salaün <mic@digikod.net> wrote:
>>>>>> Enable to either propagate the mount options from the underlying VFS
>>>>>> mount to prevent execution, or to propagate the file execute permission.
>>>>>> This may allow a script interpreter to check execution permissions
>>>>>> before reading commands from a file.
>>>>>>
>>>>>> The main goal is to be able to protect the kernel by restricting
>>>>>> arbitrary syscalls that an attacker could perform with a crafted binary
>>>>>> or certain script languages.  It also improves multilevel isolation
>>>>>> by reducing the ability of an attacker to use side channels with
>>>>>> specific code.  These restrictions can natively be enforced for ELF
>>>>>> binaries (with the noexec mount option) but require this kernel
>>>>>> extension to properly handle scripts (e.g., Python, Perl).
> 
> I like this idea, but I think it shouldn't live in Yama (since it is
> currently intended to be a ptrace-policy-only LSM). It was
> _originally_ designed to do various DAC improvements, but the
> agreement was that those should live directly in the VFS instead (i.e.
> the symlink, hardlink and now fifo and regular file defenses).
> 
> This should likely go in similarly. (But if not, it could also be its own LSM.)
> 

I think that Yama is quite handy and make sense here, but I'm fine
putting this knob elsewhere. However, I was thinking, for a future patch
series, to add another sysctl to lock this choice, i.e. generalizing the
way Yama can lock the ptrace_scope.

What matter here is the ability for an LSM to use this O_MAYEXEC flag.
Yama is a good place to showcase this feature and I think it is cleaner
to leverage the LSM framework to put new (optional) security features. I
can easily create a new LSM but it would be pretty similar to Yama...
What do you think about it James and Al?

Side question: wouldn't it be better to use a 0600 mode (instead of
0644) for this kind of sysctl?

Patch
diff mbox series

diff --git a/security/yama/Kconfig b/security/yama/Kconfig
index 96b27405558a..9457619fabd5 100644
--- a/security/yama/Kconfig
+++ b/security/yama/Kconfig
@@ -5,7 +5,8 @@  config SECURITY_YAMA
 	help
 	  This selects Yama, which extends DAC support with additional
 	  system-wide security settings beyond regular Linux discretionary
-	  access controls. Currently available is ptrace scope restriction.
+	  access controls. Currently available are ptrace scope restriction and
+	  enforcement of the O_MAYEXEC open flag.
 	  Like capabilities, this security module stacks with other LSMs.
 	  Further information can be found in
 	  Documentation/admin-guide/LSM/Yama.rst.
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index ffda91a4a1aa..120664e94ee5 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -1,10 +1,12 @@ 
 /*
  * Yama Linux Security Module
  *
- * Author: Kees Cook <keescook@chromium.org>
+ * Authors: Kees Cook <keescook@chromium.org>
+ *          Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
  *
  * Copyright (C) 2010 Canonical, Ltd.
  * Copyright (C) 2011 The Chromium OS Authors.
+ * Copyright (C) 2018 ANSSI
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2, as
@@ -28,7 +30,14 @@ 
 #define YAMA_SCOPE_CAPABILITY	2
 #define YAMA_SCOPE_NO_ATTACH	3
 
+#define YAMA_OMAYEXEC_ENFORCE_NONE	0
+#define YAMA_OMAYEXEC_ENFORCE_MOUNT	(1 << 0)
+#define YAMA_OMAYEXEC_ENFORCE_FILE	(1 << 1)
+#define _YAMA_OMAYEXEC_LAST		YAMA_OMAYEXEC_ENFORCE_FILE
+#define _YAMA_OMAYEXEC_MASK		((_YAMA_OMAYEXEC_LAST << 1) - 1)
+
 static int ptrace_scope = YAMA_SCOPE_RELATIONAL;
+static int open_mayexec_enforce = YAMA_OMAYEXEC_ENFORCE_NONE;
 
 /* describe a ptrace relationship for potential exception */
 struct ptrace_relation {
@@ -423,7 +432,40 @@  int yama_ptrace_traceme(struct task_struct *parent)
 	return rc;
 }
 
+/**
+ * yama_inode_permission - check O_MAYEXEC permission before accessing an inode
+ * @inode: inode structure to check
+ * @mask: permission mask
+ *
+ * Return 0 if access is permitted, -EACCES otherwise.
+ */
+int yama_inode_permission(struct inode *inode, int mask)
+{
+	if (!(mask & MAY_OPENEXEC))
+		return 0;
+	/*
+	 * Match regular files and directories to make it easier to
+	 * modify script interpreters.
+	 */
+	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
+		return 0;
+
+	if ((open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_MOUNT) &&
+			!(mask & MAY_EXECMOUNT))
+		return -EACCES;
+
+	/*
+	 * May prefer acl_permission_check() instead of generic_permission(),
+	 * to not be bypassable with CAP_DAC_READ_SEARCH.
+	 */
+	if (open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_FILE)
+		return generic_permission(inode, MAY_EXEC);
+
+	return 0;
+}
+
 static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(inode_permission, yama_inode_permission),
 	LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
 	LSM_HOOK_INIT(task_prctl, yama_task_prctl),
@@ -447,6 +489,37 @@  static int yama_dointvec_minmax(struct ctl_table *table, int write,
 	return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
 }
 
+static int yama_dointvec_bitmask_macadmin(struct ctl_table *table, int write,
+					  void __user *buffer, size_t *lenp,
+					  loff_t *ppos)
+{
+	int error;
+
+	if (write) {
+		struct ctl_table table_copy;
+		int tmp_mayexec_enforce;
+
+		if (!capable(CAP_MAC_ADMIN))
+			return -EPERM;
+		tmp_mayexec_enforce = *((int *)table->data);
+		table_copy = *table;
+		/* do not erase open_mayexec_enforce */
+		table_copy.data = &tmp_mayexec_enforce;
+		error = proc_dointvec(&table_copy, write, buffer, lenp, ppos);
+		if (error)
+			return error;
+		if ((tmp_mayexec_enforce | _YAMA_OMAYEXEC_MASK) !=
+				_YAMA_OMAYEXEC_MASK)
+			return -EINVAL;
+		*((int *)table->data) = tmp_mayexec_enforce;
+	} else {
+		error = proc_dointvec(table, write, buffer, lenp, ppos);
+		if (error)
+			return error;
+	}
+	return 0;
+}
+
 static int zero;
 static int max_scope = YAMA_SCOPE_NO_ATTACH;
 
@@ -466,6 +539,13 @@  static struct ctl_table yama_sysctl_table[] = {
 		.extra1         = &zero,
 		.extra2         = &max_scope,
 	},
+	{
+		.procname       = "open_mayexec_enforce",
+		.data           = &open_mayexec_enforce,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = yama_dointvec_bitmask_macadmin,
+	},
 	{ }
 };
 static void __init yama_init_sysctl(void)