From 185c04c8d41a0a6267346fc890ace39f671df128 Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Sat, 21 Nov 2015 16:11:05 -0500 Subject: [PATCH 1/2] Make isolate_path ignore unprivileged_user when not root If we are not root, we should not attempt to change the permissions of the work directory to the unprivileged user even if unprivileged_user is provided because, well, we cannot do that. --- utils/process/isolation.cpp | 2 +- utils/process/isolation_test.cpp | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/utils/process/isolation.cpp b/utils/process/isolation.cpp index 6496f42b..9dec3ab9 100644 --- a/utils/process/isolation.cpp +++ b/utils/process/isolation.cpp @@ -187,7 +187,7 @@ void process::isolate_path(const optional< passwd::user >& unprivileged_user, const fs::path& file) { - if (!unprivileged_user) + if (!unprivileged_user || !passwd::current_user().is_root()) return; const passwd::user& user = unprivileged_user.get(); diff --git a/utils/process/isolation_test.cpp b/utils/process/isolation_test.cpp index 9720d9fd..6475b49b 100644 --- a/utils/process/isolation_test.cpp +++ b/utils/process/isolation_test.cpp @@ -497,6 +497,21 @@ ATF_TEST_CASE_BODY(isolate_path__same_user) } +ATF_TEST_CASE(isolate_path__other_user_when_unprivileged); +ATF_TEST_CASE_HEAD(isolate_path__other_user_when_unprivileged) +{ + set_md_var("require.user", "unprivileged"); +} +ATF_TEST_CASE_BODY(isolate_path__other_user_when_unprivileged) +{ + passwd::user user = passwd::current_user(); + user.uid += 1; + user.gid += 1; + + do_isolate_path_test(utils::make_optional(user), none, none); +} + + ATF_TEST_CASE(isolate_path__drop_privileges); ATF_TEST_CASE_HEAD(isolate_path__drop_privileges) { @@ -563,6 +578,7 @@ ATF_INIT_TEST_CASES(tcs) ATF_ADD_TEST_CASE(tcs, isolate_path__no_user); ATF_ADD_TEST_CASE(tcs, isolate_path__same_user); + ATF_ADD_TEST_CASE(tcs, isolate_path__other_user_when_unprivileged); ATF_ADD_TEST_CASE(tcs, isolate_path__drop_privileges); ATF_ADD_TEST_CASE(tcs, isolate_path__drop_privileges_only_uid); ATF_ADD_TEST_CASE(tcs, isolate_path__drop_privileges_only_gid); From 6caa3aeb31ff9c9a3d077df9a8f9725d5b1ad159 Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Sat, 21 Nov 2015 16:25:09 -0500 Subject: [PATCH 2/2] Make isolate_child ignore unprivileged_user when not root If we are not root, we should not attempt to drop the privileges of the process to the unprivileged user even if unprivileged_user is provided because, well, we cannot do that. --- utils/process/isolation.cpp | 2 +- utils/process/isolation_test.cpp | 34 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/utils/process/isolation.cpp b/utils/process/isolation.cpp index 9dec3ab9..90dd08d5 100644 --- a/utils/process/isolation.cpp +++ b/utils/process/isolation.cpp @@ -148,7 +148,7 @@ process::isolate_child(const optional< passwd::user >& unprivileged_user, prepare_environment(work_directory); (void)::umask(0022); - if (unprivileged_user) { + if (unprivileged_user && passwd::current_user().is_root()) { const passwd::user& user = unprivileged_user.get(); if (user.gid != ::getgid()) { diff --git a/utils/process/isolation_test.cpp b/utils/process/isolation_test.cpp index 6475b49b..e88ab3af 100644 --- a/utils/process/isolation_test.cpp +++ b/utils/process/isolation_test.cpp @@ -315,6 +315,25 @@ ATF_TEST_CASE_BODY(isolate_child__clean_environment) } +ATF_TEST_CASE(isolate_child__other_user_when_unprivileged); +ATF_TEST_CASE_HEAD(isolate_child__other_user_when_unprivileged) +{ + set_md_var("require.user", "unprivileged"); +} +ATF_TEST_CASE_BODY(isolate_child__other_user_when_unprivileged) +{ + const passwd::user user = passwd::current_user(); + + passwd::user other_user = user; + other_user.uid += 1; + other_user.gid += 1; + process::isolate_child(utils::make_optional(other_user), fs::path(".")); + + ATF_REQUIRE_EQ(user.uid, ::getuid()); + ATF_REQUIRE_EQ(user.gid, ::getgid()); +} + + ATF_TEST_CASE(isolate_child__drop_privileges); ATF_TEST_CASE_HEAD(isolate_child__drop_privileges) { @@ -340,6 +359,13 @@ ATF_TEST_CASE_HEAD(isolate_child__drop_privileges_fail_uid) } ATF_TEST_CASE_BODY(isolate_child__drop_privileges_fail_uid) { + // Fake the current user as root so that we bypass the protections in + // isolate_child that prevent us from attempting a user switch when we are + // not root. We do this so we can trigger the setuid failure. + passwd::user root = passwd::user("root", 0, 0); + ATF_REQUIRE(root.is_root()); + passwd::set_current_user_for_testing(root); + passwd::user unprivileged_user = passwd::current_user(); unprivileged_user.uid += 1; @@ -359,6 +385,13 @@ ATF_TEST_CASE_HEAD(isolate_child__drop_privileges_fail_gid) } ATF_TEST_CASE_BODY(isolate_child__drop_privileges_fail_gid) { + // Fake the current user as root so that we bypass the protections in + // isolate_child that prevent us from attempting a user switch when we are + // not root. We do this so we can trigger the setgid failure. + passwd::user root = passwd::user("root", 0, 0); + ATF_REQUIRE(root.is_root()); + passwd::set_current_user_for_testing(root); + passwd::user unprivileged_user = passwd::current_user(); unprivileged_user.gid += 1; @@ -565,6 +598,7 @@ ATF_TEST_CASE_BODY(isolate_path__drop_privileges_only_gid) ATF_INIT_TEST_CASES(tcs) { ATF_ADD_TEST_CASE(tcs, isolate_child__clean_environment); + ATF_ADD_TEST_CASE(tcs, isolate_child__other_user_when_unprivileged); ATF_ADD_TEST_CASE(tcs, isolate_child__drop_privileges); ATF_ADD_TEST_CASE(tcs, isolate_child__drop_privileges_fail_uid); ATF_ADD_TEST_CASE(tcs, isolate_child__drop_privileges_fail_gid);