[RFC][PATCH 0/7] File descriptor labeling

[RFC][PATCH 0/7] File descriptor labeling

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 3428 bytes --]

File descriptor labeling issue

Actually SELinux and SMACK assign to file descriptors the same label of the
opening process and use it in LSM hooks security_file_permission(),
security_file_fcntl() and others to verify if the 'current' process has the
rights to perform the requested operation.

Using the credentials of the 'current' process may be not appropriate in
case a file descriptor is opened by a kernel service (i.e. a filesystem)
and made shared among user processes. For instance, in a system with
SELinux and eCryptfs, if the process A opens an encrypted file, eCryptfs
obtains a file descriptor to access the correspondent inode in the lower
filesystem, labeled with the A's label.

If the process B accesses the same encrypted file, it needs the 'use'
permission on the A's label other than permissions for the lower inode.
However, if B is the first accessing process, A needs the 'use' permission
on the B's label.

The solution proposed is to modify those kernel services that deal with
file descriptors to provide their set of credentials to dentry_open(), so
that obtained objects are labeled with a unique label. In this way, in the
above example, if eCryptfs provides its credentials with the label C to
dentry_open(), all user processes need the 'use' permission only on C.



File descriptor labeling and IMA

The above proposal suggests to use the file descriptor label as a new
criteria in the IMA policy to determine if a file must be measured. It will
be possible to measure all files opened by a kernel service by simply
writing a rule where the file descriptor label given as a value matches the
one provided by the same service together with other credentials to the
function dentry_open().

In the above example, if eCryptfs provides its credentials with the label C
to dentry_open(), it is possible to measure all inodes opened in the lower
filesystem by specifying a rule like:

fowner_type=C


The benefits of this new criteria will be greater with the integration of
EVM and the IMA appraisal feature in the kernel. ECryptfs can be used in
conjunction with these components to verify the integrity of the content
and extended attributes of encrypted files.

Roberto Sassu


Roberto Sassu (7):
  fs: initialize file->f_cred with credentials provided
  selinux: label new file descriptors using file->f_cred
  smack: assign the label set in file->f_cred to new file descriptors
  smack: fix label check in smack_kernel_act_as()
  smack: import the security label in smack_secctx_to_secid()
  security: new LSM hook security_file_getsecid()
  ima: added new LSM conditions in the policy

 Documentation/ABI/testing/ima_policy |    7 ++++-
 fs/file_table.c                      |    5 +--
 fs/internal.h                        |    2 +-
 fs/namei.c                           |    2 +-
 fs/open.c                            |    2 +-
 include/linux/security.h             |   12 +++++++++
 security/capability.c                |    6 ++++
 security/integrity/ima/ima.h         |    4 +-
 security/integrity/ima/ima_api.c     |    4 +-
 security/integrity/ima/ima_main.c    |    4 +-
 security/integrity/ima/ima_policy.c  |   45 +++++++++++++++++++++++++++++----
 security/security.c                  |    6 ++++
 security/selinux/hooks.c             |    9 ++++++-
 security/smack/smack_lsm.c           |   23 +++++++++++++++--
 14 files changed, 108 insertions(+), 23 deletions(-)

-- 
1.7.4.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[RFC][PATCH 1/7] fs: initialize file->f_cred with credentials provided

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

