diff --git a/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt b/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt index 02d578451ed93..d19f4cceeaaf9 100644 --- a/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt +++ b/tools/lint/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt @@ -26,17 +26,18 @@ import com.google.auto.service.AutoService @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, - CallingSettingsNonUserGetterMethodsDetector.ISSUE_NON_USER_GETTER_CALLED, - EnforcePermissionDetector.ISSUE_MISSING_ENFORCE_PERMISSION, - EnforcePermissionDetector.ISSUE_MISMATCHING_ENFORCE_PERMISSION, - SaferParcelChecker.ISSUE_UNSAFE_API_USAGE, - PackageVisibilityDetector.ISSUE_PACKAGE_NAME_NO_PACKAGE_VISIBILITY_FILTERS, + 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, + EnforcePermissionDetector.ISSUE_MISSING_ENFORCE_PERMISSION, + EnforcePermissionDetector.ISSUE_MISMATCHING_ENFORCE_PERMISSION, + SaferParcelChecker.ISSUE_UNSAFE_API_USAGE, + PackageVisibilityDetector.ISSUE_PACKAGE_NAME_NO_PACKAGE_VISIBILITY_FILTERS ) override val api: Int @@ -46,8 +47,8 @@ class AndroidFrameworkIssueRegistry : IssueRegistry() { get() = 8 override val vendor: Vendor = Vendor( - vendorName = "Android", - feedbackUrl = "http://b/issues/new?component=315013", - contact = "brufino@google.com" + 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/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt index 930378b168b2a..0c375c358e61a 100644 --- a/tools/lint/checks/src/main/java/com/google/android/lint/CallingIdentityTokenDetector.kt +++ b/tools/lint/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() override fun getApplicableUastTypes(): List> = - 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 ) } + 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 (token != null) { + context.report( + ISSUE_USE_OF_CALLER_AWARE_METHODS_WITH_CLEARED_IDENTITY, + context.getLocation(node), + getIncidentMessageUseOfCallerAwareMethodsWithClearedIdentity( + token.variableName, + node.asRenderString() + ) + ) + } + } + /** - * 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) { - val token = findFirstTokenInScope(node) - if (isCallerAwareMethod(node) && token != null) { - context.report( - 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 + 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(strict = true) ?: return null + .getParentOfType(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/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt b/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt index e1a5c613dee15..d90f3e31baf97 100644 --- a/tools/lint/checks/src/test/java/com/google/android/lint/CallingIdentityTokenDetectorTest.kt +++ b/tools/lint/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 = 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 { }