How to handle security_dentry_open when the open flags are 'special'

How to handle security_dentry_open when the open flags are 'special'

[Eric Paris]

See LKML posting http://marc.info/?l=linux-kernel&m=120534660832189&w=2

according to viro a call to sys_open like: open("/dev/null", 3) is
supposed to mean: check read and write but not set FMODE_READ or
FMODE_WRITE.  This is exactly the security check we get through the
security_inode_permission hook but not what happens through the
security_dentry_open hook.  This patch actually passes the bare flags
into security_dentry_open so that we can do the correct permission
checking there.

What do people think?

Untested, uncompiled, just thoughts and fleshing it out....

-Eric

 fs/open.c                |    2 +-
 include/linux/security.h |    6 +++---
 security/security.c      |    4 ++--
 security/selinux/hooks.c |   20 ++++++++++++++++++--
 4 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 5419853..2560a3e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -754,7 +754,7 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 	f->f_op = fops_get(inode->i_fop);
 	file_move(f, &inode->i_sb->s_files);
 
-	error = security_dentry_open(f);
+	error = security_dentry_open(f, flags);
 	if (error)
 		goto cleanup_all;
 
diff --git a/include/linux/security.h b/include/linux/security.h
index b07357c..704a5f7 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1338,7 +1338,7 @@ struct security_operations {
 	int (*file_send_sigiotask) (struct task_struct * tsk,
 				    struct fown_struct * fown, int sig);
 	int (*file_receive) (struct file * file);
-	int (*dentry_open)  (struct file *file);
+	int (*dentry_open)  (struct file *file, int flags);
 
 	int (*task_create) (unsigned long clone_flags);
 	int (*task_alloc_security) (struct task_struct * p);
@@ -1594,7 +1594,7 @@ int security_file_set_fowner(struct file *file);
 int security_file_send_sigiotask(struct task_struct *tsk,
 				  struct fown_struct *fown, int sig);
 int security_file_receive(struct file *file);
-int security_dentry_open(struct file *file);
+int security_dentry_open(struct file *file, int flags);
 int security_task_create(unsigned long clone_flags);
 int security_task_alloc(struct task_struct *p);
 void security_task_free(struct task_struct *p);
@@ -2086,7 +2086,7 @@ static inline int security_file_receive (struct file *file)
 	return 0;
 }
 
-static inline int security_dentry_open (struct file *file)
+static inline int security_dentry_open (struct file *file, int flags)
 {
 	return 0;
 }
diff --git a/security/security.c b/security/security.c
index b1387a6..28a4f2a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -582,9 +582,9 @@ int security_file_receive(struct file *file)
 	return security_ops->file_receive(file);
 }
 
-int security_dentry_open(struct file *file)
+int security_dentry_open(struct file *file, int flags)
 {
-	return security_ops->dentry_open(file);
+	return security_ops->dentry_open(file, flags);
 }
 
 int security_task_create(unsigned long clone_flags)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4bf4807..1966590 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3017,11 +3017,14 @@ static int selinux_file_receive(struct file *file)
 	return file_has_perm(current, file, file_to_av(file));
 }
 