The 'f_cred' field of a file descriptor is initialized with the credentials
of the 'current' process except in the case they are provided to the
function dentry_open(). The get_empty_filp() function's definition has been
modified to take these credentials set as argument.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 fs/file_table.c |    5 ++---
 fs/internal.h   |    2 +-
 fs/namei.c      |    2 +-
 fs/open.c       |    2 +-
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 01e4c1e..c33018c 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -102,9 +102,8 @@ int proc_nr_files(ctl_table *table, int write,
  * done, you will imbalance int the mount's writer count
  * and a warning at __fput() time.
  */
-struct file *get_empty_filp(void)
+struct file *get_empty_filp(const struct cred *cred)
 {
-	const struct cred *cred = current_cred();
 	static long old_max;
 	struct file * f;
 
@@ -171,7 +170,7 @@ struct file *alloc_file(struct path *path, fmode_t mode,
 {
 	struct file *file;
 
-	file = get_empty_filp();
+	file = get_empty_filp(current_cred());
 	if (!file)
 		return NULL;
 
diff --git a/fs/internal.h b/fs/internal.h
index b29c46e..c81fc62 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -91,7 +91,7 @@ extern void chroot_fs_refs(struct path *, struct path *);
 extern void file_sb_list_add(struct file *f, struct super_block *sb);
 extern void file_sb_list_del(struct file *f);
 extern void mark_files_ro(struct super_block *);
-extern struct file *get_empty_filp(void);
+extern struct file *get_empty_filp(const struct cred *cred);
 
 /*
  * super.c
diff --git a/fs/namei.c b/fs/namei.c
index 54fc993..88ac2e5 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2321,7 +2321,7 @@ static struct file *path_openat(int dfd, const char *pathname,
 	struct path path;
 	int error;
 
-	filp = get_empty_filp();
+	filp = get_empty_filp(current_cred());
 	if (!filp)
 		return ERR_PTR(-ENFILE);
 
diff --git a/fs/open.c b/fs/open.c
index b52cf01..6b033e6 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -839,7 +839,7 @@ struct file *dentry_open(struct dentry *dentry, struct vfsmount *mnt, int flags,
 	BUG_ON(!mnt);
 
 	error = -ENFILE;
-	f = get_empty_filp();
+	f = get_empty_filp(cred);
 	if (f == NULL) {
 		dput(dentry);
 		mntput(mnt);
-- 
1.7.4.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[RFC][PATCH 2/7] selinux: label new file descriptors using file->f_cred

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

New file descriptors are labeled using the credentials set in the 'f_cred'
field of the same structure. This permits to distinguish objects directly
managed by a user process from those created by a kernel service acting
on behalf of the application.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 security/selinux/hooks.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f9c3764..6772687 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -232,7 +232,7 @@ static void inode_free_security(struct inode *inode)
 static int file_alloc_security(struct file *file)
 {
 	struct file_security_struct *fsec;
-	u32 sid = current_sid();
+	u32 sid = cred_sid(file->f_cred);
 
 	fsec = kzalloc(sizeof(struct file_security_struct), GFP_KERNEL);
 	if (!fsec)
-- 
1.7.4.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[RFC][PATCH 3/7] smack: assign the label set in file->f_cred to new file descriptors

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

The SMACK label of new file descriptors is obtained from the credentials
set in the 'f_cred' field of the same structure.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 security/smack/smack_lsm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index c6f8fca..e3c9e54 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1011,7 +1011,7 @@ static int smack_file_permission(struct file *file, int mask)
  */
 static int smack_file_alloc_security(struct file *file)
 {
-	file->f_security = smk_of_current();
+	file->f_security = smk_of_task(file->f_cred->security);
 	return 0;
 }
 
-- 
1.7.4.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

Re: [RFC][PATCH 3/7] smack: assign the label set in file->f_cred to new file descriptors

[Casey Schaufler]

On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> The SMACK label of new file descriptors is obtained from the credentials
> set in the 'f_cred' field of the same structure.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> ---
>  security/smack/smack_lsm.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index c6f8fca..e3c9e54 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1011,7 +1011,7 @@ static int smack_file_permission(struct file *file, int mask)
>   */
>  static int smack_file_alloc_security(struct file *file)
>  {
> -	file->f_security = smk_of_current();
> +	file->f_security = smk_of_task(file->f_cred->security);

Now hang on. This just looks wrong. You're setting the value of one
field of the file structure to another value in the same file structure.
I don't see that this is what I want.

>  	return 0;
>  }
>  

Re: [RFC][PATCH 3/7] smack: assign the label set in file->f_cred to new file descriptors

[Roberto Sassu]

On Thursday, April 28, 2011 01:26:43 AM Casey Schaufler wrote:
> On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> > The SMACK label of new file descriptors is obtained from the credentials
> > set in the 'f_cred' field of the same structure.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> > ---
> >  security/smack/smack_lsm.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index c6f8fca..e3c9e54 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -1011,7 +1011,7 @@ static int smack_file_permission(struct file *file, int mask)
> >   */
> >  static int smack_file_alloc_security(struct file *file)
> >  {
> > -	file->f_security = smk_of_current();
> > +	file->f_security = smk_of_task(file->f_cred->security);
> 
> Now hang on. This just looks wrong. You're setting the value of one
> field of the file structure to another value in the same file structure.
> I don't see that this is what I want.
> 

Hi Casey

thanks for the review.
The field 'f_cred' stores the credentials of the subject that issued the open.
The first patch allows to set this field with the credentials provided to
the function get_empty_filp() which may be those of the 'current'
process as in the original case, or those provided by a kernel service that
called the function dentry_open() directly.

Roberto Sassu


> >  	return 0;
> >  }
> >  
> 
> 

[RFC][PATCH 4/7] smack: fix label check in smack_kernel_act_as()

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

The function smack_kernel_act_as() must return -EINVAL if the label
returned by smack_from_secid() is equal to 'smack_known_invalid.smk_known',
which means that no entries in the 'smack_known_list' list matching the
security identifier given are found.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 security/smack/smack_lsm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index e3c9e54..0e7ed31 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1415,7 +1415,7 @@ static int smack_kernel_act_as(struct cred *new, u32 secid)
 	struct task_smack *new_tsp = new->security;
 	char *smack = smack_from_secid(secid);
 
-	if (smack == NULL)
+	if (smack == smack_known_invalid.smk_known)
 		return -EINVAL;
 
 	new_tsp->smk_task = smack;
-- 
1.7.4.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

Re: [RFC][PATCH 4/7] smack: fix label check in smack_kernel_act_as()

[Casey Schaufler]

On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> The function smack_kernel_act_as() must return -EINVAL if the label
> returned by smack_from_secid() is equal to 'smack_known_invalid.smk_known',
> which means that no entries in the 'smack_known_list' list matching the
> security identifier given are found.

I'll admit that the code here is wrong, but I disagree with the fix.

smack_from_secid() will never return NULL, so the check for NULL is
pointless. Checking for known_invalid is not right either, as the
Smack philosophy is to return a label in all cases, as is evident
by the behavior of smack_from_secid(). Thus, the correct change
would be to remove the error check completely and set the new task
value to the value obtained from smack_from_secid in all cases.

Besides, where did the caller of this function get a secid that isn't
going to map to a Smack label?

> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> ---
>  security/smack/smack_lsm.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e3c9e54..0e7ed31 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1415,7 +1415,7 @@ static int smack_kernel_act_as(struct cred *new, u32 secid)
>  	struct task_smack *new_tsp = new->security;
>  	char *smack = smack_from_secid(secid);
>  
> -	if (smack == NULL)
> +	if (smack == smack_known_invalid.smk_known)
>  		return -EINVAL;
>  
>  	new_tsp->smk_task = smack;

Re: [RFC][PATCH 4/7] smack: fix label check in smack_kernel_act_as()

[Roberto Sassu]

On Thursday, April 28, 2011 01:22:34 AM Casey Schaufler wrote:
> On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> > The function smack_kernel_act_as() must return -EINVAL if the label
> > returned by smack_from_secid() is equal to 'smack_known_invalid.smk_known',
> > which means that no entries in the 'smack_known_list' list matching the
> > security identifier given are found.
> 
> I'll admit that the code here is wrong, but I disagree with the fix.
> 
> smack_from_secid() will never return NULL, so the check for NULL is
> pointless. Checking for known_invalid is not right either, as the
> Smack philosophy is to return a label in all cases, as is evident
> by the behavior of smack_from_secid(). Thus, the correct change
> would be to remove the error check completely and set the new task
> value to the value obtained from smack_from_secid in all cases.
> 
> Besides, where did the caller of this function get a secid that isn't
> going to map to a Smack label?
> 

I discovered the issue when calling the function set_security_override_from_ctx(),
located in kernel/cred.c, which will be used in eCryptfs to override the set
of initial credentials (i'm about to post the patches).
This function accepts an arbitrary string and uses it as a security label to replace
the one in the credentials provided. If the string passed is not yet imported
to SMACK, the 'smack_known_invalid.smk_known' label is applied without this fact
is notified to the caller.
I tried to solve the problem by importing the string during the execution of the
security_secctx_to_secid() hook, so that set_security_override() will never fail.
So, probably the string must be already been imported if no new memory can be
allocated during smack_secctx_to_secid().


> > Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> > ---
> >  security/smack/smack_lsm.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index e3c9e54..0e7ed31 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -1415,7 +1415,7 @@ static int smack_kernel_act_as(struct cred *new, u32 secid)
> >  	struct task_smack *new_tsp = new->security;
> >  	char *smack = smack_from_secid(secid);
> >  
> > -	if (smack == NULL)
> > +	if (smack == smack_known_invalid.smk_known)
> >  		return -EINVAL;
> >  
> >  	new_tsp->smk_task = smack;
> 
> 

[RFC][PATCH 5/7] smack: import the security label in smack_secctx_to_secid()

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

The security label passed as argument in smack_secctx_to_secid() must be
first imported in the 'smack_known_list' list before finding the security
identifier associated to it. This allows the function
set_security_override_from_ctx() to be performed successfully even if the
label is not yet present in the smack list.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 security/smack/smack_lsm.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0e7ed31..6612ba1 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3346,7 +3346,13 @@ static int smack_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
  */
 static int smack_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
 {
-	*secid = smack_to_secid(secdata);
+	char *smack;
+
+	smack = smk_import(secdata, seclen);
+	if (smack == NULL)
+		return -EINVAL;
+
+	*secid = smack_to_secid(smack);
 	return 0;
 }
 
-- 
1.7.4.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

Re: [RFC][PATCH 5/7] smack: import the security label in smack_secctx_to_secid()

[Casey Schaufler]

On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> The security label passed as argument in smack_secctx_to_secid() must be
> first imported in the 'smack_known_list' list before finding the security
> identifier associated to it. This allows the function
> set_security_override_from_ctx() to be performed successfully even if the
> label is not yet present in the smack list.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>

Nacked-by: Casey Schaufler <casey@schaufler-ca.com>

security_secctx_to_secid() is called from the netlabel code
and hence cannot (to my understanding) allocate memory,
which is something that smk_import will do if the label is
new. I am willing to be convinced that doing so is safe,
but my understanding is that it is not.

> ---
>  security/smack/smack_lsm.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0e7ed31..6612ba1 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3346,7 +3346,13 @@ static int smack_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
>   */
>  static int smack_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
>  {
> -	*secid = smack_to_secid(secdata);
> +	char *smack;
> +
> +	smack = smk_import(secdata, seclen);
> +	if (smack == NULL)
> +		return -EINVAL;
> +
> +	*secid = smack_to_secid(smack);
>  	return 0;
>  }
>  

[RFC][PATCH 6/7] security: new LSM hook security_file_getsecid()

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 5450 bytes --]

The new LSM hook security_file_getsecid() and its implementation in the
capability module, SELinux and SMACK allows to obtain the security
identifier associated to a file descriptor.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 include/linux/security.h   |   12 ++++++++++++
 security/capability.c      |    6 ++++++
 security/security.c        |    6 ++++++
 security/selinux/hooks.c   |    7 +++++++
 security/smack/smack_lsm.c |   11 +++++++++++
 5 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index ca02f17..6e73a1a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -630,6 +630,11 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	to receive an open file descriptor via socket IPC.
  *	@file contains the file structure being received.
  *	Return 0 if permission is granted.
+ * @file_getsecid:
+ *	Get the secid associated with the file descriptor.
+ *	@file contains a pointer to the file descriptor.
+ *	@secid contains a pointer to the location where result will be saved.
+ *	In case of failure, @secid will be set to zero.
  *
  * Security hook for dentry
  *
@@ -1492,6 +1497,7 @@ struct security_operations {
 	int (*file_send_sigiotask) (struct task_struct *tsk,
 				    struct fown_struct *fown, int sig);
 	int (*file_receive) (struct file *file);
+	void (*file_getsecid)(const struct file *file, u32 *secid);
 	int (*dentry_open) (struct file *file, const struct cred *cred);
 
 	int (*task_create) (unsigned long clone_flags);
@@ -1751,6 +1757,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);
+void security_file_getsecid(const struct file *file, u32 *secid);
 int security_dentry_open(struct file *file, const struct cred *cred);
 int security_task_create(unsigned long clone_flags);
 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
@@ -2251,6 +2258,11 @@ static inline int security_file_receive(struct file *file)
 	return 0;
 }
 
+static inline void security_file_getsecid(const struct file *file, u32 *secid)
+{
+	*secid = 0;
+}
+
 static inline int security_dentry_open(struct file *file,
 				       const struct cred *cred)
 {
diff --git a/security/capability.c b/security/capability.c
index 2984ea4..fcb569d 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -349,6 +349,11 @@ static int cap_file_receive(struct file *file)
 	return 0;
 }
 
+static void cap_file_getsecid(const struct file *file, u32 *secid)
+{
+	*secid = 0;
+}
+
 static int cap_dentry_open(struct file *file, const struct cred *cred)
 {
 	return 0;
@@ -953,6 +958,7 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, file_set_fowner);
 	set_to_cap_if_null(ops, file_send_sigiotask);
 	set_to_cap_if_null(ops, file_receive);
+	set_to_cap_if_null(ops, file_getsecid);
 	set_to_cap_if_null(ops, dentry_open);
 	set_to_cap_if_null(ops, task_create);
 	set_to_cap_if_null(ops, cred_alloc_blank);
diff --git a/security/security.c b/security/security.c
index 1011423..9973dab 100644
--- a/security/security.c
+++ b/security/security.c
@@ -688,6 +688,12 @@ int security_file_receive(struct file *file)
 	return security_ops->file_receive(file);
 }
 
+void security_file_getsecid(const struct file *file, u32 *secid)
+{
+	security_ops->file_getsecid(file, secid);
+}
+EXPORT_SYMBOL(security_file_getsecid);
+
 int security_dentry_open(struct file *file, const struct cred *cred)
 {
 	int ret;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6772687..e1e787c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3179,6 +3179,12 @@ static int selinux_file_receive(struct file *file)
 	return file_has_perm(cred, file, file_to_av(file));
 }
 
+static void selinux_file_getsecid(const struct file *file, u32 *secid)
+{
+	struct file_security_struct *fsec = file->f_security;
+	*secid = fsec->sid;
+}
+
 static int selinux_dentry_open(struct file *file, const struct cred *cred)
 {
 	struct file_security_struct *fsec;
@@ -5498,6 +5504,7 @@ static struct security_operations selinux_ops = {
 	.file_set_fowner =		selinux_file_set_fowner,
 	.file_send_sigiotask =		selinux_file_send_sigiotask,
 	.file_receive =			selinux_file_receive,
+	.file_getsecid =		selinux_file_getsecid,
 
 	.dentry_open =			selinux_dentry_open,
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 6612ba1..a583736 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1304,6 +1304,16 @@ static int smack_file_receive(struct file *file)
 	return smk_curacc(file->f_security, may, &ad);
 }
 
+/**
+ * smack_file_getsecid - Extract file descriptor's security id
+ * @file: file descriptor to extract the info from
+ * @secid: where result will be saved
+ */
+static void smack_file_getsecid(const struct file *file, u32 *secid)
+{
+	*secid = smack_to_secid(file->f_security);
+}
+
 /*
  * Task hooks
  */
@@ -3434,6 +3444,7 @@ struct security_operations smack_ops = {
 	.file_set_fowner = 		smack_file_set_fowner,
 	.file_send_sigiotask = 		smack_file_send_sigiotask,
 	.file_receive = 		smack_file_receive,
+	.file_getsecid =		smack_file_getsecid,
 
 	.cred_alloc_blank =		smack_cred_alloc_blank,
 	.cred_free =			smack_cred_free,
-- 
1.7.4.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

Re: [RFC][PATCH 6/7] security: new LSM hook security_file_getsecid()

[Casey Schaufler]

On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> The new LSM hook security_file_getsecid() and its implementation in the
> capability module, SELinux and SMACK allows to obtain the security
> identifier associated to a file descriptor.

Why do you want a secid? You have the security information from
the file structure readily available.

What use to you intend to put this to?

> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> ---
>  include/linux/security.h   |   12 ++++++++++++
>  security/capability.c      |    6 ++++++
>  security/security.c        |    6 ++++++
>  security/selinux/hooks.c   |    7 +++++++
>  security/smack/smack_lsm.c |   11 +++++++++++
>  5 files changed, 42 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ca02f17..6e73a1a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -630,6 +630,11 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   *	to receive an open file descriptor via socket IPC.
>   *	@file contains the file structure being received.
>   *	Return 0 if permission is granted.
> + * @file_getsecid:
> + *	Get the secid associated with the file descriptor.
> + *	@file contains a pointer to the file descriptor.
> + *	@secid contains a pointer to the location where result will be saved.
> + *	In case of failure, @secid will be set to zero.
>   *
>   * Security hook for dentry
>   *
> @@ -1492,6 +1497,7 @@ struct security_operations {
>  	int (*file_send_sigiotask) (struct task_struct *tsk,
>  				    struct fown_struct *fown, int sig);
>  	int (*file_receive) (struct file *file);
> +	void (*file_getsecid)(const struct file *file, u32 *secid);
>  	int (*dentry_open) (struct file *file, const struct cred *cred);
>  
>  	int (*task_create) (unsigned long clone_flags);
> @@ -1751,6 +1757,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);
> +void security_file_getsecid(const struct file *file, u32 *secid);
>  int security_dentry_open(struct file *file, const struct cred *cred);
>  int security_task_create(unsigned long clone_flags);
>  int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
> @@ -2251,6 +2258,11 @@ static inline int security_file_receive(struct file *file)
>  	return 0;
>  }
>  
> +static inline void security_file_getsecid(const struct file *file, u32 *secid)
> +{
> +	*secid = 0;
> +}
> +
>  static inline int security_dentry_open(struct file *file,
>  				       const struct cred *cred)
>  {
> diff --git a/security/capability.c b/security/capability.c
> index 2984ea4..fcb569d 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -349,6 +349,11 @@ static int cap_file_receive(struct file *file)
>  	return 0;
>  }
>  
> +static void cap_file_getsecid(const struct file *file, u32 *secid)
> +{
> +	*secid = 0;
> +}
> +
>  static int cap_dentry_open(struct file *file, const struct cred *cred)
>  {
>  	return 0;
> @@ -953,6 +958,7 @@ void __init security_fixup_ops(struct security_operations *ops)
>  	set_to_cap_if_null(ops, file_set_fowner);
>  	set_to_cap_if_null(ops, file_send_sigiotask);
>  	set_to_cap_if_null(ops, file_receive);
> +	set_to_cap_if_null(ops, file_getsecid);
>  	set_to_cap_if_null(ops, dentry_open);
>  	set_to_cap_if_null(ops, task_create);
>  	set_to_cap_if_null(ops, cred_alloc_blank);
> diff --git a/security/security.c b/security/security.c
> index 1011423..9973dab 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -688,6 +688,12 @@ int security_file_receive(struct file *file)
>  	return security_ops->file_receive(file);
>  }
>  
> +void security_file_getsecid(const struct file *file, u32 *secid)
> +{
> +	security_ops->file_getsecid(file, secid);
> +}
> +EXPORT_SYMBOL(security_file_getsecid);
> +
>  int security_dentry_open(struct file *file, const struct cred *cred)
>  {
>  	int ret;
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6772687..e1e787c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3179,6 +3179,12 @@ static int selinux_file_receive(struct file *file)
>  	return file_has_perm(cred, file, file_to_av(file));
>  }
>  
> +static void selinux_file_getsecid(const struct file *file, u32 *secid)
> +{
> +	struct file_security_struct *fsec = file->f_security;
> +	*secid = fsec->sid;
> +}
> +
>  static int selinux_dentry_open(struct file *file, const struct cred *cred)
>  {
>  	struct file_security_struct *fsec;
> @@ -5498,6 +5504,7 @@ static struct security_operations selinux_ops = {
>  	.file_set_fowner =		selinux_file_set_fowner,
>  	.file_send_sigiotask =		selinux_file_send_sigiotask,
>  	.file_receive =			selinux_file_receive,
> +	.file_getsecid =		selinux_file_getsecid,
>  
>  	.dentry_open =			selinux_dentry_open,
>  
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 6612ba1..a583736 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1304,6 +1304,16 @@ static int smack_file_receive(struct file *file)
>  	return smk_curacc(file->f_security, may, &ad);
>  }
>  
> +/**
> + * smack_file_getsecid - Extract file descriptor's security id
> + * @file: file descriptor to extract the info from
> + * @secid: where result will be saved
> + */
> +static void smack_file_getsecid(const struct file *file, u32 *secid)
> +{
> +	*secid = smack_to_secid(file->f_security);
> +}
> +
>  /*
>   * Task hooks
>   */
> @@ -3434,6 +3444,7 @@ struct security_operations smack_ops = {
>  	.file_set_fowner = 		smack_file_set_fowner,
>  	.file_send_sigiotask = 		smack_file_send_sigiotask,
>  	.file_receive = 		smack_file_receive,
> +	.file_getsecid =		smack_file_getsecid,
>  
>  	.cred_alloc_blank =		smack_cred_alloc_blank,
>  	.cred_free =			smack_cred_free,

Re: [RFC][PATCH 6/7] security: new LSM hook security_file_getsecid()

[Roberto Sassu]

On Thursday, April 28, 2011 01:50:58 AM Casey Schaufler wrote:
> On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> > The new LSM hook security_file_getsecid() and its implementation in the
> > capability module, SELinux and SMACK allows to obtain the security
> > identifier associated to a file descriptor.
> 
> Why do you want a secid? You have the security information from
> the file structure readily available.
> 
> What use to you intend to put this to?
> 

This new hook will be used in the IMA code (patch 7/7) to find inodes that
must be measured by checking the security label applied to file descriptors.
This new criteria allows to easily measure all inodes opened by a kernel
service because their file descriptors have the same label of what the
service provided with other credentials to the function dentry_open().


> > Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> > ---
> >  include/linux/security.h   |   12 ++++++++++++
> >  security/capability.c      |    6 ++++++
> >  security/security.c        |    6 ++++++
> >  security/selinux/hooks.c   |    7 +++++++
> >  security/smack/smack_lsm.c |   11 +++++++++++
> >  5 files changed, 42 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index ca02f17..6e73a1a 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -630,6 +630,11 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
> >   *	to receive an open file descriptor via socket IPC.
> >   *	@file contains the file structure being received.
> >   *	Return 0 if permission is granted.
> > + * @file_getsecid:
> > + *	Get the secid associated with the file descriptor.
> > + *	@file contains a pointer to the file descriptor.
> > + *	@secid contains a pointer to the location where result will be saved.
> > + *	In case of failure, @secid will be set to zero.
> >   *
> >   * Security hook for dentry
> >   *
> > @@ -1492,6 +1497,7 @@ struct security_operations {
> >  	int (*file_send_sigiotask) (struct task_struct *tsk,
> >  				    struct fown_struct *fown, int sig);
> >  	int (*file_receive) (struct file *file);
> > +	void (*file_getsecid)(const struct file *file, u32 *secid);
> >  	int (*dentry_open) (struct file *file, const struct cred *cred);
> >  
> >  	int (*task_create) (unsigned long clone_flags);
> > @@ -1751,6 +1757,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);
> > +void security_file_getsecid(const struct file *file, u32 *secid);
> >  int security_dentry_open(struct file *file, const struct cred *cred);
> >  int security_task_create(unsigned long clone_flags);
> >  int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
> > @@ -2251,6 +2258,11 @@ static inline int security_file_receive(struct file *file)
> >  	return 0;
> >  }
> >  
> > +static inline void security_file_getsecid(const struct file *file, u32 *secid)
> > +{
> > +	*secid = 0;
> > +}
> > +
> >  static inline int security_dentry_open(struct file *file,
> >  				       const struct cred *cred)
> >  {
> > diff --git a/security/capability.c b/security/capability.c
> > index 2984ea4..fcb569d 100644
> > --- a/security/capability.c
> > +++ b/security/capability.c
> > @@ -349,6 +349,11 @@ static int cap_file_receive(struct file *file)
> >  	return 0;
> >  }
> >  
> > +static void cap_file_getsecid(const struct file *file, u32 *secid)
> > +{
> > +	*secid = 0;
> > +}
> > +
> >  static int cap_dentry_open(struct file *file, const struct cred *cred)
> >  {
> >  	return 0;
> > @@ -953,6 +958,7 @@ void __init security_fixup_ops(struct security_operations *ops)
> >  	set_to_cap_if_null(ops, file_set_fowner);
> >  	set_to_cap_if_null(ops, file_send_sigiotask);
> >  	set_to_cap_if_null(ops, file_receive);
> > +	set_to_cap_if_null(ops, file_getsecid);
> >  	set_to_cap_if_null(ops, dentry_open);
> >  	set_to_cap_if_null(ops, task_create);
> >  	set_to_cap_if_null(ops, cred_alloc_blank);
> > diff --git a/security/security.c b/security/security.c
> > index 1011423..9973dab 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -688,6 +688,12 @@ int security_file_receive(struct file *file)
> >  	return security_ops->file_receive(file);
> >  }
> >  
> > +void security_file_getsecid(const struct file *file, u32 *secid)
> > +{
> > +	security_ops->file_getsecid(file, secid);
> > +}
> > +EXPORT_SYMBOL(security_file_getsecid);
> > +
> >  int security_dentry_open(struct file *file, const struct cred *cred)
> >  {
> >  	int ret;
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 6772687..e1e787c 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -3179,6 +3179,12 @@ static int selinux_file_receive(struct file *file)
> >  	return file_has_perm(cred, file, file_to_av(file));
> >  }
> >  
> > +static void selinux_file_getsecid(const struct file *file, u32 *secid)
> > +{
> > +	struct file_security_struct *fsec = file->f_security;
> > +	*secid = fsec->sid;
> > +}
> > +
> >  static int selinux_dentry_open(struct file *file, const struct cred *cred)
> >  {
> >  	struct file_security_struct *fsec;
> > @@ -5498,6 +5504,7 @@ static struct security_operations selinux_ops = {
> >  	.file_set_fowner =		selinux_file_set_fowner,
> >  	.file_send_sigiotask =		selinux_file_send_sigiotask,
> >  	.file_receive =			selinux_file_receive,
> > +	.file_getsecid =		selinux_file_getsecid,
> >  
> >  	.dentry_open =			selinux_dentry_open,
> >  
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index 6612ba1..a583736 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -1304,6 +1304,16 @@ static int smack_file_receive(struct file *file)
> >  	return smk_curacc(file->f_security, may, &ad);
> >  }
> >  
> > +/**
> > + * smack_file_getsecid - Extract file descriptor's security id
> > + * @file: file descriptor to extract the info from
> > + * @secid: where result will be saved
> > + */
> > +static void smack_file_getsecid(const struct file *file, u32 *secid)
> > +{
> > +	*secid = smack_to_secid(file->f_security);
> > +}
> > +
> >  /*
> >   * Task hooks
> >   */
> > @@ -3434,6 +3444,7 @@ struct security_operations smack_ops = {
> >  	.file_set_fowner = 		smack_file_set_fowner,
> >  	.file_send_sigiotask = 		smack_file_send_sigiotask,
> >  	.file_receive = 		smack_file_receive,
> > +	.file_getsecid =		smack_file_getsecid,
> >  
> >  	.cred_alloc_blank =		smack_cred_alloc_blank,
> >  	.cred_free =			smack_cred_free,
> 
> 

[RFC][PATCH 7/7] ima: added new LSM conditions in the policy

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 7619 bytes --]

The new parameters 'fowner_user', 'fowner_role' and 'fowner_type' are new
LSM conditions that allow to measure inodes whose opened file descriptor
has the label given as a value.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 Documentation/ABI/testing/ima_policy |    7 ++++-
 security/integrity/ima/ima.h         |    4 +-
 security/integrity/ima/ima_api.c     |    4 +-
 security/integrity/ima/ima_main.c    |    4 +-
 security/integrity/ima/ima_policy.c  |   45 +++++++++++++++++++++++++++++----
 5 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 6cd6dae..ee49345 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -18,7 +18,8 @@ Description:
 		condition:= base | lsm
 			base:	[[func=] [mask=] [fsmagic=] [uid=]]
 			lsm:	[[subj_user=] [subj_role=] [subj_type=]
-				 [obj_user=] [obj_role=] [obj_type=]]
+				 [obj_user=] [obj_role=] [obj_type=]
+				 [fowner_user=] [fowner_role=] [fowner_type=]]
 
 		base: 	func:= [BPRM_CHECK][FILE_MMAP][FILE_CHECK]
 			mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC]
@@ -46,6 +47,10 @@ Description:
 		all files mmapped executable in file_mmap, and all files
 		open for read by root in do_filp_open.
 
+		LSM conditions starting with obj_ refer to security attributes
+		of inodes while those starting with fowner_ involve file
+		descriptors.
+
 		Examples of LSM specific definitions:
 
 		SELinux:
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 08408bd..3a05625 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -110,7 +110,7 @@ struct ima_iint_cache {
 };
 
 /* LIM API function definitions */
-int ima_must_measure(struct inode *inode, int mask, int function);
+int ima_must_measure(struct file *file, int mask, int function);
 int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file);
 void ima_store_measurement(struct ima_iint_cache *iint, struct file *file,
 			   const unsigned char *filename);
@@ -128,7 +128,7 @@ struct ima_iint_cache *ima_iint_find(struct inode *inode);
 /* IMA policy related functions */
 enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
 
-int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask);
+int ima_match_policy(struct file *file, enum ima_hooks func, int mask);
 void ima_init_policy(void);
 void ima_update_policy(void);
 ssize_t ima_parse_add_rule(char *);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index da36d2c..d815392 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -108,11 +108,11 @@ err_out:
  * Return 0 to measure. For matching a DONT_MEASURE policy, no policy,
  * or other error, return an error code.
 */
-int ima_must_measure(struct inode *inode, int mask, int function)
+int ima_must_measure(struct file *file, int mask, int function)
 {
 	int must_measure;
 
-	must_measure = ima_match_policy(inode, function, mask);
+	must_measure = ima_match_policy(file, function, mask);
 	return must_measure ? 0 : -EACCES;
 }
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 39d66dc..9eaca61 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -65,7 +65,7 @@ static void ima_rdwr_violation_check(struct file *file)
 		goto out;
 	}
 
