From 5a2df65f5a8264066efa001ec1ce45765ac0d982 Mon Sep 17 00:00:00 2001 From: Bernardo Rufino Date: Thu, 28 May 2020 09:58:25 +0100 Subject: [PATCH] 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 --- Android.bp | 1 + core/java/android/app/ContextImpl.java | 1 + .../ClientSidePermissionCheckChecker.java | 82 +++++++++++++++++++ 3 files changed, 84 insertions(+) create mode 100644 errorprone/java/com/google/errorprone/bugpatterns/android/ClientSidePermissionCheckChecker.java diff --git a/Android.bp b/Android.bp index 325077f584ea0..10e5f7ae8128d 100644 --- a/Android.bp +++ b/Android.bp @@ -435,6 +435,7 @@ java_defaults { 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 a4256a9d8ab3e..48d333f178bd2 100644 --- a/core/java/android/app/ContextImpl.java +++ b/core/java/android/app/ContextImpl.java @@ -1933,6 +1933,7 @@ class ContextImpl extends Context { * 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 0000000000000..8651a1a7fbeba --- /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 INSIDE_MANAGER = + enclosingClass(hasAnnotation("android.annotation.SystemService")); + private static final Matcher 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; + } +}