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.
Signed-off-by: Jeff Mahoney <[email protected]>
---
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
Acked-by: Edward Shishkin <[email protected]>
Andrew, I think it should be applied ASAP.
Thanks to Matt McCutchen who discovered this issue.
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.
>
> Signed-off-by: Jeff Mahoney <[email protected]>
> ---
>
> 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
>
>
On Thu, 2010-04-08 at 23:38 +0200, Edward Shishkin wrote:
> Jeff Mahoney wrote:
> > [...] 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. [...]
The patch seems to rely on the fact that the priv_root dentry is stuck
in the dcache for as long as the filesystem is mounted, so that attempts
to follow ".reiserfs_priv" are guaranteed to match that dentry in the
cache and be refused by xattr_hide_revalidate. Is my understanding
correct?
--
Matt
On 04/08/2010 06:39 PM, Matt McCutchen wrote:
> On Thu, 2010-04-08 at 23:38 +0200, Edward Shishkin wrote:
>> Jeff Mahoney wrote:
>>> [...] 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. [...]
>
> The patch seems to rely on the fact that the priv_root dentry is stuck
> in the dcache for as long as the filesystem is mounted, so that attempts
> to follow ".reiserfs_priv" are guaranteed to match that dentry in the
> cache and be refused by xattr_hide_revalidate. Is my understanding
> correct?
>
Yes. The other half of it is that the priv_root dentry being stuck in
the dcache for as long as the flie system is mounted is a guaranteed
since REISERFS_SB(sb) holds a reference to it until reiserfs_kill_sb is
called.
-Jeff
--
Jeff Mahoney
SuSE Labs