Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
many: detect potentially insecure use of snap-confine #2630
Conversation
zyga
added some commits
Jan 12, 2017
All checks have passed
mvo5
approved these changes
Jan 17, 2017
Looks good but we need a Jamie review here as well I think.
jdstrand
reviewed
Jan 17, 2017
This change is fine as is however it doesn't address a non-setuid snap-confine that has filesystem capabilities. This could be added in a future commit, but if deferring that work, please add a TODO comment in the code that this is unimplemented. AIUI, snap-confine with fscaps is only being used on non-AppArmor enabled systems, but there is no technical reason why snap-confine must be setuid instead of utilizing fscaps on an AppArmor-enabled system.
| @@ -90,6 +90,23 @@ int main(int argc, char **argv) | ||
| #endif | ||
| struct sc_apparmor apparmor; | ||
| sc_init_apparmor_support(&apparmor); | ||
| + if (!apparmor.is_confined && apparmor.mode != SC_AA_NOT_APPLICABLE | ||
| + && getuid() != 0 && geteuid() == 0) { |
jdstrand
Jan 17, 2017
•
Edited 1 time
-
jdstrand
Jan 17, 2017
Contributor
-
Jan 17, 2017
To summarize this logic, only die() when:
- snap-confine is compiled with apparmor support AND
- this process is running unconfined AND
- the system is capable of using apparmor AND
- the real uid is not root AND
- the effective uid is root
This should work to prevent attacks by malicious, non-root, unconfined processes on the binary in the squashfs. It does not prevent against unconfined root processes attacking this binary, but that doesn't matter, an unconfined root process doesn't need this binary to escalate privileges.
| + // doesn't trigger apparmor to load and apply the correct profile but | ||
| + // has managed to keep the setuid root bit intact (e.g. running the | ||
| + // program directory from the core snap). To avoid escalation attacks | ||
| + // let's just politely refuse to run. |
jdstrand
Jan 17, 2017
Contributor
I'd prefer this text to be less conversational. Eg:
Refuse to run when this process is running unconfined on a system that
supports AppArmor when the effective uid is root and the real id is non-root.
This protects against, for example, unprivileged users trying to leverage the
snap-confine in the core snap to escalate privileges.
| @@ -26,12 +26,14 @@ update_core_snap_for_classic_reexec() { | ||
| unsquashfs "$snap" | ||
| cp /usr/lib/snapd/snap-exec squashfs-root/usr/lib/snapd/ | ||
| cp /usr/bin/snapctl squashfs-root/usr/bin/ | ||
| + # also inject new version of snap-confine and snap-scard-ns | ||
| + cp /usr/lib/snapd/snap-discard-ns squashfs-root/usr/lib/snapd/ | ||
| + cp /usr/lib/snapd/snap-confine squashfs-root/usr/lib/snapd/ |
jdstrand
Jan 17, 2017
Contributor
These changes seem unrelated, but perhaps they are needed for the spread test?
zyga
added some commits
Jan 18, 2017
All checks have passed
| @@ -101,6 +101,7 @@ int main(int argc, char **argv) | ||
| " but should be. Refusing to continue to avoid" | ||
| " permission escalation attacks"); | ||
| } | ||
| + // TODO: check for smimilar situation and linux capabilities. |
zyga
added some commits
Jan 18, 2017
Some checks were not successful
zyga commentedJan 13, 2017
The core snap is the only snap that is allowed to ship setuid-root
executables. Amnong those executables is snap-confine. Typically it is
itself confined with a tailored apparmor profile. Due to the way
apparmor profiles are applied, this is only happening when it is
installed as a system package or when it executes in the core snap on an
all-snap system.
On any system supplied with apparmor that has access to a mounted core
snap, unprivileged users may attempt to attack snap-confine with
malicious input (environment, arguments, etc). To defend against such
attacks snap-confine will now refuse to run if it should be confined and
has elevated permissions but the profile is not applied.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com