Re: [PATCH v2] xfsprogs: don't allow udisks to automount XFS filesystems with no prompt

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Aug 25, 2023 at 02:27:09PM +0200, Carlos Maiolino wrote:
> On Thu, Aug 24, 2023 at 05:00:55PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The unending stream of syzbot bug reports and overwrought filing of CVEs
> > for corner case handling (i.e. things that distract from actual user
> > complaints) in XFS has generated all sorts of of overheated rhetoric
> > about how every bug is a Serious Security Issue(tm) because anyone can
> > craft a malicious filesystem on a USB stick, insert the stick into a
> > victim machine, and mount will trigger a bug in the kernel driver that
> > leads to some compromise or DoS or something.
> > 
> > I thought that nobody would be foolish enough to automount an XFS
> > filesystem.  What a fool I was!  It turns out that udisks can be told
> > that it's okay to automount things, and then GNOME will do exactly that.
> > Including mounting mangled XFS filesystems!
> > 
> > <delete angry rant about poor decisionmaking and armchair fs developers
> > blasting us on X while not actually doing any of the work>
> > 
> > Turn off /this/ idiocy by adding a udev rule to tell udisks not to
> > automount XFS filesystems.
> > 
> > This will not stop a logged in user from unwittingly inserting a
> > malicious storage device and pressing [mount] and getting breached.
> > This is not a substitute for a thorough audit.  This is not a substitute
> > for lklfuse.  This does not solve the general problem of in-kernel fs
> > drivers being a huge attack surface.  I just want a vacation from the
> > sh*tstorm of bad ideas and threat models that I never agreed to support.
> > 
> 
> This seems great, thanks for doing it.
> 
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> 
> /me wonders how long until distros/users start to complain and report problems
> their OS'es are not automounting xfs :)   /me runs

Well you don't have to include the make install steps in the fedora/rhel
xfsprogs.spec files. :)

Judging from the udisks git repo it sounds like they've mostly neutered
the automount hint to a subset of "commonly" removable devices like usb
and firewire.

--D