-static int selinux_dentry_open(struct file *file)
+static int selinux_dentry_open(struct file *file, int flags)
 {
 	struct file_security_struct *fsec;
 	struct inode *inode;
 	struct inode_security_struct *isec;
+	u32 av = 0;
+	mode_t f_mode;
+
 	inode = file->f_path.dentry->d_inode;
 	fsec = file->f_security;
 	isec = inode->i_security;
@@ -3034,6 +3037,19 @@ static int selinux_dentry_open(struct file *file)
 	 */
 	fsec->isid = isec->sid;
 	fsec->pseqno = avc_policy_seqno();
+
+	f_mode = flags & O_ACCMODE;
+	if ((f_mode+1) & O_ACCMODE)
+		f_mode++;
+
+	if (f_mode & FMODE_READ)
+		av |= FILE__READ;
+	if (f_mode & FMODE_WRITE) {
+		if (file->f_flags & O_APPEND)
+			av |= FILE__APPEND;
+		else
+			av |= FILE__WRITE;
+	}
 	/*
 	 * Since the inode label or policy seqno may have changed
 	 * between the selinux_inode_permission check and the saving
@@ -3042,7 +3058,7 @@ static int selinux_dentry_open(struct file *file)
 	 * new inode label or new policy.
 	 * This check is not redundant - do not remove.
 	 */
-	return inode_has_perm(current, inode, file_to_av(file), NULL);
+	return inode_has_perm(current, inode, av, NULL);
 }
 
 /* task security operations */



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: How to handle security_dentry_open when the open flags are 'special'

[Eric Paris]

Which I think then blows up if we pass this file to another process or
if we exec something.  Uggh....

-Eric


On 3/12/08, Eric Paris <eparis@redhat.com> wrote:
> See LKML posting http://marc.info/?l=linux-kernel&m=120534660832189&w=2
>
>  according to viro a call to sys_open like: open("/dev/null", 3) is
>  supposed to mean: check read and write but not set FMODE_READ or
>  FMODE_WRITE.  This is exactly the security check we get through the
>  security_inode_permission hook but not what happens through the
>  security_dentry_open hook.  This patch actually passes the bare flags
>  into security_dentry_open so that we can do the correct permission
>  checking there.
>
>  What do people think?
>
>  Untested, uncompiled, just thoughts and fleshing it out....
>
>  -Eric
>
>   fs/open.c                |    2 +-
>   include/linux/security.h |    6 +++---
>   security/security.c      |    4 ++--
>   security/selinux/hooks.c |   20 ++++++++++++++++++--
>   4 files changed, 24 insertions(+), 8 deletions(-)
>
>  diff --git a/fs/open.c b/fs/open.c
>  index 5419853..2560a3e 100644
>  --- a/fs/open.c
>  +++ b/fs/open.c
>  @@ -754,7 +754,7 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
>         f->f_op = fops_get(inode->i_fop);
>         file_move(f, &inode->i_sb->s_files);
>
>  -       error = security_dentry_open(f);
>  +       error = security_dentry_open(f, flags);
>         if (error)
>                 goto cleanup_all;
>
>  diff --git a/include/linux/security.h b/include/linux/security.h
>  index b07357c..704a5f7 100644
>  --- a/include/linux/security.h
>  +++ b/include/linux/security.h
>  @@ -1338,7 +1338,7 @@ struct security_operations {
>         int (*file_send_sigiotask) (struct task_struct * tsk,
>                                     struct fown_struct * fown, int sig);
>         int (*file_receive) (struct file * file);
>  -       int (*dentry_open)  (struct file *file);
>  +       int (*dentry_open)  (struct file *file, int flags);
>
>         int (*task_create) (unsigned long clone_flags);
>         int (*task_alloc_security) (struct task_struct * p);
>  @@ -1594,7 +1594,7 @@ int security_file_set_fowner(struct file *file);
>   int security_file_send_sigiotask(struct task_struct *tsk,
>                                   struct fown_struct *fown, int sig);
>   int security_file_receive(struct file *file);
>  -int security_dentry_open(struct file *file);
>  +int security_dentry_open(struct file *file, int flags);
>   int security_task_create(unsigned long clone_flags);
>   int security_task_alloc(struct task_struct *p);
>   void security_task_free(struct task_struct *p);
>  @@ -2086,7 +2086,7 @@ static inline int security_file_receive (struct file *file)
>         return 0;
>   }
>
>  -static inline int security_dentry_open (struct file *file)
>  +static inline int security_dentry_open (struct file *file, int flags)
>   {
>         return 0;
>   }
>  diff --git a/security/security.c b/security/security.c
>  index b1387a6..28a4f2a 100644
>  --- a/security/security.c
>  +++ b/security/security.c
>  @@ -582,9 +582,9 @@ int security_file_receive(struct file *file)
>         return security_ops->file_receive(file);
>   }
>
>  -int security_dentry_open(struct file *file)
>  +int security_dentry_open(struct file *file, int flags)
>   {
>  -       return security_ops->dentry_open(file);
>  +       return security_ops->dentry_open(file, flags);
>   }
>
>   int security_task_create(unsigned long clone_flags)
>  diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>  index 4bf4807..1966590 100644
>  --- a/security/selinux/hooks.c
>  +++ b/security/selinux/hooks.c
>  @@ -3017,11 +3017,14 @@ static int selinux_file_receive(struct file *file)
>         return file_has_perm(current, file, file_to_av(file));
>   }
>
>  -static int selinux_dentry_open(struct file *file)
>  +static int selinux_dentry_open(struct file *file, int flags)
>   {
>         struct file_security_struct *fsec;
>         struct inode *inode;
>         struct inode_security_struct *isec;
>  +       u32 av = 0;
>  +       mode_t f_mode;
>  +
>         inode = file->f_path.dentry->d_inode;
>         fsec = file->f_security;
>         isec = inode->i_security;
>  @@ -3034,6 +3037,19 @@ static int selinux_dentry_open(struct file *file)
>          */
>         fsec->isid = isec->sid;
>         fsec->pseqno = avc_policy_seqno();
>  +
>  +       f_mode = flags & O_ACCMODE;
>  +       if ((f_mode+1) & O_ACCMODE)
>  +               f_mode++;
>  +
>  +       if (f_mode & FMODE_READ)
>  +               av |= FILE__READ;
>  +       if (f_mode & FMODE_WRITE) {
>  +               if (file->f_flags & O_APPEND)
>  +                       av |= FILE__APPEND;
>  +               else
>  +                       av |= FILE__WRITE;
>  +       }
>         /*
>          * Since the inode label or policy seqno may have changed
>          * between the selinux_inode_permission check and the saving
>  @@ -3042,7 +3058,7 @@ static int selinux_dentry_open(struct file *file)
>          * new inode label or new policy.
>          * This check is not redundant - do not remove.
>          */
>  -       return inode_has_perm(current, inode, file_to_av(file), NULL);
>  +       return inode_has_perm(current, inode, av, NULL);
>   }
>
>   /* task security operations */
>
>
>
>  --
>  To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
>  the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: How to handle security_dentry_open when the open flags are 'special'

