From 1561df4a9b09a4118c3dfddd80c5691a183a9939 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 5 Oct 2020 16:01:02 -0600 Subject: [PATCH 1/2] Detector for Binder.clearCallingIdentity() bugs. Binder maintains thread-local identity information about any remote caller, which can be temporarily cleared while performing operations that need to be handled as the current process. However, it's important to restore the original remote calling identity after carefully scoping this work inside a try/finally block, to avoid obscure security vulnerabilities. Bug: 155703208 Test: atest error_prone_android_framework_test Change-Id: I568771a50af27637e4984950dcada2248ce16afe --- .../android/BinderIdentityChecker.java | 108 +++++++++++++++++ .../android/BinderIdentityCheckerTest.java | 111 ++++++++++++++++++ errorprone/tests/res/android/os/Binder.java | 8 ++ 3 files changed, 227 insertions(+) create mode 100644 errorprone/java/com/google/errorprone/bugpatterns/android/BinderIdentityChecker.java create mode 100644 errorprone/tests/java/com/google/errorprone/bugpatterns/android/BinderIdentityCheckerTest.java diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/BinderIdentityChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/BinderIdentityChecker.java new file mode 100644 index 0000000000000..68477edf97d1f --- /dev/null +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/BinderIdentityChecker.java @@ -0,0 +1,108 @@ +/* + * Copyright (C) 2020 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.errorprone.bugpatterns.android; + +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Matchers.contains; +import static com.google.errorprone.matchers.Matchers.methodInvocation; +import static com.google.errorprone.matchers.Matchers.staticMethod; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.StatementTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; +import com.sun.source.tree.TryTree; +import com.sun.source.tree.VariableTree; + +import java.util.List; + +import javax.lang.model.element.Modifier; + +/** + * Binder maintains thread-local identity information about any remote caller, + * which can be temporarily cleared while performing operations that need to be + * handled as the current process. However, it's important to restore the + * original remote calling identity after carefully scoping this work inside a + * try/finally block, to avoid obscure security vulnerabilities. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AndroidFrameworkBinderIdentity", + summary = "Verifies that Binder.clearCallingIdentity() is always restored", + severity = WARNING) +public final class BinderIdentityChecker extends BugChecker implements MethodInvocationTreeMatcher { + private static final Matcher CLEAR_CALL = methodInvocation(staticMethod() + .onClass("android.os.Binder").withSignature("clearCallingIdentity()")); + private static final Matcher RESTORE_CALL = methodInvocation(staticMethod() + .onClass("android.os.Binder").withSignature("restoreCallingIdentity(long)")); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (CLEAR_CALL.matches(tree, state)) { + // First, make sure we're recording the token for later + final VariableTree token = state.findEnclosing(VariableTree.class); + if (token == null || !token.getModifiers().getFlags().contains(Modifier.FINAL)) { + return buildDescription(tree) + .setMessage("Must store Binder.clearCallingIdentity() token as final" + + " variable to support safe restore") + .build(); + } + + // Next, verify the very next block is try-finally; any other calls + // between the clearing and try risk throwing an exception without + // doing a safe restore + final Tree next = nextStatement(token, state); + if (next == null || next.getKind() != Kind.TRY) { + return buildDescription(tree) + .setMessage("Must immediately define a try-finally block after" + + " Binder.clearCallingIdentity() to support safe restore") + .build(); + } + + // Finally, verify that we restore inside the finally block + final TryTree tryTree = (TryTree) next; + final BlockTree finallyTree = tryTree.getFinallyBlock(); + if (finallyTree == null + || !contains(ExpressionTree.class, RESTORE_CALL).matches(finallyTree, state)) { + return buildDescription(tree) + .setMessage("Must call Binder.restoreCallingIdentity() in finally" + + " block to support safe restore") + .build(); + } + } + return Description.NO_MATCH; + } + + private static Tree nextStatement(Tree tree, VisitorState state) { + final BlockTree block = state.findEnclosing(BlockTree.class); + if (block == null) return null; + final List siblings = block.getStatements(); + if (siblings == null) return null; + final int index = siblings.indexOf(tree); + if (index == -1 || index + 1 >= siblings.size()) return null; + return siblings.get(index + 1); + } +} diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/BinderIdentityCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/BinderIdentityCheckerTest.java new file mode 100644 index 0000000000000..9448344f7abb3 --- /dev/null +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/BinderIdentityCheckerTest.java @@ -0,0 +1,111 @@ +/* + * Copyright (C) 2020 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.errorprone.bugpatterns.android; + +import com.google.errorprone.CompilationTestHelper; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class BinderIdentityCheckerTest { + private CompilationTestHelper compilationHelper; + + @Before + public void setUp() { + compilationHelper = CompilationTestHelper.newInstance( + BinderIdentityChecker.class, getClass()); + } + + @Test + public void testValid() { + compilationHelper + .addSourceFile("/android/os/Binder.java") + .addSourceLines("FooService.java", + "import android.os.Binder;", + "public class FooService {", + " void bar() {", + " final long token = Binder.clearCallingIdentity();", + " try {", + " FooService.class.toString();", + " } finally {", + " Binder.restoreCallingIdentity(token);", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void testInvalid() { + compilationHelper + .addSourceFile("/android/os/Binder.java") + .addSourceLines("FooService.java", + "import android.os.Binder;", + "public class FooService {", + " void noRestore() {", + " // BUG: Diagnostic contains:", + " final long token = Binder.clearCallingIdentity();", + " FooService.class.toString();", + " }", + " void noTry() {", + " // BUG: Diagnostic contains:", + " final long token = Binder.clearCallingIdentity();", + " FooService.class.toString();", + " Binder.restoreCallingIdentity(token);", + " }", + " void noImmediateTry() {", + " // BUG: Diagnostic contains:", + " final long token = Binder.clearCallingIdentity();", + " FooService.class.toString();", + " try {", + " FooService.class.toString();", + " } finally {", + " Binder.restoreCallingIdentity(token);", + " }", + " }", + " void noFinally() {", + " // BUG: Diagnostic contains:", + " final long token = Binder.clearCallingIdentity();", + " try {", + " FooService.class.toString();", + " } catch (Exception ignored) { }", + " }", + " void noFinal() {", + " // BUG: Diagnostic contains:", + " long token = Binder.clearCallingIdentity();", + " try {", + " FooService.class.toString();", + " } finally {", + " Binder.restoreCallingIdentity(token);", + " }", + " }", + " void noRecording() {", + " // BUG: Diagnostic contains:", + " Binder.clearCallingIdentity();", + " FooService.class.toString();", + " }", + " void noWork() {", + " // BUG: Diagnostic contains:", + " final long token = Binder.clearCallingIdentity();", + " }", + "}") + .doTest(); + } +} diff --git a/errorprone/tests/res/android/os/Binder.java b/errorprone/tests/res/android/os/Binder.java index d388587c2f58b..c969108bd425b 100644 --- a/errorprone/tests/res/android/os/Binder.java +++ b/errorprone/tests/res/android/os/Binder.java @@ -20,4 +20,12 @@ public class Binder { public static int getCallingUid() { throw new UnsupportedOperationException(); } + + public static long clearCallingIdentity() { + throw new UnsupportedOperationException(); + } + + public static void restoreCallingIdentity(long token) { + throw new UnsupportedOperationException(); + } } From 749789d24047f546a18392f83a9438bc737de490 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 5 Oct 2020 16:49:24 -0600 Subject: [PATCH 2/2] Detector to suggest more efficient collections. Android offers several efficient alternatives to some upstream Collections containers, such as SparseIntArray instead of Map. Bug: 155703208 Test: atest error_prone_android_framework_test Change-Id: I080fd9489fb037391b717901345a905a9753b370 --- .../android/EfficientCollectionsChecker.java | 98 +++++++++++++++++++ .../EfficientCollectionsCheckerTest.java | 96 ++++++++++++++++++ 2 files changed, 194 insertions(+) create mode 100644 errorprone/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsChecker.java create mode 100644 errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsCheckerTest.java diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsChecker.java new file mode 100644 index 0000000000000..c4c1ab6482ee5 --- /dev/null +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsChecker.java @@ -0,0 +1,98 @@ +/* + * Copyright (C) 2020 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.errorprone.bugpatterns.android; + +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Matchers.isSubtypeOf; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Type; + +import java.util.Collections; +import java.util.List; + +/** + * Android offers several efficient alternatives to some upstream + * {@link Collections} containers, such as {@code SparseIntArray} instead of + * {@code Map}. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AndroidFrameworkEfficientCollections", + summary = "Verifies efficient collections best-practices", + severity = WARNING) +public final class EfficientCollectionsChecker extends BugChecker implements NewClassTreeMatcher { + private static final Matcher IS_LIST = isSubtypeOf("java.util.List"); + private static final Matcher IS_MAP = isSubtypeOf("java.util.Map"); + + private static final String INTEGER = "java.lang.Integer"; + private static final String LONG = "java.lang.Long"; + private static final String BOOLEAN = "java.lang.Boolean"; + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + final List types = ASTHelpers.getType(tree).getTypeArguments(); + if (IS_LIST.matches(tree, state) && types != null && types.size() == 1) { + final Type first = types.get(0); + if (ASTHelpers.isSameType(first, state.getTypeFromString(INTEGER), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with IntArray for efficiency") + .build(); + } else if (ASTHelpers.isSameType(first, state.getTypeFromString(LONG), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with LongArray for efficiency") + .build(); + } + } else if (IS_MAP.matches(tree, state) && types != null && types.size() == 2) { + final Type first = types.get(0); + final Type second = types.get(1); + if (ASTHelpers.isSameType(first, state.getTypeFromString(INTEGER), state)) { + if (ASTHelpers.isSameType(second, state.getTypeFromString(INTEGER), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with SparseIntArray for efficiency") + .build(); + } else if (ASTHelpers.isSameType(second, state.getTypeFromString(LONG), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with SparseLongArray for efficiency") + .build(); + } else if (ASTHelpers.isSameType(second, state.getTypeFromString(BOOLEAN), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with SparseBooleanArray for efficiency") + .build(); + } else { + return buildDescription(tree) + .setMessage("Consider replacing with SparseArray for efficiency") + .build(); + } + } else if (ASTHelpers.isSameType(first, state.getTypeFromString(LONG), state)) { + return buildDescription(tree) + .setMessage("Consider replacing with LongSparseArray for efficiency") + .build(); + } + } + return Description.NO_MATCH; + } +} diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsCheckerTest.java new file mode 100644 index 0000000000000..e128b6ad2cfd9 --- /dev/null +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientCollectionsCheckerTest.java @@ -0,0 +1,96 @@ +/* + * Copyright (C) 2020 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.errorprone.bugpatterns.android; + +import com.google.errorprone.CompilationTestHelper; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class EfficientCollectionsCheckerTest { + private CompilationTestHelper compilationHelper; + + @Before + public void setUp() { + compilationHelper = CompilationTestHelper.newInstance( + EfficientCollectionsChecker.class, getClass()); + } + + @Test + public void testMap() { + compilationHelper + .addSourceLines("Example.java", + "import java.util.HashMap;", + "public class Example {", + " public void exampleInteger() {", + " // BUG: Diagnostic contains:", + " HashMap a = new HashMap<>();", + " // BUG: Diagnostic contains:", + " HashMap b = new HashMap<>();", + " // BUG: Diagnostic contains:", + " HashMap c = new HashMap<>();", + " // BUG: Diagnostic contains:", + " HashMap d = new HashMap<>();", + " }", + " public void exampleLong() {", + " // BUG: Diagnostic contains:", + " HashMap res = new HashMap<>();", + " }", + " public void exampleOther() {", + " HashMap res = new HashMap<>();", + " }", + "}") + .doTest(); + } + + @Test + public void testList() { + compilationHelper + .addSourceLines("Example.java", + "import java.util.ArrayList;", + "public class Example {", + " public void exampleInteger() {", + " // BUG: Diagnostic contains:", + " ArrayList res = new ArrayList<>();", + " }", + " public void exampleLong() {", + " // BUG: Diagnostic contains:", + " ArrayList res = new ArrayList<>();", + " }", + " public void exampleOther() {", + " ArrayList res = new ArrayList<>();", + " }", + "}") + .doTest(); + } + + @Test + public void testErasure() { + compilationHelper + .addSourceLines("Example.java", + "import java.util.HashMap;", + "public class Example {", + " public void example() {", + " HashMap a = new HashMap();", + " }", + "}") + .doTest(); + } +}