On Wed, Dec 6, 2017 at 6:33 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> 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()?
>
And better call this helper from within ovl_parse_opt() instead of polluting
ovl_fill_super().
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
