DO NOT MERGE: 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
(cherry picked from commit de6d7b467e572d384f2bc1bc788259340ebe2f93)

4 files changed, 87 insertions, 1 deletions

diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp
index 2f31fbbb68..bb4b44684f 100644
--- a/libs/gui/LayerState.cpp
+++ b/libs/gui/LayerState.cpp
@@ -381,6 +381,71 @@ void DisplayState::merge(const DisplayState& other) {
     }
 }
 
+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 & 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 6ffd25d227..3f1277ed57 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -527,6 +527,13 @@ SurfaceComposerClient::Transaction::Transaction(const Transaction& other)
     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>();
@@ -611,7 +618,6 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel
         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 feef343748..9460319f4b 100644
--- a/libs/gui/include/gui/LayerState.h
+++ b/libs/gui/include/gui/LayerState.h
@@ -63,6 +63,12 @@ struct client_cache_t {
  * 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
@@ -130,6 +136,7 @@ struct layer_state_t {
     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 6e17212911..4b2075082c 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -587,6 +587,14 @@ public:
         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;