Create parent class for permission enforcement annotation detectors

Introduce AidlImplementationDetector - an abstract class for detectors that analyse methods implementing generated AIDL interface stubs. Refactor ManualPermissionCheckDetector into SimpleManualPermissionEnforcementDetector that inherits from AidlImplementationDetector. Bug: 260314719 Test: SimpleManualPermissionEnforcementDetectorTest Change-Id: I6221be50b3b358885404894ba158797bb8ea85e4
2022-11-24 10:32:11 +00:00
parent 607bdbf713
commit 635b108cc6
5 changed files with 214 additions and 170 deletions
--- a/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt
+++ b/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt
@@ -21,7 +21,7 @@ import com.android.tools.lint.client.api.Vendor
 import com.android.tools.lint.detector.api.CURRENT_API
 import com.google.android.lint.aidl.EnforcePermissionDetector
 import com.google.android.lint.aidl.EnforcePermissionHelperDetector
-import com.google.android.lint.aidl.ManualPermissionCheckDetector
+import com.google.android.lint.aidl.SimpleManualPermissionEnforcementDetector
 import com.google.android.lint.parcel.SaferParcelChecker
 import com.google.auto.service.AutoService

@@ -40,7 +40,7 @@ class AndroidFrameworkIssueRegistry : IssueRegistry() {
        EnforcePermissionDetector.ISSUE_MISSING_ENFORCE_PERMISSION,
        EnforcePermissionDetector.ISSUE_MISMATCHING_ENFORCE_PERMISSION,
        EnforcePermissionHelperDetector.ISSUE_ENFORCE_PERMISSION_HELPER,
-        ManualPermissionCheckDetector.ISSUE_USE_ENFORCE_PERMISSION_ANNOTATION,
+        SimpleManualPermissionEnforcementDetector.ISSUE_USE_ENFORCE_PERMISSION_ANNOTATION,
        SaferParcelChecker.ISSUE_UNSAFE_API_USAGE,
        PackageVisibilityDetector.ISSUE_PACKAGE_NAME_NO_PACKAGE_VISIBILITY_FILTERS,
        RegisterReceiverFlagDetector.ISSUE_RECEIVER_EXPORTED_FLAG,
--- a/tools/lint/checks/src/main/java/com/google/android/lint/aidl/AidlImplementationDetector.kt
+++ b/tools/lint/checks/src/main/java/com/google/android/lint/aidl/AidlImplementationDetector.kt
@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.android.lint.aidl
+
+import com.android.tools.lint.client.api.UElementHandler
+import com.android.tools.lint.detector.api.Detector
+import com.android.tools.lint.detector.api.JavaContext
+import com.android.tools.lint.detector.api.SourceCodeScanner
+import org.jetbrains.uast.UBlockExpression
+import org.jetbrains.uast.UElement
+import org.jetbrains.uast.UMethod
+
+/**
+ * Abstract class for detectors that look for methods implementing
+ * generated AIDL interface stubs
+ */
+abstract class AidlImplementationDetector : Detector(), SourceCodeScanner {
+    override fun getApplicableUastTypes(): List<Class<out UElement?>> =
+            listOf(UMethod::class.java)
+
+    override fun createUastHandler(context: JavaContext): UElementHandler = AidlStubHandler(context)
+
+    private inner class AidlStubHandler(val context: JavaContext) : UElementHandler() {
+        override fun visitMethod(node: UMethod) {
+            val interfaceName = getContainingAidlInterface(node)
+                    .takeUnless(EXCLUDED_CPP_INTERFACES::contains) ?: return
+            val body = (node.uastBody as? UBlockExpression) ?: return
+            visitAidlMethod(context, node, interfaceName, body)
+        }
+    }
+
+    abstract fun visitAidlMethod(
+            context: JavaContext,
+            node: UMethod,
+            interfaceName: String,
+            body: UBlockExpression,
+    )
+}
--- a/tools/lint/checks/src/main/java/com/google/android/lint/aidl/ManualPermissionCheckDetector.kt
+++ b/tools/lint/checks/src/main/java/com/google/android/lint/aidl/ManualPermissionCheckDetector.kt
@@ -1,158 +0,0 @@
-/*
- * Copyright (C) 2022 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package com.google.android.lint.aidl
-
-import com.android.tools.lint.client.api.UElementHandler
-import com.android.tools.lint.detector.api.Category
-import com.android.tools.lint.detector.api.Detector
-import com.android.tools.lint.detector.api.Implementation
-import com.android.tools.lint.detector.api.Issue
-import com.android.tools.lint.detector.api.JavaContext
-import com.android.tools.lint.detector.api.Scope
-import com.android.tools.lint.detector.api.Severity
-import com.android.tools.lint.detector.api.SourceCodeScanner
-import org.jetbrains.uast.UBlockExpression
-import org.jetbrains.uast.UCallExpression
-import org.jetbrains.uast.UElement
-import org.jetbrains.uast.UIfExpression
-import org.jetbrains.uast.UMethod
-import org.jetbrains.uast.UQualifiedReferenceExpression
-
-/**
- * Looks for methods implementing generated AIDL interface stubs
- * that can have simple permission checks migrated to
- * @EnforcePermission annotations
- *
- * TODO: b/242564870 (enable parse and autoFix of .aidl files)
- */
-@Suppress("UnstableApiUsage")
-class ManualPermissionCheckDetector : Detector(), SourceCodeScanner {
-    override fun getApplicableUastTypes(): List<Class<out UElement?>> =
-        listOf(UMethod::class.java)
-
-    override fun createUastHandler(context: JavaContext): UElementHandler = AidlStubHandler(context)
-
-    private inner class AidlStubHandler(val context: JavaContext) : UElementHandler() {
-        override fun visitMethod(node: UMethod) {
-            val interfaceName = getContainingAidlInterface(node)
-                .takeUnless(EXCLUDED_CPP_INTERFACES::contains) ?: return
-            val body = (node.uastBody as? UBlockExpression) ?: return
-            val fix = accumulateSimplePermissionCheckFixes(body) ?: return
-
-            val javaRemoveFixes = fix.locations.map {
-                fix()
-                    .replace()
-                    .reformat(true)
-                    .range(it)
-                    .with("")
-                    .autoFix()
-                    .build()
-            }
-
-            val javaAnnotateFix = fix()
-                .annotate(fix.annotation)
-                .range(context.getLocation(node))
-                .autoFix()
-                .build()
-
-            val message =
-                "$interfaceName permission check can be converted to @EnforcePermission annotation"
-
-            context.report(
-                ISSUE_USE_ENFORCE_PERMISSION_ANNOTATION,
-                fix.locations.last(),
-                message,
-                fix().composite(*javaRemoveFixes.toTypedArray(), javaAnnotateFix)
-            )
-        }
-
-        /**
-         * Walk the expressions in the method, looking for simple permission checks.
-         *
-         * If a single permission check is found at the beginning of the method,
-         * this should be migrated to @EnforcePermission(value).
-         *
-         * If multiple consecutive permission checks are found,
-         * these should be migrated to @EnforcePermission(allOf={value1, value2, ...})
-         *
-         * As soon as something other than a permission check is encountered, stop looking,
-         * as some other business logic is happening that prevents an automated fix.
-         */
-        private fun accumulateSimplePermissionCheckFixes(methodBody: UBlockExpression):
-                EnforcePermissionFix? {
-            val singleFixes = mutableListOf<EnforcePermissionFix>()
-            for (expression in methodBody.expressions) {
-                singleFixes.add(getPermissionCheckFix(expression) ?: break)
-            }
-            return when (singleFixes.size) {
-                0 -> null
-                1 -> singleFixes[0]
-                else -> EnforcePermissionFix.compose(singleFixes)
-            }
-        }
-
-        /**
-         * If an expression boils down to a permission check, return
-         * the helper for creating a lint auto fix to @EnforcePermission
-         */
-        private fun getPermissionCheckFix(startingExpression: UElement?):
-                EnforcePermissionFix? {
-            return when (startingExpression) {
-                is UQualifiedReferenceExpression -> getPermissionCheckFix(
-                    startingExpression.selector
-                )
-
-                is UIfExpression -> getPermissionCheckFix(startingExpression.condition)
-
-                is UCallExpression -> return EnforcePermissionFix
-                            .fromCallExpression(context, startingExpression)
-
-                else -> null
-            }
-        }
-    }
-
-    companion object {
-
-        private val EXPLANATION = """
-            Whenever possible, method implementations of AIDL interfaces should use the @EnforcePermission
-            annotation to declare the permissions to be enforced.  The verification code is then
-            generated by the AIDL compiler, which also takes care of annotating the generated java
-            code.
-
-            This reduces the risk of bugs around these permission checks (that often become vulnerabilities).
-            It also enables easier auditing and review.
-
-            Please migrate to an @EnforcePermission annotation. (See: go/aidl-enforce-howto)
-        """.trimIndent()
-
-        @JvmField
-        val ISSUE_USE_ENFORCE_PERMISSION_ANNOTATION = Issue.create(
-            id = "UseEnforcePermissionAnnotation",
-            briefDescription = "Manual permission check can be @EnforcePermission annotation",
-            explanation = EXPLANATION,
-            category = Category.SECURITY,
-            priority = 5,
-            severity = Severity.WARNING,
-            implementation = Implementation(
-                ManualPermissionCheckDetector::class.java,
-                Scope.JAVA_FILE_SCOPE
-            ),
-            enabledByDefault = false, // TODO: enable once b/241171714 is resolved
-        )
-    }
-}
--- a/tools/lint/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt
+++ b/tools/lint/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt
@@ -0,0 +1,150 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.android.lint.aidl
+
+import com.android.tools.lint.detector.api.Category
+import com.android.tools.lint.detector.api.Implementation
+import com.android.tools.lint.detector.api.Issue
+import com.android.tools.lint.detector.api.JavaContext
+import com.android.tools.lint.detector.api.Scope
+import com.android.tools.lint.detector.api.Severity
+import org.jetbrains.uast.UBlockExpression
+import org.jetbrains.uast.UCallExpression
+import org.jetbrains.uast.UElement
+import org.jetbrains.uast.UIfExpression
+import org.jetbrains.uast.UMethod
+import org.jetbrains.uast.UQualifiedReferenceExpression
+
+/**
+ * Looks for methods implementing generated AIDL interface stubs
+ * that can have simple permission checks migrated to
+ * @EnforcePermission annotations
+ *
+ * TODO: b/242564870 (enable parse and autoFix of .aidl files)
+ */
+@Suppress("UnstableApiUsage")
+class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() {
+    override fun visitAidlMethod(
+            context: JavaContext,
+            node: UMethod,
+            interfaceName: String,
+            body: UBlockExpression
+    ) {
+        val fix = accumulateSimplePermissionCheckFixes(body, context) ?: return
+
+        val javaRemoveFixes = fix.locations.map {
+            fix()
+                    .replace()
+                    .reformat(true)
+                    .range(it)
+                    .with("")
+                    .autoFix()
+                    .build()
+        }
+
+        val javaAnnotateFix = fix()
+                .annotate(fix.annotation)
+                .range(context.getLocation(node))
+                .autoFix()
+                .build()
+
+        context.report(
+                ISSUE_USE_ENFORCE_PERMISSION_ANNOTATION,
+                fix.locations.last(),
+                "$interfaceName permission check can be converted to @EnforcePermission annotation",
+                fix().composite(*javaRemoveFixes.toTypedArray(), javaAnnotateFix)
+        )
+    }
+
+    /**
+     * Walk the expressions in the method, looking for simple permission checks.
+     *
+     * If a single permission check is found at the beginning of the method,
+     * this should be migrated to @EnforcePermission(value).
+     *
+     * If multiple consecutive permission checks are found,
+     * these should be migrated to @EnforcePermission(allOf={value1, value2, ...})
+     *
+     * As soon as something other than a permission check is encountered, stop looking,
+     * as some other business logic is happening that prevents an automated fix.
+     */
+    private fun accumulateSimplePermissionCheckFixes(
+            methodBody: UBlockExpression,
+            context: JavaContext
+    ):
+            EnforcePermissionFix? {
+        val singleFixes = mutableListOf<EnforcePermissionFix>()
+        for (expression in methodBody.expressions) {
+            singleFixes.add(getPermissionCheckFix(expression, context) ?: break)
+        }
+        return when (singleFixes.size) {
+            0 -> null
+            1 -> singleFixes[0]
+            else -> EnforcePermissionFix.compose(singleFixes)
+        }
+    }
+
+    /**
+     * If an expression boils down to a permission check, return
+     * the helper for creating a lint auto fix to @EnforcePermission
+     */
+    private fun getPermissionCheckFix(startingExpression: UElement?, context: JavaContext):
+            EnforcePermissionFix? {
+        return when (startingExpression) {
+            is UQualifiedReferenceExpression -> getPermissionCheckFix(
+                    startingExpression.selector, context
+            )
+
+            is UIfExpression -> getPermissionCheckFix(startingExpression.condition, context)
+
+            is UCallExpression -> return EnforcePermissionFix
+                    .fromCallExpression(context, startingExpression)
+
+            else -> null
+        }
+    }
+
+    companion object {
+
+        private val EXPLANATION = """
+            Whenever possible, method implementations of AIDL interfaces should use the @EnforcePermission
+            annotation to declare the permissions to be enforced.  The verification code is then
+            generated by the AIDL compiler, which also takes care of annotating the generated java
+            code.
+
+            This reduces the risk of bugs around these permission checks (that often become vulnerabilities).
+            It also enables easier auditing and review.
+
+            Please migrate to an @EnforcePermission annotation. (See: go/aidl-enforce-howto)
+        """.trimIndent()
+
+        @JvmField
+        val ISSUE_USE_ENFORCE_PERMISSION_ANNOTATION = Issue.create(
+                id = "SimpleManualPermissionEnforcement",
+                briefDescription = "Manual permission check can be @EnforcePermission annotation",
+                explanation = EXPLANATION,
+                category = Category.SECURITY,
+                priority = 5,
+                severity = Severity.WARNING,
+                implementation = Implementation(
+                        SimpleManualPermissionEnforcementDetector::class.java,
+                        Scope.JAVA_FILE_SCOPE
+                ),
+                enabledByDefault = false, // TODO: enable once b/241171714 is resolved
+        )
+    }
+}
--- a/tools/lint/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt
+++ b/tools/lint/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt
@@ -23,10 +23,10 @@ import com.android.tools.lint.detector.api.Detector
 import com.android.tools.lint.detector.api.Issue

@Suppress("UnstableApiUsage")
-class ManualPermissionCheckDetectorTest : LintDetectorTest() {
-    override fun getDetector(): Detector = ManualPermissionCheckDetector()
+class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
+    override fun getDetector(): Detector = SimpleManualPermissionEnforcementDetector()
    override fun getIssues(): List<Issue> = listOf(
-        ManualPermissionCheckDetector
+            SimpleManualPermissionEnforcementDetector
            .ISSUE_USE_ENFORCE_PERMISSION_ANNOTATION
    )

@@ -52,7 +52,7 @@ class ManualPermissionCheckDetectorTest : LintDetectorTest() {
            .run()
            .expect(
                """
-                src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [UseEnforcePermissionAnnotation]
+                src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
                        mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");
                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                0 errors, 1 warnings
@@ -92,7 +92,7 @@ class ManualPermissionCheckDetectorTest : LintDetectorTest() {
            .run()
            .expect(
                """
-                src/Foo.java:8: Warning: ITest permission check can be converted to @EnforcePermission annotation [UseEnforcePermissionAnnotation]
+                src/Foo.java:8: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
                            mContext.enforceCallingOrSelfPermission(
                            ^
                0 errors, 1 warnings
@@ -132,7 +132,7 @@ class ManualPermissionCheckDetectorTest : LintDetectorTest() {
            .run()
            .expect(
                """
-                src/Foo.java:8: Warning: ITest permission check can be converted to @EnforcePermission annotation [UseEnforcePermissionAnnotation]
+                src/Foo.java:8: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
                        mContext.enforceCallingOrSelfPermission(android.Manifest.permission.READ_CONTACTS, "foo");
                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                0 errors, 1 warnings
@@ -174,7 +174,7 @@ class ManualPermissionCheckDetectorTest : LintDetectorTest() {
            .run()
            .expect(
                """
-                src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [UseEnforcePermissionAnnotation]
+                src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
                            mContext.enforceCallingOrSelfPermission(
                            ^
                0 errors, 1 warnings
@@ -243,7 +243,7 @@ class ManualPermissionCheckDetectorTest : LintDetectorTest() {
            .run()
            .expect(
                """
-                src/Foo.java:14: Warning: ITest permission check can be converted to @EnforcePermission annotation [UseEnforcePermissionAnnotation]
+                src/Foo.java:14: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
                        helper();
                        ~~~~~~~~~
                0 errors, 1 warnings
@@ -289,7 +289,7 @@ class ManualPermissionCheckDetectorTest : LintDetectorTest() {
            .run()
            .expect(
                """
-                src/Foo.java:16: Warning: ITest permission check can be converted to @EnforcePermission annotation [UseEnforcePermissionAnnotation]
+                src/Foo.java:16: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
                        mContext.enforceCallingOrSelfPermission("FOO", "foo");
                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                0 errors, 1 warnings
@@ -340,7 +340,7 @@ class ManualPermissionCheckDetectorTest : LintDetectorTest() {
            .run()
            .expect(
                """
-                src/Foo.java:19: Warning: ITest permission check can be converted to @EnforcePermission annotation [UseEnforcePermissionAnnotation]
+                src/Foo.java:19: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
                        helperHelper();
                        ~~~~~~~~~~~~~~~
                0 errors, 1 warnings