Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd: add helpers for mounting / unmounting #2827
Conversation
zyga
added some commits
Jan 18, 2017
All checks have passed
jdstrand
requested changes
Feb 14, 2017
The code looks ok but I thought we agreed that we were working towards only going through all the string handling debug message code only when sc_is_debug_enabled() (and that in and of itself would become a compile time option). I'd prefer that the calls to ensure_mount_cmd() only be used with debug() and not die() (indeed, if we are only showing them with debugging enabled, the output from die() becomes redundant).
| + } | ||
| + } | ||
| + | ||
| + if (sc_is_debug_enabled()) { |
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
Feb 14, 2017
•
Edited 1 time
-
zyga
Feb 14, 2017
Contributor
-
Feb 14, 2017
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
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.
| + || mount(source, target, fs_type, mountflags, data) < 0) { | ||
| + // Save errno as ensure can clobber it. | ||
| + int saved_errno = errno; | ||
| + ensure_mount_cmd(); |
jdstrand
Feb 14, 2017
Contributor
This means that a failed mount goes through the whole string handling machinery of sc_mount_cmd() which I thought we agreed we would only do in debug mode, not production mode.
| + if (sc_faulty("umount", NULL) || umount2(target, flags) < 0) { | ||
| + // Save errno as ensure can clobber it. | ||
| + int saved_errno = errno; | ||
| + ensure_umount_cmd(); |
|
I just discussed this with jamie on IRC and the agreement is to use the new debugging messages when debug is enabled (runtime choice) but only die with the new message after permanently dropping privileges. |
|
The privilege separation helpers are on #2852 |
|
The privilege dropping code is now used before constructing a non-debug error message. |
zyga
added some commits
Feb 17, 2017
All checks have failed
All checks have failed
Some checks were not successful
| + if (sc_is_debug_enabled()) { | ||
| + ensure_mount_cmd(); | ||
| + debug("performing operation: %s", mount_cmd); | ||
| + } |
zyga
added some commits
Feb 23, 2017
Some checks were not successful
zyga commentedFeb 9, 2017
This branch adds two helper function for mounting and un-mounting in snap-confine and friends.
The main benefit is that thanks to built-in debugging and error checking mount code can become
much shorter than if it was to be done manually.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com