diff options
5 files changed, 143 insertions, 196 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 a20266a9b140..28eab8f62e74 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 @@ -20,7 +20,6 @@ import com.android.tools.lint.client.api.IssueRegistry 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.SimpleManualPermissionEnforcementDetector import com.google.auto.service.AutoService @@ -30,7 +29,8 @@ class AndroidGlobalIssueRegistry : IssueRegistry() { override val issues = listOf( EnforcePermissionDetector.ISSUE_MISSING_ENFORCE_PERMISSION, EnforcePermissionDetector.ISSUE_MISMATCHING_ENFORCE_PERMISSION, - EnforcePermissionHelperDetector.ISSUE_ENFORCE_PERMISSION_HELPER, + EnforcePermissionDetector.ISSUE_ENFORCE_PERMISSION_HELPER, + EnforcePermissionDetector.ISSUE_MISUSING_ENFORCE_PERMISSION, SimpleManualPermissionEnforcementDetector.ISSUE_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT, ) @@ -45,4 +45,4 @@ class AndroidGlobalIssueRegistry : IssueRegistry() { feedbackUrl = "http://b/issues/new?component=315013", contact = "repsonsible-apis@google.com" ) -}
\ No newline at end of file +} diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt index 0a66cd510353..a74400d3c0b3 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt @@ -30,31 +30,34 @@ 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.google.android.lint.findCallExpression import com.intellij.psi.PsiAnnotation import com.intellij.psi.PsiArrayInitializerMemberValue import com.intellij.psi.PsiClass import com.intellij.psi.PsiElement import com.intellij.psi.PsiMethod -import org.jetbrains.uast.UAnnotation +import org.jetbrains.uast.UBlockExpression +import org.jetbrains.uast.UDeclarationsExpression import org.jetbrains.uast.UElement +import org.jetbrains.uast.UExpression import org.jetbrains.uast.UMethod -import org.jetbrains.uast.toUElement +import org.jetbrains.uast.skipParenthesizedExprDown import java.util.EnumSet /** - * Lint Detector that ensures that any method overriding a method annotated - * with @EnforcePermission is also annotated with the exact same annotation. - * The intent is to surface the effective permission checks to the service - * implementations. + * Lint Detector that ensures consistency when using the @EnforcePermission + * annotation. Multiple verifications are implemented: * - * This is done with 2 mechanisms: * 1. Visit any annotation usage, to ensure that any derived class will have - * the correct annotation on each methods. This is for the top to bottom - * propagation. - * 2. Visit any annotation, to ensure that if a method is annotated, it has + * the correct annotation on each methods. Even if the subclass does not + * have the annotation, visitAnnotationUsage will be called which allows us + * to capture the issue. + * 2. Visit any method, to ensure that if a method is annotated, it has * its ancestor also annotated. This is to avoid having an annotation on a * Java method without the corresponding annotation on the AIDL interface. + * 3. When annotated, ensures that the first instruction is to call the helper + * method (or the parent helper). */ class EnforcePermissionDetector : Detector(), SourceCodeScanner { @@ -62,9 +65,8 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner { return listOf(ANNOTATION_ENFORCE_PERMISSION) } - override fun getApplicableUastTypes(): List<Class<out UElement>> { - return listOf(UAnnotation::class.java) - } + override fun getApplicableUastTypes(): List<Class<out UElement?>> = + listOf(UMethod::class.java) private fun annotationValueGetChildren(elem: PsiElement): Array<PsiElement> { if (elem is PsiArrayInitializerMemberValue) @@ -123,11 +125,6 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner { overriddenMethod: PsiMethod, checkEquivalence: Boolean = true ) { - // If method is not from a Stub subclass, this method shouldn't use @EP at all. - // This is handled by EnforcePermissionHelperDetector. - if (!isContainedInSubclassOfStub(context, overridingMethod.toUElement() as? UMethod)) { - return - } val overridingAnnotation = overridingMethod.getAnnotation(ANNOTATION_ENFORCE_PERMISSION) val overriddenAnnotation = overriddenMethod.getAnnotation(ANNOTATION_ENFORCE_PERMISSION) val location = context.getLocation(element) @@ -163,40 +160,102 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner { ) { if (usageInfo.type == AnnotationUsageType.METHOD_OVERRIDE && annotationInfo.origin == AnnotationOrigin.METHOD) { + /* Ignore implementations that are not a sub-class of Stub (i.e., Proxy). */ + val uMethod = element as? UMethod ?: return + if (!isContainedInSubclassOfStub(context, uMethod)) { + return + } val overridingMethod = element.sourcePsi as PsiMethod val overriddenMethod = usageInfo.referenced as PsiMethod compareMethods(context, element, overridingMethod, overriddenMethod) } } - override fun createUastHandler(context: JavaContext): UElementHandler { - return object : UElementHandler() { - override fun visitAnnotation(node: UAnnotation) { - if (node.qualifiedName != ANNOTATION_ENFORCE_PERMISSION) { - return - } - val method = node.uastParent as? UMethod ?: return - val overridingMethod = method as PsiMethod - val parents = overridingMethod.findSuperMethods() - for (overriddenMethod in parents) { - // The equivalence check can be skipped, if both methods are - // annotated, it will be verified by visitAnnotationUsage. - compareMethods(context, method, overridingMethod, - overriddenMethod, checkEquivalence = false) - } + override fun createUastHandler(context: JavaContext): UElementHandler = AidlStubHandler(context) + + private inner class AidlStubHandler(val context: JavaContext) : UElementHandler() { + override fun visitMethod(node: UMethod) { + if (context.evaluator.isAbstract(node)) return + if (!node.hasAnnotation(ANNOTATION_ENFORCE_PERMISSION)) return + + if (!isContainedInSubclassOfStub(context, node)) { + context.report( + ISSUE_MISUSING_ENFORCE_PERMISSION, + node, + context.getLocation(node), + "The class of ${node.name} does not inherit from an AIDL generated Stub class" + ) + return + } + + /* Check that we are connected to the super class */ + val overridingMethod = node as PsiMethod + val parents = overridingMethod.findSuperMethods() + for (overriddenMethod in parents) { + // The equivalence check can be skipped, if both methods are + // annotated, it will be verified by visitAnnotationUsage. + compareMethods(context, node, overridingMethod, + overriddenMethod, checkEquivalence = false) + } + + /* Check that the helper is called as a first instruction */ + val targetExpression = getHelperMethodCallSourceString(node) + val message = + "Method must start with $targetExpression or super.${node.name}(), if applicable" + + val firstExpression = (node.uastBody as? UBlockExpression) + ?.expressions?.firstOrNull() + + if (firstExpression == null) { + context.report( + ISSUE_ENFORCE_PERMISSION_HELPER, + context.getLocation(node), + message, + ) + return + } + + val firstExpressionSource = firstExpression.skipParenthesizedExprDown() + .asSourceString() + .filterNot(Char::isWhitespace) + + if (firstExpressionSource != targetExpression && + firstExpressionSource != "super.$targetExpression") { + // calling super.<methodName>() is also legal + val directSuper = context.evaluator.getSuperMethod(node) + val firstCall = findCallExpression(firstExpression)?.resolve() + if (directSuper != null && firstCall == directSuper) return + + val locationTarget = getLocationTarget(firstExpression) + val expressionLocation = context.getLocation(locationTarget) + + context.report( + ISSUE_ENFORCE_PERMISSION_HELPER, + context.getLocation(node), + message, + getHelperMethodFix(node, expressionLocation), + ) } } } companion object { + + private const val HELPER_SUFFIX = "_enforcePermission" + val EXPLANATION = """ - The @EnforcePermission annotation is used to indicate that the underlying binder code - has already verified the caller's permissions before calling the appropriate method. The - verification code is usually generated by the AIDL compiler, which also takes care of - annotating the generated Java code. + The @EnforcePermission annotation is used to delegate the verification of the caller's + permissions to a generated AIDL method. In order to surface that information to platform developers, the same annotation must be used on the implementation class or methods. + + The @EnforcePermission annotation can only be used on methods whose class extends from + the Stub class generated by the AIDL compiler. When @EnforcePermission is applied, the + AIDL compiler generates a Stub method to do the permission check called yourMethodName$HELPER_SUFFIX. + + yourMethodName$HELPER_SUFFIX must be executed before any other operation. To do that, you can + either call it directly, or call it indirectly via super.yourMethodName(). """ val ISSUE_MISSING_ENFORCE_PERMISSION: Issue = Issue.create( @@ -224,5 +283,44 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner { EnumSet.of(Scope.JAVA_FILE, Scope.TEST_SOURCES) ) ) + + val ISSUE_ENFORCE_PERMISSION_HELPER: Issue = Issue.create( + id = "MissingEnforcePermissionHelper", + briefDescription = """Missing permission-enforcing method call in AIDL method + |annotated with @EnforcePermission""".trimMargin(), + explanation = EXPLANATION, + category = Category.SECURITY, + priority = 6, + severity = Severity.ERROR, + implementation = Implementation( + EnforcePermissionDetector::class.java, + EnumSet.of(Scope.JAVA_FILE, Scope.TEST_SOURCES) + ) + ) + + val ISSUE_MISUSING_ENFORCE_PERMISSION: Issue = Issue.create( + id = "MisusingEnforcePermissionAnnotation", + briefDescription = "@EnforcePermission cannot be used here", + explanation = EXPLANATION, + category = Category.SECURITY, + priority = 6, + severity = Severity.ERROR, + implementation = Implementation( + EnforcePermissionDetector::class.java, + EnumSet.of(Scope.JAVA_FILE, Scope.TEST_SOURCES) + ) + ) + + /** + * handles an edge case with UDeclarationsExpression, where sourcePsi is null, + * resulting in an incorrect Location if used directly + */ + private fun getLocationTarget(firstExpression: UExpression): PsiElement? { + if (firstExpression.sourcePsi != null) return firstExpression.sourcePsi + if (firstExpression is UDeclarationsExpression) { + return firstExpression.declarations.firstOrNull()?.sourcePsi + } + return null + } } } diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt deleted file mode 100644 index 758de4dfccf8..000000000000 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt +++ /dev/null @@ -1,151 +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 com.google.android.lint.findCallExpression -import com.intellij.psi.PsiElement -import org.jetbrains.uast.UBlockExpression -import org.jetbrains.uast.UDeclarationsExpression -import org.jetbrains.uast.UElement -import org.jetbrains.uast.UExpression -import org.jetbrains.uast.UMethod -import org.jetbrains.uast.skipParenthesizedExprDown - -import java.util.EnumSet - -class EnforcePermissionHelperDetector : 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) { - if (context.evaluator.isAbstract(node)) return - if (!node.hasAnnotation(ANNOTATION_ENFORCE_PERMISSION)) return - - if (!isContainedInSubclassOfStub(context, node)) { - context.report( - ISSUE_MISUSING_ENFORCE_PERMISSION, - node, - context.getLocation(node), - "The class of ${node.name} does not inherit from an AIDL generated Stub class" - ) - return - } - - val targetExpression = getHelperMethodCallSourceString(node) - val message = - "Method must start with $targetExpression or super.${node.name}(), if applicable" - - val firstExpression = (node.uastBody as? UBlockExpression) - ?.expressions?.firstOrNull() - - if (firstExpression == null) { - context.report( - ISSUE_ENFORCE_PERMISSION_HELPER, - context.getLocation(node), - message, - ) - return - } - - val firstExpressionSource = firstExpression.skipParenthesizedExprDown() - .asSourceString() - .filterNot(Char::isWhitespace) - - if (firstExpressionSource != targetExpression && - firstExpressionSource != "super.$targetExpression") { - // calling super.<methodName>() is also legal - val directSuper = context.evaluator.getSuperMethod(node) - val firstCall = findCallExpression(firstExpression)?.resolve() - if (directSuper != null && firstCall == directSuper) return - - val locationTarget = getLocationTarget(firstExpression) - val expressionLocation = context.getLocation(locationTarget) - - context.report( - ISSUE_ENFORCE_PERMISSION_HELPER, - context.getLocation(node), - message, - getHelperMethodFix(node, expressionLocation), - ) - } - } - } - - companion object { - private const val HELPER_SUFFIX = "_enforcePermission" - - private const val EXPLANATION = """ - The @EnforcePermission annotation can only be used on methods whose class extends from - the Stub class generated by the AIDL compiler. When @EnforcePermission is applied, the - AIDL compiler generates a Stub method to do the permission check called yourMethodName$HELPER_SUFFIX. - - yourMethodName$HELPER_SUFFIX must be executed before any other operation. To do that, you can - either call it directly, or call it indirectly via super.yourMethodName(). - """ - - val ISSUE_ENFORCE_PERMISSION_HELPER: Issue = Issue.create( - id = "MissingEnforcePermissionHelper", - briefDescription = """Missing permission-enforcing method call in AIDL method - |annotated with @EnforcePermission""".trimMargin(), - explanation = EXPLANATION, - category = Category.SECURITY, - priority = 6, - severity = Severity.ERROR, - implementation = Implementation( - EnforcePermissionHelperDetector::class.java, - EnumSet.of(Scope.JAVA_FILE, Scope.TEST_SOURCES) - ) - ) - - val ISSUE_MISUSING_ENFORCE_PERMISSION: Issue = Issue.create( - id = "MisusingEnforcePermissionAnnotation", - briefDescription = "@EnforcePermission cannot be used here", - explanation = EXPLANATION, - category = Category.SECURITY, - priority = 6, - severity = Severity.ERROR, - implementation = Implementation( - EnforcePermissionHelperDetector::class.java, - EnumSet.of(Scope.JAVA_FILE, Scope.TEST_SOURCES) - ) - ) - - /** - * handles an edge case with UDeclarationsExpression, where sourcePsi is null, - * resulting in an incorrect Location if used directly - */ - private fun getLocationTarget(firstExpression: UExpression): PsiElement? { - if (firstExpression.sourcePsi != null) return firstExpression.sourcePsi - if (firstExpression is UDeclarationsExpression) { - return firstExpression.declarations.firstOrNull()?.sourcePsi - } - return null - } - } -} diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorCodegenTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorCodegenTest.kt index 5a63bb4084d2..3ef02f865355 100644 --- a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorCodegenTest.kt +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorCodegenTest.kt @@ -25,10 +25,10 @@ import com.android.tools.lint.detector.api.Issue @Suppress("UnstableApiUsage") class EnforcePermissionHelperDetectorCodegenTest : LintDetectorTest() { - override fun getDetector(): Detector = EnforcePermissionHelperDetector() + override fun getDetector(): Detector = EnforcePermissionDetector() override fun getIssues(): List<Issue> = listOf( - EnforcePermissionHelperDetector.ISSUE_ENFORCE_PERMISSION_HELPER + EnforcePermissionDetector.ISSUE_ENFORCE_PERMISSION_HELPER ) override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt index 10a6e1da91dc..64e2bfbad7bb 100644 --- a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt @@ -20,10 +20,10 @@ import com.android.tools.lint.checks.infrastructure.LintDetectorTest import com.android.tools.lint.checks.infrastructure.TestLintTask class EnforcePermissionHelperDetectorTest : LintDetectorTest() { - override fun getDetector() = EnforcePermissionHelperDetector() + override fun getDetector() = EnforcePermissionDetector() override fun getIssues() = listOf( - EnforcePermissionHelperDetector.ISSUE_ENFORCE_PERMISSION_HELPER, - EnforcePermissionHelperDetector.ISSUE_MISUSING_ENFORCE_PERMISSION + EnforcePermissionDetector.ISSUE_ENFORCE_PERMISSION_HELPER, + EnforcePermissionDetector.ISSUE_MISUSING_ENFORCE_PERMISSION ) override fun lint(): TestLintTask = super.lint().allowMissingSdk() |