Merge "Add a PermissionMethodDetector lint to enforce proper usage of the annotation"
diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index 3130d2e..7cedf9c 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -6036,6 +6036,7 @@
* This can be called with or without the global lock held.
*/
@Override
+ @PackageManager.PermissionResult
@PermissionMethod
public int checkPermission(@PermissionName String permission, int pid, int uid) {
if (permission == null) {
@@ -6048,6 +6049,7 @@
* Binder IPC calls go through the public entry point.
* This can be called with or without the global lock held.
*/
+ @PackageManager.PermissionResult
@PermissionMethod
int checkCallingPermission(@PermissionName String permission) {
return checkPermission(permission,
diff --git 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
index 8aa3e25..4d69d26 100644
--- 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
@@ -42,6 +42,8 @@
SaferParcelChecker.ISSUE_UNSAFE_API_USAGE,
PackageVisibilityDetector.ISSUE_PACKAGE_NAME_NO_PACKAGE_VISIBILITY_FILTERS,
RegisterReceiverFlagDetector.ISSUE_RECEIVER_EXPORTED_FLAG,
+ PermissionMethodDetector.ISSUE_PERMISSION_METHOD_USAGE,
+ PermissionMethodDetector.ISSUE_CAN_BE_PERMISSION_METHOD,
)
override val api: Int
diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/Constants.kt b/tools/lint/checks/src/main/java/com/google/android/lint/Constants.kt
index 82eb8ed..3d5d01c 100644
--- a/tools/lint/checks/src/main/java/com/google/android/lint/Constants.kt
+++ b/tools/lint/checks/src/main/java/com/google/android/lint/Constants.kt
@@ -34,3 +34,7 @@
Method(CLASS_ACTIVITY_MANAGER_SERVICE, "checkPermission"),
Method(CLASS_ACTIVITY_MANAGER_INTERNAL, "enforceCallingPermission")
)
+
+const val ANNOTATION_PERMISSION_METHOD = "android.content.pm.PermissionMethod"
+const val ANNOTATION_PERMISSION_NAME = "android.content.pm.PermissionName"
+const val ANNOTATION_PERMISSION_RESULT = "android.content.pm.PackageManager.PermissionResult"
diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/PermissionMethodDetector.kt b/tools/lint/checks/src/main/java/com/google/android/lint/PermissionMethodDetector.kt
new file mode 100644
index 0000000..68a450d
--- /dev/null
+++ b/tools/lint/checks/src/main/java/com/google/android/lint/PermissionMethodDetector.kt
@@ -0,0 +1,201 @@
+/*
+ * 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
+
+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 com.android.tools.lint.detector.api.getUMethod
+import com.intellij.psi.PsiType
+import org.jetbrains.uast.UAnnotation
+import org.jetbrains.uast.UBlockExpression
+import org.jetbrains.uast.UCallExpression
+import org.jetbrains.uast.UElement
+import org.jetbrains.uast.UExpression
+import org.jetbrains.uast.UIfExpression
+import org.jetbrains.uast.UMethod
+import org.jetbrains.uast.UQualifiedReferenceExpression
+import org.jetbrains.uast.UReturnExpression
+import org.jetbrains.uast.getContainingUMethod
+
+/**
+ * Stops incorrect usage of {@link PermissionMethod}
+ * TODO: add tests once re-enabled (b/240445172, b/247542171)
+ */
+class PermissionMethodDetector : Detector(), SourceCodeScanner {
+
+ override fun getApplicableUastTypes(): List<Class<out UElement>> =
+ listOf(UAnnotation::class.java, UMethod::class.java)
+
+ override fun createUastHandler(context: JavaContext): UElementHandler =
+ PermissionMethodHandler(context)
+
+ private inner class PermissionMethodHandler(val context: JavaContext) : UElementHandler() {
+ override fun visitMethod(node: UMethod) {
+ if (hasPermissionMethodAnnotation(node)) return
+ if (onlyCallsPermissionMethod(node)) {
+ val location = context.getLocation(node.javaPsi.modifierList)
+ val fix = fix()
+ .annotate(ANNOTATION_PERMISSION_METHOD)
+ .range(location)
+ .autoFix()
+ .build()
+
+ context.report(
+ ISSUE_CAN_BE_PERMISSION_METHOD,
+ location,
+ "Annotate method with @PermissionMethod",
+ fix
+ )
+ }
+ }
+
+ override fun visitAnnotation(node: UAnnotation) {
+ if (node.qualifiedName != ANNOTATION_PERMISSION_METHOD) return
+ val method = node.getContainingUMethod() ?: return
+
+ if (!isPermissionMethodReturnType(method)) {
+ context.report(
+ ISSUE_PERMISSION_METHOD_USAGE,
+ context.getLocation(node),
+ """
+ Methods annotated with `@PermissionMethod` should return `void`, \
+ `boolean`, or `@PackageManager.PermissionResult int`."
+ """.trimIndent()
+ )
+ }
+
+ if (method.returnType == PsiType.INT &&
+ method.annotations.none { it.hasQualifiedName(ANNOTATION_PERMISSION_RESULT) }
+ ) {
+ context.report(
+ ISSUE_PERMISSION_METHOD_USAGE,
+ context.getLocation(node),
+ """
+ Methods annotated with `@PermissionMethod` that return `int` should \
+ also be annotated with `@PackageManager.PermissionResult.`"
+ """.trimIndent()
+ )
+ }
+ }
+ }
+
+ companion object {
+
+ private val EXPLANATION_PERMISSION_METHOD_USAGE = """
+ `@PermissionMethod` should annotate methods that ONLY perform permission lookups. \
+ Said methods should return `boolean`, `@PackageManager.PermissionResult int`, or return \
+ `void` and potentially throw `SecurityException`.
+ """.trimIndent()
+
+ @JvmField
+ val ISSUE_PERMISSION_METHOD_USAGE = Issue.create(
+ id = "PermissionMethodUsage",
+ briefDescription = "@PermissionMethod used incorrectly",
+ explanation = EXPLANATION_PERMISSION_METHOD_USAGE,
+ category = Category.CORRECTNESS,
+ priority = 5,
+ severity = Severity.ERROR,
+ implementation = Implementation(
+ PermissionMethodDetector::class.java,
+ Scope.JAVA_FILE_SCOPE
+ ),
+ enabledByDefault = true
+ )
+
+ private val EXPLANATION_CAN_BE_PERMISSION_METHOD = """
+ Methods that only call other methods annotated with @PermissionMethod (and do NOTHING else) can themselves \
+ be annotated with @PermissionMethod. For example:
+ ```
+ void wrapperHelper() {
+ // Context.enforceCallingPermission is annotated with @PermissionMethod
+ context.enforceCallingPermission(SOME_PERMISSION)
+ }
+ ```
+ """.trimIndent()
+
+ @JvmField
+ val ISSUE_CAN_BE_PERMISSION_METHOD = Issue.create(
+ id = "CanBePermissionMethod",
+ briefDescription = "Method can be annotated with @PermissionMethod",
+ explanation = EXPLANATION_CAN_BE_PERMISSION_METHOD,
+ category = Category.SECURITY,
+ priority = 5,
+ severity = Severity.WARNING,
+ implementation = Implementation(
+ PermissionMethodDetector::class.java,
+ Scope.JAVA_FILE_SCOPE
+ ),
+ enabledByDefault = false
+ )
+
+ private fun hasPermissionMethodAnnotation(method: UMethod): Boolean = method.annotations
+ .any {
+ it.hasQualifiedName(ANNOTATION_PERMISSION_METHOD)
+ }
+
+ private fun isPermissionMethodReturnType(method: UMethod): Boolean =
+ listOf(PsiType.VOID, PsiType.INT, PsiType.BOOLEAN).contains(method.returnType)
+
+ /**
+ * Identifies methods that...
+ * DO call other methods annotated with @PermissionMethod
+ * DO NOT do anything else
+ */
+ private fun onlyCallsPermissionMethod(method: UMethod): Boolean {
+ val body = method.uastBody as? UBlockExpression ?: return false
+ if (body.expressions.isEmpty()) return false
+ for (expression in body.expressions) {
+ when (expression) {
+ is UQualifiedReferenceExpression -> {
+ if (!isPermissionMethodCall(expression.selector)) return false
+ }
+ is UReturnExpression -> {
+ if (!isPermissionMethodCall(expression.returnExpression)) return false
+ }
+ is UCallExpression -> {
+ if (!isPermissionMethodCall(expression)) return false
+ }
+ is UIfExpression -> {
+ if (expression.thenExpression !is UReturnExpression) return false
+ if (!isPermissionMethodCall(expression.condition)) return false
+ }
+ else -> return false
+ }
+ }
+ return true
+ }
+
+ private fun isPermissionMethodCall(expression: UExpression?): Boolean {
+ return when (expression) {
+ is UQualifiedReferenceExpression ->
+ return isPermissionMethodCall(expression.selector)
+ is UCallExpression -> {
+ val calledMethod = expression.resolve()?.getUMethod() ?: return false
+ return hasPermissionMethodAnnotation(calledMethod)
+ }
+ else -> false
+ }
+ }
+ }
+}