Merge "Only allow system and graphics to create secure displays" (3982f0e9) · Commits · e / os / android_frameworks_native

libs/gui/ISurfaceComposer.cpp

+25 −8

Original line number	Original line	Diff line number	Diff line
	@@ -244,10 +244,25 @@ public:
	{		{
	Parcel data, reply;		Parcel data, reply;
	data.writeInterfaceToken(ISurfaceComposer::getInterfaceDescriptor());		data.writeInterfaceToken(ISurfaceComposer::getInterfaceDescriptor());
	data.writeString8(displayName);		status_t status = data.writeString8(displayName);
	data.writeInt32(secure ? 1 : 0);		if (status) {
	remote()->transact(BnSurfaceComposer::CREATE_DISPLAY, data, &reply);		return nullptr;
	return reply.readStrongBinder();		}
			status = data.writeBool(secure);
			if (status) {
			return nullptr;
			}

			status = remote()->transact(BnSurfaceComposer::CREATE_DISPLAY, data, &reply);
			if (status) {
			return nullptr;
			}
			sp<IBinder> display;
			status = reply.readNullableStrongBinder(&display);
			if (status) {
			return nullptr;
			}
			return display;
	}		}

	virtual void destroyDisplay(const sp<IBinder>& display)		virtual void destroyDisplay(const sp<IBinder>& display)
	@@ -1295,10 +1310,12 @@ status_t BnSurfaceComposer::onTransact(
	}		}
	case CREATE_DISPLAY: {		case CREATE_DISPLAY: {
	CHECK_INTERFACE(ISurfaceComposer, data, reply);		CHECK_INTERFACE(ISurfaceComposer, data, reply);
	String8 displayName = data.readString8();		String8 displayName;
	bool secure = bool(data.readInt32());		SAFE_PARCEL(data.readString8, &displayName);
	sp<IBinder> display(createDisplay(displayName, secure));		bool secure = false;
	reply->writeStrongBinder(display);		SAFE_PARCEL(data.readBool, &secure);
			sp<IBinder> display = createDisplay(displayName, secure);
			SAFE_PARCEL(reply->writeStrongBinder, display);
	return NO_ERROR;		return NO_ERROR;
	}		}
	case DESTROY_DISPLAY: {		case DESTROY_DISPLAY: {

services/surfaceflinger/SurfaceFlinger.cpp

+9 −0

Original line number	Original line	Diff line number	Diff line
	@@ -509,6 +509,15 @@ sp<ISurfaceComposerClient> SurfaceFlinger::createConnection() {
	}		}

	sp<IBinder> SurfaceFlinger::createDisplay(const String8& displayName, bool secure) {		sp<IBinder> SurfaceFlinger::createDisplay(const String8& displayName, bool secure) {
			// onTransact already checks for some permissions, but adding an additional check here.
			// This is to ensure that only system and graphics can request to create a secure
			// display. Secure displays can show secure content so we add an additional restriction on it.
			const int uid = IPCThreadState::self()->getCallingUid();
			if (secure && uid != AID_GRAPHICS && uid != AID_SYSTEM) {
			ALOGE("Only privileged processes can create a secure display");
			return nullptr;
			}

	class DisplayToken : public BBinder {		class DisplayToken : public BBinder {
	sp<SurfaceFlinger> flinger;		sp<SurfaceFlinger> flinger;
	virtual ~DisplayToken() {		virtual ~DisplayToken() {

services/surfaceflinger/tests/Credentials_test.cpp

+25 −21

Original line number	Original line	Diff line number	Diff line
	@@ -98,26 +98,6 @@ protected:
	t.setLayer(mBGSurfaceControl, INT_MAX - 3).show(mBGSurfaceControl).apply());		t.setLayer(mBGSurfaceControl, INT_MAX - 3).show(mBGSurfaceControl).apply());
	}		}

	void setupVirtualDisplay() {
	mVirtualDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, true);
	const ssize_t displayWidth = 100;
	const ssize_t displayHeight = 100;

	// Background surface
	mVirtualSurfaceControl =
	mComposerClient->createSurface(SURFACE_NAME, displayWidth, displayHeight,
	PIXEL_FORMAT_RGBA_8888, 0);
	ASSERT_TRUE(mVirtualSurfaceControl != nullptr);
	ASSERT_TRUE(mVirtualSurfaceControl->isValid());

	Transaction t;
	t.setDisplayLayerStack(mVirtualDisplay, 0);
	ASSERT_EQ(NO_ERROR,
	t.setLayer(mVirtualSurfaceControl, INT_MAX - 3)
	.show(mVirtualSurfaceControl)
	.apply());
	}

	/**		/**
	* Sets UID to imitate Graphic's process.		* Sets UID to imitate Graphic's process.
	*/		*/
	@@ -165,6 +145,10 @@ protected:
	// Check as a non-supported user.		// Check as a non-supported user.
	setBinUID();		setBinUID();
	ASSERT_EQ(unprivilegedValue, condition());		ASSERT_EQ(unprivilegedValue, condition());

			// Check as shell since shell has some additional permissions
			seteuid(AID_SHELL);
			ASSERT_EQ(unprivilegedValue, condition());
	}		}
	};		};

	@@ -262,11 +246,31 @@ TEST_F(CredentialsTest, SetActiveColorModeTest) {
	}		}

	TEST_F(CredentialsTest, CreateDisplayTest) {		TEST_F(CredentialsTest, CreateDisplayTest) {
			// Only graphics and system processes can create a secure display.
	std::function<bool()> condition = [=]() {		std::function<bool()> condition = [=]() {
	sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, true);		sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, true);
	return testDisplay.get() != nullptr;		return testDisplay.get() != nullptr;
	};		};
	ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, true, false));
			// Check with root.
			seteuid(AID_ROOT);
			ASSERT_FALSE(condition());

			// Check as a Graphics user.
			setGraphicsUID();
			ASSERT_TRUE(condition());

			// Check as a system user.
			setSystemUID();
			ASSERT_TRUE(condition());

			// Check as a non-supported user.
			setBinUID();
			ASSERT_FALSE(condition());

			// Check as shell since shell has some additional permissions
			seteuid(AID_SHELL);
			ASSERT_FALSE(condition());

	condition = [=]() {		condition = [=]() {
	sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, false);		sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, false);

