On Wed, Dec 6, 2017 at 5:03 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> Overlayfs is following redirects even when redirects are disabled. If this
> is unintentional (probably the majority of cases) then this can be a
> problem. E.g. upper layer comes from untrusted USB drive, and attacker
> crafts a redirect to enable read access to otherwise unreadable
> directories.
>
> If "redirect=off", then turn off following as well as creation of
> redirects. If "redirect=follow", then turn on following, but turn off
> creation of redirects (which is what "redirect=off" does now).
>
s/redirect=/redirect_dir=/
> This is a backward incompatible change, so make it dependent on a config
> option.
>
> Reported-by: David Howells <dhowells@xxxxxxxxxx>
> Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> ---
> v2:
> - added module option for always following (defaulting to Kconfig)
> - added mount option "nofollow" to force not following even if default is to
> always follow
> - fixed .show_options()
> - added more documentation to overlayfs.txt and removed some from Kconfig
>
> Documentation/filesystems/overlayfs.txt | 34 +++++++++++++++++++
> fs/overlayfs/Kconfig | 10 +++++
> fs/overlayfs/namei.c | 16 +++++++++
> fs/overlayfs/ovl_entry.h | 2 +
> fs/overlayfs/super.c | 56 ++++++++++++++++++++++----------
> 5 files changed, 102 insertions(+), 16 deletions(-)
>
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -681,6 +681,22 @@ struct dentry *ovl_lookup(struct inode *
> if (d.stop)
> break;
>
> + /*
> + * Following redirects can have security consequences: it's like
> + * a symlink into the lower layer without the permission checks.
> + * This is only a problem if the upper layer is untrusted (e.g
> + * comes from an USB drive). This can allow a non-readable file
> + * or directory to become readable.
> + *
> + * Only following redirects when redirects are enabled disables
> + * this attack vector when not necessary.
> + */
> + err = -EPERM;
> + if (d.redirect && !ofs->config.redirect_follow) {
> + pr_warn_ratelimited("overlay: refusing to follow redirect for (%pd2)\n", dentry);
> + goto out_put;
> + }
> +
> if (d.redirect && d.redirect[0] == '/' && poe != roe) {
> poe = roe;
>
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -24,6 +24,16 @@ config OVERLAY_FS_REDIRECT_DIR
> an overlay which has redirects on a kernel that doesn't support this
> feature will have unexpected results.
>
> +config OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW
> + bool "Overlayfs: follow redirects even if redirects are turned off"
> + default y
> + depends on OVERLAY_FS
> + help
> + Disable this to get a possibly more secure configuration, but that
> + might not be backward compatible with previous kernels.
> +
> + For more information, see Documentation/filesystems/overlayfs.txt
> +
> config OVERLAY_FS_INDEX
> bool "Overlayfs: turn on inodes index feature by default"
> depends on OVERLAY_FS
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -14,6 +14,8 @@ struct ovl_config {
> char *workdir;
> bool default_permissions;
> bool redirect_dir;
> + bool redirect_follow;
IMO the configuration modes would be better describes by:
enum ovl_redirect {
OVL_REDIRECT_OFF,
OVL_REDIRECT_FOLLOW,
OVL_REDIRECT_ON,
}
Making the combination (!redirect_follow && redirect_dir) impossible
instead of testing ofs->config.redirect_follow, can test
ofs->config.redirect >= OVL_REDIRECT_FOLLOW
Then I could later add:
OVL_REDIRECT_FIX > OVL_REDIRECT_ON
to restore redirect from origin.
> + const char *redirect_mode;
> bool index;
> };
>
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -33,6 +33,13 @@ module_param_named(redirect_dir, ovl_red
> MODULE_PARM_DESC(ovl_redirect_dir_def,
> "Default to on or off for the redirect_dir feature");
>
> +static bool ovl_redirect_always_follow =
> + IS_ENABLED(CONFIG_OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW);
> +module_param_named(redirect_always_follow, ovl_redirect_always_follow,
> + bool, 0644);
> +MODULE_PARM_DESC(ovl_redirect_always_follow,
> + "Follow redirects even if redirect_dir feature is turned off");
> +
> static bool ovl_index_def = IS_ENABLED(CONFIG_OVERLAY_FS_INDEX);
> module_param_named(index, ovl_index_def, bool, 0644);
> MODULE_PARM_DESC(ovl_index_def,
> @@ -232,6 +239,7 @@ static void ovl_free_fs(struct ovl_fs *o
> kfree(ofs->config.lowerdir);
> kfree(ofs->config.upperdir);
> kfree(ofs->config.workdir);
> + kfree(ofs->config.redirect_mode);
> if (ofs->creator_cred)
> put_cred(ofs->creator_cred);
> kfree(ofs);
> @@ -295,6 +303,11 @@ static bool ovl_force_readonly(struct ov
> return (!ofs->upper_mnt || !ofs->workdir);
> }
>
> +static const char *ovl_def_mode_str(void)
ovl_redirect_mode_def()?
> +{
> + return ovl_redirect_dir_def ? "on" : "off";
> +}
> +
> /**
> * ovl_show_options
> *
> @@ -313,12 +326,10 @@ static int ovl_show_options(struct seq_f
> }
> if (ofs->config.default_permissions)
> seq_puts(m, ",default_permissions");
> - if (ofs->config.redirect_dir != ovl_redirect_dir_def)
> - seq_printf(m, ",redirect_dir=%s",
> - ofs->config.redirect_dir ? "on" : "off");
> + if (strcmp(ofs->config.redirect_mode, ovl_def_mode_str()) != 0)
> + seq_printf(m, ",redirect_dir=%s", ofs->config.redirect_mode);
> if (ofs->config.index != ovl_index_def)
> - seq_printf(m, ",index=%s",
> - ofs->config.index ? "on" : "off");
> + seq_printf(m, ",index=%s", ofs->config.index ? "on" : "off");
> return 0;
> }
>
> @@ -348,8 +359,7 @@ enum {
> OPT_UPPERDIR,
> OPT_WORKDIR,
> OPT_DEFAULT_PERMISSIONS,
> - OPT_REDIRECT_DIR_ON,
> - OPT_REDIRECT_DIR_OFF,
> + OPT_REDIRECT_DIR,
> OPT_INDEX_ON,
> OPT_INDEX_OFF,
> OPT_ERR,
> @@ -360,8 +370,7 @@ static const match_table_t ovl_tokens =
> {OPT_UPPERDIR, "upperdir=%s"},
> {OPT_WORKDIR, "workdir=%s"},
> {OPT_DEFAULT_PERMISSIONS, "default_permissions"},
> - {OPT_REDIRECT_DIR_ON, "redirect_dir=on"},
> - {OPT_REDIRECT_DIR_OFF, "redirect_dir=off"},
> + {OPT_REDIRECT_DIR, "redirect_dir=%s"},
> {OPT_INDEX_ON, "index=on"},
> {OPT_INDEX_OFF, "index=off"},
> {OPT_ERR, NULL}
> @@ -428,12 +437,11 @@ static int ovl_parse_opt(char *opt, stru
> config->default_permissions = true;
> break;
>
> - case OPT_REDIRECT_DIR_ON:
> - config->redirect_dir = true;
> - break;
> -
> - case OPT_REDIRECT_DIR_OFF:
> - config->redirect_dir = false;
> + case OPT_REDIRECT_DIR:
> + kfree(config->redirect_mode);
> + config->redirect_mode = match_strdup(&args[0]);
> + if (!config->redirect_mode)
> + return -ENOMEM;
> break;
>
> case OPT_INDEX_ON:
> @@ -1160,12 +1168,28 @@ static int ovl_fill_super(struct super_b
> if (!cred)
> goto out_err;
>
> - ofs->config.redirect_dir = ovl_redirect_dir_def;
> + ofs->config.redirect_mode = kstrdup(ovl_def_mode_str(), GFP_KERNEL);
> + if (!ofs->config.redirect_mode)
> + goto out_err;
> +
> ofs->config.index = ovl_index_def;
> err = ovl_parse_opt((char *) data, &ofs->config);
> if (err)
> goto out_err;
>
And then this would look nicer IMO, maybe even can use token parser:
> + if (strcmp(ofs->config.redirect_mode, "on") == 0) {
> + ofs->config.redirect_dir = true;
> + ofs->config.redirect_follow = true;
ofs->config.redirect = OVL_REDIRECT_ON
> + } else if (strcmp(ofs->config.redirect_mode, "follow") == 0) {
> + ofs->config.redirect_follow = true;
ofs->config.redirect = OVL_REDIRECT_FOLLOW
> + } else if (strcmp(ofs->config.redirect_mode, "off") == 0) {
> + if (ovl_redirect_always_follow)
> + ofs->config.redirect_follow = true;
ofs->config.redirect = OVL_REDIRECT_FOLLOW;
> + } else if (strcmp(ofs->config.redirect_mode, "nofollow") != 0) {
> + pr_err("overlayfs: bad mount option \"redirect_dir=%s\"\n",
> + ofs->config.redirect_mode);
> + }
> +
Helper? ovl_parse_redirect_mode()?
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
