2018-11-05 18:22:40

by Mark Salyzyn

[permalink] [raw]

Subject: [PATCH v6 0/2]

---

overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

Assumption never checked, should fail if the mounter creds are not
sufficient.

overlayfs: override_creds=off option bypass creator_cred

By default, all access to the upper, lower and work directories is the
recorded mounter's MAC and DAC credentials. The incoming accesses are
checked against the caller's credentials.

If the principles of least privilege are applied, the mounter's
credentials might not overlap the credentials of the caller's when
accessing the overlayfs filesystem. For example, a file that a lower
DAC privileged caller can execute, is MAC denied to the generally
higher DAC privileged mounter, to prevent an attack vector.

We add the option to turn off override_creds in the mount options; all
subsequent operations after mount on the filesystem will be only the
caller's credentials. The module boolean parameter and mount option
override_creds is also added as a presence check for this "feature",
existence of /sys/module/overlay/parameters/overlay_creds

Signed-off-by: Mark Salyzyn <[email protected]>
Cc: Miklos Szeredi <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Vivek Goyal <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Cc: Amir Goldstein <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Stephen Smalley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

v2:
- Forward port changed attr to stat, resulting in a build error.
- altered commit message.

v3:
- Change name from caller_credentials / creator_credentials to the
boolean override_creds.
- Changed from creator to mounter credentials.
- Updated and fortified the documentation.
- Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS

v4:
- spelling and grammar errors in text

v5:
- beefed up the caveats in the Documentation
- Is dependent on
"overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
"overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
- Added prwarn when override_creds=off

v6:
- Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
- Do better with the documentation, drop rationalizations.
- pr_warn message adjusted to report consequences.

2018-11-05 18:22:56

by Mark Salyzyn

[permalink] [raw]

Subject: [PATCH v6 1/2] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

---

Assumption never checked, should fail if the mounter creds are not
sufficient.

Signed-off-by: Mark Salyzyn <[email protected]>
Cc: Miklos Szeredi <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Vivek Goyal <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Cc: Amir Goldstein <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Stephen Smalley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

v5:
- dependency of "overlayfs: override_creds=off option bypass creator_cred"

v6:
- rebase
---
fs/overlayfs/namei.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index efd372312ef1..aa012b6bd46e 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -163,6 +163,11 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
return NULL;

+ if (!capable(CAP_DAC_READ_SEARCH)) {
+ origin = ERR_PTR(-EPERM);
+ goto out;
+ }
+
bytes = (fh->len - offsetof(struct ovl_fh, fid));
real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
bytes >> 2, (int)fh->type,
--
2.19.1.930.g4563a0d9d0-goog

2018-11-05 18:23:25

by Mark Salyzyn

[permalink] [raw]

Subject: [PATCH v6 2/2] overlayfs: override_creds=off option bypass creator_cred

---

By default, all access to the upper, lower and work directories is the
recorded mounter's MAC and DAC credentials. The incoming accesses are
checked against the caller's credentials.

If the principles of least privilege are applied, the mounter's
credentials might not overlap the credentials of the caller's when
accessing the overlayfs filesystem. For example, a file that a lower
DAC privileged caller can execute, is MAC denied to the generally
higher DAC privileged mounter, to prevent an attack vector.

We add the option to turn off override_creds in the mount options; all
subsequent operations after mount on the filesystem will be only the
caller's credentials. The module boolean parameter and mount option
override_creds is also added as a presence check for this "feature",
existence of /sys/module/overlay/parameters/overlay_creds.

Signed-off-by: Mark Salyzyn <[email protected]>
Cc: Miklos Szeredi <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Vivek Goyal <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Cc: Amir Goldstein <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Stephen Smalley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

v2:
- Forward port changed attr to stat, resulting in a build error.
- altered commit message.

v3:
- Change name from caller_credentials / creator_credentials to the
boolean override_creds.
- Changed from creator to mounter credentials.
- Updated and fortified the documentation.
- Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS

v4:
- spelling and grammar errors in text

v5:
- beefed up the caveats in the Documentation
- Is dependent on
"overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
"overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
- Added prwarn when override_creds=off

v6:
- Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
- Do better with the documentation.
- pr_warn message adjusted to report consequences.
---
Documentation/filesystems/overlayfs.txt | 17 +++++++++++++++++
fs/overlayfs/copy_up.c | 2 +-
fs/overlayfs/dir.c | 9 +++++----
fs/overlayfs/inode.c | 16 ++++++++--------
fs/overlayfs/namei.c | 6 +++---
fs/overlayfs/overlayfs.h | 1 +
fs/overlayfs/ovl_entry.h | 1 +
fs/overlayfs/readdir.c | 4 ++--
fs/overlayfs/super.c | 23 ++++++++++++++++++++++-
fs/overlayfs/util.c | 12 ++++++++++--
10 files changed, 70 insertions(+), 21 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
index eef7d9d259e8..5cc299df4436 100644
--- a/Documentation/filesystems/overlayfs.txt
+++ b/Documentation/filesystems/overlayfs.txt
@@ -102,6 +102,23 @@ Only the lists of names from directories are merged. Other content
such as metadata and extended attributes are reported for the upper
directory only. These attributes of the lower directory are hidden.

+credentials
+-----------
+
+By default, all access to the upper, lower and work directories is the
+recorded mounter's MAC and DAC credentials. The incoming accesses are
+checked against the caller's credentials.
+
+override_creds mount flag turned off is reserved for when mounter and
+caller MAC or DAC credentials do not overlap. Several unintended side
+effects will occur. The caller with a lower privilege will not be
+able to delete files or directories, create nodes, or search some
+directories. The caller with higher privilege can perform unexpected
+or unsecured operations. The uneven security model where upperdir
+and workdir are opened at privilege, but accessed without, should only
+be used with strict understanding of the side effects and of the
+security policies.
+
whiteouts and opaque directories
--------------------------------

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 9e62dcf06fc4..dfab62ce7504 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -860,7 +860,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
dput(parent);
dput(next);
}
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);

