Remove information leak in Linux CIFS client

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 <[email protected]>

Index: linux/fs/cifs/dir.c
===================================================================

Quoted 20 lines · 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 <[email protected]>

Index: linux/fs/cifs/dir.c
===================================================================

fs/cifs/dir.c cifs_lookup(struct inode *parent_dir_ino · line 522

Quoted 6 lines

fs/cifs/dir.c +1 -1

@@ -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 <[email protected]> Senior Software Engineer at Red Hat Inc. <[email protected]>

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:

Quoted 2 lines · 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

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.

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.

Quoted 6 lines · Andi Kleen

Quoted 2 lines · 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

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.

Quoted 8 lines · Steve French

Quoted 6 lines · Andi Kleen

Quoted 2 lines · 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

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.

Quoted 7 lines

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.

Quoted 3 lines

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