-	rc = ima_must_measure(inode, MAY_READ, FILE_CHECK);
+	rc = ima_must_measure(file, MAY_READ, FILE_CHECK);
 	if (rc < 0)
 		goto out;
 
@@ -127,7 +127,7 @@ static int process_measurement(struct file *file, const unsigned char *filename,
 	if (!ima_initialized || !S_ISREG(inode->i_mode))
 		return 0;
 
-	rc = ima_must_measure(inode, mask, function);
+	rc = ima_must_measure(file, mask, function);
 	if (rc != 0)
 		return rc;
 retry:
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d661afb..115c2e7 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -27,9 +27,10 @@
 
 enum ima_action { UNKNOWN = -1, DONT_MEASURE = 0, MEASURE };
 
-#define MAX_LSM_RULES 6
+#define MAX_LSM_RULES 9
 enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
-	LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE
+	LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE,
+	LSM_FOWNER_USER, LSM_FOWNER_ROLE, LSM_FOWNER_TYPE
 };
 
 struct ima_measure_rule_entry {
@@ -96,9 +97,10 @@ __setup("ima_tcb", default_policy_setup);
  * Returns true on rule match, false on failure.
  */
 static bool ima_match_rules(struct ima_measure_rule_entry *rule,
-			    struct inode *inode, enum ima_hooks func, int mask)
+			    struct file *file, enum ima_hooks func, int mask)
 {
 	struct task_struct *tsk = current;
+	struct inode *inode = file->f_dentry->d_inode;
 	int i;
 
 	if ((rule->flags & IMA_FUNC) && rule->func != func)
@@ -112,7 +114,7 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
 		return false;
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		int rc = 0;
-		u32 osid, sid;
+		u32 osid, sid, fsid;
 
 		if (!rule->lsm[i].rule)
 			continue;
@@ -137,6 +139,15 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
 							Audit_equal,
 							rule->lsm[i].rule,
 							NULL);
