/snapd

cmd: add helpers for mounting / unmounting #2827

Merged
merged 9 commits into from Mar 1, 2017
Commits
Jump to file or symbol
Failed to load files and symbols.
+8 −0
Split
Viewing a subset of changes. View all
Prev Next

cmd/libsnap: constrain mount debug to -debug builds 

In the non-debug build of snap-confine (the one that is setuid-root) we
will only print mount/umount debug data when we're about to die and
privileges are dropped.

In the -debug build we generate such messages (with all the added
complexity) depending on the runtime switch via the environment variable
(SNAP_CONFINE_DEBUG=yes)

The -debug build is not currently installed but we can eventually
install it (without any security elevation mechanism) and come up with a
way to use it (e.g. via a bind mount).

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
  • Loading branch information...
commit 9e24bee9f0cb94aef73c23667fd80364db66d3bb @zyga zyga committed Feb 23, 2017
View
8 cmd/libsnap-confine-private/mount-opt.c
@@ -261,7 +261,11 @@ void sc_do_mount(const char *source, const char *target,
}
if (sc_is_debug_enabled()) {
@jdstrand

jdstrand Feb 14, 2017

Contributor

Note that sc_is_debug_enabled() is still only looking at SNAP_CONFINE_DEBUG, so an attacker can set that and have the whole string handling attack surface. I thought there were plans to make this compile time?

@zyga

zyga Feb 14, 2017 •
Edited 1 time

Contributor

I must have misunderstood then. I didn't think we agreed to compile-time only debug. If this is the case let's discuss this further because I would find this much more complicated and less useful for actual analysis.

I am also somewhat sceptical about not using this when we are about to die. This is exactly the sort of message that we will get in bug reports. Having no useful data in there will just make everything harder to analyse. What is the reason you would not like to format that when we are about to die?

@jdstrand

jdstrand Feb 14, 2017

Contributor

The reason is because this is an ever-growing setuid executable and I'm trying to keep the attack surface as small as possible. sc_mount_cmd() contains a lot of string handling and I'd like to err on the side of extreme caution.
+#ifdef SNAP_CONFINE_DEBUG_BUILD
ensure_mount_cmd();
+#else
+ mount_cmd = "(disabled) use debug build to see details";
+#endif
debug("performing operation: %s", mount_cmd);
}
@jdstrand

jdstrand Feb 21, 2017

Contributor

You didn't drop privs here. Perhaps drop this stanza?

@zyga

zyga Feb 23, 2017

Contributor

Done now, this is constrained to a -debug build.
if (sc_faulty("mount", NULL)
@@ -295,7 +299,11 @@ void sc_do_umount(const char *target, int flags)
}
if (sc_is_debug_enabled()) {
+#ifdef SNAP_CONFINE_DEBUG_BUILD
ensure_umount_cmd();
+#else
+ umount_cmd = "(disabled) use debug build to see details";
+#endif
debug("performing operation: %s", umount_cmd);
}
if (sc_faulty("umount", NULL) || umount2(target, flags) < 0) {