Use UIDFaker to ensure Credentials_test cleans up setting UID
Credntials_test currently sets the UID back to ROOT in the teardown of
the class. It also doesn't ensure the UIDs are set back after each call
to seteuid. This seems to result in a race where the UID is not updated
before calling the next test.
This may lead to failures in later tests if the UID is not updated in
time.
This fix also exposes a bug in the CredentialsTest where it verifes that
Shell UID doesn't have privilege access to SF, but that's actually
incorrect since Shell does have ACCESS_SURFACE_FLINGER permission so it
can call the privileged APIs.
Test: CredentialsTest
Bug: 243597796
Change-Id: I515f802aaedf1ea3dfc345af54e4d7383335bbb7
diff --git a/services/surfaceflinger/tests/Credentials_test.cpp b/services/surfaceflinger/tests/Credentials_test.cpp
index 353b813..775de4a 100644
--- a/services/surfaceflinger/tests/Credentials_test.cpp
+++ b/services/surfaceflinger/tests/Credentials_test.cpp
@@ -55,19 +55,12 @@
#pragma clang diagnostic ignored "-Wconversion"
class CredentialsTest : public ::testing::Test {
protected:
- void SetUp() override {
- // Start the tests as root.
- seteuid(AID_ROOT);
-
- ASSERT_NO_FATAL_FAILURE(initClient());
- }
+ void SetUp() override { ASSERT_NO_FATAL_FAILURE(initClient()); }
void TearDown() override {
mComposerClient->dispose();
mBGSurfaceControl.clear();
mComposerClient.clear();
- // Finish the tests as root.
- seteuid(AID_ROOT);
}
sp<IBinder> mDisplay;
@@ -102,31 +95,6 @@
}
/**
- * Sets UID to imitate Graphic's process.
- */
- void setGraphicsUID() {
- seteuid(AID_ROOT);
- seteuid(AID_GRAPHICS);
- }
-
- /**
- * Sets UID to imitate System's process.
- */
- void setSystemUID() {
- seteuid(AID_ROOT);
- seteuid(AID_SYSTEM);
- }
-
- /**
- * Sets UID to imitate a process that doesn't have any special privileges in
- * our code.
- */
- void setBinUID() {
- seteuid(AID_ROOT);
- seteuid(AID_BIN);
- }
-
- /**
* Template function the check a condition for different types of users: root
* graphics, system, and non-supported user. Root, graphics, and system should
* always equal privilegedValue, and non-supported user should equal unprivilegedValue.
@@ -134,24 +102,34 @@
template <typename T>
void checkWithPrivileges(std::function<T()> condition, T privilegedValue, T unprivilegedValue) {
// Check with root.
- seteuid(AID_ROOT);
- ASSERT_EQ(privilegedValue, condition());
+ {
+ UIDFaker f(AID_SYSTEM);
+ ASSERT_EQ(privilegedValue, condition());
+ }
// Check as a Graphics user.
- setGraphicsUID();
- ASSERT_EQ(privilegedValue, condition());
+ {
+ UIDFaker f(AID_GRAPHICS);
+ ASSERT_EQ(privilegedValue, condition());
+ }
// Check as a system user.
- setSystemUID();
- ASSERT_EQ(privilegedValue, condition());
+ {
+ UIDFaker f(AID_SYSTEM);
+ ASSERT_EQ(privilegedValue, condition());
+ }
// Check as a non-supported user.
- setBinUID();
- ASSERT_EQ(unprivilegedValue, condition());
+ {
+ UIDFaker f(AID_BIN);
+ ASSERT_EQ(unprivilegedValue, condition());
+ }
// Check as shell since shell has some additional permissions
- seteuid(AID_SHELL);
- ASSERT_EQ(unprivilegedValue, condition());
+ {
+ UIDFaker f(AID_SHELL);
+ ASSERT_EQ(privilegedValue, condition());
+ }
}
};
@@ -160,17 +138,23 @@
ASSERT_NO_FATAL_FAILURE(initClient());
// Graphics can init the client.
- setGraphicsUID();
- ASSERT_NO_FATAL_FAILURE(initClient());
+ {
+ UIDFaker f(AID_GRAPHICS);
+ ASSERT_NO_FATAL_FAILURE(initClient());
+ }
// System can init the client.
- setSystemUID();
- ASSERT_NO_FATAL_FAILURE(initClient());
+ {
+ UIDFaker f(AID_SYSTEM);
+ ASSERT_NO_FATAL_FAILURE(initClient());
+ }
// Anyone else can init the client.
- setBinUID();
- mComposerClient = sp<SurfaceComposerClient>::make();
- ASSERT_NO_FATAL_FAILURE(initClient());
+ {
+ UIDFaker f(AID_BIN);
+ mComposerClient = sp<SurfaceComposerClient>::make();
+ ASSERT_NO_FATAL_FAILURE(initClient());
+ }
}
TEST_F(CredentialsTest, GetBuiltInDisplayAccessTest) {
@@ -184,7 +168,7 @@
TEST_F(CredentialsTest, AllowedGetterMethodsTest) {
// The following methods are tested with a UID that is not root, graphics,
// or system, to show that anyone can access them.
- setBinUID();
+ UIDFaker f(AID_BIN);
const auto display = SurfaceComposerClient::getInternalDisplayToken();
ASSERT_TRUE(display != nullptr);
@@ -253,24 +237,34 @@
};
// Check with root.
- seteuid(AID_ROOT);
- ASSERT_FALSE(condition());
+ {
+ UIDFaker f(AID_ROOT);
+ ASSERT_FALSE(condition());
+ }
// Check as a Graphics user.
- setGraphicsUID();
- ASSERT_TRUE(condition());
+ {
+ UIDFaker f(AID_GRAPHICS);
+ ASSERT_TRUE(condition());
+ }
// Check as a system user.
- setSystemUID();
- ASSERT_TRUE(condition());
+ {
+ UIDFaker f(AID_SYSTEM);
+ ASSERT_TRUE(condition());
+ }
// Check as a non-supported user.
- setBinUID();
- ASSERT_FALSE(condition());
+ {
+ UIDFaker f(AID_BIN);
+ ASSERT_FALSE(condition());
+ }
// Check as shell since shell has some additional permissions
- seteuid(AID_SHELL);
- ASSERT_FALSE(condition());
+ {
+ UIDFaker f(AID_SHELL);
+ ASSERT_FALSE(condition());
+ }
condition = [=]() {
sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, false);
@@ -315,21 +309,27 @@
// Historically, only root and shell can access the getLayerDebugInfo which
// is called when we call dumpsys. I don't see a reason why we should change this.
std::vector<LayerDebugInfo> outLayers;
+ binder::Status status = binder::Status::ok();
// Check with root.
- seteuid(AID_ROOT);
- binder::Status status = sf->getLayerDebugInfo(&outLayers);
- ASSERT_EQ(NO_ERROR, statusTFromBinderStatus(status));
+ {
+ UIDFaker f(AID_ROOT);
+ status = sf->getLayerDebugInfo(&outLayers);
+ ASSERT_EQ(NO_ERROR, statusTFromBinderStatus(status));
+ }
// Check as a shell.
- seteuid(AID_SHELL);
- status = sf->getLayerDebugInfo(&outLayers);
- ASSERT_EQ(NO_ERROR, statusTFromBinderStatus(status));
+ {
+ UIDFaker f(AID_SHELL);
+ status = sf->getLayerDebugInfo(&outLayers);
+ ASSERT_EQ(NO_ERROR, statusTFromBinderStatus(status));
+ }
// Check as anyone else.
- seteuid(AID_ROOT);
- seteuid(AID_BIN);
- status = sf->getLayerDebugInfo(&outLayers);
- ASSERT_EQ(PERMISSION_DENIED, statusTFromBinderStatus(status));
+ {
+ UIDFaker f(AID_BIN);
+ status = sf->getLayerDebugInfo(&outLayers);
+ ASSERT_EQ(PERMISSION_DENIED, statusTFromBinderStatus(status));
+ }
}
TEST_F(CredentialsTest, IsWideColorDisplayBasicCorrectness) {