Re: [PATCH] ima: fix wrong dereferences of file->f_path

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 13, 2023 at 10:38 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> When storing IMA xattr on an overlayfs inode, the xattr is actually
> stored in the inode of the underlying (a.k.a real) filesystem, so there
> is an ambiguity whether this IMA xattr describes the integrity of the
> overlayfs inode or the real inode.
>
> For this reason and other reasons, IMA is not supported on overlayfs,
> in the sense that integrity checking on the overlayfs inode/file/path
> do not work correctly and have undefined behavior and the IMA xattr
> always describes the integrity of the real inode.
>
> When a user operates on an overlayfs file, whose underlying real file
> has IMA enabled, IMA should always operate on the real path and not
> on the overlayfs path.
>
> IMA code already uses the helper file_dentry() to get the dentry
> of the real file. Dereferencing file->f_path directly means that IMA
> will operate on the overlayfs inode, which is wrong.
>
> Therefore, all dereferences to f_path were converted to use the
> file_real_path() helper.
>
> Reported-by: syzbot+a67fc5321ffb4b311c98@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/linux-unionfs/0000000000005bd097060530b758@xxxxxxxxxx/
> Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> Cc: Jeff Layton <jlayton@xxxxxxxxxx>
> Cc: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
>
> Mimi,
>
> Some of the wrong f_path dereferences are much older than the Fixes
> commit, but they did not have as big an impact as the wrong f_path
> dereference that the Fixes commit introduced.
>
> For example, commit a408e4a86b36 ("ima: open a new file instance if no
> read permissions") worked because reading the content of the overlayfs
> file has the same result as reading the content of the real file, but it
> is actually the real file integrity that we want to verify.
>
> Anyway, the real path information, that is now available via the
> file_real_path() helper, was not available in IMA integrity check context
> at the time that commit a408e4a86b36 was merged.
>

Only problem is that fix did not resolve the syzbot bug, which
seems to do the IMA integrity check on overlayfs file (not sure).

I am pretty sure that this patch fixes "a bug" when IMA is on the filesystem
under overlayfs and this is a pretty important use case.

But I guess there are still issues with IMA over overlayfs and this is not
the only one.
Is this really a use case that needs to be supported?
Isn't the newly added SB_I_IMA_UNVERIFIABLE_SIGNATURE flag
a hint that IMA on overlayfs is not a good idea at all?

Thanks,
Amir.

>
>  security/integrity/ima/ima_api.c    |  4 ++--
>  security/integrity/ima/ima_crypto.c |  2 +-
>  security/integrity/ima/ima_main.c   | 10 +++++-----
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 452e80b541e5..badf3784a1a0 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -268,8 +268,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>          * to an initial measurement/appraisal/audit, but was modified to
>          * assume the file changed.
>          */
> -       result = vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_COOKIE,
> -                                  AT_STATX_SYNC_AS_STAT);
> +       result = vfs_getattr_nosec(file_real_path(file), &stat,
> +                                  STATX_CHANGE_COOKIE, AT_STATX_SYNC_AS_STAT);
>         if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
>                 i_version = stat.change_cookie;
>         hash.hdr.algo = algo;
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 51ad29940f05..e6c52f3c8f37 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -555,7 +555,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>                 int flags = file->f_flags & ~(O_WRONLY | O_APPEND |
>                                 O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL);
>                 flags |= O_RDONLY;
> -               f = dentry_open(&file->f_path, flags, file->f_cred);
> +               f = dentry_open(file_real_path(file), flags, file->f_cred);
>                 if (IS_ERR(f))
>                         return PTR_ERR(f);
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 365db0e43d7c..87c13effbdf4 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -94,7 +94,7 @@ static int mmap_violation_check(enum ima_hooks func, struct file *file,
>                 inode = file_inode(file);
>
>                 if (!*pathbuf)  /* ima_rdwr_violation possibly pre-fetched */
> -                       *pathname = ima_d_path(&file->f_path, pathbuf,
> +                       *pathname = ima_d_path(file_real_path(file), pathbuf,
>                                                filename);
>                 integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, *pathname,
>                                     "mmap_file", "mmapped_writers", rc, 0);
> @@ -142,7 +142,7 @@ static void ima_rdwr_violation_check(struct file *file,
>         if (!send_tomtou && !send_writers)
>                 return;
>
> -       *pathname = ima_d_path(&file->f_path, pathbuf, filename);
> +       *pathname = ima_d_path(file_real_path(file), pathbuf, filename);
>
>         if (send_tomtou)
>                 ima_add_violation(file, *pathname, iint,
> @@ -168,7 +168,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
>                 update = test_and_clear_bit(IMA_UPDATE_XATTR,
>                                             &iint->atomic_flags);
>                 if ((iint->flags & IMA_NEW_FILE) ||
> -                   vfs_getattr_nosec(&file->f_path, &stat,
> +                   vfs_getattr_nosec(file_real_path(file), &stat,
>                                       STATX_CHANGE_COOKIE,
>                                       AT_STATX_SYNC_AS_STAT) ||
>                     !(stat.result_mask & STATX_CHANGE_COOKIE) ||
> @@ -347,7 +347,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>                 goto out_locked;
>
>         if (!pathbuf)   /* ima_rdwr_violation possibly pre-fetched */
> -               pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> +               pathname = ima_d_path(file_real_path(file), &pathbuf, filename);
>
>         if (action & IMA_MEASURE)
>                 ima_store_measurement(iint, file, pathname,
> @@ -487,7 +487,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
>                 result = -EPERM;
>
>         file = vma->vm_file;
> -       pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> +       pathname = ima_d_path(file_real_path(file), &pathbuf, filename);
>         integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, pathname,
>                             "collect_data", "failed-mprotect", result, 0);
>         if (pathbuf)
> --
> 2.34.1
>




[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux