From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marian Marinov Subject: Re: ioctl CAP_LINUX_IMMUTABLE is checked in the wrong namespace Date: Wed, 30 Apr 2014 00:49:15 +0300 Message-ID: <53601E5B.5050004@1h.com> References: <535FADDA.2070803@1h.com> <20140429183534.GB19325@thunk.org> <20140429185251.GA27969@ubuntumail> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140429185251.GA27969@ubuntumail> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Serge Hallyn , Theodore Ts'o , containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, LXC development mailing-list , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: containers.vger.kernel.org On 04/29/2014 09:52 PM, Serge Hallyn wrote: > Quoting Theodore Ts'o (tytso-3s7WtUTddSA@public.gmane.org): >> On Tue, Apr 29, 2014 at 04:49:14PM +0300, Marian Marinov wrote: >>> >>> I'm proposing a fix to this, by replacing the capable(CAP_LINUX_IMMUTABLE) >>> check with ns_capable(current_cred()->user_ns, CAP_LINUX_IMMUTABLE). >> >> Um, wouldn't it be better to simply fix the capable() function? >> >> /** >> * capable - Determine if the current task has a superior capability in effect >> * @cap: The capability to be tested for >> * >> * Return true if the current task has the given superior capability currently >> * available for use, false if not. >> * >> * This sets PF_SUPERPRIV on the task if the capability is available on the >> * assumption that it's about to be used. >> */ >> bool capable(int cap) >> { >> return ns_capable(&init_user_ns, cap); >> } >> EXPORT_SYMBOL(capable); >> >> The documentation states that it is for "the current task", and I >> can't imagine any use case, where user namespaces are in effect, where >> using init_user_ns would ever make sense. > > the init_user_ns represents the user_ns owning the object, not the > subject. > > The patch by Marian is wrong. Anyone can do 'clone(CLONE_NEWUSER)', > setuid(0), execve, and end up satisfying 'ns_capable(current_cred()->userns, > CAP_SYS_IMMUTABLE)' by definition. > > So NACK to that particular patch. I'm not sure, but IIUC it should be > safe to check against the userns owning the inode? > So what you are proposing is to replace 'ns_capable(current_cred()->userns, CAP_SYS_IMMUTABLE)' with 'inode_capable(inode, CAP_SYS_IMMUTABLE)' ? I agree that this is more sane. Marian >> No? Otherwise, pretty much every single use of capable() would be >> broken, not just this once instances in ext4/ioctl.c. >> >> - Ted >> _______________________________________________ >> Containers mailing list >> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org >> https://lists.linuxfoundation.org/mailman/listinfo/containers > -- Marian Marinov Founder & CEO of 1H Ltd. Jabber/GTalk: hackman-/eSpBmjxGS4dnm+yROfE0A@public.gmane.org ICQ: 7556201 Mobile: +359 886 660 270