Merge "Add SimpleRequiresNoPermissionDetector" into main

5 files changed, 412 insertions, 51 deletions

diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt
index 290e7be9f6c4..94674348df08 100644
--- a/tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt
+++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt
@@ -22,6 +22,7 @@ import com.android.tools.lint.detector.api.CURRENT_API
 import com.google.android.lint.aidl.EnforcePermissionDetector
 import com.google.android.lint.aidl.PermissionAnnotationDetector
 import com.google.android.lint.aidl.SimpleManualPermissionEnforcementDetector
+import com.google.android.lint.aidl.SimpleRequiresNoPermissionDetector
 import com.google.auto.service.AutoService
 
 @AutoService(IssueRegistry::class)
@@ -34,6 +35,7 @@ class AndroidGlobalIssueRegistry : IssueRegistry() {
             EnforcePermissionDetector.ISSUE_MISUSING_ENFORCE_PERMISSION,
             PermissionAnnotationDetector.ISSUE_MISSING_PERMISSION_ANNOTATION,
             SimpleManualPermissionEnforcementDetector.ISSUE_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT,
+            SimpleRequiresNoPermissionDetector.ISSUE_SIMPLE_REQUIRES_NO_PERMISSION,
     )
 
     override val api: Int
diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleRequiresNoPermissionDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleRequiresNoPermissionDetector.kt
new file mode 100644
index 000000000000..1a13c0280ec6
--- /dev/null
+++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleRequiresNoPermissionDetector.kt
@@ -0,0 +1,118 @@
+/*
+ * Copyright (C) 2024 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.UastCallKind
+import org.jetbrains.uast.UBlockExpression
+import org.jetbrains.uast.UCallExpression
+import org.jetbrains.uast.UElement
+import org.jetbrains.uast.UMethod
+import org.jetbrains.uast.visitor.AbstractUastVisitor
+
+/**
+ * Ensures all AIDL implementations hosted by system_server which don't call other methods are
+ * annotated with @RequiresNoPermission. AIDL Interfaces part of `exemptAidlInterfaces` are skipped
+ * during this search to ensure the detector targets only new AIDL Interfaces.
+ */
+class SimpleRequiresNoPermissionDetector : AidlImplementationDetector() {
+    override fun visitAidlMethod(
+        context: JavaContext,
+        node: UMethod,
+        interfaceName: String,
+        body: UBlockExpression
+    ) {
+        if (!isSystemServicePath(context)) return
+        if (context.evaluator.isAbstract(node)) return
+
+        val fullyQualifiedInterfaceName =
+            getContainingAidlInterfaceQualified(context, node) ?: return
+        if (exemptAidlInterfaces.contains(fullyQualifiedInterfaceName)) return
+
+        if (node.hasAnnotation(ANNOTATION_REQUIRES_NO_PERMISSION)) return
+
+        if (!isCallingMethod(node)) {
+            context.report(
+                ISSUE_SIMPLE_REQUIRES_NO_PERMISSION,
+                node,
+                context.getLocation(node),
+                """
+                    Method ${node.name} doesn't perform any permission checks, meaning it should \
+                    be annotated with @RequiresNoPermission.
+                """.trimMargin()
+            )
+        }
+    }
+
+    private fun isCallingMethod(node: UMethod): Boolean {
+        val uCallExpressionVisitor = UCallExpressionVisitor()
+        node.accept(uCallExpressionVisitor)
+
+        return uCallExpressionVisitor.isCallingMethod
+    }
+
+    /**
+     * Visits the body of a `UMethod` and determines if it encounters a `UCallExpression` which is
+     * a `UastCallKind.METHOD_CALL`. `isCallingMethod` will hold the result of the search procedure.
+     */
+    private class UCallExpressionVisitor : AbstractUastVisitor() {
+        var isCallingMethod = false
+
+        override fun visitElement(node: UElement): Boolean {
+            // Stop the search early when a method call has been found.
+            return isCallingMethod
+        }
+
+        override fun visitCallExpression(node: UCallExpression): Boolean {
+            if (node.kind != UastCallKind.METHOD_CALL) return false
+
+            isCallingMethod = true
+            return true
+        }
+    }
+
+    companion object {
+
+        private val EXPLANATION = """
+            Method implementations of AIDL Interfaces hosted by the `system_server` which do not
+            call any other methods should be annotated with @RequiresNoPermission. That is because
+            not calling any other methods implies that the method does not perform any permission
+            checking.
+
+            Please migrate to an @RequiresNoPermission annotation.
+        """.trimIndent()
+
+        @JvmField
+        val ISSUE_SIMPLE_REQUIRES_NO_PERMISSION = Issue.create(
+            id = "SimpleRequiresNoPermission",
+            briefDescription = "System Service APIs not calling other methods should use @RNP",
+            explanation = EXPLANATION,
+            category = Category.SECURITY,
+            priority = 5,
+            severity = Severity.ERROR,
+            implementation = Implementation(
+                SimpleRequiresNoPermissionDetector::class.java,
+                Scope.JAVA_FILE_SCOPE
+            ),
+        )
+    }
+}
diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/PermissionAnnotationDetectorTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/PermissionAnnotationDetectorTest.kt
index 92d0829911bf..824be9309dbc 100644
--- a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/PermissionAnnotationDetectorTest.kt
+++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/PermissionAnnotationDetectorTest.kt
@@ -17,7 +17,6 @@
 package com.google.android.lint.aidl
 
 import com.android.tools.lint.checks.infrastructure.LintDetectorTest
