Merge "Add SimpleRequiresNoPermissionDetector" into main am: e4de8c388b (efecf166) · Commits · e / os / android_frameworks_base

tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt

+2 −0

Original line number	Diff line number	Diff line
		@@ -22,6 +22,7 @@ import com.android.tools.lint.detector.api.CURRENT_API
		import com.google.android.lint.aidl.EnforcePermissionDetector
		import com.google.android.lint.aidl.PermissionAnnotationDetector
		import com.google.android.lint.aidl.SimpleManualPermissionEnforcementDetector
		import com.google.android.lint.aidl.SimpleRequiresNoPermissionDetector
		import com.google.auto.service.AutoService

		@AutoService(IssueRegistry::class)
		@@ -34,6 +35,7 @@ class AndroidGlobalIssueRegistry : IssueRegistry() {
		EnforcePermissionDetector.ISSUE_MISUSING_ENFORCE_PERMISSION,
		PermissionAnnotationDetector.ISSUE_MISSING_PERMISSION_ANNOTATION,
		SimpleManualPermissionEnforcementDetector.ISSUE_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT,
		SimpleRequiresNoPermissionDetector.ISSUE_SIMPLE_REQUIRES_NO_PERMISSION,
		)

		override val api: Int

tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleRequiresNoPermissionDetector.kt

0 → 100644

+118 −0

Original line number	Diff line number	Diff line
		/*
		* Copyright (C) 2024 The Android Open Source Project
		*
		* Licensed under the Apache License, Version 2.0 (the "License");
		* you may not use this file except in compliance with the License.
		* You may obtain a copy of the License at
		*
		* http://www.apache.org/licenses/LICENSE-2.0
		*
		* Unless required by applicable law or agreed to in writing, software
		* distributed under the License is distributed on an "AS IS" BASIS,
		* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
		* See the License for the specific language governing permissions and
		* limitations under the License.
		*/

		package com.google.android.lint.aidl

		import com.android.tools.lint.detector.api.Category
		import com.android.tools.lint.detector.api.Implementation
		import com.android.tools.lint.detector.api.Issue
		import com.android.tools.lint.detector.api.JavaContext
		import com.android.tools.lint.detector.api.Scope
		import com.android.tools.lint.detector.api.Severity
		import org.jetbrains.uast.UastCallKind
		import org.jetbrains.uast.UBlockExpression
		import org.jetbrains.uast.UCallExpression
		import org.jetbrains.uast.UElement
		import org.jetbrains.uast.UMethod
		import org.jetbrains.uast.visitor.AbstractUastVisitor

		/**
		* Ensures all AIDL implementations hosted by system_server which don't call other methods are
		* annotated with @RequiresNoPermission. AIDL Interfaces part of `exemptAidlInterfaces` are skipped
		* during this search to ensure the detector targets only new AIDL Interfaces.
		*/
		class SimpleRequiresNoPermissionDetector : AidlImplementationDetector() {
		override fun visitAidlMethod(
		context: JavaContext,
		node: UMethod,
		interfaceName: String,
		body: UBlockExpression
		) {
		if (!isSystemServicePath(context)) return
		if (context.evaluator.isAbstract(node)) return

		val fullyQualifiedInterfaceName =
		getContainingAidlInterfaceQualified(context, node) ?: return
		if (exemptAidlInterfaces.contains(fullyQualifiedInterfaceName)) return

		if (node.hasAnnotation(ANNOTATION_REQUIRES_NO_PERMISSION)) return

		if (!isCallingMethod(node)) {
		context.report(
		ISSUE_SIMPLE_REQUIRES_NO_PERMISSION,
		node,
		context.getLocation(node),
		"""
		Method ${node.name} doesn't perform any permission checks, meaning it should \
		be annotated with @RequiresNoPermission.
		""".trimMargin()
		)
		}
		}

		private fun isCallingMethod(node: UMethod): Boolean {
		val uCallExpressionVisitor = UCallExpressionVisitor()
		node.accept(uCallExpressionVisitor)

		return uCallExpressionVisitor.isCallingMethod
		}

		/**
		* Visits the body of a `UMethod` and determines if it encounters a `UCallExpression` which is
		* a `UastCallKind.METHOD_CALL`. `isCallingMethod` will hold the result of the search procedure.
		*/
		private class UCallExpressionVisitor : AbstractUastVisitor() {
		var isCallingMethod = false

		override fun visitElement(node: UElement): Boolean {
		// Stop the search early when a method call has been found.
		return isCallingMethod
		}

		override fun visitCallExpression(node: UCallExpression): Boolean {
		if (node.kind != UastCallKind.METHOD_CALL) return false

		isCallingMethod = true
		return true
		}
		}

		companion object {

		private val EXPLANATION = """
		Method implementations of AIDL Interfaces hosted by the `system_server` which do not
		call any other methods should be annotated with @RequiresNoPermission. That is because
		not calling any other methods implies that the method does not perform any permission
		checking.

		Please migrate to an @RequiresNoPermission annotation.
		""".trimIndent()

		@JvmField
		val ISSUE_SIMPLE_REQUIRES_NO_PERMISSION = Issue.create(
		id = "SimpleRequiresNoPermission",
		briefDescription = "System Service APIs not calling other methods should use @RNP",
		explanation = EXPLANATION,
		category = Category.SECURITY,
		priority = 5,
		severity = Severity.ERROR,
		implementation = Implementation(
		SimpleRequiresNoPermissionDetector::class.java,
		Scope.JAVA_FILE_SCOPE
		),
		)
		}
		}

tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/PermissionAnnotationDetectorTest.kt

+5 −50

Original line number	Diff line number	Diff line
		@@ -17,7 +17,6 @@
		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
		@@ -64,7 +63,7 @@ class PermissionAnnotationDetectorTest : LintDetectorTest() {
		"""
		package com.android.server;
		public class Bar extends IBar.Stub {
		public void testMethod() { }
		public void testMethod(int parameter1, int parameter2) { }
		}
		"""
		)
		@@ -75,8 +74,8 @@ class PermissionAnnotationDetectorTest : LintDetectorTest() {
		.expect(
		"""
		src/frameworks/base/services/java/com/android/server/Bar.java:3: Error: The method testMethod is not permission-annotated. [MissingPermissionAnnotation]
		public void testMethod() { }
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~
		public void testMethod(int parameter1, int parameter2) { }
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
		1 errors, 0 warnings
		"""
		)
		@@ -90,7 +89,7 @@ class PermissionAnnotationDetectorTest : LintDetectorTest() {
		"""
		package com.android.server;
		public class Bar extends IBar.Stub {
		public void testMethod() { }
		public void testMethod(int parameter1, int parameter2) { }
		}
		"""
		)
		@@ -132,7 +131,7 @@ class PermissionAnnotationDetectorTest : LintDetectorTest() {
		"""
		package com.android.server;
		public abstract class Bar extends IBar.Stub {
		public abstract void testMethod();
		public abstract void testMethod(int parameter1, int parameter2);
		}
		"""
		)
		@@ -177,50 +176,6 @@ class PermissionAnnotationDetectorTest : LintDetectorTest() {
		.expectClean()
		}

		/* Stubs */

		// A service with permission annotation on the method.
		private val interfaceIFoo: TestFile = java(
		"""
		public interface IFoo extends android.os.IInterface {
		public static abstract class Stub extends android.os.Binder implements IFoo {
		}
		@Override
		@android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE)
		public void testMethod();
		@Override
		@android.annotation.RequiresNoPermission
		public void testMethodNoPermission();
		@Override
		@android.annotation.PermissionManuallyEnforced
		public void testMethodManual();
		}
		"""
		).indented()

		// A service with no permission annotation.
		private val interfaceIBar: TestFile = java(
		"""
		public interface IBar extends android.os.IInterface {
		public static abstract class Stub extends android.os.Binder implements IBar {
		}
		public void testMethod();
		}
		"""
		).indented()

		// A service whose AIDL Interface is exempted.
		private val interfaceIExempted: TestFile = java(
		"""
		package android.accessibilityservice;
		public interface IBrailleDisplayConnection extends android.os.IInterface {
		public static abstract class Stub extends android.os.Binder implements IBrailleDisplayConnection {
		}
		public void testMethod();
		}
		"""
		).indented()

		private val stubs = arrayOf(interfaceIFoo, interfaceIBar, interfaceIExempted)

		private fun createVisitedPath(filename: String) =

tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleRequiresNoPermissionDetectorTest.kt

0 → 100644

+244 −0

Original line number	Diff line number	Diff line
		/*
		* Copyright (C) 2024 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

		class SimpleRequiresNoPermissionDetectorTest : LintDetectorTest() {
		override fun getDetector(): Detector = SimpleRequiresNoPermissionDetector()
		override fun getIssues(): List<Issue> = listOf(
		SimpleRequiresNoPermissionDetector
		.ISSUE_SIMPLE_REQUIRES_NO_PERMISSION
		)

		override fun lint(): TestLintTask = super.lint().allowMissingSdk()

		fun testRequiresNoPermissionUsedCorrectly_shouldNotWarn() {
		lint()
		.files(
		java(
		createVisitedPath("Foo.java"),
		"""
		package com.android.server;
		public class Foo extends IFoo.Stub {
		private int memberInt;

		@Override
		@android.annotation.RequiresNoPermission
		public void testMethodNoPermission(int parameter1, int parameter2) {
		if (parameter1 < parameter2) {
		memberInt = parameter1;
		} else {
		memberInt = parameter2;
		}
		}
		}
		"""
		)
		.indented(),
		*stubs
		)
		.run()
		.expectClean()
		}

		fun testMissingRequiresNoPermission_shouldWarn() {
		lint()
		.files(
		java(
		createVisitedPath("Bar.java"),
		"""
		package com.android.server;
		public class Bar extends IBar.Stub {
		private int memberInt;

		@Override
		public void testMethod(int parameter1, int parameter2) {
		if (parameter1 < parameter2) {
		memberInt = parameter1;
		} else {
		memberInt = parameter2;
		}
		}
		}
		"""
		)
		.indented(),
		*stubs
		)
		.run()
		.expect(
		"""
		src/frameworks/base/services/java/com/android/server/Bar.java:5: Error: Method testMethod doesn't perform any permission checks, meaning it should be annotated with @RequiresNoPermission. [SimpleRequiresNoPermission]
		@Override
		^
		1 errors, 0 warnings
		"""
		)
		}

		fun testMethodOnlyPerformsConstructorCall_shouldWarn() {
		lint()
		.files(
		java(
		createVisitedPath("Bar.java"),
		"""
		package com.android.server;
		public class Bar extends IBar.Stub {
		private IntPair memberIntPair;

		@Override
		public void testMethod(int parameter1, int parameter2) {
		memberIntPair = new IntPair(parameter1, parameter2);
		}

		private static class IntPair {
		public int first;
		public int second;

		public IntPair(int first, int second) {
		this.first = first;
		this.second = second;
		}
		}
		}
		"""
		)
		.indented(),
		*stubs
		)
		.run()
		.expect(
		"""
		src/frameworks/base/services/java/com/android/server/Bar.java:5: Error: Method testMethod doesn't perform any permission checks, meaning it should be annotated with @RequiresNoPermission. [SimpleRequiresNoPermission]
		@Override
		^
		1 errors, 0 warnings
		"""
		)
		}

		fun testMissingRequiresNoPermissionInIgnoredDirectory_shouldNotWarn() {
		lint()
		.files(
		java(
		ignoredPath,
		"""
		package com.android.server;
		public class Bar extends IBar.Stub {
		@Override
		public void testMethod(int parameter1, int parameter2) {}
		}
		"""
		)
		.indented(),
		*stubs
		)
		.run()
		.expectClean()
		}

		fun testMissingRequiresNoPermissionAbstractMethod_shouldNotWarn() {
		lint()
		.files(
		java(
		createVisitedPath("Bar.java"),
		"""
		package com.android.server;
		public abstract class Bar extends IBar.Stub {
		private int memberInt;

		@Override
		public abstract void testMethodNoPermission(int parameter1, int parameter2);
		}
		"""
		)
		.indented(),
		*stubs
		)
		.run()
		.expectClean()
		}

		// If this test fails, consider the following steps:
		// 1. Pick the first entry (interface) from `exemptAidlInterfaces`.
		// 2. Change `interfaceIExempted` to use that interface.
		// 3. Change this test's class to extend the interface's Stub.
		fun testMissingRequiresNoPermissionAidlInterfaceExempted_shouldNotWarn() {
		lint()
		.files(
		java(
		createVisitedPath("Bar.java"),
		"""
		package com.android.server;
		public class Bar extends android.accessibilityservice.IBrailleDisplayConnection.Stub {
		public void testMethod(int parameter1, int parameter2) {}
		}
		"""
		)
		.indented(),
		*stubs
		)
		.run()
		.expectClean()
		}

		fun testMethodMakesAnotherMethodCall_shouldNotWarn() {
		lint()
		.files(
		java(
		createVisitedPath("Bar.java"),
		"""
		package com.android.server;
		public class Bar extends IBar.Stub {
		private int memberInt;

		@Override
		public void testMethod(int parameter1, int parameter2) {
		if (!hasPermission()) return;

		if (parameter1 < parameter2) {
		memberInt = parameter1;
		} else {
		memberInt = parameter2;
		}
		}

		private bool hasPermission() {
		// Perform a permission check.
		return true;
		}
		}
		"""
		)
		.indented(),
		*stubs
		)
		.run()
		.expectClean()
		}

		private val stubs = arrayOf(interfaceIFoo, interfaceIBar, interfaceIExempted)

		private fun createVisitedPath(filename: String) =
		"src/frameworks/base/services/java/com/android/server/$filename"

		private val ignoredPath = "src/test/pkg/TestClass.java"
		}

tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt

+43 −1

Original line number	Diff line number	Diff line
		@@ -86,3 +86,45 @@ val manifestStub: TestFile = java(
		}
		""".trimIndent()
		)

		// A service with permission annotation on the method.
		val interfaceIFoo: TestFile = java(
		"""
		public interface IFoo extends android.os.IInterface {
		public static abstract class Stub extends android.os.Binder implements IFoo {
		}
		@Override
		@android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE)
		public void testMethod();
		@Override
		@android.annotation.RequiresNoPermission
		public void testMethodNoPermission(int parameter1, int parameter2);
		@Override
		@android.annotation.PermissionManuallyEnforced
		public void testMethodManual();
		}
		"""
		).indented()

		// A service with no permission annotation.
		val interfaceIBar: TestFile = java(
		"""
		public interface IBar extends android.os.IInterface {
		public static abstract class Stub extends android.os.Binder implements IBar {
		}
		public void testMethod(int parameter1, int parameter2);
		}
		"""
		).indented()

		// A service whose AIDL Interface is exempted.
		val interfaceIExempted: TestFile = java(
		"""
		package android.accessibilityservice;
		public interface IBrailleDisplayConnection extends android.os.IInterface {
		public static abstract class Stub extends android.os.Binder implements IBrailleDisplayConnection {
		}
		public void testMethod();
		}
		"""
		).indented()