From 1ff38ab351a617c4870eec236b70932ff2c4473b Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Fri, 7 Jan 2022 18:23:06 -0800 Subject: 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) --- libs/gui/LayerState.cpp | 65 ++++++++++++++++++++++++++++ libs/gui/SurfaceComposerClient.cpp | 8 +++- libs/gui/include/gui/LayerState.h | 7 +++ libs/gui/include/gui/SurfaceComposerClient.h | 8 ++++ 4 files changed, 87 insertions(+), 1 deletion(-) 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::createFromParcel(const Parcel* parcel) { auto transaction = std::make_unique(); @@ -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& token) const; -- cgit v1.2.3 From 842a412840fbab2332a27d646a167177b110abf4 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Fri, 7 Jan 2022 18:23:06 -0800 Subject: 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) --- libs/gui/LayerState.cpp | 65 ++++++++++++++++++++++++++++ libs/gui/SurfaceComposerClient.cpp | 8 +++- libs/gui/include/gui/LayerState.h | 7 +++ libs/gui/include/gui/SurfaceComposerClient.h | 8 ++++ 4 files changed, 87 insertions(+), 1 deletion(-) 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::createFromParcel(const Parcel* parcel) { auto transaction = std::make_unique(); @@ -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& token) const; -- cgit v1.2.3 From ade0d07ba1ae18d9aee25b22ff6ef49599217f67 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Fri, 7 Jan 2022 18:23:06 -0800 Subject: 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) --- libs/gui/LayerState.cpp | 65 ++++++++++++++++++++++++++++ libs/gui/SurfaceComposerClient.cpp | 8 +++- libs/gui/include/gui/LayerState.h | 7 +++ libs/gui/include/gui/SurfaceComposerClient.h | 8 ++++ 4 files changed, 87 insertions(+), 1 deletion(-) diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index 77a883b332..bf275a5900 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -391,6 +391,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 725ea6571d..4bcc9d56c8 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -551,6 +551,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::createFromParcel(const Parcel* parcel) { auto transaction = std::make_unique(); @@ -635,7 +642,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 03e4aacdbe..2a8d30d2da 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -61,6 +61,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 @@ -128,6 +134,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 0d1d1a37bd..76b6d44fd2 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -590,6 +590,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& token) const; -- cgit v1.2.3