[Windows] Simplify token privilege removal.
This CL simplifies the removal of privileges in a restricted token. The
fixed levels only permit leaving all privileges, removing all but the
traversal privilege or removing all. Therefore we just codify that in
the API.
Bug: 1270309
Change-Id: Ifa8a6a1ba1a4406271bb90af72f38ac6a1d24a93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3309405
Commit-Queue: James Forshaw <forshaw@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#946753}
NOKEYCHECK=True
GitOrigin-RevId: 6d62640f4bd0731e6c5f1723d5effbf0adce9d6c
diff --git a/win/src/restricted_token.cc b/win/src/restricted_token.cc
index 7977565..544b03d 100644
--- a/win/src/restricted_token.cc
+++ b/win/src/restricted_token.cc
@@ -18,18 +18,6 @@
namespace {
-LUID ConvertToLuid(const CHROME_LUID& luid) {
- LUID ret;
- memcpy(&ret, &luid, sizeof(luid));
- return ret;
-}
-
-CHROME_LUID ConvertToChromeLuid(const LUID& luid) {
- CHROME_LUID ret;
- memcpy(&ret, &luid, sizeof(luid));
- return ret;
-}
-
std::vector<SID_AND_ATTRIBUTES> ConvertToAttributes(
const std::vector<base::win::Sid>& sids,
DWORD attributes) {
@@ -41,6 +29,17 @@
return ret;
}
+bool DeletePrivilege(const base::win::ScopedHandle& token,
+ const wchar_t* name) {
+ TOKEN_PRIVILEGES privs = {};
+ privs.PrivilegeCount = 1;
+ if (!::LookupPrivilegeValue(nullptr, name, &privs.Privileges[0].Luid))
+ return false;
+ privs.Privileges[0].Attributes = SE_PRIVILEGE_REMOVED;
+ return !!::AdjustTokenPrivileges(token.Get(), FALSE, &privs, 0, nullptr,
+ nullptr);
+}
+
} // namespace
namespace sandbox {
@@ -48,7 +47,9 @@
RestrictedToken::RestrictedToken()
: integrity_level_(INTEGRITY_LEVEL_LAST),
init_(false),
- lockdown_default_dacl_(false) {}
+ lockdown_default_dacl_(false),
+ delete_all_privileges_(false),
+ remove_traversal_privilege_(false) {}
RestrictedToken::~RestrictedToken() {}
@@ -92,20 +93,16 @@
ConvertToAttributes(sids_for_deny_only_, SE_GROUP_USE_FOR_DENY_ONLY);
std::vector<SID_AND_ATTRIBUTES> restrict_sids =
ConvertToAttributes(sids_to_restrict_, 0);
- std::vector<LUID_AND_ATTRIBUTES> disable_privs(privileges_to_disable_.size());
- for (size_t i = 0; i < privileges_to_disable_.size(); ++i) {
- disable_privs[i].Attributes = 0;
- disable_privs[i].Luid = ConvertToLuid(privileges_to_disable_[i]);
- }
bool result = true;
HANDLE new_token_handle = nullptr;
- if (!deny_sids.empty() || !restrict_sids.empty() || !disable_privs.empty()) {
+ if (!deny_sids.empty() || !restrict_sids.empty() || delete_all_privileges_) {
result = ::CreateRestrictedToken(
- effective_token_.Get(), 0, static_cast<DWORD>(deny_sids.size()),
- deny_sids.data(), static_cast<DWORD>(disable_privs.size()),
- disable_privs.data(), static_cast<DWORD>(restrict_sids.size()),
- restrict_sids.data(), &new_token_handle);
+ effective_token_.Get(),
+ delete_all_privileges_ ? DISABLE_MAX_PRIVILEGE : 0,
+ static_cast<DWORD>(deny_sids.size()), deny_sids.data(), 0, nullptr,
+ static_cast<DWORD>(restrict_sids.size()), restrict_sids.data(),
+ &new_token_handle);
} else {
// Duplicate the token even if it's not modified at this point
// because any subsequent changes to this token would also affect the
@@ -119,6 +116,10 @@
return ::GetLastError();
base::win::ScopedHandle new_token(new_token_handle);
+ if (delete_all_privileges_ && remove_traversal_privilege_) {
+ if (!DeletePrivilege(new_token, SE_CHANGE_NOTIFY_NAME))
+ return ::GetLastError();
+ }
if (lockdown_default_dacl_) {
// Don't add Restricted sid and also remove logon sid access.
@@ -232,35 +233,12 @@
return ERROR_SUCCESS;
}
-DWORD RestrictedToken::DeleteAllPrivileges(
- const std::vector<std::wstring>& exceptions) {
+DWORD RestrictedToken::DeleteAllPrivileges(bool remove_traversal_privilege) {
DCHECK(init_);
if (!init_)
return ERROR_NO_TOKEN;
- std::unordered_set<std::wstring> privilege_set(exceptions.begin(),
- exceptions.end());
- // Build the list of privileges to disable
- for (const base::win::AccessToken::Privilege& privilege :
- query_token_->Privileges()) {
- if (privilege_set.count(privilege.GetName()) == 0) {
- privileges_to_disable_.push_back(privilege.GetLuid());
- }
- }
-
- return ERROR_SUCCESS;
-}
-
-DWORD RestrictedToken::DeletePrivilege(const wchar_t* privilege) {
- DCHECK(init_);
- if (!init_)
- return ERROR_NO_TOKEN;
-
- LUID luid = {0};
- if (::LookupPrivilegeValue(nullptr, privilege, &luid))
- privileges_to_disable_.push_back(ConvertToChromeLuid(luid));
- else
- return ::GetLastError();
-
+ delete_all_privileges_ = true;
+ remove_traversal_privilege_ = remove_traversal_privilege;
return ERROR_SUCCESS;
}
diff --git a/win/src/restricted_token.h b/win/src/restricted_token.h
index 71ac545..9cb7e1d 100644
--- a/win/src/restricted_token.h
+++ b/win/src/restricted_token.h
@@ -109,32 +109,14 @@
// the error.
DWORD AddUserSidForDenyOnly();
- // Lists all privileges in the token and add them to the list of privileges
- // to remove except for those present in the exceptions parameter. If
- // there is no exception needed, the caller can pass an empty list or nullptr
- // for the exceptions parameter.
+ // Specify to remove all privileges in the restricted token. By default this
+ // will not remove SeChangeNotifyPrivilege, however you can specify true for
+ // |remove_traversal_privilege| to remove that privilege as well.
//
// If the function succeeds, the return value is ERROR_SUCCESS. If the
// function fails, the return value is the win32 error code corresponding to
// the error.
- //
- // Sample usage:
- // std::vector<std::wstring> privilege_exceptions;
- // privilege_exceptions.push_back(SE_CHANGE_NOTIFY_NAME);
- // restricted_token.DeleteAllPrivileges(privilege_exceptions);
- DWORD DeleteAllPrivileges(const std::vector<std::wstring>& exceptions);
-
- // Adds a privilege to the list of privileges to remove in the restricted
- // token.
- // Parameter: privilege is the privilege name to remove. This is the string
- // representing the privilege. (e.g. "SeChangeNotifyPrivilege").
- // If the function succeeds, the return value is ERROR_SUCCESS. If the
- // function fails, the return value is the win32 error code corresponding to
- // the error.
- //
- // Sample usage:
- // restricted_token.DeletePrivilege(SE_LOAD_DRIVER_NAME);
- DWORD DeletePrivilege(const wchar_t* privilege);
+ DWORD DeleteAllPrivileges(bool remove_traversal_privilege);
// Adds a SID to the list of restricting sids in the restricted token.
// Parameter: sid is the sid to add to the list restricting sids.
@@ -206,8 +188,6 @@
private:
// The list of restricting sids in the restricted token.
std::vector<base::win::Sid> sids_to_restrict_;
- // The list of privileges to remove in the restricted token.
- std::vector<CHROME_LUID> privileges_to_disable_;
// The list of sids to mark as Deny Only in the restricted token.
std::vector<base::win::Sid> sids_for_deny_only_;
// The list of sids to add to the default DACL of the restricted token.
@@ -223,6 +203,10 @@
bool init_;
// Lockdown the default DACL when creating new tokens.
bool lockdown_default_dacl_;
+ // Delete all privileges except for SeChangeNotifyPrivilege.
+ bool delete_all_privileges_;
+ // Also delete SeChangeNotifyPrivilege if delete_all_privileges_ is true.
+ bool remove_traversal_privilege_;
};
} // namespace sandbox
diff --git a/win/src/restricted_token_unittest.cc b/win/src/restricted_token_unittest.cc
index e0dd7da..3f35043 100644
--- a/win/src/restricted_token_unittest.cc
+++ b/win/src/restricted_token_unittest.cc
@@ -410,7 +410,8 @@
base::win::ScopedHandle token_handle;
ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS), token.Init(nullptr));
- ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS), token.DeleteAllPrivileges({}));
+ ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS),
+ token.DeleteAllPrivileges(/*remove_traversal_privilege=*/true));
ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS),
token.GetRestrictedToken(&token_handle));
auto restricted_token = base::win::AccessToken::FromToken(token_handle.Get());
@@ -423,12 +424,9 @@
RestrictedToken token;
base::win::ScopedHandle token_handle;
- std::vector<std::wstring> exceptions;
- exceptions.push_back(SE_CHANGE_NOTIFY_NAME);
-
ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS), token.Init(nullptr));
ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS),
- token.DeleteAllPrivileges(exceptions));
+ token.DeleteAllPrivileges(/*remove_traversal_privilege=*/false));
ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS),
token.GetRestrictedToken(&token_handle));
@@ -439,23 +437,6 @@
EXPECT_EQ(privileges[0].GetName(), SE_CHANGE_NOTIFY_NAME);
}
-// Tests the method DeletePrivilege.
-TEST(RestrictedTokenTest, DeletePrivilege) {
- RestrictedToken token;
- base::win::ScopedHandle token_handle;
-
- ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS), token.Init(nullptr));
- ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS),
- token.DeletePrivilege(SE_CHANGE_NOTIFY_NAME));
- ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS),
- token.GetRestrictedToken(&token_handle));
- auto restricted_token = base::win::AccessToken::FromToken(token_handle.Get());
- ASSERT_TRUE(restricted_token);
- for (const auto& priv : restricted_token->Privileges()) {
- ASSERT_NE(priv.GetName(), SE_CHANGE_NOTIFY_NAME);
- }
-}
-
// Tests the method AddRestrictingSid.
TEST(RestrictedTokenTest, AddRestrictingSid) {
RestrictedToken token;
diff --git a/win/src/restricted_token_utils.cc b/win/src/restricted_token_utils.cc
index f68fe1d..3185f4e 100644
--- a/win/src/restricted_token_utils.cc
+++ b/win/src/restricted_token_utils.cc
@@ -82,6 +82,7 @@
bool deny_sids = true;
bool remove_privileges = true;
+ bool remove_traverse_privilege = false;
switch (security_level) {
case USER_UNPROTECTED: {
@@ -105,7 +106,6 @@
AddSidException(sid_exceptions, base::win::WellKnownSid::kInteractive);
AddSidException(sid_exceptions,
base::win::WellKnownSid::kAuthenticatedUser);
- privilege_exceptions.push_back(SE_CHANGE_NOTIFY_NAME);
break;
}
case USER_RESTRICTED_NON_ADMIN: {
@@ -114,7 +114,6 @@
AddSidException(sid_exceptions, base::win::WellKnownSid::kInteractive);
AddSidException(sid_exceptions,
base::win::WellKnownSid::kAuthenticatedUser);
- privilege_exceptions.push_back(SE_CHANGE_NOTIFY_NAME);
restricted_token.AddRestrictingSid(
base::win::WellKnownSid::kBuiltinUsers);
restricted_token.AddRestrictingSid(base::win::WellKnownSid::kWorld);
@@ -134,7 +133,6 @@
AddSidException(sid_exceptions, base::win::WellKnownSid::kInteractive);
AddSidException(sid_exceptions,
base::win::WellKnownSid::kAuthenticatedUser);
- privilege_exceptions.push_back(SE_CHANGE_NOTIFY_NAME);
restricted_token.AddRestrictingSid(
base::win::WellKnownSid::kBuiltinUsers);
restricted_token.AddRestrictingSid(base::win::WellKnownSid::kWorld);
@@ -149,7 +147,6 @@
AddSidException(sid_exceptions, base::win::WellKnownSid::kBuiltinUsers);
AddSidException(sid_exceptions, base::win::WellKnownSid::kWorld);
AddSidException(sid_exceptions, base::win::WellKnownSid::kInteractive);
- privilege_exceptions.push_back(SE_CHANGE_NOTIFY_NAME);
restricted_token.AddRestrictingSid(
base::win::WellKnownSid::kBuiltinUsers);
restricted_token.AddRestrictingSid(base::win::WellKnownSid::kWorld);
@@ -166,7 +163,6 @@
break;
}
case USER_RESTRICTED: {
- privilege_exceptions.push_back(SE_CHANGE_NOTIFY_NAME);
restricted_token.AddUserSidForDenyOnly();
restricted_token.AddRestrictingSid(base::win::WellKnownSid::kRestricted);
if (unique_restricted_sid)
@@ -174,6 +170,7 @@
break;
}
case USER_LOCKDOWN: {
+ remove_traverse_privilege = true;
restricted_token.AddUserSidForDenyOnly();
restricted_token.AddRestrictingSid(base::win::WellKnownSid::kNull);
if (unique_restricted_sid)
@@ -191,7 +188,7 @@
}
if (remove_privileges) {
- err_code = restricted_token.DeleteAllPrivileges(privilege_exceptions);
+ err_code = restricted_token.DeleteAllPrivileges(remove_traverse_privilege);
if (ERROR_SUCCESS != err_code)
return err_code;
}
