All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH #2] reiserfs: Fix permissions on .reiserfs_priv
@ 2010-04-08 17:30 ` Jeff Mahoney
  0 siblings, 0 replies; 3+ messages in thread
From: Jeff Mahoney @ 2010-04-08 17:30 UTC (permalink / raw)
  To: Edward Shishkin
  Cc: ReiserFS Mailing List, Linux Kernel Mailing List, Andrew Morton

 Commit 677c9b2e393a0cd203bd54e9c18b012b2c73305a removed the magic
 from the lookup code to hide the .reiserfs_priv directory since it
 was getting loaded at mount-time instead. The intent was that the
 entry would be hidden from the user via a poisoned d_compare, but
 this was faulty.

 This introduced a security issue where unpriviledged users could
 access and modify extended attributes or ACLs belonging to other
 users, including root.

 This patch resolves the issue by properly hiding .reiserfs_priv. This
 was the intent of the xattr poisoning code, but it appears to have
 never worked as expected. This is fixed by using d_revalidate instead
 of d_compare.

 This patch makes -oexpose_privroot a no-op. I'm fine leaving it this
 way. The effort involved in working out the corner cases wrt permissions
 and caching outweigh the benefit of the feature.

 Re-sent because I forgot to turn off the GPG signing when I sent the first
 time.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---

 fs/reiserfs/dir.c   |    2 --
 fs/reiserfs/xattr.c |   17 ++++-------------
 2 files changed, 4 insertions(+), 15 deletions(-)

--- a/fs/reiserfs/dir.c
+++ b/fs/reiserfs/dir.c
@@ -45,8 +45,6 @@ static inline bool is_privroot_deh(struc
 				   struct reiserfs_de_head *deh)
 {
 	struct dentry *privroot = REISERFS_SB(dir->d_sb)->priv_root;
-	if (reiserfs_expose_privroot(dir->d_sb))
-		return 0;
 	return (dir == dir->d_parent && privroot->d_inode &&
 	        deh->deh_objectid == INODE_PKEY(privroot->d_inode)->k_objectid);
 }
--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -972,21 +972,13 @@ int reiserfs_permission(struct inode *in
 	return generic_permission(inode, mask, NULL);
 }
 - -/* This will catch lookups from the fs root to .reiserfs_priv */
-static int
-xattr_lookup_poison(struct dentry *dentry, struct qstr *q1, struct
qstr *name)
+static int xattr_hide_revalidate(struct dentry *dentry, struct
nameidata *nd)
 {
-	struct dentry *priv_root = REISERFS_SB(dentry->d_sb)->priv_root;
-	if (container_of(q1, struct dentry, d_name) == priv_root)
-		return -ENOENT;
-	if (q1->len == name->len &&
-		   !memcmp(q1->name, name->name, name->len))
-		return 0;
-	return 1;
+	return -EPERM;
 }
  static const struct dentry_operations xattr_lookup_poison_ops = {
-	.d_compare = xattr_lookup_poison,
+	.d_revalidate = xattr_hide_revalidate,
 };
  int reiserfs_lookup_privroot(struct super_block *s)
@@ -1000,8 +992,7 @@ int reiserfs_lookup_privroot(struct supe
 				strlen(PRIVROOT_NAME));
 	if (!IS_ERR(dentry)) {
 		REISERFS_SB(s)->priv_root = dentry;
-		if (!reiserfs_expose_privroot(s))
-			s->s_root->d_op = &xattr_lookup_poison_ops;
+		dentry->d_op = &xattr_lookup_poison_ops;
 		if (dentry->d_inode)
 			dentry->d_inode->i_flags |= S_PRIVATE;
 	} else
-- 
Jeff Mahoney
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 3+ messages in thread
* [PATCH #2] reiserfs: Fix permissions on .reiserfs_priv
@ 2010-04-08 17:30 ` Jeff Mahoney
  0 siblings, 0 replies; 3+ messages in thread