[Stephen Smalley]

Eric,

IIUC, in this situation, we are setting up a file that cannot be used
for read or write operations due to its lack of FMODE_READ or
FMODE_WRITE.  Thus, we don't actually need to check anything in
selinux_dentry_open - that check is only to avoid losing checking on
read/write revalidation altogether in the case where the policy seqno
or file label has changed since the inode_permission check.  If we can
never use this file for read/write, it isn't needed.

For the revalidation of open files on exec or transfer, if it cannot
be used for read or write, then we likewise can't (and don't need to)
revalidate it.  If/when it gets used in a ioctl call, we'll check it
via selinux_file_ioctl.

Thus, IMHO, we need to change callers of file_to_av() to check for a 0
return and skip checking in that case - it is apparently a legal case
that we didn't realize originally.

Stephen "not in the office today" Smalley


On 3/12/08, Eric Paris <eparis@parisplace.org> wrote:
> Which I think then blows up if we pass this file to another process or
> if we exec something.  Uggh....
>
> -Eric
>
>
> On 3/12/08, Eric Paris <eparis@redhat.com> wrote:
> > See LKML posting http://marc.info/?l=linux-kernel&m=120534660832189&w=2
> >
> >  according to viro a call to sys_open like: open("/dev/null", 3) is
> >  supposed to mean: check read and write but not set FMODE_READ or
> >  FMODE_WRITE.  This is exactly the security check we get through the
> >  security_inode_permission hook but not what happens through the
> >  security_dentry_open hook.  This patch actually passes the bare flags
> >  into security_dentry_open so that we can do the correct permission
> >  checking there.
> >
> >  What do people think?
> >
> >  Untested, uncompiled, just thoughts and fleshing it out....
> >
> >  -Eric
> >
> >   fs/open.c                |    2 +-
> >   include/linux/security.h |    6 +++---
> >   security/security.c      |    4 ++--
> >   security/selinux/hooks.c |   20 ++++++++++++++++++--
> >   4 files changed, 24 insertions(+), 8 deletions(-)
> >
> >  diff --git a/fs/open.c b/fs/open.c
> >  index 5419853..2560a3e 100644
> >  --- a/fs/open.c
> >  +++ b/fs/open.c
> >  @@ -754,7 +754,7 @@ static struct file *__dentry_open(struct dentry
> *dentry, struct vfsmount *mnt,
> >         f->f_op = fops_get(inode->i_fop);
> >         file_move(f, &inode->i_sb->s_files);
> >
> >  -       error = security_dentry_open(f);
> >  +       error = security_dentry_open(f, flags);
> >         if (error)
> >                 goto cleanup_all;
> >
> >  diff --git a/include/linux/security.h b/include/linux/security.h
> >  index b07357c..704a5f7 100644
> >  --- a/include/linux/security.h
> >  +++ b/include/linux/security.h
> >  @@ -1338,7 +1338,7 @@ struct security_operations {
> >         int (*file_send_sigiotask) (struct task_struct * tsk,
> >                                     struct fown_struct * fown, int sig);
> >         int (*file_receive) (struct file * file);
> >  -       int (*dentry_open)  (struct file *file);
> >  +       int (*dentry_open)  (struct file *file, int flags);
> >
> >         int (*task_create) (unsigned long clone_flags);
> >         int (*task_alloc_security) (struct task_struct * p);
> >  @@ -1594,7 +1594,7 @@ int security_file_set_fowner(struct file *file);
> >   int security_file_send_sigiotask(struct task_struct *tsk,
> >                                   struct fown_struct *fown, int sig);
> >   int security_file_receive(struct file *file);
> >  -int security_dentry_open(struct file *file);
> >  +int security_dentry_open(struct file *file, int flags);
> >   int security_task_create(unsigned long clone_flags);
> >   int security_task_alloc(struct task_struct *p);
> >   void security_task_free(struct task_struct *p);
> >  @@ -2086,7 +2086,7 @@ static inline int security_file_receive (struct
> file *file)
> >         return 0;
> >   }
> >
> >  -static inline int security_dentry_open (struct file *file)
> >  +static inline int security_dentry_open (struct file *file, int flags)
> >   {
> >         return 0;
> >   }
> >  diff --git a/security/security.c b/security/security.c
> >  index b1387a6..28a4f2a 100644
> >  --- a/security/security.c
> >  +++ b/security/security.c
> >  @@ -582,9 +582,9 @@ int security_file_receive(struct file *file)
> >         return security_ops->file_receive(file);
> >   }
> >
> >  -int security_dentry_open(struct file *file)
> >  +int security_dentry_open(struct file *file, int flags)
> >   {
> >  -       return security_ops->dentry_open(file);
> >  +       return security_ops->dentry_open(file, flags);
> >   }
> >
> >   int security_task_create(unsigned long clone_flags)
> >  diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >  index 4bf4807..1966590 100644
> >  --- a/security/selinux/hooks.c
> >  +++ b/security/selinux/hooks.c
> >  @@ -3017,11 +3017,14 @@ static int selinux_file_receive(struct file
> *file)
> >         return file_has_perm(current, file, file_to_av(file));
> >   }
> >
> >  -static int selinux_dentry_open(struct file *file)
> >  +static int selinux_dentry_open(struct file *file, int flags)
> >   {
> >         struct file_security_struct *fsec;
> >         struct inode *inode;
> >         struct inode_security_struct *isec;
> >  +       u32 av = 0;
> >  +       mode_t f_mode;
> >  +
> >         inode = file->f_path.dentry->d_inode;
> >         fsec = file->f_security;
> >         isec = inode->i_security;
> >  @@ -3034,6 +3037,19 @@ static int selinux_dentry_open(struct file *file)
> >          */
> >         fsec->isid = isec->sid;
> >         fsec->pseqno = avc_policy_seqno();
> >  +
> >  +       f_mode = flags & O_ACCMODE;
> >  +       if ((f_mode+1) & O_ACCMODE)
> >  +               f_mode++;
> >  +
> >  +       if (f_mode & FMODE_READ)
> >  +               av |= FILE__READ;
> >  +       if (f_mode & FMODE_WRITE) {
> >  +               if (file->f_flags & O_APPEND)
> >  +                       av |= FILE__APPEND;
> >  +               else
> >  +                       av |= FILE__WRITE;
> >  +       }
> >         /*
> >          * Since the inode label or policy seqno may have changed
> >          * between the selinux_inode_permission check and the saving
> >  @@ -3042,7 +3058,7 @@ static int selinux_dentry_open(struct file *file)
> >          * new inode label or new policy.
> >          * This check is not redundant - do not remove.
> >          */
> >  -       return inode_has_perm(current, inode, file_to_av(file), NULL);
> >  +       return inode_has_perm(current, inode, av, NULL);
> >   }
> >
> >   /* task security operations */
> >
> >
> >
> >  --
> >  To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> >  the body of a message to majordomo@vger.kernel.org
> >  More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: How to handle security_dentry_open when the open flags are 'special'

