Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge "EnforcePermissionHelperDetector: @EP can only be used in Stub ancestors"

Browse Source
This commit is contained in:
TreeHugger Robot
2023-01-07 00:37:50 +00:00
committed by Android (Google) Code Review
parent d53be76a55 5dd60db56e
commit 16ec882aa8
7 changed files with 140 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/AidlImplementationDetector.kt
View File

@@ -36,7 +36,7 @@ abstract class AidlImplementationDetector : Detector(), SourceCodeScanner {
private inner class AidlStubHandler(val context: JavaContext) : UElementHandler() {
override fun visitMethod(node: UMethod) {
val interfaceName = getContainingAidlInterface(node)
val interfaceName = getContainingAidlInterface(context, node)
.takeUnless(EXCLUDED_CPP_INTERFACES::contains) ?: return
val body = (node.uastBody as? UBlockExpression) ?: return
visitAidlMethod(context, node, interfaceName, body)

1
tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt
View File

@@ -26,6 +26,7 @@ val AIDL_PERMISSION_ANNOTATIONS = listOf(
ANNOTATION_PERMISSION_MANUALLY_ENFORCED
)
const val BINDER_CLASS = "android.os.Binder"
const val IINTERFACE_INTERFACE = "android.os.IInterface"
/**

67
tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt
View File

@@ -38,6 +38,7 @@ 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
@@ -55,9 +56,6 @@ import org.jetbrains.uast.UMethod
*/
class EnforcePermissionDetector : Detector(), SourceCodeScanner {
val BINDER_CLASS = "android.os.Binder"
val JAVA_OBJECT = "java.lang.Object"
override fun applicableAnnotations(): List<String> {
return listOf(ANNOTATION_ENFORCE_PERMISSION)
}
@@ -123,6 +121,11 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner {
overriddenMethod: PsiMethod,
checkEquivalence: Boolean = true
) {
// If method is not from a Stub subclass, this method shouldn't use @EP at all.
// This is handled by EnforcePermissionHelperDetector.
if (!isContainedInSubclassOfStub(context, overridingMethod.toUElement() as? UMethod)) {
return
}
val overridingAnnotation = overridingMethod.getAnnotation(ANNOTATION_ENFORCE_PERMISSION)
val overriddenAnnotation = overriddenMethod.getAnnotation(ANNOTATION_ENFORCE_PERMISSION)
val location = context.getLocation(element)
@@ -131,13 +134,6 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner {
val overridingName = "${overridingClass.name}.${overridingMethod.name}"
val overriddenName = "${overriddenClass.name}.${overriddenMethod.name}"
if (overridingAnnotation == null) {
if (shouldIgnoreGeneratedMethod(
context,
overriddenClass = overriddenClass,
overridingClass = overridingClass)
) {
return
}
val msg = "The method $overridingName overrides the method $overriddenName which " +
"is annotated with @EnforcePermission. The same annotation must be used " +
"on $overridingName"
@@ -177,54 +173,19 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner {
if (node.qualifiedName != ANNOTATION_ENFORCE_PERMISSION) {
return
}
val method = node.uastParent as? UMethod
if (method != null) {
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)
}
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)
}
}
}
}
/**
* since this lint runs globally, it will also run against generated
* test code e.g.
* system/tools/aidl/tests/golden_output/aidl-test-interface-permission-java-source/gen/android/aidl/tests/permission/IProtected.java
* system/tools/aidl/tests/golden_output/aidl-test-interface-permission-java-source/gen/android/aidl/tests/permission/IProtectedInterface.java
* we do not want to report errors against generated `Stub` and `Proxy` classes in those files
*/
private fun shouldIgnoreGeneratedMethod(
context: JavaContext,
overriddenClass: PsiClass,
overridingClass: PsiClass,
): Boolean {
if (isInterfaceAndExtendsIInterface(overriddenClass) &&
context.evaluator.isStatic(overridingClass)) {
if (overridingClass.name == "Default") return true
if (overridingClass.name == "Proxy") {
val shouldBeStub = overridingClass.parent as? PsiClass ?: return false
return shouldBeStub.name == "Stub" &&
context.evaluator.isAbstract(shouldBeStub) &&
context.evaluator.isStatic(shouldBeStub) &&
shouldBeStub.extendsList?.referenceElements
?.any { it.qualifiedName == BINDER_CLASS } == true
}
}
return false
}
private fun isInterfaceAndExtendsIInterface(overriddenClass: PsiClass): Boolean =
overriddenClass.isInterface &&
overriddenClass.extendsList?.referenceElements
?.any { it.qualifiedName == IINTERFACE_INTERFACE } == true
companion object {
val EXPLANATION = """
The @EnforcePermission annotation is used to indicate that the underlying binder code

35
tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt
View File

@@ -45,8 +45,19 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner {
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 = "${node.name}$HELPER_SUFFIX()"
val message = "Method must start with $targetExpression or super.${node.name}()"
val message =
"Method must start with $targetExpression or super.${node.name}(), if applicable"
val firstExpression = (node.uastBody as? UBlockExpression)
?.expressions?.firstOrNull()
@@ -99,16 +110,17 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner {
private const val HELPER_SUFFIX = "_enforcePermission"
private const val EXPLANATION = """
When @EnforcePermission is applied, the AIDL compiler generates a Stub method to do the
permission check called yourMethodName$HELPER_SUFFIX.
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 indirectly via super.yourMethodName().
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
briefDescription = """Missing permission-enforcing method call in AIDL method
|annotated with @EnforcePermission""".trimMargin(),
explanation = EXPLANATION,
category = Category.SECURITY,
@@ -120,6 +132,19 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner {
)
)
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

47
tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt
View File

@@ -16,17 +16,18 @@
package com.google.android.lint.aidl
import com.google.android.lint.CLASS_STUB
import com.intellij.psi.PsiAnonymousClass
import com.android.tools.lint.detector.api.JavaContext
import com.intellij.psi.PsiClass
import com.intellij.psi.PsiReferenceList
import org.jetbrains.uast.UMethod
/**
* Given a UMethod, determine if this method is
* an entrypoint to an interface generated by AIDL,
* returning the interface name if so
* the entrypoint to an interface generated by AIDL,
* returning the interface name if so, otherwise returning null
*/
fun getContainingAidlInterface(node: UMethod): String? {
if (!isInClassCalledStub(node)) return 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) {
@@ -38,11 +39,33 @@ fun getContainingAidlInterface(node: UMethod): String? {
return null
}
private fun isInClassCalledStub(node: UMethod): Boolean {
(node.containingClass as? PsiAnonymousClass)?.let {
return it.baseClassReference.referenceName == CLASS_STUB
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 node.containingClass?.extendsList?.referenceElements?.any {
it.referenceName == CLASS_STUB
} ?: false
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

4
tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorTest.kt
View File

@@ -326,7 +326,7 @@ class EnforcePermissionDetectorTest : LintDetectorTest() {
// A service with permission annotation on the method.
private val interfaceIFooMethodStub: TestFile = java(
"""
public interface IFooMethod {
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)
@@ -361,7 +361,7 @@ class EnforcePermissionDetectorTest : LintDetectorTest() {
// A service without any permission annotation.
private val interfaceIBarStub: TestFile = java(
"""
public interface IBar {
public interface IBar extends android.os.IInterface {
public static abstract class Stub extends android.os.Binder implements IBar {
@Override
public void testMethod() {}

87
tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt
View File

@@ -22,7 +22,9 @@ 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_ENFORCE_PERMISSION_HELPER,
EnforcePermissionHelperDetector.ISSUE_MISUSING_ENFORCE_PERMISSION
)
override fun lint(): TestLintTask = super.lint().allowMissingSdk()
@@ -47,7 +49,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() {
.run()
.expect(
"""
src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper]
src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper]
@Override
^
1 errors, 0 warnings
@@ -55,9 +57,9 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() {
)
.expectFixDiffs(
"""
Autofix for src/Foo.java line 5: Replace with super.test_enforcePermission();...:
Autofix for src/Foo.java line 5: Replace with test_enforcePermission();...:
@@ -8 +8
+ super.test_enforcePermission();
+ test_enforcePermission();
+
"""
)
@@ -85,7 +87,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() {
.run()
.expect(
"""
src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper]
src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper]
@Override
^
1 errors, 0 warnings
@@ -93,9 +95,9 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() {
)
.expectFixDiffs(
"""
Autofix for src/Foo.java line 5: Replace with super.test_enforcePermission();...:
Autofix for src/Foo.java line 5: Replace with test_enforcePermission();...:
@@ -8 +8
+ super.test_enforcePermission();
+ test_enforcePermission();
+
"""
)
@@ -120,7 +122,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() {
.run()
.expect(
"""
src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper]
src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper]
@Override
^
1 errors, 0 warnings
@@ -172,29 +174,29 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() {
.expectClean()
}
fun testInterfaceDefaultMethod_wouldStillReport() {
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";
}
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
}
"""
).indented(),
*stubs
)
.run()
.expect(
"""
src/IProtected.java:2: Error: Method must start with super.PermissionProtected_enforcePermission() or super.PermissionProtected() [MissingEnforcePermissionHelper]
@android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE)
^
1 errors, 0 warnings
"""
)
.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() {
@@ -343,7 +345,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() {
.run()
.expect(
"""
src/test/Bar.java:4: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper]
src/test/Bar.java:4: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper]
@Override
^
1 errors, 0 warnings
@@ -399,7 +401,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() {
.run()
.expect(
"""
src/test/Baz.java:4: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper]
src/test/Baz.java:4: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper]
@Override
^
1 errors, 0 warnings
@@ -407,6 +409,31 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() {
)
}
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)
}