SurfaceFlinger: Add Transaction#sanitize
Various elements of the Transaction interface require
a permission in order to apply. In particular the setTrustedOverlay
and setInputWindowInfo fields. These permission checks are
implemented by checking the PID and the UID of the process which
sent the transaction. Unfortunately widespread use of transaction
merging makes this inadequate. At the moment
IWindowSession#finishDrawing seems to be the only boundary on which
transactions move from client to system processes, and so we expose
a sanitize method and use it from there to resolve the situation
in an easily backportable way.
Moving forward it likely make sense to move security sensitive
interfaces off of Transaction. Most of the things behind permissions
currently are not truly security sensitive, more of just a request
not to use them.
It was also considered to sanitize transactions at all process
boundaries through writeToParcel, however this could be disruptive
as previously permissioned processes (WM and SysUI) could freely
exchange transactions. As the change needs to be backportable the
lowest risk option was chosen.
Bug: 213644870
Test: Existing tests pass
Change-Id: I424f45bc30ea8e56e4c4493203ee0749eabf239c
diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp
index 27d86bb..eec4a87 100644
--- a/libs/gui/LayerState.cpp
+++ b/libs/gui/LayerState.cpp
@@ -340,6 +340,79 @@
}
}
+void layer_state_t::sanitize(int32_t permissions) {
+ // TODO: b/109894387
+ //
+ // SurfaceFlinger's renderer is not prepared to handle cropping in the face of arbitrary
+ // rotation. To see the problem observe that if we have a square parent, and a child
+ // of the same size, then we rotate the child 45 degrees around its center, the child
+ // must now be cropped to a non rectangular 8 sided region.
+ //
+ // Of course we can fix this in the future. For now, we are lucky, SurfaceControl is
+ // private API, and arbitrary rotation is used in limited use cases, for instance:
+ // - WindowManager only uses rotation in one case, which is on a top level layer in which
+ // cropping is not an issue.
+ // - Launcher, as a privileged app, uses this to transition an application to PiP
+ // (picture-in-picture) mode.
+ //
+ // However given that abuse of rotation matrices could lead to surfaces extending outside
+ // of cropped areas, we need to prevent non-root clients without permission
+ // ACCESS_SURFACE_FLINGER nor ROTATE_SURFACE_FLINGER
+ // (a.k.a. everyone except WindowManager / tests / Launcher) from setting non rectangle
+ // preserving transformations.
+ if (what & eMatrixChanged) {
+ if (!(permissions & Permission::ROTATE_SURFACE_FLINGER)) {
+ ui::Transform t;
+ t.set(matrix.dsdx, matrix.dtdy, matrix.dtdx, matrix.dsdy);
+ if (!t.preserveRects()) {
+ what &= ~eMatrixChanged;
+ ALOGE("Stripped non rect preserving matrix in sanitize");
+ }
+ }
+ }
+
+ if (what & eFlagsChanged) {
+ if ((flags & eLayerIsDisplayDecoration) &&
+ !(permissions & Permission::INTERNAL_SYSTEM_WINDOW)) {
+ flags &= ~eLayerIsDisplayDecoration;
+ ALOGE("Stripped attempt to set LayerIsDisplayDecoration in sanitize");
+ }
+ }
+
+ if (what & layer_state_t::eInputInfoChanged) {
+ if (!(permissions & Permission::ACCESS_SURFACE_FLINGER)) {
+ what &= ~eInputInfoChanged;
+ ALOGE("Stripped attempt to set eInputInfoChanged in sanitize");
+ }
+ }
+ if (what & layer_state_t::eTrustedOverlayChanged) {
+ if (!(permissions & Permission::ACCESS_SURFACE_FLINGER)) {
+ what &= ~eTrustedOverlayChanged;
+ ALOGE("Stripped attempt to set eTrustedOverlay in sanitize");
+ }
+ }
+ if (what & layer_state_t::eDropInputModeChanged) {
+ if (!(permissions & Permission::ACCESS_SURFACE_FLINGER)) {
+ what &= ~eDropInputModeChanged;
+ ALOGE("Stripped attempt to set eDropInputModeChanged in sanitize");
+ }
+ }
+ if (what & layer_state_t::eFrameRateSelectionPriority) {
+ if (!(permissions & Permission::ACCESS_SURFACE_FLINGER)) {
+ what &= ~eFrameRateSelectionPriority;
+ ALOGE("Stripped attempt to set eFrameRateSelectionPriority in sanitize");
+ }
+ }
+ if (what & layer_state_t::eFrameRateChanged) {
+ if (!ValidateFrameRate(frameRate, frameRateCompatibility,
+ changeFrameRateStrategy,
+ "layer_state_t::sanitize",
+ permissions & Permission::ACCESS_SURFACE_FLINGER)) {
+ what &= ~eFrameRateChanged; // logged in ValidateFrameRate
+ }
+ }
+}
+
void layer_state_t::merge(const layer_state_t& other) {
if (other.what & ePositionChanged) {
what |= ePositionChanged;
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 31456cd..a7b2bcd 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -565,6 +565,13 @@
mListenerCallbacks = other.mListenerCallbacks;
}
+void SurfaceComposerClient::Transaction::sanitize() {
+ for (auto & [handle, composerState] : mComposerStates) {
+ composerState.state.sanitize(0 /* permissionMask */);
+ }
+ mInputWindowCommands.clear();
+}
+
std::unique_ptr<SurfaceComposerClient::Transaction>
SurfaceComposerClient::Transaction::createFromParcel(const Parcel* parcel) {
auto transaction = std::make_unique<Transaction>();
@@ -646,7 +653,6 @@
if (composerState.read(*parcel) == BAD_VALUE) {
return BAD_VALUE;
}
-
composerStates[surfaceControlHandle] = composerState;
}
diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h
index 968ace9..ef4ab1f 100644
--- a/libs/gui/include/gui/LayerState.h
+++ b/libs/gui/include/gui/LayerState.h
@@ -113,6 +113,12 @@
* Used to communicate layer information between SurfaceFlinger and its clients.
*/
struct layer_state_t {
+ enum Permission {
+ ACCESS_SURFACE_FLINGER = 0x1,
+ ROTATE_SURFACE_FLINGER = 0x2,
+ INTERNAL_SYSTEM_WINDOW = 0x4,
+ };
+
enum {
eLayerHidden = 0x01, // SURFACE_HIDDEN in SurfaceControl.java
eLayerOpaque = 0x02, // SURFACE_OPAQUE
@@ -181,6 +187,7 @@
status_t read(const Parcel& input);
bool hasBufferChanges() const;
bool hasValidBuffer() const;
+ void sanitize(int32_t permissions);
struct matrix22_t {
float dsdx{0};
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index 4f92878..366577d 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -624,6 +624,14 @@
void setAnimationTransaction();
void setEarlyWakeupStart();
void setEarlyWakeupEnd();
+
+ /**
+ * Strip the transaction of all permissioned requests, required when
+ * accepting transactions across process boundaries.
+ *
+ * TODO (b/213644870): Remove all permissioned things from Transaction
+ */
+ void sanitize();
};
status_t clearLayerFrameStats(const sp<IBinder>& token) const;
diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp
index 0c93872..40fc342 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -316,8 +316,7 @@
return assignTransform(&mDrawingState.transform, t);
}
-bool BufferStateLayer::setMatrix(const layer_state_t::matrix22_t& matrix,
- bool allowNonRectPreservingTransforms) {
+bool BufferStateLayer::setMatrix(const layer_state_t::matrix22_t& matrix) {
if (mRequestedTransform.dsdx() == matrix.dsdx && mRequestedTransform.dtdy() == matrix.dtdy &&
mRequestedTransform.dtdx() == matrix.dtdx && mRequestedTransform.dsdy() == matrix.dsdy) {
return false;
@@ -326,12 +325,6 @@
ui::Transform t;
t.set(matrix.dsdx, matrix.dtdy, matrix.dtdx, matrix.dsdy);
- if (!allowNonRectPreservingTransforms && !t.preserveRects()) {
- ALOGW("Attempt to set rotation matrix without permission ACCESS_SURFACE_FLINGER nor "
- "ROTATE_SURFACE_FLINGER ignored");
- return false;
- }
-
mRequestedTransform.set(matrix.dsdx, matrix.dtdy, matrix.dtdx, matrix.dsdy);
mDrawingState.sequence++;
diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h
index 2f613d7..248e013 100644
--- a/services/surfaceflinger/BufferStateLayer.h
+++ b/services/surfaceflinger/BufferStateLayer.h
@@ -70,8 +70,7 @@
bool addFrameEvent(const sp<Fence>& acquireFence, nsecs_t postedTime,
nsecs_t requestedPresentTime) override;
bool setPosition(float /*x*/, float /*y*/) override;
- bool setMatrix(const layer_state_t::matrix22_t& /*matrix*/,
- bool /*allowNonRectPreservingTransforms*/);
+ bool setMatrix(const layer_state_t::matrix22_t& /*matrix*/);
// Override to ignore legacy layer state properties that are not used by BufferStateLayer
bool setSize(uint32_t /*w*/, uint32_t /*h*/) override { return false; }
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index fa2c92d..7af1237 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -945,16 +945,10 @@
setTransactionFlags(eTransactionNeeded);
return true;
}
-bool Layer::setMatrix(const layer_state_t::matrix22_t& matrix,
- bool allowNonRectPreservingTransforms) {
+bool Layer::setMatrix(const layer_state_t::matrix22_t& matrix) {
ui::Transform t;
t.set(matrix.dsdx, matrix.dtdy, matrix.dtdx, matrix.dsdy);
- if (!allowNonRectPreservingTransforms && !t.preserveRects()) {
- ALOGW("Attempt to set rotation matrix without permission ACCESS_SURFACE_FLINGER nor "
- "ROTATE_SURFACE_FLINGER ignored");
- return false;
- }
mDrawingState.sequence++;
mDrawingState.transform.set(matrix.dsdx, matrix.dtdy, matrix.dtdx, matrix.dsdy);
mDrawingState.modified = true;
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 4cdd8fa..c7a3937 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -366,8 +366,7 @@
// Set a 2x2 transformation matrix on the layer. This transform
// will be applied after parent transforms, but before any final
// producer specified transform.
- virtual bool setMatrix(const layer_state_t::matrix22_t& matrix,
- bool allowNonRectPreservingTransforms);
+ virtual bool setMatrix(const layer_state_t::matrix22_t& matrix);
// This second set of geometry attributes are controlled by
// setGeometryAppliesWithResize, and their default mode is to be
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 88317e9..6361f5b 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -271,12 +271,6 @@
}
-enum Permission {
- ACCESS_SURFACE_FLINGER = 0x1,
- ROTATE_SURFACE_FLINGER = 0x2,
- INTERNAL_SYSTEM_WINDOW = 0x4,
-};
-
struct IdleTimerConfig {
int32_t timeoutMs;
bool supportKernelIdleTimer;
@@ -3995,19 +3989,20 @@
ATRACE_CALL();
uint32_t permissions =
- callingThreadHasUnscopedSurfaceFlingerAccess() ? Permission::ACCESS_SURFACE_FLINGER : 0;
+ callingThreadHasUnscopedSurfaceFlingerAccess() ?
+ layer_state_t::Permission::ACCESS_SURFACE_FLINGER : 0;
// Avoid checking for rotation permissions if the caller already has ACCESS_SURFACE_FLINGER
// permissions.
- if ((permissions & Permission::ACCESS_SURFACE_FLINGER) ||
+ if ((permissions & layer_state_t::Permission::ACCESS_SURFACE_FLINGER) ||
callingThreadHasRotateSurfaceFlingerAccess()) {
- permissions |= Permission::ROTATE_SURFACE_FLINGER;
+ permissions |= layer_state_t::Permission::ROTATE_SURFACE_FLINGER;
}
if (callingThreadHasInternalSystemWindowAccess()) {
- permissions |= Permission::INTERNAL_SYSTEM_WINDOW;
+ permissions |= layer_state_t::Permission::INTERNAL_SYSTEM_WINDOW;
}
- if (!(permissions & Permission::ACCESS_SURFACE_FLINGER) &&
+ if (!(permissions & layer_state_t::Permission::ACCESS_SURFACE_FLINGER) &&
(flags & (eEarlyWakeupStart | eEarlyWakeupEnd))) {
ALOGE("Only WindowManager is allowed to use eEarlyWakeup[Start|End] flags");
flags &= ~(eEarlyWakeupStart | eEarlyWakeupEnd);
@@ -4069,7 +4064,8 @@
uint32_t clientStateFlags = 0;
for (const ComposerState& state : states) {
- clientStateFlags |= setClientStateLocked(frameTimelineInfo, state, desiredPresentTime,
+ ComposerState stateCopy = state;
+ clientStateFlags |= setClientStateLocked(frameTimelineInfo, stateCopy, desiredPresentTime,
isAutoTimestamp, postTime, permissions);
if ((flags & eAnimation) && state.state.surface) {
if (const auto layer = fromHandle(state.state.surface).promote()) {
@@ -4083,7 +4079,7 @@
transactionFlags |= clientStateFlags;
- if (permissions & Permission::ACCESS_SURFACE_FLINGER) {
+ if (permissions & layer_state_t::Permission::ACCESS_SURFACE_FLINGER) {
transactionFlags |= addInputWindowCommands(inputWindowCommands);
} else if (!inputWindowCommands.empty()) {
ALOGE("Only privileged callers are allowed to send input commands.");
@@ -4194,11 +4190,11 @@
}
uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTimelineInfo,
- const ComposerState& composerState,
+ ComposerState& composerState,
int64_t desiredPresentTime, bool isAutoTimestamp,
int64_t postTime, uint32_t permissions) {
- const layer_state_t& s = composerState.state;
- const bool privileged = permissions & Permission::ACCESS_SURFACE_FLINGER;
+ layer_state_t& s = composerState.state;
+ s.sanitize(permissions);
std::vector<ListenerCallbacks> filteredListeners;
for (auto& listener : s.listeners) {
@@ -4316,43 +4312,14 @@
}
}
if (what & layer_state_t::eMatrixChanged) {
- // TODO: b/109894387
- //
- // SurfaceFlinger's renderer is not prepared to handle cropping in the face of arbitrary
- // rotation. To see the problem observe that if we have a square parent, and a child
- // of the same size, then we rotate the child 45 degrees around it's center, the child
- // must now be cropped to a non rectangular 8 sided region.
- //
- // Of course we can fix this in the future. For now, we are lucky, SurfaceControl is
- // private API, and arbitrary rotation is used in limited use cases, for instance:
- // - WindowManager only uses rotation in one case, which is on a top level layer in which
- // cropping is not an issue.
- // - Launcher, as a privileged app, uses this to transition an application to PiP
- // (picture-in-picture) mode.
- //
- // However given that abuse of rotation matrices could lead to surfaces extending outside
- // of cropped areas, we need to prevent non-root clients without permission
- // ACCESS_SURFACE_FLINGER nor ROTATE_SURFACE_FLINGER
- // (a.k.a. everyone except WindowManager / tests / Launcher) from setting non rectangle
- // preserving transformations.
- const bool allowNonRectPreservingTransforms =
- permissions & Permission::ROTATE_SURFACE_FLINGER;
- if (layer->setMatrix(s.matrix, allowNonRectPreservingTransforms)) flags |= eTraversalNeeded;
+ if (layer->setMatrix(s.matrix)) flags |= eTraversalNeeded;
}
if (what & layer_state_t::eTransparentRegionChanged) {
if (layer->setTransparentRegionHint(s.transparentRegion))
flags |= eTraversalNeeded;
}
if (what & layer_state_t::eFlagsChanged) {
- auto changedFlags = s.flags;
- if (changedFlags & layer_state_t::eLayerIsDisplayDecoration) {
- if ((permissions & Permission::INTERNAL_SYSTEM_WINDOW) == 0) {
- changedFlags &= ~layer_state_t::eLayerIsDisplayDecoration;
- ALOGE("Attempt to use eLayerIsDisplayDecoration without permission "
- "INTERNAL_SYSTEM_WINDOW!");
- }
- }
- if (layer->setFlags(changedFlags, s.mask)) flags |= eTraversalNeeded;
+ if (layer->setFlags(s.flags, s.mask)) flags |= eTraversalNeeded;
}
if (what & layer_state_t::eCornerRadiusChanged) {
if (layer->setCornerRadius(s.cornerRadius))
@@ -4410,12 +4377,8 @@
if (layer->setSidebandStream(s.sidebandStream)) flags |= eTraversalNeeded;
}
if (what & layer_state_t::eInputInfoChanged) {
- if (privileged) {
- layer->setInputInfo(*s.windowInfoHandle->getInfo());
- flags |= eTraversalNeeded;
- } else {
- ALOGE("Attempt to update WindowInfo without permission ACCESS_SURFACE_FLINGER");
- }
+ layer->setInputInfo(*s.windowInfoHandle->getInfo());
+ flags |= eTraversalNeeded;
}
std::optional<nsecs_t> dequeueBufferTimestamp;
if (what & layer_state_t::eMetadataChanged) {
@@ -4439,22 +4402,19 @@
if (layer->setShadowRadius(s.shadowRadius)) flags |= eTraversalNeeded;
}
if (what & layer_state_t::eFrameRateSelectionPriority) {
- if (privileged && layer->setFrameRateSelectionPriority(s.frameRateSelectionPriority)) {
+ if (layer->setFrameRateSelectionPriority(s.frameRateSelectionPriority)) {
flags |= eTraversalNeeded;
}
}
if (what & layer_state_t::eFrameRateChanged) {
- if (ValidateFrameRate(s.frameRate, s.frameRateCompatibility, s.changeFrameRateStrategy,
- "SurfaceFlinger::setClientStateLocked", privileged)) {
- const auto compatibility =
- Layer::FrameRate::convertCompatibility(s.frameRateCompatibility);
- const auto strategy =
- Layer::FrameRate::convertChangeFrameRateStrategy(s.changeFrameRateStrategy);
+ const auto compatibility =
+ Layer::FrameRate::convertCompatibility(s.frameRateCompatibility);
+ const auto strategy =
+ Layer::FrameRate::convertChangeFrameRateStrategy(s.changeFrameRateStrategy);
- if (layer->setFrameRate(
- Layer::FrameRate(Fps::fromValue(s.frameRate), compatibility, strategy))) {
- flags |= eTraversalNeeded;
- }
+ if (layer->setFrameRate(
+ Layer::FrameRate(Fps::fromValue(s.frameRate), compatibility, strategy))) {
+ flags |= eTraversalNeeded;
}
}
if (what & layer_state_t::eFixedTransformHintChanged) {
@@ -4466,12 +4426,8 @@
layer->setAutoRefresh(s.autoRefresh);
}
if (what & layer_state_t::eTrustedOverlayChanged) {
- if (privileged) {
- if (layer->setTrustedOverlay(s.isTrustedOverlay)) {
- flags |= eTraversalNeeded;
- }
- } else {
- ALOGE("Attempt to set trusted overlay without permission ACCESS_SURFACE_FLINGER");
+ if (layer->setTrustedOverlay(s.isTrustedOverlay)) {
+ flags |= eTraversalNeeded;
}
}
if (what & layer_state_t::eStretchChanged) {
@@ -4490,13 +4446,9 @@
}
}
if (what & layer_state_t::eDropInputModeChanged) {
- if (privileged) {
- if (layer->setDropInputMode(s.dropInputMode)) {
- flags |= eTraversalNeeded;
- mInputInfoChanged = true;
- }
- } else {
- ALOGE("Attempt to update DropInputMode without permission ACCESS_SURFACE_FLINGER");
+ if (layer->setDropInputMode(s.dropInputMode)) {
+ flags |= eTraversalNeeded;
+ mInputInfoChanged = true;
}
}
// This has to happen after we reparent children because when we reparent to null we remove
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 8ca9982..a6a17b8 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -729,7 +729,7 @@
// Returns true if there is at least one transaction that needs to be flushed
bool transactionFlushNeeded();
- uint32_t setClientStateLocked(const FrameTimelineInfo&, const ComposerState&,
+ uint32_t setClientStateLocked(const FrameTimelineInfo&, ComposerState&,
int64_t desiredPresentTime, bool isAutoTimestamp,
int64_t postTime, uint32_t permissions) REQUIRES(mStateLock);