[Stephen Smalley]

On Thu, 2008-03-13 at 07:56 -0400, Stephen Smalley wrote:
> Eric,
> 
> IIUC, in this situation, we are setting up a file that cannot be used
> for read or write operations due to its lack of FMODE_READ or
> FMODE_WRITE.  Thus, we don't actually need to check anything in
> selinux_dentry_open - that check is only to avoid losing checking on
> read/write revalidation altogether in the case where the policy seqno
> or file label has changed since the inode_permission check.  If we can
> never use this file for read/write, it isn't needed.
> 
> For the revalidation of open files on exec or transfer, if it cannot
> be used for read or write, then we likewise can't (and don't need to)
> revalidate it.  If/when it gets used in a ioctl call, we'll check it
> via selinux_file_ioctl.
> 
> Thus, IMHO, we need to change callers of file_to_av() to check for a 0
> return and skip checking in that case - it is apparently a legal case
> that we didn't realize originally.

Alternatively, we could default to returning FILE__IOCTL from
file_to_av() if the f_mode has neither FMODE_READ nor FMODE_WRITE, and
thus check ioctl permission on exec or transfer, thereby validating such
descriptors early as with normal r/w descriptors and catching leaks of
them prior to attempted usage.

selinux_dentry_open() though doesn't need to check anything in this
case; its checking is only required for descriptors that can later be
used in read/write operations.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: How to handle security_dentry_open when the open flags are 'special'