return err;
}
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index c6289147c787..b7052e23c467 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -566,7 +566,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
override_cred->fsgid = inode->i_gid;
if (!attr->hardlink) {
err = security_dentry_create_files_as(dentry,
- attr->mode, &dentry->d_name, old_cred,
+ attr->mode, &dentry->d_name,
+ old_cred ? old_cred : current_cred(),
override_cred);
if (err) {
put_cred(override_cred);
@@ -582,7 +583,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
err = ovl_create_over_whiteout(dentry, inode, attr);
}
out_revert_creds:
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
return err;
}

@@ -842,7 +843,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
err = ovl_remove_upper(dentry, is_dir, &list);
else
err = ovl_remove_and_whiteout(dentry, &list);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
if (!err) {
if (is_dir)
clear_nlink(dentry->d_inode);
@@ -1212,7 +1213,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
out_unlock:
unlock_rename(new_upperdir, old_upperdir);
out_revert_creds:
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
if (update_nlink)
ovl_nlink_end(new);
out_drop_write:
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 6bcc9dedc342..192f5508ed45 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -64,7 +64,7 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
inode_lock(upperdentry->d_inode);
old_cred = ovl_override_creds(dentry->d_sb);
err = notify_change(upperdentry, attr, NULL);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
if (!err)
ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
inode_unlock(upperdentry->d_inode);
@@ -260,7 +260,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
stat->nlink = dentry->d_inode->i_nlink;

out:
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);

return err;
}
@@ -303,7 +303,7 @@ int ovl_permission(struct inode *inode, int mask)

old_cred = ovl_override_creds(inode->i_sb);
err = inode_permission(realinode, mask);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);

return err;
}
@@ -320,7 +320,7 @@ static const char *ovl_get_link(struct dentry *dentry,

old_cred = ovl_override_creds(dentry->d_sb);
p = vfs_get_link(ovl_dentry_real(dentry), done);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
return p;
}

@@ -363,7 +363,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
WARN_ON(flags != XATTR_REPLACE);
err = vfs_removexattr(realdentry, name);
}
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);

/* copy c/mtime */
ovl_copyattr(d_inode(realdentry), inode);
@@ -384,7 +384,7 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,

old_cred = ovl_override_creds(dentry->d_sb);
res = vfs_getxattr(realdentry, name, value, size);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
return res;
}

@@ -408,7 +408,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)

old_cred = ovl_override_creds(dentry->d_sb);
res = vfs_listxattr(realdentry, list, size);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
if (res <= 0 || size == 0)
return res;

@@ -443,7 +443,7 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)

old_cred = ovl_override_creds(inode->i_sb);
acl = get_acl(realinode, type);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);

return acl;
}
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index aa012b6bd46e..b73e5f7aea2e 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1074,7 +1074,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
goto out_free_oe;
}

- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
if (origin_path) {
dput(origin_path->dentry);
kfree(origin_path);
@@ -1101,7 +1101,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
kfree(upperredirect);
out:
kfree(d.redirect);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
return ERR_PTR(err);
}

@@ -1155,7 +1155,7 @@ bool ovl_lower_positive(struct dentry *dentry)
dput(this);
}
}
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);

return positive;
}
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 5e45cb3630a0..6f8b6f9ff357 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -208,6 +208,7 @@ int ovl_want_write(struct dentry *dentry);
void ovl_drop_write(struct dentry *dentry);
struct dentry *ovl_workdir(struct dentry *dentry);
const struct cred *ovl_override_creds(struct super_block *sb);
+void ovl_revert_creds(const struct cred *oldcred);
struct super_block *ovl_same_sb(struct super_block *sb);
int ovl_can_decode_fh(struct super_block *sb);
struct dentry *ovl_indexdir(struct super_block *sb);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index ec237035333a..e38eea8104be 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -20,6 +20,7 @@ struct ovl_config {
bool nfs_export;
int xino;
bool metacopy;
+ bool override_creds;
};

struct ovl_sb {
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index cc8303a806b4..ec591b49e902 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -289,7 +289,7 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
}
inode_unlock(dir->d_inode);
}
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);

return err;
}
@@ -921,7 +921,7 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)

old_cred = ovl_override_creds(dentry->d_sb);
err = ovl_dir_read_merged(dentry, list, &root);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
if (err)
return err;

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 0116735cc321..1669d4fa7ad8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -56,6 +56,11 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
MODULE_PARM_DESC(ovl_xino_auto_def,
"Auto enable xino feature");

+static bool __read_mostly ovl_default_override_creds = true;
+module_param_named(override_creds, ovl_default_override_creds, bool, 0644);
+MODULE_PARM_DESC(ovl_default_override_creds,
+ "Use mounter's credentials for accesses");
+
static void ovl_entry_stack_free(struct ovl_entry *oe)
{
unsigned int i;
@@ -362,6 +367,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
if (ofs->config.metacopy != ovl_metacopy_def)
seq_printf(m, ",metacopy=%s",
ofs->config.metacopy ? "on" : "off");
+ seq_show_option(m, "override_creds",
+ ofs->config.override_creds ? "on" : "off");
return 0;
}

@@ -401,6 +408,8 @@ enum {
OPT_XINO_AUTO,
OPT_METACOPY_ON,
OPT_METACOPY_OFF,
+ OPT_OVERRIDE_CREDS_ON,
+ OPT_OVERRIDE_CREDS_OFF,
OPT_ERR,
};

@@ -419,6 +428,8 @@ static const match_table_t ovl_tokens = {
{OPT_XINO_AUTO, "xino=auto"},
{OPT_METACOPY_ON, "metacopy=on"},
{OPT_METACOPY_OFF, "metacopy=off"},
+ {OPT_OVERRIDE_CREDS_ON, "override_creds=on"},
+ {OPT_OVERRIDE_CREDS_OFF, "override_creds=off"},
{OPT_ERR, NULL}
};

@@ -477,6 +488,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
if (!config->redirect_mode)
return -ENOMEM;
+ config->override_creds = ovl_default_override_creds;

