diff options
| author | 2023-03-21 17:27:44 -0700 | |
|---|---|---|
| committer | 2023-03-21 17:27:44 -0700 | |
| commit | d88a18995cb026d780f9cdbace906f108517c37a (patch) | |
| tree | ba5f689f570bfdd5dd26bed899d0f1becae47072 | |
| parent | a9524e206f844b733082caadee3ba33c2854e018 (diff) | |
Update lint checks to their state on internal master
GlobalLintChecker was added as a prebuilt already on aosp, but we
could make it a source build if we had the source on aosp.
Bug: 264451752
Test: Presubmits
Change-Id: Iea5e5d2adef8c261490e14269cf6737c2a369e6d
38 files changed, 7134 insertions, 651 deletions
diff --git a/tools/lint/README.md b/tools/lint/README.md index b534b62cb395..b235ad60c799 100644 --- a/tools/lint/README.md +++ b/tools/lint/README.md @@ -1,15 +1,44 @@ -# Android Framework Lint Checker +# Android Lint Checks for AOSP -Custom lint checks written here are going to be executed for modules that opt in to those (e.g. any +Custom Android Lint checks are written here to be executed against java modules +in AOSP. These checks are broken down into two subdirectories: + +1. [Global Checks](#android-global-lint-checker) +2. [Framework Checks](#android-framework-lint-checker) + +# [Android Global Lint Checker](/global) +Checks written here are executed for the entire tree. The `AndroidGlobalLintChecker` +build target produces a jar file that is included in the overall build output +(`AndroidGlobalLintChecker.jar`). This file is then downloaded as a prebuilt under the +`prebuilts/cmdline-tools` subproject, and included by soong with all invocations of lint. + +## How to add new global lint checks +1. Write your detector with its issues and put it into + `global/checks/src/main/java/com/google/android/lint`. +2. Add your detector's issues into `AndroidGlobalIssueRegistry`'s `issues` + field. +3. Write unit tests for your detector in one file and put it into + `global/checks/test/java/com/google/android/lint`. +4. Have your change reviewed and merged. Once your change is merged, + obtain a build number from a successful build that includes your change. +5. Run `prebuilts/cmdline-tools/update-android-global-lint-checker.sh + <build_number>`. The script will create a commit that you can upload for + approval to the `prebuilts/cmdline-tools` subproject. +6. Done! Your lint check should be applied in lint report builds across the + entire tree! + +# [Android Framework Lint Checker](/framework) + +Checks written here are going to be executed for modules that opt in to those (e.g. any `services.XXX` module) and results will be automatically reported on CLs on gerrit. -## How to add new lint checks +## How to add new framework lint checks 1. Write your detector with its issues and put it into - `checks/src/main/java/com/google/android/lint`. + `framework/checks/src/main/java/com/google/android/lint`. 2. Add your detector's issues into `AndroidFrameworkIssueRegistry`'s `issues` field. 3. Write unit tests for your detector in one file and put it into - `checks/test/java/com/google/android/lint`. + `framework/checks/test/java/com/google/android/lint`. 4. Done! Your lint checks should be applied in lint report builds for modules that include `AndroidFrameworkLintChecker`. @@ -44,7 +73,11 @@ m out/soong/.intermediates/frameworks/base/services/autofill/services.autofill/a environment variable with the id of the lint. For example: `ANDROID_LINT_CHECK=UnusedTokenOfOriginalCallingIdentity m out/[...]/lint-report.html` -## Create or update a baseline +# How to apply automatic fixes suggested by lint + +See [lint_fix](fix/README.md) + +# Create or update a baseline Baseline files can be used to silence known errors (and warnings) that are deemed to be safe. When there is a lint-baseline.xml file in the root folder of the java library, soong will @@ -75,9 +108,10 @@ locally change the soong code in [lint.go](http://cs/aosp-master/build/soong/java/lint.go;l=451;rcl=2e778d5bc4a8d1d77b4f4a3029a4a254ad57db75) adding `cmd.Flag("--nowarn")` and running lint again. -## Documentation +# Documentation - [Android Lint Docs](https://googlesamples.github.io/android-custom-lint-rules/) +- [Lint Check Unit Testing](https://googlesamples.github.io/android-custom-lint-rules/api-guide/unit-testing.md.html) - [Android Lint source files](https://source.corp.google.com/studio-main/tools/base/lint/libs/lint-api/src/main/java/com/android/tools/lint/) - [PSI source files](https://github.com/JetBrains/intellij-community/tree/master/java/java-psi-api/src/com/intellij/psi) - [UAST source files](https://upsource.jetbrains.com/idea-ce/structure/idea-ce-7b9b8cc138bbd90aec26433f82cd2c6838694003/uast/uast-common/src/org/jetbrains/uast) diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/EnforcePermissionDetector.kt b/tools/lint/checks/src/main/java/com/google/android/lint/EnforcePermissionDetector.kt deleted file mode 100644 index 8f553abfee31..000000000000 --- a/tools/lint/checks/src/main/java/com/google/android/lint/EnforcePermissionDetector.kt +++ /dev/null @@ -1,177 +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 - -import com.android.tools.lint.detector.api.AnnotationInfo -import com.android.tools.lint.detector.api.AnnotationOrigin -import com.android.tools.lint.detector.api.AnnotationUsageInfo -import com.android.tools.lint.detector.api.AnnotationUsageType -import com.android.tools.lint.detector.api.ConstantEvaluator -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.intellij.psi.PsiAnnotation -import com.intellij.psi.PsiClass -import com.intellij.psi.PsiMethod -import org.jetbrains.uast.UElement - -/** - * 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. - */ -class EnforcePermissionDetector : Detector(), SourceCodeScanner { - - val ENFORCE_PERMISSION = "android.annotation.EnforcePermission" - - override fun applicableAnnotations(): List<String> { - return listOf(ENFORCE_PERMISSION) - } - - private fun areAnnotationsEquivalent( - context: JavaContext, - anno1: PsiAnnotation, - anno2: PsiAnnotation - ): Boolean { - if (anno1.qualifiedName != anno2.qualifiedName) { - return false - } - val attr1 = anno1.parameterList.attributes - val attr2 = anno2.parameterList.attributes - if (attr1.size != attr2.size) { - return false - } - for (i in attr1.indices) { - if (attr1[i].name != attr2[i].name) { - return false - } - val value1 = attr1[i].value - val value2 = attr2[i].value - if (value1 == null && value2 == null) { - continue - } - if (value1 == null || value2 == null) { - return false - } - val v1 = ConstantEvaluator.evaluate(context, value1) - val v2 = ConstantEvaluator.evaluate(context, value2) - if (v1 != v2) { - return false - } - } - return true - } - - override fun visitAnnotationUsage( - context: JavaContext, - element: UElement, - annotationInfo: AnnotationInfo, - usageInfo: AnnotationUsageInfo - ) { - if (usageInfo.type == AnnotationUsageType.EXTENDS) { - val newClass = element.sourcePsi?.parent?.parent as PsiClass - val extendedClass: PsiClass = usageInfo.referenced as PsiClass - val newAnnotation = newClass.getAnnotation(ENFORCE_PERMISSION) - val extendedAnnotation = extendedClass.getAnnotation(ENFORCE_PERMISSION)!! - - val location = context.getLocation(element) - val newClassName = newClass.qualifiedName - val extendedClassName = extendedClass.qualifiedName - if (newAnnotation == null) { - val msg = "The class $newClassName extends the class $extendedClassName which " + - "is annotated with @EnforcePermission. The same annotation must be used " + - "on $newClassName." - context.report(ISSUE_MISSING_ENFORCE_PERMISSION, element, location, msg) - } else if (!areAnnotationsEquivalent(context, newAnnotation, extendedAnnotation)) { - val msg = "The class $newClassName is annotated with ${newAnnotation.text} " + - "which differs from the parent class $extendedClassName: " + - "${extendedAnnotation.text}. The same annotation must be used for " + - "both classes." - context.report(ISSUE_MISMATCHING_ENFORCE_PERMISSION, element, location, msg) - } - } else if (usageInfo.type == AnnotationUsageType.METHOD_OVERRIDE && - annotationInfo.origin == AnnotationOrigin.METHOD) { - val overridingMethod = element.sourcePsi as PsiMethod - val overriddenMethod = usageInfo.referenced as PsiMethod - val overridingAnnotation = overridingMethod.getAnnotation(ENFORCE_PERMISSION) - val overriddenAnnotation = overriddenMethod.getAnnotation(ENFORCE_PERMISSION)!! - - val location = context.getLocation(element) - val overridingClass = overridingMethod.parent as PsiClass - val overriddenClass = overriddenMethod.parent as PsiClass - val overridingName = "${overridingClass.name}.${overridingMethod.name}" - val overriddenName = "${overriddenClass.name}.${overriddenMethod.name}" - if (overridingAnnotation == null) { - val msg = "The method $overridingName overrides the method $overriddenName which " + - "is annotated with @EnforcePermission. The same annotation must be used " + - "on $overridingName" - context.report(ISSUE_MISSING_ENFORCE_PERMISSION, element, location, msg) - } else if (!areAnnotationsEquivalent( - context, overridingAnnotation, overriddenAnnotation)) { - val msg = "The method $overridingName is annotated with " + - "${overridingAnnotation.text} which differs from the overridden " + - "method $overriddenName: ${overriddenAnnotation.text}. The same " + - "annotation must be used for both methods." - context.report(ISSUE_MISMATCHING_ENFORCE_PERMISSION, element, location, msg) - } - } - } - - companion object { - 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. - - In order to surface that information to platform developers, the same annotation must be - used on the implementation class or methods. - """ - - val ISSUE_MISSING_ENFORCE_PERMISSION: Issue = Issue.create( - id = "MissingEnforcePermissionAnnotation", - briefDescription = "Missing @EnforcePermission annotation on Binder method", - explanation = EXPLANATION, - category = Category.SECURITY, - priority = 6, - severity = Severity.ERROR, - implementation = Implementation( - EnforcePermissionDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) - ) - - val ISSUE_MISMATCHING_ENFORCE_PERMISSION: Issue = Issue.create( - id = "MismatchingEnforcePermissionAnnotation", - briefDescription = "Incorrect @EnforcePermission annotation on Binder method", - explanation = EXPLANATION, - category = Category.SECURITY, - priority = 6, - severity = Severity.ERROR, - implementation = Implementation( - EnforcePermissionDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) - ) - } -} diff --git a/tools/lint/checks/src/test/java/com/google/android/lint/EnforcePermissionDetectorTest.kt b/tools/lint/checks/src/test/java/com/google/android/lint/EnforcePermissionDetectorTest.kt deleted file mode 100644 index f5f4ebee24e0..000000000000 --- a/tools/lint/checks/src/test/java/com/google/android/lint/EnforcePermissionDetectorTest.kt +++ /dev/null @@ -1,202 +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 - -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 - -@Suppress("UnstableApiUsage") -class EnforcePermissionDetectorTest : LintDetectorTest() { - override fun getDetector(): Detector = EnforcePermissionDetector() - - override fun getIssues(): List<Issue> = listOf( - EnforcePermissionDetector.ISSUE_MISSING_ENFORCE_PERMISSION, - EnforcePermissionDetector.ISSUE_MISMATCHING_ENFORCE_PERMISSION - ) - - override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) - - fun testDoesNotDetectIssuesCorrectAnnotationOnClass() { - lint().files(java( - """ - package test.pkg; - @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) - public class TestClass1 extends IFoo.Stub { - public void testMethod() {} - } - """).indented(), - *stubs - ) - .run() - .expectClean() - } - - fun testDoesNotDetectIssuesCorrectAnnotationOnMethod() { - lint().files(java( - """ - package test.pkg; - import android.annotation.EnforcePermission; - public class TestClass2 extends IFooMethod.Stub { - @Override - @EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) - public void testMethod() {} - } - """).indented(), - *stubs - ) - .run() - .expectClean() - } - - fun testDetectIssuesMismatchingAnnotationOnClass() { - lint().files(java( - """ - package test.pkg; - @android.annotation.EnforcePermission(android.Manifest.permission.INTERNET) - public class TestClass3 extends IFoo.Stub { - public void testMethod() {} - } - """).indented(), - *stubs - ) - .run() - .expect("""src/test/pkg/TestClass3.java:3: Error: The class test.pkg.TestClass3 is \ -annotated with @android.annotation.EnforcePermission(android.Manifest.permission.INTERNET) \ -which differs from the parent class IFoo.Stub: \ -@android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE). The \ -same annotation must be used for both classes. [MismatchingEnforcePermissionAnnotation] -public class TestClass3 extends IFoo.Stub { - ~~~~~~~~~ -1 errors, 0 warnings""".addLineContinuation()) - } - - fun testDetectIssuesMismatchingAnnotationOnMethod() { - lint().files(java( - """ - package test.pkg; - public class TestClass4 extends IFooMethod.Stub { - @android.annotation.EnforcePermission(android.Manifest.permission.INTERNET) - public void testMethod() {} - } - """).indented(), - *stubs - ) - .run() - .expect("""src/test/pkg/TestClass4.java:4: Error: The method TestClass4.testMethod is \ -annotated with @android.annotation.EnforcePermission(android.Manifest.permission.INTERNET) \ -which differs from the overridden method Stub.testMethod: \ -@android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE). The same \ -annotation must be used for both methods. [MismatchingEnforcePermissionAnnotation] - public void testMethod() {} - ~~~~~~~~~~ -1 errors, 0 warnings""".addLineContinuation()) - } - - fun testDetectIssuesMissingAnnotationOnClass() { - lint().files(java( - """ - package test.pkg; - public class TestClass5 extends IFoo.Stub { - public void testMethod() {} - } - """).indented(), - *stubs - ) - .run() - .expect("""src/test/pkg/TestClass5.java:2: Error: The class test.pkg.TestClass5 extends \ -the class IFoo.Stub which is annotated with @EnforcePermission. The same annotation must be \ -used on test.pkg.TestClass5. [MissingEnforcePermissionAnnotation] -public class TestClass5 extends IFoo.Stub { - ~~~~~~~~~ -1 errors, 0 warnings""".addLineContinuation()) - } - - fun testDetectIssuesMissingAnnotationOnMethod() { - lint().files(java( - """ - package test.pkg; - public class TestClass6 extends IFooMethod.Stub { - public void testMethod() {} - } - """).indented(), - *stubs - ) - .run() - .expect("""src/test/pkg/TestClass6.java:3: Error: The method TestClass6.testMethod \ -overrides the method Stub.testMethod which is annotated with @EnforcePermission. The same \ -annotation must be used on TestClass6.testMethod [MissingEnforcePermissionAnnotation] - public void testMethod() {} - ~~~~~~~~~~ -1 errors, 0 warnings""".addLineContinuation()) - } - - /* Stubs */ - - private val interfaceIFooStub: TestFile = java( - """ - @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) - public interface IFoo { - @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) - public static abstract class Stub implements IFoo { - @Override - public void testMethod() {} - } - public void testMethod(); - } - """ - ).indented() - - private val interfaceIFooMethodStub: TestFile = java( - """ - public interface IFooMethod { - public static abstract class Stub implements IFooMethod { - @Override - @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) - public void testMethod() {} - } - @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) - public void testMethod(); - } - """ - ).indented() - - private val manifestPermissionStub: TestFile = java( - """ - package android.Manifest; - class permission { - public static final String READ_PHONE_STATE = "android.permission.READ_PHONE_STATE"; - public static final String INTERNET = "android.permission.INTERNET"; - } - """ - ).indented() - - private val enforcePermissionAnnotationStub: TestFile = java( - """ - package android.annotation; - public @interface EnforcePermission {} - """ - ).indented() - - private val stubs = arrayOf(interfaceIFooStub, interfaceIFooMethodStub, - manifestPermissionStub, enforcePermissionAnnotationStub) - - // Substitutes "backslash + new line" with an empty string to imitate line continuation - private fun String.addLineContinuation(): String = this.trimIndent().replace("\\\n", "") -} diff --git a/tools/lint/common/Android.bp b/tools/lint/common/Android.bp new file mode 100644 index 000000000000..898f88b8759c --- /dev/null +++ b/tools/lint/common/Android.bp @@ -0,0 +1,29 @@ +// 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 { + // See: http://go/android-license-faq + // A large-scale-change added 'default_applicable_licenses' to import + // all of the 'license_kinds' from "frameworks_base_license" + // to get the below license kinds: + // SPDX-license-identifier-Apache-2.0 + default_applicable_licenses: ["frameworks_base_license"], +} + +java_library_host { + name: "AndroidCommonLint", + srcs: ["src/main/java/**/*.kt"], + libs: ["lint_api"], + kotlincflags: ["-Xjvm-default=all"], +} diff --git a/tools/lint/common/src/main/java/com/google/android/lint/Constants.kt b/tools/lint/common/src/main/java/com/google/android/lint/Constants.kt new file mode 100644 index 000000000000..0ef165f1523b --- /dev/null +++ b/tools/lint/common/src/main/java/com/google/android/lint/Constants.kt @@ -0,0 +1,40 @@ +/* + * 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.google.android.lint.model.Method + +const val CLASS_STUB = "Stub" +const val CLASS_CONTEXT = "android.content.Context" +const val CLASS_ACTIVITY_MANAGER_SERVICE = "com.android.server.am.ActivityManagerService" +const val CLASS_ACTIVITY_MANAGER_INTERNAL = "android.app.ActivityManagerInternal" + +// Enforce permission APIs +val ENFORCE_PERMISSION_METHODS = listOf( + Method(CLASS_CONTEXT, "checkPermission"), + Method(CLASS_CONTEXT, "checkCallingPermission"), + Method(CLASS_CONTEXT, "checkCallingOrSelfPermission"), + Method(CLASS_CONTEXT, "enforcePermission"), + Method(CLASS_CONTEXT, "enforceCallingPermission"), + Method(CLASS_CONTEXT, "enforceCallingOrSelfPermission"), + Method(CLASS_ACTIVITY_MANAGER_SERVICE, "checkPermission"), + Method(CLASS_ACTIVITY_MANAGER_INTERNAL, "enforceCallingPermission") +) + +const val ANNOTATION_PERMISSION_METHOD = "android.annotation.PermissionMethod" +const val ANNOTATION_PERMISSION_NAME = "android.annotation.PermissionName" +const val ANNOTATION_PERMISSION_RESULT = "android.content.pm.PackageManager.PermissionResult" diff --git a/tools/lint/common/src/main/java/com/google/android/lint/PermissionMethodUtils.kt b/tools/lint/common/src/main/java/com/google/android/lint/PermissionMethodUtils.kt new file mode 100644 index 000000000000..9a7f8fa53d87 --- /dev/null +++ b/tools/lint/common/src/main/java/com/google/android/lint/PermissionMethodUtils.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 + +import com.android.tools.lint.detector.api.getUMethod +import org.jetbrains.uast.UAnnotation +import org.jetbrains.uast.UCallExpression +import org.jetbrains.uast.UElement +import org.jetbrains.uast.UMethod +import org.jetbrains.uast.UParameter +import org.jetbrains.uast.UQualifiedReferenceExpression + +fun isPermissionMethodCall(callExpression: UCallExpression): Boolean { + val method = callExpression.resolve()?.getUMethod() ?: return false + return hasPermissionMethodAnnotation(method) +} + +fun hasPermissionMethodAnnotation(method: UMethod): Boolean = + getPermissionMethodAnnotation(method) != null + +fun getPermissionMethodAnnotation(method: UMethod?): UAnnotation? = method?.uAnnotations + ?.firstOrNull { it.qualifiedName == ANNOTATION_PERMISSION_METHOD } + +fun hasPermissionNameAnnotation(parameter: UParameter) = parameter.annotations.any { + it.hasQualifiedName(ANNOTATION_PERMISSION_NAME) +} + +/** + * Attempts to return a CallExpression from a QualifiedReferenceExpression (or returns it directly if passed directly) + * @param callOrReferenceCall expected to be UCallExpression or UQualifiedReferenceExpression + * @return UCallExpression, if available + */ +fun findCallExpression(callOrReferenceCall: UElement?): UCallExpression? = + when (callOrReferenceCall) { + is UCallExpression -> callOrReferenceCall + is UQualifiedReferenceExpression -> callOrReferenceCall.selector as? UCallExpression + else -> null + } diff --git a/tools/lint/common/src/main/java/com/google/android/lint/model/Method.kt b/tools/lint/common/src/main/java/com/google/android/lint/model/Method.kt new file mode 100644 index 000000000000..3939b6109eaa --- /dev/null +++ b/tools/lint/common/src/main/java/com/google/android/lint/model/Method.kt @@ -0,0 +1,26 @@ +/* + * 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.model + +/** + * Data class to represent a Method + */ +data class Method(val clazz: String, val name: String) { + override fun toString(): String { + return "$clazz#$name" + } +} diff --git a/tools/lint/fix/Android.bp b/tools/lint/fix/Android.bp new file mode 100644 index 000000000000..43f21221ae5a --- /dev/null +++ b/tools/lint/fix/Android.bp @@ -0,0 +1,33 @@ +// 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 { + // See: http://go/android-license-faq + // A large-scale-change added 'default_applicable_licenses' to import + // all of the 'license_kinds' from "frameworks_base_license" + // to get the below license kinds: + // SPDX-license-identifier-Apache-2.0 + default_applicable_licenses: ["frameworks_base_license"], +} + +python_binary_host { + name: "lint_fix", + main: "soong_lint_fix.py", + srcs: ["soong_lint_fix.py"], +} + +python_library_host { + name: "soong_lint_fix", + srcs: ["soong_lint_fix.py"], +} diff --git a/tools/lint/fix/README.md b/tools/lint/fix/README.md new file mode 100644 index 000000000000..a5ac2be1c18a --- /dev/null +++ b/tools/lint/fix/README.md @@ -0,0 +1,30 @@ +# Refactoring the platform with lint +Inspiration: go/refactor-the-platform-with-lint\ +**Special Thanks: brufino@, azharaa@, for the prior work that made this all possible** + +## What is this? + +It's a python script that runs the framework linter, +and then (optionally) copies modified files back into the source tree.\ +Why python, you ask? Because python is cool ¯\_(ツ)_/¯. + +Incidentally, this exposes a much simpler way to run individual lint checks +against individual modules, so it's useful beyond applying fixes. + +## Why? + +Lint is not allowed to modify source files directly via lint's `--apply-suggestions` flag. +As a compromise, soong zips up the (potentially) modified sources and leaves them in an intermediate +directory. This script runs the lint, unpacks those files, and copies them back into the tree. + +## How do I run it? +**WARNING: You probably want to commit/stash any changes to your working tree before doing this...** + +``` +source build/envsetup.sh +lunch cf_x86_64_phone-userdebug # or any lunch target +m lint_fix +lint_fix -h +``` + +The script's help output explains things that are omitted here. diff --git a/tools/lint/fix/soong_lint_fix.py b/tools/lint/fix/soong_lint_fix.py new file mode 100644 index 000000000000..cd4d778d1dec --- /dev/null +++ b/tools/lint/fix/soong_lint_fix.py @@ -0,0 +1,173 @@ +# 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. + +import argparse +import json +import os +import shutil +import subprocess +import sys +import zipfile + +ANDROID_BUILD_TOP = os.environ.get("ANDROID_BUILD_TOP") +ANDROID_PRODUCT_OUT = os.environ.get("ANDROID_PRODUCT_OUT") +PRODUCT_OUT = ANDROID_PRODUCT_OUT.removeprefix(f"{ANDROID_BUILD_TOP}/") + +SOONG_UI = "build/soong/soong_ui.bash" +PATH_PREFIX = "out/soong/.intermediates" +PATH_SUFFIX = "android_common/lint" +FIX_ZIP = "suggested-fixes.zip" + +class SoongLintFix: + """ + This class creates a command line tool that will + apply lint fixes to the platform via the necessary + combination of soong and shell commands. + + It breaks up these operations into a few "private" methods + that are intentionally exposed so experimental code can tweak behavior. + + The entry point, `run`, will apply lint fixes using the + intermediate `suggested-fixes` directory that soong creates during its + invocation of lint. + + Basic usage: + ``` + from soong_lint_fix import SoongLintFix + + SoongLintFix().run() + ``` + """ + def __init__(self): + self._parser = _setup_parser() + self._args = None + self._kwargs = None + self._path = None + self._target = None + + + def run(self, additional_setup=None, custom_fix=None): + """ + Run the script + """ + self._setup() + self._find_module() + self._lint() + + if not self._args.no_fix: + self._fix() + + if self._args.print: + self._print() + + def _setup(self): + self._args = self._parser.parse_args() + env = os.environ.copy() + if self._args.check: + env["ANDROID_LINT_CHECK"] = self._args.check + if self._args.lint_module: + env["ANDROID_LINT_CHECK_EXTRA_MODULES"] = self._args.lint_module + + self._kwargs = { + "env": env, + "executable": "/bin/bash", + "shell": True, + } + + os.chdir(ANDROID_BUILD_TOP) + + + def _find_module(self): + print("Refreshing soong modules...") + try: + os.mkdir(ANDROID_PRODUCT_OUT) + except OSError: + pass + subprocess.call(f"{SOONG_UI} --make-mode {PRODUCT_OUT}/module-info.json", **self._kwargs) + print("done.") + + with open(f"{ANDROID_PRODUCT_OUT}/module-info.json") as f: + module_info = json.load(f) + + if self._args.module not in module_info: + sys.exit(f"Module {self._args.module} not found!") + + module_path = module_info[self._args.module]["path"][0] + print(f"Found module {module_path}/{self._args.module}.") + + self._path = f"{PATH_PREFIX}/{module_path}/{self._args.module}/{PATH_SUFFIX}" + self._target = f"{self._path}/lint-report.txt" + + + def _lint(self): + print("Cleaning up any old lint results...") + try: + os.remove(f"{self._target}") + os.remove(f"{self._path}/{FIX_ZIP}") + except FileNotFoundError: + pass + print("done.") + + print(f"Generating {self._target}") + subprocess.call(f"{SOONG_UI} --make-mode {self._target}", **self._kwargs) + print("done.") + + + def _fix(self): + print("Copying suggested fixes to the tree...") + with zipfile.ZipFile(f"{self._path}/{FIX_ZIP}") as zip: + for name in zip.namelist(): + if name.startswith("out") or not name.endswith(".java"): + continue + with zip.open(name) as src, open(f"{ANDROID_BUILD_TOP}/{name}", "wb") as dst: + shutil.copyfileobj(src, dst) + print("done.") + + + def _print(self): + print("### lint-report.txt ###", end="\n\n") + with open(self._target, "r") as f: + print(f.read()) + + +def _setup_parser(): + parser = argparse.ArgumentParser(description=""" + This is a python script that applies lint fixes to the platform: + 1. Set up the environment, etc. + 2. Run lint on the specified target. + 3. Copy the modified files, from soong's intermediate directory, back into the tree. + + **Gotcha**: You must have run `source build/envsetup.sh` and `lunch` first. + """, formatter_class=argparse.RawTextHelpFormatter) + + parser.add_argument('module', + help='The soong build module to run ' + '(e.g. framework-minus-apex or services.core.unboosted)') + + parser.add_argument('--check', + help='Which lint to run. Passed to the ANDROID_LINT_CHECK environment variable.') + + parser.add_argument('--lint-module', + help='Specific lint module to run. Passed to the ANDROID_LINT_CHECK_EXTRA_MODULES environment variable.') + + parser.add_argument('--no-fix', action='store_true', + help='Just build and run the lint, do NOT apply the fixes.') + + parser.add_argument('--print', action='store_true', + help='Print the contents of the generated lint-report.txt at the end.') + + return parser + +if __name__ == "__main__": + SoongLintFix().run()
\ No newline at end of file diff --git a/tools/lint/Android.bp b/tools/lint/framework/Android.bp index 260104145505..30a6daaef2a4 100644 --- a/tools/lint/Android.bp +++ b/tools/lint/framework/Android.bp @@ -1,4 +1,4 @@ -// Copyright (C) 2021 The Android Open Source Project +// 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. @@ -29,17 +29,14 @@ java_library_host { "auto_service_annotations", "lint_api", ], + static_libs: [ + "AndroidCommonLint", + ], kotlincflags: ["-Xjvm-default=all"], } java_test_host { name: "AndroidFrameworkLintCheckerTest", - // TODO(b/239881504): Since this test was written, Android - // Lint was updated, and now includes classes that were - // compiled for java 15. The soong build doesn't support - // java 15 yet, so we can't compile against "lint". Disable - // the test until java 15 is supported. - enabled: false, srcs: ["checks/src/test/java/**/*.kt"], static_libs: [ "AndroidFrameworkLintChecker", @@ -49,5 +46,19 @@ java_test_host { ], test_options: { unit_test: true, + tradefed_options: [ + { + // lint bundles in some classes that were built with older versions + // of libraries, and no longer load. Since tradefed tries to load + // all classes in the jar to look for tests, it crashes loading them. + // Exclude these classes from tradefed's search. + name: "exclude-paths", + value: "org/apache", + }, + { + name: "exclude-paths", + value: "META-INF", + }, + ], }, } diff --git a/tools/lint/framework/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt b/tools/lint/framework/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt new file mode 100644 index 000000000000..b4f6a1c12d8f --- /dev/null +++ b/tools/lint/framework/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt @@ -0,0 +1,54 @@ +/* + * Copyright (C) 2021 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.IssueRegistry +import com.android.tools.lint.client.api.Vendor +import com.android.tools.lint.detector.api.CURRENT_API +import com.google.android.lint.parcel.SaferParcelChecker +import com.google.auto.service.AutoService + +@AutoService(IssueRegistry::class) +@Suppress("UnstableApiUsage") +class AndroidFrameworkIssueRegistry : IssueRegistry() { + override val issues = listOf( + CallingIdentityTokenDetector.ISSUE_UNUSED_TOKEN, + CallingIdentityTokenDetector.ISSUE_NON_FINAL_TOKEN, + CallingIdentityTokenDetector.ISSUE_NESTED_CLEAR_IDENTITY_CALLS, + CallingIdentityTokenDetector.ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, + CallingIdentityTokenDetector.ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, + CallingIdentityTokenDetector.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY, + CallingIdentityTokenDetector.ISSUE_RESULT_OF_CLEAR_IDENTITY_CALL_NOT_STORED_IN_VARIABLE, + CallingSettingsNonUserGetterMethodsDetector.ISSUE_NON_USER_GETTER_CALLED, + SaferParcelChecker.ISSUE_UNSAFE_API_USAGE, + PackageVisibilityDetector.ISSUE_PACKAGE_NAME_NO_PACKAGE_VISIBILITY_FILTERS, + PermissionMethodDetector.ISSUE_PERMISSION_METHOD_USAGE, + PermissionMethodDetector.ISSUE_CAN_BE_PERMISSION_METHOD, + ) + + override val api: Int + get() = CURRENT_API + + override val minApi: Int + get() = 8 + + override val vendor: Vendor = Vendor( + vendorName = "Android", + feedbackUrl = "http://b/issues/new?component=315013", + contact = "brufino@google.com" + ) +} diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt b/tools/lint/framework/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt index 930378b168b2..0c375c358e61 100644 --- a/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt +++ b/tools/lint/framework/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt @@ -33,6 +33,7 @@ import org.jetbrains.uast.UBlockExpression import org.jetbrains.uast.UCallExpression import org.jetbrains.uast.UDeclarationsExpression import org.jetbrains.uast.UElement +import org.jetbrains.uast.UIfExpression import org.jetbrains.uast.ULocalVariable import org.jetbrains.uast.USimpleNameReferenceExpression import org.jetbrains.uast.UTryExpression @@ -52,10 +53,10 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { private val tokensMap = mutableMapOf<String, Token>() override fun getApplicableUastTypes(): List<Class<out UElement?>> = - listOf(ULocalVariable::class.java, UCallExpression::class.java) + listOf(ULocalVariable::class.java, UCallExpression::class.java) override fun createUastHandler(context: JavaContext): UElementHandler = - TokenUastHandler(context) + TokenUastHandler(context) /** File analysis starts with a clear map */ override fun beforeCheckFile(context: Context) { @@ -70,9 +71,9 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { override fun afterCheckFile(context: Context) { for (token in tokensMap.values) { context.report( - ISSUE_UNUSED_TOKEN, - token.location, - getIncidentMessageUnusedToken(token.variableName) + ISSUE_UNUSED_TOKEN, + token.location, + getIncidentMessageUnusedToken(token.variableName) ) } tokensMap.clear() @@ -96,9 +97,9 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { val variableName = node.getName() if (!node.isFinal) { context.report( - ISSUE_NON_FINAL_TOKEN, - location, - getIncidentMessageNonFinalToken(variableName) + ISSUE_NON_FINAL_TOKEN, + location, + getIncidentMessageNonFinalToken(variableName) ) } // If there exists an unused variable with the same name in the map, we can imply that @@ -106,9 +107,9 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { val oldToken = tokensMap[variableName] if (oldToken != null) { context.report( - ISSUE_UNUSED_TOKEN, - oldToken.location, - getIncidentMessageUnusedToken(oldToken.variableName) + ISSUE_UNUSED_TOKEN, + oldToken.location, + getIncidentMessageUnusedToken(oldToken.variableName) ) } // If there exists a token in the same scope as the current new token, it means that @@ -117,56 +118,84 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { val firstCallToken = findFirstTokenInScope(node) if (firstCallToken != null) { context.report( - ISSUE_NESTED_CLEAR_IDENTITY_CALLS, - createNestedLocation(firstCallToken, location), - getIncidentMessageNestedClearIdentityCallsPrimary( - firstCallToken.variableName, - variableName - ) + ISSUE_NESTED_CLEAR_IDENTITY_CALLS, + createNestedLocation(firstCallToken, location), + getIncidentMessageNestedClearIdentityCallsPrimary( + firstCallToken.variableName, + variableName + ) ) } // If the next statement in the tree is not a try-finally statement, we need to report // the "clearCallingIdentity() is not followed by try-finally" issue val finallyClause = (getNextStatementOfLocalVariable(node) as? UTryExpression) - ?.finallyClause + ?.finallyClause if (finallyClause == null) { context.report( - ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY, - location, - getIncidentMessageClearIdentityCallNotFollowedByTryFinally(variableName) + ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY, + location, + getIncidentMessageClearIdentityCallNotFollowedByTryFinally(variableName) ) } tokensMap[variableName] = Token( - variableName, - node.sourcePsi?.getUseScope(), - location, - finallyClause + variableName, + node.sourcePsi?.getUseScope(), + location, + finallyClause ) } - /** - * For every method(): - * - Checks use of caller-aware methods issue - * For every call of Binder.restoreCallingIdentity(token): - * - Checks for restoreCallingIdentity() not in the finally block issue - * - Removes token from tokensMap if token is within the scope of the method - */ override fun visitCallExpression(node: UCallExpression) { + when { + isMethodCall(node, Method.BINDER_CLEAR_CALLING_IDENTITY) -> { + checkClearCallingIdentityCall(node) + } + isMethodCall(node, Method.BINDER_RESTORE_CALLING_IDENTITY) -> { + checkRestoreCallingIdentityCall(node) + } + isCallerAwareMethod(node) -> checkCallerAwareMethod(node) + } + } + + private fun checkClearCallingIdentityCall(node: UCallExpression) { + var firstNonQualifiedParent = getFirstNonQualifiedParent(node) + // if the call expression is inside a ternary, and the ternary is assigned + // to a variable, then we are still technically assigning + // any result of clearCallingIdentity to a variable + if (firstNonQualifiedParent is UIfExpression && firstNonQualifiedParent.isTernary) { + firstNonQualifiedParent = firstNonQualifiedParent.uastParent + } + if (firstNonQualifiedParent !is ULocalVariable) { + context.report( + ISSUE_RESULT_OF_CLEAR_IDENTITY_CALL_NOT_STORED_IN_VARIABLE, + context.getLocation(node), + getIncidentMessageResultOfClearIdentityCallNotStoredInVariable( + node.getQualifiedParentOrThis().asRenderString() + ) + ) + } + } + + private fun checkCallerAwareMethod(node: UCallExpression) { val token = findFirstTokenInScope(node) - if (isCallerAwareMethod(node) && token != null) { + if (token != null) { context.report( - ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, - context.getLocation(node), - getIncidentMessageUseOfCallerAwareMethodsWithClearedIdentity( - token.variableName, - node.asRenderString() - ) + ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, + context.getLocation(node), + getIncidentMessageUseOfCallerAwareMethodsWithClearedIdentity( + token.variableName, + node.asRenderString() + ) ) - return } - if (!isMethodCall(node, Method.BINDER_RESTORE_CALLING_IDENTITY)) return - val first = node.valueArguments[0].skipParenthesizedExprDown() - val arg = first as? USimpleNameReferenceExpression ?: return + } + + /** + * - Checks for restoreCallingIdentity() not in the finally block issue + * - Removes token from tokensMap if token is within the scope of the method + */ + private fun checkRestoreCallingIdentityCall(node: UCallExpression) { + val arg = node.valueArguments[0] as? USimpleNameReferenceExpression ?: return val variableName = arg.identifier val originalScope = tokensMap[variableName]?.scope ?: return val psi = arg.sourcePsi ?: return @@ -174,26 +203,31 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { // token declaration. If not within the scope, no action is needed because the token is // irrelevant i.e. not in the same scope or was not declared with clearCallingIdentity() if (!PsiSearchScopeUtil.isInScope(originalScope, psi)) return - // - We do not report "restore identity call not in finally" issue when there is no + // We do not report "restore identity call not in finally" issue when there is no // finally block because that case is already handled by "clear identity call not // followed by try-finally" issue - // - UCallExpression can be a child of UQualifiedReferenceExpression, i.e. - // receiver.selector, so to get the call's immediate parent we need to get the topmost - // parent qualified reference expression and access its parent if (tokensMap[variableName]?.finallyBlock != null && - skipParenthesizedExprUp(node.getQualifiedParentOrThis().uastParent) != - tokensMap[variableName]?.finallyBlock) { + getFirstNonQualifiedParent(node) != + tokensMap[variableName]?.finallyBlock + ) { context.report( - ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, - context.getLocation(node), - getIncidentMessageRestoreIdentityCallNotInFinallyBlock(variableName) + ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, + context.getLocation(node), + getIncidentMessageRestoreIdentityCallNotInFinallyBlock(variableName) ) } tokensMap.remove(variableName) } + private fun getFirstNonQualifiedParent(expression: UCallExpression): UElement? { + // UCallExpression can be a child of UQualifiedReferenceExpression, i.e. + // receiver.selector, so to get the call's immediate parent we need to get the topmost + // parent qualified reference expression and access its parent + return skipParenthesizedExprUp(expression.getQualifiedParentOrThis().uastParent) + } + private fun isCallerAwareMethod(expression: UCallExpression): Boolean = - callerAwareMethods.any { method -> isMethodCall(expression, method) } + callerAwareMethods.any { method -> isMethodCall(expression, method) } private fun isMethodCall( expression: UCallExpression, @@ -201,12 +235,12 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { ): Boolean { val psiMethod = expression.resolve() ?: return false return psiMethod.getName() == method.methodName && - context.evaluator.methodMatches( - psiMethod, - method.className, - /* allowInherit */ true, - *method.args - ) + context.evaluator.methodMatches( + psiMethod, + method.className, + /* allowInherit */ true, + *method.args + ) } /** @@ -255,7 +289,7 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { return declarations[indexInDeclarations + 1] } val enclosingBlock = node - .getParentOfType<UBlockExpression>(strict = true) ?: return null + .getParentOfType<UBlockExpression>(strict = true) ?: return null val expressions = enclosingBlock.expressions val indexInBlock = expressions.indexOf(declarationsExpression as UElement) return if (indexInBlock == -1) null else expressions.getOrNull(indexInBlock + 1) @@ -301,12 +335,12 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { secondCallTokenLocation: Location ): Location { return cloneLocation(secondCallTokenLocation) - .withSecondary( - cloneLocation(firstCallToken.location), - getIncidentMessageNestedClearIdentityCallsSecondary( - firstCallToken.variableName - ) + .withSecondary( + cloneLocation(firstCallToken.location), + getIncidentMessageNestedClearIdentityCallsSecondary( + firstCallToken.variableName ) + ) } private fun cloneLocation(location: Location): Location { @@ -347,20 +381,20 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { const val CLASS_USER_HANDLE = "android.os.UserHandle" private val callerAwareMethods = listOf( - Method.BINDER_GET_CALLING_PID, - Method.BINDER_GET_CALLING_UID, - Method.BINDER_GET_CALLING_UID_OR_THROW, - Method.BINDER_GET_CALLING_USER_HANDLE, - Method.USER_HANDLE_GET_CALLING_APP_ID, - Method.USER_HANDLE_GET_CALLING_USER_ID + Method.BINDER_GET_CALLING_PID, + Method.BINDER_GET_CALLING_UID, + Method.BINDER_GET_CALLING_UID_OR_THROW, + Method.BINDER_GET_CALLING_USER_HANDLE, + Method.USER_HANDLE_GET_CALLING_APP_ID, + Method.USER_HANDLE_GET_CALLING_USER_ID ) /** Issue: unused token from Binder.clearCallingIdentity() */ @JvmField val ISSUE_UNUSED_TOKEN: Issue = Issue.create( - id = "UnusedTokenOfOriginalCallingIdentity", - briefDescription = "Unused token of Binder.clearCallingIdentity()", - explanation = """ + id = "UnusedTokenOfOriginalCallingIdentity", + briefDescription = "Unused token of Binder.clearCallingIdentity()", + explanation = """ You cleared the original calling identity with \ `Binder.clearCallingIdentity()`, but have not used the returned token to \ restore the identity. @@ -370,26 +404,26 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { `token` is the result of `Binder.clearCallingIdentity()` """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) ) private fun getIncidentMessageUnusedToken(variableName: String) = "`$variableName` has " + - "not been used to restore the calling identity. Introduce a `try`-`finally` " + - "after the declaration and call `Binder.restoreCallingIdentity($variableName)` " + - "in `finally` or remove `$variableName`." + "not been used to restore the calling identity. Introduce a `try`-`finally` " + + "after the declaration and call `Binder.restoreCallingIdentity($variableName)` " + + "in `finally` or remove `$variableName`." /** Issue: non-final token from Binder.clearCallingIdentity() */ @JvmField val ISSUE_NON_FINAL_TOKEN: Issue = Issue.create( - id = "NonFinalTokenOfOriginalCallingIdentity", - briefDescription = "Non-final token of Binder.clearCallingIdentity()", - explanation = """ + id = "NonFinalTokenOfOriginalCallingIdentity", + briefDescription = "Non-final token of Binder.clearCallingIdentity()", + explanation = """ You cleared the original calling identity with \ `Binder.clearCallingIdentity()`, but have not made the returned token `final`. @@ -397,47 +431,47 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { which can cause problems when restoring the identity with \ `Binder.restoreCallingIdentity(token)`. """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) ) private fun getIncidentMessageNonFinalToken(variableName: String) = "`$variableName` is " + - "a non-final token from `Binder.clearCallingIdentity()`. Add `final` keyword to " + - "`$variableName`." + "a non-final token from `Binder.clearCallingIdentity()`. Add `final` keyword to " + + "`$variableName`." /** Issue: nested calls of Binder.clearCallingIdentity() */ @JvmField val ISSUE_NESTED_CLEAR_IDENTITY_CALLS: Issue = Issue.create( - id = "NestedClearCallingIdentityCalls", - briefDescription = "Nested calls of Binder.clearCallingIdentity()", - explanation = """ + id = "NestedClearCallingIdentityCalls", + briefDescription = "Nested calls of Binder.clearCallingIdentity()", + explanation = """ You cleared the original calling identity with \ `Binder.clearCallingIdentity()` twice without restoring identity with the \ result of the first call. Make sure to restore the identity after each clear identity call. """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) ) private fun getIncidentMessageNestedClearIdentityCallsPrimary( firstCallVariableName: String, secondCallVariableName: String ): String = "The calling identity has already been cleared and returned into " + - "`$firstCallVariableName`. Move `$secondCallVariableName` declaration after " + - "restoring the calling identity with " + - "`Binder.restoreCallingIdentity($firstCallVariableName)`." + "`$firstCallVariableName`. Move `$secondCallVariableName` declaration after " + + "restoring the calling identity with " + + "`Binder.restoreCallingIdentity($firstCallVariableName)`." private fun getIncidentMessageNestedClearIdentityCallsSecondary( firstCallVariableName: String @@ -446,10 +480,10 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { /** Issue: Binder.clearCallingIdentity() is not followed by `try-finally` statement */ @JvmField val ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY: Issue = Issue.create( - id = "ClearIdentityCallNotFollowedByTryFinally", - briefDescription = "Binder.clearCallingIdentity() is not followed by try-finally " + - "statement", - explanation = """ + id = "ClearIdentityCallNotFollowedByTryFinally", + briefDescription = "Binder.clearCallingIdentity() is not followed by try-finally " + + "statement", + explanation = """ You cleared the original calling identity with \ `Binder.clearCallingIdentity()`, but the next statement is not a `try` \ statement. @@ -472,30 +506,30 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { code with your identity that was originally intended to run with the calling \ application's identity. """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) ) private fun getIncidentMessageClearIdentityCallNotFollowedByTryFinally( variableName: String ): String = "You cleared the calling identity and returned the result into " + - "`$variableName`, but the next statement is not a `try`-`finally` statement. " + - "Define a `try`-`finally` block after `$variableName` declaration to ensure a " + - "safe restore of the calling identity by calling " + - "`Binder.restoreCallingIdentity($variableName)` and making it an immediate child " + - "of the `finally` block." + "`$variableName`, but the next statement is not a `try`-`finally` statement. " + + "Define a `try`-`finally` block after `$variableName` declaration to ensure a " + + "safe restore of the calling identity by calling " + + "`Binder.restoreCallingIdentity($variableName)` and making it an immediate child " + + "of the `finally` block." /** Issue: Binder.restoreCallingIdentity() is not in finally block */ @JvmField val ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK: Issue = Issue.create( - id = "RestoreIdentityCallNotInFinallyBlock", - briefDescription = "Binder.restoreCallingIdentity() is not in finally block", - explanation = """ + id = "RestoreIdentityCallNotInFinallyBlock", + briefDescription = "Binder.restoreCallingIdentity() is not in finally block", + explanation = """ You are restoring the original calling identity with \ `Binder.restoreCallingIdentity()`, but the call is not an immediate child of \ the `finally` block of the `try` statement. @@ -516,28 +550,28 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { `finally` block, you may run code with your identity that was originally \ intended to run with the calling application's identity. """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) ) private fun getIncidentMessageRestoreIdentityCallNotInFinallyBlock( variableName: String ): String = "`Binder.restoreCallingIdentity($variableName)` is not an immediate child of " + - "the `finally` block of the try statement after `$variableName` declaration. " + - "Surround the call with `finally` block and call it unconditionally." + "the `finally` block of the try statement after `$variableName` declaration. " + + "Surround the call with `finally` block and call it unconditionally." /** Issue: Use of caller-aware methods after Binder.clearCallingIdentity() */ @JvmField val ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY: Issue = Issue.create( - id = "UseOfCallerAwareMethodsWithClearedIdentity", - briefDescription = "Use of caller-aware methods after " + - "Binder.clearCallingIdentity()", - explanation = """ + id = "UseOfCallerAwareMethodsWithClearedIdentity", + briefDescription = "Use of caller-aware methods after " + + "Binder.clearCallingIdentity()", + explanation = """ You cleared the original calling identity with \ `Binder.clearCallingIdentity()`, but used one of the methods below before \ restoring the identity. These methods will use your own identity instead of \ @@ -556,22 +590,59 @@ class CallingIdentityTokenDetector : Detector(), SourceCodeScanner { UserHandle.getCallingUserId() ``` """, - category = Category.SECURITY, - priority = 6, - severity = Severity.WARNING, - implementation = Implementation( - CallingIdentityTokenDetector::class.java, - Scope.JAVA_FILE_SCOPE - ) + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) ) private fun getIncidentMessageUseOfCallerAwareMethodsWithClearedIdentity( variableName: String, methodName: String ): String = "You cleared the original identity with `Binder.clearCallingIdentity()` " + - "and returned into `$variableName`, so `$methodName` will be using your own " + - "identity instead of the caller's. Either explicitly query your own identity or " + - "move it after restoring the identity with " + - "`Binder.restoreCallingIdentity($variableName)`." + "and returned into `$variableName`, so `$methodName` will be using your own " + + "identity instead of the caller's. Either explicitly query your own identity or " + + "move it after restoring the identity with " + + "`Binder.restoreCallingIdentity($variableName)`." + + /** Issue: Result of Binder.clearCallingIdentity() is not stored in a variable */ + @JvmField + val ISSUE_RESULT_OF_CLEAR_IDENTITY_CALL_NOT_STORED_IN_VARIABLE: Issue = Issue.create( + id = "ResultOfClearIdentityCallNotStoredInVariable", + briefDescription = "Result of Binder.clearCallingIdentity() is not stored in a " + + "variable", + explanation = """ + You cleared the original calling identity with \ + `Binder.clearCallingIdentity()`, but did not store the result of the method \ + call in a variable. You need to store the result in a variable and restore it later. + + Use the following pattern for running operations with your own identity: + + ``` + final long token = Binder.clearCallingIdentity(); + try { + // Code using your own identity + } finally { + Binder.restoreCallingIdentity(token); + } + ``` + """, + category = Category.SECURITY, + priority = 6, + severity = Severity.WARNING, + implementation = Implementation( + CallingIdentityTokenDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + + private fun getIncidentMessageResultOfClearIdentityCallNotStoredInVariable( + methodName: String + ): String = "You cleared the original identity with `$methodName` but did not store the " + + "result in a variable. You need to store the result in a variable and restore it " + + "later." } } diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/CallingSettingsNonUserGetterMethodsDetector.kt b/tools/lint/framework/checks/src/main/java/com/google/android/lint/CallingSettingsNonUserGetterMethodsDetector.kt index fe567da7c017..fe567da7c017 100644 --- a/tools/lint/checks/src/main/java/com/google/android/lint/CallingSettingsNonUserGetterMethodsDetector.kt +++ b/tools/lint/framework/checks/src/main/java/com/google/android/lint/CallingSettingsNonUserGetterMethodsDetector.kt diff --git a/tools/lint/framework/checks/src/main/java/com/google/android/lint/PackageVisibilityDetector.kt b/tools/lint/framework/checks/src/main/java/com/google/android/lint/PackageVisibilityDetector.kt new file mode 100644 index 000000000000..48540b1da565 --- /dev/null +++ b/tools/lint/framework/checks/src/main/java/com/google/android/lint/PackageVisibilityDetector.kt @@ -0,0 +1,515 @@ +/* + * 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.UastParser +import com.android.tools.lint.detector.api.Category +import com.android.tools.lint.detector.api.Context +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.Scope +import com.android.tools.lint.detector.api.Severity +import com.android.tools.lint.detector.api.SourceCodeScanner +import com.android.tools.lint.detector.api.interprocedural.CallGraph +import com.android.tools.lint.detector.api.interprocedural.CallGraphResult +import com.android.tools.lint.detector.api.interprocedural.searchForPaths +import com.intellij.psi.PsiAnonymousClass +import com.intellij.psi.PsiMethod +import java.util.LinkedList +import org.jetbrains.uast.UCallExpression +import org.jetbrains.uast.UElement +import org.jetbrains.uast.UMethod +import org.jetbrains.uast.UParameter +import org.jetbrains.uast.USimpleNameReferenceExpression +import org.jetbrains.uast.visitor.AbstractUastVisitor + +/** + * A lint checker to detect potential package visibility issues for system's APIs. APIs working + * in the system_server and taking the package name as a parameter may have chance to reveal + * package existence status on the device, and break the + * <a href="https://developer.android.com/about/versions/11/privacy/package-visibility"> + * Package Visibility</a> that we introduced in Android 11. + * <p> + * Take an example of the API `boolean setFoo(String packageName)`, a malicious app may have chance + * to detect package existence state on the device from the result of the API, if there is no + * package visibility filtering rule or uid identify checks applying to the parameter of the + * package name. + */ +class PackageVisibilityDetector : Detector(), SourceCodeScanner { + + // Enables call graph analysis + override fun isCallGraphRequired(): Boolean = true + + override fun analyzeCallGraph( + context: Context, + callGraph: CallGraphResult + ) { + val systemServerApiNodes = callGraph.callGraph.nodes.filter(::isSystemServerApi) + val sinkMethodNodes = callGraph.callGraph.nodes.filter { + // TODO(b/228285232): Remove enforce permission sink methods + isNodeInList(it, ENFORCE_PERMISSION_METHODS) || isNodeInList(it, APPOPS_METHODS) + } + val parser = context.client.getUastParser(context.project) + analyzeApisContainPackageNameParameters( + context, parser, systemServerApiNodes, sinkMethodNodes) + } + + /** + * Looking for API contains package name parameters, report the lint issue if the API does not + * invoke any sink methods. + */ + private fun analyzeApisContainPackageNameParameters( + context: Context, + parser: UastParser, + systemServerApiNodes: List<CallGraph.Node>, + sinkMethodNodes: List<CallGraph.Node> + ) { + for (apiNode in systemServerApiNodes) { + val apiMethod = apiNode.getUMethod() ?: continue + val pkgNameParamIndexes = apiMethod.uastParameters.mapIndexedNotNull { index, param -> + if (Parameter(param) in PACKAGE_NAME_PATTERNS && apiNode.isArgumentInUse(index)) { + index + } else { + null + } + }.takeIf(List<Int>::isNotEmpty) ?: continue + + for (pkgNameParamIndex in pkgNameParamIndexes) { + // Trace the call path of the method's argument, pass the lint checks if a sink + // method is found + if (traceArgumentCallPath( + apiNode, pkgNameParamIndex, PACKAGE_NAME_SINK_METHOD_LIST)) { + continue + } + // Pass the check if one of the sink methods is invoked + if (hasValidPath( + searchForPaths( + sources = listOf(apiNode), + isSink = { it in sinkMethodNodes }, + getNeighbors = { node -> node.edges.map { it.node!! } } + ) + ) + ) continue + + // Report issue + val reportElement = apiMethod.uastParameters[pkgNameParamIndex] as UElement + val location = parser.createLocation(reportElement) + context.report( + ISSUE_PACKAGE_NAME_NO_PACKAGE_VISIBILITY_FILTERS, + location, + getMsgPackageNameNoPackageVisibilityFilters(apiMethod, pkgNameParamIndex) + ) + } + } + } + + /** + * Returns {@code true} if the method associated with the given node is a system server's + * public API that extends from Stub class. + */ + private fun isSystemServerApi( + node: CallGraph.Node + ): Boolean { + val method = node.getUMethod() ?: return false + if (!method.hasModifierProperty("public") || + method.uastBody == null || + method.containingClass is PsiAnonymousClass) { + return false + } + val className = method.containingClass?.qualifiedName ?: return false + if (!className.startsWith(SYSTEM_PACKAGE_PREFIX)) { + return false + } + return (method.containingClass ?: return false).supers + .filter { it.name == CLASS_STUB } + .filter { it.qualifiedName !in BYPASS_STUBS } + .any { it.findMethodBySignature(method, /* checkBases */ true) != null } + } + + /** + * Returns {@code true} if the list contains the node of the call graph. + */ + private fun isNodeInList( + node: CallGraph.Node, + filters: List<Method> + ): Boolean { + val method = node.getUMethod() ?: return false + return Method(method) in filters + } + + /** + * Trace the call paths of the argument of the method in the start entry. Return {@code true} + * if one of methods in the sink call list is invoked. + * Take an example of the call path: + * foo(packageName) -> a(packageName) -> b(packageName) -> filterAppAccess() + * It returns {@code true} if the filterAppAccess() is in the sink call list. + */ + private fun traceArgumentCallPath( + apiNode: CallGraph.Node, + pkgNameParamIndex: Int, + sinkList: List<Method> + ): Boolean { + val startEntry = TraceEntry(apiNode, pkgNameParamIndex) + val traceQueue = LinkedList<TraceEntry>().apply { add(startEntry) } + val allVisits = mutableSetOf<TraceEntry>().apply { add(startEntry) } + while (!traceQueue.isEmpty()) { + val entry = traceQueue.poll() + val entryNode = entry.node + val entryMethod = entryNode.getUMethod() ?: continue + val entryArgumentName = entryMethod.uastParameters[entry.argumentIndex].name + for (outEdge in entryNode.edges) { + val outNode = outEdge.node ?: continue + val outMethod = outNode.getUMethod() ?: continue + val outArgumentIndex = + outEdge.call?.findArgumentIndex( + entryArgumentName, outMethod.uastParameters.size) + val sinkMethod = findInSinkList(outMethod, sinkList) + if (sinkMethod == null) { + if (outArgumentIndex == null) { + // Path is not relevant to the sink method and argument + continue + } + // Path is relevant to the argument, add a new trace entry if never visit before + val newEntry = TraceEntry(outNode, outArgumentIndex) + if (newEntry !in allVisits) { + traceQueue.add(newEntry) + allVisits.add(newEntry) + } + continue + } + if (sinkMethod.matchArgument && outArgumentIndex == null) { + // The sink call is required to match the argument, but not found + continue + } + if (sinkMethod.checkCaller && + entryMethod.isInClearCallingIdentityScope(outEdge.call!!)) { + // The sink call is in the scope of Binder.clearCallingIdentify + continue + } + // A sink method is matched + return true + } + } + return false + } + + /** + * Returns the UMethod associated with the given node of call graph. + */ + private fun CallGraph.Node.getUMethod(): UMethod? = this.target.element as? UMethod + + /** + * Returns the system module name (e.g. com.android.server.pm) of the method of the + * call graph node. + */ + private fun CallGraph.Node.getModuleName(): String? { + val method = getUMethod() ?: return null + val className = method.containingClass?.qualifiedName ?: return null + if (!className.startsWith(SYSTEM_PACKAGE_PREFIX)) { + return null + } + val dotPos = className.indexOf(".", SYSTEM_PACKAGE_PREFIX.length) + if (dotPos == -1) { + return SYSTEM_PACKAGE_PREFIX + } + return className.substring(0, dotPos) + } + + /** + * Return {@code true} if the argument in the method's body is in-use. + */ + private fun CallGraph.Node.isArgumentInUse(argIndex: Int): Boolean { + val method = getUMethod() ?: return false + val argumentName = method.uastParameters[argIndex].name + var foundArg = false + val methodVisitor = object : AbstractUastVisitor() { + override fun visitSimpleNameReferenceExpression( + node: USimpleNameReferenceExpression + ): Boolean { + if (node.identifier == argumentName) { + foundArg = true + } + return true + } + } + method.uastBody?.accept(methodVisitor) + return foundArg + } + + /** + * Given an argument name, returns the index of argument in the call expression. + */ + private fun UCallExpression.findArgumentIndex( + argumentName: String, + parameterSize: Int + ): Int? { + if (valueArgumentCount == 0 || parameterSize == 0) { + return null + } + var match = false + val argVisitor = object : AbstractUastVisitor() { + override fun visitSimpleNameReferenceExpression( + node: USimpleNameReferenceExpression + ): Boolean { + if (node.identifier == argumentName) { + match = true + } + return true + } + override fun visitCallExpression(node: UCallExpression): Boolean { + return true + } + } + valueArguments.take(parameterSize).forEachIndexed { index, argument -> + argument.accept(argVisitor) + if (match) { + return index + } + } + return null + } + + /** + * Given a UMethod, returns a method from the sink method list. + */ + private fun findInSinkList( + uMethod: UMethod, + sinkCallList: List<Method> + ): Method? { + return sinkCallList.find { + it == Method(uMethod) || + it == Method(uMethod.containingClass?.qualifiedName ?: "", "*") + } + } + + /** + * Returns {@code true} if the call expression is in the scope of the + * Binder.clearCallingIdentify. + */ + private fun UMethod.isInClearCallingIdentityScope(call: UCallExpression): Boolean { + var isInScope = false + val methodVisitor = object : AbstractUastVisitor() { + private var clearCallingIdentity = 0 + override fun visitCallExpression(node: UCallExpression): Boolean { + if (call == node && clearCallingIdentity != 0) { + isInScope = true + return true + } + val visitMethod = Method(node.resolve() ?: return false) + if (visitMethod == METHOD_CLEAR_CALLING_IDENTITY) { + clearCallingIdentity++ + } else if (visitMethod == METHOD_RESTORE_CALLING_IDENTITY) { + clearCallingIdentity-- + } + return false + } + } + accept(methodVisitor) + return isInScope + } + + /** + * Checks the module name of the start node and the last node that invokes the sink method + * (e.g. checkPermission) in a path, returns {@code true} if one of the paths has the same + * module name for both nodes. + */ + private fun hasValidPath(paths: Collection<List<CallGraph.Node>>): Boolean { + for (pathNodes in paths) { + if (pathNodes.size < VALID_CALL_PATH_NODES_SIZE) { + continue + } + val startModule = pathNodes[0].getModuleName() ?: continue + val lastCallModule = pathNodes[pathNodes.size - 2].getModuleName() ?: continue + if (startModule == lastCallModule) { + return true + } + } + return false + } + + /** + * A data class to represent the method. + */ + private data class Method( + val clazz: String, + val name: String + ) { + // Used by traceArgumentCallPath to indicate that the method is required to match the + // argument name + var matchArgument = true + + // Used by traceArgumentCallPath to indicate that the method is required to check whether + // the Binder.clearCallingIdentity is invoked. + var checkCaller = false + + constructor( + clazz: String, + name: String, + matchArgument: Boolean = true, + checkCaller: Boolean = false + ) : this(clazz, name) { + this.matchArgument = matchArgument + this.checkCaller = checkCaller + } + + constructor( + method: PsiMethod + ) : this(method.containingClass?.qualifiedName ?: "", method.name) + + constructor( + method: com.google.android.lint.model.Method + ) : this(method.clazz, method.name) + } + + /** + * A data class to represent the parameter of the method. The parameter name is converted to + * lower case letters for comparison. + */ + private data class Parameter private constructor( + val typeName: String, + val parameterName: String + ) { + constructor(uParameter: UParameter) : this( + uParameter.type.canonicalText, + uParameter.name.lowercase() + ) + + companion object { + fun create(typeName: String, parameterName: String) = + Parameter(typeName, parameterName.lowercase()) + } + } + + /** + * A data class wraps a method node of the call graph and an index that indicates an + * argument of the method to record call trace information. + */ + private data class TraceEntry( + val node: CallGraph.Node, + val argumentIndex: Int + ) + + companion object { + private const val SYSTEM_PACKAGE_PREFIX = "com.android.server." + // A valid call path list needs to contain a start node and a sink node + private const val VALID_CALL_PATH_NODES_SIZE = 2 + + private const val CLASS_STRING = "java.lang.String" + private const val CLASS_PACKAGE_MANAGER = "android.content.pm.PackageManager" + private const val CLASS_IPACKAGE_MANAGER = "android.content.pm.IPackageManager" + private const val CLASS_APPOPS_MANAGER = "android.app.AppOpsManager" + private const val CLASS_BINDER = "android.os.Binder" + private const val CLASS_PACKAGE_MANAGER_INTERNAL = + "android.content.pm.PackageManagerInternal" + + // Patterns of package name parameter + private val PACKAGE_NAME_PATTERNS = setOf( + Parameter.create(CLASS_STRING, "packageName"), + Parameter.create(CLASS_STRING, "callingPackage"), + Parameter.create(CLASS_STRING, "callingPackageName"), + Parameter.create(CLASS_STRING, "pkgName"), + Parameter.create(CLASS_STRING, "callingPkg"), + Parameter.create(CLASS_STRING, "pkg") + ) + + // Package manager APIs + private val PACKAGE_NAME_SINK_METHOD_LIST = listOf( + Method(CLASS_PACKAGE_MANAGER_INTERNAL, "filterAppAccess", matchArgument = false), + Method(CLASS_PACKAGE_MANAGER_INTERNAL, "canQueryPackage"), + Method(CLASS_PACKAGE_MANAGER_INTERNAL, "isSameApp"), + Method(CLASS_PACKAGE_MANAGER, "*", checkCaller = true), + Method(CLASS_IPACKAGE_MANAGER, "*", checkCaller = true), + Method(CLASS_PACKAGE_MANAGER, "getPackagesForUid", matchArgument = false), + Method(CLASS_IPACKAGE_MANAGER, "getPackagesForUid", matchArgument = false) + ) + + // AppOps APIs which include uid and package visibility filters checks + private val APPOPS_METHODS = listOf( + Method(CLASS_APPOPS_MANAGER, "noteOp"), + Method(CLASS_APPOPS_MANAGER, "noteOpNoThrow"), + Method(CLASS_APPOPS_MANAGER, "noteOperation"), + Method(CLASS_APPOPS_MANAGER, "noteProxyOp"), + Method(CLASS_APPOPS_MANAGER, "noteProxyOpNoThrow"), + Method(CLASS_APPOPS_MANAGER, "startOp"), + Method(CLASS_APPOPS_MANAGER, "startOpNoThrow"), + Method(CLASS_APPOPS_MANAGER, "FinishOp"), + Method(CLASS_APPOPS_MANAGER, "finishProxyOp"), + Method(CLASS_APPOPS_MANAGER, "checkPackage") + ) + + // Enforce permission APIs + private val ENFORCE_PERMISSION_METHODS = + com.google.android.lint.ENFORCE_PERMISSION_METHODS + .map(PackageVisibilityDetector::Method) + + private val BYPASS_STUBS = listOf( + "android.content.pm.IPackageDataObserver.Stub", + "android.content.pm.IPackageDeleteObserver.Stub", + "android.content.pm.IPackageDeleteObserver2.Stub", + "android.content.pm.IPackageInstallObserver2.Stub", + "com.android.internal.app.IAppOpsCallback.Stub", + + // TODO(b/228285637): Do not bypass PackageManagerService API + "android.content.pm.IPackageManager.Stub", + "android.content.pm.IPackageManagerNative.Stub" + ) + + private val METHOD_CLEAR_CALLING_IDENTITY = + Method(CLASS_BINDER, "clearCallingIdentity") + private val METHOD_RESTORE_CALLING_IDENTITY = + Method(CLASS_BINDER, "restoreCallingIdentity") + + private fun getMsgPackageNameNoPackageVisibilityFilters( + method: UMethod, + argumentIndex: Int + ): String = "Api: ${method.name} contains a package name parameter: " + + "${method.uastParameters[argumentIndex].name} does not apply " + + "package visibility filtering rules." + + private val EXPLANATION = """ + APIs working in the system_server and taking the package name as a parameter may have + chance to reveal package existence status on the device, and break the package + visibility that we introduced in Android 11. + (https://developer.android.com/about/versions/11/privacy/package-visibility) + + Take an example of the API `boolean setFoo(String packageName)`, a malicious app may + have chance to get package existence state on the device from the result of the API, + if there is no package visibility filtering rule or uid identify checks applying to + the parameter of the package name. + + To resolve it, you could apply package visibility filtering rules to the package name + via PackageManagerInternal.filterAppAccess API, before starting to use the package name. + If the parameter is a calling package name, use the PackageManager API such as + PackageManager.getPackagesForUid to verify the calling identify. + """ + + val ISSUE_PACKAGE_NAME_NO_PACKAGE_VISIBILITY_FILTERS = Issue.create( + id = "ApiMightLeakAppVisibility", + briefDescription = "Api takes package name parameter doesn't apply " + + "package visibility filters", + explanation = EXPLANATION, + category = Category.SECURITY, + priority = 1, + severity = Severity.WARNING, + implementation = Implementation( + PackageVisibilityDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + } +} diff --git a/tools/lint/framework/checks/src/main/java/com/google/android/lint/PermissionMethodDetector.kt b/tools/lint/framework/checks/src/main/java/com/google/android/lint/PermissionMethodDetector.kt new file mode 100644 index 000000000000..e12ec3d4a77c --- /dev/null +++ b/tools/lint/framework/checks/src/main/java/com/google/android/lint/PermissionMethodDetector.kt @@ -0,0 +1,199 @@ +/* + * 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 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 + } + } + + private fun hasPermissionMethodAnnotation(method: UMethod): Boolean = method.annotations + .any { it.hasQualifiedName(ANNOTATION_PERMISSION_METHOD) } + } +} diff --git a/tools/lint/framework/checks/src/main/java/com/google/android/lint/parcel/CallMigrators.kt b/tools/lint/framework/checks/src/main/java/com/google/android/lint/parcel/CallMigrators.kt new file mode 100644 index 000000000000..06c098df385d --- /dev/null +++ b/tools/lint/framework/checks/src/main/java/com/google/android/lint/parcel/CallMigrators.kt @@ -0,0 +1,229 @@ +/* + * 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.parcel + +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.intellij.psi.PsiArrayType +import com.intellij.psi.PsiCallExpression +import com.intellij.psi.PsiClassType +import com.intellij.psi.PsiIntersectionType +import com.intellij.psi.PsiMethod +import com.intellij.psi.PsiType +import com.intellij.psi.PsiTypeParameter +import com.intellij.psi.PsiWildcardType +import org.jetbrains.uast.UCallExpression +import org.jetbrains.uast.UElement +import org.jetbrains.uast.UExpression +import org.jetbrains.uast.UVariable + +/** + * Subclass this class and override {@link #getBoundingClass} to report an unsafe Parcel API issue + * with a fix that migrates towards the new safer API by appending an argument in the form of + * {@code com.package.ItemType.class} coming from the result of the overridden method. + */ +abstract class CallMigrator( + val method: Method, + private val rejects: Set<String> = emptySet(), +) { + open fun report(context: JavaContext, call: UCallExpression, method: PsiMethod) { + val location = context.getLocation(call) + val itemType = filter(getBoundingClass(context, call, method)) + val fix = (itemType as? PsiClassType)?.let { type -> + getParcelFix(location, this.method.name, getArgumentSuffix(type)) + } + val message = "Unsafe `${this.method.className}.${this.method.name}()` API usage" + context.report(SaferParcelChecker.ISSUE_UNSAFE_API_USAGE, call, location, message, fix) + } + + protected open fun getArgumentSuffix(type: PsiClassType) = + ", ${type.rawType().canonicalText}.class" + + protected open fun getBoundingClass( + context: JavaContext, + call: UCallExpression, + method: PsiMethod, + ): PsiType? = null + + protected fun getItemType(type: PsiType, container: String): PsiClassType? { + val supers = getParentTypes(type).mapNotNull { it as? PsiClassType } + val containerType = supers.firstOrNull { it.rawType().canonicalText == container } + ?: return null + val itemType = containerType.parameters.getOrNull(0) ?: return null + // TODO: Expand to other types, see PsiTypeVisitor + return when (itemType) { + is PsiClassType -> itemType + is PsiWildcardType -> itemType.bound as PsiClassType + else -> null + } + } + + /** + * Tries to obtain the type expected by the "receiving" end given a certain {@link UElement}. + * + * This could be an assignment, an argument passed to a method call, to a constructor call, a + * type cast, etc. If no receiving end is found, the type of the UExpression itself is returned. + */ + protected fun getReceivingType(expression: UElement): PsiType? { + val parent = expression.uastParent + var type = when (parent) { + is UCallExpression -> { + val i = parent.valueArguments.indexOf(expression) + val psiCall = parent.sourcePsi as? PsiCallExpression ?: return null + val typeSubstitutor = psiCall.resolveMethodGenerics().substitutor + val method = psiCall.resolveMethod()!! + method.getSignature(typeSubstitutor).parameterTypes[i] + } + is UVariable -> parent.type + is UExpression -> parent.getExpressionType() + else -> null + } + if (type == null && expression is UExpression) { + type = expression.getExpressionType() + } + return type + } + + protected fun filter(type: PsiType?): PsiType? { + // It's important that PsiIntersectionType case is above the one that check the type in + // rejects, because for intersect types, the canonicalText is one of the terms. + if (type is PsiIntersectionType) { + return type.conjuncts.mapNotNull(this::filter).firstOrNull() + } + if (type == null || type.canonicalText in rejects) { + return null + } + if (type is PsiClassType && type.resolve() is PsiTypeParameter) { + return null + } + return type + } + + private fun getParentTypes(type: PsiType): Set<PsiType> = + type.superTypes.flatMap(::getParentTypes).toSet() + type + + protected fun getParcelFix(location: Location, method: String, arguments: String) = + LintFix + .create() + .name("Migrate to safer Parcel.$method() API") + .replace() + .range(location) + .pattern("$method\\s*\\(((?:.|\\n)*)\\)") + .with("\\k<1>$arguments") + .autoFix() + .build() +} + +/** + * This class derives the type to be appended by inferring the generic type of the {@code container} + * type (eg. "java.util.List") of the {@code argument}-th argument. + */ +class ContainerArgumentMigrator( + method: Method, + private val argument: Int, + private val container: String, + rejects: Set<String> = emptySet(), +) : CallMigrator(method, rejects) { + override fun getBoundingClass( + context: JavaContext, call: UCallExpression, method: PsiMethod + ): PsiType? { + val firstParamType = call.valueArguments[argument].getExpressionType() ?: return null + return getItemType(firstParamType, container)!! + } + + /** + * We need to insert a casting construct in the class parameter. For example: + * (Class<Foo<Bar>>) (Class<?>) Foo.class. + * This is needed for when the arguments of the conflict (eg. when there is List<Foo<Bar>> and + * class type is Class<Foo?). + */ + override fun getArgumentSuffix(type: PsiClassType): String { + if (type.parameters.isNotEmpty()) { + val rawType = type.rawType() + return ", (Class<${type.canonicalText}>) (Class<?>) ${rawType.canonicalText}.class" + } + return super.getArgumentSuffix(type) + } +} + +/** + * This class derives the type to be appended by inferring the generic type of the {@code container} + * type (eg. "java.util.List") of the return type of the method. + */ +class ContainerReturnMigrator( + method: Method, + private val container: String, + rejects: Set<String> = emptySet(), +) : CallMigrator(method, rejects) { + override fun getBoundingClass( + context: JavaContext, call: UCallExpression, method: PsiMethod + ): PsiType? { + val type = getReceivingType(call.uastParent!!) ?: return null + return getItemType(type, container) + } +} + +/** + * This class derives the type to be appended by inferring the expected type for the method result. + */ +class ReturnMigrator( + method: Method, + rejects: Set<String> = emptySet(), +) : CallMigrator(method, rejects) { + override fun getBoundingClass( + context: JavaContext, call: UCallExpression, method: PsiMethod + ): PsiType? { + return getReceivingType(call.uastParent!!) + } +} + +/** + * This class appends the class loader and the class object by deriving the type from the method + * result. + */ +class ReturnMigratorWithClassLoader( + method: Method, + rejects: Set<String> = emptySet(), +) : CallMigrator(method, rejects) { + override fun getBoundingClass( + context: JavaContext, call: UCallExpression, method: PsiMethod + ): PsiType? { + return getReceivingType(call.uastParent!!) + } + + override fun getArgumentSuffix(type: PsiClassType): String = + "${type.rawType().canonicalText}.class.getClassLoader(), " + + "${type.rawType().canonicalText}.class" + +} + +/** + * This class derives the type to be appended by inferring the expected array type + * for the method result. + */ +class ArrayReturnMigrator( + method: Method, + rejects: Set<String> = emptySet(), +) : CallMigrator(method, rejects) { + override fun getBoundingClass( + context: JavaContext, call: UCallExpression, method: PsiMethod + ): PsiType? { + val type = getReceivingType(call.uastParent!!) + return (type as? PsiArrayType)?.componentType + } +} diff --git a/tools/lint/framework/checks/src/main/java/com/google/android/lint/parcel/Method.kt b/tools/lint/framework/checks/src/main/java/com/google/android/lint/parcel/Method.kt new file mode 100644 index 000000000000..0826e8e74431 --- /dev/null +++ b/tools/lint/framework/checks/src/main/java/com/google/android/lint/parcel/Method.kt @@ -0,0 +1,42 @@ +/* + * 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.parcel + +data class Method( + val params: List<String>, + val clazz: String, + val name: String, + val parameters: List<String> +) { + constructor( + clazz: String, + name: String, + parameters: List<String> + ) : this( + listOf(), clazz, name, parameters + ) + + val signature: String + get() { + val prefix = if (params.isEmpty()) "" else "${params.joinToString(", ", "<", ">")} " + return "$prefix$clazz.$name(${parameters.joinToString()})" + } + + val className: String by lazy { + clazz.split(".").last() + } +} diff --git a/tools/lint/framework/checks/src/main/java/com/google/android/lint/parcel/SaferParcelChecker.kt b/tools/lint/framework/checks/src/main/java/com/google/android/lint/parcel/SaferParcelChecker.kt new file mode 100644 index 000000000000..f92826316be4 --- /dev/null +++ b/tools/lint/framework/checks/src/main/java/com/google/android/lint/parcel/SaferParcelChecker.kt @@ -0,0 +1,126 @@ +/* + * 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.parcel + +import com.android.tools.lint.detector.api.* +import com.intellij.psi.PsiMethod +import com.intellij.psi.PsiSubstitutor +import com.intellij.psi.PsiType +import com.intellij.psi.PsiTypeParameter +import org.jetbrains.uast.UCallExpression +import java.util.* + +@Suppress("UnstableApiUsage") +class SaferParcelChecker : Detector(), SourceCodeScanner { + override fun getApplicableMethodNames(): List<String> = + MIGRATORS + .map(CallMigrator::method) + .map(Method::name) + + override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) { + if (!isAtLeastT(context)) return + val signature = getSignature(method) + val migrator = MIGRATORS.firstOrNull { it.method.signature == signature } ?: return + migrator.report(context, node, method) + } + + private fun getSignature(method: PsiMethod): String { + val name = UastLintUtils.getQualifiedName(method) + val signature = method.getSignature(PsiSubstitutor.EMPTY) + val parameters = + signature.parameterTypes.joinToString(transform = PsiType::getCanonicalText) + val types = signature.typeParameters.map(PsiTypeParameter::getName) + val prefix = if (types.isEmpty()) "" else types.joinToString(", ", "<", ">") + " " + return "$prefix$name($parameters)" + } + + private fun isAtLeastT(context: Context): Boolean { + val project = if (context.isGlobalAnalysis()) context.mainProject else context.project + return project.isAndroidProject && project.minSdkVersion.featureLevel >= 33 + } + + companion object { + @JvmField + val ISSUE_UNSAFE_API_USAGE: Issue = Issue.create( + id = "UnsafeParcelApi", + briefDescription = "Use of unsafe deserialization API", + explanation = """ + You are using a deprecated deserialization API that doesn't accept the expected class as\ + a parameter. This means that unexpected classes could be instantiated and\ + unexpected code executed. + + Please migrate to the safer alternative that takes an extra Class<T> parameter. + """, + category = Category.SECURITY, + priority = 8, + severity = Severity.WARNING, + + implementation = Implementation( + SaferParcelChecker::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + + // Parcel + private val PARCEL_METHOD_READ_SERIALIZABLE = Method("android.os.Parcel", "readSerializable", listOf()) + private val PARCEL_METHOD_READ_ARRAY_LIST = Method("android.os.Parcel", "readArrayList", listOf("java.lang.ClassLoader")) + private val PARCEL_METHOD_READ_LIST = Method("android.os.Parcel", "readList", listOf("java.util.List", "java.lang.ClassLoader")) + private val PARCEL_METHOD_READ_PARCELABLE = Method(listOf("T"), "android.os.Parcel", "readParcelable", listOf("java.lang.ClassLoader")) + private val PARCEL_METHOD_READ_PARCELABLE_LIST = Method(listOf("T"), "android.os.Parcel", "readParcelableList", listOf("java.util.List<T>", "java.lang.ClassLoader")) + private val PARCEL_METHOD_READ_SPARSE_ARRAY = Method(listOf("T"), "android.os.Parcel", "readSparseArray", listOf("java.lang.ClassLoader")) + private val PARCEL_METHOD_READ_ARRAY = Method("android.os.Parcel", "readArray", listOf("java.lang.ClassLoader")) + private val PARCEL_METHOD_READ_PARCELABLE_ARRAY = Method("android.os.Parcel", "readParcelableArray", listOf("java.lang.ClassLoader")) + + // Bundle + private val BUNDLE_METHOD_GET_SERIALIZABLE = Method("android.os.Bundle", "getSerializable", listOf("java.lang.String")) + private val BUNDLE_METHOD_GET_PARCELABLE = Method(listOf("T"), "android.os.Bundle", "getParcelable", listOf("java.lang.String")) + private val BUNDLE_METHOD_GET_PARCELABLE_ARRAY_LIST = Method(listOf("T"), "android.os.Bundle", "getParcelableArrayList", listOf("java.lang.String")) + private val BUNDLE_METHOD_GET_PARCELABLE_ARRAY = Method("android.os.Bundle", "getParcelableArray", listOf("java.lang.String")) + private val BUNDLE_METHOD_GET_SPARSE_PARCELABLE_ARRAY = Method(listOf("T"), "android.os.Bundle", "getSparseParcelableArray", listOf("java.lang.String")) + + // Intent + private val INTENT_METHOD_GET_SERIALIZABLE_EXTRA = Method("android.content.Intent", "getSerializableExtra", listOf("java.lang.String")) + private val INTENT_METHOD_GET_PARCELABLE_EXTRA = Method(listOf("T"), "android.content.Intent", "getParcelableExtra", listOf("java.lang.String")) + private val INTENT_METHOD_GET_PARCELABLE_ARRAY_EXTRA = Method("android.content.Intent", "getParcelableArrayExtra", listOf("java.lang.String")) + private val INTENT_METHOD_GET_PARCELABLE_ARRAY_LIST_EXTRA = Method(listOf("T"), "android.content.Intent", "getParcelableArrayListExtra", listOf("java.lang.String")) + + // TODO: Write migrators for methods below + private val PARCEL_METHOD_READ_PARCELABLE_CREATOR = Method("android.os.Parcel", "readParcelableCreator", listOf("java.lang.ClassLoader")) + + private val MIGRATORS = listOf( + ReturnMigrator(PARCEL_METHOD_READ_PARCELABLE, setOf("android.os.Parcelable")), + ContainerArgumentMigrator(PARCEL_METHOD_READ_LIST, 0, "java.util.List"), + ContainerReturnMigrator(PARCEL_METHOD_READ_ARRAY_LIST, "java.util.Collection"), + ContainerReturnMigrator(PARCEL_METHOD_READ_SPARSE_ARRAY, "android.util.SparseArray"), + ContainerArgumentMigrator(PARCEL_METHOD_READ_PARCELABLE_LIST, 0, "java.util.List"), + ReturnMigratorWithClassLoader(PARCEL_METHOD_READ_SERIALIZABLE), + ArrayReturnMigrator(PARCEL_METHOD_READ_ARRAY, setOf("java.lang.Object")), + ArrayReturnMigrator(PARCEL_METHOD_READ_PARCELABLE_ARRAY, setOf("android.os.Parcelable")), + + ReturnMigrator(BUNDLE_METHOD_GET_PARCELABLE, setOf("android.os.Parcelable")), + ContainerReturnMigrator(BUNDLE_METHOD_GET_PARCELABLE_ARRAY_LIST, "java.util.Collection", setOf("android.os.Parcelable")), + ArrayReturnMigrator(BUNDLE_METHOD_GET_PARCELABLE_ARRAY, setOf("android.os.Parcelable")), + ContainerReturnMigrator(BUNDLE_METHOD_GET_SPARSE_PARCELABLE_ARRAY, "android.util.SparseArray", setOf("android.os.Parcelable")), + ReturnMigrator(BUNDLE_METHOD_GET_SERIALIZABLE, setOf("java.io.Serializable")), + + ReturnMigrator(INTENT_METHOD_GET_PARCELABLE_EXTRA, setOf("android.os.Parcelable")), + ContainerReturnMigrator(INTENT_METHOD_GET_PARCELABLE_ARRAY_LIST_EXTRA, "java.util.Collection", setOf("android.os.Parcelable")), + ArrayReturnMigrator(INTENT_METHOD_GET_PARCELABLE_ARRAY_EXTRA, setOf("android.os.Parcelable")), + ReturnMigrator(INTENT_METHOD_GET_SERIALIZABLE_EXTRA, setOf("java.io.Serializable")), + ) + } +} diff --git a/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt b/tools/lint/framework/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt index e1a5c613dee1..d90f3e31baf9 100644 --- a/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt +++ b/tools/lint/framework/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt @@ -27,12 +27,13 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { override fun getDetector(): Detector = CallingIdentityTokenDetector() override fun getIssues(): List<Issue> = listOf( - CallingIdentityTokenDetector.ISSUE_UNUSED_TOKEN, - CallingIdentityTokenDetector.ISSUE_NON_FINAL_TOKEN, - CallingIdentityTokenDetector.ISSUE_NESTED_CLEAR_IDENTITY_CALLS, - CallingIdentityTokenDetector.ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, - CallingIdentityTokenDetector.ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, - CallingIdentityTokenDetector.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY + CallingIdentityTokenDetector.ISSUE_UNUSED_TOKEN, + CallingIdentityTokenDetector.ISSUE_NON_FINAL_TOKEN, + CallingIdentityTokenDetector.ISSUE_NESTED_CLEAR_IDENTITY_CALLS, + CallingIdentityTokenDetector.ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, + CallingIdentityTokenDetector.ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, + CallingIdentityTokenDetector.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY, + CallingIdentityTokenDetector.ISSUE_RESULT_OF_CLEAR_IDENTITY_CALL_NOT_STORED_IN_VARIABLE ) override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) @@ -41,8 +42,8 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { fun testDoesNotDetectIssuesInCorrectScenario() { lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; public class TestClass1 extends Binder { @@ -62,22 +63,29 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } finally { restoreCallingIdentity(token3); } + final Long token4 = true ? Binder.clearCallingIdentity() : null; + try { + } finally { + if (token4 != null) { + restoreCallingIdentity(token4); + } + } } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expectClean() + .run() + .expectClean() } /** Unused token issue tests */ fun testDetectsUnusedTokens() { lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; public class TestClass1 extends Binder { @@ -101,12 +109,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expect( - """ + .run() + .expect( + """ src/test/pkg/TestClass1.java:5: Warning: token1 has not been used to \ restore the calling identity. Introduce a try-finally after the \ declaration and call Binder.restoreCallingIdentity(token1) in finally or \ @@ -127,13 +135,13 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 3 warnings """.addLineContinuation() - ) + ) } fun testDetectsUnusedTokensInScopes() { lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; public class TestClass1 { @@ -152,12 +160,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expect( - """ + .run() + .expect( + """ src/test/pkg/TestClass1.java:5: Warning: token has not been used to \ restore the calling identity. Introduce a try-finally after the \ declaration and call Binder.restoreCallingIdentity(token) in finally or \ @@ -166,13 +174,13 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 1 warnings """.addLineContinuation() - ) + ) } fun testDoesNotDetectUsedTokensInScopes() { lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; public class TestClass1 { @@ -192,17 +200,17 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expectClean() + .run() + .expectClean() } fun testDetectsUnusedTokensWithSimilarNamesInScopes() { lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; public class TestClass1 { @@ -220,12 +228,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expect( - """ + .run() + .expect( + """ src/test/pkg/TestClass1.java:5: Warning: token has not been used to \ restore the calling identity. Introduce a try-finally after the \ declaration and call Binder.restoreCallingIdentity(token) in finally or \ @@ -240,15 +248,15 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 2 warnings """.addLineContinuation() - ) + ) } /** Non-final token issue tests */ fun testDetectsNonFinalTokens() { lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; public class TestClass1 extends Binder { @@ -271,12 +279,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expect( - """ + .run() + .expect( + """ src/test/pkg/TestClass1.java:5: Warning: token1 is a non-final token from \ Binder.clearCallingIdentity(). Add final keyword to token1. \ [NonFinalTokenOfOriginalCallingIdentity] @@ -294,7 +302,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 3 warnings """.addLineContinuation() - ) + ) } /** Nested clearCallingIdentity() calls issue tests */ @@ -302,8 +310,8 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { fun testDetectsNestedClearCallingIdentityCalls() { // Pattern: clear - clear - clear - restore - restore - restore lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; public class TestClass1 extends Binder { @@ -326,12 +334,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expect( - """ + .run() + .expect( + """ src/test/pkg/TestClass1.java:7: Warning: The calling identity has already \ been cleared and returned into token1. Move token2 declaration after \ restoring the calling identity with Binder.restoreCallingIdentity(token1). \ @@ -348,15 +356,15 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { src/test/pkg/TestClass1.java:5: Location of the token1 declaration. 0 errors, 2 warnings """.addLineContinuation() - ) + ) } /** clearCallingIdentity() not followed by try-finally issue tests */ fun testDetectsClearIdentityCallNotFollowedByTryFinally() { lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; public class TestClass1 extends Binder{ @@ -397,12 +405,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expect( - """ + .run() + .expect( + """ src/test/pkg/TestClass1.java:5: Warning: You cleared the calling identity \ and returned the result into token, but the next statement is not a \ try-finally statement. Define a try-finally block after token declaration \ @@ -445,15 +453,15 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 5 warnings """.addLineContinuation() - ) + ) } /** restoreCallingIdentity() call not in finally block issue tests */ fun testDetectsRestoreCallingIdentityCallNotInFinally() { lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; public class TestClass1 extends Binder { @@ -482,12 +490,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expect( - """ + .run() + .expect( + """ src/test/pkg/TestClass1.java:10: Warning: \ Binder.restoreCallingIdentity(token) is not an immediate child of the \ finally block of the try statement after token declaration. Surround the c\ @@ -511,13 +519,13 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 3 warnings """.addLineContinuation() - ) + ) } fun testDetectsRestoreCallingIdentityCallNotInFinallyInScopes() { lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; public class TestClass1 extends Binder { @@ -560,12 +568,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expect( - """ + .run() + .expect( + """ src/test/pkg/TestClass1.java:11: Warning: \ Binder.restoreCallingIdentity(token1) is not an immediate child of the \ finally block of the try statement after token1 declaration. Surround the \ @@ -596,15 +604,15 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 4 warnings """.addLineContinuation() - ) + ) } /** Use of caller-aware methods after clearCallingIdentity() issue tests */ fun testDetectsUseOfCallerAwareMethodsWithClearedIdentityIssuesInScopes() { lint().files( - java( - """ + java( + """ package test.pkg; import android.os.Binder; import android.os.UserHandle; @@ -632,12 +640,12 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { } } """ - ).indented(), - *stubs + ).indented(), + *stubs ) - .run() - .expect( - """ + .run() + .expect( + """ src/test/pkg/TestClass1.java:8: Warning: You cleared the original identity \ with Binder.clearCallingIdentity() and returned into token, so \ getCallingPid() will be using your own identity instead of the \ @@ -736,13 +744,58 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 12 warnings """.addLineContinuation() - ) + ) + } + + /** Result of Binder.clearCallingIdentity() is not stored in a variable issue tests */ + + fun testDetectsResultOfClearIdentityCallNotStoredInVariable() { + lint().files( + java( + """ + package test.pkg; + import android.os.Binder; + public class TestClass1 extends Binder { + private void testMethod() { + Binder.clearCallingIdentity(); + android.os.Binder.clearCallingIdentity(); + clearCallingIdentity(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/test/pkg/TestClass1.java:5: Warning: You cleared the original identity \ + with Binder.clearCallingIdentity() but did not store the result in a \ + variable. You need to store the result in a variable and restore it later. \ + [ResultOfClearIdentityCallNotStoredInVariable] + Binder.clearCallingIdentity(); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + src/test/pkg/TestClass1.java:6: Warning: You cleared the original identity \ + with android.os.Binder.clearCallingIdentity() but did not store the result \ + in a variable. You need to store the result in a variable and restore it \ + later. [ResultOfClearIdentityCallNotStoredInVariable] + android.os.Binder.clearCallingIdentity(); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + src/test/pkg/TestClass1.java:7: Warning: You cleared the original identity \ + with clearCallingIdentity() but did not store the result in a variable. \ + You need to store the result in a variable and restore it later. \ + [ResultOfClearIdentityCallNotStoredInVariable] + clearCallingIdentity(); + ~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 3 warnings + """.addLineContinuation() + ) } /** Stubs for classes used for testing */ private val binderStub: TestFile = java( - """ + """ package android.os; public class Binder { public static final native long clearCallingIdentity() { @@ -767,7 +820,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ).indented() private val userHandleStub: TestFile = java( - """ + """ package android.os; import android.annotation.AppIdInt; import android.annotation.UserIdInt; @@ -792,7 +845,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ).indented() private val userIdIntStub: TestFile = java( - """ + """ package android.annotation; public @interface UserIdInt { } @@ -800,7 +853,7 @@ class CallingIdentityTokenDetectorTest : LintDetectorTest() { ).indented() private val appIdIntStub: TestFile = java( - """ + """ package android.annotation; public @interface AppIdInt { } diff --git a/tools/lint/checks/src/test/java/com/google/android/lint/CallingSettingsNonUserGetterMethodsIssueDetectorTest.kt b/tools/lint/framework/checks/src/test/java/com/google/android/lint/CallingSettingsNonUserGetterMethodsIssueDetectorTest.kt index e72f38416310..e72f38416310 100644 --- a/tools/lint/checks/src/test/java/com/google/android/lint/CallingSettingsNonUserGetterMethodsIssueDetectorTest.kt +++ b/tools/lint/framework/checks/src/test/java/com/google/android/lint/CallingSettingsNonUserGetterMethodsIssueDetectorTest.kt diff --git a/tools/lint/framework/checks/src/test/java/com/google/android/lint/PackageVisibilityDetectorTest.kt b/tools/lint/framework/checks/src/test/java/com/google/android/lint/PackageVisibilityDetectorTest.kt new file mode 100644 index 000000000000..a70644ab8532 --- /dev/null +++ b/tools/lint/framework/checks/src/test/java/com/google/android/lint/PackageVisibilityDetectorTest.kt @@ -0,0 +1,271 @@ +/* + * 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.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 + +@Suppress("UnstableApiUsage") +class PackageVisibilityDetectorTest : LintDetectorTest() { + override fun getDetector(): Detector = PackageVisibilityDetector() + + override fun getIssues(): MutableList<Issue> = mutableListOf( + PackageVisibilityDetector.ISSUE_PACKAGE_NAME_NO_PACKAGE_VISIBILITY_FILTERS + ) + + override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) + + fun testDetectIssuesParameterDoesNotApplyPackageVisibilityFilters() { + lint().files(java( + """ + package com.android.server.lint.test; + import android.internal.test.IFoo; + + public class TestClass extends IFoo.Stub { + @Override + public boolean hasPackage(String packageName) { + return packageName != null; + } + } + """).indented(), *stubs + ).run().expect( + """ + src/com/android/server/lint/test/TestClass.java:6: Warning: \ + Api: hasPackage contains a package name parameter: packageName does not apply \ + package visibility filtering rules. \ + [ApiMightLeakAppVisibility] + public boolean hasPackage(String packageName) { + ~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testDoesNotDetectIssuesApiInvokesAppOps() { + lint().files(java( + """ + package com.android.server.lint.test; + import android.app.AppOpsManager; + import android.os.Binder; + import android.internal.test.IFoo; + + public class TestClass extends IFoo.Stub { + private AppOpsManager mAppOpsManager; + + @Override + public boolean hasPackage(String packageName) { + checkPackage(packageName); + return packageName != null; + } + + private void checkPackage(String packageName) { + mAppOpsManager.checkPackage(Binder.getCallingUid(), packageName); + } + } + """ + ).indented(), *stubs).run().expectClean() + } + + fun testDoesNotDetectIssuesApiInvokesEnforcePermission() { + lint().files(java( + """ + package com.android.server.lint.test; + import android.content.Context; + import android.internal.test.IFoo; + + public class TestClass extends IFoo.Stub { + private Context mContext; + + @Override + public boolean hasPackage(String packageName) { + enforcePermission(); + return packageName != null; + } + + private void enforcePermission() { + mContext.checkCallingPermission( + android.Manifest.permission.ACCESS_INPUT_FLINGER); + } + } + """ + ).indented(), *stubs).run().expectClean() + } + + fun testDoesNotDetectIssuesApiInvokesPackageManager() { + lint().files(java( + """ + package com.android.server.lint.test; + import android.content.pm.PackageInfo; + import android.content.pm.PackageManager; + import android.internal.test.IFoo; + + public class TestClass extends IFoo.Stub { + private PackageManager mPackageManager; + + @Override + public boolean hasPackage(String packageName) { + return getPackageInfo(packageName) != null; + } + + private PackageInfo getPackageInfo(String packageName) { + try { + return mPackageManager.getPackageInfo(packageName, 0); + } catch (PackageManager.NameNotFoundException e) { + return null; + } + } + } + """ + ).indented(), *stubs).run().expectClean() + } + + fun testDetectIssuesApiInvokesPackageManagerAndClearCallingIdentify() { + lint().files(java( + """ + package com.android.server.lint.test; + import android.content.pm.PackageInfo; + import android.content.pm.PackageManager; + import android.internal.test.IFoo;import android.os.Binder; + + public class TestClass extends IFoo.Stub { + private PackageManager mPackageManager; + + @Override + public boolean hasPackage(String packageName) { + return getPackageInfo(packageName) != null; + } + + private PackageInfo getPackageInfo(String packageName) { + long token = Binder.clearCallingIdentity(); + try { + try { + return mPackageManager.getPackageInfo(packageName, 0); + } catch (PackageManager.NameNotFoundException e) { + return null; + } + } finally{ + Binder.restoreCallingIdentity(token); + } + } + } + """).indented(), *stubs + ).run().expect( + """ + src/com/android/server/lint/test/TestClass.java:10: Warning: \ + Api: hasPackage contains a package name parameter: packageName does not apply \ + package visibility filtering rules. \ + [ApiMightLeakAppVisibility] + public boolean hasPackage(String packageName) { + ~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testDoesNotDetectIssuesApiNotSystemPackagePrefix() { + lint().files(java( + """ + package com.test.not.system.prefix; + import android.internal.test.IFoo; + + public class TestClass extends IFoo.Stub { + @Override + public boolean hasPackage(String packageName) { + return packageName != null; + } + } + """ + ).indented(), *stubs).run().expectClean() + } + + private val contextStub: TestFile = java( + """ + package android.content; + + public abstract class Context { + public abstract int checkCallingPermission(String permission); + } + """ + ).indented() + + private val appOpsManagerStub: TestFile = java( + """ + package android.app; + + public class AppOpsManager { + public void checkPackage(int uid, String packageName) { + } + } + """ + ).indented() + + private val packageManagerStub: TestFile = java( + """ + package android.content.pm; + import android.content.pm.PackageInfo; + + public abstract class PackageManager { + public static class NameNotFoundException extends AndroidException { + } + + public abstract PackageInfo getPackageInfo(String packageName, int flags) + throws NameNotFoundException; + } + """ + ).indented() + + private val packageInfoStub: TestFile = java( + """ + package android.content.pm; + public class PackageInfo {} + """ + ).indented() + + private val binderStub: TestFile = java( + """ + package android.os; + + public class Binder { + public static final native long clearCallingIdentity(); + public static final native void restoreCallingIdentity(long token); + public static final native int getCallingUid(); + } + """ + ).indented() + + private val interfaceIFooStub: TestFile = java( + """ + package android.internal.test; + import android.os.Binder; + + public interface IFoo { + boolean hasPackage(String packageName); + public abstract static class Stub extends Binder implements IFoo { + } + } + """ + ).indented() + + private val stubs = arrayOf(contextStub, appOpsManagerStub, packageManagerStub, + packageInfoStub, binderStub, interfaceIFooStub) + + // Substitutes "backslash + new line" with an empty string to imitate line continuation + private fun String.addLineContinuation(): String = this.trimIndent().replace("\\\n", "") +} diff --git a/tools/lint/framework/checks/src/test/java/com/google/android/lint/parcel/SaferParcelCheckerTest.kt b/tools/lint/framework/checks/src/test/java/com/google/android/lint/parcel/SaferParcelCheckerTest.kt new file mode 100644 index 000000000000..e686695ca804 --- /dev/null +++ b/tools/lint/framework/checks/src/test/java/com/google/android/lint/parcel/SaferParcelCheckerTest.kt @@ -0,0 +1,823 @@ +/* + * 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.parcel + +import com.android.tools.lint.checks.infrastructure.LintDetectorTest +import com.android.tools.lint.checks.infrastructure.TestLintTask +import com.android.tools.lint.checks.infrastructure.TestMode +import com.android.tools.lint.detector.api.Detector +import com.android.tools.lint.detector.api.Issue + +@Suppress("UnstableApiUsage") +class SaferParcelCheckerTest : LintDetectorTest() { + override fun getDetector(): Detector = SaferParcelChecker() + + override fun getIssues(): List<Issue> = listOf( + SaferParcelChecker.ISSUE_UNSAFE_API_USAGE + ) + + override fun lint(): TestLintTask = + super.lint() + .allowMissingSdk(true) + // We don't do partial analysis in the platform + .skipTestModes(TestMode.PARTIAL) + + /** Parcel Tests */ + + fun testParcelDetectUnsafeReadSerializable() { + lint() + .files( + java( + """ + package test.pkg; + import android.os.Parcel; + import java.io.Serializable; + + public class TestClass { + private TestClass(Parcel p) { + Serializable ans = p.readSerializable(); + } + } + """ + ).indented(), + *includes + ) + .expectIdenticalTestModeOutput(false) + .run() + .expect( + """ + src/test/pkg/TestClass.java:7: Warning: Unsafe Parcel.readSerializable() \ + API usage [UnsafeParcelApi] + Serializable ans = p.readSerializable(); + ~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testParcelDoesNotDetectSafeReadSerializable() { + lint() + .files( + java( + """ + package test.pkg; + import android.os.Parcel; + import java.io.Serializable; + + public class TestClass { + private TestClass(Parcel p) { + String ans = p.readSerializable(null, String.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + fun testParcelDetectUnsafeReadArrayList() { + lint() + .files( + java( + """ + package test.pkg; + import android.os.Parcel; + + public class TestClass { + private TestClass(Parcel p) { + ArrayList ans = p.readArrayList(null); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:6: Warning: Unsafe Parcel.readArrayList() API \ + usage [UnsafeParcelApi] + ArrayList ans = p.readArrayList(null); + ~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testParcelDoesNotDetectSafeReadArrayList() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + + public class TestClass { + private TestClass(Parcel p) { + ArrayList<Intent> ans = p.readArrayList(null, Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + fun testParcelDetectUnsafeReadList() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + import java.util.List; + + public class TestClass { + private TestClass(Parcel p) { + List<Intent> list = new ArrayList<Intent>(); + p.readList(list, null); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:9: Warning: Unsafe Parcel.readList() API usage \ + [UnsafeParcelApi] + p.readList(list, null); + ~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testDParceloesNotDetectSafeReadList() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + import java.util.List; + + public class TestClass { + private TestClass(Parcel p) { + List<Intent> list = new ArrayList<Intent>(); + p.readList(list, null, Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + fun testParcelDetectUnsafeReadParcelable() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + + public class TestClass { + private TestClass(Parcel p) { + Intent ans = p.readParcelable(null); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:7: Warning: Unsafe Parcel.readParcelable() API \ + usage [UnsafeParcelApi] + Intent ans = p.readParcelable(null); + ~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testParcelDoesNotDetectSafeReadParcelable() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + + public class TestClass { + private TestClass(Parcel p) { + Intent ans = p.readParcelable(null, Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + fun testParcelDetectUnsafeReadParcelableList() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + import java.util.List; + + public class TestClass { + private TestClass(Parcel p) { + List<Intent> list = new ArrayList<Intent>(); + List<Intent> ans = p.readParcelableList(list, null); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:9: Warning: Unsafe Parcel.readParcelableList() \ + API usage [UnsafeParcelApi] + List<Intent> ans = p.readParcelableList(list, null); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testParcelDoesNotDetectSafeReadParcelableList() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + import java.util.List; + + public class TestClass { + private TestClass(Parcel p) { + List<Intent> list = new ArrayList<Intent>(); + List<Intent> ans = + p.readParcelableList(list, null, Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + fun testParcelDetectUnsafeReadSparseArray() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + import android.util.SparseArray; + + public class TestClass { + private TestClass(Parcel p) { + SparseArray<Intent> ans = p.readSparseArray(null); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:8: Warning: Unsafe Parcel.readSparseArray() API\ + usage [UnsafeParcelApi] + SparseArray<Intent> ans = p.readSparseArray(null); + ~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testParcelDoesNotDetectSafeReadSparseArray() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + import android.util.SparseArray; + + public class TestClass { + private TestClass(Parcel p) { + SparseArray<Intent> ans = + p.readSparseArray(null, Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + fun testParcelDetectUnsafeReadSArray() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + + public class TestClass { + private TestClass(Parcel p) { + Intent[] ans = p.readArray(null); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:7: Warning: Unsafe Parcel.readArray() API\ + usage [UnsafeParcelApi] + Intent[] ans = p.readArray(null); + ~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testParcelDoesNotDetectSafeReadArray() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + + public class TestClass { + private TestClass(Parcel p) { + Intent[] ans = p.readArray(null, Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + fun testParcelDetectUnsafeReadParcelableSArray() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + + public class TestClass { + private TestClass(Parcel p) { + Intent[] ans = p.readParcelableArray(null); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:7: Warning: Unsafe Parcel.readParcelableArray() API\ + usage [UnsafeParcelApi] + Intent[] ans = p.readParcelableArray(null); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testParcelDoesNotDetectSafeReadParcelableArray() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Parcel; + + public class TestClass { + private TestClass(Parcel p) { + Intent[] ans = p.readParcelableArray(null, Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + /** Bundle Tests */ + + fun testBundleDetectUnsafeGetParcelable() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Bundle; + + public class TestClass { + private TestClass(Bundle b) { + Intent ans = b.getParcelable("key"); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:7: Warning: Unsafe Bundle.getParcelable() API usage [UnsafeParcelApi] + Intent ans = b.getParcelable("key"); + ~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testBundleDoesNotDetectSafeGetParcelable() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Bundle; + + public class TestClass { + private TestClass(Bundle b) { + Intent ans = b.getParcelable("key", Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + fun testBundleDetectUnsafeGetParcelableArrayList() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Bundle; + + public class TestClass { + private TestClass(Bundle b) { + ArrayList<Intent> ans = b.getParcelableArrayList("key"); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:7: Warning: Unsafe Bundle.getParcelableArrayList() API usage [UnsafeParcelApi] + ArrayList<Intent> ans = b.getParcelableArrayList("key"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testBundleDoesNotDetectSafeGetParcelableArrayList() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Bundle; + + public class TestClass { + private TestClass(Bundle b) { + ArrayList<Intent> ans = b.getParcelableArrayList("key", Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + fun testBundleDetectUnsafeGetParcelableArray() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Bundle; + + public class TestClass { + private TestClass(Bundle b) { + Intent[] ans = b.getParcelableArray("key"); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:7: Warning: Unsafe Bundle.getParcelableArray() API usage [UnsafeParcelApi] + Intent[] ans = b.getParcelableArray("key"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testBundleDoesNotDetectSafeGetParcelableArray() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Bundle; + + public class TestClass { + private TestClass(Bundle b) { + Intent[] ans = b.getParcelableArray("key", Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + fun testBundleDetectUnsafeGetSparseParcelableArray() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Bundle; + + public class TestClass { + private TestClass(Bundle b) { + SparseArray<Intent> ans = b.getSparseParcelableArray("key"); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:7: Warning: Unsafe Bundle.getSparseParcelableArray() API usage [UnsafeParcelApi] + SparseArray<Intent> ans = b.getSparseParcelableArray("key"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testBundleDoesNotDetectSafeGetSparseParcelableArray() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + import android.os.Bundle; + + public class TestClass { + private TestClass(Bundle b) { + SparseArray<Intent> ans = b.getSparseParcelableArray("key", Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + /** Intent Tests */ + + fun testIntentDetectUnsafeGetParcelableExtra() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + + public class TestClass { + private TestClass(Intent i) { + Intent ans = i.getParcelableExtra("name"); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect( + """ + src/test/pkg/TestClass.java:6: Warning: Unsafe Intent.getParcelableExtra() API usage [UnsafeParcelApi] + Intent ans = i.getParcelableExtra("name"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """.addLineContinuation() + ) + } + + fun testIntentDoesNotDetectSafeGetParcelableExtra() { + lint() + .files( + java( + """ + package test.pkg; + import android.content.Intent; + + public class TestClass { + private TestClass(Intent i) { + Intent ans = i.getParcelableExtra("name", Intent.class); + } + } + """ + ).indented(), + *includes + ) + .run() + .expect("No warnings.") + } + + + /** Stubs for classes used for testing */ + + + private val includes = + arrayOf( + manifest().minSdk("33"), + java( + """ + package android.os; + import java.util.ArrayList; + import java.util.List; + import java.util.Map; + import java.util.HashMap; + + public final class Parcel { + // Deprecated + public Object[] readArray(ClassLoader loader) { return null; } + public ArrayList readArrayList(ClassLoader loader) { return null; } + public HashMap readHashMap(ClassLoader loader) { return null; } + public void readList(List outVal, ClassLoader loader) {} + public void readMap(Map outVal, ClassLoader loader) {} + public <T extends Parcelable> T readParcelable(ClassLoader loader) { return null; } + public Parcelable[] readParcelableArray(ClassLoader loader) { return null; } + public Parcelable.Creator<?> readParcelableCreator(ClassLoader loader) { return null; } + public <T extends Parcelable> List<T> readParcelableList(List<T> list, ClassLoader cl) { return null; } + public Serializable readSerializable() { return null; } + public <T> SparseArray<T> readSparseArray(ClassLoader loader) { return null; } + + // Replacements + public <T> T[] readArray(ClassLoader loader, Class<T> clazz) { return null; } + public <T> ArrayList<T> readArrayList(ClassLoader loader, Class<? extends T> clazz) { return null; } + public <K, V> HashMap<K,V> readHashMap(ClassLoader loader, Class<? extends K> clazzKey, Class<? extends V> clazzValue) { return null; } + public <T> void readList(List<? super T> outVal, ClassLoader loader, Class<T> clazz) {} + public <K, V> void readMap(Map<? super K, ? super V> outVal, ClassLoader loader, Class<K> clazzKey, Class<V> clazzValue) {} + public <T> T readParcelable(ClassLoader loader, Class<T> clazz) { return null; } + public <T> T[] readParcelableArray(ClassLoader loader, Class<T> clazz) { return null; } + public <T> Parcelable.Creator<T> readParcelableCreator(ClassLoader loader, Class<T> clazz) { return null; } + public <T> List<T> readParcelableList(List<T> list, ClassLoader cl, Class<T> clazz) { return null; } + public <T> T readSerializable(ClassLoader loader, Class<T> clazz) { return null; } + public <T> SparseArray<T> readSparseArray(ClassLoader loader, Class<? extends T> clazz) { return null; } + } + """ + ).indented(), + java( + """ + package android.os; + import java.util.ArrayList; + import java.util.List; + import java.util.Map; + import java.util.HashMap; + + public final class Bundle { + // Deprecated + public <T extends Parcelable> T getParcelable(String key) { return null; } + public <T extends Parcelable> ArrayList<T> getParcelableArrayList(String key) { return null; } + public Parcelable[] getParcelableArray(String key) { return null; } + public <T extends Parcelable> SparseArray<T> getSparseParcelableArray(String key) { return null; } + + // Replacements + public <T> T getParcelable(String key, Class<T> clazz) { return null; } + public <T> ArrayList<T> getParcelableArrayList(String key, Class<? extends T> clazz) { return null; } + public <T> T[] getParcelableArray(String key, Class<T> clazz) { return null; } + public <T> SparseArray<T> getSparseParcelableArray(String key, Class<? extends T> clazz) { return null; } + + } + """ + ).indented(), + java( + """ + package android.os; + public interface Parcelable {} + """ + ).indented(), + java( + """ + package android.content; + public class Intent implements Parcelable, Cloneable { + // Deprecated + public <T extends Parcelable> T getParcelableExtra(String name) { return null; } + + // Replacements + public <T> T getParcelableExtra(String name, Class<T> clazz) { return null; } + + } + """ + ).indented(), + java( + """ + package android.util; + public class SparseArray<E> implements Cloneable {} + """ + ).indented(), + ) + + // Substitutes "backslash + new line" with an empty string to imitate line continuation + private fun String.addLineContinuation(): String = this.trimIndent().replace("\\\n", "") +} diff --git a/tools/lint/global/Android.bp b/tools/lint/global/Android.bp new file mode 100644 index 000000000000..bedb7bd78a29 --- /dev/null +++ b/tools/lint/global/Android.bp @@ -0,0 +1,65 @@ +// 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 { + // See: http://go/android-license-faq + // A large-scale-change added 'default_applicable_licenses' to import + // all of the 'license_kinds' from "frameworks_base_license" + // to get the below license kinds: + // SPDX-license-identifier-Apache-2.0 + default_applicable_licenses: ["frameworks_base_license"], +} + +java_library_host { + name: "AndroidGlobalLintChecker", + srcs: ["checks/src/main/java/**/*.kt"], + plugins: ["auto_service_plugin"], + libs: [ + "auto_service_annotations", + "lint_api", + ], + static_libs: ["AndroidCommonLint"], + kotlincflags: ["-Xjvm-default=all"], + dist: { + targets: ["droid"], + }, +} + +java_test_host { + name: "AndroidGlobalLintCheckerTest", + srcs: ["checks/src/test/java/**/*.kt"], + static_libs: [ + "AndroidGlobalLintChecker", + "junit", + "lint", + "lint_tests", + ], + test_options: { + unit_test: true, + tradefed_options: [ + { + // lint bundles in some classes that were built with older versions + // of libraries, and no longer load. Since tradefed tries to load + // all classes in the jar to look for tests, it crashes loading them. + // Exclude these classes from tradefed's search. + name: "exclude-paths", + value: "org/apache", + }, + { + name: "exclude-paths", + value: "META-INF", + }, + ], + }, +} diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt index a6fd9bba6192..a20266a9b140 100644 --- a/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt @@ -1,5 +1,5 @@ /* - * Copyright (C) 2021 The Android Open Source Project + * 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. @@ -19,21 +19,19 @@ package com.google.android.lint 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 @AutoService(IssueRegistry::class) @Suppress("UnstableApiUsage") -class AndroidFrameworkIssueRegistry : IssueRegistry() { +class AndroidGlobalIssueRegistry : IssueRegistry() { override val issues = listOf( - CallingIdentityTokenDetector.ISSUE_UNUSED_TOKEN, - CallingIdentityTokenDetector.ISSUE_NON_FINAL_TOKEN, - CallingIdentityTokenDetector.ISSUE_NESTED_CLEAR_IDENTITY_CALLS, - CallingIdentityTokenDetector.ISSUE_RESTORE_IDENTITY_CALL_NOT_IN_FINALLY_BLOCK, - CallingIdentityTokenDetector.ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, - CallingIdentityTokenDetector.ISSUE_CLEAR_IDENTITY_CALL_NOT_FOLLOWED_BY_TRY_FINALLY, - CallingSettingsNonUserGetterMethodsDetector.ISSUE_NON_USER_GETTER_CALLED, EnforcePermissionDetector.ISSUE_MISSING_ENFORCE_PERMISSION, - EnforcePermissionDetector.ISSUE_MISMATCHING_ENFORCE_PERMISSION + EnforcePermissionDetector.ISSUE_MISMATCHING_ENFORCE_PERMISSION, + EnforcePermissionHelperDetector.ISSUE_ENFORCE_PERMISSION_HELPER, + SimpleManualPermissionEnforcementDetector.ISSUE_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT, ) override val api: Int @@ -45,6 +43,6 @@ class AndroidFrameworkIssueRegistry : IssueRegistry() { override val vendor: Vendor = Vendor( vendorName = "Android", feedbackUrl = "http://b/issues/new?component=315013", - contact = "brufino@google.com" + 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/AidlImplementationDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/AidlImplementationDetector.kt new file mode 100644 index 000000000000..ab6d871d6ea6 --- /dev/null +++ b/tools/lint/global/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<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) { + val interfaceName = getContainingAidlInterface(context, 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/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt new file mode 100644 index 000000000000..dcfbe953f955 --- /dev/null +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt @@ -0,0 +1,76 @@ +/* + * 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 + +const val ANNOTATION_ENFORCE_PERMISSION = "android.annotation.EnforcePermission" +const val ANNOTATION_REQUIRES_NO_PERMISSION = "android.annotation.RequiresNoPermission" +const val ANNOTATION_PERMISSION_MANUALLY_ENFORCED = "android.annotation.PermissionManuallyEnforced" + +val AIDL_PERMISSION_ANNOTATIONS = listOf( + ANNOTATION_ENFORCE_PERMISSION, + ANNOTATION_REQUIRES_NO_PERMISSION, + ANNOTATION_PERMISSION_MANUALLY_ENFORCED +) + +const val BINDER_CLASS = "android.os.Binder" +const val IINTERFACE_INTERFACE = "android.os.IInterface" + +const val AIDL_PERMISSION_HELPER_SUFFIX = "_enforcePermission" + +/** + * If a non java (e.g. c++) backend is enabled, the @EnforcePermission + * annotation cannot be used. At time of writing, the mechanism + * is not implemented for non java backends. + * TODO: b/242564874 (have lint know which interfaces have the c++ backend enabled) + * rather than hard coding this list? + */ +val EXCLUDED_CPP_INTERFACES = listOf( + "AdbTransportType", + "FingerprintAndPairDevice", + "IAdbCallback", + "IAdbManager", + "PairDevice", + "IStatsBootstrapAtomService", + "StatsBootstrapAtom", + "StatsBootstrapAtomValue", + "FixedSizeArrayExample", + "PlaybackTrackMetadata", + "RecordTrackMetadata", + "SinkMetadata", + "SourceMetadata", + "IUpdateEngineStable", + "IUpdateEngineStableCallback", + "AudioCapabilities", + "ConfidenceLevel", + "ModelParameter", + "ModelParameterRange", + "Phrase", + "PhraseRecognitionEvent", + "PhraseRecognitionExtra", + "PhraseSoundModel", + "Properties", + "RecognitionConfig", + "RecognitionEvent", + "RecognitionMode", + "RecognitionStatus", + "SoundModel", + "SoundModelType", + "Status", + "IThermalService", + "IPowerManager", + "ITunerResourceManager" +) 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 new file mode 100644 index 000000000000..0baac2c7aacf --- /dev/null +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt @@ -0,0 +1,226 @@ +/* + * 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.AnnotationInfo +import com.android.tools.lint.detector.api.AnnotationOrigin +import com.android.tools.lint.detector.api.AnnotationUsageInfo +import com.android.tools.lint.detector.api.AnnotationUsageType +import com.android.tools.lint.detector.api.Category +import com.android.tools.lint.detector.api.ConstantEvaluator +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.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.UElement +import org.jetbrains.uast.UMethod +import org.jetbrains.uast.toUElement + +/** + * 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. + * + * 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 + * its ancestor also annotated. This is to avoid having an annotation on a + * Java method without the corresponding annotation on the AIDL interface. + */ +class EnforcePermissionDetector : Detector(), SourceCodeScanner { + + override fun applicableAnnotations(): List<String> { + return listOf(ANNOTATION_ENFORCE_PERMISSION) + } + + override fun getApplicableUastTypes(): List<Class<out UElement>> { + return listOf(UAnnotation::class.java) + } + + private fun annotationValueGetChildren(elem: PsiElement): Array<PsiElement> { + if (elem is PsiArrayInitializerMemberValue) + return elem.getInitializers().map { it as PsiElement }.toTypedArray() + return elem.getChildren() + } + + private fun areAnnotationsEquivalent( + context: JavaContext, + anno1: PsiAnnotation, + anno2: PsiAnnotation + ): Boolean { + if (anno1.qualifiedName != anno2.qualifiedName) { + return false + } + val attr1 = anno1.parameterList.attributes + val attr2 = anno2.parameterList.attributes + if (attr1.size != attr2.size) { + return false + } + for (i in attr1.indices) { + if (attr1[i].name != attr2[i].name) { + return false + } + val value1 = attr1[i].value ?: return false + val value2 = attr2[i].value ?: return false + // Try to compare values directly with each other. + val v1 = ConstantEvaluator.evaluate(context, value1) + val v2 = ConstantEvaluator.evaluate(context, value2) + if (v1 != null && v2 != null) { + if (v1 != v2) { + return false + } + } else { + val children1 = annotationValueGetChildren(value1) + val children2 = annotationValueGetChildren(value2) + if (children1.size != children2.size) { + return false + } + for (j in children1.indices) { + val c1 = ConstantEvaluator.evaluate(context, children1[j]) + val c2 = ConstantEvaluator.evaluate(context, children2[j]) + if (c1 != c2) { + return false + } + } + } + } + return true + } + + private fun compareMethods( + context: JavaContext, + element: UElement, + overridingMethod: PsiMethod, + 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) + val overridingClass = overridingMethod.parent as PsiClass + val overriddenClass = overriddenMethod.parent as PsiClass + val overridingName = "${overridingClass.name}.${overridingMethod.name}" + val overriddenName = "${overriddenClass.name}.${overriddenMethod.name}" + if (overridingAnnotation == null) { + val msg = "The method $overridingName overrides the method $overriddenName which " + + "is annotated with @EnforcePermission. The same annotation must be used " + + "on $overridingName" + context.report(ISSUE_MISSING_ENFORCE_PERMISSION, element, location, msg) + } else if (overriddenAnnotation == null) { + val msg = "The method $overridingName overrides the method $overriddenName which " + + "is not annotated with @EnforcePermission. The same annotation must be " + + "used on $overriddenName. Did you forget to annotate the AIDL definition?" + context.report(ISSUE_MISSING_ENFORCE_PERMISSION, element, location, msg) + } else if (checkEquivalence && !areAnnotationsEquivalent( + context, overridingAnnotation, overriddenAnnotation)) { + val msg = "The method $overridingName is annotated with " + + "${overridingAnnotation.text} which differs from the overridden " + + "method $overriddenName: ${overriddenAnnotation.text}. The same " + + "annotation must be used for both methods." + context.report(ISSUE_MISMATCHING_ENFORCE_PERMISSION, element, location, msg) + } + } + + override fun visitAnnotationUsage( + context: JavaContext, + element: UElement, + annotationInfo: AnnotationInfo, + usageInfo: AnnotationUsageInfo + ) { + if (usageInfo.type == AnnotationUsageType.METHOD_OVERRIDE && + annotationInfo.origin == AnnotationOrigin.METHOD) { + 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) + } + } + } + } + + companion object { + 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. + + In order to surface that information to platform developers, the same annotation must be + used on the implementation class or methods. + """ + + val ISSUE_MISSING_ENFORCE_PERMISSION: Issue = Issue.create( + id = "MissingEnforcePermissionAnnotation", + briefDescription = "Missing @EnforcePermission annotation on Binder method", + explanation = EXPLANATION, + category = Category.SECURITY, + priority = 6, + severity = Severity.ERROR, + implementation = Implementation( + EnforcePermissionDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + + val ISSUE_MISMATCHING_ENFORCE_PERMISSION: Issue = Issue.create( + id = "MismatchingEnforcePermissionAnnotation", + briefDescription = "Incorrect @EnforcePermission annotation on Binder method", + explanation = EXPLANATION, + category = Category.SECURITY, + priority = 6, + severity = Severity.ERROR, + implementation = Implementation( + EnforcePermissionDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + } +} 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 new file mode 100644 index 000000000000..25d208db14ec --- /dev/null +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionFix.kt @@ -0,0 +1,384 @@ +/* + * 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.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 +import com.google.android.lint.getPermissionMethodAnnotation +import com.google.android.lint.hasPermissionNameAnnotation +import com.google.android.lint.isPermissionMethodCall +import com.intellij.psi.PsiClassType +import com.intellij.psi.PsiType +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.UMethod +import org.jetbrains.uast.UThrowExpression +import org.jetbrains.uast.UastBinaryOperator +import org.jetbrains.uast.evaluateString +import org.jetbrains.uast.skipParenthesizedExprDown +import org.jetbrains.uast.visitor.AbstractUastVisitor + +/** + * Helper class that facilitates the creation of lint auto fixes + */ +data class EnforcePermissionFix( + val manualCheckLocations: List<Location>, + val permissionNames: List<String>, + val errorLevel: Boolean, + val anyOf: Boolean, +) { + fun toLintFix(context: JavaContext, node: UMethod): LintFix { + val methodLocation = context.getLocation(node) + val replaceOrRemoveFixes = manualCheckLocations.mapIndexed { index, manualCheckLocation -> + if (index == 0) { + // Replace the first manual check with a call to the helper method + getHelperMethodFix(node, manualCheckLocation, false) + } else { + // Remove all subsequent manual checks + LintFix.create() + .replace() + .reformat(true) + .range(manualCheckLocation) + .with("") + .autoFix() + .build() + } + } + + // Annotate the method with @EnforcePermission(...) + val annotateFix = LintFix.create() + .annotate(annotation) + .range(methodLocation) + .autoFix() + .build() + + return LintFix.create().composite(annotateFix, *replaceOrRemoveFixes.toTypedArray()) + } + + private val annotation: String + get() { + val quotedPermissions = permissionNames.joinToString(", ") { """"$it"""" } + + val attributeName = + if (permissionNames.size > 1) { + if (anyOf) "anyOf" else "allOf" + } else null + + val annotationParameter = + if (attributeName != null) "$attributeName={$quotedPermissions}" + else quotedPermissions + + return "@$ANNOTATION_ENFORCE_PERMISSION($annotationParameter)" + } + + companion object { + /** + * Walks the expressions in a block, looking for simple permission checks. + * + * 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. + */ + fun fromBlockExpression( + context: JavaContext, + blockExpression: UBlockExpression + ): EnforcePermissionFix? { + try { + val singleFixes = mutableListOf<EnforcePermissionFix>() + for (expression in blockExpression.expressions) { + val fix = fromExpression(context, expression) ?: break + singleFixes.add(fix) + } + return compose(singleFixes) + } catch (e: AnyOfAllOfException) { + return null + } + } + + /** + * Conditionally constructs EnforcePermissionFix from any UExpression + * + * @return EnforcePermissionFix if the expression boils down to a permission check, + * else null + */ + fun fromExpression( + context: JavaContext, + expression: UExpression + ): EnforcePermissionFix? { + val trimmedExpression = expression.skipParenthesizedExprDown() + if (trimmedExpression is UIfExpression) { + return fromIfExpression(context, trimmedExpression) + } + findCallExpression(trimmedExpression)?.let { + return fromCallExpression(context, it) + } + return null + } + + /** + * Conditionally constructs EnforcePermissionFix from a UCallExpression + * + * @return EnforcePermissionFix if the called method is annotated with @PermissionMethod, else null + */ + fun fromCallExpression( + context: JavaContext, + callExpression: UCallExpression + ): EnforcePermissionFix? { + val method = callExpression.resolve()?.getUMethod() ?: return null + 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), + anyOf, + ) + } + + /** + * Conditionally constructs EnforcePermissionFix from a UCallExpression + * + * @return EnforcePermissionFix IF AND ONLY IF: + * * The condition of the if statement compares the return value of a + * PermissionMethod to one of the PackageManager.PermissionResult values + * * The expression inside the if statement does nothing but throw SecurityException + */ + fun fromIfExpression( + context: JavaContext, + ifExpression: UIfExpression + ): EnforcePermissionFix? { + val condition = ifExpression.condition.skipParenthesizedExprDown() + if (condition !is UBinaryExpression) return null + + val maybeLeftCall = findCallExpression(condition.leftOperand) + val maybeRightCall = findCallExpression(condition.rightOperand) + + val (callExpression, comparison) = + if (maybeLeftCall is UCallExpression) { + Pair(maybeLeftCall, condition.rightOperand) + } else if (maybeRightCall is UCallExpression) { + Pair(maybeRightCall, condition.leftOperand) + } else return null + + val permissionMethodAnnotation = getPermissionMethodAnnotation( + callExpression.resolve()?.getUMethod()) ?: return null + + val equalityCheck = + when (comparison.findSelector().asSourceString() + .filterNot(Char::isWhitespace)) { + "PERMISSION_GRANTED" -> UastBinaryOperator.IDENTITY_NOT_EQUALS + "PERMISSION_DENIED" -> UastBinaryOperator.IDENTITY_EQUALS + else -> return null + } + + if (condition.operator != equalityCheck) return null + + val throwExpression: UThrowExpression? = + ifExpression.thenExpression as? UThrowExpression + ?: (ifExpression.thenExpression as? UBlockExpression) + ?.expressions?.firstOrNull() + as? UThrowExpression + + + val thrownClass = (throwExpression?.thrownExpression?.getExpressionType() + as? PsiClassType)?.resolve() ?: return null + if (!context.evaluator.inheritsFrom( + thrownClass, "java.lang.SecurityException")){ + return null + } + + 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? { + if (individuals.isEmpty()) return null + 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::manualCheckLocations), + individuals.flatMap(EnforcePermissionFix::permissionNames), + errorLevel = individuals.all(EnforcePermissionFix::errorLevel), + anyOf = anyOfs.isNotEmpty() + ) + } + + /** + * Given a permission check, get its proper location + * so that a lint fix can remove the entire expression + */ + private fun getPermissionCheckLocation( + context: JavaContext, + callExpression: UCallExpression + ): + Location { + val javaPsi = callExpression.javaPsi!! + return Location.create( + context.file, + javaPsi.containingFile?.text, + javaPsi.textRange.startOffset, + // unfortunately the element doesn't include the ending semicolon + javaPsi.textRange.endOffset + 1 + ) + } + + /** + * Given a @PermissionMethod, find arguments annotated with @PermissionName + * 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<String> { + if (!isPermissionMethodCall(callExpression)) return emptyList() + + val result = mutableSetOf<String>() // protect against duplicate permission values + val visitedCalls = mutableSetOf<UCallExpression>() // don't visit the same call twice + val bfsQueue = ArrayDeque(listOf(callExpression)) + + var anyOfAllOfState: AnyOfAllOfState = AnyOfAllOfState.INITIAL + + // Bread First Search - evaluating nested @PermissionMethod(s) in the available + // source code for @PermissionName(s). + while (bfsQueue.isNotEmpty()) { + val currentCallExpression = bfsQueue.removeFirst() + visitedCalls.add(currentCallExpression) + val currentPermissions = findPermissions(currentCallExpression) + result.addAll(currentPermissions) + + 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<UCallExpression>, + val bfsQueue: ArrayDeque<UCallExpression> + ) : AbstractUastVisitor() { + override fun visitCallExpression(node: UCallExpression): Boolean { + if (isPermissionMethodCall(node) && node !in visitedCalls) { + bfsQueue.add(node) + } + return false + } + } + + private fun findPermissions( + callExpression: UCallExpression, + ): List<String> { + val annotation = getPermissionMethodAnnotation(callExpression.resolve()?.getUMethod()) + + 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 + } + + /** + * If we detect that the PermissionMethod enforces that permission is granted, + * AND is of the "orSelf" variety, we are very confident that this is a behavior + * preserving migration to @EnforcePermission. Thus, the incident should be ERROR + * level. + */ + 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/EnforcePermissionHelperDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt new file mode 100644 index 000000000000..df13af516514 --- /dev/null +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt @@ -0,0 +1,149 @@ +/* + * 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 + +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, + Scope.JAVA_FILE_SCOPE + ) + ) + + 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, + Scope.JAVA_FILE_SCOPE + ) + ) + + /** + * 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/EnforcePermissionUtils.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt new file mode 100644 index 000000000000..d41fee3fc0dc --- /dev/null +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt @@ -0,0 +1,96 @@ +/* + * 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.JavaContext +import com.android.tools.lint.detector.api.LintFix +import com.android.tools.lint.detector.api.Location +import com.intellij.psi.PsiClass +import com.intellij.psi.PsiReferenceList +import org.jetbrains.uast.UMethod + +/** + * Given a UMethod, determine if this method is + * the entrypoint to an interface generated by AIDL, + * returning the interface name if so, otherwise returning null + */ +fun getContainingAidlInterface(context: JavaContext, node: UMethod): String? { + if (!isContainedInSubclassOfStub(context, node)) return null + for (superMethod in node.findSuperMethods()) { + for (extendsInterface in superMethod.containingClass?.extendsList?.referenceElements + ?: continue) { + if (extendsInterface.qualifiedName == IINTERFACE_INTERFACE) { + return superMethod.containingClass?.name + } + } + } + return null +} + +fun isContainedInSubclassOfStub(context: JavaContext, node: UMethod?): Boolean { + var superClass = node?.containingClass?.superClass + while (superClass != null) { + if (isStub(context, superClass)) return true + superClass = superClass.superClass + } + return false +} + +fun isStub(context: JavaContext, psiClass: PsiClass?): Boolean { + if (psiClass == null) return false + if (psiClass.name != "Stub") return false + if (!context.evaluator.isStatic(psiClass)) return false + if (!context.evaluator.isAbstract(psiClass)) return false + + if (!hasSingleAncestor(psiClass.extendsList, BINDER_CLASS)) return false + + val parent = psiClass.parent as? PsiClass ?: return false + if (!hasSingleAncestor(parent.extendsList, IINTERFACE_INTERFACE)) return false + + val parentName = parent.qualifiedName ?: return false + if (!hasSingleAncestor(psiClass.implementsList, parentName)) return false + + return true +} + +private fun hasSingleAncestor(references: PsiReferenceList?, qualifiedName: String) = + references != null && + references.referenceElements.size == 1 && + references.referenceElements[0].qualifiedName == qualifiedName + +fun getHelperMethodCallSourceString(node: UMethod) = "${node.name}$AIDL_PERMISSION_HELPER_SUFFIX()" + +fun getHelperMethodFix( + node: UMethod, + manualCheckLocation: Location, + prepend: Boolean = true +): LintFix { + val helperMethodSource = getHelperMethodCallSourceString(node) + val indent = " ".repeat(manualCheckLocation.start?.column ?: 0) + val newText = "$helperMethodSource;${if (prepend) "\n\n$indent" else ""}" + + val fix = LintFix.create() + .replace() + .range(manualCheckLocation) + .with(newText) + .reformat(true) + .autoFix() + + if (prepend) fix.beginning() + + return fix.build() +} 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 new file mode 100644 index 000000000000..c7be36efd991 --- /dev/null +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt @@ -0,0 +1,92 @@ +/* + * 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.Incident +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.UMethod + +/** + * Looks for methods implementing generated AIDL interface stubs + * that can have simple permission checks migrated to + * @EnforcePermission annotations + */ +@Suppress("UnstableApiUsage") +class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { + override fun visitAidlMethod( + context: JavaContext, + node: UMethod, + interfaceName: String, + body: UBlockExpression + ) { + val enforcePermissionFix = EnforcePermissionFix.fromBlockExpression(context, body) ?: return + val lintFix = enforcePermissionFix.toLintFix(context, node) + val message = + "$interfaceName permission check ${ + if (enforcePermissionFix.errorLevel) "should" else "can" + } be converted to @EnforcePermission annotation" + + val incident = Incident( + ISSUE_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT, + enforcePermissionFix.manualCheckLocations.last(), + message, + lintFix + ) + + // TODO(b/265014041): turn on errors once all code that would cause one is fixed + // if (enforcePermissionFix.errorLevel) { + // incident.overrideSeverity(Severity.ERROR) + // } + + context.report(incident) + } + + 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_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT = 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 + ), + ) + } +} diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorCodegenTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorCodegenTest.kt new file mode 100644 index 000000000000..f2930d9faac7 --- /dev/null +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorCodegenTest.kt @@ -0,0 +1,557 @@ +/* + * 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.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 + +@Suppress("UnstableApiUsage") +class EnforcePermissionDetectorCodegenTest : LintDetectorTest() { + override fun getDetector(): Detector = EnforcePermissionDetector() + + override fun getIssues(): List<Issue> = listOf( + EnforcePermissionDetector.ISSUE_MISSING_ENFORCE_PERMISSION, + EnforcePermissionDetector.ISSUE_MISMATCHING_ENFORCE_PERMISSION + ) + + override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) + + fun test_generated_IProtected() { + lint().files( + java( + """ + /* + * This file is auto-generated. DO NOT MODIFY. + */ + package android.aidl.tests.permission; + public interface IProtected extends android.os.IInterface + { + /** Default implementation for IProtected. */ + public static class Default implements android.aidl.tests.permission.IProtected + { + @Override public void PermissionProtected() throws android.os.RemoteException + { + } + @Override public void MultiplePermissionsAll() throws android.os.RemoteException + { + } + @Override public void MultiplePermissionsAny() throws android.os.RemoteException + { + } + @Override public void NonManifestPermission() throws android.os.RemoteException + { + } + // Used by the integration tests to dynamically set permissions that are considered granted. + @Override public void SetGranted(java.util.List<java.lang.String> permissions) throws android.os.RemoteException + { + } + @Override + public android.os.IBinder asBinder() { + return null; + } + } + /** Local-side IPC implementation stub class. */ + public static abstract class Stub extends android.os.Binder implements android.aidl.tests.permission.IProtected + { + private final android.os.PermissionEnforcer mEnforcer; + /** Construct the stub using the Enforcer provided. */ + public Stub(android.os.PermissionEnforcer enforcer) + { + this.attachInterface(this, DESCRIPTOR); + if (enforcer == null) { + throw new IllegalArgumentException("enforcer cannot be null"); + } + mEnforcer = enforcer; + } + @Deprecated + /** Default constructor. */ + public Stub() { + this(android.os.PermissionEnforcer.fromContext( + android.app.ActivityThread.currentActivityThread().getSystemContext())); + } + /** + * Cast an IBinder object into an android.aidl.tests.permission.IProtected interface, + * generating a proxy if needed. + */ + public static android.aidl.tests.permission.IProtected asInterface(android.os.IBinder obj) + { + if ((obj==null)) { + return null; + } + android.os.IInterface iin = obj.queryLocalInterface(DESCRIPTOR); + if (((iin!=null)&&(iin instanceof android.aidl.tests.permission.IProtected))) { + return ((android.aidl.tests.permission.IProtected)iin); + } + return new android.aidl.tests.permission.IProtected.Stub.Proxy(obj); + } + @Override public android.os.IBinder asBinder() + { + return this; + } + /** @hide */ + public static java.lang.String getDefaultTransactionName(int transactionCode) + { + switch (transactionCode) + { + case TRANSACTION_PermissionProtected: + { + return "PermissionProtected"; + } + case TRANSACTION_MultiplePermissionsAll: + { + return "MultiplePermissionsAll"; + } + case TRANSACTION_MultiplePermissionsAny: + { + return "MultiplePermissionsAny"; + } + case TRANSACTION_NonManifestPermission: + { + return "NonManifestPermission"; + } + case TRANSACTION_SetGranted: + { + return "SetGranted"; + } + default: + { + return null; + } + } + } + /** @hide */ + public java.lang.String getTransactionName(int transactionCode) + { + return this.getDefaultTransactionName(transactionCode); + } + @Override public boolean onTransact(int code, android.os.Parcel data, android.os.Parcel reply, int flags) throws android.os.RemoteException + { + java.lang.String descriptor = DESCRIPTOR; + if (code >= android.os.IBinder.FIRST_CALL_TRANSACTION && code <= android.os.IBinder.LAST_CALL_TRANSACTION) { + data.enforceInterface(descriptor); + } + switch (code) + { + case INTERFACE_TRANSACTION: + { + reply.writeString(descriptor); + return true; + } + } + switch (code) + { + case TRANSACTION_PermissionProtected: + { + this.PermissionProtected(); + reply.writeNoException(); + break; + } + case TRANSACTION_MultiplePermissionsAll: + { + this.MultiplePermissionsAll(); + reply.writeNoException(); + break; + } + case TRANSACTION_MultiplePermissionsAny: + { + this.MultiplePermissionsAny(); + reply.writeNoException(); + break; + } + case TRANSACTION_NonManifestPermission: + { + this.NonManifestPermission(); + reply.writeNoException(); + break; + } + case TRANSACTION_SetGranted: + { + java.util.List<java.lang.String> _arg0; + _arg0 = data.createStringArrayList(); + data.enforceNoDataAvail(); + this.SetGranted(_arg0); + reply.writeNoException(); + break; + } + default: + { + return super.onTransact(code, data, reply, flags); + } + } + return true; + } + private static class Proxy implements android.aidl.tests.permission.IProtected + { + private android.os.IBinder mRemote; + Proxy(android.os.IBinder remote) + { + mRemote = remote; + } + @Override public android.os.IBinder asBinder() + { + return mRemote; + } + public java.lang.String getInterfaceDescriptor() + { + return DESCRIPTOR; + } + @Override public void PermissionProtected() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_PermissionProtected, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + @Override public void MultiplePermissionsAll() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_MultiplePermissionsAll, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + @Override public void MultiplePermissionsAny() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_MultiplePermissionsAny, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + @Override public void NonManifestPermission() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_NonManifestPermission, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + // Used by the integration tests to dynamically set permissions that are considered granted. + @Override public void SetGranted(java.util.List<java.lang.String> permissions) throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + _data.writeStringList(permissions); + boolean _status = mRemote.transact(Stub.TRANSACTION_SetGranted, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + } + static final int TRANSACTION_PermissionProtected = (android.os.IBinder.FIRST_CALL_TRANSACTION + 0); + /** Helper method to enforce permissions for PermissionProtected */ + protected void PermissionProtected_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermission(android.Manifest.permission.READ_PHONE_STATE, source); + } + static final int TRANSACTION_MultiplePermissionsAll = (android.os.IBinder.FIRST_CALL_TRANSACTION + 1); + /** Helper method to enforce permissions for MultiplePermissionsAll */ + protected void MultiplePermissionsAll_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermissionAllOf(new String[]{android.Manifest.permission.INTERNET, android.Manifest.permission.VIBRATE}, source); + } + static final int TRANSACTION_MultiplePermissionsAny = (android.os.IBinder.FIRST_CALL_TRANSACTION + 2); + /** Helper method to enforce permissions for MultiplePermissionsAny */ + protected void MultiplePermissionsAny_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermissionAnyOf(new String[]{android.Manifest.permission.INTERNET, android.Manifest.permission.VIBRATE}, source); + } + static final int TRANSACTION_NonManifestPermission = (android.os.IBinder.FIRST_CALL_TRANSACTION + 3); + /** Helper method to enforce permissions for NonManifestPermission */ + protected void NonManifestPermission_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermission(android.net.NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK, source); + } + static final int TRANSACTION_SetGranted = (android.os.IBinder.FIRST_CALL_TRANSACTION + 4); + /** @hide */ + public int getMaxTransactionId() + { + return 4; + } + } + + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + public void PermissionProtected() throws android.os.RemoteException; + @android.annotation.EnforcePermission(allOf = {android.Manifest.permission.INTERNET, android.Manifest.permission.VIBRATE}) + public void MultiplePermissionsAll() throws android.os.RemoteException; + @android.annotation.EnforcePermission(anyOf = {android.Manifest.permission.INTERNET, android.Manifest.permission.VIBRATE}) + public void MultiplePermissionsAny() throws android.os.RemoteException; + @android.annotation.EnforcePermission(android.net.NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK) + public void NonManifestPermission() throws android.os.RemoteException; + // Used by the integration tests to dynamically set permissions that are considered granted. + @android.annotation.RequiresNoPermission + public void SetGranted(java.util.List<java.lang.String> permissions) throws android.os.RemoteException; + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun test_generated_IProtectedInterface() { + lint().files( + java( + """ + /* + * This file is auto-generated. DO NOT MODIFY. + */ + package android.aidl.tests.permission; + public interface IProtectedInterface extends android.os.IInterface + { + /** Default implementation for IProtectedInterface. */ + public static class Default implements android.aidl.tests.permission.IProtectedInterface + { + @Override public void Method1() throws android.os.RemoteException + { + } + @Override public void Method2() throws android.os.RemoteException + { + } + @Override + public android.os.IBinder asBinder() { + return null; + } + } + /** Local-side IPC implementation stub class. */ + public static abstract class Stub extends android.os.Binder implements android.aidl.tests.permission.IProtectedInterface + { + private final android.os.PermissionEnforcer mEnforcer; + /** Construct the stub using the Enforcer provided. */ + public Stub(android.os.PermissionEnforcer enforcer) + { + this.attachInterface(this, DESCRIPTOR); + if (enforcer == null) { + throw new IllegalArgumentException("enforcer cannot be null"); + } + mEnforcer = enforcer; + } + @Deprecated + /** Default constructor. */ + public Stub() { + this(android.os.PermissionEnforcer.fromContext( + android.app.ActivityThread.currentActivityThread().getSystemContext())); + } + /** + * Cast an IBinder object into an android.aidl.tests.permission.IProtectedInterface interface, + * generating a proxy if needed. + */ + public static android.aidl.tests.permission.IProtectedInterface asInterface(android.os.IBinder obj) + { + if ((obj==null)) { + return null; + } + android.os.IInterface iin = obj.queryLocalInterface(DESCRIPTOR); + if (((iin!=null)&&(iin instanceof android.aidl.tests.permission.IProtectedInterface))) { + return ((android.aidl.tests.permission.IProtectedInterface)iin); + } + return new android.aidl.tests.permission.IProtectedInterface.Stub.Proxy(obj); + } + @Override public android.os.IBinder asBinder() + { + return this; + } + /** @hide */ + public static java.lang.String getDefaultTransactionName(int transactionCode) + { + switch (transactionCode) + { + case TRANSACTION_Method1: + { + return "Method1"; + } + case TRANSACTION_Method2: + { + return "Method2"; + } + default: + { + return null; + } + } + } + /** @hide */ + public java.lang.String getTransactionName(int transactionCode) + { + return this.getDefaultTransactionName(transactionCode); + } + @Override public boolean onTransact(int code, android.os.Parcel data, android.os.Parcel reply, int flags) throws android.os.RemoteException + { + java.lang.String descriptor = DESCRIPTOR; + if (code >= android.os.IBinder.FIRST_CALL_TRANSACTION && code <= android.os.IBinder.LAST_CALL_TRANSACTION) { + data.enforceInterface(descriptor); + } + switch (code) + { + case INTERFACE_TRANSACTION: + { + reply.writeString(descriptor); + return true; + } + } + switch (code) + { + case TRANSACTION_Method1: + { + this.Method1(); + reply.writeNoException(); + break; + } + case TRANSACTION_Method2: + { + this.Method2(); + reply.writeNoException(); + break; + } + default: + { + return super.onTransact(code, data, reply, flags); + } + } + return true; + } + private static class Proxy implements android.aidl.tests.permission.IProtectedInterface + { + private android.os.IBinder mRemote; + Proxy(android.os.IBinder remote) + { + mRemote = remote; + } + @Override public android.os.IBinder asBinder() + { + return mRemote; + } + public java.lang.String getInterfaceDescriptor() + { + return DESCRIPTOR; + } + @Override public void Method1() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_Method1, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + @Override public void Method2() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_Method2, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + } + static final int TRANSACTION_Method1 = (android.os.IBinder.FIRST_CALL_TRANSACTION + 0); + /** Helper method to enforce permissions for Method1 */ + protected void Method1_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermission(android.Manifest.permission.ACCESS_FINE_LOCATION, source); + } + static final int TRANSACTION_Method2 = (android.os.IBinder.FIRST_CALL_TRANSACTION + 1); + /** Helper method to enforce permissions for Method2 */ + protected void Method2_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermission(android.Manifest.permission.ACCESS_FINE_LOCATION, source); + } + /** @hide */ + public int getMaxTransactionId() + { + return 1; + } + } + + @android.annotation.EnforcePermission(android.Manifest.permission.ACCESS_FINE_LOCATION) + public void Method1() throws android.os.RemoteException; + @android.annotation.EnforcePermission(android.Manifest.permission.ACCESS_FINE_LOCATION) + public void Method2() throws android.os.RemoteException; + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + /* Stubs */ + + private val manifestPermissionStub: TestFile = java( + """ + package android.Manifest; + class permission { + public static final String READ_PHONE_STATE = "android.permission.READ_PHONE_STATE"; + public static final String INTERNET = "android.permission.INTERNET"; + } + """ + ).indented() + + private val enforcePermissionAnnotationStub: TestFile = java( + """ + package android.annotation; + public @interface EnforcePermission {} + """ + ).indented() + + private val stubs = arrayOf(manifestPermissionStub, enforcePermissionAnnotationStub) +} diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorTest.kt new file mode 100644 index 000000000000..75b00737a168 --- /dev/null +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorTest.kt @@ -0,0 +1,425 @@ +/* + * 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.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 + +@Suppress("UnstableApiUsage") +class EnforcePermissionDetectorTest : LintDetectorTest() { + override fun getDetector(): Detector = EnforcePermissionDetector() + + override fun getIssues(): List<Issue> = listOf( + EnforcePermissionDetector.ISSUE_MISSING_ENFORCE_PERMISSION, + EnforcePermissionDetector.ISSUE_MISMATCHING_ENFORCE_PERMISSION + ) + + override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) + + fun testDoesNotDetectIssuesCorrectAnnotationOnMethod() { + lint().files(java( + """ + package test.pkg; + import android.annotation.EnforcePermission; + public class TestClass2 extends IFooMethod.Stub { + @Override + @EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + public void testMethod() {} + } + """).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testDoesNotDetectIssuesCorrectAnnotationAllOnMethod() { + lint().files(java( + """ + package test.pkg; + import android.annotation.EnforcePermission; + public class TestClass11 extends IFooMethod.Stub { + @Override + @EnforcePermission(allOf={android.Manifest.permission.INTERNET, android.Manifest.permission.READ_PHONE_STATE}) + public void testMethodAll() {} + } + """).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testDoesNotDetectIssuesCorrectAnnotationAllLiteralOnMethod() { + lint().files(java( + """ + package test.pkg; + import android.annotation.EnforcePermission; + public class TestClass111 extends IFooMethod.Stub { + @Override + @EnforcePermission(allOf={"android.permission.INTERNET", android.Manifest.permission.READ_PHONE_STATE}) + public void testMethodAllLiteral() {} + } + """).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testDoesNotDetectIssuesCorrectAnnotationAnyOnMethod() { + lint().files(java( + """ + package test.pkg; + import android.annotation.EnforcePermission; + public class TestClass12 extends IFooMethod.Stub { + @Override + @EnforcePermission(anyOf={android.Manifest.permission.INTERNET, android.Manifest.permission.READ_PHONE_STATE}) + public void testMethodAny() {} + } + """).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testDoesNotDetectIssuesCorrectAnnotationAnyLiteralOnMethod() { + lint().files(java( + """ + package test.pkg; + import android.annotation.EnforcePermission; + public class TestClass121 extends IFooMethod.Stub { + @Override + @EnforcePermission(anyOf={"android.permission.INTERNET", android.Manifest.permission.READ_PHONE_STATE}) + public void testMethodAnyLiteral() {} + } + """).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testDetectIssuesMismatchingAnnotationOnMethod() { + lint().files(java( + """ + package test.pkg; + public class TestClass4 extends IFooMethod.Stub { + @android.annotation.EnforcePermission(android.Manifest.permission.INTERNET) + public void testMethod() {} + } + """).indented(), + *stubs + ) + .run() + .expect(""" + src/test/pkg/TestClass4.java:4: Error: The method TestClass4.testMethod is annotated with @android.annotation.EnforcePermission(android.Manifest.permission.INTERNET) \ + which differs from the overridden method Stub.testMethod: @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE). \ + The same annotation must be used for both methods. [MismatchingEnforcePermissionAnnotation] + public void testMethod() {} + ~~~~~~~~~~ + 1 errors, 0 warnings + """.addLineContinuation()) + } + + fun testDetectIssuesEmptyAnnotationOnMethod() { + lint().files(java( + """ + package test.pkg; + public class TestClass41 extends IFooMethod.Stub { + @android.annotation.EnforcePermission + public void testMethod() {} + } + """).indented(), + *stubs + ) + .run() + .expect(""" + src/test/pkg/TestClass41.java:4: Error: The method TestClass41.testMethod is annotated with @android.annotation.EnforcePermission \ + which differs from the overridden method Stub.testMethod: @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE). \ + The same annotation must be used for both methods. [MismatchingEnforcePermissionAnnotation] + public void testMethod() {} + ~~~~~~~~~~ + 1 errors, 0 warnings + """.addLineContinuation()) + } + + fun testDetectIssuesMismatchingAnyAnnotationOnMethod() { + lint().files(java( + """ + package test.pkg; + public class TestClass9 extends IFooMethod.Stub { + @android.annotation.EnforcePermission(anyOf={android.Manifest.permission.INTERNET, android.Manifest.permission.NFC}) + public void testMethodAny() {} + } + """).indented(), + *stubs + ) + .run() + .expect(""" + src/test/pkg/TestClass9.java:4: Error: The method TestClass9.testMethodAny is annotated with \ + @android.annotation.EnforcePermission(anyOf={android.Manifest.permission.INTERNET, android.Manifest.permission.NFC}) \ + which differs from the overridden method Stub.testMethodAny: \ + @android.annotation.EnforcePermission(anyOf={android.Manifest.permission.INTERNET, android.Manifest.permission.READ_PHONE_STATE}). \ + The same annotation must be used for both methods. [MismatchingEnforcePermissionAnnotation] + public void testMethodAny() {} + ~~~~~~~~~~~~~ + 1 errors, 0 warnings + """.addLineContinuation()) + } + + fun testDetectIssuesMismatchingAnyLiteralAnnotationOnMethod() { + lint().files(java( + """ + package test.pkg; + public class TestClass91 extends IFooMethod.Stub { + @android.annotation.EnforcePermission(anyOf={"android.permission.INTERNET", "android.permissionoopsthisisatypo.READ_PHONE_STATE"}) + public void testMethodAnyLiteral() {} + } + """).indented(), + *stubs + ) + .run() + .expect(""" + src/test/pkg/TestClass91.java:4: Error: The method TestClass91.testMethodAnyLiteral is annotated with \ + @android.annotation.EnforcePermission(anyOf={"android.permission.INTERNET", "android.permissionoopsthisisatypo.READ_PHONE_STATE"}) \ + which differs from the overridden method Stub.testMethodAnyLiteral: \ + @android.annotation.EnforcePermission(anyOf={android.Manifest.permission.INTERNET, "android.permission.READ_PHONE_STATE"}). \ + The same annotation must be used for both methods. [MismatchingEnforcePermissionAnnotation] + public void testMethodAnyLiteral() {} + ~~~~~~~~~~~~~~~~~~~~ + 1 errors, 0 warnings + """.addLineContinuation()) + } + + fun testDetectIssuesMismatchingAllAnnotationOnMethod() { + lint().files(java( + """ + package test.pkg; + public class TestClass10 extends IFooMethod.Stub { + @android.annotation.EnforcePermission(allOf={android.Manifest.permission.INTERNET, android.Manifest.permission.NFC}) + public void testMethodAll() {} + } + """).indented(), + *stubs + ) + .run() + .expect(""" + src/test/pkg/TestClass10.java:4: Error: The method TestClass10.testMethodAll is annotated with \ + @android.annotation.EnforcePermission(allOf={android.Manifest.permission.INTERNET, android.Manifest.permission.NFC}) \ + which differs from the overridden method Stub.testMethodAll: \ + @android.annotation.EnforcePermission(allOf={android.Manifest.permission.INTERNET, android.Manifest.permission.READ_PHONE_STATE}). \ + The same annotation must be used for both methods. [MismatchingEnforcePermissionAnnotation] + public void testMethodAll() {} + ~~~~~~~~~~~~~ + 1 errors, 0 warnings + """.addLineContinuation()) + } + + fun testDetectIssuesMismatchingAllLiteralAnnotationOnMethod() { + lint().files(java( + """ + package test.pkg; + public class TestClass101 extends IFooMethod.Stub { + @android.annotation.EnforcePermission(allOf={"android.permission.INTERNET", "android.permissionoopsthisisatypo.READ_PHONE_STATE"}) + public void testMethodAllLiteral() {} + } + """).indented(), + *stubs + ) + .run() + .expect(""" + src/test/pkg/TestClass101.java:4: Error: The method TestClass101.testMethodAllLiteral is annotated with \ + @android.annotation.EnforcePermission(allOf={"android.permission.INTERNET", "android.permissionoopsthisisatypo.READ_PHONE_STATE"}) \ + which differs from the overridden method Stub.testMethodAllLiteral: \ + @android.annotation.EnforcePermission(allOf={android.Manifest.permission.INTERNET, "android.permission.READ_PHONE_STATE"}). \ + The same annotation must be used for both methods. [MismatchingEnforcePermissionAnnotation] + public void testMethodAllLiteral() {} + ~~~~~~~~~~~~~~~~~~~~ + 1 errors, 0 warnings + """.addLineContinuation()) + } + + fun testDetectIssuesMissingAnnotationOnMethod() { + lint().files(java( + """ + package test.pkg; + public class TestClass6 extends IFooMethod.Stub { + public void testMethod() {} + } + """).indented(), + *stubs + ) + .run() + .expect(""" + src/test/pkg/TestClass6.java:3: Error: The method TestClass6.testMethod overrides the method Stub.testMethod which is annotated with @EnforcePermission. \ + The same annotation must be used on TestClass6.testMethod [MissingEnforcePermissionAnnotation] + public void testMethod() {} + ~~~~~~~~~~ + 1 errors, 0 warnings + """.addLineContinuation()) + } + + fun testDetectIssuesExtraAnnotationMethod() { + lint().files(java( + """ + package test.pkg; + public class TestClass7 extends IBar.Stub { + @android.annotation.EnforcePermission(android.Manifest.permission.INTERNET) + public void testMethod() {} + } + """).indented(), + *stubs + ) + .run() + .expect(""" + src/test/pkg/TestClass7.java:4: Error: The method TestClass7.testMethod overrides the method Stub.testMethod which is not annotated with @EnforcePermission. \ + The same annotation must be used on Stub.testMethod. Did you forget to annotate the AIDL definition? [MissingEnforcePermissionAnnotation] + public void testMethod() {} + ~~~~~~~~~~ + 1 errors, 0 warnings + """.addLineContinuation()) + } + + fun testDetectIssuesMissingAnnotationOnMethodWhenClassIsCalledDefault() { + lint().files(java( + """ + package test.pkg; + public class Default extends IFooMethod.Stub { + public void testMethod() {} + } + """).indented(), + *stubs + ) + .run() + .expect( + """ + src/test/pkg/Default.java:3: Error: The method Default.testMethod \ + overrides the method Stub.testMethod which is annotated with @EnforcePermission. The same annotation must be used on Default.testMethod [MissingEnforcePermissionAnnotation] + public void testMethod() {} + ~~~~~~~~~~ + 1 errors, 0 warnings + """.addLineContinuation() + ) + } + + fun testDoesDetectIssuesShortStringsNotAllowed() { + lint().files(java( + """ + package test.pkg; + import android.annotation.EnforcePermission; + public class TestClass121 extends IFooMethod.Stub { + @Override + @EnforcePermission(anyOf={"INTERNET", "READ_PHONE_STATE"}) + public void testMethodAnyLiteral() {} + } + """).indented(), + *stubs + ) + .run() + .expect( + """ + src/test/pkg/TestClass121.java:6: Error: The method \ + TestClass121.testMethodAnyLiteral is annotated with @EnforcePermission(anyOf={"INTERNET", "READ_PHONE_STATE"}) \ + which differs from the overridden method Stub.testMethodAnyLiteral: \ + @android.annotation.EnforcePermission(anyOf={android.Manifest.permission.INTERNET, "android.permission.READ_PHONE_STATE"}). \ + The same annotation must be used for both methods. [MismatchingEnforcePermissionAnnotation] + public void testMethodAnyLiteral() {} + ~~~~~~~~~~~~~~~~~~~~ + 1 errors, 0 warnings + """.addLineContinuation() + ) + } + + /* Stubs */ + + // A service with permission annotation on the method. + private val interfaceIFooMethodStub: TestFile = java( + """ + public interface IFooMethod extends android.os.IInterface { + public static abstract class Stub extends android.os.Binder implements IFooMethod { + @Override + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + public void testMethod() {} + @Override + @android.annotation.EnforcePermission(anyOf={android.Manifest.permission.INTERNET, android.Manifest.permission.READ_PHONE_STATE}) + public void testMethodAny() {} + @Override + @android.annotation.EnforcePermission(anyOf={android.Manifest.permission.INTERNET, "android.permission.READ_PHONE_STATE"}) + public void testMethodAnyLiteral() {} + @Override + @android.annotation.EnforcePermission(allOf={android.Manifest.permission.INTERNET, android.Manifest.permission.READ_PHONE_STATE}) + public void testMethodAll() {} + @Override + @android.annotation.EnforcePermission(allOf={android.Manifest.permission.INTERNET, "android.permission.READ_PHONE_STATE"}) + public void testMethodAllLiteral() {} + } + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + public void testMethod(); + @android.annotation.EnforcePermission(anyOf={android.Manifest.permission.INTERNET, android.Manifest.permission.READ_PHONE_STATE}) + public void testMethodAny() {} + @android.annotation.EnforcePermission(anyOf={android.Manifest.permission.INTERNET, "android.permission.READ_PHONE_STATE"}) + public void testMethodAnyLiteral() {} + @android.annotation.EnforcePermission(allOf={android.Manifest.permission.INTERNET, android.Manifest.permission.READ_PHONE_STATE}) + public void testMethodAll() {} + @android.annotation.EnforcePermission(allOf={android.Manifest.permission.INTERNET, "android.permission.READ_PHONE_STATE"}) + public void testMethodAllLiteral() {} + } + """ + ).indented() + + // A service without any permission annotation. + private val interfaceIBarStub: TestFile = java( + """ + public interface IBar extends android.os.IInterface { + public static abstract class Stub extends android.os.Binder implements IBar { + @Override + public void testMethod() {} + } + public void testMethod(); + } + """ + ).indented() + + private val manifestPermissionStub: TestFile = java( + """ + package android.Manifest; + class permission { + public static final String READ_PHONE_STATE = "android.permission.READ_PHONE_STATE"; + public static final String NFC = "android.permission.NFC"; + public static final String INTERNET = "android.permission.INTERNET"; + } + """ + ).indented() + + private val enforcePermissionAnnotationStub: TestFile = java( + """ + package android.annotation; + public @interface EnforcePermission {} + """ + ).indented() + + private val stubs = arrayOf(interfaceIFooMethodStub, interfaceIBarStub, + manifestPermissionStub, enforcePermissionAnnotationStub) + + // Substitutes "backslash + new line" with an empty string to imitate line continuation + private fun String.addLineContinuation(): String = this.trimIndent().replace("\\\n", "") +} 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 new file mode 100644 index 000000000000..5a63bb4084d2 --- /dev/null +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorCodegenTest.kt @@ -0,0 +1,557 @@ +/* + * 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.checks.infrastructure.LintDetectorTest +import com.android.tools.lint.checks.infrastructure.TestFile +import com.android.tools.lint.checks.infrastructure.TestLintTask +import com.android.tools.lint.checks.infrastructure.TestMode +import com.android.tools.lint.detector.api.Detector +import com.android.tools.lint.detector.api.Issue + +@Suppress("UnstableApiUsage") +class EnforcePermissionHelperDetectorCodegenTest : LintDetectorTest() { + override fun getDetector(): Detector = EnforcePermissionHelperDetector() + + override fun getIssues(): List<Issue> = listOf( + EnforcePermissionHelperDetector.ISSUE_ENFORCE_PERMISSION_HELPER + ) + + override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) + + fun test_generated_IProtected() { + lint().testModes(TestMode.DEFAULT).files( + java( + """ + /* + * This file is auto-generated. DO NOT MODIFY. + */ + package android.aidl.tests.permission; + public interface IProtected extends android.os.IInterface + { + /** Default implementation for IProtected. */ + public static class Default implements android.aidl.tests.permission.IProtected + { + @Override public void PermissionProtected() throws android.os.RemoteException + { + } + @Override public void MultiplePermissionsAll() throws android.os.RemoteException + { + } + @Override public void MultiplePermissionsAny() throws android.os.RemoteException + { + } + @Override public void NonManifestPermission() throws android.os.RemoteException + { + } + // Used by the integration tests to dynamically set permissions that are considered granted. + @Override public void SetGranted(java.util.List<java.lang.String> permissions) throws android.os.RemoteException + { + } + @Override + public android.os.IBinder asBinder() { + return null; + } + } + /** Local-side IPC implementation stub class. */ + public static abstract class Stub extends android.os.Binder implements android.aidl.tests.permission.IProtected + { + private final android.os.PermissionEnforcer mEnforcer; + /** Construct the stub using the Enforcer provided. */ + public Stub(android.os.PermissionEnforcer enforcer) + { + this.attachInterface(this, DESCRIPTOR); + if (enforcer == null) { + throw new IllegalArgumentException("enforcer cannot be null"); + } + mEnforcer = enforcer; + } + @Deprecated + /** Default constructor. */ + public Stub() { + this(android.os.PermissionEnforcer.fromContext( + android.app.ActivityThread.currentActivityThread().getSystemContext())); + } + /** + * Cast an IBinder object into an android.aidl.tests.permission.IProtected interface, + * generating a proxy if needed. + */ + public static android.aidl.tests.permission.IProtected asInterface(android.os.IBinder obj) + { + if ((obj==null)) { + return null; + } + android.os.IInterface iin = obj.queryLocalInterface(DESCRIPTOR); + if (((iin!=null)&&(iin instanceof android.aidl.tests.permission.IProtected))) { + return ((android.aidl.tests.permission.IProtected)iin); + } + return new android.aidl.tests.permission.IProtected.Stub.Proxy(obj); + } + @Override public android.os.IBinder asBinder() + { + return this; + } + /** @hide */ + public static java.lang.String getDefaultTransactionName(int transactionCode) + { + switch (transactionCode) + { + case TRANSACTION_PermissionProtected: + { + return "PermissionProtected"; + } + case TRANSACTION_MultiplePermissionsAll: + { + return "MultiplePermissionsAll"; + } + case TRANSACTION_MultiplePermissionsAny: + { + return "MultiplePermissionsAny"; + } + case TRANSACTION_NonManifestPermission: + { + return "NonManifestPermission"; + } + case TRANSACTION_SetGranted: + { + return "SetGranted"; + } + default: + { + return null; + } + } + } + /** @hide */ + public java.lang.String getTransactionName(int transactionCode) + { + return this.getDefaultTransactionName(transactionCode); + } + @Override public boolean onTransact(int code, android.os.Parcel data, android.os.Parcel reply, int flags) throws android.os.RemoteException + { + java.lang.String descriptor = DESCRIPTOR; + if (code >= android.os.IBinder.FIRST_CALL_TRANSACTION && code <= android.os.IBinder.LAST_CALL_TRANSACTION) { + data.enforceInterface(descriptor); + } + switch (code) + { + case INTERFACE_TRANSACTION: + { + reply.writeString(descriptor); + return true; + } + } + switch (code) + { + case TRANSACTION_PermissionProtected: + { + this.PermissionProtected(); + reply.writeNoException(); + break; + } + case TRANSACTION_MultiplePermissionsAll: + { + this.MultiplePermissionsAll(); + reply.writeNoException(); + break; + } + case TRANSACTION_MultiplePermissionsAny: + { + this.MultiplePermissionsAny(); + reply.writeNoException(); + break; + } + case TRANSACTION_NonManifestPermission: + { + this.NonManifestPermission(); + reply.writeNoException(); + break; + } + case TRANSACTION_SetGranted: + { + java.util.List<java.lang.String> _arg0; + _arg0 = data.createStringArrayList(); + data.enforceNoDataAvail(); + this.SetGranted(_arg0); + reply.writeNoException(); + break; + } + default: + { + return super.onTransact(code, data, reply, flags); + } + } + return true; + } + private static class Proxy implements android.aidl.tests.permission.IProtected + { + private android.os.IBinder mRemote; + Proxy(android.os.IBinder remote) + { + mRemote = remote; + } + @Override public android.os.IBinder asBinder() + { + return mRemote; + } + public java.lang.String getInterfaceDescriptor() + { + return DESCRIPTOR; + } + @Override public void PermissionProtected() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_PermissionProtected, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + @Override public void MultiplePermissionsAll() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_MultiplePermissionsAll, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + @Override public void MultiplePermissionsAny() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_MultiplePermissionsAny, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + @Override public void NonManifestPermission() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_NonManifestPermission, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + // Used by the integration tests to dynamically set permissions that are considered granted. + @Override public void SetGranted(java.util.List<java.lang.String> permissions) throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + _data.writeStringList(permissions); + boolean _status = mRemote.transact(Stub.TRANSACTION_SetGranted, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + } + static final int TRANSACTION_PermissionProtected = (android.os.IBinder.FIRST_CALL_TRANSACTION + 0); + /** Helper method to enforce permissions for PermissionProtected */ + protected void PermissionProtected_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermission(android.Manifest.permission.READ_PHONE_STATE, source); + } + static final int TRANSACTION_MultiplePermissionsAll = (android.os.IBinder.FIRST_CALL_TRANSACTION + 1); + /** Helper method to enforce permissions for MultiplePermissionsAll */ + protected void MultiplePermissionsAll_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermissionAllOf(new String[]{android.Manifest.permission.INTERNET, android.Manifest.permission.VIBRATE}, source); + } + static final int TRANSACTION_MultiplePermissionsAny = (android.os.IBinder.FIRST_CALL_TRANSACTION + 2); + /** Helper method to enforce permissions for MultiplePermissionsAny */ + protected void MultiplePermissionsAny_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermissionAnyOf(new String[]{android.Manifest.permission.INTERNET, android.Manifest.permission.VIBRATE}, source); + } + static final int TRANSACTION_NonManifestPermission = (android.os.IBinder.FIRST_CALL_TRANSACTION + 3); + /** Helper method to enforce permissions for NonManifestPermission */ + protected void NonManifestPermission_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermission(android.net.NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK, source); + } + static final int TRANSACTION_SetGranted = (android.os.IBinder.FIRST_CALL_TRANSACTION + 4); + /** @hide */ + public int getMaxTransactionId() + { + return 4; + } + } + + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + public void PermissionProtected() throws android.os.RemoteException; + @android.annotation.EnforcePermission(allOf = {android.Manifest.permission.INTERNET, android.Manifest.permission.VIBRATE}) + public void MultiplePermissionsAll() throws android.os.RemoteException; + @android.annotation.EnforcePermission(anyOf = {android.Manifest.permission.INTERNET, android.Manifest.permission.VIBRATE}) + public void MultiplePermissionsAny() throws android.os.RemoteException; + @android.annotation.EnforcePermission(android.net.NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK) + public void NonManifestPermission() throws android.os.RemoteException; + // Used by the integration tests to dynamically set permissions that are considered granted. + @android.annotation.RequiresNoPermission + public void SetGranted(java.util.List<java.lang.String> permissions) throws android.os.RemoteException; + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun test_generated_IProtectedInterface() { + lint().files( + java( + """ + /* + * This file is auto-generated. DO NOT MODIFY. + */ + package android.aidl.tests.permission; + public interface IProtectedInterface extends android.os.IInterface + { + /** Default implementation for IProtectedInterface. */ + public static class Default implements android.aidl.tests.permission.IProtectedInterface + { + @Override public void Method1() throws android.os.RemoteException + { + } + @Override public void Method2() throws android.os.RemoteException + { + } + @Override + public android.os.IBinder asBinder() { + return null; + } + } + /** Local-side IPC implementation stub class. */ + public static abstract class Stub extends android.os.Binder implements android.aidl.tests.permission.IProtectedInterface + { + private final android.os.PermissionEnforcer mEnforcer; + /** Construct the stub using the Enforcer provided. */ + public Stub(android.os.PermissionEnforcer enforcer) + { + this.attachInterface(this, DESCRIPTOR); + if (enforcer == null) { + throw new IllegalArgumentException("enforcer cannot be null"); + } + mEnforcer = enforcer; + } + @Deprecated + /** Default constructor. */ + public Stub() { + this(android.os.PermissionEnforcer.fromContext( + android.app.ActivityThread.currentActivityThread().getSystemContext())); + } + /** + * Cast an IBinder object into an android.aidl.tests.permission.IProtectedInterface interface, + * generating a proxy if needed. + */ + public static android.aidl.tests.permission.IProtectedInterface asInterface(android.os.IBinder obj) + { + if ((obj==null)) { + return null; + } + android.os.IInterface iin = obj.queryLocalInterface(DESCRIPTOR); + if (((iin!=null)&&(iin instanceof android.aidl.tests.permission.IProtectedInterface))) { + return ((android.aidl.tests.permission.IProtectedInterface)iin); + } + return new android.aidl.tests.permission.IProtectedInterface.Stub.Proxy(obj); + } + @Override public android.os.IBinder asBinder() + { + return this; + } + /** @hide */ + public static java.lang.String getDefaultTransactionName(int transactionCode) + { + switch (transactionCode) + { + case TRANSACTION_Method1: + { + return "Method1"; + } + case TRANSACTION_Method2: + { + return "Method2"; + } + default: + { + return null; + } + } + } + /** @hide */ + public java.lang.String getTransactionName(int transactionCode) + { + return this.getDefaultTransactionName(transactionCode); + } + @Override public boolean onTransact(int code, android.os.Parcel data, android.os.Parcel reply, int flags) throws android.os.RemoteException + { + java.lang.String descriptor = DESCRIPTOR; + if (code >= android.os.IBinder.FIRST_CALL_TRANSACTION && code <= android.os.IBinder.LAST_CALL_TRANSACTION) { + data.enforceInterface(descriptor); + } + switch (code) + { + case INTERFACE_TRANSACTION: + { + reply.writeString(descriptor); + return true; + } + } + switch (code) + { + case TRANSACTION_Method1: + { + this.Method1(); + reply.writeNoException(); + break; + } + case TRANSACTION_Method2: + { + this.Method2(); + reply.writeNoException(); + break; + } + default: + { + return super.onTransact(code, data, reply, flags); + } + } + return true; + } + private static class Proxy implements android.aidl.tests.permission.IProtectedInterface + { + private android.os.IBinder mRemote; + Proxy(android.os.IBinder remote) + { + mRemote = remote; + } + @Override public android.os.IBinder asBinder() + { + return mRemote; + } + public java.lang.String getInterfaceDescriptor() + { + return DESCRIPTOR; + } + @Override public void Method1() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_Method1, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + @Override public void Method2() throws android.os.RemoteException + { + android.os.Parcel _data = android.os.Parcel.obtain(asBinder()); + android.os.Parcel _reply = android.os.Parcel.obtain(); + try { + _data.writeInterfaceToken(DESCRIPTOR); + boolean _status = mRemote.transact(Stub.TRANSACTION_Method2, _data, _reply, 0); + _reply.readException(); + } + finally { + _reply.recycle(); + _data.recycle(); + } + } + } + static final int TRANSACTION_Method1 = (android.os.IBinder.FIRST_CALL_TRANSACTION + 0); + /** Helper method to enforce permissions for Method1 */ + protected void Method1_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermission(android.Manifest.permission.ACCESS_FINE_LOCATION, source); + } + static final int TRANSACTION_Method2 = (android.os.IBinder.FIRST_CALL_TRANSACTION + 1); + /** Helper method to enforce permissions for Method2 */ + protected void Method2_enforcePermission() throws SecurityException { + android.content.AttributionSource source = new android.content.AttributionSource(getCallingUid(), null, null); + mEnforcer.enforcePermission(android.Manifest.permission.ACCESS_FINE_LOCATION, source); + } + /** @hide */ + public int getMaxTransactionId() + { + return 1; + } + } + + @android.annotation.EnforcePermission(android.Manifest.permission.ACCESS_FINE_LOCATION) + public void Method1() throws android.os.RemoteException; + @android.annotation.EnforcePermission(android.Manifest.permission.ACCESS_FINE_LOCATION) + public void Method2() throws android.os.RemoteException; + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + /* Stubs */ + + private val manifestPermissionStub: TestFile = java( + """ + package android.Manifest; + class permission { + public static final String READ_PHONE_STATE = "android.permission.READ_PHONE_STATE"; + public static final String INTERNET = "android.permission.INTERNET"; + } + """ + ).indented() + + private val enforcePermissionAnnotationStub: TestFile = java( + """ + package android.annotation; + public @interface EnforcePermission {} + """ + ).indented() + + private val stubs = arrayOf(manifestPermissionStub, enforcePermissionAnnotationStub) +} 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 new file mode 100644 index 000000000000..10a6e1da91dc --- /dev/null +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt @@ -0,0 +1,443 @@ +/* +* 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.checks.infrastructure.LintDetectorTest +import com.android.tools.lint.checks.infrastructure.TestLintTask + +class EnforcePermissionHelperDetectorTest : LintDetectorTest() { + override fun getDetector() = EnforcePermissionHelperDetector() + override fun getIssues() = listOf( + EnforcePermissionHelperDetector.ISSUE_ENFORCE_PERMISSION_HELPER, + EnforcePermissionHelperDetector.ISSUE_MISUSING_ENFORCE_PERMISSION + ) + + override fun lint(): TestLintTask = super.lint().allowMissingSdk() + + fun testFirstExpressionIsFunctionCall() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + Binder.getCallingUid(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper] + @Override + ^ + 1 errors, 0 warnings + """ + ) + .expectFixDiffs( + """ + Autofix for src/Foo.java line 5: Replace with test_enforcePermission();...: + @@ -8 +8 + + test_enforcePermission(); + + + """ + ) + } + + fun testFirstExpressionIsVariableDeclaration() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + String foo = "bar"; + Binder.getCallingUid(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper] + @Override + ^ + 1 errors, 0 warnings + """ + ) + .expectFixDiffs( + """ + Autofix for src/Foo.java line 5: Replace with test_enforcePermission();...: + @@ -8 +8 + + test_enforcePermission(); + + + """ + ) + } + + fun testMethodIsEmpty() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException {} + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper] + @Override + ^ + 1 errors, 0 warnings + """ + ) + } + + fun testOkay() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testHelperWithoutSuperPrefix_Okay() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + test_enforcePermission(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testInterfaceDefaultMethod_notStubAncestor_error() { + lint().files( + java( + """ + public interface IProtected extends android.os.IInterface { + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + default void PermissionProtected() throws android.os.RemoteException { + String foo = "bar"; + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/IProtected.java:2: Error: The class of PermissionProtected does not inherit from an AIDL generated Stub class [MisusingEnforcePermissionAnnotation] + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + ^ + 1 errors, 0 warnings + """ + ) + } + + fun testInheritance_callSuper_okay() { + lint().files( + java( + """ + package test; + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Foo; + public class Bar extends Foo { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Bar; + public class Baz extends Bar { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testInheritance_callHelper_okay() { + lint().files( + java( + """ + package test; + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Foo; + public class Bar extends Foo { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Bar; + public class Baz extends Bar { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testInheritance_missingCallInChain_error() { + lint().files( + java( + """ + package test; + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Foo; + public class Bar extends Foo { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + doStuff(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Bar; + public class Baz extends Bar { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/test/Bar.java:4: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper] + @Override + ^ + 1 errors, 0 warnings + """ + ) + } + + fun testInheritance_missingCall_error() { + lint().files( + java( + """ + package test; + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Foo; + public class Bar extends Foo { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Bar; + public class Baz extends Bar { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + doStuff(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/test/Baz.java:4: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper] + @Override + ^ + 1 errors, 0 warnings + """ + ) + } + + fun testRandomClass_notStubAncestor_error() { + lint().files( + java( + """ + public class Foo { + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + void PermissionProtected() throws android.os.RemoteException { + String foo = "bar"; + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:2: Error: The class of PermissionProtected does not inherit from an AIDL generated Stub class [MisusingEnforcePermissionAnnotation] + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + ^ + 1 errors, 0 warnings + """ + ) + } + + companion object { + val stubs = arrayOf(aidlStub, contextStub, binderStub) + } +} + + + 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 new file mode 100644 index 000000000000..6b8e72cf9222 --- /dev/null +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt @@ -0,0 +1,843 @@ +/* + * 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.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 + +@Suppress("UnstableApiUsage") +class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { + override fun getDetector(): Detector = SimpleManualPermissionEnforcementDetector() + override fun getIssues(): List<Issue> = listOf( + SimpleManualPermissionEnforcementDetector + .ISSUE_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT + ) + + override fun lint(): TestLintTask = super.lint().allowMissingSdk() + + fun testClass() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + public void test() throws android.os.RemoteException { + mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:7: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 7: Annotate with @EnforcePermission: + @@ -5 +5 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -7 +8 + - mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + + test_enforcePermission(); + """ + ) + } + + fun testClass_orSelfFalse_warning() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + public void test() throws android.os.RemoteException { + mContext.enforceCallingPermission("android.permission.READ_CONTACTS", "foo"); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.enforceCallingPermission("android.permission.READ_CONTACTS", "foo"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 7: Annotate with @EnforcePermission: + @@ -5 +5 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -7 +8 + - mContext.enforceCallingPermission("android.permission.READ_CONTACTS", "foo"); + + test_enforcePermission(); + """ + ) + } + + fun testClass_enforcesFalse_warning() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + public void test() throws android.os.RemoteException { + mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 7: Annotate with @EnforcePermission: + @@ -5 +5 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -7 +8 + - mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + + test_enforcePermission(); + """ + ) + } + + fun testAnonClass() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo { + private Context mContext; + private ITest itest = new ITest.Stub() { + @Override + public void test() throws android.os.RemoteException { + mContext.enforceCallingOrSelfPermission( + "android.permission.READ_CONTACTS", "foo"); + } + }; + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:8: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.enforceCallingOrSelfPermission( + ^ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 8: Annotate with @EnforcePermission: + @@ -6 +6 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -8 +9 + - mContext.enforceCallingOrSelfPermission( + - "android.permission.READ_CONTACTS", "foo"); + + test_enforcePermission(); + """ + ) + } + + fun testConstantEvaluation() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + @Override + public void test() throws android.os.RemoteException { + mContext.enforceCallingOrSelfPermission(android.Manifest.permission.READ_CONTACTS, "foo"); + } + } + """ + ).indented(), + *stubs, + manifestStub + ) + .run() + .expect( + """ + src/Foo.java:8: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.enforceCallingOrSelfPermission(android.Manifest.permission.READ_CONTACTS, "foo"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 8: Annotate with @EnforcePermission: + @@ -6 +6 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -8 +9 + - mContext.enforceCallingOrSelfPermission(android.Manifest.permission.READ_CONTACTS, "foo"); + + test_enforcePermission(); + """ + ) + } + + fun testAllOf() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo { + private Context mContext; + private ITest itest = new ITest.Stub() { + @Override + public void test() throws android.os.RemoteException { + mContext.enforceCallingOrSelfPermission( + "android.permission.READ_CONTACTS", "foo"); + mContext.enforceCallingOrSelfPermission( + "android.permission.WRITE_CONTACTS", "foo"); + } + }; + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:10: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.enforceCallingOrSelfPermission( + ^ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 10: Annotate with @EnforcePermission: + @@ -6 +6 + + @android.annotation.EnforcePermission(allOf={"android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS"}) + @@ -8 +9 + - mContext.enforceCallingOrSelfPermission( + - "android.permission.READ_CONTACTS", "foo"); + - mContext.enforceCallingOrSelfPermission( + - "android.permission.WRITE_CONTACTS", "foo"); + + test_enforcePermission(); + """ + ) + } + + fun testAllOf_mixedOrSelf_warning() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo { + private Context mContext; + private ITest itest = new ITest.Stub() { + @Override + public void test() throws android.os.RemoteException { + mContext.enforceCallingOrSelfPermission( + "android.permission.READ_CONTACTS", "foo"); + mContext.enforceCallingPermission( + "android.permission.WRITE_CONTACTS", "foo"); + } + }; + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.enforceCallingPermission( + ^ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 10: Annotate with @EnforcePermission: + @@ -6 +6 + + @android.annotation.EnforcePermission(allOf={"android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS"}) + @@ -8 +9 + - mContext.enforceCallingOrSelfPermission( + - "android.permission.READ_CONTACTS", "foo"); + - mContext.enforceCallingPermission( + - "android.permission.WRITE_CONTACTS", "foo"); + + test_enforcePermission(); + """ + ) + } + + fun testAllOf_mixedEnforces_warning() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo { + private Context mContext; + private ITest itest = new ITest.Stub() { + @Override + public void test() throws android.os.RemoteException { + mContext.enforceCallingOrSelfPermission( + "android.permission.READ_CONTACTS", "foo"); + mContext.checkCallingOrSelfPermission( + "android.permission.WRITE_CONTACTS", "foo"); + } + }; + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.checkCallingOrSelfPermission( + ^ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 10: Annotate with @EnforcePermission: + @@ -6 +6 + + @android.annotation.EnforcePermission(allOf={"android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS"}) + @@ -8 +9 + - mContext.enforceCallingOrSelfPermission( + - "android.permission.READ_CONTACTS", "foo"); + - mContext.checkCallingOrSelfPermission( + - "android.permission.WRITE_CONTACTS", "foo"); + + test_enforcePermission(); + """ + ) + } + + fun testPrecedingExpressions() { + lint().files( + java( + """ + import android.os.Binder; + import android.test.ITest; + public class Foo extends ITest.Stub { + private mContext Context; + @Override + public void test() throws android.os.RemoteException { + long uid = Binder.getCallingUid(); + mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testPermissionHelper() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.annotation.PermissionMethod(orSelf = true) + private void helper() { + mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + } + + @Override + public void test() throws android.os.RemoteException { + helper(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:14: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + helper(); + ~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 14: Annotate with @EnforcePermission: + @@ -12 +12 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -14 +15 + - helper(); + + test_enforcePermission(); + """ + ) + } + + fun testPermissionHelper_orSelfNotBubbledUp_warning() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.annotation.PermissionMethod + private void helper() { + mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + } + + @Override + public void test() throws android.os.RemoteException { + helper(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:14: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + helper(); + ~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 14: Annotate with @EnforcePermission: + @@ -12 +12 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -14 +15 + - helper(); + + test_enforcePermission(); + """ + ) + } + + fun testPermissionHelperAllOf() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.annotation.PermissionMethod(orSelf = true) + private void helper() { + mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + mContext.enforceCallingOrSelfPermission("android.permission.WRITE_CONTACTS", "foo"); + } + + @Override + public void test() throws android.os.RemoteException { + helper(); + mContext.enforceCallingOrSelfPermission("FOO", "foo"); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:16: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.enforceCallingOrSelfPermission("FOO", "foo"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 16: Annotate with @EnforcePermission: + @@ -13 +13 + + @android.annotation.EnforcePermission(allOf={"android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS", "FOO"}) + @@ -15 +16 + - helper(); + - mContext.enforceCallingOrSelfPermission("FOO", "foo"); + + test_enforcePermission(); + """ + ) + } + + + fun testPermissionHelperNested() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.annotation.PermissionMethod(orSelf = true) + private void helperHelper() { + helper("android.permission.WRITE_CONTACTS"); + } + + @android.annotation.PermissionMethod(orSelf = true) + private void helper(@android.annotation.PermissionName String extraPermission) { + mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + } + + @Override + public void test() throws android.os.RemoteException { + helperHelper(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:19: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + helperHelper(); + ~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 19: Annotate with @EnforcePermission: + @@ -17 +17 + + @android.annotation.EnforcePermission(allOf={"android.permission.WRITE_CONTACTS", "android.permission.READ_CONTACTS"}) + @@ -19 +20 + - helperHelper(); + + test_enforcePermission(); + """ + ) + } + + fun testIfExpression() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + public void test() throws android.os.RemoteException { + if (mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo") + != PackageManager.PERMISSION_GRANTED) { + throw new SecurityException("yikes!"); + } + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:7: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + if (mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo") + ^ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 7: Annotate with @EnforcePermission: + @@ -5 +5 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -7 +8 + - if (mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo") + - != PackageManager.PERMISSION_GRANTED) { + - throw new SecurityException("yikes!"); + - } + + test_enforcePermission(); + """ + ) + } + + fun testIfExpression_orSelfFalse_warning() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + public void test() throws android.os.RemoteException { + if (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") + != PackageManager.PERMISSION_GRANTED) { + throw new SecurityException("yikes!"); + } + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + if (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") + ^ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 7: Annotate with @EnforcePermission: + @@ -5 +5 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -7 +8 + - if (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") + - != PackageManager.PERMISSION_GRANTED) { + - throw new SecurityException("yikes!"); + - } + + test_enforcePermission(); + """ + ) + } + + fun testIfExpression_otherSideEffect_ignored() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + public void test() throws android.os.RemoteException { + if (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") + != PackageManager.PERMISSION_GRANTED) { + doSomethingElse(); + throw new SecurityException("yikes!"); + } + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testIfExpression_inlinedWithSideEffect_ignored() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + public void test() throws android.os.RemoteException { + if (somethingElse() && mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") + != PackageManager.PERMISSION_GRANTED) { + throw new SecurityException("yikes!"); + } + } + + private boolean somethingElse() { + return true; + } + } + """ + ).indented(), + *stubs + ) + .run() + .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.annotation.PermissionMethod(anyOf = true) + private void helperHelper() { + helper("FOO", "BAR"); + } + + @android.annotation.PermissionMethod(anyOf = true, value = {"BAZ", "BUZZ"}) + private void helper(@android.annotation.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(); + + test_enforcePermission(); + """ + ) + } + + + 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.annotation.PermissionMethod + private void allOfhelper() { + mContext.enforceCallingOrSelfPermission("FOO"); + mContext.enforceCallingOrSelfPermission("BAR"); + } + + @android.annotation.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.annotation.PermissionName; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.annotation.PermissionMethod(anyOf = true) + private void anyOfCheck(@PermissionName String... permissions) { + allOfCheck("BAZ", "BUZZ"); + } + + @android.annotation.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, + contextStub, + binderStub, + permissionMethodStub, + permissionNameStub + ) + } +} 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 new file mode 100644 index 000000000000..2ec8fddbb4e9 --- /dev/null +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt @@ -0,0 +1,88 @@ +package com.google.android.lint.aidl + +import com.android.tools.lint.checks.infrastructure.LintDetectorTest.java +import com.android.tools.lint.checks.infrastructure.TestFile + +val aidlStub: TestFile = java( + """ + package android.test; + public interface ITest extends android.os.IInterface { + public static abstract class Stub extends android.os.Binder implements android.test.ITest { + protected void test_enforcePermission() throws SecurityException {} + } + public void test() throws android.os.RemoteException; + } + """ +).indented() + +val contextStub: TestFile = java( + """ + package android.content; + public class Context { + @android.annotation.PermissionMethod(orSelf = true) + public void enforceCallingOrSelfPermission(@android.annotation.PermissionName String permission, String message) {} + @android.annotation.PermissionMethod + public void enforceCallingPermission(@android.annotation.PermissionName String permission, String message) {} + @android.annotation.PermissionMethod(orSelf = true) + public int checkCallingOrSelfPermission(@android.annotation.PermissionName String permission, String message) {} + @android.annotation.PermissionMethod + public int checkCallingPermission(@android.annotation.PermissionName String permission, String message) {} + } + """ +).indented() + +val binderStub: TestFile = java( + """ + package android.os; + public class Binder { + public static int getCallingUid() {} + } + """ +).indented() + +val permissionMethodStub: TestFile = java( + """ + package android.annotation; + + import static java.lang.annotation.ElementType.METHOD; + import static java.lang.annotation.RetentionPolicy.CLASS; + + import java.lang.annotation.Retention; + import java.lang.annotation.Target; + + @Retention(CLASS) + @Target({METHOD}) + public @interface PermissionMethod {} + """ +).indented() + +val permissionNameStub: TestFile = java( + """ + package android.annotation; + + import static java.lang.annotation.ElementType.FIELD; + import static java.lang.annotation.ElementType.LOCAL_VARIABLE; + import static java.lang.annotation.ElementType.METHOD; + import static java.lang.annotation.ElementType.PARAMETER; + import static java.lang.annotation.RetentionPolicy.CLASS; + + import java.lang.annotation.Retention; + import java.lang.annotation.Target; + + @Retention(CLASS) + @Target({PARAMETER, METHOD, LOCAL_VARIABLE, FIELD}) + public @interface PermissionName {} + """ +).indented() + +val manifestStub: TestFile = java( + """ + package android; + + public final class Manifest { + public static final class permission { + public static final String READ_CONTACTS="android.permission.READ_CONTACTS"; + } + } + """.trimIndent() +)
\ No newline at end of file |