[James Morris]

On Fri, 14 Mar 2008, Stephen Smalley wrote:

> Alternatively, we could default to returning FILE__IOCTL from
> file_to_av() if the f_mode has neither FMODE_READ nor FMODE_WRITE, and
> thus check ioctl permission on exec or transfer, thereby validating such
> descriptors early as with normal r/w descriptors and catching leaks of
> them prior to attempted usage.

I think this sounds like a good plan.

> 
> selinux_dentry_open() though doesn't need to check anything in this
> case; its checking is only required for descriptors that can later be
> used in read/write operations.
> 
> 

-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

[patch] selinux: handle files opened with flags 3 by checking ioctl permission

[Stephen Smalley]

On Mon, 2008-03-17 at 08:41 +1100, James Morris wrote:
> On Fri, 14 Mar 2008, Stephen Smalley wrote:
> 
> > Alternatively, we could default to returning FILE__IOCTL from
> > file_to_av() if the f_mode has neither FMODE_READ nor FMODE_WRITE, and
> > thus check ioctl permission on exec or transfer, thereby validating such
> > descriptors early as with normal r/w descriptors and catching leaks of
> > them prior to attempted usage.
> 
> I think this sounds like a good plan.

Handle files opened with flags 3 by checking ioctl permission.

Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>

---

 security/selinux/hooks.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4bf4807..7d82aa2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1629,6 +1629,12 @@ static inline u32 file_to_av(struct file *file)
 		else
 			av |= FILE__WRITE;
 	}
