Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/libsnap-confine-private, cmd/snap-confine: fix logging of failed [u]mount when run with SNAP_CONFINE_DEBUG=1 #4449

Closed
wants to merge 7 commits into from
Closed

cmd/libsnap-confine-private, cmd/snap-confine: fix logging of failed [u]mount when run with SNAP_CONFINE_DEBUG=1 #4449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2a150fc
cmd/libsnap-confine-private: extend sc_do_mount, sc_do_umount tests t…
bboozzoo Jan 8, 2018
276a805
cmd/libsnap-confine-private: print failed mount/umount command when d…
bboozzoo Jan 8, 2018
4ee5ea9
cmd: fix snap-confine-debug, SNAP_CONFINE_DEBUG_BUILD does not propag…
bboozzoo Jan 8, 2018
0248623
cmd: add debug-build variant of libsnap-confine-private.a
bboozzoo Jan 9, 2018
cee2dbd
cmd: use debug-build of libsnap-confine-private for snap-confine-debug
bboozzoo Jan 9, 2018
7020381
cmd/snap-confine: workaround link time dependencies issues on Debian 9
bboozzoo Jan 9, 2018
8f29d35
cmd: cleaner workaround for automake snap-confine-debug DEPENDENCIES
bboozzoo Jan 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 14 additions & 5 deletions cmd/Makefile.am
View file
Open in desktop
Expand Up @@ -102,6 +102,10 @@ libsnap_confine_private_a_SOURCES = \
libsnap-confine-private/utils.h
libsnap_confine_private_a_CFLAGS = $(CHECK_CFLAGS)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ok, thank you for doing that :)

noinst_LIBRARIES += libsnap-confine-private-debug.a
libsnap_confine_private_debug_a_SOURCES = $(libsnap_confine_private_a_SOURCES)
libsnap_confine_private_debug_a_CFLAGS = $(CHECK_CFLAGS) -DSNAP_CONFINE_DEBUG_BUILD=1

if WITH_UNIT_TESTS
noinst_PROGRAMS += libsnap-confine-private/unit-tests
libsnap_confine_private_unit_tests_SOURCES = \
Expand Down Expand Up @@ -205,14 +209,19 @@ snap_confine_snap_confine_CFLAGS = $(CHECK_CFLAGS) $(AM_CFLAGS) -DLIBEXECDIR=\"$
snap_confine_snap_confine_LDFLAGS = $(AM_LDFLAGS)
snap_confine_snap_confine_LDADD = libsnap-confine-private.a
snap_confine_snap_confine_CFLAGS += $(LIBUDEV_CFLAGS)
snap_confine_snap_confine_LDADD += $(LIBUDEV_LIBS)
snap_confine_snap_confine_LDADD += $(snap_confine_snap_confine_extra_libs)
# _STATIC is where we collect statically linked in libraries
snap_confine_snap_confine_STATIC =
# use a separate variable instead of snap_confine_snap_confine_LDADD to collect
# all external libraries, this way it can be reused in
# snap_confine_snap_confine_debug_LDADD withouth applying any text
# transformations
snap_confine_snap_confine_extra_libs = $(LIBUDEV_LIBS)

if STATIC_LIBCAP
snap_confine_snap_confine_STATIC += -lcap
else
snap_confine_snap_confine_LDADD += -lcap
snap_confine_snap_confine_extra_libs += -lcap
endif # STATIC_LIBCAP

# Use a hacked rule if we're doing static build. This allows us to inject the LIBS += .. rule below.
Expand All @@ -235,7 +244,7 @@ snap_confine_snap_confine_CFLAGS += $(SECCOMP_CFLAGS)
if STATIC_LIBSECCOMP
snap_confine_snap_confine_STATIC += $(shell pkg-config --static --libs libseccomp)
else
snap_confine_snap_confine_LDADD += $(SECCOMP_LIBS)
snap_confine_snap_confine_extra_libs += $(SECCOMP_LIBS)
endif # STATIC_LIBSECCOMP
endif # SECCOMP

