| Message ID | 0292ade77250a8bb563744f596ecaab5614cbd80.1683102959.git.alexl@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ovl: Add support for fs-verity checking of lowerdata | expand |
On Wed, May 3, 2023 at 11:52 AM Alexander Larsson <alexl@redhat.com> wrote: > > This adds the scaffolding (docs, config, mount options) for supporting > for a new overlay xattr "overlay.verity", which contains a fs-verity > digest. This is used for metacopy files, and the actual fs-verity > digest of the lowerdata file needs to match it. The mount option > "verity" specifies how this xattrs is handled. > > If you enable verity it ("verity=on") all existing xattrs are > validated before use, and during metacopy we generate verity xattr in > the upper metacopy file if the source file has verity enabled. This > means later accesses can guarantee that the correct data is used. > > Additionally you can use "verity=require". In this mode all metacopy > files must have a valid verity xattr. For this to work metadata > copy-up must be able to create a verity xattr (so that later accesses > are validated). Therefore, in this mode, if the lower data file > doesn't have fs-verity enabled we fall back to a full copy rather than > a metacopy. > > Actual implementation follows in a separate commit. > > Signed-off-by: Alexander Larsson <alexl@redhat.com> Reviewed-by: Amir Goldstein <amir73il@gmail.com> > --- > Documentation/filesystems/overlayfs.rst | 27 +++++++++ > fs/overlayfs/ovl_entry.h | 3 + > fs/overlayfs/super.c | 74 ++++++++++++++++++++++++- > 3 files changed, 102 insertions(+), 2 deletions(-) > > diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst > index bc95343bafba..7e2b445a4139 100644 > --- a/Documentation/filesystems/overlayfs.rst > +++ b/Documentation/filesystems/overlayfs.rst > @@ -407,6 +407,33 @@ when a "metacopy" file in one of the lower layers above it, has a "redirect" > to the absolute path of the "lower data" file in the "data-only" lower layer. > > > +fs-verity support > +---------------------- > + > +When metadata copy up is used for a file, then the xattr > +"trusted.overlay.verity" may be set on the metacopy file. This > +specifies the expected fs-verity digest of the lowerdata file. This > +may then be used to verify the content of the source file at the time > +the file is opened. During metacopy copy up overlayfs can also set > +this xattr. > + > +This is controlled by the "verity" mount option, which supports > +these values: > + > +- "off": > + The verity xattr is never used. This is the default if verity > + option is not specified. > +- "on": > + Whenever a metacopy files specifies an expected digest, the > + corresponding data file must match the specified digest. > + When generating a metacopy file the verity xattr will be set > + from the source file fs-verity digest (if it has one). > +- "require": > + Same as "on", but additionally all metacopy files must specify a > + verity xattr. This means metadata copy up will only be used if > + the data file has fs-verity enabled, otherwise a full copy-up is > + used. > + > Sharing and copying layers > -------------------------- > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h > index c6c7d09b494e..95464a1cb371 100644 > --- a/fs/overlayfs/ovl_entry.h > +++ b/fs/overlayfs/ovl_entry.h > @@ -13,6 +13,9 @@ struct ovl_config { > bool redirect_dir; > bool redirect_follow; > const char *redirect_mode; > + bool verity; > + bool require_verity; > + const char *verity_mode; > bool index; > bool uuid; > bool nfs_export; > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index c6209592bb3f..a4662883b619 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -244,6 +244,7 @@ static void ovl_free_fs(struct ovl_fs *ofs) > kfree(ofs->config.upperdir); > kfree(ofs->config.workdir); > kfree(ofs->config.redirect_mode); > + kfree(ofs->config.verity_mode); > if (ofs->creator_cred) > put_cred(ofs->creator_cred); > kfree(ofs); > @@ -334,6 +335,11 @@ static const char *ovl_redirect_mode_def(void) > return ovl_redirect_dir_def ? "on" : "off"; > } > > +static const char *ovl_verity_mode_def(void) > +{ > + return "off"; > +} > + > static const char * const ovl_xino_str[] = { > "off", > "auto", > @@ -383,6 +389,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry) > seq_puts(m, ",volatile"); > if (ofs->config.userxattr) > seq_puts(m, ",userxattr"); > + if (strcmp(ofs->config.verity_mode, ovl_verity_mode_def()) != 0) > + seq_printf(m, ",verity=%s", ofs->config.verity_mode); > return 0; > } > > @@ -438,6 +446,7 @@ enum { > OPT_METACOPY_ON, > OPT_METACOPY_OFF, > OPT_VOLATILE, > + OPT_VERITY, > OPT_ERR, > }; > > @@ -460,6 +469,7 @@ static const match_table_t ovl_tokens = { > {OPT_METACOPY_ON, "metacopy=on"}, > {OPT_METACOPY_OFF, "metacopy=off"}, > {OPT_VOLATILE, "volatile"}, > + {OPT_VERITY, "verity=%s"}, > {OPT_ERR, NULL} > }; > > @@ -509,6 +519,21 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode) > return 0; > } > > +static int ovl_parse_verity_mode(struct ovl_config *config, const char *mode) > +{ > + if (strcmp(mode, "on") == 0) { > + config->verity = true; > + } else if (strcmp(mode, "require") == 0) { > + config->verity = true; > + config->require_verity = true; > + } else if (strcmp(mode, "off") != 0) { > + pr_err("bad mount option \"verity=%s\"\n", mode); > + return -EINVAL; > + } > + > + return 0; > +} > + > static int ovl_parse_opt(char *opt, struct ovl_config *config) > { > char *p; > @@ -520,6 +545,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > if (!config->redirect_mode) > return -ENOMEM; > > + config->verity_mode = kstrdup(ovl_verity_mode_def(), GFP_KERNEL); > + if (!config->verity_mode) > + return -ENOMEM; > + > while ((p = ovl_next_opt(&opt)) != NULL) { > int token; > substring_t args[MAX_OPT_ARGS]; > @@ -620,6 +649,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > config->userxattr = true; > break; > > + case OPT_VERITY: > + kfree(config->verity_mode); > + config->verity_mode = match_strdup(&args[0]); > + if (!config->verity_mode) > + return -ENOMEM; > + break; > + > default: > pr_err("unrecognized mount option \"%s\" or missing value\n", > p); > @@ -651,6 +687,22 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > if (err) > return err; > > + err = ovl_parse_verity_mode(config, config->verity_mode); > + if (err) > + return err; > + > + /* Resolve verity -> metacopy dependency */ > + if (config->verity && !config->metacopy) { > + /* Don't allow explicit specified conflicting combinations */ > + if (metacopy_opt) { > + pr_err("conflicting options: metacopy=off,verity=%s\n", > + config->verity_mode); > + return -EINVAL; > + } > + /* Otherwise automatically enable metacopy. */ > + config->metacopy = true; > + } > + > /* > * This is to make the logic below simpler. It doesn't make any other > * difference, since config->redirect_dir is only used for upper. > @@ -665,6 +717,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > config->redirect_mode); > return -EINVAL; > } > + if (config->verity && redirect_opt) { > + pr_err("conflicting options: verity=%s,redirect_dir=%s\n", > + config->verity_mode, config->redirect_mode); > + return -EINVAL; > + } > if (redirect_opt) { > /* > * There was an explicit redirect_dir=... that resulted > @@ -700,7 +757,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > } > } > > - /* Resolve nfs_export -> !metacopy dependency */ > + /* Resolve nfs_export -> !metacopy && !verity dependency */ > if (config->nfs_export && config->metacopy) { > if (nfs_export_opt && metacopy_opt) { > pr_err("conflicting options: nfs_export=on,metacopy=on\n"); > @@ -713,6 +770,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > */ > pr_info("disabling nfs_export due to metacopy=on\n"); > config->nfs_export = false; > + } else if (config->verity) { > + /* > + * There was an explicit verity=.. that resulted > + * in this conflict. > + */ > + pr_info("disabling nfs_export due to verity=%s\n", > + config->verity_mode); > + config->nfs_export = false; > } else { > /* > * There was an explicit nfs_export=on that resulted > @@ -724,7 +789,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > } > > > - /* Resolve userxattr -> !redirect && !metacopy dependency */ > + /* Resolve userxattr -> !redirect && !metacopy && !verity dependency */ > if (config->userxattr) { > if (config->redirect_follow && redirect_opt) { > pr_err("conflicting options: userxattr,redirect_dir=%s\n", > @@ -735,6 +800,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > pr_err("conflicting options: userxattr,metacopy=on\n"); > return -EINVAL; > } > + if (config->verity) { > + pr_err("conflicting options: userxattr,verity=%s\n", > + config->verity_mode); > + return -EINVAL; > + } > /* > * Silently disable default setting of redirect and metacopy. > * This shall be the default in the future as well: these > -- > 2.39.2 >
On Wed, May 03, 2023 at 10:51:37AM +0200, Alexander Larsson wrote: > +- "require": > + Same as "on", but additionally all metacopy files must specify a > + verity xattr. This means metadata copy up will only be used if > + the data file has fs-verity enabled, otherwise a full copy-up is > + used. The second sentence makes it sound like an attacker can inject arbitrary data just by replacing a data file with one that doesn't have fsverity enabled. I really hope that's not the case? I *think* there is a subtlety here involving "metacopy files" that were created ahead of time by the user, vs. being generated by overlayfs. But it's not really explained. - Eric
On Sun, May 14, 2023 at 9:22 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Wed, May 03, 2023 at 10:51:37AM +0200, Alexander Larsson wrote: > > +- "require": > > + Same as "on", but additionally all metacopy files must specify a > > + verity xattr. This means metadata copy up will only be used if > > + the data file has fs-verity enabled, otherwise a full copy-up is > > + used. > > The second sentence makes it sound like an attacker can inject arbitrary data > just by replacing a data file with one that doesn't have fsverity enabled. > > I really hope that's not the case? > > I *think* there is a subtlety here involving "metacopy files" that were created > ahead of time by the user, vs. being generated by overlayfs. But it's not > really explained. I'm not sure what you mean here? When you say "replacing a data file", do you mean "changing the content of the lowerdir"? Because if you can just change lowerdir content then you can make users of the overlayfs mount read whatever data you want (independent of metacopy or any of this). The second sentence above relates to this case: * A lower dir file "lower/a" does not have fsverity enabled. * Someone does "chown mnt/a" * This causes overlayfs to make a "copy up" operation of the "lower/a" to "upper/a" * But, we can't use the optimized "copy metacopy only" version of "copy up", because we could then not specify a digest xattr on the new file, so we copy the content of "lower/a" to "upper/a". What exactly is your worry in this case?
On Mon, May 15, 2023 at 07:44:13AM +0200, Alexander Larsson wrote: > On Sun, May 14, 2023 at 9:22 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Wed, May 03, 2023 at 10:51:37AM +0200, Alexander Larsson wrote: > > > +- "require": > > > + Same as "on", but additionally all metacopy files must specify a > > > + verity xattr. This means metadata copy up will only be used if > > > + the data file has fs-verity enabled, otherwise a full copy-up is > > > + used. > > > > The second sentence makes it sound like an attacker can inject arbitrary data > > just by replacing a data file with one that doesn't have fsverity enabled. > > > > I really hope that's not the case? > > > > I *think* there is a subtlety here involving "metacopy files" that were created > > ahead of time by the user, vs. being generated by overlayfs. But it's not > > really explained. > > I'm not sure what you mean here? When you say "replacing a data file", > do you mean "changing the content of the lowerdir"? Yes. Specifically the data-only lowerdir. > Because if you can just change lowerdir content then you can make users of the > overlayfs mount read whatever data you want (independent of metacopy or any of > this). But isn't preventing that the whole point of your feature? What am I missing? - Eric
On Mon, May 15, 2023 at 8:00 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Mon, May 15, 2023 at 07:44:13AM +0200, Alexander Larsson wrote: > > On Sun, May 14, 2023 at 9:22 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Wed, May 03, 2023 at 10:51:37AM +0200, Alexander Larsson wrote: > > > > +- "require": > > > > + Same as "on", but additionally all metacopy files must specify a > > > > + verity xattr. This means metadata copy up will only be used if > > > > + the data file has fs-verity enabled, otherwise a full copy-up is > > > > + used. > > > > > > The second sentence makes it sound like an attacker can inject arbitrary data > > > just by replacing a data file with one that doesn't have fsverity enabled. > > > > > > I really hope that's not the case? > > > > > > I *think* there is a subtlety here involving "metacopy files" that were created > > > ahead of time by the user, vs. being generated by overlayfs. But it's not > > > really explained. > > > > I'm not sure what you mean here? When you say "replacing a data file", > > do you mean "changing the content of the lowerdir"? > > Yes. Specifically the data-only lowerdir. > > > Because if you can just change lowerdir content then you can make users of the > > overlayfs mount read whatever data you want (independent of metacopy or any of > > this). > > But isn't preventing that the whole point of your feature? > > What am I missing? If by "your feature", you mean the full composefs idea, then things are different. In this case the root mount is an overlayfs with two lowerdirs and no upper. The uppermost lower is a read-only erofs mount with metadata and overlay xattrs for all files, and the lowermost one is a data-only dir. You cannot modify the uppermost layer (since it's a fsverity readonly erofs file). If you modify a data-only file it will be detected at validation, because it can only be reached from redirects in the image file, and we ensured all image files specified a digest. There is no upper dir, so the overlayfs mount itself is not writable, and thus there can be any meta-copy-up operations triggered at all. However, even if you had an upper dir ("writable composefs" usecase) I don't see a problem. If you trigger a full (i.e. non-metadata) copy-up then the copy-up operation itself will validate the data from the lower at copy time (due to the verity xattr on the "middle layer"). I think again there is some confusion due to the two kinds of usecases for the verity support. The basic usecase is just for general robustness. I.e it allows the kernel to record information about the original state of the content file when generating a metacopy reference to it. If you later accidentally mount the overlayfs against the wrong lower version you will get told about it. The second usecase is the tamper proofing image case (composefs), and its security depends on more details (i.e. the readonly image part and its construction).
diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst index bc95343bafba..7e2b445a4139 100644 --- a/Documentation/filesystems/overlayfs.rst +++ b/Documentation/filesystems/overlayfs.rst @@ -407,6 +407,33 @@ when a "metacopy" file in one of the lower layers above it, has a "redirect" to the absolute path of the "lower data" file in the "data-only" lower layer. +fs-verity support +---------------------- + +When metadata copy up is used for a file, then the xattr +"trusted.overlay.verity" may be set on the metacopy file. This +specifies the expected fs-verity digest of the lowerdata file. This +may then be used to verify the content of the source file at the time +the file is opened. During metacopy copy up overlayfs can also set +this xattr. + +This is controlled by the "verity" mount option, which supports +these values: + +- "off": + The verity xattr is never used. This is the default if verity + option is not specified. +- "on": + Whenever a metacopy files specifies an expected digest, the + corresponding data file must match the specified digest. + When generating a metacopy file the verity xattr will be set + from the source file fs-verity digest (if it has one). +- "require": + Same as "on", but additionally all metacopy files must specify a + verity xattr. This means metadata copy up will only be used if + the data file has fs-verity enabled, otherwise a full copy-up is + used. + Sharing and copying layers -------------------------- diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h index c6c7d09b494e..95464a1cb371 100644 --- a/fs/overlayfs/ovl_entry.h +++ b/fs/overlayfs/ovl_entry.h @@ -13,6 +13,9 @@ struct ovl_config { bool redirect_dir; bool redirect_follow; const char *redirect_mode; + bool verity; + bool require_verity; + const char *verity_mode; bool index; bool uuid; bool nfs_export; diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index c6209592bb3f..a4662883b619 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -244,6 +244,7 @@ static void ovl_free_fs(struct ovl_fs *ofs) kfree(ofs->config.upperdir); kfree(ofs->config.workdir); kfree(ofs->config.redirect_mode); + kfree(ofs->config.verity_mode); if (ofs->creator_cred) put_cred(ofs->creator_cred); kfree(ofs); @@ -334,6 +335,11 @@ static const char *ovl_redirect_mode_def(void) return ovl_redirect_dir_def ? "on" : "off"; } +static const char *ovl_verity_mode_def(void) +{ + return "off"; +} + static const char * const ovl_xino_str[] = { "off", "auto", @@ -383,6 +389,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry) seq_puts(m, ",volatile"); if (ofs->config.userxattr) seq_puts(m, ",userxattr"); + if (strcmp(ofs->config.verity_mode, ovl_verity_mode_def()) != 0) + seq_printf(m, ",verity=%s", ofs->config.verity_mode); return 0; } @@ -438,6 +446,7 @@ enum { OPT_METACOPY_ON, OPT_METACOPY_OFF, OPT_VOLATILE, + OPT_VERITY, OPT_ERR, }; @@ -460,6 +469,7 @@ static const match_table_t ovl_tokens = { {OPT_METACOPY_ON, "metacopy=on"}, {OPT_METACOPY_OFF, "metacopy=off"}, {OPT_VOLATILE, "volatile"}, + {OPT_VERITY, "verity=%s"}, {OPT_ERR, NULL} }; @@ -509,6 +519,21 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode) return 0; } +static int ovl_parse_verity_mode(struct ovl_config *config, const char *mode) +{ + if (strcmp(mode, "on") == 0) { + config->verity = true; + } else if (strcmp(mode, "require") == 0) { + config->verity = true; + config->require_verity = true; + } else if (strcmp(mode, "off") != 0) { + pr_err("bad mount option \"verity=%s\"\n", mode); + return -EINVAL; + } + + return 0; +} + static int ovl_parse_opt(char *opt, struct ovl_config *config) { char *p; @@ -520,6 +545,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) if (!config->redirect_mode) return -ENOMEM; + config->verity_mode = kstrdup(ovl_verity_mode_def(), GFP_KERNEL); + if (!config->verity_mode) + return -ENOMEM; + while ((p = ovl_next_opt(&opt)) != NULL) { int token; substring_t args[MAX_OPT_ARGS]; @@ -620,6 +649,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) config->userxattr = true; break; + case OPT_VERITY: + kfree(config->verity_mode); + config->verity_mode = match_strdup(&args[0]); + if (!config->verity_mode) + return -ENOMEM; + break; + default: pr_err("unrecognized mount option \"%s\" or missing value\n", p); @@ -651,6 +687,22 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) if (err) return err; + err = ovl_parse_verity_mode(config, config->verity_mode); + if (err) + return err; + + /* Resolve verity -> metacopy dependency */ + if (config->verity && !config->metacopy) { + /* Don't allow explicit specified conflicting combinations */ + if (metacopy_opt) { + pr_err("conflicting options: metacopy=off,verity=%s\n", + config->verity_mode); + return -EINVAL; + } + /* Otherwise automatically enable metacopy. */ + config->metacopy = true; + } + /* * This is to make the logic below simpler. It doesn't make any other * difference, since config->redirect_dir is only used for upper. @@ -665,6 +717,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) config->redirect_mode); return -EINVAL; } + if (config->verity && redirect_opt) { + pr_err("conflicting options: verity=%s,redirect_dir=%s\n", + config->verity_mode, config->redirect_mode); + return -EINVAL; + } if (redirect_opt) { /* * There was an explicit redirect_dir=... that resulted @@ -700,7 +757,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) } } - /* Resolve nfs_export -> !metacopy dependency */ + /* Resolve nfs_export -> !metacopy && !verity dependency */ if (config->nfs_export && config->metacopy) { if (nfs_export_opt && metacopy_opt) { pr_err("conflicting options: nfs_export=on,metacopy=on\n"); @@ -713,6 +770,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) */ pr_info("disabling nfs_export due to metacopy=on\n"); config->nfs_export = false; + } else if (config->verity) { + /* + * There was an explicit verity=.. that resulted + * in this conflict. + */ + pr_info("disabling nfs_export due to verity=%s\n", + config->verity_mode); + config->nfs_export = false; } else { /* * There was an explicit nfs_export=on that resulted @@ -724,7 +789,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) } - /* Resolve userxattr -> !redirect && !metacopy dependency */ + /* Resolve userxattr -> !redirect && !metacopy && !verity dependency */ if (config->userxattr) { if (config->redirect_follow && redirect_opt) { pr_err("conflicting options: userxattr,redirect_dir=%s\n", @@ -735,6 +800,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) pr_err("conflicting options: userxattr,metacopy=on\n"); return -EINVAL; } + if (config->verity) { + pr_err("conflicting options: userxattr,verity=%s\n", + config->verity_mode); + return -EINVAL; + } /* * Silently disable default setting of redirect and metacopy. * This shall be the default in the future as well: these
This adds the scaffolding (docs, config, mount options) for supporting for a new overlay xattr "overlay.verity", which contains a fs-verity digest. This is used for metacopy files, and the actual fs-verity digest of the lowerdata file needs to match it. The mount option "verity" specifies how this xattrs is handled. If you enable verity it ("verity=on") all existing xattrs are validated before use, and during metacopy we generate verity xattr in the upper metacopy file if the source file has verity enabled. This means later accesses can guarantee that the correct data is used. Additionally you can use "verity=require". In this mode all metacopy files must have a valid verity xattr. For this to work metadata copy-up must be able to create a verity xattr (so that later accesses are validated). Therefore, in this mode, if the lower data file doesn't have fs-verity enabled we fall back to a full copy rather than a metacopy. Actual implementation follows in a separate commit. Signed-off-by: Alexander Larsson <alexl@redhat.com> --- Documentation/filesystems/overlayfs.rst | 27 +++++++++ fs/overlayfs/ovl_entry.h | 3 + fs/overlayfs/super.c | 74 ++++++++++++++++++++++++- 3 files changed, 102 insertions(+), 2 deletions(-)