Add AndroidFrameworkClientSidePermissionCheck errorprone check
Often a permission check in the app's process is an indicative of a
security issue since the app could work around it. Permission checks
should be done on system_server.
This errorprone warning checks for invocations of
Context.checkPermission() in any class inside android.app package and
emits a warning if it finds one. I also added a @SuppressWarnings for
one such call that has a todo and it and seems like an already tracked
workaround. The other call found by the checker is tracked in
b/157548188.
I also found that errorprone was not running for framework-minus-apex,
so I added the plugin to the relevant build rule. Let me know if this is
not the way to go!
Test: build/soong/soong_ui.bash --make-mode framework-minus-apex
RUN_ERROR_PRONE=true
Bug: 157626959
Change-Id: Ieb94f2f43722837c8354ac66474797f4f338ae16
diff --git a/Android.bp b/Android.bp
index 325077f..10e5f7a 100644
--- a/Android.bp
+++ b/Android.bp
@@ -435,6 +435,7 @@
plugins: [
"view-inspector-annotation-processor",
"staledataclass-annotation-processor",
+ "error_prone_android_framework",
],
required: [
diff --git a/core/java/android/app/ContextImpl.java b/core/java/android/app/ContextImpl.java
index a4256a9..48d333f 100644
--- a/core/java/android/app/ContextImpl.java
+++ b/core/java/android/app/ContextImpl.java
@@ -1933,6 +1933,7 @@
* Temporary workaround to permit incorrect usages of Context by SystemUI.
* TODO(b/147647877): Fix usages and remove.
*/
+ @SuppressWarnings("AndroidFrameworkClientSidePermissionCheck")
private static boolean isSystemOrSystemUI(Context context) {
return ActivityThread.isSystem() || context.checkPermission(
"android.permission.STATUS_BAR_SERVICE",
diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/ClientSidePermissionCheckChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/ClientSidePermissionCheckChecker.java
new file mode 100644
index 0000000..8651a1a
--- /dev/null
+++ b/errorprone/java/com/google/errorprone/bugpatterns/android/ClientSidePermissionCheckChecker.java
@@ -0,0 +1,82 @@
+/*
+ * 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.anyOf;
+import static com.google.errorprone.matchers.Matchers.enclosingClass;
+import static com.google.errorprone.matchers.Matchers.hasAnnotation;
+import static com.google.errorprone.matchers.Matchers.instanceMethod;
+import static com.google.errorprone.matchers.Matchers.methodInvocation;
+
+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.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.Tree;
+
+/**
+ * Often a permission check in the app's process is an indicative of a security issue since the app
+ * could work around it. Permission checks should be done on system_server.
+ */
+@AutoService(BugChecker.class)
+@BugPattern(
+ name = "AndroidFrameworkClientSidePermissionCheck",
+ summary = "Verifies that permission checks aren't done in the app's process",
+ severity = WARNING)
+public final class ClientSidePermissionCheckChecker
+ extends BugChecker implements MethodInvocationTreeMatcher {
+ private static final Matcher<Tree> INSIDE_MANAGER =
+ enclosingClass(hasAnnotation("android.annotation.SystemService"));
+ private static final Matcher<ExpressionTree> PERMISSION_CHECK_METHOD =
+ anyOf(
+ methodInvocation(
+ instanceMethod()
+ .onDescendantOf("android.content.Context")
+ .named("checkPermission")),
+ methodInvocation(
+ instanceMethod()
+ .onDescendantOf("android.content.Context")
+ .named("enforceCallingPermission")),
+ methodInvocation(
+ instanceMethod()
+ .onDescendantOf("android.content.Context")
+ .named("enforceCallingOrSelfPermission")),
+ methodInvocation(
+ instanceMethod()
+ .onDescendantOf("android.content.pm.PackageManager")
+ .named("checkPermission")));
+ private static final String ERROR_MESSAGE =
+ "Permission checks should be made in system_server, not in the app's process, since "
+ + "they could be easily bypassed";
+
+ @Override
+ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+ if (INSIDE_MANAGER.matches(tree, state)
+ && PERMISSION_CHECK_METHOD.matches(tree, state)) {
+ return buildDescription(tree)
+ .setMessage(ERROR_MESSAGE)
+ .build();
+ }
+ return Description.NO_MATCH;
+ }
+}