while ((p = ovl_next_opt(&opt)) != NULL) {
int token;
@@ -557,6 +569,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
config->metacopy = false;
break;

+ case OPT_OVERRIDE_CREDS_ON:
+ config->override_creds = true;
+ break;
+
+ case OPT_OVERRIDE_CREDS_OFF:
+ config->override_creds = false;
+ break;
+
default:
pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
return -EINVAL;
@@ -1549,7 +1569,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
ovl_dentry_lower(root_dentry), NULL);

sb->s_root = root_dentry;
-
+ if (!ofs->config.override_creds)
+ pr_warn("overlayfs: override_creds=off, caller credentials may not be enough to delete file or directories, create nodes, or search directories.\n");
return 0;

out_free_oe:
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 7c01327b1852..484d7f76ac9c 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -40,9 +40,17 @@ const struct cred *ovl_override_creds(struct super_block *sb)
{
struct ovl_fs *ofs = sb->s_fs_info;

+ if (!ofs->config.override_creds)
+ return NULL;
return override_creds(ofs->creator_cred);
}

+void ovl_revert_creds(const struct cred *old_cred)
+{
+ if (old_cred)
+ revert_creds(old_cred);
+}
+
struct super_block *ovl_same_sb(struct super_block *sb)
{
struct ovl_fs *ofs = sb->s_fs_info;
@@ -782,7 +790,7 @@ int ovl_nlink_start(struct dentry *dentry)
* value relative to the upper inode nlink in an upper inode xattr.
*/
err = ovl_set_nlink_upper(dentry);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);

out:
if (err)
@@ -800,7 +808,7 @@ void ovl_nlink_end(struct dentry *dentry)

old_cred = ovl_override_creds(dentry->d_sb);
ovl_cleanup_index(dentry);
- revert_creds(old_cred);
+ ovl_revert_creds(old_cred);
}

ovl_inode_unlock(inode);
--
2.19.1.930.g4563a0d9d0-goog

2018-11-05 18:36:18

by Amir Goldstein

[permalink] [raw]

Subject: Re: [PATCH v6 1/2] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

---

On Mon, Nov 5, 2018 at 8:22 PM Mark Salyzyn <[email protected]> wrote:
>
> Assumption never checked, should fail if the mounter creds are not
> sufficient.
>
> Signed-off-by: Mark Salyzyn <[email protected]>
> Cc: Miklos Szeredi <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Vivek Goyal <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: Amir Goldstein <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> v5:
> - dependency of "overlayfs: override_creds=off option bypass creator_cred"
>
> v6:
> - rebase

1. rebase onto which branch? doesn't look like the right one
2. pls keep patch revision outside of commit message
you can put it after --- line
3. revisions are usually ordered latest on the top

> ---
> fs/overlayfs/namei.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index efd372312ef1..aa012b6bd46e 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -163,6 +163,11 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
> if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
> return NULL;
>
> + if (!capable(CAP_DAC_READ_SEARCH)) {
> + origin = ERR_PTR(-EPERM);
> + goto out;

I have a deja vu
there is no out label in this function in upstream kernel
you must post patches that are applies and tested on upstream kernel

Thanks,
Amir.

> + }
> +
> bytes = (fh->len - offsetof(struct ovl_fh, fid));
> real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
> bytes >> 2, (int)fh->type,
> --
> 2.19.1.930.g4563a0d9d0-goog
>

2018-11-05 18:48:45

by Amir Goldstein

[permalink] [raw]

Subject: Re: [PATCH v6 2/2] overlayfs: override_creds=off option bypass creator_cred

---

On Mon, Nov 5, 2018 at 8:22 PM Mark Salyzyn <[email protected]> wrote:
>
> By default, all access to the upper, lower and work directories is the
> recorded mounter's MAC and DAC credentials. The incoming accesses are
> checked against the caller's credentials.
>
> If the principles of least privilege are applied, the mounter's
> credentials might not overlap the credentials of the caller's when
> accessing the overlayfs filesystem. For example, a file that a lower
> DAC privileged caller can execute, is MAC denied to the generally
> higher DAC privileged mounter, to prevent an attack vector.
>
> We add the option to turn off override_creds in the mount options; all
> subsequent operations after mount on the filesystem will be only the
> caller's credentials. The module boolean parameter and mount option
> override_creds is also added as a presence check for this "feature",
> existence of /sys/module/overlay/parameters/overlay_creds.
>
> Signed-off-by: Mark Salyzyn <[email protected]>
> Cc: Miklos Szeredi <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Vivek Goyal <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: Amir Goldstein <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> v2:
> - Forward port changed attr to stat, resulting in a build error.
> - altered commit message.
>
> v3:
> - Change name from caller_credentials / creator_credentials to the
> boolean override_creds.
> - Changed from creator to mounter credentials.
> - Updated and fortified the documentation.
> - Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS
>
> v4:
> - spelling and grammar errors in text
>
> v5:
> - beefed up the caveats in the Documentation
> - Is dependent on
> "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
> "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
> - Added prwarn when override_creds=off
>
> v6:
> - Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
> - Do better with the documentation.
> - pr_warn message adjusted to report consequences.

same comment about patch revision - not in commit message.

