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

["Steve French" <smfrench@gmail.com>]

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