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

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

[Darrick J. Wong]

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 it will.  Including 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 does not solve the
general problem of in-kernel fs drivers being a huge attack surface.
I just want a vacation from the shitstorm of bad ideas and threat
models that I never agreed to support.

[Does this actually stop udisks?  I turned off all automounting at the
DE level years ago because I'm not that stupid.]

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 configure.ac           |    1 +
 include/builddefs.in   |    2 ++
 m4/package_services.m4 |   42 ++++++++++++++++++++++++++++++++++++++++++
 scrub/64-xfs.rules     |   10 ++++++++++
 scrub/Makefile         |    9 +++++++++
 5 files changed, 64 insertions(+)
 create mode 100644 scrub/64-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/64-xfs.rules b/scrub/64-xfs.rules
new file mode 100644
index 00000000000..39f17850097
--- /dev/null
+++ b/scrub/64-xfs.rules
@@ -0,0 +1,10 @@
+# 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.
+SUBSYSTEM=="block", ENV{ID_FS_TYPE}=="xfs", ENV{UDISKS_AUTO}="0"
diff --git a/scrub/Makefile b/scrub/Makefile
index ab9c2d14832..74193ed270b 100644
--- a/scrub/Makefile
+++ b/scrub/Makefile
@@ -41,6 +41,11 @@ endif
 
 endif	# scrub_prereqs
 
+UDEV_RULES = 64-xfs.rules
+ifeq ($(HAVE_UDEV),yes)
+	INSTALL_SCRUB += install-udev
+endif
+
 HFILES = \
 common.h \
 counter.h \
@@ -180,6 +185,10 @@ 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)
+	$(INSTALL) -m 644 $(UDEV_RULES) $(UDEV_RULE_DIR)
+
 install-dev:
 
 -include .dep

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

[Dave Chinner]

On Wed, Aug 23, 2023 at 03:36:30PM -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 it will.  Including mangled
> XFS filesystems!

*nod*

> <delete angry rant about poor decisionmaking and armchair fs developers
> blasting us on X while not actually doing any of the work>

If only I had a dollar for every time I've deleted a similar rant...

> 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 does not solve the
> general problem of in-kernel fs drivers being a huge attack surface.
> I just want a vacation from the shitstorm of bad ideas and threat
> models that I never agreed to support.

Yup, this seems like a right thing to do given the lack of action
from the userspace side of the fence.

[ The argument that "prompting the user to ask if they trust the
device teaches them to ignore security prompts" is just stupid. We
have persistent identifiers in filesystems - keep a database of
known trusted identifiers and only prompt for "is this a trusted
device" when an unknown device is inserted. Other desktop OS's have
been doing this for years.... ]

> [Does this actually stop udisks?  I turned off all automounting at the
> DE level years ago because I'm not that stupid.]

Yeah, I turned off all the DE level automount stuff years ago, too.
It's the first thing I do when setting up a new machine for anyone,
too.

.....

> diff --git a/scrub/64-xfs.rules b/scrub/64-xfs.rules
> new file mode 100644
> index 00000000000..39f17850097
> --- /dev/null
> +++ b/scrub/64-xfs.rules
> @@ -0,0 +1,10 @@
> +# 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.
> +SUBSYSTEM=="block", ENV{ID_FS_TYPE}=="xfs", ENV{UDISKS_AUTO}="0"

I think this is correct, but the lack of documentation on how
udev rules and overrides are supposed to work doesn't help
me one bit.

Ok, just tracked it through - only gvfs and clevis actually look at
udisks_block_get_hint_auto() from udisks, so at least the gnome and
lxde/lxqt desktop environments will no longer automount XFS
filesystems.  Who knows what magic is needed for other DEs, but this
is a good start.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

On Thu, Aug 24, 2023 at 09:49:41AM +1000, Dave Chinner wrote:
> On Wed, Aug 23, 2023 at 03:36:30PM -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 it will.  Including mangled
> > XFS filesystems!
> 
> *nod*
> 
> > <delete angry rant about poor decisionmaking and armchair fs developers
> > blasting us on X while not actually doing any of the work>
> 
> If only I had a dollar for every time I've deleted a similar rant...

I do, and I'm raking in the Benjamins!
https://www.youtube.com/watch?v=qpMvS1Q1sos

> > 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 does not solve the
> > general problem of in-kernel fs drivers being a huge attack surface.
> > I just want a vacation from the shitstorm of bad ideas and threat
> > models that I never agreed to support.
> 
> Yup, this seems like a right thing to do given the lack of action
> from the userspace side of the fence.
> 
> [ The argument that "prompting the user to ask if they trust the
> device teaches them to ignore security prompts" is just stupid. We
> have persistent identifiers in filesystems - keep a database of
> known trusted identifiers and only prompt for "is this a trusted
> device" when an unknown device is inserted. Other desktop OS's have
> been doing this for years.... ]

Not to mention that having a prompt at least stops the evil maid from
waking up your laptop, plugging in a usb stick, and being able to mount
crap in your system without even having to unlock it.

But. I'm preaching to the choir here.

> > [Does this actually stop udisks?  I turned off all automounting at the
> > DE level years ago because I'm not that stupid.]
> 
> Yeah, I turned off all the DE level automount stuff years ago, too.
> It's the first thing I do when setting up a new machine for anyone,
> too.
> 
> .....
> 
> > diff --git a/scrub/64-xfs.rules b/scrub/64-xfs.rules
> > new file mode 100644
> > index 00000000000..39f17850097
> > --- /dev/null
> > +++ b/scrub/64-xfs.rules
> > @@ -0,0 +1,10 @@
> > +# 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.
> > +SUBSYSTEM=="block", ENV{ID_FS_TYPE}=="xfs", ENV{UDISKS_AUTO}="0"
> 
> I think this is correct, but the lack of documentation on how
> udev rules and overrides are supposed to work doesn't help
> me one bit.
> 
> Ok, just tracked it through - only gvfs and clevis actually look at
> udisks_block_get_hint_auto() from udisks, so at least the gnome and
> lxde/lxqt desktop environments will no longer automount XFS
> filesystems.  Who knows what magic is needed for other DEs, but this
> is a good start.

Hah, we could encode that too?

kde5:
[General]
AutomountEnabled=false
AutomountOnPlugin=false
AutomountOnLogin=false
AutomountUnknownDevices=false

gnome3:
[org.gnome.desktop.media-handling]
automount=false
automount-open=false
autorun-never=true

gnome2:
/apps/nautilus/preferences/media_automount	false
/apps/nautilus/preferences/media_autorun_never	true
/apps/nautilus/preferences/media_autorun_never	true
/apps/nautilus/preferences/media_autorun_x_content_open_folder	[]


> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thanks!

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com