> ---
> Documentation/filesystems/overlayfs.txt | 17 +++++++++++++++++
> fs/overlayfs/copy_up.c | 2 +-
> fs/overlayfs/dir.c | 9 +++++----
> fs/overlayfs/inode.c | 16 ++++++++--------
> fs/overlayfs/namei.c | 6 +++---
> fs/overlayfs/overlayfs.h | 1 +
> fs/overlayfs/ovl_entry.h | 1 +
> fs/overlayfs/readdir.c | 4 ++--
> fs/overlayfs/super.c | 23 ++++++++++++++++++++++-
> fs/overlayfs/util.c | 12 ++++++++++--
> 10 files changed, 70 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index eef7d9d259e8..5cc299df4436 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -102,6 +102,23 @@ Only the lists of names from directories are merged. Other content
> such as metadata and extended attributes are reported for the upper
> directory only. These attributes of the lower directory are hidden.
>
> +credentials
> +-----------
> +
> +By default, all access to the upper, lower and work directories is the
> +recorded mounter's MAC and DAC credentials. The incoming accesses are
> +checked against the caller's credentials.
> +
> +override_creds mount flag turned off is reserved for when mounter and
> +caller MAC or DAC credentials do not overlap. Several unintended side
> +effects will occur. The caller with a lower privilege will not be
> +able to delete files or directories, create nodes, or search some
> +directories. The caller with higher privilege can perform unexpected
> +or unsecured operations. The uneven security model where upperdir
> +and workdir are opened at privilege, but accessed without, should only
> +be used with strict understanding of the side effects and of the
> +security policies.
> +
> whiteouts and opaque directories
> --------------------------------
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 9e62dcf06fc4..dfab62ce7504 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -860,7 +860,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
> dput(parent);
> dput(next);
> }
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return err;
> }
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index c6289147c787..b7052e23c467 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -566,7 +566,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> override_cred->fsgid = inode->i_gid;
> if (!attr->hardlink) {
> err = security_dentry_create_files_as(dentry,
> - attr->mode, &dentry->d_name, old_cred,
> + attr->mode, &dentry->d_name,
> + old_cred ? old_cred : current_cred(),
> override_cred);
> if (err) {
> put_cred(override_cred);
> @@ -582,7 +583,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> err = ovl_create_over_whiteout(dentry, inode, attr);
> }
> out_revert_creds:
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> return err;
> }
>
> @@ -842,7 +843,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
> err = ovl_remove_upper(dentry, is_dir, &list);
> else
> err = ovl_remove_and_whiteout(dentry, &list);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (!err) {
> if (is_dir)
> clear_nlink(dentry->d_inode);
> @@ -1212,7 +1213,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
> out_unlock:
> unlock_rename(new_upperdir, old_upperdir);
> out_revert_creds:
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (update_nlink)
> ovl_nlink_end(new);
> out_drop_write:
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 6bcc9dedc342..192f5508ed45 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -64,7 +64,7 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
> inode_lock(upperdentry->d_inode);
> old_cred = ovl_override_creds(dentry->d_sb);
> err = notify_change(upperdentry, attr, NULL);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (!err)
> ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
> inode_unlock(upperdentry->d_inode);
> @@ -260,7 +260,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> stat->nlink = dentry->d_inode->i_nlink;
>
> out:
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return err;
> }
> @@ -303,7 +303,7 @@ int ovl_permission(struct inode *inode, int mask)
>
> old_cred = ovl_override_creds(inode->i_sb);
> err = inode_permission(realinode, mask);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return err;
> }
> @@ -320,7 +320,7 @@ static const char *ovl_get_link(struct dentry *dentry,
>
> old_cred = ovl_override_creds(dentry->d_sb);
> p = vfs_get_link(ovl_dentry_real(dentry), done);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> return p;
> }
>
> @@ -363,7 +363,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
> WARN_ON(flags != XATTR_REPLACE);
> err = vfs_removexattr(realdentry, name);
> }
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> /* copy c/mtime */
> ovl_copyattr(d_inode(realdentry), inode);
> @@ -384,7 +384,7 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
>
> old_cred = ovl_override_creds(dentry->d_sb);
> res = vfs_getxattr(realdentry, name, value, size);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> return res;
> }
>
> @@ -408,7 +408,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
>
> old_cred = ovl_override_creds(dentry->d_sb);
> res = vfs_listxattr(realdentry, list, size);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (res <= 0 || size == 0)
> return res;
>
> @@ -443,7 +443,7 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
>
> old_cred = ovl_override_creds(inode->i_sb);
> acl = get_acl(realinode, type);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return acl;
> }
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index aa012b6bd46e..b73e5f7aea2e 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1074,7 +1074,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> goto out_free_oe;
> }
>
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (origin_path) {
> dput(origin_path->dentry);
> kfree(origin_path);
> @@ -1101,7 +1101,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> kfree(upperredirect);
> out:
> kfree(d.redirect);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> return ERR_PTR(err);
> }
>
> @@ -1155,7 +1155,7 @@ bool ovl_lower_positive(struct dentry *dentry)
> dput(this);
> }
> }
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return positive;
> }
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 5e45cb3630a0..6f8b6f9ff357 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -208,6 +208,7 @@ int ovl_want_write(struct dentry *dentry);
> void ovl_drop_write(struct dentry *dentry);
> struct dentry *ovl_workdir(struct dentry *dentry);
> const struct cred *ovl_override_creds(struct super_block *sb);
> +void ovl_revert_creds(const struct cred *oldcred);
> struct super_block *ovl_same_sb(struct super_block *sb);
> int ovl_can_decode_fh(struct super_block *sb);
> struct dentry *ovl_indexdir(struct super_block *sb);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index ec237035333a..e38eea8104be 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -20,6 +20,7 @@ struct ovl_config {
> bool nfs_export;
> int xino;
> bool metacopy;
> + bool override_creds;
> };
>
> struct ovl_sb {
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index cc8303a806b4..ec591b49e902 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -289,7 +289,7 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
> }
> inode_unlock(dir->d_inode);
> }
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return err;
> }
> @@ -921,7 +921,7 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
>
> old_cred = ovl_override_creds(dentry->d_sb);
> err = ovl_dir_read_merged(dentry, list, &root);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (err)
> return err;
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 0116735cc321..1669d4fa7ad8 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -56,6 +56,11 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
> MODULE_PARM_DESC(ovl_xino_auto_def,
> "Auto enable xino feature");
>
> +static bool __read_mostly ovl_default_override_creds = true;

Please stick to conventions - ovl_override_creds_def.

