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

--

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>
--

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);
--

Thanks,
Steve

--

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 
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

--

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
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

--

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.
Sure errors should be logged, but not with path names. 
-Andi

--

Just merged into the cifs-2.6 tree, changing the last patch as you

just suggested to take out the logged path name.
--

Thanks,
Steve

--