-import com.android.tools.lint.checks.infrastructure.TestFile
 import com.android.tools.lint.checks.infrastructure.TestLintTask
 import com.android.tools.lint.detector.api.Detector
 import com.android.tools.lint.detector.api.Issue
@@ -64,7 +63,7 @@ class PermissionAnnotationDetectorTest : LintDetectorTest() {
                     """
                         package com.android.server;
                         public class Bar extends IBar.Stub {
-                            public void testMethod() { }
+                            public void testMethod(int parameter1, int parameter2) { }
                         }
                     """
                 )
@@ -75,8 +74,8 @@ class PermissionAnnotationDetectorTest : LintDetectorTest() {
             .expect(
                 """
                 src/frameworks/base/services/java/com/android/server/Bar.java:3: Error: The method testMethod is not permission-annotated. [MissingPermissionAnnotation]
-                    public void testMethod() { }
-                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                    public void testMethod(int parameter1, int parameter2) { }
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                 1 errors, 0 warnings
                 """
             )
@@ -90,7 +89,7 @@ class PermissionAnnotationDetectorTest : LintDetectorTest() {
                     """
                         package com.android.server;
                         public class Bar extends IBar.Stub {
-                            public void testMethod() { }
+                            public void testMethod(int parameter1, int parameter2) { }
                         }
                     """
                 )
@@ -132,7 +131,7 @@ class PermissionAnnotationDetectorTest : LintDetectorTest() {
                     """
                         package com.android.server;
                         public abstract class Bar extends IBar.Stub {
-                            public abstract void testMethod();
+                            public abstract void testMethod(int parameter1, int parameter2);
                         }
                     """
                 )
@@ -177,50 +176,6 @@ class PermissionAnnotationDetectorTest : LintDetectorTest() {
             .expectClean()
     }
 
-    /* Stubs */
-
-    // A service with permission annotation on the method.
-    private val interfaceIFoo: TestFile = java(
-        """
-        public interface IFoo extends android.os.IInterface {
-         public static abstract class Stub extends android.os.Binder implements IFoo {
-          }
-          @Override
-          @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE)
-          public void testMethod();
-          @Override
-          @android.annotation.RequiresNoPermission
-          public void testMethodNoPermission();
-          @Override
-          @android.annotation.PermissionManuallyEnforced
-          public void testMethodManual();
-        }
-        """
-    ).indented()
-
-    // A service with no permission annotation.
-    private val interfaceIBar: TestFile = java(
-        """
-        public interface IBar extends android.os.IInterface {
-         public static abstract class Stub extends android.os.Binder implements IBar {
-          }
-          public void testMethod();
-        }
-        """
-    ).indented()
-
-    // A service whose AIDL Interface is exempted.
-    private val interfaceIExempted: TestFile = java(
-        """
-        package android.accessibilityservice;
-        public interface IBrailleDisplayConnection extends android.os.IInterface {
-         public static abstract class Stub extends android.os.Binder implements IBrailleDisplayConnection {
-          }
-          public void testMethod();
-        }
-        """
-    ).indented()
-
     private val stubs = arrayOf(interfaceIFoo, interfaceIBar, interfaceIExempted)
 
     private fun createVisitedPath(filename: String) =
diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleRequiresNoPermissionDetectorTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleRequiresNoPermissionDetectorTest.kt
new file mode 100644
index 000000000000..a33b48c7eaa0
--- /dev/null
+++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleRequiresNoPermissionDetectorTest.kt
@@ -0,0 +1,244 @@
+/*
+ * Copyright (C) 2024 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.checks.infrastructure.LintDetectorTest
+import com.android.tools.lint.checks.infrastructure.TestLintTask
+import com.android.tools.lint.detector.api.Detector
+import com.android.tools.lint.detector.api.Issue
+
+class SimpleRequiresNoPermissionDetectorTest : LintDetectorTest() {
+    override fun getDetector(): Detector = SimpleRequiresNoPermissionDetector()
+    override fun getIssues(): List<Issue> = listOf(
+        SimpleRequiresNoPermissionDetector
+            .ISSUE_SIMPLE_REQUIRES_NO_PERMISSION
+    )
+
+    override fun lint(): TestLintTask = super.lint().allowMissingSdk()
+
+    fun testRequiresNoPermissionUsedCorrectly_shouldNotWarn() {
+        lint()
+            .files(
+                java(
+                    createVisitedPath("Foo.java"),
+                    """
+                        package com.android.server;
+                        public class Foo extends IFoo.Stub {
+                            private int memberInt;
+
+                            @Override
+                            @android.annotation.RequiresNoPermission
+                            public void testMethodNoPermission(int parameter1, int parameter2) {
+                                if (parameter1 < parameter2) {
+                                    memberInt = parameter1;
+                                } else {
+                                    memberInt = parameter2;
+                                }
+                            }
+                        }
+                    """
+                )
+                    .indented(),
+                *stubs
+            )
+            .run()
+            .expectClean()
+    }
+
+    fun testMissingRequiresNoPermission_shouldWarn() {
+        lint()
+            .files(
+                java(
+                    createVisitedPath("Bar.java"),
+                    """
+                        package com.android.server;
+                        public class Bar extends IBar.Stub {
+                            private int memberInt;
+
+                            @Override
+                            public void testMethod(int parameter1, int parameter2) {
+                                if (parameter1 < parameter2) {
+                                    memberInt = parameter1;
+                                } else {
+                                    memberInt = parameter2;
+                                }
+                            }
+                        }
+                    """
+                )
+                    .indented(),
+                *stubs
+            )
+            .run()
+            .expect(
+                """
+                src/frameworks/base/services/java/com/android/server/Bar.java:5: Error: Method testMethod doesn't perform any permission checks, meaning it should be annotated with @RequiresNoPermission. [SimpleRequiresNoPermission]
+                    @Override
+                    ^
+                1 errors, 0 warnings
+                """
+            )
+    }
+
+    fun testMethodOnlyPerformsConstructorCall_shouldWarn() {
+        lint()
+            .files(
+                java(
+                    createVisitedPath("Bar.java"),
+                    """
+                        package com.android.server;
+                        public class Bar extends IBar.Stub {
+                            private IntPair memberIntPair;
+
+                            @Override
+                            public void testMethod(int parameter1, int parameter2) {
+                                memberIntPair = new IntPair(parameter1, parameter2);
+                            }
+
+                            private static class IntPair {
+                                public int first;
+                                public int second;
+
+                                public IntPair(int first, int second) {
+                                    this.first = first;
+                                    this.second = second;
+                                }
+                            }
+                        }
+                    """
+                )
+                    .indented(),
+                *stubs
+            )
+            .run()
+            .expect(
+                """
+                src/frameworks/base/services/java/com/android/server/Bar.java:5: Error: Method testMethod doesn't perform any permission checks, meaning it should be annotated with @RequiresNoPermission. [SimpleRequiresNoPermission]
+                    @Override
+                    ^
+                1 errors, 0 warnings
+                """
+            )
+    }
+
+    fun testMissingRequiresNoPermissionInIgnoredDirectory_shouldNotWarn() {
+        lint()
+            .files(
+                java(
+                    ignoredPath,
+                    """
+                        package com.android.server;
+                        public class Bar extends IBar.Stub {
+                            @Override
+                            public void testMethod(int parameter1, int parameter2) {}
+                        }
+                    """
+                )
+                    .indented(),
+                *stubs
+            )
+            .run()
+            .expectClean()
+    }
+
+    fun testMissingRequiresNoPermissionAbstractMethod_shouldNotWarn() {
+        lint()
+            .files(
+                java(
+                    createVisitedPath("Bar.java"),
+                    """
+                        package com.android.server;
+                        public abstract class Bar extends IBar.Stub {
+                            private int memberInt;
+
+                            @Override
+                            public abstract void testMethodNoPermission(int parameter1, int parameter2);
+                        }
+                    """
+                )
+                    .indented(),
+                *stubs
+            )
+            .run()
+            .expectClean()
+    }
+
+    // If this test fails, consider the following steps:
+    //   1. Pick the first entry (interface) from `exemptAidlInterfaces`.
+    //   2. Change `interfaceIExempted` to use that interface.
+    //   3. Change this test's class to extend the interface's Stub.
+    fun testMissingRequiresNoPermissionAidlInterfaceExempted_shouldNotWarn() {
+        lint()
+            .files(
+                java(
+                    createVisitedPath("Bar.java"),
+                    """
+                        package com.android.server;
+                        public class Bar extends android.accessibilityservice.IBrailleDisplayConnection.Stub {
+                            public void testMethod(int parameter1, int parameter2) {}
+                        }
+                    """
+                )
+                    .indented(),
+                *stubs
+            )
+            .run()
+            .expectClean()
+    }
+
+    fun testMethodMakesAnotherMethodCall_shouldNotWarn() {
+        lint()
+            .files(
+                java(
+                    createVisitedPath("Bar.java"),
+                    """
+                        package com.android.server;
+                        public class Bar extends IBar.Stub {
+                            private int memberInt;
+
+                            @Override
+                            public void testMethod(int parameter1, int parameter2) {
+                                if (!hasPermission()) return;
+
+                                if (parameter1 < parameter2) {
+                                    memberInt = parameter1;
+                                } else {
+                                    memberInt = parameter2;
+                                }
+                            }
+
+                            private bool hasPermission() {
+                                // Perform a permission check.
+                                return true;
+                            }
+                        }
+                    """
+                )
+                    .indented(),
+                *stubs
+            )
+            .run()
+            .expectClean()
+    }
+
+    private val stubs = arrayOf(interfaceIFoo, interfaceIBar, interfaceIExempted)
+
+    private fun createVisitedPath(filename: String) =
+        "src/frameworks/base/services/java/com/android/server/$filename"
+
+    private val ignoredPath = "src/test/pkg/TestClass.java"
+}
diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt
index 2ec8fddbb4e9..18a8f186b624 100644
--- a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt
+++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt
@@ -85,4 +85,46 @@ val manifestStub: TestFile = java(
         }
     }
     """.trimIndent()
-)
\ No newline at end of file
+)
+
+// A service with permission annotation on the method.
+val interfaceIFoo: TestFile = java(
+    """
+        public interface IFoo extends android.os.IInterface {
+         public static abstract class Stub extends android.os.Binder implements IFoo {
+          }
+          @Override
+          @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE)
+          public void testMethod();
+          @Override
+          @android.annotation.RequiresNoPermission
+          public void testMethodNoPermission(int parameter1, int parameter2);
+          @Override
+          @android.annotation.PermissionManuallyEnforced
+          public void testMethodManual();
+        }
+        """
+).indented()
+
+// A service with no permission annotation.
+val interfaceIBar: TestFile = java(
+    """
+        public interface IBar extends android.os.IInterface {
+         public static abstract class Stub extends android.os.Binder implements IBar {
+          }
+          public void testMethod(int parameter1, int parameter2);
+        }
+        """
+).indented()
+
+// A service whose AIDL Interface is exempted.
+val interfaceIExempted: TestFile = java(
+    """
+        package android.accessibilityservice;
+        public interface IBrailleDisplayConnection extends android.os.IInterface {
+         public static abstract class Stub extends android.os.Binder implements IBrailleDisplayConnection {
+          }
+          public void testMethod();
+        }
+        """
+).indented()