headers
David Chinner | 2 Aug 14:21
Picon
Favicon

Re: [xfs-masters] [RFC: 2.6 patch] make the *FS_SECURITY options no longer user visible

On Mon, Jul 30, 2007 at 08:27:47AM -0400, Stephen Smalley wrote:
> On Mon, 2007-07-30 at 09:29 +1000, David Chinner wrote:
> > On Sun, Jul 29, 2007 at 05:02:09PM +0200, Adrian Bunk wrote:
> > > Please correct me if any of the following assumptions is wrong:
> > > - SELinux is currently the only user of filesystem security labels
> > >   shipped with the Linux kernel
> > > - if a user has SELinux enabled he wants his filesystems to support
> > >   security labels
> > > 
> > > Based on these assumption, it doesn't make sense to have the
> > > *FS_SECURITY user visible since we can perfectly determine automatically 
> > > when turning them on makes sense.
> > 
> > Hmmm. The code in XFS is not dependent on selinux, but this change
> > would mean that testing the security xattr namespace would require a
> > selinux enabled kernel.
> > 
> > I agree that the default for these should be "y" and selected if
> > selinux is enabled, but forcing us to use selinux enabled kernels
> > (on distro's that may not support selinux) just to test the
> > security xattr namespace is a bit of a pain.
> 
> You can enable SECURITY_SELINUX in the kernel config but still have it
> boot disabled by default via SECURITY_SELINUX_BOOTPARAM_VALUE=0.

Ok, that shouldn't cause a problem then. Objection withdrawn. ;)

Cheers,

Dave.
(Continue reading)

Permalink | Reply | 0 comments |
headers
Jeff Layton | 6 Aug 15:54
Picon
Favicon

[PATCH 00/25] move handling of setuid/gid bits from VFS into individual setattr functions (RESEND)

Apologies for the resend, but the original sending had the date in the
email header and it caused some of these to bounce...

( Please consider trimming the Cc list if discussing some aspect of this
that doesn't concern everyone.)

When an unprivileged process attempts to modify a file that has the
setuid or setgid bits set, the VFS will attempt to clear these bits. The
VFS will set the ATTR_KILL_SUID or ATTR_KILL_SGID bits in the ia_valid
mask, and then call notify_change to clear these bits and set the mode
accordingly.

With a networked filesystem (NFS in particular but most likely others),
the client machine may not have credentials that allow for the clearing
of these bits. In some situations, this can lead to file corruption, or
to an operation failing outright because the setattr fails.

In this situation, we'd like to just leave the handling of this to
the server and ignore these bits. The problem is that by the time
nfs_setattr is called, the VFS has already reinterpreted the ATTR_KILL_*
bits into a mode change. We can't fix this in the filesystems where
this is a problem, as doing so would leave us having to second-guess
what the VFS wants us to do. So we need to change it so that filesystems
have more flexibility in how to interpret the ATTR_KILL_* bits.

The first patch in the following patchset moves this logic into a helper
function, and then only calls this helper function for inodes that do
not have a setattr operation defined. The subsequent patches fix up
individual filesystem setattr functions to call this helper function.
(Continue reading)

Permalink | Reply | 6 comments |
headers
Jeff Layton | 6 Aug 15:54
Picon
Favicon

[PATCH 01/25] VFS: move attr_kill logic from notify_change into helper function

Separate the handling of the local ia_valid bitmask from the one in
attr->ia_valid. This allows us to hand off the actual handling of the
ATTR_KILL_* flags to the .setattr i_op when one is defined.

notify_change still needs to process those flags for the local ia_valid
variable, since it uses that to decide whether to return early, and to pass
a (hopefully) appropriate bitmask to fsnotify_change.

Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 fs/attr.c          |   54 +++++++++++++++++++++++++++++++++------------------
 include/linux/fs.h |    1 +
 2 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index f8dfc22..47015e0 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -100,15 +100,39 @@ int inode_setattr(struct inode * inode, struct iattr * attr)
 }
 EXPORT_SYMBOL(inode_setattr);

+void attr_kill_to_mode(struct inode *inode, struct iattr *attr)
+{
+	if (attr->ia_valid & ATTR_KILL_SUID) {
+		attr->ia_valid &= ~ATTR_KILL_SUID;
+		if (inode->i_mode & S_ISUID) {
+			if (!(attr->ia_valid & ATTR_MODE)) {
+				attr->ia_valid |= ATTR_MODE;
+				attr->ia_mode = inode->i_mode;
(Continue reading)

Permalink | Reply | 4 comments |
headers
Jeff Layton | 6 Aug 15:54
Picon
Favicon

[PATCH 14/25] JFFS2: call attr_kill_to_mode from jffs2_do_setattr


Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 fs/jffs2/fs.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 1d3b7a9..5218f04 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -37,6 +37,7 @@ static int jffs2_do_setattr (struct inode *inode, struct iattr *iattr)
 	uint32_t alloclen;
 	int ret;
 	D1(printk(KERN_DEBUG "jffs2_setattr(): ino #%lu\n", inode->i_ino));
+	attr_kill_to_mode(inode, iattr);
 	ret = inode_change_ok(inode, iattr);
 	if (ret)
 		return ret;
--

-- 
1.5.2.2

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |
headers
Miklos Szeredi | 6 Aug 19:43
Picon

Re: [PATCH 01/25] VFS: move attr_kill logic from notify_change into helper function

> Separate the handling of the local ia_valid bitmask from the one in
> attr->ia_valid. This allows us to hand off the actual handling of the
> ATTR_KILL_* flags to the .setattr i_op when one is defined.
> 
> notify_change still needs to process those flags for the local ia_valid
> variable, since it uses that to decide whether to return early, and to pass
> a (hopefully) appropriate bitmask to fsnotify_change.

I agree with this change and fuse will make use of it as well.

Maybe instead of unconditionally moving attr_kill_to_mode() inside
->setattr() it could be made conditional based on an inode flag
similarly to S_NOCMTIME.  Advantages:

 - no need to modify a lot of in-tree filesystems
 - no silent breakage of out-of-tree fs

Actually I think the new flag would be used by exacly the same
filesystems as S_NOCMTIME, so maybe it would make sense to rename
S_NOCMTIME to something more generic (S_NOATTRUPDATE or whatever) and
use that.

But that could still break out-of-tree fs, so a separate flag is
probably better.

Miklos

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
(Continue reading)

Permalink | Reply | 2 comments |
headers
Jeff Layton | 6 Aug 20:13
Picon
Favicon

Re: [fuse-devel] [PATCH 01/25] VFS: move attr_kill logic from notify_change into helper function

On Mon, 06 Aug 2007 19:43:46 +0200
Miklos Szeredi <miklos <at> szeredi.hu> wrote:

> > Separate the handling of the local ia_valid bitmask from the one in
> > attr->ia_valid. This allows us to hand off the actual handling of the
> > ATTR_KILL_* flags to the .setattr i_op when one is defined.
> > 
> > notify_change still needs to process those flags for the local ia_valid
> > variable, since it uses that to decide whether to return early, and to pass
> > a (hopefully) appropriate bitmask to fsnotify_change.
> 
> I agree with this change and fuse will make use of it as well.
> 
> Maybe instead of unconditionally moving attr_kill_to_mode() inside
> ->setattr() it could be made conditional based on an inode flag
> similarly to S_NOCMTIME.  Advantages:
> 
>  - no need to modify a lot of in-tree filesystems
>  - no silent breakage of out-of-tree fs
> 
> Actually I think the new flag would be used by exacly the same
> filesystems as S_NOCMTIME, so maybe it would make sense to rename
> S_NOCMTIME to something more generic (S_NOATTRUPDATE or whatever) and
> use that.
> 
> But that could still break out-of-tree fs, so a separate flag is
> probably better.
> 

In the past I've been told that adding new flags is something of a
(Continue reading)

Permalink | Reply | 2 comments |
headers
Miklos Szeredi | 6 Aug 20:28
Picon

Re: [PATCH 01/25] VFS: move attr_kill logic from notify_change into helper function

> > I agree with this change and fuse will make use of it as well.
> > 
> > Maybe instead of unconditionally moving attr_kill_to_mode() inside
> > ->setattr() it could be made conditional based on an inode flag
> > similarly to S_NOCMTIME.  Advantages:
> > 
> >  - no need to modify a lot of in-tree filesystems
> >  - no silent breakage of out-of-tree fs
> > 
> > Actually I think the new flag would be used by exacly the same
> > filesystems as S_NOCMTIME, so maybe it would make sense to rename
> > S_NOCMTIME to something more generic (S_NOATTRUPDATE or whatever) and
> > use that.
> > 
> > But that could still break out-of-tree fs, so a separate flag is
> > probably better.
> > 
> 
> In the past I've been told that adding new flags is something of a
> "last resort". Since it's not strictly necessary to fix this then
> it may be best to avoid that.
> 
> That said, if the concensus is that we need a transition mechanism,
> then I'd be open to such a suggestion.

I think there's really no other choice here.

Your patch is changing the API in a very unsafe way, since there will
be no error or warning on an unconverted fs.  And that could lead to
security holes.
(Continue reading)

Permalink | Reply | 2 comments |
headers
Trond Myklebust | 6 Aug 21:04
Picon
Picon

Re: [fuse-devel] [PATCH 01/25] VFS: move attr_kill logic from notify_change into helper function

On Mon, 2007-08-06 at 20:28 +0200, Miklos Szeredi wrote:

> Your patch is changing the API in a very unsafe way, since there will
> be no error or warning on an unconverted fs.  And that could lead to
> security holes.
> 
> If we would rename the setattr method to setattr_new as well as
> changing it's behavior, that would be fine.  But I guess we do not
> want to do that.

Which "unconverted fses"? If we're talking out of tree stuff, then too
bad: it is _their_ responsibility to keep up with kernel changes.

Trond

-
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 2 comments |
headers
Miklos Szeredi | 6 Aug 21:37
Picon

Re: [fuse-devel] [PATCH 01/25] VFS: move attr_kill logic from notify_change into helper function

> > Your patch is changing the API in a very unsafe way, since there will
> > be no error or warning on an unconverted fs.  And that could lead to
> > security holes.
> > 
> > If we would rename the setattr method to setattr_new as well as
> > changing it's behavior, that would be fine.  But I guess we do not
> > want to do that.
> 
> Which "unconverted fses"? If we're talking out of tree stuff, then too
> bad: it is _their_ responsibility to keep up with kernel changes.

It is usually a good idea to not change the semantics of an API in a
backward incompatible way without changing the syntax as well.

This is true regardless of whether we care about out-of-tree code or
not (and we should care to some degree).  And especially true if the
change in question is security sensitive.

Miklos
Permalink | Reply | 1 comment |
headers
Trond Myklebust | 6 Aug 23:23
Picon
Picon

Re: [fuse-devel] [PATCH 01/25] VFS: move attr_kill logic from notify_change into helper function

On Mon, 2007-08-06 at 21:37 +0200, Miklos Szeredi wrote:
> > > Your patch is changing the API in a very unsafe way, since there will
> > > be no error or warning on an unconverted fs.  And that could lead to
> > > security holes.
> > > 
> > > If we would rename the setattr method to setattr_new as well as
> > > changing it's behavior, that would be fine.  But I guess we do not
> > > want to do that.
> > 
> > Which "unconverted fses"? If we're talking out of tree stuff, then too
> > bad: it is _their_ responsibility to keep up with kernel changes.
> 
> It is usually a good idea to not change the semantics of an API in a
> backward incompatible way without changing the syntax as well.

We're taking two setattr flags ATTR_KILL_SGID, and ATTR_KILL_SUID which
have existed for several years in the VFS, and making them visible to
the filesystems. Out-of-tree filesystems that care can check for them in
a completely backward compatible way: you don't even need to add a
#define.

> This is true regardless of whether we care about out-of-tree code or
> not (and we should care to some degree).  And especially true if the
> change in question is security sensitive.

It is not true "regardless": the in-tree code is being converted.
Out-of-tree code is the only "problem" here, and their only problem is
that they may have to add support for the new flags if they also support
suid/sgid mode bits.
(Continue reading)

Permalink | Reply | 1 comment |

Gmane