From: Jeff Mahoney @ 2010-04-08 17:30 UTC (permalink / raw)
  To: Edward Shishkin
  Cc: ReiserFS Mailing List, Linux Kernel Mailing List, Andrew Morton

 Commit 677c9b2e393a0cd203bd54e9c18b012b2c73305a removed the magic
 from the lookup code to hide the .reiserfs_priv directory since it
 was getting loaded at mount-time instead. The intent was that the
 entry would be hidden from the user via a poisoned d_compare, but
 this was faulty.

 This introduced a security issue where unpriviledged users could
 access and modify extended attributes or ACLs belonging to other
 users, including root.

 This patch resolves the issue by properly hiding .reiserfs_priv. This
 was the intent of the xattr poisoning code, but it appears to have
 never worked as expected. This is fixed by using d_revalidate instead
 of d_compare.

 This patch makes -oexpose_privroot a no-op. I'm fine leaving it this
 way. The effort involved in working out the corner cases wrt permissions
 and caching outweigh the benefit of the feature.

 Re-sent because I forgot to turn off the GPG signing when I sent the first
 time.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---

 fs/reiserfs/dir.c   |    2 --
 fs/reiserfs/xattr.c |   17 ++++-------------
 2 files changed, 4 insertions(+), 15 deletions(-)

--- a/fs/reiserfs/dir.c
+++ b/fs/reiserfs/dir.c
@@ -45,8 +45,6 @@ static inline bool is_privroot_deh(struc
 				   struct reiserfs_de_head *deh)
 {
 	struct dentry *privroot = REISERFS_SB(dir->d_sb)->priv_root;
-	if (reiserfs_expose_privroot(dir->d_sb))
-		return 0;
 	return (dir == dir->d_parent && privroot->d_inode &&
 	        deh->deh_objectid == INODE_PKEY(privroot->d_inode)->k_objectid);
 }
--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -972,21 +972,13 @@ int reiserfs_permission(struct inode *in
 	return generic_permission(inode, mask, NULL);
 }
 - -/* This will catch lookups from the fs root to .reiserfs_priv */
-static int
-xattr_lookup_poison(struct dentry *dentry, struct qstr *q1, struct
qstr *name)
+static int xattr_hide_revalidate(struct dentry *dentry, struct
nameidata *nd)
 {
-	struct dentry *priv_root = REISERFS_SB(dentry->d_sb)->priv_root;
-	if (container_of(q1, struct dentry, d_name) == priv_root)
-		return -ENOENT;
-	if (q1->len == name->len &&
-		   !memcmp(q1->name, name->name, name->len))
-		return 0;
-	return 1;
+	return -EPERM;
 }
  static const struct dentry_operations xattr_lookup_poison_ops = {
-	.d_compare = xattr_lookup_poison,
+	.d_revalidate = xattr_hide_revalidate,
 };
  int reiserfs_lookup_privroot(struct super_block *s)
@@ -1000,8 +992,7 @@ int reiserfs_lookup_privroot(struct supe
 				strlen(PRIVROOT_NAME));
 	if (!IS_ERR(dentry)) {
 		REISERFS_SB(s)->priv_root = dentry;
-		if (!reiserfs_expose_privroot(s))
-			s->s_root->d_op = &xattr_lookup_poison_ops;
+		dentry->d_op = &xattr_lookup_poison_ops;
 		if (dentry->d_inode)
 			dentry->d_inode->i_flags |= S_PRIVATE;
 	} else
-- 
Jeff Mahoney
SUSE Labs

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH #2] reiserfs: Fix permissions on .reiserfs_priv
  2010-04-08 17:30 ` Jeff Mahoney
  (?)
@ 2010-04-08 20:52 ` Edward Shishkin
  -1 siblings, 0 replies; 3+ messages in thread
From: Edward Shishkin @ 2010-04-08 20:52 UTC (permalink / raw)
  To: Jeff Mahoney
  Cc: ReiserFS Mailing List, Linux Kernel Mailing List, Andrew Morton

