Re: ioctl CAP_LINUX_IMMUTABLE is checked in the wrong namespace
From: Marian Marinov
Date: Tue Apr 29 2014 - 18:25:00 EST
On 04/30/2014 01:02 AM, Serge Hallyn wrote:
Quoting Marian Marinov (mm@xxxxxx):
On 04/29/2014 09:52 PM, Serge Hallyn wrote:
Quoting Theodore Ts'o (tytso@xxxxxxx):
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.
Right, and I think the two operations you're looking at seem sane
to allow.
If you are ok with this patch, I will fix all file systems and send patches.
Signed-off-by: Marian Marinov <mm@xxxxxxxx>
---
fs/ext4/ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index d011b69..9418634 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -265,7 +265,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
* This test looks nicer. Thanks to Pauline Middelink
*/
if ((flags ^ oldflags) & (EXT4_APPEND_FL | EXT4_IMMUTABLE_FL)) {
- if (!capable(CAP_LINUX_IMMUTABLE))
+ if (!inode_capable(inode, CAP_LINUX_IMMUTABLE))
goto flags_out;
}
---
1.8.4
Marian
thanks,
-serge
--
Marian Marinov
Founder & CEO of 1H Ltd.
Jabber/GTalk: hackman@xxxxxxxxxx
ICQ: 7556201
Mobile: +359 886 660 270
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/