| Message ID | 20190116173945.mmnwsw5bglk6yryj@merlin (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry | expand |
On Thu, Jan 17, 2019 at 1:22 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > Since copy_up() happens when you are modifying a file on overlay, > it is still a new file for the underlying filesystem. Mark it > in IMA for re-evaluating as a new file. > > Putting ima calls within overlayfs may not be the best method, but this is > the only one which I thought would work. > Doesn't look right. Overlayfs creates the new inode with vfs_tmpfile() and I think that is where you should plug the IMA hook. > Here is a test case: > mount /dev/vdb /lower > mount /dev/vdc /upper > echo "Original contents" > /lower/existingfile.txt > mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower > echo "New contents" > /mnt/existingfile.txt > I bet you can reproduce that same issue without overlayfs by creating an O_TMPFILE from userspace. The ima_file_check() hook in do_last() does not cover the O_TMPFILE case. Thanks, Amir.
On 8:57 17/01, Amir Goldstein wrote: > On Thu, Jan 17, 2019 at 1:22 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > > > Since copy_up() happens when you are modifying a file on overlay, > > it is still a new file for the underlying filesystem. Mark it > > in IMA for re-evaluating as a new file. > > > > Putting ima calls within overlayfs may not be the best method, but this is > > the only one which I thought would work. > > > > Doesn't look right. > Overlayfs creates the new inode with vfs_tmpfile() and I think that is > where you should plug the IMA hook. > > > Here is a test case: > > mount /dev/vdb /lower > > mount /dev/vdc /upper > > echo "Original contents" > /lower/existingfile.txt > > mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower > > echo "New contents" > /mnt/existingfile.txt > > > > I bet you can reproduce that same issue without overlayfs > by creating an O_TMPFILE from userspace. > > The ima_file_check() hook in do_last() does not cover the O_TMPFILE > case. > The problem you mention was resolved by https://lkml.org/lkml/2018/12/18/809 which I have in my tree. The current patch is on top of that.
On Thu, Jan 17, 2019 at 5:02 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > On 8:57 17/01, Amir Goldstein wrote: > > On Thu, Jan 17, 2019 at 1:22 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > > > > > Since copy_up() happens when you are modifying a file on overlay, > > > it is still a new file for the underlying filesystem. Mark it > > > in IMA for re-evaluating as a new file. > > > > > > Putting ima calls within overlayfs may not be the best method, but this is > > > the only one which I thought would work. > > > > > > > Doesn't look right. > > Overlayfs creates the new inode with vfs_tmpfile() and I think that is > > where you should plug the IMA hook. > > > > > Here is a test case: > > > mount /dev/vdb /lower > > > mount /dev/vdc /upper > > > echo "Original contents" > /lower/existingfile.txt > > > mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower > > > echo "New contents" > /mnt/existingfile.txt > > > > > > > I bet you can reproduce that same issue without overlayfs > > by creating an O_TMPFILE from userspace. > > > > The ima_file_check() hook in do_last() does not cover the O_TMPFILE > > case. > > > > The problem you mention was resolved by https://lkml.org/lkml/2018/12/18/809 > which I have in my tree. > The proposed hook ima_post_create_tmpfile() inside do_tmpfile() takes a file argument, uses only file_inode() and sets IMA_NEW_FILE. Now because that hook does not get called from vfs_tmpfile() you want to add more ima hook inside overlayfs code after calling vfs_tmpfile(). If you move the IMA hook inside vfs_tmpfile() and pass the dentry or inode, you will get the same result and you won't need to change overlayfs code. Is there a problem with that proposal? Thanks, Amir.
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 9e62dcf06fc4..f3f7f65ce4d3 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -21,6 +21,7 @@ #include <linux/fdtable.h> #include <linux/ratelimit.h> #include <linux/exportfs.h> +#include <linux/ima.h> #include "overlayfs.h" #define OVL_COPY_UP_CHUNK_SIZE (1 << 20) @@ -102,6 +103,11 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new) goto retry; } + if (!strcmp(name, XATTR_NAME_IMA)) { + ima_post_path_mknod(new); + continue; + } + error = security_inode_copy_up_xattr(name); if (error < 0 && error != -EOPNOTSUPP) break; @@ -485,6 +491,8 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) err = ovl_set_size(temp, &c->stat); if (!err) err = ovl_set_attr(temp, &c->stat); + if (!err) + ima_post_path_mknod(c->dentry); inode_unlock(temp->d_inode); return err; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index dbd4c8decde0..2229ea2a0ba6 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -449,6 +449,7 @@ void ima_post_path_mknod(struct dentry *dentry) /* needed for re-opening empty files */ iint->flags |= IMA_NEW_FILE; } +EXPORT_SYMBOL_GPL(ima_post_path_mknod); /** * ima_read_file - pre-measure/appraise hook decision based on policy
Since copy_up() happens when you are modifying a file on overlay, it is still a new file for the underlying filesystem. Mark it in IMA for re-evaluating as a new file. Putting ima calls within overlayfs may not be the best method, but this is the only one which I thought would work. Here is a test case: mount /dev/vdb /lower mount /dev/vdc /upper echo "Original contents" > /lower/existingfile.txt mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower echo "New contents" > /mnt/existingfile.txt Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> --- fs/overlayfs/copy_up.c | 8 ++++++++ security/integrity/ima/ima_main.c | 1 + 2 files changed, 9 insertions(+)