From ba2336ca909aa999272f3751c7f7f22093f59712 Mon Sep 17 00:00:00 2001 From: mattgilbride Date: Thu, 1 Dec 2022 22:02:50 +0000 Subject: [PATCH] SimpleManualPermissionEnforcementDetector: Handle anyOf Handle "anyOf" style checks. @PermissionMethod now takes a parameter indicating that, if multiple permissions are checked, any ONE of them being granted means the permission check passes. This equates to the same "anyOf" parameter on the @EnforcePermission annotation, so generate fixes accordingly. The "anyOf" logic also includes some sanity checking to prevent suggesting a fix if "anyOf" and "allOf" checks are mixed, as we don't know what to do if that is the case. This change also enhances the linter's ability to pull out permission values (annotated with @PermissionName). - it reads hard coded values provided as parameters to the @PermissionMethod annotation - it reads multiple @PermissionName values passed as varargs to a method Bug: 232058525 Test: SimpleManualPermissionEnforcementDetectorTest Change-Id: I4e6a54a2820d54726725ea1e4a4b208e337e34c5 --- .../android/lint/aidl/EnforcePermissionFix.kt | 145 ++++++++++++++---- ...mpleManualPermissionEnforcementDetector.kt | 32 ++-- ...ManualPermissionEnforcementDetectorTest.kt | 112 ++++++++++++++ 3 files changed, 244 insertions(+), 45 deletions(-) diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionFix.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionFix.kt index 9ef6da14877e3..485765ba90909 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionFix.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionFix.kt @@ -20,6 +20,7 @@ import com.android.tools.lint.detector.api.JavaContext import com.android.tools.lint.detector.api.LintFix import com.android.tools.lint.detector.api.Location import com.android.tools.lint.detector.api.UastLintUtils.Companion.getAnnotationBooleanValue +import com.android.tools.lint.detector.api.UastLintUtils.Companion.getAnnotationStringValues import com.android.tools.lint.detector.api.findSelector import com.android.tools.lint.detector.api.getUMethod import com.google.android.lint.findCallExpression @@ -32,6 +33,8 @@ import org.jetbrains.kotlin.psi.psiUtil.parameterIndex import org.jetbrains.uast.UBinaryExpression import org.jetbrains.uast.UBlockExpression import org.jetbrains.uast.UCallExpression +import org.jetbrains.uast.UExpression +import org.jetbrains.uast.UExpressionList import org.jetbrains.uast.UIfExpression import org.jetbrains.uast.UThrowExpression import org.jetbrains.uast.UastBinaryOperator @@ -41,16 +44,12 @@ import org.jetbrains.uast.visitor.AbstractUastVisitor /** * Helper class that facilitates the creation of lint auto fixes - * - * Handles "Single" permission checks that should be migrated to @EnforcePermission(...), as well as consecutive checks - * that should be migrated to @EnforcePermission(allOf={...}) - * - * TODO: handle anyOf style annotations */ data class EnforcePermissionFix( val locations: List, val permissionNames: List, val errorLevel: Boolean, + val anyOf: Boolean, ) { fun toLintFix(annotationLocation: Location): LintFix { val removeFixes = this.locations.map { @@ -76,8 +75,13 @@ data class EnforcePermissionFix( get() { val quotedPermissions = permissionNames.joinToString(", ") { """"$it"""" } + val attributeName = + if (permissionNames.size > 1) { + if (anyOf) "anyOf" else "allOf" + } else null + val annotationParameter = - if (permissionNames.size > 1) "allOf={$quotedPermissions}" + if (attributeName != null) "$attributeName={$quotedPermissions}" else quotedPermissions return "@$ANNOTATION_ENFORCE_PERMISSION($annotationParameter)" @@ -96,10 +100,12 @@ data class EnforcePermissionFix( val annotation = getPermissionMethodAnnotation(method) ?: return null val returnsVoid = method.returnType == PsiType.VOID val orSelf = getAnnotationBooleanValue(annotation, "orSelf") ?: false + val anyOf = getAnnotationBooleanValue(annotation, "anyOf") ?: false return EnforcePermissionFix( listOf(getPermissionCheckLocation(context, callExpression)), getPermissionCheckValues(callExpression), - errorLevel = isErrorLevel(throws = returnsVoid, orSelf = orSelf) + errorLevel = isErrorLevel(throws = returnsVoid, orSelf = orSelf), + anyOf, ) } @@ -155,21 +161,31 @@ data class EnforcePermissionFix( } val orSelf = getAnnotationBooleanValue(permissionMethodAnnotation, "orSelf") ?: false + val anyOf = getAnnotationBooleanValue(permissionMethodAnnotation, "anyOf") ?: false return EnforcePermissionFix( listOf(context.getLocation(ifExpression)), getPermissionCheckValues(callExpression), errorLevel = isErrorLevel(throws = true, orSelf = orSelf), + anyOf = anyOf ) } - fun compose(individuals: List): EnforcePermissionFix = - EnforcePermissionFix( - individuals.flatMap { it.locations }, - individuals.flatMap { it.permissionNames }, - errorLevel = individuals.all(EnforcePermissionFix::errorLevel) + fun compose(individuals: List): EnforcePermissionFix { + val anyOfs = individuals.filter(EnforcePermissionFix::anyOf) + // anyOf/allOf should be consistent. If we encounter some @PermissionMethods that are anyOf + // and others that aren't, we don't know what to do. + if (anyOfs.isNotEmpty() && anyOfs.size < individuals.size) { + throw AnyOfAllOfException() + } + return EnforcePermissionFix( + individuals.flatMap(EnforcePermissionFix::locations), + individuals.flatMap(EnforcePermissionFix::permissionNames), + errorLevel = individuals.all(EnforcePermissionFix::errorLevel), + anyOf = anyOfs.isNotEmpty() ) + } /** * Given a permission check, get its proper location @@ -195,6 +211,7 @@ data class EnforcePermissionFix( * and pull out the permission value(s) being used. Also evaluates nested calls * to @PermissionMethod(s) in the given method's body. */ + @Throws(AnyOfAllOfException::class) private fun getPermissionCheckValues( callExpression: UCallExpression ): List { @@ -204,38 +221,94 @@ data class EnforcePermissionFix( val visitedCalls = mutableSetOf() // don't visit the same call twice val bfsQueue = ArrayDeque(listOf(callExpression)) - // Breadth First Search - evalutaing nested @PermissionMethod(s) in the available + var anyOfAllOfState: AnyOfAllOfState = AnyOfAllOfState.INITIAL + + // Bread First Search - evaluating nested @PermissionMethod(s) in the available // source code for @PermissionName(s). while (bfsQueue.isNotEmpty()) { - val current = bfsQueue.removeFirst() - visitedCalls.add(current) - result.addAll(findPermissions(current)) + val currentCallExpression = bfsQueue.removeFirst() + visitedCalls.add(currentCallExpression) + val currentPermissions = findPermissions(currentCallExpression) + result.addAll(currentPermissions) - current.resolve()?.getUMethod()?.accept(object : AbstractUastVisitor() { - override fun visitCallExpression(node: UCallExpression): Boolean { - if (isPermissionMethodCall(node) && node !in visitedCalls) { - bfsQueue.add(node) - } - return false - } - }) + val currentAnnotation = getPermissionMethodAnnotation( + currentCallExpression.resolve()?.getUMethod()) + val currentAnyOf = getAnnotationBooleanValue(currentAnnotation, "anyOf") ?: false + + // anyOf/allOf should be consistent. If we encounter a nesting of @PermissionMethods + // where we start in an anyOf state and switch to allOf, or vice versa, + // we don't know what to do. + if (anyOfAllOfState == AnyOfAllOfState.INITIAL) { + if (currentAnyOf) anyOfAllOfState = AnyOfAllOfState.ANY_OF + else if (result.isNotEmpty()) anyOfAllOfState = AnyOfAllOfState.ALL_OF + } + + if (anyOfAllOfState == AnyOfAllOfState.ALL_OF && currentAnyOf) { + throw AnyOfAllOfException() + } + + if (anyOfAllOfState == AnyOfAllOfState.ANY_OF && + !currentAnyOf && currentPermissions.size > 1) { + throw AnyOfAllOfException() + } + + currentCallExpression.resolve()?.getUMethod() + ?.accept(PermissionCheckValuesVisitor(visitedCalls, bfsQueue)) } return result.toList() } + private enum class AnyOfAllOfState { + INITIAL, + ANY_OF, + ALL_OF + } + + /** + * Adds visited permission method calls to the provided + * queue in support of the BFS traversal happening while + * this is used + */ + private class PermissionCheckValuesVisitor( + val visitedCalls: Set, + val bfsQueue: ArrayDeque + ) : AbstractUastVisitor() { + override fun visitCallExpression(node: UCallExpression): Boolean { + if (isPermissionMethodCall(node) && node !in visitedCalls) { + bfsQueue.add(node) + } + return false + } + } + private fun findPermissions( callExpression: UCallExpression, ): List { - val indices = callExpression.resolve()?.getUMethod() - ?.uastParameters - ?.filter(::hasPermissionNameAnnotation) - ?.mapNotNull { it.sourcePsi?.parameterIndex() } - ?: emptyList() + val annotation = getPermissionMethodAnnotation(callExpression.resolve()?.getUMethod()) - return indices.mapNotNull { - callExpression.getArgumentForParameter(it)?.evaluateString() - } + val hardCodedPermissions = (getAnnotationStringValues(annotation, "value") + ?: emptyArray()) + .toList() + + val indices = callExpression.resolve()?.getUMethod() + ?.uastParameters + ?.filter(::hasPermissionNameAnnotation) + ?.mapNotNull { it.sourcePsi?.parameterIndex() } + ?: emptyList() + + val argPermissions = indices + .flatMap { i -> + when (val argument = callExpression.getArgumentForParameter(i)) { + null -> listOf(null) + is UExpressionList -> // varargs e.g. someMethod(String...) + argument.expressions.map(UExpression::evaluateString) + else -> listOf(argument.evaluateString()) + } + } + .filterNotNull() + + return hardCodedPermissions + argPermissions } /** @@ -247,3 +320,11 @@ data class EnforcePermissionFix( private fun isErrorLevel(throws: Boolean, orSelf: Boolean): Boolean = throws && orSelf } } +/** + * anyOf/allOf @PermissionMethods must be consistent to apply @EnforcePermission - + * meaning if we encounter some @PermissionMethods that are anyOf, and others are allOf, + * we don't know which to apply. + */ +class AnyOfAllOfException : Exception() { + override val message: String = "anyOf/allOf permission methods cannot be mixed" +} diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt index 46517e1166a68..11a283af30495 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt @@ -79,22 +79,28 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { * as some other business logic is happening that prevents an automated fix. */ private fun accumulateSimplePermissionCheckFixes( - methodBody: UBlockExpression, - context: JavaContext - ): - EnforcePermissionFix? { - val singleFixes = mutableListOf() - for (expression in methodBody.expressions) { - singleFixes.add(getPermissionCheckFix(expression.skipParenthesizedExprDown(), context) - ?: break) - } - return when (singleFixes.size) { - 0 -> null - 1 -> singleFixes[0] - else -> EnforcePermissionFix.compose(singleFixes) + methodBody: UBlockExpression, + context: JavaContext + ): EnforcePermissionFix? { + try { + val singleFixes = mutableListOf() + for (expression in methodBody.expressions) { + val fix = getPermissionCheckFix( + expression.skipParenthesizedExprDown(), + context) ?: break + singleFixes.add(fix) + } + return when (singleFixes.size) { + 0 -> null + 1 -> singleFixes[0] + else -> EnforcePermissionFix.compose(singleFixes) + } + } catch (e: AnyOfAllOfException) { + return null } } + /** * If an expression boils down to a permission check, return * the helper for creating a lint auto fix to @EnforcePermission diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt index 5ab964d8efdd3..2ac550bd468aa 100644 --- a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt @@ -675,6 +675,118 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .expectClean() } + fun testAnyOf_hardCodedAndVarArgs() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.content.pm.PermissionMethod(anyOf = true) + private void helperHelper() { + helper("FOO", "BAR"); + } + + @android.content.pm.PermissionMethod(anyOf = true, value = {"BAZ", "BUZZ"}) + private void helper(@android.content.pm.PermissionName String... extraPermissions) {} + + @Override + public void test() throws android.os.RemoteException { + helperHelper(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:17: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + helperHelper(); + ~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 17: Annotate with @EnforcePermission: + @@ -15 +15 + + @android.annotation.EnforcePermission(anyOf={"BAZ", "BUZZ", "FOO", "BAR"}) + @@ -17 +18 + - helperHelper(); + """ + ) + } + + + fun testAnyOfAllOf_mixedConsecutiveCalls_ignored() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.content.pm.PermissionMethod + private void allOfhelper() { + mContext.enforceCallingOrSelfPermission("FOO"); + mContext.enforceCallingOrSelfPermission("BAR"); + } + + @android.content.pm.PermissionMethod(anyOf = true, permissions = {"BAZ", "BUZZ"}) + private void anyOfHelper() {} + + @Override + public void test() throws android.os.RemoteException { + allOfhelper(); + anyOfHelper(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testAnyOfAllOf_mixedNestedCalls_ignored() { + lint().files( + java( + """ + import android.content.Context; + import android.content.pm.PermissionName;import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.content.pm.PermissionMethod(anyOf = true) + private void anyOfCheck(@PermissionName String... permissions) { + allOfCheck("BAZ", "BUZZ"); + } + + @android.content.pm.PermissionMethod + private void allOfCheck(@PermissionName String... permissions) {} + + @Override + public void test() throws android.os.RemoteException { + anyOfCheck("FOO", "BAR"); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + companion object { val stubs = arrayOf( aidlStub,