+	if (!av) {
+		/*
+		 * Special file opened with flags 3 for ioctl-only use.
+		 */
+		av = FILE__IOCTL;
+	}
 
 	return av;
 }



-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] selinux: handle files opened with flags 3 by checking ioctl permission

[Eric Paris]

On Mon, 2008-03-17 at 08:55 -0400, Stephen Smalley wrote:
> On Mon, 2008-03-17 at 08:41 +1100, James Morris wrote:
> > On Fri, 14 Mar 2008, Stephen Smalley wrote:
> > 
> > > Alternatively, we could default to returning FILE__IOCTL from
> > > file_to_av() if the f_mode has neither FMODE_READ nor FMODE_WRITE, and
> > > thus check ioctl permission on exec or transfer, thereby validating such
> > > descriptors early as with normal r/w descriptors and catching leaks of
> > > them prior to attempted usage.
> > 
> > I think this sounds like a good plan.
> 
> Handle files opened with flags 3 by checking ioctl permission.
> 
> Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
> 
> ---
> 
>  security/selinux/hooks.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4bf4807..7d82aa2 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1629,6 +1629,12 @@ static inline u32 file_to_av(struct file *file)
>  		else
>  			av |= FILE__WRITE;
>  	}
> +	if (!av) {

We have the flags, would we rather check

if ((file->f_flags & O_ACCMODE) == O_ACCMODE)

This way we would still catch and BUG's we don't know about?

> +		/*
> +		 * Special file opened with flags 3 for ioctl-only use.
> +		 */
> +		av = FILE__IOCTL;
> +	}
>  
>  	return av;
>  }
> 
> 
> 


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] selinux: handle files opened with flags 3 by checking ioctl permission

[Stephen Smalley]

On Mon, 2008-03-17 at 10:01 -0400, Eric Paris wrote:
> On Mon, 2008-03-17 at 08:55 -0400, Stephen Smalley wrote:
> > On Mon, 2008-03-17 at 08:41 +1100, James Morris wrote:
> > > On Fri, 14 Mar 2008, Stephen Smalley wrote:
> > > 
> > > > Alternatively, we could default to returning FILE__IOCTL from
> > > > file_to_av() if the f_mode has neither FMODE_READ nor FMODE_WRITE, and
> > > > thus check ioctl permission on exec or transfer, thereby validating such
> > > > descriptors early as with normal r/w descriptors and catching leaks of
> > > > them prior to attempted usage.
> > > 
> > > I think this sounds like a good plan.
> > 
> > Handle files opened with flags 3 by checking ioctl permission.
> > 
> > Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
> > 
> > ---
> > 
> >  security/selinux/hooks.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 4bf4807..7d82aa2 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1629,6 +1629,12 @@ static inline u32 file_to_av(struct file *file)
> >  		else
> >  			av |= FILE__WRITE;
> >  	}
> > +	if (!av) {
> 
> We have the flags, would we rather check
> 
> if ((file->f_flags & O_ACCMODE) == O_ACCMODE)
> 
> This way we would still catch and BUG's we don't know about?

That would be a bug in the core kernel, not a bug in SELinux, so it
seems more like something one would test in the VFS than in SELinux.

Also, even if there were other ways to create such an open file other
than opening with flags 3, I'm not sure we'd do anything differently
here - the descriptor is for control purposes only and checking ioctl
makes sense.

> 
> > +		/*
> > +		 * Special file opened with flags 3 for ioctl-only use.
> > +		 */
> > +		av = FILE__IOCTL;
> > +	}
> >  
> >  	return av;
> >  }
> > 
> > 
> > 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] selinux: handle files opened with flags 3 by checking ioctl permission

