lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 19 Jan 2008 16:06:57 -0600 From: "Steve French" <smfrench@...il.com> To: simo <idra@...ba.org> Cc: "Andi Kleen" <andi@...stfloor.org>, linux-kernel@...r.kernel.org, linux-cifs-client@...ts.samba.org, samba-technical@...ts.samba.org Subject: Re: [linux-cifs-client] [PATCH] Remove information leak in Linux CIFS client 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@...ba.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@...e.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@...ba.org> > Senior Software Engineer at Red Hat Inc. <ssorce@...hat.com> > > -- Thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists