cmd/libsnap: add privilege elevation helpers

[zyga]

This patch adds a small set of functions for working with privilege
elevation. Since snap-confine runs as root via the setuid-root mechanism
we always start with elevated permissions. For added security, througout
the execution of the process, permission are lowered and raised
(temporarily) as appropriate, to limit the extent of the possible damage
should the given code be compromised. Finally snap-confine permanently
lowers permissions before handing off to the next executable.

All this code was in snap-confine.c but now it is easier to reuse
elesewhere. The error messages have been cleaned up for clarity and
readability.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

[jdstrand]

The more I worked through this PR the less I liked how I imagined it would be used. For example, in the code we currently only drop and raise the uid but this PR would move that over to dropping/raising both. It's conceivable we'd want to drop just the gid sometimes. It is also tricky to keep track of the ruid, euid, and suid in a library in the ways that we are in the code when dropping and raising.

I propose instead that since all the current priv dropping code is simple, heavily audited and verified, we don't add all these helpers and instead add a single helper: sc_drop_privs() that drops setgroups(), gid and uid permanently (ie, lines 183-195 of snap-confine.c, but with dropping setgroups() before). Call that anywhere you want to permanently drop. This will allow you to use a library function to drop for the debugging output and string parsing you want to use this in.

[jdstrand]

Let's not use static variables. Instead, put uid_t real_uid and gid_t real_gid in each function that needs them and call getuid()/getgid() or getresuid()/getresgid() as needed. The static variables allow for the possibility that the variables are not updated. Indeed, in this PR they aren't ever updated when raising or dropping.

[jdstrand]

Feel free to drop this (see above).

[jdstrand]

The terminology is typically 'drop' and 'raise'. Please rename to 'sc_drop_privs_permanently()'.

[jdstrand]

This isn't true for the die() scenario. If we are successful all the way through to exec(), then not calling setgroups() is ok, but we want to call setgroups() on error conditions.

I suggest:

• create sc_drop_privs_permanently_no_setgroups() that is this function
• create sc_drop_privs_permanently() that drops via setgroups() then calls sc_drop_privs_permanently_no_setgroups()
• use sc_drop_privs_permanently() everwhere except in snap-confine.c line 182, where you replace the if clause with sc_drop_privs_permanently_no_setgroups()

[jdstrand]

You changed the order. It is supposed to be, setgid(), setuid(), verify everything dropped correctly. With what you have here, you interleaved the verification and added 'lowered'. Please keep this simple. Take what is in snap-confine.c lines 182-195 as is. You don't need 'lowered' because failures should've die()d. Just debug() unconditionally at the end that you lowered.

[jdstrand]

Please rename 'sc_drop_privs_temporarily()'.

[jdstrand]

All the same comments here as for above, except take lines 160-168 of snap-confine.c

[jdstrand]

I prefer 'sc_raise_privs' and 'sc_drop_privs_*', but if you prefer 'sc_privs_raise' and 'sc_privs_drop_*' I won't block.

[jdstrand]

It's a little weird to use 'die("... %d", 0);' instead of just 'die("... 0");'

[jdstrand]

All the same comments here as for above, except take lines 769-777 of seccomp-support.c and add setegid() and remove the 'raised' variable.

[zyga]

I read the inline comments and I just now read the main remark and I think you are spot on. We don't need the elaborate mechanism now, just the simple "drop everything now" call. I'll make this happen in a moment. I'm sorry for taking so long but I wasn't feeling well today and I didn't work for the fist half of the day much.

I'll close and re-open when I have this.