Expand All @@ -244,7 +253,7 @@ snap_confine_snap_confine_CFLAGS += $(APPARMOR_CFLAGS)
if STATIC_LIBAPPARMOR
snap_confine_snap_confine_STATIC += $(shell pkg-config --static --libs libapparmor)
else
snap_confine_snap_confine_LDADD += $(APPARMOR_LIBS)
snap_confine_snap_confine_extra_libs += $(APPARMOR_LIBS)
endif # STATIC_LIBAPPARMOR
endif # APPARMOR

Expand All @@ -254,7 +263,7 @@ noinst_PROGRAMS += snap-confine/snap-confine-debug
snap_confine_snap_confine_debug_SOURCES = $(snap_confine_snap_confine_SOURCES)
snap_confine_snap_confine_debug_CFLAGS = $(snap_confine_snap_confine_CFLAGS)
snap_confine_snap_confine_debug_LDFLAGS = $(snap_confine_snap_confine_LDFLAGS)
snap_confine_snap_confine_debug_LDADD = $(snap_confine_snap_confine_LDADD)
snap_confine_snap_confine_debug_LDADD = libsnap-confine-private-debug.a $(snap_confine_snap_confine_extra_libs)
snap_confine_snap_confine_debug_CFLAGS += -DSNAP_CONFINE_DEBUG_BUILD=1
snap_confine_snap_confine_debug_STATIC = $(snap_confine_snap_confine_STATIC)

Expand Down
40 changes: 32 additions & 8 deletions cmd/libsnap-confine-private/mount-opt-test.c
View file
Open in desktop
Expand Up @@ -211,10 +211,13 @@ static bool broken_mount(struct sc_fault_state *state, void *ptr)
return true;
}

static void test_sc_do_mount(void)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please spin this off into a separate PR and discuss with @jdstrand when he's back in two weeks?

static void test_sc_do_mount(gconstpointer snap_debug)
{
if (g_test_subprocess()) {
sc_break("mount", broken_mount);
if (GPOINTER_TO_INT(snap_debug) == 1) {
g_setenv("SNAP_CONFINE_DEBUG", "1", true);
}
sc_do_mount("/foo", "/bar", "ext4", MS_RDONLY, NULL);

g_test_message("expected sc_do_mount not to return");
Expand All @@ -224,8 +227,14 @@ static void test_sc_do_mount(void)
}
g_test_trap_subprocess(NULL, 0, 0);
g_test_trap_assert_failed();
g_test_trap_assert_stderr
("cannot perform operation: mount -t ext4 -o ro /foo /bar: Permission denied\n");
if (GPOINTER_TO_INT(snap_debug) == 0) {
g_test_trap_assert_stderr
("cannot perform operation: mount -t ext4 -o ro /foo /bar: Permission denied\n");
} else {
g_test_trap_assert_stderr
("DEBUG: performing operation: (disabled) use debug build to see details\n"
"cannot perform operation: mount -t ext4 -o ro /foo /bar: Permission denied\n");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pardon my ignorance - but why do I see the real operation here still? Isn't the point of the non-debug build to hide this? Maybe a comment here that explains what is going on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect it to be visible regardless of debug build and SNAP_CONFINE_DEBUG setting as it's useful for figuring out what failed exactly. Previously, in non debug builds, you'd only see the error to contain mount .. if you ran without SNAP_CONFINE_DEBUG. When running with SNAP_CONFINE_DEBUG, you'd get (disabled) use debug build to see details instead.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was meant as a security precaution against running diagnostic code in production builds. If that code contains any errors it could be used to attack a setuid-root program.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably a bit slow and/or missing something - but if we show the real operation, why do we show the "diabled use debug build" message at all?

}
}

static bool broken_umount(struct sc_fault_state *state, void *ptr)
Expand All @@ -234,10 +243,13 @@ static bool broken_umount(struct sc_fault_state *state, void *ptr)
return true;
}