> +module_param_named(override_creds, ovl_default_override_creds, bool, 0644);
> +MODULE_PARM_DESC(ovl_default_override_creds,
> + "Use mounter's credentials for accesses");
> +
> static void ovl_entry_stack_free(struct ovl_entry *oe)
> {
> unsigned int i;
> @@ -362,6 +367,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
> if (ofs->config.metacopy != ovl_metacopy_def)
> seq_printf(m, ",metacopy=%s",
> ofs->config.metacopy ? "on" : "off");
> + seq_show_option(m, "override_creds",

show only if != ovl_override_creds_def

> + ofs->config.override_creds ? "on" : "off");
> return 0;
> }
>
> @@ -401,6 +408,8 @@ enum {
> OPT_XINO_AUTO,
> OPT_METACOPY_ON,
> OPT_METACOPY_OFF,
> + OPT_OVERRIDE_CREDS_ON,
> + OPT_OVERRIDE_CREDS_OFF,
> OPT_ERR,
> };
>
> @@ -419,6 +428,8 @@ static const match_table_t ovl_tokens = {
> {OPT_XINO_AUTO, "xino=auto"},
> {OPT_METACOPY_ON, "metacopy=on"},
> {OPT_METACOPY_OFF, "metacopy=off"},
> + {OPT_OVERRIDE_CREDS_ON, "override_creds=on"},
> + {OPT_OVERRIDE_CREDS_OFF, "override_creds=off"},
> {OPT_ERR, NULL}
> };
>
> @@ -477,6 +488,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
> if (!config->redirect_mode)
> return -ENOMEM;
> + config->override_creds = ovl_default_override_creds;
>
> while ((p = ovl_next_opt(&opt)) != NULL) {
> int token;
> @@ -557,6 +569,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> config->metacopy = false;
> break;
>
> + case OPT_OVERRIDE_CREDS_ON:
> + config->override_creds = true;
> + break;
> +
> + case OPT_OVERRIDE_CREDS_OFF:
> + config->override_creds = false;
> + break;
> +
> default:
> pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
> return -EINVAL;
> @@ -1549,7 +1569,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> ovl_dentry_lower(root_dentry), NULL);
>
> sb->s_root = root_dentry;
> -
> + if (!ofs->config.override_creds)
> + pr_warn("overlayfs: override_creds=off, caller credentials may not be enough to delete file or directories, create nodes, or search directories.\n");

The audience is someone that has this feature on by mistake or someone
that turn it
on without understanding what it does. I am not sure that this is
scary enough, but
I don't have a better suggestion.
Will let others state their opinion.

Thanks,
Amir.

2018-11-05 18:56:28

by Amir Goldstein

[permalink] [raw]

Subject: Re: [PATCH v6 0/2]

---

On Mon, Nov 5, 2018 at 8:22 PM Mark Salyzyn <[email protected]> wrote:
>
> overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh
>
> Assumption never checked, should fail if the mounter creds are not
> sufficient.
>
> overlayfs: override_creds=off option bypass creator_cred
>
> By default, all access to the upper, lower and work directories is the
> recorded mounter's MAC and DAC credentials. The incoming accesses are
> checked against the caller's credentials.
>
> If the principles of least privilege are applied, the mounter's
> credentials might not overlap the credentials of the caller's when
> accessing the overlayfs filesystem. For example, a file that a lower
> DAC privileged caller can execute, is MAC denied to the generally
> higher DAC privileged mounter, to prevent an attack vector.
>
> We add the option to turn off override_creds in the mount options; all
> subsequent operations after mount on the filesystem will be only the
> caller's credentials. The module boolean parameter and mount option
> override_creds is also added as a presence check for this "feature",
> existence of /sys/module/overlay/parameters/overlay_creds
>
> Signed-off-by: Mark Salyzyn <[email protected]>
> Cc: Miklos Szeredi <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Vivek Goyal <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: Amir Goldstein <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> v2:
> - Forward port changed attr to stat, resulting in a build error.
> - altered commit message.
>
> v3:
> - Change name from caller_credentials / creator_credentials to the
> boolean override_creds.
> - Changed from creator to mounter credentials.
> - Updated and fortified the documentation.
> - Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS
>
> v4:
> - spelling and grammar errors in text
>
> v5:
> - beefed up the caveats in the Documentation
> - Is dependent on
> "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
> "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
> - Added prwarn when override_creds=off
>
> v6:
> - Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
> - Do better with the documentation, drop rationalizations.
> - pr_warn message adjusted to report consequences.

For future reference, the cover letter is meant for introduction,
not for concatenating all commit messages...
Not sure how you generated this text.
I think you were looking for git format-patch --cover.
Anyway, IMO, having the patch revision in the cover letter is sufficient
and you needn't bother with patch revision in every single patch, unless
there is a good reason.

Thanks,
Amir.

2018-11-06 01:06:59

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v6 1/2] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

---

Hi Mark,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on miklos-vfs/overlayfs-next]
[also build test ERROR on v4.20-rc1 next-20181105]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Mark-Salyzyn/overlayfs-check-CAP_DAC_READ_SEARCH-before-issuing-exportfs_decode_fh/20181106-073038
base: https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: i386-randconfig-x002-201844 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

All errors (new ones prefixed by >>):

fs/overlayfs/namei.c: In function 'ovl_decode_real_fh':
>> fs/overlayfs/namei.c:167:3: error: 'origin' undeclared (first use in this function); did you mean 'orig_ist'?
origin = ERR_PTR(-EPERM);
^~~~~~
orig_ist
fs/overlayfs/namei.c:167:3: note: each undeclared identifier is reported only once for each function it appears in
>> fs/overlayfs/namei.c:168:3: error: label 'out' used but not defined
goto out;
^~~~

vim +167 fs/overlayfs/namei.c

152
153 struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
154 bool connected)
155 {
156 struct dentry *real;
157 int bytes;
158
159 /*
160 * Make sure that the stored uuid matches the uuid of the lower
161 * layer where file handle will be decoded.
162 */
163 if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
164 return NULL;
165
166 if (!capable(CAP_DAC_READ_SEARCH)) {
> 167 origin = ERR_PTR(-EPERM);
> 168 goto out;
169 }
170
171 bytes = (fh->len - offsetof(struct ovl_fh, fid));
172 real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
173 bytes >> 2, (int)fh->type,
174 connected ? ovl_acceptable : NULL, mnt);
175 if (IS_ERR(real)) {
176 /*
177 * Treat stale file handle to lower file as "origin unknown".
178 * upper file handle could become stale when upper file is
179 * unlinked and this information is needed to handle stale
180 * index entries correctly.
181 */
182 if (real == ERR_PTR(-ESTALE) &&
183 !(fh->flags & OVL_FH_FLAG_PATH_UPPER))
184 real = NULL;
185 return real;
186 }
187
188 if (ovl_dentry_weird(real)) {
189 dput(real);
190 return NULL;
191 }
192
193 return real;
194 }
195

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

