[PATCH] Remove information leak in Linux CIFS client

[PATCH] Remove information leak in Linux CIFS client

[Andi Kleen]

Fix information leak in CIFS client lookup

Putting arbitary file names on lookup failures into the system log is not
a good idea, because usually everybody can read dmesg and that is thus
an information leak if a directory was read protected.

Also changed the error printout for this case to a signed number, because
it is normally negative and that makes it easier to read.

I'm not sure the message is all that useful anyways. Perhaps it 
should be just removed completely? Or at least rate limited because
it allows to spam the kernel log nicely.

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/fs/cifs/dir.c
===================================================================
--- linux.orig/fs/cifs/dir.c
+++ linux/fs/cifs/dir.c
@@ -518,7 +518,7 @@ cifs_lookup(struct inode *parent_dir_ino
 	/*	if it was once a directory (but how can we tell?) we could do
 		shrink_dcache_parent(direntry); */
 	} else {
-		cERROR(1, ("Error 0x%x on cifs_get_inode_info in lookup of %s",
+		cERROR(1, ("Error %d on cifs_get_inode_info in lookup of file",
 			   rc, full_path));
 		/* BB special case check for Access Denied - watch security
 		exposure of returning dir info implicitly via different rc

Re: [linux-cifs-client] [PATCH] Remove information leak in Linux CIFS client

[simo]

On Sat, 2008-01-19 at 05:55 +0100, Andi Kleen wrote:
> Fix information leak in CIFS client lookup
> 
> Putting arbitary file names on lookup failures into the system log is not
> a good idea, because usually everybody can read dmesg and that is thus
> an information leak if a directory was read protected.
> 
> Also changed the error printout for this case to a signed number, because
> it is normally negative and that makes it easier to read.
> 
> I'm not sure the message is all that useful anyways. Perhaps it 
> should be just removed completely? Or at least rate limited because
> it allows to spam the kernel log nicely.
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> 
> Index: linux/fs/cifs/dir.c
> ===================================================================
> --- linux.orig/fs/cifs/dir.c
> +++ linux/fs/cifs/dir.c
> @@ -518,7 +518,7 @@ cifs_lookup(struct inode *parent_dir_ino
>  	/*	if it was once a directory (but how can we tell?) we could do
>  		shrink_dcache_parent(direntry); */
>  	} else {
> -		cERROR(1, ("Error 0x%x on cifs_get_inode_info in lookup of %s",
> +		cERROR(1, ("Error %d on cifs_get_inode_info in lookup of file",
>  			   rc, full_path));

then please remove also full_path here ^^^^

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo@samba.org>
Senior Software Engineer at Red Hat Inc. <ssorce@redhat.com>

Re: [linux-cifs-client] [PATCH] Remove information leak in Linux CIFS client

[Steve French]

The access denied message in the dmesg log reveals no more information
than strace on stat of a local file does (which also returns access
denied and displays access denied), but I agree that logging on
-EACCESS on lookup does clutter the log.

I think it is ok to log a message on unexpected errors (for
QueryPathInfo those would include anything other than ENOENT and
EACCESS - Simo, can you think of others?)  I don't mind ratelimiting
logging on this clause (for errors other than ENOENT and EACCESS) but
it would complicate the code for a case I have not seen in the wild.

I prefer the following to remove the log cluttering on this case:

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 37dc97a..b2802e5 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -517,12 +517,11 @@ cifs_lookup(struct inode *parent_dir_inode,
struct dentry *direntry,
                d_add(direntry, NULL);
        /*      if it was once a directory (but how can we tell?) we could do

                shrink_dcache_parent(direntry); */
-       } else {
+       } else if (rc != -EACCES) {

                cERROR(1, ("Error 0x%x on cifs_get_inode_info in lookup of %s",
                           rc, full_path));
-               /* BB special case check for Access Denied - watch security
-               exposure of returning dir info implicitly via different rc
-               if file exists or not but no access BB */
+               /* We special case check for Access Denied - since that
+               is a common return code */
        }

        kfree(full_path);

On Jan 19, 2008 2:18 AM, simo <idra@samba.org> wrote:
>
>
> On Sat, 2008-01-19 at 05:55 +0100, Andi Kleen wrote:
> > Fix information leak in CIFS client lookup
> >
> > Putting arbitary file names on lookup failures into the system log is not
> > a good idea, because usually everybody can read dmesg and that is thus
> > an information leak if a directory was read protected.
> >
> > Also changed the error printout for this case to a signed number, because
> > it is normally negative and that makes it easier to read.
> >
> > I'm not sure the message is all that useful anyways. Perhaps it
> > should be just removed completely? Or at least rate limited because
> > it allows to spam the kernel log nicely.
> >
> > Signed-off-by: Andi Kleen <ak@suse.de>
> >
> > Index: linux/fs/cifs/dir.c
> > ===================================================================
> > --- linux.orig/fs/cifs/dir.c
> > +++ linux/fs/cifs/dir.c
> > @@ -518,7 +518,7 @@ cifs_lookup(struct inode *parent_dir_ino
> >       /*      if it was once a directory (but how can we tell?) we could do
> >               shrink_dcache_parent(direntry); */
> >       } else {
> > -             cERROR(1, ("Error 0x%x on cifs_get_inode_info in lookup of %s",
> > +             cERROR(1, ("Error %d on cifs_get_inode_info in lookup of file",
> >                          rc, full_path));
>
> then please remove also full_path here ^^^^
>
> Simo.
>
> --
> Simo Sorce
> Samba Team GPL Compliance Officer <simo@samba.org>
> Senior Software Engineer at Red Hat Inc. <ssorce@redhat.com>
>
>



-- 
Thanks,

Steve

Re: [linux-cifs-client] [PATCH] Remove information leak in Linux CIFS clientg

[Andi Kleen]

On Sat, Jan 19, 2008 at 04:06:57PM -0600, Steve French wrote:
> The access denied message in the dmesg log reveals no more information
> than strace on stat of a local file does (which also returns access

You can't strace a process you don't own. And you might not be able
to access the directory below which the file is.

But there is no such access control on "dmesg". So if there is an
access error you leak the potentially protected information in 
the file name

> denied and displays access denied), but I agree that logging on
> -EACCESS on lookup does clutter the log.
> 
> I think it is ok to log a message on unexpected errors (for
> QueryPathInfo those would include anything other than ENOENT and
> EACCESS - Simo, can you think of others?)  I don't mind ratelimiting
> logging on this clause (for errors other than ENOENT and EACCESS) but
> it would complicate the code for a case I have not seen in the wild.
> 
> I prefer the following to remove the log cluttering on this case:

That would still be an information leak for other errors.

Logging the path would be only safe if you determine that the
file and all its parent directories were world read (and accessable)able,
but that would be probably difficult.

-Andi

Re: [linux-cifs-client] [PATCH] Remove information leak in Linux CIFS clientg

[Steve French]

On Jan 19, 2008 4:30 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Sat, Jan 19, 2008 at 04:06:57PM -0600, Steve French wrote:
> > The access denied message in the dmesg log reveals no more information
> > than strace on stat of a local file does (which also returns access
>
> You can't strace a process you don't own. And you might not be able
> to access the directory below which the file is.

If you can't access the directory that the file is in then you get
access denied on stat of the file (local over ext3 or remote over
cifs) - it does not tell you anything about whether the file existed
or not.  If you do "stat
/mnt/dir-with-0700-perm/file-which-does-not-exist"  I get access
denied.  I don't think that it really tells you anything interesting
since the same error comes back whether or not the file existed.
Other unexpected errors (e.g. -EIO) should be logged because they
indicate possibly severe problems with the network, but also don't
tell you anything about whether the file exists.

> Logging the path would be only safe if you determine that the
> file and all its parent directories were world read (and accessable)able,
> but that would be probably difficult.

If the parent of the parent were not readable/accessable then you
would not have gotten this far (the stat of the parent directory
rather than the file would have returned the error).  If the parent
were readable then the access denied on stat is the same over cifs and
ext3 - and the logging of the error only occurs in cases like EIO in
which e.g. the network has crashed (but you can't tell from the error
whether the file exists or not).  I don't mind taking out the path
name from the logging of EIO in this case but it could make debugging
harder if we ever hit a strange bug.


-- 
Thanks,

Steve

Re: [linux-cifs-client] [PATCH] Remove information leak in Linux CIFS clientg

[Andi Kleen]

On Sat, Jan 19, 2008 at 04:55:53PM -0600, Steve French wrote:
> On Jan 19, 2008 4:30 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > On Sat, Jan 19, 2008 at 04:06:57PM -0600, Steve French wrote:
> > > The access denied message in the dmesg log reveals no more information
> > > than strace on stat of a local file does (which also returns access
> >
> > You can't strace a process you don't own. And you might not be able
> > to access the directory below which the file is.
> 
> If you can't access the directory that the file is in then you get
> access denied on stat of the file (local over ext3 or remote over
> cifs) - it does not tell you anything about whether the file existed
> or not.  If you do "stat
> /mnt/dir-with-0700-perm/file-which-does-not-exist"  I get access
> denied.  I don't think that it really tells you anything interesting
> since the same error comes back whether or not the file existed.

The problem is that the file name ends up in the log for everybody to
read even if they're totally unrelated. So if someone in a protected directory
tree where they have access to does something that is denied the
file names will still leak to everybody else to the log.

e.g. more concrete example. you do something and get that message.

Now even 'nobody" running in a chroot will know that you tried
that and that at least parts of the file name likely exist.

That is an information leak and imho a privacy problem.

> Other unexpected errors (e.g. -EIO) should be logged because they
> indicate possibly severe problems with the network, but also don't
> tell you anything about whether the file exists.

Sure errors should be logged, but not with path names. 

-Andi

Re: [linux-cifs-client] [PATCH] Remove information leak in Linux CIFS clientg

[Steve French]

Just merged into the cifs-2.6 tree, changing the last patch as you
just suggested to take out the logged path name.

On Jan 19, 2008 5:25 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Sat, Jan 19, 2008 at 04:55:53PM -0600, Steve French wrote:
> > On Jan 19, 2008 4:30 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > > On Sat, Jan 19, 2008 at 04:06:57PM -0600, Steve French wrote:
> > > > The access denied message in the dmesg log reveals no more information
> > > > than strace on stat of a local file does (which also returns access
> > >
> > > You can't strace a process you don't own. And you might not be able
> > > to access the directory below which the file is.
> >
> > If you can't access the directory that the file is in then you get
> > access denied on stat of the file (local over ext3 or remote over
> > cifs) - it does not tell you anything about whether the file existed
> > or not.  If you do "stat
> > /mnt/dir-with-0700-perm/file-which-does-not-exist"  I get access
> > denied.  I don't think that it really tells you anything interesting
> > since the same error comes back whether or not the file existed.
>
> The problem is that the file name ends up in the log for everybody to
> read even if they're totally unrelated. So if someone in a protected directory
> tree where they have access to does something that is denied the
> file names will still leak to everybody else to the log.
>
> e.g. more concrete example. you do something and get that message.
>
> Now even 'nobody" running in a chroot will know that you tried
> that and that at least parts of the file name likely exist.
>
> That is an information leak and imho a privacy problem.
>
> > Other unexpected errors (e.g. -EIO) should be logged because they
> > indicate possibly severe problems with the network, but also don't
> > tell you anything about whether the file exists.
>
> Sure errors should be logged, but not with path names.
>
> -Andi
>



-- 
Thanks,

Steve