> 
> Carlos
> 
> > v2: Add external logs to the list too, and document the var we set
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  configure.ac           |    1 +
> >  include/builddefs.in   |    2 ++
> >  m4/package_services.m4 |   42 ++++++++++++++++++++++++++++++++++++++++++
> >  scrub/Makefile         |   11 +++++++++++
> >  scrub/xfs.rules        |   13 +++++++++++++
> >  5 files changed, 69 insertions(+)
> >  create mode 100644 scrub/xfs.rules
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 58f3b8e2e90..e447121a344 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -209,6 +209,7 @@ AC_HAVE_SG_IO
> >  AC_HAVE_HDIO_GETGEO
> >  AC_CONFIG_SYSTEMD_SYSTEM_UNIT_DIR
> >  AC_CONFIG_CROND_DIR
> > +AC_CONFIG_UDEV_RULE_DIR
> > 
> >  if test "$enable_blkid" = yes; then
> >  AC_HAVE_BLKID_TOPO
> > diff --git a/include/builddefs.in b/include/builddefs.in
> > index fb8e239cab2..3318e00316c 100644
> > --- a/include/builddefs.in
> > +++ b/include/builddefs.in
> > @@ -184,6 +184,8 @@ HAVE_SYSTEMD = @have_systemd@
> >  SYSTEMD_SYSTEM_UNIT_DIR = @systemd_system_unit_dir@
> >  HAVE_CROND = @have_crond@
> >  CROND_DIR = @crond_dir@
> > +HAVE_UDEV = @have_udev@
> > +UDEV_RULE_DIR = @udev_rule_dir@
> >  HAVE_LIBURCU_ATOMIC64 = @have_liburcu_atomic64@
> >  HAVE_MEMFD_CLOEXEC = @have_memfd_cloexec@
> >  HAVE_MEMFD_NOEXEC_SEAL = @have_memfd_noexec_seal@
> > diff --git a/m4/package_services.m4 b/m4/package_services.m4
> > index f2d888a099a..a683ddb93e0 100644
> > --- a/m4/package_services.m4
> > +++ b/m4/package_services.m4
> > @@ -75,3 +75,45 @@ AC_DEFUN([AC_CONFIG_CROND_DIR],
> >  	AC_SUBST(have_crond)
> >  	AC_SUBST(crond_dir)
> >  ])
> > +
> > +#
> > +# Figure out where to put udev rule files
> > +#
> > +AC_DEFUN([AC_CONFIG_UDEV_RULE_DIR],
> > +[
> > +	AC_REQUIRE([PKG_PROG_PKG_CONFIG])
> > +	AC_ARG_WITH([udev_rule_dir],
> > +	  [AS_HELP_STRING([--with-udev-rule-dir@<:@=DIR@:>@],
> > +		[Install udev rules into DIR.])],
> > +	  [],
> > +	  [with_udev_rule_dir=yes])
> > +	AS_IF([test "x${with_udev_rule_dir}" != "xno"],
> > +	  [
> > +		AS_IF([test "x${with_udev_rule_dir}" = "xyes"],
> > +		  [
> > +			PKG_CHECK_MODULES([udev], [udev],
> > +			  [
> > +				with_udev_rule_dir="$($PKG_CONFIG --variable=udev_dir udev)/rules.d"
> > +			  ], [
> > +				with_udev_rule_dir=""
> > +			  ])
> > +			m4_pattern_allow([^PKG_(MAJOR|MINOR|BUILD|REVISION)$])
> > +		  ])
> > +		AC_MSG_CHECKING([for udev rule dir])
> > +		udev_rule_dir="${with_udev_rule_dir}"
> > +		AS_IF([test -n "${udev_rule_dir}"],
> > +		  [
> > +			AC_MSG_RESULT(${udev_rule_dir})
> > +			have_udev="yes"
> > +		  ],
> > +		  [
> > +			AC_MSG_RESULT(no)
> > +			have_udev="no"
> > +		  ])
> > +	  ],
> > +	  [
> > +		have_udev="disabled"
> > +	  ])
> > +	AC_SUBST(have_udev)
> > +	AC_SUBST(udev_rule_dir)
> > +])
> > diff --git a/scrub/Makefile b/scrub/Makefile
> > index ab9c2d14832..2b9b8d977f6 100644
> > --- a/scrub/Makefile
> > +++ b/scrub/Makefile
> > @@ -41,6 +41,11 @@ endif
> > 
> >  endif	# scrub_prereqs
> > 
> > +UDEV_RULES = xfs.rules
> > +ifeq ($(HAVE_UDEV),yes)
> > +	INSTALL_SCRUB += install-udev
> > +endif
> > +
> >  HFILES = \
> >  common.h \
> >  counter.h \
> > @@ -180,6 +185,12 @@ install-scrub: default
> >  	$(INSTALL) -m 755 $(XFS_SCRUB_ALL_PROG) $(PKG_SBIN_DIR)
> >  	$(INSTALL) -m 755 -d $(PKG_STATE_DIR)
> > 
> > +install-udev: $(UDEV_RULES)
> > +	$(INSTALL) -m 755 -d $(UDEV_RULE_DIR)
> > +	for i in $(UDEV_RULES); do \
> > +		$(INSTALL) -m 644 $$i $(UDEV_RULE_DIR)/64-$$i; \
> > +	done
> > +
> >  install-dev:
> > 
> >  -include .dep
> > diff --git a/scrub/xfs.rules b/scrub/xfs.rules
> > new file mode 100644
> > index 00000000000..c3f69b3ab90
> > --- /dev/null
> > +++ b/scrub/xfs.rules
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +#
> > +# Copyright (C) 2023 Oracle.  All rights reserved.
> > +# Author: Darrick J. Wong <djwong@kernel.org>
> > +#
> > +# Don't let udisks automount XFS filesystems without even asking a user.
> > +# This doesn't eliminate filesystems as an attack surface; it only prevents
> > +# evil maid attacks when all sessions are locked.
> > +#
> > +# According to http://storaged.org/doc/udisks2-api/latest/udisks.8.html,
> > +# supplying UDISKS_AUTO=0 here changes the HintAuto property of the block
> > +# device abstraction to mean "do not automatically start" (e.g. mount).
> > +SUBSYSTEM=="block", ENV{ID_FS_TYPE}=="xfs|xfs_external_log", ENV{UDISKS_AUTO}="0"