+		case LSM_FOWNER_USER:
+		case LSM_FOWNER_ROLE:
+		case LSM_FOWNER_TYPE:
+			security_file_getsecid(file, &fsid);
+			rc = security_filter_rule_match(fsid,
+							rule->lsm[i].type,
+							Audit_equal,
+							rule->lsm[i].rule,
+							NULL);
 		default:
 			break;
 		}
@@ -159,14 +170,14 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
  * as elements in the list are never deleted, nor does the list
  * change.)
  */
-int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask)
+int ima_match_policy(struct file *file, enum ima_hooks func, int mask)
 {
 	struct ima_measure_rule_entry *entry;
 
 	list_for_each_entry(entry, ima_measure, list) {
 		bool rc;
 
-		rc = ima_match_rules(entry, inode, func, mask);
+		rc = ima_match_rules(entry, file, func, mask);
 		if (rc)
 			return entry->action;
 	}
@@ -222,6 +233,7 @@ enum {
 	Opt_measure = 1, Opt_dont_measure,
 	Opt_obj_user, Opt_obj_role, Opt_obj_type,
 	Opt_subj_user, Opt_subj_role, Opt_subj_type,
+	Opt_fowner_user, Opt_fowner_role, Opt_fowner_type,
 	Opt_func, Opt_mask, Opt_fsmagic, Opt_uid
 };
 
@@ -234,6 +246,9 @@ static match_table_t policy_tokens = {
 	{Opt_subj_user, "subj_user=%s"},
 	{Opt_subj_role, "subj_role=%s"},
 	{Opt_subj_type, "subj_type=%s"},
+	{Opt_fowner_user, "fowner_user=%s"},
+	{Opt_fowner_role, "fowner_role=%s"},
+	{Opt_fowner_type, "fowner_type=%s"},
 	{Opt_func, "func=%s"},
 	{Opt_mask, "mask=%s"},
 	{Opt_fsmagic, "fsmagic=%s"},
@@ -407,6 +422,24 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
 						   LSM_SUBJ_TYPE,
 						   AUDIT_SUBJ_TYPE);
 			break;
+		case Opt_fowner_user:
+			ima_log_string(ab, "fowner_user", args[0].from);
+			result = ima_lsm_rule_init(entry, args[0].from,
+						   LSM_FOWNER_USER,
+						   AUDIT_SUBJ_USER);
+			break;
+		case Opt_fowner_role:
+			ima_log_string(ab, "fowner_role", args[0].from);
+			result = ima_lsm_rule_init(entry, args[0].from,
+						   LSM_FOWNER_ROLE,
+						   AUDIT_SUBJ_ROLE);
+			break;
+		case Opt_fowner_type:
+			ima_log_string(ab, "fowner_type", args[0].from);
+			result = ima_lsm_rule_init(entry, args[0].from,
+						   LSM_FOWNER_TYPE,
+						   AUDIT_SUBJ_TYPE);
+			break;
 		case Opt_err:
 			ima_log_string(ab, "UNKNOWN", p);
 			result = -EINVAL;
-- 
1.7.4.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

Re: [RFC][PATCH 7/7] ima: added new LSM conditions in the policy

[Mimi Zohar]

On Wed, 2011-04-27 at 14:34 +0200, Roberto Sassu wrote:
> The new parameters 'fowner_user', 'fowner_role' and 'fowner_type' are new
> LSM conditions that allow to measure inodes whose opened file descriptor
> has the label given as a value.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>

Nice. I really like the 'fowner_' prefix. If you don't object, I'll
change the ima-appraisal keyword from 'owner' to 'fowner' as well.

Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

thanks,

Mimi

> ---
>  Documentation/ABI/testing/ima_policy |    7 ++++-
>  security/integrity/ima/ima.h         |    4 +-
>  security/integrity/ima/ima_api.c     |    4 +-
>  security/integrity/ima/ima_main.c    |    4 +-
>  security/integrity/ima/ima_policy.c  |   45 +++++++++++++++++++++++++++++----
>  5 files changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 6cd6dae..ee49345 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -18,7 +18,8 @@ Description:
>  		condition:= base | lsm
>  			base:	[[func=] [mask=] [fsmagic=] [uid=]]
>  			lsm:	[[subj_user=] [subj_role=] [subj_type=]
> -				 [obj_user=] [obj_role=] [obj_type=]]
> +				 [obj_user=] [obj_role=] [obj_type=]
> +				 [fowner_user=] [fowner_role=] [fowner_type=]]
> 
>  		base: 	func:= [BPRM_CHECK][FILE_MMAP][FILE_CHECK]
>  			mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC]
> @@ -46,6 +47,10 @@ Description:
>  		all files mmapped executable in file_mmap, and all files
>  		open for read by root in do_filp_open.
> 
> +		LSM conditions starting with obj_ refer to security attributes
> +		of inodes while those starting with fowner_ involve file
> +		descriptors.
> +
>  		Examples of LSM specific definitions:
> 
>  		SELinux:
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 08408bd..3a05625 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -110,7 +110,7 @@ struct ima_iint_cache {
>  };
> 
>  /* LIM API function definitions */
> -int ima_must_measure(struct inode *inode, int mask, int function);
> +int ima_must_measure(struct file *file, int mask, int function);
>  int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file);
>  void ima_store_measurement(struct ima_iint_cache *iint, struct file *file,
>  			   const unsigned char *filename);
> @@ -128,7 +128,7 @@ struct ima_iint_cache *ima_iint_find(struct inode *inode);
>  /* IMA policy related functions */
>  enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
> 
> -int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask);
> +int ima_match_policy(struct file *file, enum ima_hooks func, int mask);
>  void ima_init_policy(void);
>  void ima_update_policy(void);
>  ssize_t ima_parse_add_rule(char *);
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index da36d2c..d815392 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -108,11 +108,11 @@ err_out:
>   * Return 0 to measure. For matching a DONT_MEASURE policy, no policy,
>   * or other error, return an error code.
>  */
> -int ima_must_measure(struct inode *inode, int mask, int function)
> +int ima_must_measure(struct file *file, int mask, int function)
>  {
>  	int must_measure;
> 
> -	must_measure = ima_match_policy(inode, function, mask);
> +	must_measure = ima_match_policy(file, function, mask);
>  	return must_measure ? 0 : -EACCES;
>  }
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 39d66dc..9eaca61 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -65,7 +65,7 @@ static void ima_rdwr_violation_check(struct file *file)
>  		goto out;
>  	}
> 
> -	rc = ima_must_measure(inode, MAY_READ, FILE_CHECK);
> +	rc = ima_must_measure(file, MAY_READ, FILE_CHECK);
>  	if (rc < 0)
>  		goto out;
> 
> @@ -127,7 +127,7 @@ static int process_measurement(struct file *file, const unsigned char *filename,
>  	if (!ima_initialized || !S_ISREG(inode->i_mode))
>  		return 0;
> 
> -	rc = ima_must_measure(inode, mask, function);
> +	rc = ima_must_measure(file, mask, function);
>  	if (rc != 0)
>  		return rc;
>  retry:
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index d661afb..115c2e7 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -27,9 +27,10 @@
> 
>  enum ima_action { UNKNOWN = -1, DONT_MEASURE = 0, MEASURE };
> 
> -#define MAX_LSM_RULES 6
> +#define MAX_LSM_RULES 9
>  enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
> -	LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE
> +	LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE,
> +	LSM_FOWNER_USER, LSM_FOWNER_ROLE, LSM_FOWNER_TYPE
>  };
> 
>  struct ima_measure_rule_entry {
> @@ -96,9 +97,10 @@ __setup("ima_tcb", default_policy_setup);
>   * Returns true on rule match, false on failure.
>   */
>  static bool ima_match_rules(struct ima_measure_rule_entry *rule,
> -			    struct inode *inode, enum ima_hooks func, int mask)
> +			    struct file *file, enum ima_hooks func, int mask)
>  {
>  	struct task_struct *tsk = current;
> +	struct inode *inode = file->f_dentry->d_inode;
>  	int i;
> 
>  	if ((rule->flags & IMA_FUNC) && rule->func != func)
> @@ -112,7 +114,7 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
>  		return false;
>  	for (i = 0; i < MAX_LSM_RULES; i++) {
>  		int rc = 0;
> -		u32 osid, sid;
> +		u32 osid, sid, fsid;
> 
>  		if (!rule->lsm[i].rule)
>  			continue;
> @@ -137,6 +139,15 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
>  							Audit_equal,
>  							rule->lsm[i].rule,
>  							NULL);
> +		case LSM_FOWNER_USER:
> +		case LSM_FOWNER_ROLE:
> +		case LSM_FOWNER_TYPE:
> +			security_file_getsecid(file, &fsid);
> +			rc = security_filter_rule_match(fsid,
> +							rule->lsm[i].type,
> +							Audit_equal,
> +							rule->lsm[i].rule,
> +							NULL);
>  		default:
>  			break;
>  		}
> @@ -159,14 +170,14 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
>   * as elements in the list are never deleted, nor does the list
>   * change.)
>   */
> -int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask)
> +int ima_match_policy(struct file *file, enum ima_hooks func, int mask)
>  {
>  	struct ima_measure_rule_entry *entry;
> 
>  	list_for_each_entry(entry, ima_measure, list) {
>  		bool rc;
> 
> -		rc = ima_match_rules(entry, inode, func, mask);
> +		rc = ima_match_rules(entry, file, func, mask);
>  		if (rc)
>  			return entry->action;
>  	}
> @@ -222,6 +233,7 @@ enum {
>  	Opt_measure = 1, Opt_dont_measure,
>  	Opt_obj_user, Opt_obj_role, Opt_obj_type,
>  	Opt_subj_user, Opt_subj_role, Opt_subj_type,
> +	Opt_fowner_user, Opt_fowner_role, Opt_fowner_type,
>  	Opt_func, Opt_mask, Opt_fsmagic, Opt_uid
>  };
> 
> @@ -234,6 +246,9 @@ static match_table_t policy_tokens = {
>  	{Opt_subj_user, "subj_user=%s"},
>  	{Opt_subj_role, "subj_role=%s"},
>  	{Opt_subj_type, "subj_type=%s"},
> +	{Opt_fowner_user, "fowner_user=%s"},
> +	{Opt_fowner_role, "fowner_role=%s"},
> +	{Opt_fowner_type, "fowner_type=%s"},
>  	{Opt_func, "func=%s"},
>  	{Opt_mask, "mask=%s"},
>  	{Opt_fsmagic, "fsmagic=%s"},
> @@ -407,6 +422,24 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
>  						   LSM_SUBJ_TYPE,
>  						   AUDIT_SUBJ_TYPE);
>  			break;
> +		case Opt_fowner_user:
> +			ima_log_string(ab, "fowner_user", args[0].from);
> +			result = ima_lsm_rule_init(entry, args[0].from,
> +						   LSM_FOWNER_USER,
> +						   AUDIT_SUBJ_USER);
> +			break;
> +		case Opt_fowner_role:
> +			ima_log_string(ab, "fowner_role", args[0].from);
> +			result = ima_lsm_rule_init(entry, args[0].from,
> +						   LSM_FOWNER_ROLE,
> +						   AUDIT_SUBJ_ROLE);
> +			break;
> +		case Opt_fowner_type:
> +			ima_log_string(ab, "fowner_type", args[0].from);
> +			result = ima_lsm_rule_init(entry, args[0].from,
> +						   LSM_FOWNER_TYPE,
> +						   AUDIT_SUBJ_TYPE);
> +			break;
>  		case Opt_err:
>  			ima_log_string(ab, "UNKNOWN", p);
>  			result = -EINVAL;

Re: [RFC][PATCH 7/7] ima: added new LSM conditions in the policy

[Roberto Sassu]

On Thursday, April 28, 2011 03:32:38 PM Mimi Zohar wrote:
> On Wed, 2011-04-27 at 14:34 +0200, Roberto Sassu wrote:
> > The new parameters 'fowner_user', 'fowner_role' and 'fowner_type' are new
> > LSM conditions that allow to measure inodes whose opened file descriptor
> > has the label given as a value.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> 
> Nice. I really like the 'fowner_' prefix. If you don't object, I'll
> change the ima-appraisal keyword from 'owner' to 'fowner' as well.
> 

Hi Mimi

i agree about this change.

Thanks

Roberto Sassu


> Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> 
> thanks,
> 
> Mimi
> 
> > ---
> >  Documentation/ABI/testing/ima_policy |    7 ++++-
> >  security/integrity/ima/ima.h         |    4 +-
> >  security/integrity/ima/ima_api.c     |    4 +-
> >  security/integrity/ima/ima_main.c    |    4 +-
> >  security/integrity/ima/ima_policy.c  |   45 +++++++++++++++++++++++++++++----
> >  5 files changed, 51 insertions(+), 13 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > index 6cd6dae..ee49345 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -18,7 +18,8 @@ Description:
> >  		condition:= base | lsm
> >  			base:	[[func=] [mask=] [fsmagic=] [uid=]]
> >  			lsm:	[[subj_user=] [subj_role=] [subj_type=]
> > -				 [obj_user=] [obj_role=] [obj_type=]]
> > +				 [obj_user=] [obj_role=] [obj_type=]
> > +				 [fowner_user=] [fowner_role=] [fowner_type=]]
> > 
> >  		base: 	func:= [BPRM_CHECK][FILE_MMAP][FILE_CHECK]
> >  			mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC]
> > @@ -46,6 +47,10 @@ Description:
> >  		all files mmapped executable in file_mmap, and all files
> >  		open for read by root in do_filp_open.
> > 
> > +		LSM conditions starting with obj_ refer to security attributes
> > +		of inodes while those starting with fowner_ involve file
> > +		descriptors.
> > +
> >  		Examples of LSM specific definitions:
> > 
> >  		SELinux:
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index 08408bd..3a05625 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -110,7 +110,7 @@ struct ima_iint_cache {
> >  };
> > 
> >  /* LIM API function definitions */
> > -int ima_must_measure(struct inode *inode, int mask, int function);
> > +int ima_must_measure(struct file *file, int mask, int function);
> >  int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file);
> >  void ima_store_measurement(struct ima_iint_cache *iint, struct file *file,
> >  			   const unsigned char *filename);
> > @@ -128,7 +128,7 @@ struct ima_iint_cache *ima_iint_find(struct inode *inode);
> >  /* IMA policy related functions */
> >  enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
> > 
> > -int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask);
> > +int ima_match_policy(struct file *file, enum ima_hooks func, int mask);
> >  void ima_init_policy(void);
> >  void ima_update_policy(void);
> >  ssize_t ima_parse_add_rule(char *);
> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index da36d2c..d815392 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -108,11 +108,11 @@ err_out:
> >   * Return 0 to measure. For matching a DONT_MEASURE policy, no policy,
> >   * or other error, return an error code.
> >  */
> > -int ima_must_measure(struct inode *inode, int mask, int function)
> > +int ima_must_measure(struct file *file, int mask, int function)
> >  {
> >  	int must_measure;
> > 
> > -	must_measure = ima_match_policy(inode, function, mask);
> > +	must_measure = ima_match_policy(file, function, mask);
> >  	return must_measure ? 0 : -EACCES;
> >  }
> > 
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 39d66dc..9eaca61 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -65,7 +65,7 @@ static void ima_rdwr_violation_check(struct file *file)
> >  		goto out;
> >  	}
> > 
> > -	rc = ima_must_measure(inode, MAY_READ, FILE_CHECK);
> > +	rc = ima_must_measure(file, MAY_READ, FILE_CHECK);
> >  	if (rc < 0)
> >  		goto out;
> > 
> > @@ -127,7 +127,7 @@ static int process_measurement(struct file *file, const unsigned char *filename,
> >  	if (!ima_initialized || !S_ISREG(inode->i_mode))
> >  		return 0;
> > 
> > -	rc = ima_must_measure(inode, mask, function);
> > +	rc = ima_must_measure(file, mask, function);
> >  	if (rc != 0)
> >  		return rc;
> >  retry:
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index d661afb..115c2e7 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -27,9 +27,10 @@
> > 
> >  enum ima_action { UNKNOWN = -1, DONT_MEASURE = 0, MEASURE };
> > 
> > -#define MAX_LSM_RULES 6
> > +#define MAX_LSM_RULES 9
> >  enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
> > -	LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE
> > +	LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE,
> > +	LSM_FOWNER_USER, LSM_FOWNER_ROLE, LSM_FOWNER_TYPE
> >  };
> > 
> >  struct ima_measure_rule_entry {
> > @@ -96,9 +97,10 @@ __setup("ima_tcb", default_policy_setup);
> >   * Returns true on rule match, false on failure.
> >   */
> >  static bool ima_match_rules(struct ima_measure_rule_entry *rule,
> > -			    struct inode *inode, enum ima_hooks func, int mask)
> > +			    struct file *file, enum ima_hooks func, int mask)
> >  {
> >  	struct task_struct *tsk = current;
> > +	struct inode *inode = file->f_dentry->d_inode;
> >  	int i;
> > 
> >  	if ((rule->flags & IMA_FUNC) && rule->func != func)
> > @@ -112,7 +114,7 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
> >  		return false;
> >  	for (i = 0; i < MAX_LSM_RULES; i++) {
> >  		int rc = 0;
> > -		u32 osid, sid;
> > +		u32 osid, sid, fsid;
> > 
> >  		if (!rule->lsm[i].rule)
> >  			continue;
> > @@ -137,6 +139,15 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
> >  							Audit_equal,
> >  							rule->lsm[i].rule,
> >  							NULL);
> > +		case LSM_FOWNER_USER:
> > +		case LSM_FOWNER_ROLE:
> > +		case LSM_FOWNER_TYPE:
> > +			security_file_getsecid(file, &fsid);
> > +			rc = security_filter_rule_match(fsid,
> > +							rule->lsm[i].type,
> > +							Audit_equal,
> > +							rule->lsm[i].rule,
> > +							NULL);
> >  		default:
> >  			break;
> >  		}
> > @@ -159,14 +170,14 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
> >   * as elements in the list are never deleted, nor does the list
> >   * change.)
> >   */
> > -int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask)
> > +int ima_match_policy(struct file *file, enum ima_hooks func, int mask)
> >  {
> >  	struct ima_measure_rule_entry *entry;
> > 
> >  	list_for_each_entry(entry, ima_measure, list) {
> >  		bool rc;
> > 
> > -		rc = ima_match_rules(entry, inode, func, mask);
> > +		rc = ima_match_rules(entry, file, func, mask);
> >  		if (rc)
> >  			return entry->action;
> >  	}
> > @@ -222,6 +233,7 @@ enum {
> >  	Opt_measure = 1, Opt_dont_measure,
> >  	Opt_obj_user, Opt_obj_role, Opt_obj_type,
> >  	Opt_subj_user, Opt_subj_role, Opt_subj_type,
> > +	Opt_fowner_user, Opt_fowner_role, Opt_fowner_type,
> >  	Opt_func, Opt_mask, Opt_fsmagic, Opt_uid
> >  };
> > 
> > @@ -234,6 +246,9 @@ static match_table_t policy_tokens = {
> >  	{Opt_subj_user, "subj_user=%s"},
> >  	{Opt_subj_role, "subj_role=%s"},
> >  	{Opt_subj_type, "subj_type=%s"},
> > +	{Opt_fowner_user, "fowner_user=%s"},
> > +	{Opt_fowner_role, "fowner_role=%s"},
> > +	{Opt_fowner_type, "fowner_type=%s"},
> >  	{Opt_func, "func=%s"},
> >  	{Opt_mask, "mask=%s"},
> >  	{Opt_fsmagic, "fsmagic=%s"},
> > @@ -407,6 +422,24 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
> >  						   LSM_SUBJ_TYPE,
> >  						   AUDIT_SUBJ_TYPE);
> >  			break;
> > +		case Opt_fowner_user:
> > +			ima_log_string(ab, "fowner_user", args[0].from);
> > +			result = ima_lsm_rule_init(entry, args[0].from,
> > +						   LSM_FOWNER_USER,
> > +						   AUDIT_SUBJ_USER);
> > +			break;
> > +		case Opt_fowner_role:
> > +			ima_log_string(ab, "fowner_role", args[0].from);
> > +			result = ima_lsm_rule_init(entry, args[0].from,
> > +						   LSM_FOWNER_ROLE,
> > +						   AUDIT_SUBJ_ROLE);
> > +			break;
> > +		case Opt_fowner_type:
> > +			ima_log_string(ab, "fowner_type", args[0].from);
> > +			result = ima_lsm_rule_init(entry, args[0].from,
> > +						   LSM_FOWNER_TYPE,
> > +						   AUDIT_SUBJ_TYPE);
> > +			break;
> >  		case Opt_err:
> >  			ima_log_string(ab, "UNKNOWN", p);
> >  			result = -EINVAL;
> 
> 
> 

Re: [RFC][PATCH 0/7] File descriptor labeling

[Casey Schaufler]

On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> File descriptor labeling issue
>
> Actually SELinux and SMACK assign to file descriptors the same label of the
> opening process and use it in LSM hooks security_file_permission(),
> security_file_fcntl() and others to verify if the 'current' process has the
> rights to perform the requested operation.
>
> Using the credentials of the 'current' process may be not appropriate in
> case a file descriptor is opened by a kernel service (i.e. a filesystem)
> and made shared among user processes. For instance, in a system with
> SELinux and eCryptfs, if the process A opens an encrypted file, eCryptfs
> obtains a file descriptor to access the correspondent inode in the lower
> filesystem, labeled with the A's label.
>
> If the process B accesses the same encrypted file, it needs the 'use'
> permission on the A's label other than permissions for the lower inode.
> However, if B is the first accessing process, A needs the 'use' permission
> on the B's label.
>
> The solution proposed is to modify those kernel services that deal with
> file descriptors to provide their set of credentials to dentry_open(), so
> that obtained objects are labeled with a unique label. In this way, in the
> above example, if eCryptfs provides its credentials with the label C to
> dentry_open(), all user processes need the 'use' permission only on C.
>
>
>
> File descriptor labeling and IMA
>
> The above proposal suggests to use the file descriptor label as a new
> criteria in the IMA policy to determine if a file must be measured. It will
> be possible to measure all files opened by a kernel service by simply
> writing a rule where the file descriptor label given as a value matches the
> one provided by the same service together with other credentials to the
> function dentry_open().
>
> In the above example, if eCryptfs provides its credentials with the label C
> to dentry_open(), it is possible to measure all inodes opened in the lower
> filesystem by specifying a rule like:
>
> fowner_type=C
>
>
> The benefits of this new criteria will be greater with the integration of
> EVM and the IMA appraisal feature in the kernel. ECryptfs can be used in
> conjunction with these components to verify the integrity of the content
> and extended attributes of encrypted files.
>
> Roberto Sassu
>
>
> Roberto Sassu (7):
>   fs: initialize file->f_cred with credentials provided
>   selinux: label new file descriptors using file->f_cred
>   smack: assign the label set in file->f_cred to new file descriptors
>   smack: fix label check in smack_kernel_act_as()
>   smack: import the security label in smack_secctx_to_secid()
>   security: new LSM hook security_file_getsecid()
>   ima: added new LSM conditions in the policy
>
>  Documentation/ABI/testing/ima_policy |    7 ++++-
>  fs/file_table.c                      |    5 +--
>  fs/internal.h                        |    2 +-
>  fs/namei.c                           |    2 +-
>  fs/open.c                            |    2 +-
>  include/linux/security.h             |   12 +++++++++
>  security/capability.c                |    6 ++++
>  security/integrity/ima/ima.h         |    4 +-
>  security/integrity/ima/ima_api.c     |    4 +-
>  security/integrity/ima/ima_main.c    |    4 +-
>  security/integrity/ima/ima_policy.c  |   45 +++++++++++++++++++++++++++++----
>  security/security.c                  |    6 ++++
>  security/selinux/hooks.c             |    9 ++++++-
>  security/smack/smack_lsm.c           |   23 +++++++++++++++--
>  14 files changed, 108 insertions(+), 23 deletions(-)

I have not given this patch set a complete review, but at first
glance I do not like it. You seem to be making a lot of assumptions
about the interactions between services and LSMs that I don't
know are valid. It is possible that I have not dug in deeply
enough to understand where this is headed. I will have a more
complete review in a bit.

Re: [RFC][PATCH 0/7] File descriptor labeling

[Casey Schaufler]

On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> File descriptor labeling issue
>
> Actually SELinux and SMACK assign to file descriptors the same label of the
> opening process and use it in LSM hooks security_file_permission(),
> security_file_fcntl() and others to verify if the 'current' process has the
> rights to perform the requested operation.
>
> Using the credentials of the 'current' process may be not appropriate in
> case a file descriptor is opened by a kernel service (i.e. a filesystem)
> and made shared among user processes. For instance, in a system with
> SELinux and eCryptfs, if the process A opens an encrypted file, eCryptfs
> obtains a file descriptor to access the correspondent inode in the lower
> filesystem, labeled with the A's label.
>
> If the process B accesses the same encrypted file, it needs the 'use'
> permission on the A's label other than permissions for the lower inode.
> However, if B is the first accessing process, A needs the 'use' permission
> on the B's label.

I am having trouble understanding the argument. I will pose my
question in Smack terms, as I can speak most definitively in them.

A process running with a Smack label "A" creates a file, and that
file gets labeled "A", as it ought. If eCryptfs is behaving correctly
this ought not change. If eCryptfs in encrypting the label it needs
to do so in such a way as to be able to decrypt it prior to
presentation to the vfs layer, where it will be used in an access
check. When the process running with a Smack label "B" comes along
the vfs code will check the fetched and possibly decrypted "A"
against "B" and, unless there is an explicit Smack rule in place
granting "B" access to "A", fail.

What is the problem? What is eCryptfs doing that prevents this
from working?

Re: [RFC][PATCH 0/7] File descriptor labeling

[Tyler Hicks]

On Wed Apr 27, 2011 at 01:19:55PM -0700, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> > File descriptor labeling issue
> >
> > Actually SELinux and SMACK assign to file descriptors the same label of the
> > opening process and use it in LSM hooks security_file_permission(),
> > security_file_fcntl() and others to verify if the 'current' process has the
> > rights to perform the requested operation.
> >
> > Using the credentials of the 'current' process may be not appropriate in
> > case a file descriptor is opened by a kernel service (i.e. a filesystem)
> > and made shared among user processes. For instance, in a system with
> > SELinux and eCryptfs, if the process A opens an encrypted file, eCryptfs
> > obtains a file descriptor to access the correspondent inode in the lower
> > filesystem, labeled with the A's label.
> >
> > If the process B accesses the same encrypted file, it needs the 'use'
> > permission on the A's label other than permissions for the lower inode.
> > However, if B is the first accessing process, A needs the 'use' permission
> > on the B's label.
> 
> I am having trouble understanding the argument. I will pose my
> question in Smack terms, as I can speak most definitively in them.
> 
> A process running with a Smack label "A" creates a file, and that
> file gets labeled "A", as it ought. If eCryptfs is behaving correctly
> this ought not change. If eCryptfs in encrypting the label it needs
> to do so in such a way as to be able to decrypt it prior to
> presentation to the vfs layer, where it will be used in an access
> check. When the process running with a Smack label "B" comes along
> the vfs code will check the fetched and possibly decrypted "A"
> against "B" and, unless there is an explicit Smack rule in place
> granting "B" access to "A", fail.
> 
> What is the problem? What is eCryptfs doing that prevents this
> from working?

Hi Casey - I think what Roberto is getting at is the way eCryptfs uses
only one lower file per eCryptfs inode. Imagine that there are 5
files open for ~/secret/foo at the eCryptfs layer, only 1 file is going
to be open in the lower filesystem and all eCryptfs file operations will
be multiplexed through it.

To make things more complicated, if the eCryptfs file is opened for
writing, the lower file must be opened for reading and writing. This is
because a write operation requires eCryptfs to vfs_read() from the lower
filesystem, decrypt that data and then vfs_write() the new data.

If the lower file can't be opened O_RDWR by the calling process, the
request is handed off to a kernel thread to open the lower file on
behalf of the calling process. It is definitely ugly.

Roberto, I hope I correctly described the situation that you're trying
to address. Can you tell me why we can't have a 1:1 mapping of eCryptfs
files to lower files?

Instead of having just one lower file attached to the eCryptfs inode, we
could have a list of opened files. There would be one for each eCryptfs
file that was opened. ecryptfs_writepage() would have to pick, in a
somewhat random fashion, one of the lower files to use. Of course, we
would still need to solve the problem of opening the lower file O_RDWR
when the calling process is only allowed write access (I may have just
answered my own question of why the 1:1 mapping technique won't solve
this problem).

Tyler

Re: [RFC][PATCH 0/7] File descriptor labeling

[Casey Schaufler]

On 4/27/2011 4:27 PM, Tyler Hicks wrote:
> On Wed Apr 27, 2011 at 01:19:55PM -0700, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 4/27/2011 5:34 AM, Roberto Sassu wrote:
>>> File descriptor labeling issue
>>>
>>> Actually SELinux and SMACK assign to file descriptors the same label of the
>>> opening process and use it in LSM hooks security_file_permission(),
>>> security_file_fcntl() and others to verify if the 'current' process has the
>>> rights to perform the requested operation.
>>>
>>> Using the credentials of the 'current' process may be not appropriate in
>>> case a file descriptor is opened by a kernel service (i.e. a filesystem)
>>> and made shared among user processes. For instance, in a system with
>>> SELinux and eCryptfs, if the process A opens an encrypted file, eCryptfs
>>> obtains a file descriptor to access the correspondent inode in the lower
>>> filesystem, labeled with the A's label.
>>>
>>> If the process B accesses the same encrypted file, it needs the 'use'
>>> permission on the A's label other than permissions for the lower inode.
>>> However, if B is the first accessing process, A needs the 'use' permission
>>> on the B's label.
>> I am having trouble understanding the argument. I will pose my
>> question in Smack terms, as I can speak most definitively in them.
>>
>> A process running with a Smack label "A" creates a file, and that
>> file gets labeled "A", as it ought. If eCryptfs is behaving correctly
>> this ought not change. If eCryptfs in encrypting the label it needs
>> to do so in such a way as to be able to decrypt it prior to
>> presentation to the vfs layer, where it will be used in an access
>> check. When the process running with a Smack label "B" comes along
>> the vfs code will check the fetched and possibly decrypted "A"
>> against "B" and, unless there is an explicit Smack rule in place
>> granting "B" access to "A", fail.
>>
>> What is the problem? What is eCryptfs doing that prevents this
>> from working?
> Hi Casey - I think what Roberto is getting at is the way eCryptfs uses
> only one lower file per eCryptfs inode. Imagine that there are 5
> files open for ~/secret/foo at the eCryptfs layer, only 1 file is going
> to be open in the lower filesystem and all eCryptfs file operations will
> be multiplexed through it.
>
> To make things more complicated, if the eCryptfs file is opened for
> writing, the lower file must be opened for reading and writing. This is
> because a write operation requires eCryptfs to vfs_read() from the lower
> filesystem, decrypt that data and then vfs_write() the new data.
>
> If the lower file can't be opened O_RDWR by the calling process, the
> request is handed off to a kernel thread to open the lower file on
> behalf of the calling process. It is definitely ugly.

Is eCryptfs handling xattrs? It needs to be if it isn't.

> Roberto, I hope I correctly described the situation that you're trying
> to address. Can you tell me why we can't have a 1:1 mapping of eCryptfs
> files to lower files?
>
> Instead of having just one lower file attached to the eCryptfs inode, we
> could have a list of opened files. There would be one for each eCryptfs
> file that was opened. ecryptfs_writepage() would have to pick, in a
> somewhat random fashion, one of the lower files to use. Of course, we
> would still need to solve the problem of opening the lower file O_RDWR
> when the calling process is only allowed write access (I may have just
> answered my own question of why the 1:1 mapping technique won't solve
> this problem).
>
> Tyler
>
>

Re: [RFC][PATCH 0/7] File descriptor labeling

[Tyler Hicks]

On Wed Apr 27, 2011 at 04:57:42PM -0700, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/27/2011 4:27 PM, Tyler Hicks wrote:
> > On Wed Apr 27, 2011 at 01:19:55PM -0700, Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> >>> File descriptor labeling issue
> >>>
> >>> Actually SELinux and SMACK assign to file descriptors the same label of the
> >>> opening process and use it in LSM hooks security_file_permission(),
> >>> security_file_fcntl() and others to verify if the 'current' process has the
> >>> rights to perform the requested operation.
> >>>
> >>> Using the credentials of the 'current' process may be not appropriate in
> >>> case a file descriptor is opened by a kernel service (i.e. a filesystem)
> >>> and made shared among user processes. For instance, in a system with
> >>> SELinux and eCryptfs, if the process A opens an encrypted file, eCryptfs
> >>> obtains a file descriptor to access the correspondent inode in the lower
> >>> filesystem, labeled with the A's label.
> >>>
> >>> If the process B accesses the same encrypted file, it needs the 'use'
> >>> permission on the A's label other than permissions for the lower inode.
> >>> However, if B is the first accessing process, A needs the 'use' permission
> >>> on the B's label.
> >> I am having trouble understanding the argument. I will pose my
> >> question in Smack terms, as I can speak most definitively in them.
> >>
> >> A process running with a Smack label "A" creates a file, and that
> >> file gets labeled "A", as it ought. If eCryptfs is behaving correctly
> >> this ought not change. If eCryptfs in encrypting the label it needs
> >> to do so in such a way as to be able to decrypt it prior to
> >> presentation to the vfs layer, where it will be used in an access
> >> check. When the process running with a Smack label "B" comes along
> >> the vfs code will check the fetched and possibly decrypted "A"
> >> against "B" and, unless there is an explicit Smack rule in place
> >> granting "B" access to "A", fail.
> >>
> >> What is the problem? What is eCryptfs doing that prevents this
> >> from working?
> > Hi Casey - I think what Roberto is getting at is the way eCryptfs uses
> > only one lower file per eCryptfs inode. Imagine that there are 5
> > files open for ~/secret/foo at the eCryptfs layer, only 1 file is going
> > to be open in the lower filesystem and all eCryptfs file operations will
> > be multiplexed through it.
> >
> > To make things more complicated, if the eCryptfs file is opened for
> > writing, the lower file must be opened for reading and writing. This is
> > because a write operation requires eCryptfs to vfs_read() from the lower
> > filesystem, decrypt that data and then vfs_write() the new data.
> >
> > If the lower file can't be opened O_RDWR by the calling process, the
> > request is handed off to a kernel thread to open the lower file on
> > behalf of the calling process. It is definitely ugly.
> 
> Is eCryptfs handling xattrs? It needs to be if it isn't.

Yes - we pass the calls to the lower filesystem and return the result.

The only catch would be that users can opt to have eCryptfs store the
crypto metadata in user.ecryptfs instead of the first 8k of the file.

Tyler

> 
> > Roberto, I hope I correctly described the situation that you're trying
> > to address. Can you tell me why we can't have a 1:1 mapping of eCryptfs
> > files to lower files?
> >
> > Instead of having just one lower file attached to the eCryptfs inode, we
> > could have a list of opened files. There would be one for each eCryptfs
> > file that was opened. ecryptfs_writepage() would have to pick, in a
> > somewhat random fashion, one of the lower files to use. Of course, we
> > would still need to solve the problem of opening the lower file O_RDWR
> > when the calling process is only allowed write access (I may have just
> > answered my own question of why the 1:1 mapping technique won't solve
> > this problem).
> >
> > Tyler
> >
> >
> 

Re: [RFC][PATCH 0/7] File descriptor labeling

[Roberto Sassu]

On Thursday, April 28, 2011 01:27:19 AM Tyler Hicks wrote:
> On Wed Apr 27, 2011 at 01:19:55PM -0700, Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> > > File descriptor labeling issue
> > >
> > > Actually SELinux and SMACK assign to file descriptors the same label of the
> > > opening process and use it in LSM hooks security_file_permission(),
> > > security_file_fcntl() and others to verify if the 'current' process has the
> > > rights to perform the requested operation.
> > >
> > > Using the credentials of the 'current' process may be not appropriate in
> > > case a file descriptor is opened by a kernel service (i.e. a filesystem)
> > > and made shared among user processes. For instance, in a system with
> > > SELinux and eCryptfs, if the process A opens an encrypted file, eCryptfs
> > > obtains a file descriptor to access the correspondent inode in the lower
> > > filesystem, labeled with the A's label.
> > >
> > > If the process B accesses the same encrypted file, it needs the 'use'
> > > permission on the A's label other than permissions for the lower inode.
> > > However, if B is the first accessing process, A needs the 'use' permission
> > > on the B's label.
> > 
> > I am having trouble understanding the argument. I will pose my
> > question in Smack terms, as I can speak most definitively in them.
> > 
> > A process running with a Smack label "A" creates a file, and that
> > file gets labeled "A", as it ought. If eCryptfs is behaving correctly
> > this ought not change. If eCryptfs in encrypting the label it needs
> > to do so in such a way as to be able to decrypt it prior to
> > presentation to the vfs layer, where it will be used in an access
> > check. When the process running with a Smack label "B" comes along
> > the vfs code will check the fetched and possibly decrypted "A"
> > against "B" and, unless there is an explicit Smack rule in place
> > granting "B" access to "A", fail.
> > 
> > What is the problem? What is eCryptfs doing that prevents this
> > from working?
> 
> Hi Casey - I think what Roberto is getting at is the way eCryptfs uses
> only one lower file per eCryptfs inode. Imagine that there are 5
> files open for ~/secret/foo at the eCryptfs layer, only 1 file is going
> to be open in the lower filesystem and all eCryptfs file operations will
> be multiplexed through it.
> 
> To make things more complicated, if the eCryptfs file is opened for
> writing, the lower file must be opened for reading and writing. This is
> because a write operation requires eCryptfs to vfs_read() from the lower
> filesystem, decrypt that data and then vfs_write() the new data.
> 
> If the lower file can't be opened O_RDWR by the calling process, the
> request is handed off to a kernel thread to open the lower file on
> behalf of the calling process. It is definitely ugly.
> 
> Roberto, I hope I correctly described the situation that you're trying
> to address. Can you tell me why we can't have a 1:1 mapping of eCryptfs
> files to lower files?
> 
> Instead of having just one lower file attached to the eCryptfs inode, we
> could have a list of opened files. There would be one for each eCryptfs
> file that was opened. ecryptfs_writepage() would have to pick, in a
> somewhat random fashion, one of the lower files to use. Of course, we
> would still need to solve the problem of opening the lower file O_RDWR
> when the calling process is only allowed write access (I may have just
> answered my own question of why the 1:1 mapping technique won't solve
> this problem).
> 

Hi Tyler

i think the 1:1 mapping isn't necessary at least from the security perspective.
Since eCryptfs is a stacked filesystem access control is performed on
both the upper and the lower layer.
ECryptfs relies on the lower filesystem for the management of extended
attributes, so this means that the security label of both the upper and
the lower inodes is the same (however this is not the current behavior
in SELinux, which assigns the label 'ecryptfs_t' to the upper inode).
In my view, for this reason the access control checks can be performed
only at the upper layer, letting eCryptfs full privileges to access inodes
in the lower filesystem.
This solves the problem of opening the lower file in r/w mode even if only
the read is requested, because at the upper layer the subject is the
accessing process with its own credentials which needs the read permission
and at the lower layer the subject is the eCryptfs kernel module with
unlimited privileges.
The issue i described in the cover letter is related to the label assigned
to the file descriptor obtained by eCryptfs (or another kernel service) when
opening an inode in the lower filesystem, which actually depends on the
first accessing process.
This label is checked against the credentials of the 'current' process in the
hook security_file_permission(), which is triggered by vfs calls (read, write,
readdir) performed on both the upper and the lower inodes.
In SELinux, a process needs the permission to 'use' a opened file descriptor.
So, having a fixed label helps in defining the rule that must be added in the
policy for eCryptfs to ensure it works properly.


PS: i'm adding in CC the Apparmor's mantainer and the mailing list to have
their opinion about the protection offered for the eCryptfs filesystem and
other kernel services. The overall thread is available at the url:

https://lkml.org/lkml/2011/4/27/201

Thanks

Roberto Sassu


> Tyler
> 

Re: [RFC][PATCH 0/7] File descriptor labeling

[Casey Schaufler]

On 4/28/2011 5:35 AM, Roberto Sassu wrote:
> On Thursday, April 28, 2011 01:27:19 AM Tyler Hicks wrote:
>> On Wed Apr 27, 2011 at 01:19:55PM -0700, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 4/27/2011 5:34 AM, Roberto Sassu wrote:
>>>> File descriptor labeling issue
>>>>
>>>> Actually SELinux and SMACK assign to file descriptors the same label of the
>>>> opening process and use it in LSM hooks security_file_permission(),
>>>> security_file_fcntl() and others to verify if the 'current' process has the
>>>> rights to perform the requested operation.
>>>>
>>>> Using the credentials of the 'current' process may be not appropriate in
>>>> case a file descriptor is opened by a kernel service (i.e. a filesystem)
>>>> and made shared among user processes. For instance, in a system with
>>>> SELinux and eCryptfs, if the process A opens an encrypted file, eCryptfs
>>>> obtains a file descriptor to access the correspondent inode in the lower
>>>> filesystem, labeled with the A's label.
>>>>
>>>> If the process B accesses the same encrypted file, it needs the 'use'
>>>> permission on the A's label other than permissions for the lower inode.
>>>> However, if B is the first accessing process, A needs the 'use' permission
>>>> on the B's label.
>>> I am having trouble understanding the argument. I will pose my
>>> question in Smack terms, as I can speak most definitively in them.
>>>
>>> A process running with a Smack label "A" creates a file, and that
>>> file gets labeled "A", as it ought. If eCryptfs is behaving correctly
>>> this ought not change. If eCryptfs in encrypting the label it needs
>>> to do so in such a way as to be able to decrypt it prior to
>>> presentation to the vfs layer, where it will be used in an access
>>> check. When the process running with a Smack label "B" comes along
>>> the vfs code will check the fetched and possibly decrypted "A"
>>> against "B" and, unless there is an explicit Smack rule in place
>>> granting "B" access to "A", fail.
>>>
>>> What is the problem? What is eCryptfs doing that prevents this
>>> from working?
>> Hi Casey - I think what Roberto is getting at is the way eCryptfs uses
>> only one lower file per eCryptfs inode. Imagine that there are 5
>> files open for ~/secret/foo at the eCryptfs layer, only 1 file is going
>> to be open in the lower filesystem and all eCryptfs file operations will
>> be multiplexed through it.
>>
>> To make things more complicated, if the eCryptfs file is opened for
>> writing, the lower file must be opened for reading and writing. This is
>> because a write operation requires eCryptfs to vfs_read() from the lower
>> filesystem, decrypt that data and then vfs_write() the new data.
>>
>> If the lower file can't be opened O_RDWR by the calling process, the
>> request is handed off to a kernel thread to open the lower file on
>> behalf of the calling process. It is definitely ugly.
>>
>> Roberto, I hope I correctly described the situation that you're trying
>> to address. Can you tell me why we can't have a 1:1 mapping of eCryptfs
>> files to lower files?
>>
>> Instead of having just one lower file attached to the eCryptfs inode, we
>> could have a list of opened files. There would be one for each eCryptfs
>> file that was opened. ecryptfs_writepage() would have to pick, in a
>> somewhat random fashion, one of the lower files to use. Of course, we
>> would still need to solve the problem of opening the lower file O_RDWR
>> when the calling process is only allowed write access (I may have just
>> answered my own question of why the 1:1 mapping technique won't solve
>> this problem).
>>
> Hi Tyler
>
> i think the 1:1 mapping isn't necessary at least from the security perspective.
> Since eCryptfs is a stacked filesystem access control is performed on
> both the upper and the lower layer.
> ECryptfs relies on the lower filesystem for the management of extended
> attributes, so this means that the security label of both the upper and
> the lower inodes is the same (however this is not the current behavior
> in SELinux, which assigns the label 'ecryptfs_t' to the upper inode).

Where does this assignment occur?

> In my view, for this reason the access control checks can be performed
> only at the upper layer, letting eCryptfs full privileges to access inodes
> in the lower filesystem.

On this point I most strongly disagree.

The behavior of a filesystem and the data that it uses to determine
that behavior is wrought with complex interactions which may include
but are not limited to caching, read-aheads, garbage collection,
and various side effects of access control. If eCryptfs needs to go
mucking about with the data used by the underlying filesystem it is
not stacking properly. A stacked filesystem has no business whatever
changing the data of the underlying filesystem.

> This solves the problem of opening the lower file in r/w mode even if only
> the read is requested, because at the upper layer the subject is the
> accessing process with its own credentials which needs the read permission
> and at the lower layer the subject is the eCryptfs kernel module with
> unlimited privileges.

Excuse my ignorance for a moment. Is eCryptfs a user mode filesystem,
or in the kernel properly? The behavior makes it sound like the former
while the interfaces you're requesting make it seem like the latter.

> The issue i described in the cover letter is related to the label assigned
> to the file descriptor obtained by eCryptfs (or another kernel service) when
> opening an inode in the lower filesystem, which actually depends on the
> first accessing process.
> This label is checked against the credentials of the 'current' process in the
> hook security_file_permission(), which is triggered by vfs calls (read, write,
> readdir) performed on both the upper and the lower inodes.
> In SELinux, a process needs the permission to 'use' a opened file descriptor.
> So, having a fixed label helps in defining the rule that must be added in the
> policy for eCryptfs to ensure it works properly.

I'm afraid to suggest this, but it looks as if you may be
able to solve your problems with some SELinux policy. I am
of course not an expert on SELinux policy, but it looks as
if not specifying an appropriate policy for the user space
component is what is in the way here.

> PS: i'm adding in CC the Apparmor's mantainer and the mailing list to have
> their opinion about the protection offered for the eCryptfs filesystem and
> other kernel services. The overall thread is available at the url:
>
> https://lkml.org/lkml/2011/4/27/201
>
> Thanks
>
> Roberto Sassu
>
>
>> Tyler
>>
>

Re: [RFC][PATCH 0/7] File descriptor labeling

[Eric Paris]

On Thu, Apr 28, 2011 at 1:37 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/28/2011 5:35 AM, Roberto Sassu wrote:
>> On Thursday, April 28, 2011 01:27:19 AM Tyler Hicks wrote:
>>> On Wed Apr 27, 2011 at 01:19:55PM -0700, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 4/27/2011 5:34 AM, Roberto Sassu wrote:

> On this point I most strongly disagree.


Casey, I'm glad you're trying to figure out what's going on here
because I certainly don't understand all the problems!

If there were some mechanism by which the 'lower' inode could be ONLY
accessibly by ecyptfs kernel internals it oculd be marked IS_PRIVATE
and skip all security checks on it.  Then you only have SELinux
security checks on the upper inode.  Which seems to make sense.  My
problem is that this is ONLY acceptable if there is no way for
userspace to directly reference the lower struct inode.

Just a thought.

-Eric

Re: [RFC][PATCH 0/7] File descriptor labeling

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 9130 bytes --]

