cmd: add mount / unmount helpers

[zyga]

This branch adds sc_do_mount and sc_do_umount that mirror mount/umount library functions with
additional logging and switches the tree over to them. This cuts significant and error prone boilerplate
from the code.

[mvo5]

Do we need the else if here? If the user gives a combination that is not part of this if/else if/... tree (maybe by accident) we won't get accurate logging? Or is this no concern for some reason?

[zyga]

Interesting combination. I was thinking that perhaps apart from logging all the options as the appear (even if they make no sense) we could check if the arguments make sense and die() if they dont.

[mvo5]

Looks good, just one question about the logging

[mvo5]

I'm slightly concerned about the direction we are talking with snap-confine. It is a suid root binary and we want it to be small and easy. Functions like this make me uneasy. It feels like we re-inventing low-level library. Sorry for being so negative but I think we should think about alternatives, maybe just using simpler logging?

[zyga]

When we're processing mount profiles we can see almost arbitrary mounts. I agree this is extra complexity but I think it is at some level unavoidable (and better than a static buffer one can overflow). One alternative would be to remove all logging entirely but I think this hurts debuggability -- often that list of mount operations is very useful when exploring unexpected behavior.

[zyga]

After discussing this on telegram we agreed to try a simpler constant size-buffer.

[jdstrand]

In general, I like the cleanup of sc_do_umount() and sc_do_mount() since it makes the code clearer and cleaner, but I'd like to see the string handling cleaned up, the debug output conditional and the removal of the static variable.

[jdstrand]

We have? Using static here means that if sc_mount_cmd() is called a second time, buf will have whatever contents are already present. Because of how you initialize 'to' to 'buf', this works out but there is no reason I can think of where sc_mount_cmd() would benefit from this behavior and I'd much prefer explicit initialization here. If you follow my debug_mount_command() and debug_umount_command() advise below, you can have these functions initialize the buf to pass into sc_mount_cmd() and sc_umount_cmd() instead.

[zyga]

We == several team members. I implemented this to dynamically allocate memory earlier and that version was rejected.

[jdstrand]

As discussed on IRC, there was a misunderstanding. The team was not advocating a static local variable, they were advocating a statically sized array (ie, a stack variable). While I don't have any issues with properly coded dynamic allocations, since this is cleared up please remove 'static char *buf[BUF_SIZE]' and simply use 'char buf[BUF_SIZE]' as described.

[jdstrand]

Where did you come up with this calculation?

[zyga]

"two paths + some extra space". Believe me I much preferred the version that had a dynamically allocated buffer.

[jdstrand]

You have to be careful with stpcpy() in that dst must be large enough to add src + NULL. As a library function, I think we need to be checking to make 100% sure that 'to' is always large enough to add 'src'. Perhaps you want to write a helper function similar to must_snprintf that does this for you.

This might provide inspiration for you:

#define BUF_SIZE PATH_MAX*2+1000
...
char* sc_append_string(char *dst, const char *src, size_t size)
{
        int n;

        char *tmp = strdup(dst);
        if (tmp == NULL)
                die("could not strdup");

        n = snprintf(dst, size, "%s%s", tmp, src);
        free(tmp);
        if (n < 0 || n >= size)
                die("failed to snprintf %s", src);

        return dst;
}
...
    char buf[BUF_SIZE] = {0};
    char *to = buf;
    to = sc_append_string(to, ..., BUF_SIZE);

Edit: in the original comment, the above function was (at the last minute) mistakenly named to must_strncpy(). I've edited it to reflect the intended feedback.

[jdstrand]

Same here.

[jdstrand]

I don't care for how we have sc_mount_cmd() and sc_umount_cmd() unconditionally doing all kinds of string operations only for debug output that may not be displayed. IMO, it would be better to create a separate debug_mount() and debug_umount() and have them exit if SNAP_CONFINE_DEBUG is not set and only conditionally compiled with build flags (same for debug() for that matter).

[zyga]

Just to get this clear, would you like to have a special debug build of snap-confine and have debug() do nothing unless in that special build?

[jdstrand]

That is my preference. While putting it behind an env is convenient, an attacker attacking snap-confine and trying to escalate privileges will simply set that variable to get at this part of the codebase.

[jdstrand]

Note, before you did '!= 0' where here you are doing '< 0'. Per man mount, this change is fine as mount will only return -1 or 0.

[zyga]

I think this is in line with our earlier plan to try to use < 0 everywhere.

[jdstrand]

That's fine, just always check the man pages! ;)

[zyga]

I'm closing this for the rework that @jdstrand requested. I'll reopen it shortly.