[Eric Paris]

On Mon, 2008-03-17 at 08:55 -0400, Stephen Smalley wrote:
> On Mon, 2008-03-17 at 08:41 +1100, James Morris wrote:
> > On Fri, 14 Mar 2008, Stephen Smalley wrote:
> > 
> > > Alternatively, we could default to returning FILE__IOCTL from
> > > file_to_av() if the f_mode has neither FMODE_READ nor FMODE_WRITE, and
> > > thus check ioctl permission on exec or transfer, thereby validating such
> > > descriptors early as with normal r/w descriptors and catching leaks of
> > > them prior to attempted usage.
> > 
> > I think this sounds like a good plan.
> 
> Handle files opened with flags 3 by checking ioctl permission.
> 
> Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>

Acked-by: Eric Paris <eparis@redhat.com>

> 
> ---
> 
>  security/selinux/hooks.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4bf4807..7d82aa2 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1629,6 +1629,12 @@ static inline u32 file_to_av(struct file *file)
>  		else
>  			av |= FILE__WRITE;
>  	}
> +	if (!av) {
> +		/*
> +		 * Special file opened with flags 3 for ioctl-only use.
> +		 */
> +		av = FILE__IOCTL;
> +	}
>  
>  	return av;
>  }
> 
> 
> 


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] selinux: handle files opened with flags 3 by checking ioctl permission

[James Morris]

On Mon, 17 Mar 2008, Eric Paris wrote:

> 
> On Mon, 2008-03-17 at 08:55 -0400, Stephen Smalley wrote:
> > On Mon, 2008-03-17 at 08:41 +1100, James Morris wrote:
> > > On Fri, 14 Mar 2008, Stephen Smalley wrote:
> > > 
> > > > Alternatively, we could default to returning FILE__IOCTL from
> > > > file_to_av() if the f_mode has neither FMODE_READ nor FMODE_WRITE, and
> > > > thus check ioctl permission on exec or transfer, thereby validating such
> > > > descriptors early as with normal r/w descriptors and catching leaks of
> > > > them prior to attempted usage.
> > > 
> > > I think this sounds like a good plan.
> > 
> > Handle files opened with flags 3 by checking ioctl permission.
> > 
> > Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
> 
> Acked-by: Eric Paris <eparis@redhat.com>

Fixes the problem I was seeing.  Applied to for-akpm.


-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] selinux: handle files opened with flags 3 by checking ioctl permission

[Eric Paris]

On Tue, 2008-03-18 at 09:15 +1100, James Morris wrote:
> On Mon, 17 Mar 2008, Eric Paris wrote:
> 
> > 
> > On Mon, 2008-03-17 at 08:55 -0400, Stephen Smalley wrote:
> > > On Mon, 2008-03-17 at 08:41 +1100, James Morris wrote:
> > > > On Fri, 14 Mar 2008, Stephen Smalley wrote:
> > > > 
> > > > > Alternatively, we could default to returning FILE__IOCTL from
> > > > > file_to_av() if the f_mode has neither FMODE_READ nor FMODE_WRITE, and
> > > > > thus check ioctl permission on exec or transfer, thereby validating such
> > > > > descriptors early as with normal r/w descriptors and catching leaks of
> > > > > them prior to attempted usage.
> > > > 
> > > > I think this sounds like a good plan.
> > > 
> > > Handle files opened with flags 3 by checking ioctl permission.
> > > 
> > > Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
> > 
> > Acked-by: Eric Paris <eparis@redhat.com>
> 
> Fixes the problem I was seeing.  Applied to for-akpm.