On Thursday, April 28, 2011 07:37:29 PM Casey Schaufler wrote:
> On 4/28/2011 5:35 AM, Roberto Sassu wrote:
> > On Thursday, April 28, 2011 01:27:19 AM Tyler Hicks wrote:
> >> On Wed Apr 27, 2011 at 01:19:55PM -0700, Casey Schaufler <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> wrote:
> >>> On 4/27/2011 5:34 AM, Roberto Sassu wrote:
> >>>> File descriptor labeling issue
> >>>>
> >>>> Actually SELinux and SMACK assign to file descriptors the same label of the
> >>>> opening process and use it in LSM hooks security_file_permission(),
> >>>> security_file_fcntl() and others to verify if the 'current' process has the
> >>>> rights to perform the requested operation.
> >>>>
> >>>> Using the credentials of the 'current' process may be not appropriate in
> >>>> case a file descriptor is opened by a kernel service (i.e. a filesystem)
> >>>> and made shared among user processes. For instance, in a system with
> >>>> SELinux and eCryptfs, if the process A opens an encrypted file, eCryptfs
> >>>> obtains a file descriptor to access the correspondent inode in the lower
> >>>> filesystem, labeled with the A's label.
> >>>>
> >>>> If the process B accesses the same encrypted file, it needs the 'use'
> >>>> permission on the A's label other than permissions for the lower inode.
> >>>> However, if B is the first accessing process, A needs the 'use' permission
> >>>> on the B's label.
> >>> I am having trouble understanding the argument. I will pose my
> >>> question in Smack terms, as I can speak most definitively in them.
> >>>
> >>> A process running with a Smack label "A" creates a file, and that
> >>> file gets labeled "A", as it ought. If eCryptfs is behaving correctly
> >>> this ought not change. If eCryptfs in encrypting the label it needs
> >>> to do so in such a way as to be able to decrypt it prior to
> >>> presentation to the vfs layer, where it will be used in an access
> >>> check. When the process running with a Smack label "B" comes along
> >>> the vfs code will check the fetched and possibly decrypted "A"
> >>> against "B" and, unless there is an explicit Smack rule in place
> >>> granting "B" access to "A", fail.
> >>>
> >>> What is the problem? What is eCryptfs doing that prevents this
> >>> from working?
> >> Hi Casey - I think what Roberto is getting at is the way eCryptfs uses
> >> only one lower file per eCryptfs inode. Imagine that there are 5
> >> files open for ~/secret/foo at the eCryptfs layer, only 1 file is going
> >> to be open in the lower filesystem and all eCryptfs file operations will
> >> be multiplexed through it.
> >>
> >> To make things more complicated, if the eCryptfs file is opened for
> >> writing, the lower file must be opened for reading and writing. This is
> >> because a write operation requires eCryptfs to vfs_read() from the lower
> >> filesystem, decrypt that data and then vfs_write() the new data.
> >>
> >> If the lower file can't be opened O_RDWR by the calling process, the
> >> request is handed off to a kernel thread to open the lower file on
> >> behalf of the calling process. It is definitely ugly.
> >>
> >> Roberto, I hope I correctly described the situation that you're trying
> >> to address. Can you tell me why we can't have a 1:1 mapping of eCryptfs
> >> files to lower files?
> >>
> >> Instead of having just one lower file attached to the eCryptfs inode, we
> >> could have a list of opened files. There would be one for each eCryptfs
> >> file that was opened. ecryptfs_writepage() would have to pick, in a
> >> somewhat random fashion, one of the lower files to use. Of course, we
> >> would still need to solve the problem of opening the lower file O_RDWR
> >> when the calling process is only allowed write access (I may have just
> >> answered my own question of why the 1:1 mapping technique won't solve
> >> this problem).
> >>
> > Hi Tyler
> >
> > i think the 1:1 mapping isn't necessary at least from the security perspective.
> > Since eCryptfs is a stacked filesystem access control is performed on
> > both the upper and the lower layer.
> > ECryptfs relies on the lower filesystem for the management of extended
> > attributes, so this means that the security label of both the upper and
> > the lower inodes is the same (however this is not the current behavior
> > in SELinux, which assigns the label 'ecryptfs_t' to the upper inode).
> 
> Where does this assignment occur?
> 

