[RFC,1/5] path_fchdir and path_fhandle LSM hooks
diff mbox

Message ID 1469777680-3687-2-git-send-email-elena.reshetova@intel.com
State New
Headers show

Commit Message

Reshetova, Elena July 29, 2016, 7:34 a.m. UTC
This introduces two new LSM hooks operating on paths.

  - security_path_fchdir() checks for permission on
    changing working directory. It can be used by
    LSMs concerned on fchdir system call
  - security_path_fhandle() checks for permission
    before converting file handle to path. It can be
    used by LSMs concerned with file handle transfers

Both hooks are under CONFIG_SECURITY_PATH.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 fs/fhandle.c              |  5 +++++
 fs/open.c                 |  3 +++
 include/linux/lsm_hooks.h | 12 ++++++++++++
 include/linux/security.h  | 13 +++++++++++++
 security/security.c       | 11 +++++++++++
 5 files changed, 44 insertions(+)

Comments

Jann Horn July 29, 2016, 6:12 p.m. UTC | #1
On Fri, Jul 29, 2016 at 10:34:36AM +0300, Elena Reshetova wrote:
> This introduces two new LSM hooks operating on paths.
> 
>   - security_path_fchdir() checks for permission on
>     changing working directory. It can be used by
>     LSMs concerned on fchdir system call

I don't think security_path_fchdir() is a good abstraction
level. It neither covers the whole case of "cwd is changed" nor does
it cover the whole case of "someone uses a file descriptor to a
directory to look up stuff outside that directory".

For example, security_path_fchdir() seems to be intended to prevent
the use of a leaked file descriptor to the outside world for accessing
other files in the outside world. But this is trivially bypassed by
first using openat() directly instead of fchdir()+open() (something
that used to work against grsecurity, but was fixed quite a while
ago).
Reshetova, Elena July 31, 2016, 10:55 a.m. UTC | #2
On Fri, Jul 29, 2016 at 10:34:36AM +0300, Elena Reshetova wrote:
> This introduces two new LSM hooks operating on paths.
> 
>   - security_path_fchdir() checks for permission on
>     changing working directory. It can be used by
>     LSMs concerned on fchdir system call

>I don't think security_path_fchdir() is a good abstraction level. It
neither covers the whole case of "cwd is changed" nor does it cover the
whole case of "someone uses a file descriptor to a directory to look up
stuff outside that directory".
Do you have a suggestion on what can be a good place? 

>For example, security_path_fchdir() seems to be intended to prevent the use
of a leaked file descriptor to the outside world for accessing other files
in the outside world. 
Yes, this was exactly the use case.

>But this is trivially bypassed by first using openat() directly instead of
fchdir()+open() (something that used to work against grsecurity, but was
fixed quite a while ago).
The way it has been addressed in grsecurity is having a check inside
filename_lookup() , but it doesn't look a very great place for putting a
hook. I was thinking about it , but so far didn't find any other good
alternatives.
Jann Horn July 31, 2016, 12:02 p.m. UTC | #3
On Sun, Jul 31, 2016 at 10:55:04AM +0000, Reshetova, Elena wrote:
> On Fri, Jul 29, 2016 at 10:34:36AM +0300, Elena Reshetova wrote:
> > This introduces two new LSM hooks operating on paths.
> > 
> >   - security_path_fchdir() checks for permission on
> >     changing working directory. It can be used by
> >     LSMs concerned on fchdir system call
> 
> >I don't think security_path_fchdir() is a good abstraction level. It
> neither covers the whole case of "cwd is changed" nor does it cover the
> whole case of "someone uses a file descriptor to a directory to look up
> stuff outside that directory".
> Do you have a suggestion on what can be a good place? 
> 
> >For example, security_path_fchdir() seems to be intended to prevent the use
> of a leaked file descriptor to the outside world for accessing other files
> in the outside world. 
> Yes, this was exactly the use case.
> 
> >But this is trivially bypassed by first using openat() directly instead of
> fchdir()+open() (something that used to work against grsecurity, but was
> fixed quite a while ago).
> The way it has been addressed in grsecurity is having a check inside
> filename_lookup() , but it doesn't look a very great place for putting a
> hook. I was thinking about it , but so far didn't find any other good
> alternatives. 

Yeah, if you want to have such a hook, I think it needs to be in
filename_lookup() or below - but that's a relatively hot function, so it
might have a measurable performance impact.

Alternatively, you could forbid double-chroots and use the LSM hooks for
file descriptor passing via unix domain sockets and binder to check
incoming file descriptors. As long as no directories are moved out of the
chroot directory by something outside the chroot (a process chrooted into
a parent directory or a process that isn't chrooted), that should
prooobably work?
Reshetova, Elena July 31, 2016, 6:28 p.m. UTC | #4
On Sun, Jul 31, 2016 at 10:55:04AM +0000, Reshetova, Elena wrote:
> On Fri, Jul 29, 2016 at 10:34:36AM +0300, Elena Reshetova wrote:
> > This introduces two new LSM hooks operating on paths.
> > 
> >   - security_path_fchdir() checks for permission on
> >     changing working directory. It can be used by
> >     LSMs concerned on fchdir system call
> 
> >I don't think security_path_fchdir() is a good abstraction level. It
> neither covers the whole case of "cwd is changed" nor does it cover 
> the whole case of "someone uses a file descriptor to a directory to 
> look up stuff outside that directory".
> Do you have a suggestion on what can be a good place? 
> 
> >For example, security_path_fchdir() seems to be intended to prevent 
> >the use
> of a leaked file descriptor to the outside world for accessing other 
> files in the outside world.
> Yes, this was exactly the use case.
> 
> >But this is trivially bypassed by first using openat() directly 
> >instead of
> fchdir()+open() (something that used to work against grsecurity, but 
> was fixed quite a while ago).
> The way it has been addressed in grsecurity is having a check inside
> filename_lookup() , but it doesn't look a very great place for putting 
> a hook. I was thinking about it , but so far didn't find any other 
> good alternatives.

>Yeah, if you want to have such a hook, I think it needs to be in
>filename_lookup() or below - but that's a relatively hot function, so it
might have a measurable performance impact.

Yes, it is way too much used from everywhere and doesn't even fit into LSM
design...

>Alternatively, you could forbid double-chroots and use the LSM hooks for
file descriptor passing via unix domain sockets and binder to check incoming
file descriptors.

This would not prevent guessing the file descriptor unfortunately. I was
planning to continue on chroot features and like grsecurity have an option
to disable unix domain sockets outside of chroot. Binder is a different
story since it is Android specific and we don't expect it to be used on
normal Linux system. And on Android nobody uses chroot, so I don't think
there is a use case for this.  

 >As long as no directories are moved out of the chroot directory by
something outside the chroot (a process chrooted into a parent directory or
a process that isn't chrooted), that should prooobably work?
Yes, I think this might work for given use case since we are not expecting
processes outside of chroot to help exploited process inside chroot to
escape. But guessable file descriptor is still an issue (not sure how real
is this issue in practice nowadays).
Jann Horn July 31, 2016, 9:23 p.m. UTC | #5
On Sun, Jul 31, 2016 at 06:28:08PM +0000, Reshetova, Elena wrote:
> On Sun, Jul 31, 2016 at 10:55:04AM +0000, Reshetova, Elena wrote:
[...]
> >Alternatively, you could forbid double-chroots and use the LSM hooks for
> file descriptor passing via unix domain sockets and binder to check incoming
> file descriptors.
> 
> This would not prevent guessing the file descriptor unfortunately.

That doesn't make sense to me. Can you elaborate on that, please?

How would you "guess" a file descriptor? Are you talking about file
descriptors opened before chroot() that have been leaked accidentally?
In that case, you could just do on chroot() what SELinux does on a domain
transition and replace all dangerous open file descriptors with /dev/null.

Or are you concerned about shared file descriptor tables (which really
shouldn't happen accidentally, at least when you keep in mind that for
this to be an issue, the fs_struct would have to not be shared)?
Reshetova, Elena Aug. 1, 2016, 8:38 a.m. UTC | #6
>On Sun, Jul 31, 2016 at 06:28:08PM +0000, Reshetova, Elena wrote:
> On Sun, Jul 31, 2016 at 10:55:04AM +0000, Reshetova, Elena wrote:
[...]
> >Alternatively, you could forbid double-chroots and use the LSM hooks 
> >for
> file descriptor passing via unix domain sockets and binder to check 
> incoming file descriptors.
> 
> This would not prevent guessing the file descriptor unfortunately.

>That doesn't make sense to me. Can you elaborate on that, please?

>How would you "guess" a file descriptor? Are you talking about file
descriptors opened before chroot() that have been leaked accidentally?

Yes, these ones. Also I guess in general security-wise it is better approach
to have a check in a place where descriptor will be attempted to
use/resolved vs. trying to make sure you caught all cases where/how process
might obtain some. 
Various IPC, leaked descriptors, some other potential surprises... But I
think in this case it might be worth trying to do what you suggest since I
don't see good alternatives either. 

>In that case, you could just do on chroot() what SELinux does on a domain
transition and replace all dangerous open file descriptors with /dev/null.

I guess this could work, if I can correctly close the ones that are outside
of the chroot. I will check how SELinux does it. Thank you for the tip! 

>Or are you concerned about shared file descriptor tables (which really
shouldn't happen accidentally, 

You mean CLONE_FILES on clone()? If yes, then I am less concerned of this
since it really not common as far as I understood for legitimate
processes/daemons to be started this way. However, if this case needs to be
addressed, it is trickier, you cannot just substitute these ones with
/dev/null without breaking the parent also and you would need to check them
all, not just opened ones. 

> at least when you keep in mind that for this to be an issue, the fs_struct
would have to not be shared)?

What do you mean by the last part? Not sure I understand here...

Patch
diff mbox

diff --git a/fs/fhandle.c b/fs/fhandle.c
index ca3c3dd..6e8aba5 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -8,6 +8,7 @@ 
 #include <linux/fs_struct.h>
 #include <linux/fsnotify.h>
 #include <linux/personality.h>
+#include <linux/security.h>
 #include <asm/uaccess.h>
 #include "internal.h"
 #include "mount.h"
@@ -179,6 +180,10 @@  static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 		retval = -EPERM;
 		goto out_err;
 	}
+	retval = security_path_fhandle(path);
+	if (retval)
+		goto out_err;
+
 	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) {
 		retval = -EFAULT;
 		goto out_err;
diff --git a/fs/open.c b/fs/open.c
index 93ae3cd..9c260d4 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -458,6 +458,9 @@  SYSCALL_DEFINE1(fchdir, unsigned int, fd)
 		goto out_putf;
 
 	error = inode_permission(inode, MAY_EXEC | MAY_CHDIR);
+	if (error)
+		goto out_putf;
+	error = security_path_fchdir(&f.file->f_path);
 	if (!error)
 		set_fs_pwd(current->fs, &f.file->f_path);
 out_putf:
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 7ae3976..25164b6 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -308,6 +308,14 @@ 
  *	Check for permission to change root directory.
  *	@path contains the path structure.
  *	Return 0 if permission is granted.
+ * @path_fchdir:
+ *	Check for permission to change working directory.
+ *	@path contains the path structure.
+ *	Return 0 if permission is granted.
+ * @path_fhandle:
+ *	Check for permission to convert handle to path.
+ *	@path contains the path structure.
+ *	Return 0 if permission is granted.
  * @inode_readlink:
  *	Check the permission to read the symbolic link.
  *	@dentry contains the dentry structure for the file link.
@@ -1378,6 +1386,8 @@  union security_list_options {
 	int (*path_chmod)(const struct path *path, umode_t mode);
 	int (*path_chown)(const struct path *path, kuid_t uid, kgid_t gid);
 	int (*path_chroot)(const struct path *path);
+	int (*path_fchdir)(const struct path *path);
+	int (*path_fhandle)(const struct path *path);
 #endif
 
 	int (*inode_alloc_security)(struct inode *inode);
@@ -1668,6 +1678,8 @@  struct security_hook_heads {
 	struct list_head path_chmod;
 	struct list_head path_chown;
 	struct list_head path_chroot;
+	struct list_head path_fchdir;
+	struct list_head path_fhandle;
 #endif
 	struct list_head inode_alloc_security;
 	struct list_head inode_free_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index 14df373..6745c06 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1472,6 +1472,9 @@  int security_path_rename(const struct path *old_dir, struct dentry *old_dentry,
 int security_path_chmod(const struct path *path, umode_t mode);
 int security_path_chown(const struct path *path, kuid_t uid, kgid_t gid);
 int security_path_chroot(const struct path *path);
+int security_path_fchdir(const struct path *path);
+int security_path_fhandle(const struct path *path);
+
 #else	/* CONFIG_SECURITY_PATH */
 static inline int security_path_unlink(const struct path *dir, struct dentry *dentry)
 {
@@ -1536,6 +1539,16 @@  static inline int security_path_chroot(const struct path *path)
 {
 	return 0;
 }
+
+static inline int security_path_fchdir(const struct path *path)
+{
+	return 0;
+}
+
+static inline int security_path_fhandle(const struct path *path)
+{
+	return 0;
+}
 #endif	/* CONFIG_SECURITY_PATH */
 
 #ifdef CONFIG_KEYS
diff --git a/security/security.c b/security/security.c
index 7095693..cd82276 100644
--- a/security/security.c
+++ b/security/security.c
@@ -504,6 +504,15 @@  int security_path_chroot(const struct path *path)
 {
 	return call_int_hook(path_chroot, 0, path);
 }
+
+int security_path_fchdir(const struct path *path)
+{
+	return call_int_hook(path_fchdir, 0, path);
+}
+int security_path_fhandle(const struct path *path)
+{
+	return call_int_hook(path_fhandle, 0, path);
+}
 #endif
 
 int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
@@ -1615,6 +1624,8 @@  struct security_hook_heads security_hook_heads = {
 	.path_chmod =	LIST_HEAD_INIT(security_hook_heads.path_chmod),
 	.path_chown =	LIST_HEAD_INIT(security_hook_heads.path_chown),
 	.path_chroot =	LIST_HEAD_INIT(security_hook_heads.path_chroot),
+	.path_fchdir =	LIST_HEAD_INIT(security_hook_heads.path_fchdir),
+	.path_fhandle =	LIST_HEAD_INIT(security_hook_heads.path_fhandle),
 #endif
 	.inode_alloc_security =
 		LIST_HEAD_INIT(security_hook_heads.inode_alloc_security),