| Message ID | 20221103135927.13656-1-matthias.gerstner@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/kvm_stat: fix attack vector with user controlled FUSE mounts | expand |
On 11/3/22 14:59, Matthias Gerstner wrote: > The first field in /proc/mounts can be influenced by unprivileged users > through the widespread `fusermount` setuid-root program. Example: > > ``` > user$ mkdir ~/mydebugfs > user$ export _FUSE_COMMFD=0 > user$ fusermount ~/mydebugfs -ononempty,fsname=debugfs > user$ grep debugfs /proc/mounts > debugfs /home/user/mydebugfs fuse rw,nosuid,nodev,relatime,user_id=1000,group_id=100 0 0 > ``` > > If there is no debugfs already mounted in the system then this can be > used by unprivileged users to trick kvm_stat into using a user > controlled file system location for obtaining KVM statistics. > > To exploit this also a race condition has to be won, since the code > checks for the existence of the 'kvm' subdirectory of the resulting > path. This doesn't work on a FUSE mount, because the root user is not > allowed to access non-root FUSE mounts for security reasons. If an > attacker manages to unmount the FUSE mount in time again then kvm_stat > would be using the resulting path, though. > > The impact if winning the race condition is mostly a denial-of-service > or damaged information integrity. The files in debugfs are only opened > for reading. So the attacker can cause very large data to be read in by > kvm_stat or fake data to be processed by kvm_stat. I don't see any > viable way to turn this into a privilege escalation. Ok, thanks for confirming. I will tweak the commit message. Paolo > The fix is simply to use the file system type field instead. Whitespace > in the mount path is escaped in /proc/mounts thus no further safety > measures in the parsing should be necessary to make this correct. > --- > tools/kvm/kvm_stat/kvm_stat | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat > index 9c366b3a676d..88a73999aa58 100755 > --- a/tools/kvm/kvm_stat/kvm_stat > +++ b/tools/kvm/kvm_stat/kvm_stat > @@ -1756,7 +1756,7 @@ def assign_globals(): > > debugfs = '' > for line in open('/proc/mounts'): > - if line.split(' ')[0] == 'debugfs': > + if line.split(' ')[2] == 'debugfs': > debugfs = line.split(' ')[1] > break > if debugfs == '':
On 11/3/22 14:59, Matthias Gerstner wrote: > The fix is simply to use the file system type field instead. Whitespace > in the mount path is escaped in /proc/mounts thus no further safety > measures in the parsing should be necessary to make this correct. Can you please send a patch to replace seq_printf with seq_escape in afs_show_devname though? I couldn't find anything that prevents cell->name and volume->name from containing a space, so better safe than sorry. Paolo
On Thu, Nov 03, 2022 at 03:21:12PM +0100, Paolo Bonzini wrote: > On 11/3/22 14:59, Matthias Gerstner wrote: > > The fix is simply to use the file system type field instead. Whitespace > > in the mount path is escaped in /proc/mounts thus no further safety > > measures in the parsing should be necessary to make this correct. > > Can you please send a patch to replace seq_printf with seq_escape in > afs_show_devname though? I couldn't find anything that prevents > cell->name and volume->name from containing a space, so better safe than > sorry. I only checked this during runtime using a tmpfs and assumed this would be true for all file systems. Sure I can come up with a patch. Should I send a new single patch containing both changes, a new patch series with two patches or do I need to send the afs change to a different mailing list? Sorry - I'm new to kernel development. Cheers Matthias
On 11/4/22 12:16, Matthias Gerstner wrote: > > Sure I can come up with a patch. Should I send a new single patch > containing both changes, a new patch series with two patches or do I > need to send the afs change to a different mailing list? Sorry - I'm new > to kernel development. Yes, please send the AFS patch to linux-afs@lists.infradead.org. In the meanwhile I will queue this patch as it is already an improvement. Paolo
On 11/3/22 14:59, Matthias Gerstner wrote: > The fix is simply to use the file system type field instead. Whitespace > in the mount path is escaped in /proc/mounts thus no further safety > measures in the parsing should be necessary to make this correct. > --- > tools/kvm/kvm_stat/kvm_stat | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Matthias, both this patch and the one you sent to linux-afs need to include a "Signed-off-by" line, for example: ### ### Signed-off-by: Matthias Gerstner <matthias.gerstner@suse.de> ### The meaning of this is visible at https://developercertificate.org/. For this patch you can just reply to the message with the above line (without the "###" in front) and I'll accept it. However, for linux-afs I suggest that you just resend it. Just committing your patch with the "-s" command line argument will include the line for you. Thanks, Paolo > diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat > index 9c366b3a676d..88a73999aa58 100755 > --- a/tools/kvm/kvm_stat/kvm_stat > +++ b/tools/kvm/kvm_stat/kvm_stat > @@ -1756,7 +1756,7 @@ def assign_globals(): > > debugfs = '' > for line in open('/proc/mounts'): > - if line.split(' ')[0] == 'debugfs': > + if line.split(' ')[2] == 'debugfs': > debugfs = line.split(' ')[1] > break > if debugfs == '':
Signed-off-by: Matthias Gerstner <matthias.gerstner@suse.de> On Sun, Nov 06, 2022 at 09:43:04AM +0100, Paolo Bonzini wrote: > On 11/3/22 14:59, Matthias Gerstner wrote: > > The fix is simply to use the file system type field instead. Whitespace > > in the mount path is escaped in /proc/mounts thus no further safety > > measures in the parsing should be necessary to make this correct. > > --- > > tools/kvm/kvm_stat/kvm_stat | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Matthias, both this patch and the one you sent to linux-afs need to > include a "Signed-off-by" line, for example: > > ### > ### Signed-off-by: Matthias Gerstner <matthias.gerstner@suse.de> > ### > > The meaning of this is visible at https://developercertificate.org/. > > For this patch you can just reply to the message with the above line > (without the "###" in front) and I'll accept it. However, for linux-afs > I suggest that you just resend it. Just committing your patch with the > "-s" command line argument will include the line for you. > > Thanks, > > Paolo > > > diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat > > index 9c366b3a676d..88a73999aa58 100755 > > --- a/tools/kvm/kvm_stat/kvm_stat > > +++ b/tools/kvm/kvm_stat/kvm_stat > > @@ -1756,7 +1756,7 @@ def assign_globals(): > > > > debugfs = '' > > for line in open('/proc/mounts'): > > - if line.split(' ')[0] == 'debugfs': > > + if line.split(' ')[2] == 'debugfs': > > debugfs = line.split(' ')[1] > > break > > if debugfs == '': >
diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat index 9c366b3a676d..88a73999aa58 100755 --- a/tools/kvm/kvm_stat/kvm_stat +++ b/tools/kvm/kvm_stat/kvm_stat @@ -1756,7 +1756,7 @@ def assign_globals(): debugfs = '' for line in open('/proc/mounts'): - if line.split(' ')[0] == 'debugfs': + if line.split(' ')[2] == 'debugfs': debugfs = line.split(' ')[1] break if debugfs == '':