static void test_sc_do_umount(void)
static void test_sc_do_umount(gconstpointer snap_debug)
{
if (g_test_subprocess()) {
sc_break("umount", broken_umount);
if (GPOINTER_TO_INT(snap_debug) == 1) {
g_setenv("SNAP_CONFINE_DEBUG", "1", true);
}
sc_do_umount("/foo", MNT_DETACH);

g_test_message("expected sc_do_umount not to return");
Expand All @@ -247,15 +259,27 @@ static void test_sc_do_umount(void)
}
g_test_trap_subprocess(NULL, 0, 0);
g_test_trap_assert_failed();
g_test_trap_assert_stderr
("cannot perform operation: umount --lazy /foo: Permission denied\n");
if (GPOINTER_TO_INT(snap_debug) == 0) {
g_test_trap_assert_stderr
("cannot perform operation: umount --lazy /foo: Permission denied\n");
} else {
g_test_trap_assert_stderr
("DEBUG: performing operation: (disabled) use debug build to see details\n"
"cannot perform operation: umount --lazy /foo: Permission denied\n");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above.

}
}

static void __attribute__ ((constructor)) init(void)
{
g_test_add_func("/mount/sc_mount_opt2str", test_sc_mount_opt2str);
g_test_add_func("/mount/sc_mount_cmd", test_sc_mount_cmd);
g_test_add_func("/mount/sc_umount_cmd", test_sc_umount_cmd);
g_test_add_func("/mount/sc_do_mount", test_sc_do_mount);
g_test_add_func("/mount/sc_do_umount", test_sc_do_umount);
g_test_add_data_func("/mount/sc_do_mount", GINT_TO_POINTER(0),
test_sc_do_mount);
g_test_add_data_func("/mount/sc_do_umount", GINT_TO_POINTER(0),
test_sc_do_umount);
g_test_add_data_func("/mount/sc_do_mount_with_debug",
GINT_TO_POINTER(1), test_sc_do_mount);
g_test_add_data_func("/mount/sc_do_umount_with_debug",
GINT_TO_POINTER(1), test_sc_do_umount);
}
13 changes: 7 additions & 6 deletions cmd/libsnap-confine-private/mount-opt.c
View file
Open in desktop
Expand Up @@ -251,6 +251,9 @@ const char *sc_umount_cmd(char *buf, size_t buf_size, const char *target,
return buf;
}

static const char use_debug_build[] =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use static const char use_debug_build * = "..."

"(disabled) use debug build to see details";

void sc_do_mount(const char *source, const char *target,
const char *fs_type, unsigned long mountflags,
const void *data)
Expand All @@ -262,10 +265,9 @@ void sc_do_mount(const char *source, const char *target,
#ifdef SNAP_CONFINE_DEBUG_BUILD
mount_cmd = sc_mount_cmd(buf, sizeof(buf), source,
target, fs_type, mountflags, data);
#else
mount_cmd = "(disabled) use debug build to see details";
#endif
debug("performing operation: %s", mount_cmd);
debug("performing operation: %s",
mount_cmd ? mount_cmd : use_debug_build);
}
if (sc_faulty("mount", NULL)
|| mount(source, target, fs_type, mountflags, data) < 0) {
Expand Down Expand Up @@ -296,10 +298,9 @@ void sc_do_umount(const char *target, int flags)
if (sc_is_debug_enabled()) {
#ifdef SNAP_CONFINE_DEBUG_BUILD
umount_cmd = sc_umount_cmd(buf, sizeof(buf), target, flags);
#else
umount_cmd = "(disabled) use debug build to see details";
#endif
debug("performing operation: %s", umount_cmd);
debug("performing operation: %s",
umount_cmd ? umount_cmd : use_debug_build);
}
if (sc_faulty("umount", NULL) || umount2(target, flags) < 0) {
// Save errno as ensure can clobber it.
Expand Down