SimpleManualPermissionEnforcementDetector: Handle simple if expressions (b5424c06) · Commits · e / os / android_frameworks_base

tools/lint/common/src/main/java/com/google/android/lint/PermissionMethodUtils.kt

+14 −0

Original line number	Diff line number	Diff line
		@@ -19,8 +19,10 @@ 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
		@@ -36,3 +38,15 @@ fun getPermissionMethodAnnotation(method: UMethod?): UAnnotation? = method?.uAnn
		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
		}

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

+80 −7

Original line number	Diff line number	Diff line
		@@ -20,14 +20,23 @@ 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.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.UIfExpression
		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

		/**
		@@ -76,7 +85,7 @@ data class EnforcePermissionFix(

		companion object {
		/**
		* conditionally constructs EnforcePermissionFix from a UCallExpression
		* Conditionally constructs EnforcePermissionFix from a UCallExpression
		* @return EnforcePermissionFix if the called method is annotated with @PermissionMethod, else null
		*/
		fun fromCallExpression(
		@@ -85,16 +94,72 @@ data class EnforcePermissionFix(
		): EnforcePermissionFix? {
		val method = callExpression.resolve()?.getUMethod() ?: return null
		val annotation = getPermissionMethodAnnotation(method) ?: return null
		val enforces = method.returnType == PsiType.VOID
		val returnsVoid = method.returnType == PsiType.VOID
		val orSelf = getAnnotationBooleanValue(annotation, "orSelf") ?: false
		return EnforcePermissionFix(
		listOf(getPermissionCheckLocation(context, callExpression)),
		getPermissionCheckValues(callExpression),
		// 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.
		errorLevel = enforces && orSelf
		errorLevel = isErrorLevel(throws = returnsVoid, orSelf = orSelf)
		)
		}

		/**
		* 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

		return EnforcePermissionFix(
		listOf(context.getLocation(ifExpression)),
		getPermissionCheckValues(callExpression),
		errorLevel = isErrorLevel(throws = true, orSelf = orSelf),
		)
		}

		@@ -172,5 +237,13 @@ data class EnforcePermissionFix(
		callExpression.getArgumentForParameter(it)?.evaluateString()
		}
		}

		/**
		* 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
		}
		}

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

+7 −13

Original line number	Diff line number	Diff line
		@@ -23,12 +23,11 @@ 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.google.android.lint.findCallExpression
		import org.jetbrains.uast.UBlockExpression
		import org.jetbrains.uast.UCallExpression
		import org.jetbrains.uast.UElement
		import org.jetbrains.uast.UIfExpression
		import org.jetbrains.uast.UMethod
		import org.jetbrains.uast.UQualifiedReferenceExpression
		import org.jetbrains.uast.skipParenthesizedExprDown

		/**
		@@ -102,18 +101,13 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() {
		*/
		private fun getPermissionCheckFix(startingExpression: UElement?, context: JavaContext):
		EnforcePermissionFix? {
		return when (startingExpression) {
		is UQualifiedReferenceExpression -> getPermissionCheckFix(
		startingExpression.selector, context
		)

		is UIfExpression -> getPermissionCheckFix(startingExpression.condition, context)

		is UCallExpression -> return EnforcePermissionFix
		.fromCallExpression(context, startingExpression)

		else -> null
		if (startingExpression is UIfExpression) {
		return EnforcePermissionFix.fromIfExpression(context, startingExpression)
		}
		findCallExpression(startingExpression)?.let {
		return EnforcePermissionFix.fromCallExpression(context, it)
		}
		return null
		}

		companion object {

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

+109 −0

Original line number	Diff line number	Diff line
		@@ -564,7 +564,116 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
		)
		}

		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: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
		if (mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo")
		^
		1 errors, 0 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!");
		- }
		"""
		)
		}

		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!");
		- }
		"""
		)
		}

		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()
		}

		companion object {
		val stubs = arrayOf(

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

+2 −0

Original line number	Diff line number	Diff line
		@@ -23,6 +23,8 @@ val contextStub: TestFile = java(
		public void enforceCallingPermission(@android.content.pm.PermissionName String permission, String message) {}
		@android.content.pm.PermissionMethod(orSelf = true)
		public int checkCallingOrSelfPermission(@android.content.pm.PermissionName String permission, String message) {}
		@android.content.pm.PermissionMethod
		public int checkCallingPermission(@android.content.pm.PermissionName String permission, String message) {}
		}
		"""
		).indented()