From 635b108cc670858f5b0237813cdf42e93588633a Mon Sep 17 00:00:00 2001 From: Azhara Assanova Date: Thu, 24 Nov 2022 10:32:11 +0000 Subject: [PATCH] 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 --- .../lint/AndroidFrameworkIssueRegistry.kt | 4 +- .../lint/aidl/AidlImplementationDetector.kt | 52 ++++++ .../aidl/ManualPermissionCheckDetector.kt | 158 ------------------ ...mpleManualPermissionEnforcementDetector.kt | 150 +++++++++++++++++ ...anualPermissionEnforcementDetectorTest.kt} | 20 +-- 5 files changed, 214 insertions(+), 170 deletions(-) create mode 100644 tools/lint/checks/src/main/java/com/google/android/lint/aidl/AidlImplementationDetector.kt delete mode 100644 tools/lint/checks/src/main/java/com/google/android/lint/aidl/ManualPermissionCheckDetector.kt create mode 100644 tools/lint/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt rename tools/lint/checks/src/test/java/com/google/android/lint/aidl/{ManualPermissionCheckDetectorTest.kt => SimpleManualPermissionEnforcementDetectorTest.kt} (94%) 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 741655b402f26..413e19717d50b 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 @@ -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, diff --git 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 new file mode 100644 index 0000000000000..227cdcdc2fec2 --- /dev/null +++ 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> = + 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, + ) +} diff --git 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 deleted file mode 100644 index 2c53f390128c5..0000000000000 --- a/tools/lint/checks/src/main/java/com/google/android/lint/aidl/ManualPermissionCheckDetector.kt +++ /dev/null @@ -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> = - 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() - 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 - ) - } -} diff --git 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 new file mode 100644 index 0000000000000..4c0cbe7b3adf8 --- /dev/null +++ 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() + 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 + ) + } +} diff --git a/tools/lint/checks/src/test/java/com/google/android/lint/aidl/ManualPermissionCheckDetectorTest.kt b/tools/lint/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt similarity index 94% rename from tools/lint/checks/src/test/java/com/google/android/lint/aidl/ManualPermissionCheckDetectorTest.kt rename to tools/lint/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt index d4a34979ed9f7..150fc264506f4 100644 --- a/tools/lint/checks/src/test/java/com/google/android/lint/aidl/ManualPermissionCheckDetectorTest.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 = 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