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

[Steve French]

The access denied message in the 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 18, 2008 10:55 PM, Andi Kleen <andi at firstfloor.org> 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 at 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
>



-- 
Thanks,

Steve
-------------- next part --------------
HTML attachment scrubbed and removed