From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kees Cook Subject: Re: [PATCH v2] fs: block cross-uid sticky symlinks Date: Sun, 30 May 2010 21:12:04 -0700 Message-ID: <20100531041204.GR6056@outflux.net> References: <20100531030402.GQ6056@outflux.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-doc@vger.kernel.org, Randy Dunlap , Andrew Morton , Jiri Kosina , Dave Young , Martin Schwidefsky , James Morris , Eric Paris , David Howells , Ingo Molnar , Peter Zijlstra , Tim Gardner , "Serge E. Hallyn" To: "Eric W. Biederman" Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-doc-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org Hi Eric, On Sun, May 30, 2010 at 08:50:53PM -0700, Eric W. Biederman wrote: > The name of the sysctl is horrible it is a double negative, which > makes thinking about it hard. Hmm, I see your point, the "safe" value is "weak: not". I was trying to be descriptive without needing a "true" value, but I guess that's silly; we already have things like randomize_va_space set to "2" by default, etc. What would you suggest instead? "protected_sticky_symlinks" (and reverse the default and test logic)? > Why not simply put each user in a different mount namespace and have separate > /tmp directories per user? That works today, with no kernel changes. The key here is "no kernel changes" -- trying to isolate every user and service from each other using different mount namespaces will not work quickly in current distributions. Even doing bind-mount tricks to keep /tmp away from different users is overkill, especially when you have situations like "screen" using a common /tmp directory tree (in the setuid version), etc. Things (correctly) expect to share /tmp in some cases. However, this one kernel change will allow everything to continue without userspace overhead and without breaking anything terribly. Using containers will probably be the future, but I want to solve this in the general case today. > Placing this in cap_inode_follow_link is horrible naming. There is nothing > capabilities about this. Either this needs to go into one or several > of the security modules or this needs to go into the core vfs. My thinking was that most of the LSMs call down to commoncaps first, so it's a single place to put this. When I was looking at this code originally, I thought that if it doesn't go in security_inode_follow_link, then a new function would be added to the VFS and both callers of security_inode_follow_link would need to call it just before security_inode_follow_link. It seemed like putting it in there reduced duplication of logic. However, on closer examination, it seems that this code could live in __do_follow_link instead. fs/namei.c: ... error = security_inode_follow_link(path.dentry, &nd); if (error) goto exit_dput; error = __do_follow_link(&path, &nd, &cookie); ... err = security_inode_follow_link(path->dentry, nd); if (err) goto loop; current->link_count++; current->total_link_count++; nd->depth++; err = __do_follow_link(path, nd, &cookie); ... What would you suggest for the best approach here? > I can't argue with taking action to close the too frequently security > issues in /tmp, but this changes appears to be unnecessary, difficult > to maintain, and difficult to understand. Well, we disagree about "unnecessary". :) Finding an easy to maintain solution is my goal here, and if it's difficult to understand, then I need to fix that too. What could use better clarification? Thanks! -Kees -- Kees Cook Ubuntu Security Team