Are you at least now seeing a FILE__IOCTL avc for mdadm?  If not why
would mdadm have that permission for /dev/null?

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] selinux: handle files opened with flags 3 by checking ioctl permission

[James Morris]

On Mon, 17 Mar 2008, Eric Paris wrote:

> > Fixes the problem I was seeing.  Applied to for-akpm.
> 
> Are you at least now seeing a FILE__IOCTL avc for mdadm?  If not why
> would mdadm have that permission for /dev/null?

kernel: [ 4.779280] type=1400 audit(1205792724.308:4): avc:  granted null 
for pid=1792 comm="mdadm" name="null" dev=tmpfs ino=189 
scontext=system_u:system_r:mdadm_t:s0 
tcontext=system_u:object_r:null_device_t:s0 tclass=chr_file

Possibly related.


-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] selinux: handle files opened with flags 3 by checking ioctl permission

[Stephen Smalley]

On Mon, 2008-03-17 at 18:19 -0400, Eric Paris wrote:
> On Tue, 2008-03-18 at 09:15 +1100, James Morris wrote:
> > On Mon, 17 Mar 2008, Eric Paris wrote:
> > 
> > > 
> > > On Mon, 2008-03-17 at 08:55 -0400, Stephen Smalley wrote:
> > > > On Mon, 2008-03-17 at 08:41 +1100, James Morris wrote:
> > > > > On Fri, 14 Mar 2008, Stephen Smalley wrote:
> > > > > 
> > > > > > Alternatively, we could default to returning FILE__IOCTL from
> > > > > > file_to_av() if the f_mode has neither FMODE_READ nor FMODE_WRITE, and
> > > > > > thus check ioctl permission on exec or transfer, thereby validating such
> > > > > > descriptors early as with normal r/w descriptors and catching leaks of
> > > > > > them prior to attempted usage.
> > > > > 
> > > > > I think this sounds like a good plan.
> > > > 
> > > > Handle files opened with flags 3 by checking ioctl permission.
> > > > 
> > > > Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
> > > 
> > > Acked-by: Eric Paris <eparis@redhat.com>
> > 
> > Fixes the problem I was seeing.  Applied to for-akpm.
> 
> Are you at least now seeing a FILE__IOCTL avc for mdadm?  If not why
> would mdadm have that permission for /dev/null?

ioctl permission is widely granted due to the common use of ioctl for
probing the object type (isatty).  The common permission set macros
usually include it whenever they include read or write.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] selinux: handle files opened with flags 3 by checking ioctl permission

[Stephen Smalley]

On Tue, 2008-03-18 at 10:02 +1100, James Morris wrote:
> On Mon, 17 Mar 2008, Eric Paris wrote:
> 
> > > Fixes the problem I was seeing.  Applied to for-akpm.
> > 
> > Are you at least now seeing a FILE__IOCTL avc for mdadm?  If not why
> > would mdadm have that permission for /dev/null?
> 
> kernel: [ 4.779280] type=1400 audit(1205792724.308:4): avc:  granted null 
> for pid=1792 comm="mdadm" name="null" dev=tmpfs ino=189 
> scontext=system_u:system_r:mdadm_t:s0 
> tcontext=system_u:object_r:null_device_t:s0 tclass=chr_file
> 
> Possibly related.

Did that happen before or after the patch was applied?
That indicates that we had a requested==0 value passed into the avc, so
it should have hit the BUG_ON if it happened after Eric's earlier patch,
and my patch should have eliminated the case altogether.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.