Hi Casey

The assignment happens at the inode's initialization time and depends on
the behavior configured for a specific filesystem.
The SELinux reference policy actually configures static labeling for inodes
in the eCryptfs filesystem while for example allows ext4 inodes to be
initialized using extended attributes.


> > In my view, for this reason the access control checks can be performed
> > only at the upper layer, letting eCryptfs full privileges to access inodes
> > in the lower filesystem.
> 
> On this point I most strongly disagree.
> 
> The behavior of a filesystem and the data that it uses to determine
> that behavior is wrought with complex interactions which may include
> but are not limited to caching, read-aheads, garbage collection,
> and various side effects of access control. If eCryptfs needs to go
> mucking about with the data used by the underlying filesystem it is
> not stacking properly. A stacked filesystem has no business whatever
> changing the data of the underlying filesystem.
> 

Ok, probably i have to go more in deep to explain how access control is
performed on eCryptfs. I'm talking for the SELinux's case, so SELinux experts
can correct me if i'm wrong.

First, i want to use extended attributes to initialize an eCryptfs inode, so
both the upper and the lower inodes have the same security context
because eCryptfs calls the *xattr() methods of the underlying filesystem.

Then, the process A accesses the eCryptfs inode 'I'. On the upper layer access
control is performed using the credentials of the 'current' process and the
security context of I.

