[RFC,v1,1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
diff mbox series

Message ID 20181212081712.32347-2-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
When the O_MAYEXEC flag is passed, sys_open() may be subject to
additional restrictions depending on a security policy implemented by an
LSM through the inode_permission hook.

The underlying idea is to be able to restrict scripts interpretation
according to a policy defined by the system administrator.  For this to
be possible, script interpreters must use the O_MAYEXEC flag
appropriately.  To be fully effective, these interpreters also need to
handle the other ways to execute code (for which the kernel can't help):
command line parameters (e.g., option -e for Perl), module loading
(e.g., option -m for Python), stdin, file sourcing, environment
variables, configuration files...  According to the threat model, it may
be acceptable to allow some script interpreters (e.g. Bash) to interpret
commands from stdin, may it be a TTY or a pipe, because it may not be
enough to (directly) perform syscalls.

A simple security policy implementation is available in a following
patch for Yama.

This is an updated subset of the patch initially written by Vincent
Strubel for CLIP OS:
https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
This patch has been used for more than 10 years with customized script
interpreters.  Some examples can be found here:
https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
---
 fs/fcntl.c                       | 2 +-
 fs/open.c                        | 4 ++++
 include/linux/fcntl.h            | 2 +-
 include/linux/fs.h               | 2 ++
 include/uapi/asm-generic/fcntl.h | 3 +++
 5 files changed, 11 insertions(+), 2 deletions(-)

Comments

Jan Kara Dec. 12, 2018, 2:43 p.m. UTC | #1
On Wed 12-12-18 09:17:08, Mickaël Salaün wrote:
> When the O_MAYEXEC flag is passed, sys_open() may be subject to
> additional restrictions depending on a security policy implemented by an
> LSM through the inode_permission hook.
> 
> The underlying idea is to be able to restrict scripts interpretation
> according to a policy defined by the system administrator.  For this to
> be possible, script interpreters must use the O_MAYEXEC flag
> appropriately.  To be fully effective, these interpreters also need to
> handle the other ways to execute code (for which the kernel can't help):
> command line parameters (e.g., option -e for Perl), module loading
> (e.g., option -m for Python), stdin, file sourcing, environment
> variables, configuration files...  According to the threat model, it may
> be acceptable to allow some script interpreters (e.g. Bash) to interpret
> commands from stdin, may it be a TTY or a pipe, because it may not be
> enough to (directly) perform syscalls.
> 
> A simple security policy implementation is available in a following
> patch for Yama.
> 
> This is an updated subset of the patch initially written by Vincent
> Strubel for CLIP OS:
> https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> This patch has been used for more than 10 years with customized script
> interpreters.  Some examples can be found here:
> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
> Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>

...

> diff --git a/fs/open.c b/fs/open.c
> index 0285ce7dbd51..75479b79a58f 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -974,6 +974,10 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
>  	if (flags & O_APPEND)
>  		acc_mode |= MAY_APPEND;
>  
> +	/* Check execution permissions on open. */
> +	if (flags & O_MAYEXEC)
> +		acc_mode |= MAY_OPENEXEC;
> +
>  	op->acc_mode = acc_mode;
>  
>  	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;

I don't feel experienced enough in security to tell whether we want this
functionality or not. But if we do this, shouldn't we also set FMODE_EXEC
on the resulting struct file? That way also security_file_open() can be
used to arbitrate such executable opens and in particular
fanotify permission event FAN_OPEN_EXEC will get properly generated which I
guess is desirable (support for it is sitting in my tree waiting for the
merge window) - adding some audit people involved in FAN_OPEN_EXEC to
CC. Just an idea...

								Honza
Mickaël Salaün Dec. 12, 2018, 5:09 p.m. UTC | #2
Le 12/12/2018 à 15:43, Jan Kara a écrit :
> On Wed 12-12-18 09:17:08, Mickaël Salaün wrote:
>> When the O_MAYEXEC flag is passed, sys_open() may be subject to
>> additional restrictions depending on a security policy implemented by an
>> LSM through the inode_permission hook.
>>
>> The underlying idea is to be able to restrict scripts interpretation
>> according to a policy defined by the system administrator.  For this to
>> be possible, script interpreters must use the O_MAYEXEC flag
>> appropriately.  To be fully effective, these interpreters also need to
>> handle the other ways to execute code (for which the kernel can't help):
>> command line parameters (e.g., option -e for Perl), module loading
>> (e.g., option -m for Python), stdin, file sourcing, environment
>> variables, configuration files...  According to the threat model, it may
>> be acceptable to allow some script interpreters (e.g. Bash) to interpret
>> commands from stdin, may it be a TTY or a pipe, because it may not be
>> enough to (directly) perform syscalls.
>>
>> A simple security policy implementation is available in a following
>> patch for Yama.
>>
>> This is an updated subset of the patch initially written by Vincent
>> Strubel for CLIP OS:
>> https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
>> This patch has been used for more than 10 years with customized script
>> interpreters.  Some examples can be found here:
>> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
>> Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
>> Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
> 
> ...
> 
>> diff --git a/fs/open.c b/fs/open.c
>> index 0285ce7dbd51..75479b79a58f 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -974,6 +974,10 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
>>  	if (flags & O_APPEND)
>>  		acc_mode |= MAY_APPEND;
>>  
>> +	/* Check execution permissions on open. */
>> +	if (flags & O_MAYEXEC)
>> +		acc_mode |= MAY_OPENEXEC;
>> +
>>  	op->acc_mode = acc_mode;
>>  
>>  	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
> 
> I don't feel experienced enough in security to tell whether we want this
> functionality or not. But if we do this, shouldn't we also set FMODE_EXEC
> on the resulting struct file? That way also security_file_open() can be
> used to arbitrate such executable opens and in particular
> fanotify permission event FAN_OPEN_EXEC will get properly generated which I
> guess is desirable (support for it is sitting in my tree waiting for the
> merge window) - adding some audit people involved in FAN_OPEN_EXEC to
> CC. Just an idea...

Indeed, it may be useful for other LSM.

 Mickaël
Mimi Zohar Dec. 12, 2018, 8:42 p.m. UTC | #3
On Wed, 2018-12-12 at 15:43 +0100, Jan Kara wrote:


> > diff --git a/fs/open.c b/fs/open.c
> > index 0285ce7dbd51..75479b79a58f 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -974,6 +974,10 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
> >  	if (flags & O_APPEND)
> >  		acc_mode |= MAY_APPEND;
> >  
> > +	/* Check execution permissions on open. */
> > +	if (flags & O_MAYEXEC)
> > +		acc_mode |= MAY_OPENEXEC;
> > +
> >  	op->acc_mode = acc_mode;
> >  
> >  	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
> 
> I don't feel experienced enough in security to tell whether we want this
> functionality or not. But if we do this, shouldn't we also set FMODE_EXEC
> on the resulting struct file? That way also security_file_open() can be
> used to arbitrate such executable opens and in particular
> fanotify permission event FAN_OPEN_EXEC will get properly generated which I
> guess is desirable (support for it is sitting in my tree waiting for the
> merge window) - adding some audit people involved in FAN_OPEN_EXEC to
> CC. Just an idea...

Assuming the interpreters are properly modified (and signed),
MAY_OPENEXEC closes a major IMA measurement/appraisal gap.  The kernel
has no insight into the files that the interpreter is opening.  Having
the interpreter annotate the file open, allows IMA to differentiate
scripts opening data files from code.

IMA policy rules could then be written requiring code to be signed.

Example IMA policy rules:
measure func=FILE_CHECK mask=MAY_OPENEXEC
appraise func=FILE_CHECK mask=MAY_OPENEXEC appraise_type=imasig

Mimi
Matthew Bobrowski Dec. 13, 2018, 9:47 a.m. UTC | #4
On Wed, Dec 12, 2018 at 03:43:06PM +0100, Jan Kara wrote:
> > When the O_MAYEXEC flag is passed, sys_open() may be subject to
> > additional restrictions depending on a security policy implemented by an
> > LSM through the inode_permission hook.
> > 
> > The underlying idea is to be able to restrict scripts interpretation
> > according to a policy defined by the system administrator.  For this to
> > be possible, script interpreters must use the O_MAYEXEC flag
> > appropriately.  To be fully effective, these interpreters also need to
> > handle the other ways to execute code (for which the kernel can't help):
> > command line parameters (e.g., option -e for Perl), module loading
> > (e.g., option -m for Python), stdin, file sourcing, environment
> > variables, configuration files...  According to the threat model, it may
> > be acceptable to allow some script interpreters (e.g. Bash) to interpret
> > commands from stdin, may it be a TTY or a pipe, because it may not be
> > enough to (directly) perform syscalls.
> > 
> > A simple security policy implementation is available in a following
> > patch for Yama.
> > 
> > This is an updated subset of the patch initially written by Vincent
> > Strubel for CLIP OS:
> > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > This patch has been used for more than 10 years with customized script
> > interpreters.  Some examples can be found here:
> > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> > 
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> > Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
> > Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
> 
> ...
> 
> > diff --git a/fs/open.c b/fs/open.c
> > index 0285ce7dbd51..75479b79a58f 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -974,6 +974,10 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
> >  	if (flags & O_APPEND)
> >  		acc_mode |= MAY_APPEND;
> >  
> > +	/* Check execution permissions on open. */
> > +	if (flags & O_MAYEXEC)
> > +		acc_mode |= MAY_OPENEXEC;
> > +
> >  	op->acc_mode = acc_mode;
> >  
> >  	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
> 
> I don't feel experienced enough in security to tell whether we want this
> functionality or not. But if we do this, shouldn't we also set FMODE_EXEC
> on the resulting struct file? That way also security_file_open() can be
> used to arbitrate such executable opens and in particular
> fanotify permission event FAN_OPEN_EXEC will get properly generated which I
> guess is desirable (support for it is sitting in my tree waiting for the
> merge window) - adding some audit people involved in FAN_OPEN_EXEC to
> CC. Just an idea...

If I'm understanding this patch series correctly, without an enforced LSM
policy there's realistically no added benefit from a security perspective,
right? Also, I'm in agreement with what Jan has mentioned in regards to setting
the __FMODE_EXEC flag when O_MAYEXEC has been specified. This is something that
would work quite nicely in conjunction with some of the new file access
notification events.

Rather than setting it on the resulting struct file, couldn't they simply
incorporate it as part of op->open_flags when flags & O_MAYEXEC? Not entirely
sure whether this is what you actually meant or not though? Pretty much the
same as a call to exec(2)/execat(2) when it builds its open_flags.
Mickaël Salaün Dec. 13, 2018, 2:23 p.m. UTC | #5
On 13/12/2018 10:47, Matthew Bobrowski wrote:
> On Wed, Dec 12, 2018 at 03:43:06PM +0100, Jan Kara wrote:
>>> When the O_MAYEXEC flag is passed, sys_open() may be subject to
>>> additional restrictions depending on a security policy implemented by an
>>> LSM through the inode_permission hook.
>>>
>>> The underlying idea is to be able to restrict scripts interpretation
>>> according to a policy defined by the system administrator.  For this to
>>> be possible, script interpreters must use the O_MAYEXEC flag
>>> appropriately.  To be fully effective, these interpreters also need to
>>> handle the other ways to execute code (for which the kernel can't help):
>>> command line parameters (e.g., option -e for Perl), module loading
>>> (e.g., option -m for Python), stdin, file sourcing, environment
>>> variables, configuration files...  According to the threat model, it may
>>> be acceptable to allow some script interpreters (e.g. Bash) to interpret
>>> commands from stdin, may it be a TTY or a pipe, because it may not be
>>> enough to (directly) perform syscalls.
>>>
>>> A simple security policy implementation is available in a following
>>> patch for Yama.
>>>
>>> This is an updated subset of the patch initially written by Vincent
>>> Strubel for CLIP OS:
>>> https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
>>> This patch has been used for more than 10 years with customized script
>>> interpreters.  Some examples can be found here:
>>> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
>>>
>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
>>> Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
>>> Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
>>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
>>
>> ...
>>
>>> diff --git a/fs/open.c b/fs/open.c
>>> index 0285ce7dbd51..75479b79a58f 100644
>>> --- a/fs/open.c
>>> +++ b/fs/open.c
>>> @@ -974,6 +974,10 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
>>>  	if (flags & O_APPEND)
>>>  		acc_mode |= MAY_APPEND;
>>>  
>>> +	/* Check execution permissions on open. */
>>> +	if (flags & O_MAYEXEC)
>>> +		acc_mode |= MAY_OPENEXEC;
>>> +
>>>  	op->acc_mode = acc_mode;
>>>  
>>>  	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
>>
>> I don't feel experienced enough in security to tell whether we want this
>> functionality or not. But if we do this, shouldn't we also set FMODE_EXEC
>> on the resulting struct file? That way also security_file_open() can be
>> used to arbitrate such executable opens and in particular
>> fanotify permission event FAN_OPEN_EXEC will get properly generated which I
>> guess is desirable (support for it is sitting in my tree waiting for the
>> merge window) - adding some audit people involved in FAN_OPEN_EXEC to
>> CC. Just an idea...
> 
> If I'm understanding this patch series correctly, without an enforced LSM
> policy there's realistically no added benefit from a security perspective,
> right?

That's correct. The kernel knows the semantic but the enforcement is
delegated to an LSM and its policy.

> Also, I'm in agreement with what Jan has mentioned in regards to setting
> the __FMODE_EXEC flag when O_MAYEXEC has been specified. This is something that
> would work quite nicely in conjunction with some of the new file access
> notification events.

OK, I will add it in the next patch series (for the new FAN_OPEN_EXEC
support).

Patch
diff mbox series

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 083185174c6d..6c85c4d0c006 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@  static int __init fcntl_init(void)
 	 * Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
 	 * is defined as O_NONBLOCK on some platforms and not on others.
 	 */
-	BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+	BUILD_BUG_ON(22 - 1 /* for O_RDONLY being 0 */ !=
 		HWEIGHT32(
 			(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
 			__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..75479b79a58f 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -974,6 +974,10 @@  static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
 	if (flags & O_APPEND)
 		acc_mode |= MAY_APPEND;
 
+	/* Check execution permissions on open. */
+	if (flags & O_MAYEXEC)
+		acc_mode |= MAY_OPENEXEC;
+
 	op->acc_mode = acc_mode;
 
 	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..1fc00cabe9ab 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -9,7 +9,7 @@ 
 	(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
 	 O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
 	 FASYNC	| O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
-	 O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+	 O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_MAYEXEC)
 
 #ifndef force_o_largefile
 #define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c95c0807471f..584c9329ad78 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -94,6 +94,8 @@  typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 #define MAY_CHDIR		0x00000040
 /* called from RCU mode, don't block */
 #define MAY_NOT_BLOCK		0x00000080
+/* the inode is opened with O_MAYEXEC */
+#define MAY_OPENEXEC		0x00000100
 
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..cbb9425d6e7c 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -97,6 +97,9 @@ 
 #define O_NDELAY	O_NONBLOCK
 #endif
 
+/* command execution from file is intended, check exec permissions */
+#define O_MAYEXEC	040000000
+
 #define F_DUPFD		0	/* dup */
 #define F_GETFD		1	/* get close_on_exec */
 #define F_SETFD		2	/* set/clear close_on_exec */