From 6c2029b1baa129ce4832471b2d176801616f0080 Mon Sep 17 00:00:00 2001 From: Azhara Assanova Date: Mon, 19 Jul 2021 17:31:58 +0000 Subject: [PATCH] [CallingIdentityTokenDetector] New issue - result of clearCallingIdentity() not stored in a variable If the result of clearCallingIdentity() is not stored in a variable, it would be impossible to restore the calling identity which could lead to potential security vulnerabilities with identity. The detector reports the issue if the immediate parent of clearCallingIdentity() is not of type ULocalVariable. Bug: 193779272 Test: atest AndroidFrameworkLintCheckerTest --host Change-Id: Ic9a384ef33ac7fef6da5145bdd461e7c93d83310 --- .../lint/AndroidFrameworkIssueRegistry.kt | 29 +- .../lint/CallingIdentityTokenDetector.kt | 377 +++++++++++------- .../lint/CallingIdentityTokenDetectorTest.kt | 241 ++++++----- 3 files changed, 386 insertions(+), 261 deletions(-) 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 { }