If the process is allowed to access the upper inode, eCryptfs opens the lower
inode, presenting its own credentials with the type 'kernel_t'. Then, SELinux
checks if 'kernel_t' can access the inode with the security context of 'I'. If so,
eCryptfs obtains a file descriptor which, in this patch set, is labeled using the
same credentials it provided at open time and binds it to the upper inode.

Suppose for a moment that A is working on the eCryptfs inode 'I' and the process
B requests the same inode. ECryptfs uses the already opened file descriptor and
B will be granted to open the requested inode if:

- B has the 'open' permission on the upper inode 'I';


and can perform read/write operations if:

- B has the 'read' and 'write' permissions on the upper inode 'I';
- B is allowed to use the file descriptor opened by eCryptfs ('kernel_t').
- B has the 'read' and 'write' permissions on the lower inode I;


What i miss to say in my previous email is that even if eCryptfs is granted
to access inodes in the lower filesystem with full privileges, user processes
still need the permission to deal with the lower inode, but the check on the
upper inode is sufficient because the security context is the same.

Roberto Sassu


> > This solves the problem of opening the lower file in r/w mode even if only
> > the read is requested, because at the upper layer the subject is the
> > accessing process with its own credentials which needs the read permission
> > and at the lower layer the subject is the eCryptfs kernel module with
> > unlimited privileges.
> 
> Excuse my ignorance for a moment. Is eCryptfs a user mode filesystem,
> or in the kernel properly? The behavior makes it sound like the former
> while the interfaces you're requesting make it seem like the latter.
> 
> > The issue i described in the cover letter is related to the label assigned
> > to the file descriptor obtained by eCryptfs (or another kernel service) when
> > opening an inode in the lower filesystem, which actually depends on the
> > first accessing process.
> > This label is checked against the credentials of the 'current' process in the
> > hook security_file_permission(), which is triggered by vfs calls (read, write,
> > readdir) performed on both the upper and the lower inodes.
> > In SELinux, a process needs the permission to 'use' a opened file descriptor.
> > So, having a fixed label helps in defining the rule that must be added in the
> > policy for eCryptfs to ensure it works properly.
> 
> I'm afraid to suggest this, but it looks as if you may be
> able to solve your problems with some SELinux policy. I am
> of course not an expert on SELinux policy, but it looks as
> if not specifying an appropriate policy for the user space
> component is what is in the way here.
> 
> > PS: i'm adding in CC the Apparmor's mantainer and the mailing list to have
> > their opinion about the protection offered for the eCryptfs filesystem and
> > other kernel services. The overall thread is available at the url:
> >
> > https://lkml.org/lkml/2011/4/27/201
> >
> > Thanks
> >
> > Roberto Sassu
> >
> >
> >> Tyler
> >>
> >
> 
> 

[-- Attachment #2: Type: text/html, Size: 37554 bytes --]