Re: SELinux metadata protection

Re: SELinux metadata protection

[Serge E. Hallyn]

Quoting KaiGai Kohei (kaigai@kaigai.gr.jp):
> It seems a bit curious behavior for me. Why can an unauthorized process
> be allowed to know whether the file exists or not ?
> I think it's worthwhile to conceal the existence of files from unauthorized
> processes.

This behavior is also necessary for meeting the new medium robustness
protection profiles.

> Thanks, any comments please, and have a good year :)

Haven't looked at the patch in detail, but the following stood out to me:

> --- linux-2.6.14.5-selinux/fs/readdir.c	2005-12-26 19:26:33.000000000 -0500
> +++ linux-2.6.14.5-selinux.mp/fs/readdir.c	2005-12-29 20:26:55.000000000 -0500
> @@ -33,7 +33,11 @@ int vfs_readdir(struct file *file, filld
>  	down(&inode->i_sem);
>  	res = -ENOENT;
>  	if (!IS_DEADDIR(inode)) {
> -		res = file->f_op->readdir(file, buf, filler);
> +		/* NOTE:
> +		   When LSM was not enable, security_file_readdir()
> +		   is same as 'file->f_op->readdir()'. 
> +		*/
> +		res = security_file_readdir(file, buf, filler);
>  		file_accessed(file);

I don't like this - the dir->f_op->readdir should not be done inside
a function which claims to be a security check.  Plus the added code
doesn't have a return value of it's own.  So why not stay closer to usual
linux code and do something like

	security_prep_readdir();
	res = file->f_op->readdir(file, buf, filler);

inside vfs_readdir(), where security_prep_readdir() is defined away
in the non-LSM case, and is 

> +static inline void security_file_readdir (struct file *dir, void *buffer, filldir_t filler)
> +{
> +	security_filldir_t private;
> +
> +	private.dir = dir;
> +	private.buffer = buffer;
> +	private.filler = filler;

in the LSM case?

thanks,
-serge

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

Re: SELinux metadata protection

[KaiGai Kohei]

Hi, thanks for your comments.

Serge E. Hallyn wrote:
> Quoting KaiGai Kohei (kaigai@kaigai.gr.jp):
> 
>>It seems a bit curious behavior for me. Why can an unauthorized process
>>be allowed to know whether the file exists or not ?
>>I think it's worthwhile to conceal the existence of files from unauthorized
>>processes.
> 
> 
> This behavior is also necessary for meeting the new medium robustness
> protection profiles.

Hmm... I'm not so familiar with ISO-15408.
Do you know previous discussions about such behavior on filename resolving or readdir ?

>>--- linux-2.6.14.5-selinux/fs/readdir.c	2005-12-26 19:26:33.000000000 -0500
>>+++ linux-2.6.14.5-selinux.mp/fs/readdir.c	2005-12-29 20:26:55.000000000 -0500
>>@@ -33,7 +33,11 @@ int vfs_readdir(struct file *file, filld
>> 	down(&inode->i_sem);
>> 	res = -ENOENT;
>> 	if (!IS_DEADDIR(inode)) {
>>-		res = file->f_op->readdir(file, buf, filler);
>>+		/* NOTE:
>>+		   When LSM was not enable, security_file_readdir()
>>+		   is same as 'file->f_op->readdir()'. 
>>+		*/
>>+		res = security_file_readdir(file, buf, filler);
>> 		file_accessed(file);
> 
> 
> I don't like this - the dir->f_op->readdir should not be done inside
> a function which claims to be a security check.  Plus the added code
> doesn't have a return value of it's own.  So why not stay closer to usual
> linux code and do something like
> 
> 	security_prep_readdir();
> 	res = file->f_op->readdir(file, buf, filler);
> 
> inside vfs_readdir(), where security_prep_readdir() is defined away
> in the non-LSM case, and is 
> 
> 
>>+static inline void security_file_readdir (struct file *dir, void *buffer, filldir_t filler)
>>+{
>>+	security_filldir_t private;
>>+
>>+	private.dir = dir;
>>+	private.buffer = buffer;
>>+	private.filler = filler;
> 
> 
> in the LSM case?

Indeed, file->f_op->readdir() need not necessary to be called inside LSM function.
But overwriting 'filler' is necessary to check permission for each directory entry.
I agree your suggestion if security_prep_readdir() is defined as macro, and
this can overwrite 'filler' and 'buf'.

Pay attention to what 'filler' was overwritten in this patch.
When LSM is enable, FS's readdir method (dir->f_op->readdir) always calls 'security_file_filldir'
for each directory entry instead of original 'filler' given by arguments.
When 'security_file_filldir' is called for authorized directory entry, it calls original 'filler'
for writing the entry into userspace. But 'security_file_filldir' is called for unauthorized
directory entry, it skips to call original 'filler'.
In the result, only authorized directory entries are returned to userspace.
The above code is necessary to implement such behavior.

* in normal case
vfs_readdir()
  -> FS's readdir method
     - entry-1  -> filldir("entry-1")
     - entry-2  -> filldir("entry-2")
          :
     - entry-N  -> filldir("entry-N")
RESULT: all entries are returned to userspace.

* in metadata protection case
vfs_readdir()
  -> FS's readdir method
     - entry-1  -> security_file_filldir("entry-1") -> filldir("entry-1), if permitted
     - entry-2  -> security_file_filldir("entry-2") -> filldir("entry-2), if permitted
          :
     - entry-N  -> security_file_filldir("entry-N") -> filldir("entry-N), if permitted
RESULT: only authorized entries are returned to userspace.

Thanks,

# BTW, why doesn't deliver the first mail of this thread I posted in SELinux-list?
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

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

Re: SELinux metadata protection

[Serge E. Hallyn]

Quoting KaiGai Kohei (kaigai@kaigai.gr.jp):
> Hmm... I'm not so familiar with ISO-15408.
> Do you know previous discussions about such behavior on filename resolving 
> or readdir ?

I can look around later in the week for past discussions (not sure
whether there have been any), but at least here is the protection
profile I'm talking about:
http://niap.nist.gov/cc-scheme/pp/PP_MLOSPP-MR_V1.22.html

> Pay attention to what 'filler' was overwritten in this patch.
> When LSM is enable, FS's readdir method (dir->f_op->readdir) always calls 
> 'security_file_filldir'
> for each directory entry instead of original 'filler' given by arguments.

Yes, I see, and I was being dense and misread it.  So forget what I said
before.  But please add a good comment above the security_file_filldir
definition.  Also, the kernel community doesn't like typedefs, so please
change

+typedef struct {
+       struct file *dir;
+       void *buffer;
+       filldir_t filler;
+} security_filldir_t;

to something like

+struct security_filldir_info {
+       struct file *dir;
+       void *buffer;
+       filldir_t filler;
+};


Do you have any performance measurements, say with selinux and with only
capabilities compiled in, with and without this patch?  I suspect that will
be one of the most important points of contention if this goes to the
linux-kernel list.

thanks,
-serge

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

Re: SELinux metadata protection

[KaiGai Kohei]

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

Hi,

>>Pay attention to what 'filler' was overwritten in this patch.
>>When LSM is enable, FS's readdir method (dir->f_op->readdir) always calls 
>>'security_file_filldir'
>>for each directory entry instead of original 'filler' given by arguments.
> 
> 
> Yes, I see, and I was being dense and misread it.  So forget what I said
> before.  But please add a good comment above the security_file_filldir
> definition.  Also, the kernel community doesn't like typedefs, so please
> change

OK, I'll modify the definition of 'security_filldir_t'.

> Do you have any performance measurements, say with selinux and with only
> capabilities compiled in, with and without this patch?  I suspect that will
> be one of the most important points of contention if this goes to the
> linux-kernel list.

I collected three performance measurements which includes kernel-build and
two most extreme workload.

o Environment (My desktop PC)
   CPU: Pentium4 2.4GHz (No-HT)
   Mem: 1024MB
   Kernel: 2.6.14.5-selinux / 2.6.14.5-selinux.mp
           (stock 2.6.14.5 + NSA's patch ( + metadata protection patch))
   GLIBC: glibc-2.3.5-10.3

o benchmark.1 - simple iteration of readdir()
     by 'time -p for x in `seq 1 100`; do ls -R linux-2.6.14 > /dev/null; done'
   The followings are avarage and standard deviation of 20 times trial.
   * 2.6.14.5-selinux (without metadata protection)
     real : 9.542 [sec] (std = 0.036)
     user : 6.238 [sec] (std = 0.125)
     sys  : 3.253 [sec] (std = 0.124)

   * 2.6.14.5-selinux.mp (with metadata protection)
     real : 11.920 [sec] (std = 0.015)
     user :  6.293 [sec] (std = 0.111)
     sys  :  5.569 [sec] (std = 0.114)

o benchmark.2 - iteration of resolving filename by attached short program.
   './a.out /lib/modules/2.6.14.5-selinux/build/security/selinux/hooks.c'
   The followings are average and standard diviation of 20 times trial.
   * 2.6.14.5-selinux (without metadata protection)
     Avg: 6.931 [sec] (std = 0.020)
   * 2.6.14.5-selinux.mp (with metadata protection)
     Avg: 8.528 [sec] (std = 0.096)

o benchmark.3 - kernel-build by following commands.
     % tar jxvf linux-2.6.14.5.tar.bz2
     % cd 2.6.14.5
     % make defconfig
     % time -p make -j 2
   * 2.6.14.5-selinux (without metadata protection)
     real: 391.14[sec] (user = 356.06[s], sys = 33.49[s])
   * 2.6.14.5-selinux.mp (with metadata protection)
     real: 406.09[sec] (user = 366.47[s], sys = 36.79[s])

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

[-- Attachment #2: lookup.c --]
[-- Type: text/x-csrc, Size: 665 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/time.h>

#define NLOOP	1000000
int main(int argc, char *argv[]) {
	struct timeval start, end;
	double interval;
	int i;

	if (argc != 2) {
		fprintf(stderr, "usage: %s <filename>\n", argv[0]);
		return 1;
	}

	/* iteration of stat() */
	gettimeofday(&start, NULL);
	for (i=0; i < NLOOP; i++) {
		struct stat st_buf;
		stat(argv[1], &st_buf);
	}
	gettimeofday(&end, NULL);
	interval = (double)(1000000 * (end.tv_sec - start.tv_sec) + (end.tv_usec - start.tv_usec));

	printf("%d times stat() : %f [s]\n", NLOOP, interval/1000000.0);

	return 0;
}

Re: SELinux metadata protection

[Serge E. Hallyn]

Quoting KaiGai Kohei (kaigai@kaigai.gr.jp):
> Hello,
> 
> Nowaday, I'm considering about a philosophical theme.
> 
> In my understanding, file-metadata includes _filename_ similar to filesize,
> update-timestamp, and so on. Most of permissions for reading metadata are
> defined as 'getattr' in SELinux.
> But permission for reading filename is defined as 'read' of parent directory,
> 'getattr' of each child-entries are not evaluated.
> 
> It seems a bit curious behavior for me. Why can an unauthorized process
> be allowed to know whether the file exists or not ?
> I think it's worthwhile to conceal the existence of files from unauthorized
> processes.
> 
> This patch enables to conceal the unauthorized files.
> [1/2] LSM_metadata_protection_v1.patch
>  - add security_file_readdir(), it's called when readdir() write each filename
>    to userspace.
>  - add security_inode_lookup(), it's called when filename is resolved by path_walk().
> 
> [2/2] SELinux_metadata_protection_v1.patch
>  - add checking 'getattr' permission on file_readdirand inode_lookup hooks.
> 
> Thanks, any comments please, and have a good year :)

Hmm, a question on behavior.

Let's say hallyn_t is allowed to write /var (var_t), but not to do
getattr on /var/secret_process_is_running (secret_t).  If hallyn_t
does ls /var/secret_process_is_running, he gets -ENOENT, but what
should he get if he does 'touch /var/secret_process_is_running'?
-EPERM obviously leaks information...

-serge

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

Re: SELinux metadata protection

[KaiGai Kohei]

Hi,

> Hmm, a question on behavior.
> 
> Let's say hallyn_t is allowed to write /var (var_t), but not to do
> getattr on /var/secret_process_is_running (secret_t).  If hallyn_t
> does ls /var/secret_process_is_running, he gets -ENOENT, but what
> should he get if he does 'touch /var/secret_process_is_running'?
> -EPERM obviously leaks information...

In my implementation, -ENOENT can be returned if he tries to create
new file with same name as existing unauthorized files.
Becase resolving filename is done before DAC permission checking,
and security_inode_lookup() is called inside path_walk().
(security_inode_permission() is called _after_ DAC permission checking.)

But I'm still thinking what is appropriate error code...

# I don't know whether it's true or not. I've heard that commercial Trusted OS
# creates a new file in separated namespace on such a situation.
# But it's obviously out of the scope of SELinux, I think.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

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

Re: SELinux metadata protection

[Serge E. Hallyn]

Quoting Crispin Cowan (crispin@novell.com):
> Serge E. Hallyn wrote:
> > Hmm, a question on behavior.
> >
> > Let's say hallyn_t is allowed to write /var (var_t), but not to do
> > getattr on /var/secret_process_is_running (secret_t).  If hallyn_t
> > does ls /var/secret_process_is_running, he gets -ENOENT, but what
> > should he get if he does 'touch /var/secret_process_is_running'?
> > -EPERM obviously leaks information...
> >   
> If you create a file whose existence is supposed to be secret, and you
> put it in a public directory, then you have made a mistake, because the
> file's existence is always detectable.
> 
> Suppose we create file in /var/nothingtoseehere. The adversary can
> create any file they want, *except* for /var/nothingtoseehere, which
> will return some kind of error message (doesn't matter which error).
> From this, the attacker can infer the existence of the file, and its name.

Right.  Of course we could solve this with a custom polyinstantiated
filesystem, which might even be pretty simple to do as either a
stackable fs around a real one, or a userspace filesystem.  It would
simply create a new entry for /var/nothingtoseehere with the creator's
context, so that when a process in that context did ls /var, it would
see the new file.  I was wondering whether there are any other tricks
we could play.

Sounds like no  :)

And as you point out, if we don't solve this, there's no point returning
-ENOENT on ls /var.

thanks,
-serge

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

Re: SELinux metadata protection

[schaufler-ca.com - Casey Schaufler]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

------------------------
Casey Schaufler
casey@schaufler-ca.com
650.906.1780




--- KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:

> Date: Sun, 01 Jan 2006 03:11:47 +0900
> From: KaiGai Kohei <kaigai@kaigai.gr.jp>
> To: linux-security-module@wirex.com, "SELinux(NSA)"
> <selinux@tycho.nsa.gov>
> Subject: SELinux metadata protection
>
> Hello
>
>
> Nowaday, I´m considering about a philosophical
> theme.
>
> In my understanding, file-metadata includes
> _filename_ similar to filesize
> update-timestamp, and so on.

Casey takes a deep breath...

The filename is not an attribute of the file.
The pathname components are data contained
in directory entries. The association of path name
to inode number is one way. There is no association
of path name from file. Really. This is the thing
that make audit hard.

Yes, I know "It's obvious". It's just not true.





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

Re: SELinux metadata protection

[Stephen Smalley]

On Mon, 2006-01-02 at 14:06 -0500, schaufler-ca.com - Casey Schaufler
wrote:
> Casey takes a deep breath...
> 
> The filename is not an attribute of the file.
> The pathname components are data contained
> in directory entries. The association of path name
> to inode number is one way. There is no association
> of path name from file. Really. This is the thing
> that make audit hard.
> 
> Yes, I know "It's obvious". It's just not true.

The world is ending because I agree with Casey on this one...
The filename is not an attribute of the file, and we do not want this
type of filtering on directory reads.  Use the permissions on the
directory itself to control who can see the names it contains.  It is
the data container for the filenames.

Use polyinstantiation aka Multi-Level Directories aka moldy directories
for shared directories like /tmp.

-- 
Stephen Smalley
National Security Agency


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

Re: SELinux metadata protection

[KaiGai Kohei]

Thanks for your comments.

OK, I understood positioning of filename in SELinux.
I wanted to confirm whether it was metadata or not at first
because it seems to me a bit unclarity.

Drop previous two patches.

>>Casey takes a deep breath...
>>
>>The filename is not an attribute of the file.
>>The pathname components are data contained
>>in directory entries. The association of path name
>>to inode number is one way. There is no association
>>of path name from file. Really. This is the thing
>>that make audit hard.
>>
>>Yes, I know "It's obvious". It's just not true.
> 
> 
> The world is ending because I agree with Casey on this one...
> The filename is not an attribute of the file, and we do not want this
> type of filtering on directory reads.  Use the permissions on the
> directory itself to control who can see the names it contains.  It is
> the data container for the filenames.
> 
> Use polyinstantiation aka Multi-Level Directories aka moldy directories
> for shared directories like /tmp.

-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

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

Re: SELinux metadata protection

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@tycho.nsa.gov):
> On Mon, 2006-01-02 at 14:06 -0500, schaufler-ca.com - Casey Schaufler
> wrote:
> > Casey takes a deep breath...
> > 
> > The filename is not an attribute of the file.
> > The pathname components are data contained
> > in directory entries. The association of path name
> > to inode number is one way. There is no association
> > of path name from file. Really. This is the thing
> > that make audit hard.
> > 
> > Yes, I know "It's obvious". It's just not true.
> 
> The world is ending because I agree with Casey on this one...
> The filename is not an attribute of the file, and we do not want this
> type of filtering on directory reads.  Use the permissions on the
> directory itself to control who can see the names it contains.  It is
> the data container for the filenames.
> 
> Use polyinstantiation aka Multi-Level Directories aka moldy directories
> for shared directories like /tmp.

Looking at the second "application note" for 5.3.4.1 of PP_MLOSPP_MR
(from http://niap.nist.gov/cc-scheme/pp/PP_MLOSPP-MR_V1.22.html), it
says:

"The MAC policy covers all subjects and all objects.  The list of
objects must include object attributes that are themselves objects
(such as filenames) because they can be manipulated by a user."

So it sounds like to meet this profile, we'd have to either have
separate controls on the filename, or extend a file's read/write
access rights to any dentry pointing to the inode.

Of course I'd resigned myself long ago to not being able to meet this
application note :)  Particularly due to the aforementioned hard-to-
handle information leak on touch(/var/somefilename).  So I'm not
arguing with the decision to abort this patch.

-serge

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

Re: SELinux metadata protection

[Stephen Smalley]

On Thu, 2006-01-05 at 08:56 -0600, Serge E. Hallyn wrote:
> Looking at the second "application note" for 5.3.4.1 of PP_MLOSPP_MR
> (from http://niap.nist.gov/cc-scheme/pp/PP_MLOSPP-MR_V1.22.html), it
> says:
> 
> "The MAC policy covers all subjects and all objects.  The list of
> objects must include object attributes that are themselves objects
> (such as filenames) because they can be manipulated by a user."
> 
> So it sounds like to meet this profile, we'd have to either have
> separate controls on the filename, or extend a file's read/write
> access rights to any dentry pointing to the inode.

I don't think so.  You would just argue that filenames are not separable
objects in Unix/Linux, that they are part of the content of directory
objects in Unix/Linux, and that MAC policy does control the ability to
read directories based on their label.  Further, MAC policy does already
control the ability to modify directories not only based on checks on
the directory label but also based on per-file checks (e.g. SELinux
checks rename, unlink, and link permissions on the inode in addition to
add_name/remove_name on the directory).  The unlink/link checks are
justified since they alter inode state (link count).  The rename check
is more of an integrity-oriented control.
 
> Of course I'd resigned myself long ago to not being able to meet this
> application note :)  Particularly due to the aforementioned hard-to-
> handle information leak on touch(/var/somefilename).  So I'm not
> arguing with the decision to abort this patch.

-- 
Stephen Smalley
National Security Agency


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

Re: SELinux metadata protection

[Joe Nall]

On Jan 5, 2006, at 9:10 AM, Stephen Smalley wrote:

> On Thu, 2006-01-05 at 08:56 -0600, Serge E. Hallyn wrote:
>> Looking at the second "application note" for 5.3.4.1 of PP_MLOSPP_MR
>> (from http://niap.nist.gov/cc-scheme/pp/PP_MLOSPP-MR_V1.22.html), it
>> says:
>>
>> "The MAC policy covers all subjects and all objects.  The list of
>> objects must include object attributes that are themselves objects
>> (such as filenames) because they can be manipulated by a user."

In the paper world, classified documents routinely have unclassified
(or less classified) titles and metadata. In the trusted OS I use
regularly (HP CMW), a classified file can only be written to a lower
level directory by way of elevated privilege because the process
is 'writing down' to the directory and declaring the metadata to be
classified at the directory level. (C)overt channels are dealt with
during the evaluation of the use of privilege by the application.

joe

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