| Message ID | 20240412140122.2607743-3-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Fix detection of read/write violations on stacked filesystems | expand |
On Fri, Apr 12, 2024 at 5:01 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > On a stacked filesystem, when one process opens the file holding a file's > data (e.g., on upper or lower layer on overlayfs) then issue a violation > when another process opens the file for reading on the top layer (overlay > layer on overlayfs). This then provides similar behavior to the existing > case where a violation is generated when one process opens a file for > writing and another one opens the same file for reading. On stacked > filesystem also search all the lower layers for relevant files opened for > writing and issue the violation if one is found. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- > security/integrity/ima/ima_main.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index f04f43af651c..590dd9d5d99a 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -121,8 +121,11 @@ static void ima_rdwr_violation_check(struct file *file, > const char **pathname, > char *filename) > { > + struct inode *real_inode = d_real_inode(file_dentry(file)); > struct inode *inode = file_inode(file); > + struct dentry *fd_dentry, *d; > fmode_t mode = file->f_mode; > + struct inode *fd_inode; > bool send_tomtou = false, send_writers = false; > > if (mode & FMODE_WRITE) { > @@ -134,11 +137,25 @@ static void ima_rdwr_violation_check(struct file *file, > &iint->atomic_flags)) > send_tomtou = true; > } > - } else { > - if (must_measure) > - set_bit(IMA_MUST_MEASURE, &iint->atomic_flags); > - if (inode_is_open_for_write(inode) && must_measure) > - send_writers = true; > + } else if (must_measure) { > + set_bit(IMA_MUST_MEASURE, &iint->atomic_flags); > + > + if (inode == real_inode) { > + if (inode_is_open_for_write(inode)) > + send_writers = true; > + } else { > + d = d_real(file_dentry(file), D_REAL_FILEDATA); > + do { > + fd_dentry = d; > + fd_inode = d_inode(fd_dentry); > + if (inode_is_open_for_write(fd_inode)) { > + send_writers = true; > + break; > + } > + /* next layer of stacked fs */ > + d = d_real(fd_dentry, D_REAL_FILEDATA); > + } while (d != fd_dentry); > + } The idea of digging though ovl layers feels wrong to me. As Miklos is the designer of overlayfs and its vfs architecture, I am deferring the call about adding this interface to Miklos. Thanks, Amir.
On 4/12/24 14:08, Amir Goldstein wrote: > On Fri, Apr 12, 2024 at 5:01 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> On a stacked filesystem, when one process opens the file holding a file's >> data (e.g., on upper or lower layer on overlayfs) then issue a violation >> when another process opens the file for reading on the top layer (overlay >> layer on overlayfs). This then provides similar behavior to the existing >> case where a violation is generated when one process opens a file for >> writing and another one opens the same file for reading. On stacked >> filesystem also search all the lower layers for relevant files opened for >> writing and issue the violation if one is found. >> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >> --- >> security/integrity/ima/ima_main.c | 27 ++++++++++++++++++++++----- >> 1 file changed, 22 insertions(+), 5 deletions(-) >> >> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c >> index f04f43af651c..590dd9d5d99a 100644 >> --- a/security/integrity/ima/ima_main.c >> +++ b/security/integrity/ima/ima_main.c >> @@ -121,8 +121,11 @@ static void ima_rdwr_violation_check(struct file *file, >> const char **pathname, >> char *filename) >> { >> + struct inode *real_inode = d_real_inode(file_dentry(file)); >> struct inode *inode = file_inode(file); >> + struct dentry *fd_dentry, *d; >> fmode_t mode = file->f_mode; >> + struct inode *fd_inode; >> bool send_tomtou = false, send_writers = false; >> >> if (mode & FMODE_WRITE) { >> @@ -134,11 +137,25 @@ static void ima_rdwr_violation_check(struct file *file, >> &iint->atomic_flags)) >> send_tomtou = true; >> } >> - } else { >> - if (must_measure) >> - set_bit(IMA_MUST_MEASURE, &iint->atomic_flags); >> - if (inode_is_open_for_write(inode) && must_measure) >> - send_writers = true; >> + } else if (must_measure) { >> + set_bit(IMA_MUST_MEASURE, &iint->atomic_flags); >> + >> + if (inode == real_inode) { >> + if (inode_is_open_for_write(inode)) >> + send_writers = true; >> + } else { >> + d = d_real(file_dentry(file), D_REAL_FILEDATA); >> + do { >> + fd_dentry = d; >> + fd_inode = d_inode(fd_dentry); >> + if (inode_is_open_for_write(fd_inode)) { >> + send_writers = true; >> + break; >> + } >> + /* next layer of stacked fs */ >> + d = d_real(fd_dentry, D_REAL_FILEDATA); >> + } while (d != fd_dentry); >> + } > > The idea of digging though ovl layers feels wrong to me. I have a couple of test cases that expect violations to be logged. One test case has 2 overlay filesystems stacked on top of each other (lower = A, upper = B) and it passes those test cases when for example - opening the file on lower on 'A' for writing - opening the file on overlay layer on 'B' for reading OR - opening the file on overlay layer on 'A' (= lower layer of 'B') for writing - opening the file on overlay layer on 'B' for reading After causing a copy-up only the following test case causes a violation to be logged: - opening the file on upper on 'B' for writing - opening the file on overlay layer on 'B' for reading No violation will the be logged for example for: - opening the file on overlay layer on 'A' (= lower of 'B') for writing - opening the file on overlay layer on 'B' for reading > As Miklos is the designer of overlayfs and its vfs architecture, I was hoping that this would be sufficiently generic to work with potential future stacked filesystems as well that would need to also provide support for D_REAL_FILEDATA. > I am deferring the call about adding this interface to Miklos. > > Thanks, > Amir. >
On Fri, 12 Apr 2024 at 21:09, Stefan Berger <stefanb@linux.ibm.com> wrote: > I was hoping that this would be sufficiently generic to work with > potential future stacked filesystems as well that would need to also > provide support for D_REAL_FILEDATA. I also have very bad feelings from IMA digging in the internals of overlayfs. We should strive to get rid of remaining d_real() instances, not add more. On a related note, D_REAL_METADATA was apparently added for IMA (commit 11b3f8ae7081 ("fs: remove the inode argument to ->d_real() method")), but there's no current user. What's up with this? Thanks, Miklos ,
On Mon, 2024-04-15 at 10:09 +0200, Miklos Szeredi wrote: > On Fri, 12 Apr 2024 at 21:09, Stefan Berger <stefanb@linux.ibm.com> wrote: > > > I was hoping that this would be sufficiently generic to work with > > potential future stacked filesystems as well that would need to also > > provide support for D_REAL_FILEDATA. > > I also have very bad feelings from IMA digging in the internals of overlayfs. > > We should strive to get rid of remaining d_real() instances, not add more. > > On a related note, D_REAL_METADATA was apparently added for IMA > (commit 11b3f8ae7081 ("fs: remove the inode argument to ->d_real() > method")), but there's no current user. What's up with this? It's queued in https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/ next- integrity branch and should be linux-next. thanks, Mimi
On Mon, 15 Apr 2024 at 12:47, Mimi Zohar <zohar@linux.ibm.com> wrote: > It's queued in > https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/ next- > integrity branch and should be linux-next. Is there a document about ima/evm vs. overlayfs? What exactly is it trying to achieve and how? Thanks, Miklos
On Mon, 2024-04-15 at 14:57 +0200, Miklos Szeredi wrote: > On Mon, 15 Apr 2024 at 12:47, Mimi Zohar <zohar@linux.ibm.com> wrote: > > It's queued in > > https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/ > > next- > > integrity branch and should be linux-next. > > Is there a document about ima/evm vs. overlayfs? Not yet. > > What exactly is it trying to achieve and how? Although "Changes to the underlying filesystems while part of a mounted overlay filesystem are not allowed.", from an integrity perspective these changes might affect overlay files. So they need to be detected and possibly re-measured, re- appraised, and/or re-audited [1, 2]. Saying, "If the underlying filesystem is changed, the behavior of the overlay is undefined, though it will not result in a crash or deadlock." does not address the concerns about the integrity of the overlay file being accessed. Initially we disabled EVM on overlay, since calculating the overlay EVM HMAC, could result in taking an invalid HMAC value and making it valid. Stefan's EVM patch set addresses this by only copying up the EVM portable & immutable signature [3, 4]. Lastly, if a file is being opened for read, but is already opened for write, or the reverse, the file measurement is meaningless. A violation is audited and the IMA measurement list is invalidated. This patch set similarly addresses the case where the underlying file is already opened for write and the overlay file is being opened for read. [1] Commit 18b44bc5a672 ("ovl: Always reevaluate the file signature for IMA") forced signature re-evaulation on every file access. [2] Commit b836c4d29f27 (ima: detect changes to the backing overlay file) [3] https://lore.kernel.org/linux-integrity/20231219175206.12342-1-zohar@linux.ibm.com/ [4] https://lore.kernel.org/linux-integrity/20240223172513.4049959-1-stefanb@linux.ibm.com/ thanks, Mimi
On Mon, 15 Apr 2024 at 20:35, Mimi Zohar <zohar@linux.ibm.com> wrote: > Although "Changes to the underlying filesystems while part of a mounted overlay > filesystem are not allowed.", from an integrity perspective these changes might > affect overlay files. So they need to be detected and possibly re-measured, re- > appraised, and/or re-audited [1, 2]. How are changes of non-overlay files detected? Thanks, Miklos
On Tue, 2024-04-16 at 10:05 +0200, Miklos Szeredi wrote: > On Mon, 15 Apr 2024 at 20:35, Mimi Zohar <zohar@linux.ibm.com> wrote: > > > Although "Changes to the underlying filesystems while part of a mounted overlay > > filesystem are not allowed.", from an integrity perspective these changes might > > affect overlay files. So they need to be detected and possibly re-measured, re- > > appraised, and/or re-audited [1, 2]. > > How are changes of non-overlay files detected? Originally there was a single measureent unless the filesystem was mounted with SB_I_VERSION. With commit a2a2c3c8580a ("ima: Use i_version only when filesystem supports it") this changed to always re-measure the file if the filesystem wasn't mounted with SB_I_VERSION. With commit db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version") it's not directly accessing i_version. thanks, Mimi
On Tue, 16 Apr 2024 at 14:18, Mimi Zohar <zohar@linux.ibm.com> wrote: > Originally there was a single measureent unless the filesystem was mounted with > SB_I_VERSION. With commit a2a2c3c8580a ("ima: Use i_version only when > filesystem supports it") this changed to always re-measure the file if the > filesystem wasn't mounted with SB_I_VERSION. Does the i_version get stored and compared only while the inode is in memory? In that case I think it should be possible to support a version number for the overlay inode. Thanks, Miklos
On Tue, 2024-04-16 at 16:46 +0200, Miklos Szeredi wrote: > On Tue, 16 Apr 2024 at 14:18, Mimi Zohar <zohar@linux.ibm.com> wrote: > > Originally there was a single measureent unless the filesystem was mounted with > > SB_I_VERSION. With commit a2a2c3c8580a ("ima: Use i_version only when > > filesystem supports it") this changed to always re-measure the file if the > > filesystem wasn't mounted with SB_I_VERSION. > > Does the i_version get stored and compared only while the inode is in memory? > > In that case I think it should be possible to support a version number > for the overlay inode. i_version was insufficient to detect a file change for overlay. Commit b836c4d29f27 ("ima: detect changes to the backing overlay") also compares the i_ino and s_dev as well. Refer to https://lore.kernel.org/lkml/20231025143906.133218-1-zohar@linux.ibm.com/. Here in this patch set we need to detect IMA read/write violations, based on the i_readcount/i_writecount. If an overlay file is opened for read, but the backing file is already opened for write, the file measurement is meaningless. An "open-writers" violation needs to be generated; and the IMA measurement list needs to be invalidated. thanks, Mimi
On Tue, 16 Apr 2024 at 21:06, Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Tue, 2024-04-16 at 16:46 +0200, Miklos Szeredi wrote: > > On Tue, 16 Apr 2024 at 14:18, Mimi Zohar <zohar@linux.ibm.com> wrote: > > > Originally there was a single measureent unless the filesystem was mounted with > > > SB_I_VERSION. With commit a2a2c3c8580a ("ima: Use i_version only when > > > filesystem supports it") this changed to always re-measure the file if the > > > filesystem wasn't mounted with SB_I_VERSION. > > > > Does the i_version get stored and compared only while the inode is in memory? > > > > In that case I think it should be possible to support a version number > > for the overlay inode. > > i_version was insufficient to detect a file change for overlay. Commit > b836c4d29f27 ("ima: detect changes to the backing overlay") also compares the > i_ino and s_dev as well. Refer to > https://lore.kernel.org/lkml/20231025143906.133218-1-zohar@linux.ibm.com/. Which is rather ad-hoc. I'm talking about returning something in overlay i_version, which really indicates the version of the overlay file calculated from the i_version of the underlying files. The only issue is making this i_version persistent, AFAICS. If that's not needed than the overlayfs specific logic in IMA could be moved into overlayfs, where it belongs. > Here in this patch set we need to detect IMA read/write violations, based on the > i_readcount/i_writecount. If an overlay file is opened for read, but the > backing file is already opened for write, the file measurement is > meaningless. An "open-writers" violation needs to be generated; and the IMA > measurement list needs to be invalidated. If there's no other way, then let's implement an API to query the writecount that can take overlayfs into account. This is for the VFS and/or overlayfs to calculate, not for IMA. Thanks, Miklos
On Tue, 2024-04-23 at 13:06 +0200, Miklos Szeredi wrote: > On Tue, 16 Apr 2024 at 21:06, Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Tue, 2024-04-16 at 16:46 +0200, Miklos Szeredi wrote: > > > On Tue, 16 Apr 2024 at 14:18, Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > Originally there was a single measureent unless the filesystem was mounted with > > > > SB_I_VERSION. With commit a2a2c3c8580a ("ima: Use i_version only when > > > > filesystem supports it") this changed to always re-measure the file if the > > > > filesystem wasn't mounted with SB_I_VERSION. > > > > > > Does the i_version get stored and compared only while the inode is in memory? > > > > > > In that case I think it should be possible to support a version number > > > for the overlay inode. > > > > i_version was insufficient to detect a file change for overlay. Commit > > b836c4d29f27 ("ima: detect changes to the backing overlay") also compares the > > i_ino and s_dev as well. Refer to > > https://lore.kernel.org/lkml/20231025143906.133218-1-zohar@linux.ibm.com/. > > Which is rather ad-hoc. > > I'm talking about returning something in overlay i_version, which > really indicates the version of the overlay file calculated from the > i_version of the underlying files. The only issue is making this > i_version persistent, AFAICS. If that's not needed than the overlayfs > specific logic in IMA could be moved into overlayfs, where it belongs. IMA saves the i_version in order to detect whether or not the file has changed. between one access and another. The i_version value, itself, does not need to be persistent but needs to be consistent. > > > Here in this patch set we need to detect IMA read/write violations, based on the > > i_readcount/i_writecount. If an overlay file is opened for read, but the > > backing file is already opened for write, the file measurement is > > meaningless. An "open-writers" violation needs to be generated; and the IMA > > measurement list needs to be invalidated. > > If there's no other way, then let's implement an API to query the > writecount that can take overlayfs into account. This is for the VFS > and/or overlayfs to calculate, not for IMA. Thanks, that will definitely simplify IMA. Mimi
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index f04f43af651c..590dd9d5d99a 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -121,8 +121,11 @@ static void ima_rdwr_violation_check(struct file *file, const char **pathname, char *filename) { + struct inode *real_inode = d_real_inode(file_dentry(file)); struct inode *inode = file_inode(file); + struct dentry *fd_dentry, *d; fmode_t mode = file->f_mode; + struct inode *fd_inode; bool send_tomtou = false, send_writers = false; if (mode & FMODE_WRITE) { @@ -134,11 +137,25 @@ static void ima_rdwr_violation_check(struct file *file, &iint->atomic_flags)) send_tomtou = true; } - } else { - if (must_measure) - set_bit(IMA_MUST_MEASURE, &iint->atomic_flags); - if (inode_is_open_for_write(inode) && must_measure) - send_writers = true; + } else if (must_measure) { + set_bit(IMA_MUST_MEASURE, &iint->atomic_flags); + + if (inode == real_inode) { + if (inode_is_open_for_write(inode)) + send_writers = true; + } else { + d = d_real(file_dentry(file), D_REAL_FILEDATA); + do { + fd_dentry = d; + fd_inode = d_inode(fd_dentry); + if (inode_is_open_for_write(fd_inode)) { + send_writers = true; + break; + } + /* next layer of stacked fs */ + d = d_real(fd_dentry, D_REAL_FILEDATA); + } while (d != fd_dentry); + } } if (!send_tomtou && !send_writers)
On a stacked filesystem, when one process opens the file holding a file's data (e.g., on upper or lower layer on overlayfs) then issue a violation when another process opens the file for reading on the top layer (overlay layer on overlayfs). This then provides similar behavior to the existing case where a violation is generated when one process opens a file for writing and another one opens the same file for reading. On stacked filesystem also search all the lower layers for relevant files opened for writing and issue the violation if one is found. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- security/integrity/ima/ima_main.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)