---

Attachments:

(No filename) (2.72 kB)
.config.gz (31.61 kB)
Download all attachments

2018-11-06 01:34:59

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v6 1/2] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

---

Hi Mark,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on miklos-vfs/overlayfs-next]
[also build test ERROR on v4.20-rc1 next-20181105]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Mark-Salyzyn/overlayfs-check-CAP_DAC_READ_SEARCH-before-issuing-exportfs_decode_fh/20181106-073038
base: https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=sh

All errors (new ones prefixed by >>):

fs//overlayfs/namei.c: In function 'ovl_decode_real_fh':
>> fs//overlayfs/namei.c:167:3: error: 'origin' undeclared (first use in this function); did you mean 'isodigit'?
origin = ERR_PTR(-EPERM);
^~~~~~
isodigit
fs//overlayfs/namei.c:167:3: note: each undeclared identifier is reported only once for each function it appears in
fs//overlayfs/namei.c:168:3: error: label 'out' used but not defined
goto out;
^~~~

vim +167 fs//overlayfs/namei.c

152
153 struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
154 bool connected)
155 {
156 struct dentry *real;
157 int bytes;
158
159 /*
160 * Make sure that the stored uuid matches the uuid of the lower
161 * layer where file handle will be decoded.
162 */
163 if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
164 return NULL;
165
166 if (!capable(CAP_DAC_READ_SEARCH)) {
> 167 origin = ERR_PTR(-EPERM);
168 goto out;
169 }
170
171 bytes = (fh->len - offsetof(struct ovl_fh, fid));
172 real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
173 bytes >> 2, (int)fh->type,
174 connected ? ovl_acceptable : NULL, mnt);
175 if (IS_ERR(real)) {
176 /*
177 * Treat stale file handle to lower file as "origin unknown".
178 * upper file handle could become stale when upper file is
179 * unlinked and this information is needed to handle stale
180 * index entries correctly.
181 */
182 if (real == ERR_PTR(-ESTALE) &&
183 !(fh->flags & OVL_FH_FLAG_PATH_UPPER))
184 real = NULL;
185 return real;
186 }
187
188 if (ovl_dentry_weird(real)) {
189 dput(real);
190 return NULL;
191 }
192
193 return real;
194 }
195

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

---

Attachments:

(No filename) (2.89 kB)
.config.gz (48.49 kB)
Download all attachments

2018-11-06 08:40:47

by Miklos Szeredi

[permalink] [raw]

Subject: Re: [PATCH v6 2/2] overlayfs: override_creds=off option bypass creator_cred

---

On Mon, Nov 5, 2018 at 7:47 PM, Amir Goldstein <[email protected]> wrote:
> On Mon, Nov 5, 2018 at 8:22 PM Mark Salyzyn <[email protected]> wrote:
>>
>> By default, all access to the upper, lower and work directories is the
>> recorded mounter's MAC and DAC credentials. The incoming accesses are
>> checked against the caller's credentials.
>>
>> If the principles of least privilege are applied, the mounter's
>> credentials might not overlap the credentials of the caller's when
>> accessing the overlayfs filesystem. For example, a file that a lower
>> DAC privileged caller can execute, is MAC denied to the generally
>> higher DAC privileged mounter, to prevent an attack vector.
>>
>> We add the option to turn off override_creds in the mount options; all
>> subsequent operations after mount on the filesystem will be only the
>> caller's credentials. The module boolean parameter and mount option
>> override_creds is also added as a presence check for this "feature",
>> existence of /sys/module/overlay/parameters/overlay_creds.
>>
>> Signed-off-by: Mark Salyzyn <[email protected]>
>> Cc: Miklos Szeredi <[email protected]>
>> Cc: Jonathan Corbet <[email protected]>
>> Cc: Vivek Goyal <[email protected]>
>> Cc: Eric W. Biederman <[email protected]>
>> Cc: Amir Goldstein <[email protected]>
>> Cc: Randy Dunlap <[email protected]>
>> Cc: Stephen Smalley <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>>
>> v2:
>> - Forward port changed attr to stat, resulting in a build error.
>> - altered commit message.
>>
>> v3:
>> - Change name from caller_credentials / creator_credentials to the
>> boolean override_creds.
>> - Changed from creator to mounter credentials.
>> - Updated and fortified the documentation.
>> - Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS
>>
>> v4:
>> - spelling and grammar errors in text
>>
>> v5:
>> - beefed up the caveats in the Documentation
>> - Is dependent on
>> "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
>> "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
>> - Added prwarn when override_creds=off
>>
>> v6:
>> - Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
>> - Do better with the documentation.
>> - pr_warn message adjusted to report consequences.
>
> same comment about patch revision - not in commit message.
>
>> ---
>> Documentation/filesystems/overlayfs.txt | 17 +++++++++++++++++
>> fs/overlayfs/copy_up.c | 2 +-
>> fs/overlayfs/dir.c | 9 +++++----
>> fs/overlayfs/inode.c | 16 ++++++++--------
>> fs/overlayfs/namei.c | 6 +++---
>> fs/overlayfs/overlayfs.h | 1 +
>> fs/overlayfs/ovl_entry.h | 1 +
>> fs/overlayfs/readdir.c | 4 ++--
>> fs/overlayfs/super.c | 23 ++++++++++++++++++++++-
>> fs/overlayfs/util.c | 12 ++++++++++--
>> 10 files changed, 70 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
>> index eef7d9d259e8..5cc299df4436 100644
>> --- a/Documentation/filesystems/overlayfs.txt
>> +++ b/Documentation/filesystems/overlayfs.txt
>> @@ -102,6 +102,23 @@ Only the lists of names from directories are merged. Other content
>> such as metadata and extended attributes are reported for the upper
>> directory only. These attributes of the lower directory are hidden.
>>
>> +credentials
>> +-----------
>> +
>> +By default, all access to the upper, lower and work directories is the
>> +recorded mounter's MAC and DAC credentials. The incoming accesses are
>> +checked against the caller's credentials.
>> +
>> +override_creds mount flag turned off is reserved for when mounter and
>> +caller MAC or DAC credentials do not overlap. Several unintended side
>> +effects will occur. The caller with a lower privilege will not be
>> +able to delete files or directories, create nodes, or search some
>> +directories. The caller with higher privilege can perform unexpected
>> +or unsecured operations. The uneven security model where upperdir
>> +and workdir are opened at privilege, but accessed without, should only
>> +be used with strict understanding of the side effects and of the
>> +security policies.
>> +
>> whiteouts and opaque directories
>> --------------------------------
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index 9e62dcf06fc4..dfab62ce7504 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -860,7 +860,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
>> dput(parent);
>> dput(next);
>> }
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>>
>> return err;
>> }
>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> index c6289147c787..b7052e23c467 100644
>> --- a/fs/overlayfs/dir.c
>> +++ b/fs/overlayfs/dir.c
>> @@ -566,7 +566,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>> override_cred->fsgid = inode->i_gid;
>> if (!attr->hardlink) {
>> err = security_dentry_create_files_as(dentry,
>> - attr->mode, &dentry->d_name, old_cred,
>> + attr->mode, &dentry->d_name,
>> + old_cred ? old_cred : current_cred(),
>> override_cred);
>> if (err) {
>> put_cred(override_cred);
>> @@ -582,7 +583,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>> err = ovl_create_over_whiteout(dentry, inode, attr);
>> }
>> out_revert_creds:
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>> return err;
>> }
>>
>> @@ -842,7 +843,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
>> err = ovl_remove_upper(dentry, is_dir, &list);
>> else
>> err = ovl_remove_and_whiteout(dentry, &list);
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>> if (!err) {
>> if (is_dir)
>> clear_nlink(dentry->d_inode);
>> @@ -1212,7 +1213,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>> out_unlock:
>> unlock_rename(new_upperdir, old_upperdir);
>> out_revert_creds:
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>> if (update_nlink)
>> ovl_nlink_end(new);
>> out_drop_write:
>> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>> index 6bcc9dedc342..192f5508ed45 100644
>> --- a/fs/overlayfs/inode.c
>> +++ b/fs/overlayfs/inode.c
>> @@ -64,7 +64,7 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
>> inode_lock(upperdentry->d_inode);
>> old_cred = ovl_override_creds(dentry->d_sb);
>> err = notify_change(upperdentry, attr, NULL);
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>> if (!err)
>> ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
>> inode_unlock(upperdentry->d_inode);
>> @@ -260,7 +260,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>> stat->nlink = dentry->d_inode->i_nlink;
>>
>> out:
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>>
>> return err;
>> }
>> @@ -303,7 +303,7 @@ int ovl_permission(struct inode *inode, int mask)
>>
>> old_cred = ovl_override_creds(inode->i_sb);
>> err = inode_permission(realinode, mask);
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>>
>> return err;
>> }
>> @@ -320,7 +320,7 @@ static const char *ovl_get_link(struct dentry *dentry,
>>
>> old_cred = ovl_override_creds(dentry->d_sb);
>> p = vfs_get_link(ovl_dentry_real(dentry), done);
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>> return p;
>> }
>>
>> @@ -363,7 +363,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
>> WARN_ON(flags != XATTR_REPLACE);
>> err = vfs_removexattr(realdentry, name);
>> }
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>>
>> /* copy c/mtime */
>> ovl_copyattr(d_inode(realdentry), inode);
>> @@ -384,7 +384,7 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
>>
>> old_cred = ovl_override_creds(dentry->d_sb);
>> res = vfs_getxattr(realdentry, name, value, size);
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>> return res;
>> }
>>
>> @@ -408,7 +408,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
>>
>> old_cred = ovl_override_creds(dentry->d_sb);
>> res = vfs_listxattr(realdentry, list, size);
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>> if (res <= 0 || size == 0)
>> return res;
>>
>> @@ -443,7 +443,7 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
>>
>> old_cred = ovl_override_creds(inode->i_sb);
>> acl = get_acl(realinode, type);
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>>
>> return acl;
>> }
>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> index aa012b6bd46e..b73e5f7aea2e 100644
>> --- a/fs/overlayfs/namei.c
>> +++ b/fs/overlayfs/namei.c
>> @@ -1074,7 +1074,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> goto out_free_oe;
>> }
>>
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>> if (origin_path) {
>> dput(origin_path->dentry);
>> kfree(origin_path);
>> @@ -1101,7 +1101,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> kfree(upperredirect);
>> out:
>> kfree(d.redirect);
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>> return ERR_PTR(err);
>> }
>>
>> @@ -1155,7 +1155,7 @@ bool ovl_lower_positive(struct dentry *dentry)
>> dput(this);
>> }
>> }
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>>
>> return positive;
>> }
>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> index 5e45cb3630a0..6f8b6f9ff357 100644
>> --- a/fs/overlayfs/overlayfs.h
>> +++ b/fs/overlayfs/overlayfs.h
>> @@ -208,6 +208,7 @@ int ovl_want_write(struct dentry *dentry);
>> void ovl_drop_write(struct dentry *dentry);
>> struct dentry *ovl_workdir(struct dentry *dentry);
>> const struct cred *ovl_override_creds(struct super_block *sb);
>> +void ovl_revert_creds(const struct cred *oldcred);
>> struct super_block *ovl_same_sb(struct super_block *sb);
>> int ovl_can_decode_fh(struct super_block *sb);
>> struct dentry *ovl_indexdir(struct super_block *sb);
>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
>> index ec237035333a..e38eea8104be 100644
>> --- a/fs/overlayfs/ovl_entry.h
>> +++ b/fs/overlayfs/ovl_entry.h
>> @@ -20,6 +20,7 @@ struct ovl_config {
>> bool nfs_export;
>> int xino;
>> bool metacopy;
>> + bool override_creds;
>> };
>>
>> struct ovl_sb {
>> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
>> index cc8303a806b4..ec591b49e902 100644
>> --- a/fs/overlayfs/readdir.c
>> +++ b/fs/overlayfs/readdir.c
>> @@ -289,7 +289,7 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
>> }
>> inode_unlock(dir->d_inode);
>> }
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>>
>> return err;
>> }
>> @@ -921,7 +921,7 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
>>
>> old_cred = ovl_override_creds(dentry->d_sb);
>> err = ovl_dir_read_merged(dentry, list, &root);
>> - revert_creds(old_cred);
>> + ovl_revert_creds(old_cred);
>> if (err)
>> return err;
>>
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index 0116735cc321..1669d4fa7ad8 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -56,6 +56,11 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
>> MODULE_PARM_DESC(ovl_xino_auto_def,
>> "Auto enable xino feature");
>>
>> +static bool __read_mostly ovl_default_override_creds = true;
>
> Please stick to conventions - ovl_override_creds_def.
>
>> +module_param_named(override_creds, ovl_default_override_creds, bool, 0644);
>> +MODULE_PARM_DESC(ovl_default_override_creds,
>> + "Use mounter's credentials for accesses");
>> +
>> static void ovl_entry_stack_free(struct ovl_entry *oe)
>> {
>> unsigned int i;
>> @@ -362,6 +367,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>> if (ofs->config.metacopy != ovl_metacopy_def)
>> seq_printf(m, ",metacopy=%s",
>> ofs->config.metacopy ? "on" : "off");
>> + seq_show_option(m, "override_creds",
>
> show only if != ovl_override_creds_def
>
>> + ofs->config.override_creds ? "on" : "off");
>> return 0;
>> }
>>
>> @@ -401,6 +408,8 @@ enum {
>> OPT_XINO_AUTO,
>> OPT_METACOPY_ON,
>> OPT_METACOPY_OFF,
>> + OPT_OVERRIDE_CREDS_ON,
>> + OPT_OVERRIDE_CREDS_OFF,
>> OPT_ERR,
>> };
>>
>> @@ -419,6 +428,8 @@ static const match_table_t ovl_tokens = {
>> {OPT_XINO_AUTO, "xino=auto"},
>> {OPT_METACOPY_ON, "metacopy=on"},
>> {OPT_METACOPY_OFF, "metacopy=off"},
>> + {OPT_OVERRIDE_CREDS_ON, "override_creds=on"},
>> + {OPT_OVERRIDE_CREDS_OFF, "override_creds=off"},
>> {OPT_ERR, NULL}
>> };
>>
>> @@ -477,6 +488,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>> config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
>> if (!config->redirect_mode)
>> return -ENOMEM;
>> + config->override_creds = ovl_default_override_creds;
>>
>> while ((p = ovl_next_opt(&opt)) != NULL) {
>> int token;
>> @@ -557,6 +569,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>> config->metacopy = false;
>> break;
>>
>> + case OPT_OVERRIDE_CREDS_ON:
>> + config->override_creds = true;
>> + break;
>> +
>> + case OPT_OVERRIDE_CREDS_OFF:
>> + config->override_creds = false;
>> + break;
>> +
>> default:
>> pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
>> return -EINVAL;
>> @@ -1549,7 +1569,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>> ovl_dentry_lower(root_dentry), NULL);
>>
>> sb->s_root = root_dentry;
>> -
>> + if (!ofs->config.override_creds)
>> + pr_warn("overlayfs: override_creds=off, caller credentials may not be enough to delete file or directories, create nodes, or search directories.\n");
>
> The audience is someone that has this feature on by mistake or someone
> that turn it
> on without understanding what it does. I am not sure that this is
> scary enough, but
> I don't have a better suggestion.
> Will let others state their opinion.