services/surfaceflinger/tests/SurfaceInterceptor_test.cpp

+2 −2

Original line number	Original line	Diff line number	Diff line
	@@ -422,7 +422,7 @@ void SurfaceInterceptorTest::shadowRadiusUpdate(Transaction& t) {
	}		}

	void SurfaceInterceptorTest::displayCreation(Transaction&) {		void SurfaceInterceptorTest::displayCreation(Transaction&) {
	sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, true);		sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, false);
	SurfaceComposerClient::destroyDisplay(testDisplay);		SurfaceComposerClient::destroyDisplay(testDisplay);
	}		}

	@@ -819,7 +819,7 @@ bool SurfaceInterceptorTest::surfaceDeletionFound(const Increment& increment,

	bool SurfaceInterceptorTest::displayCreationFound(const Increment& increment, bool foundDisplay) {		bool SurfaceInterceptorTest::displayCreationFound(const Increment& increment, bool foundDisplay) {
	bool isMatch(increment.display_creation().name() == DISPLAY_NAME.string() &&		bool isMatch(increment.display_creation().name() == DISPLAY_NAME.string() &&
	increment.display_creation().is_secure());		!increment.display_creation().is_secure());
	if (isMatch && !foundDisplay) {		if (isMatch && !foundDisplay) {
	foundDisplay = true;		foundDisplay = true;
	} else if (isMatch && foundDisplay) {		} else if (isMatch && foundDisplay) {

services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp

+7 −1

Original line number	Original line	Diff line number	Diff line
	@@ -24,6 +24,7 @@
	#include <type_traits>		#include <type_traits>

	#include <android/hardware/power/Boost.h>		#include <android/hardware/power/Boost.h>
			#include <binder/IPCThreadState.h>
	#include <compositionengine/Display.h>		#include <compositionengine/Display.h>
	#include <compositionengine/DisplayColorProfile.h>		#include <compositionengine/DisplayColorProfile.h>
	#include <compositionengine/impl/Display.h>		#include <compositionengine/impl/Display.h>
	@@ -37,6 +38,7 @@
	#include <gui/mock/GraphicBufferConsumer.h>		#include <gui/mock/GraphicBufferConsumer.h>
	#include <gui/mock/GraphicBufferProducer.h>		#include <gui/mock/GraphicBufferProducer.h>
	#include <log/log.h>		#include <log/log.h>
			#include <private/android_filesystem_config.h>
	#include <renderengine/mock/RenderEngine.h>		#include <renderengine/mock/RenderEngine.h>
	#include <ui/DebugUtils.h>		#include <ui/DebugUtils.h>

	@@ -1230,8 +1232,12 @@ TEST_F(DisplayTransactionTest, createDisplaySetsCurrentStateForSecureDisplay) {

	// --------------------------------------------------------------------		// --------------------------------------------------------------------
	// Invocation		// Invocation
			int64_t oldId = IPCThreadState::self()->clearCallingIdentity();
			// Set the calling identity to graphics so captureDisplay with secure is allowed.
			IPCThreadState::self()->restoreCallingIdentity(static_cast<int64_t>(AID_GRAPHICS) << 32 \|
			AID_GRAPHICS);
	sp<IBinder> displayToken = mFlinger.createDisplay(name, true);		sp<IBinder> displayToken = mFlinger.createDisplay(name, true);
			IPCThreadState::self()->restoreCallingIdentity(oldId);

	// --------------------------------------------------------------------		// --------------------------------------------------------------------
	// Postconditions		// Postconditions