Jeff, it seems your MUA mangles patches,
would you please send it by another tool?

Thanks,
Edward.


Jeff Mahoney wrote:
>  Commit 677c9b2e393a0cd203bd54e9c18b012b2c73305a removed the magic
>  from the lookup code to hide the .reiserfs_priv directory since it
>  was getting loaded at mount-time instead. The intent was that the
>  entry would be hidden from the user via a poisoned d_compare, but
>  this was faulty.
>
>  This introduced a security issue where unpriviledged users could
>  access and modify extended attributes or ACLs belonging to other
>  users, including root.
>
>  This patch resolves the issue by properly hiding .reiserfs_priv. This
>  was the intent of the xattr poisoning code, but it appears to have
>  never worked as expected. This is fixed by using d_revalidate instead
>  of d_compare.
>
>  This patch makes -oexpose_privroot a no-op. I'm fine leaving it this
>  way. The effort involved in working out the corner cases wrt permissions
>  and caching outweigh the benefit of the feature.
>
>  Re-sent because I forgot to turn off the GPG signing when I sent the first
>  time.
>
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>
>  fs/reiserfs/dir.c   |    2 --
>  fs/reiserfs/xattr.c |   17 ++++-------------
>  2 files changed, 4 insertions(+), 15 deletions(-)
>
> --- a/fs/reiserfs/dir.c
> +++ b/fs/reiserfs/dir.c
> @@ -45,8 +45,6 @@ static inline bool is_privroot_deh(struc
>  				   struct reiserfs_de_head *deh)
>  {
>  	struct dentry *privroot = REISERFS_SB(dir->d_sb)->priv_root;
> -	if (reiserfs_expose_privroot(dir->d_sb))
> -		return 0;
>  	return (dir == dir->d_parent && privroot->d_inode &&
>  	        deh->deh_objectid == INODE_PKEY(privroot->d_inode)->k_objectid);
>  }
> --- a/fs/reiserfs/xattr.c
> +++ b/fs/reiserfs/xattr.c
> @@ -972,21 +972,13 @@ int reiserfs_permission(struct inode *in
>  	return generic_permission(inode, mask, NULL);
>  }
>  - -/* This will catch lookups from the fs root to .reiserfs_priv */
> -static int
> -xattr_lookup_poison(struct dentry *dentry, struct qstr *q1, struct
> qstr *name)
> +static int xattr_hide_revalidate(struct dentry *dentry, struct
> nameidata *nd)
>  {
> -	struct dentry *priv_root = REISERFS_SB(dentry->d_sb)->priv_root;
> -	if (container_of(q1, struct dentry, d_name) == priv_root)
> -		return -ENOENT;
> -	if (q1->len == name->len &&
> -		   !memcmp(q1->name, name->name, name->len))
> -		return 0;
> -	return 1;
> +	return -EPERM;
>  }
>   static const struct dentry_operations xattr_lookup_poison_ops = {
> -	.d_compare = xattr_lookup_poison,
> +	.d_revalidate = xattr_hide_revalidate,
>  };
>   int reiserfs_lookup_privroot(struct super_block *s)
> @@ -1000,8 +992,7 @@ int reiserfs_lookup_privroot(struct supe
>  				strlen(PRIVROOT_NAME));
>  	if (!IS_ERR(dentry)) {
>  		REISERFS_SB(s)->priv_root = dentry;
> -		if (!reiserfs_expose_privroot(s))
> -			s->s_root->d_op = &xattr_lookup_poison_ops;
> +		dentry->d_op = &xattr_lookup_poison_ops;
>  		if (dentry->d_inode)
>  			dentry->d_inode->i_flags |= S_PRIVATE;
>  	} else
>   


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-04-08 20:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-08 17:30 [PATCH #2] reiserfs: Fix permissions on .reiserfs_priv Jeff Mahoney
2010-04-08 17:30 ` Jeff Mahoney
2010-04-08 20:52 ` Edward Shishkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.