I don't think we need any warning message, writing down the rules in
the documentation should be enough.

Thanks,
Miklos

2018-11-06 16:52:19

by Mark Salyzyn

[permalink] [raw]

Subject: Re: [PATCH v6 2/2] overlayfs: override_creds=off option bypass creator_cred

---

On 11/06/2018 12:39 AM, Miklos Szeredi wrote:
> On Mon, Nov 5, 2018 at 7:47 PM, Amir Goldstein <[email protected]> wrote:
>> On Mon, Nov 5, 2018 at 8:22 PM Mark Salyzyn <[email protected]> wrote:
>>> @@ -1549,7 +1569,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>>> ovl_dentry_lower(root_dentry), NULL);
>>>
>>> sb->s_root = root_dentry;
>>> -
>>> + if (!ofs->config.override_creds)
>>> + pr_warn("overlayfs: override_creds=off, caller credentials may not be enough to delete file or directories, create nodes, or search directories.\n");
>> The audience is someone that has this feature on by mistake or someone
>> that turn it
>> on without understanding what it does. I am not sure that this is
>> scary enough, but
>> I don't have a better suggestion.
>> Will let others state their opinion.
> I don't think we need any warning message, writing down the rules in
> the documentation should be enough.
I would be pleased to remove them, but maybe more historical background
is required in the documentation (see below [TL;DR])? I have been told
to not talk rationalization, history, use cases; but only about side
effects in the documentation.

Yes, the documentation (in the v7 patch set) cites this problem, so can
remove this pr_warn in a v8 respin. Does anyone disagree? will respin by
EOD if nothing said.

[TL;DR]

In 4.4 the default behaviour was effectively !override_creds since the
mounter or creator MAC and DAC credentials wrapping the existing caller
credentials had not yet been added until later. Except at that time the
capabilities were temporarily elevated inside the overlayfs driver
during the cited operations to (blindly) permit these few.

When the mounter's MAC and DAC credentials were added (later), security
was greatly improved by not counting on the elevated DAC, but it broke
the expected 4.4 user space API. So in 4.9 and higher Android will
require this patch to restore the behaviour that supports
non-overlapping MAC credentials. But we chose _not_ to re-add the
(inadvisable) hard coded elevated credentials for these specific
accesses thus requiring the caller to _have_ the DAC credentials to
perform them.

- For those using the 4.4 way of doing things, these noted operations work.
- For those using the 4.9 way of doing things, the non-overlapping
creator and caller MAC credentials broke.
- For latest, this patch brought back the support for non-overlapping
MAC credentials, without the security issues of the 4.4 implementation,
but alas breaks the 4.4 way of doing things as noted in this warning
message.

-- Mark