From db25df71be4c1bcc654f69ce9a8bff7e3ef46360 Mon Sep 17 00:00:00 2001 From: Yohei Yukawa Date: Thu, 27 Dec 2018 08:40:41 -0800 Subject: [PATCH] Simplify permission checks in IMMS#onShellCommand With this CL, all commands implemented in InputMethodManagerService always WRITE_SECURE_SETTINGS and INTERACT_ACROSS_USERS_FULL permissions no matter what command is requested. This should not be a big deal because we have already restricted the possible caller of IInputMethodManager#shellCommand() to shell user [1], which is expected to have this kind of permissions. The biggest motivation to do so is simplifying shell command handlers. By eagerly calling Binder.clearCallingIdentity() when start handling shell commands, we just focus on what each shell command handler need to check in terms of security and any other restriction such as UserManager.DISALLOW_DEBUGGING_FEATURES. In short, there should be no observable behavior differences. What this CL does are just removing redundant permission checks and redundant Binder.clearCallingIdentity(). [1]: If87189563ccaacd4f9c666bab4f9ad08a9343084 b8d240fa3f96b7b4ea35dd271beda789044d63ab Bug: 34886274 Test: 'adb shell ime' commands still work Change-Id: I6fd47b5cc1e7da7222774df20247a2c69a70f45b --- .../InputMethodManagerService.java | 90 +++++++++---------- 1 file changed, 43 insertions(+), 47 deletions(-) diff --git a/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java b/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java index c87e5cc081571..ef7555b5d8be7 100644 --- a/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java +++ b/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java @@ -4619,10 +4619,30 @@ public class InputMethodManagerService extends IInputMethodManager.Stub mService = service; } + @RequiresPermission(allOf = { + Manifest.permission.WRITE_SECURE_SETTINGS, + Manifest.permission.INTERACT_ACROSS_USERS_FULL}) @BinderThread @ShellCommandResult @Override public int onCommand(@Nullable String cmd) { + // For shell command, require all the permissions here in favor of code simplicity. + mService.mContext.enforceCallingPermission( + Manifest.permission.INTERACT_ACROSS_USERS_FULL, null); + mService.mContext.enforceCallingPermission( + Manifest.permission.WRITE_SECURE_SETTINGS, null); + + final long identity = Binder.clearCallingIdentity(); + try { + return onCommandWithSystemIdentity(cmd); + } finally { + Binder.restoreCallingIdentity(identity); + } + } + + @BinderThread + @ShellCommandResult + private int onCommandWithSystemIdentity(@Nullable String cmd) { if ("refresh_debug_properties".equals(cmd)) { return refreshDebugProperties(); } @@ -4767,25 +4787,13 @@ public class InputMethodManagerService extends IInputMethodManager.Stub */ @BinderThread @ShellCommandResult - @RequiresPermission(Manifest.permission.WRITE_SECURE_SETTINGS) private int handleShellCommandEnableDisableInputMethod( @NonNull ShellCommand shellCommand, boolean enabled) { final String id = shellCommand.getNextArgRequired(); final boolean previouslyEnabled; synchronized (mMethodMap) { - if (!calledFromValidUserLocked()) { - shellCommand.getErrPrintWriter().print("Must be called from the foreground user or" - + " with INTERACT_ACROSS_USERS_FULL"); - return ShellCommandResult.FAILURE; - } - mContext.enforceCallingPermission(Manifest.permission.WRITE_SECURE_SETTINGS, null); - final long ident = Binder.clearCallingIdentity(); - try { - previouslyEnabled = setInputMethodEnabledLocked(id, enabled); - } finally { - Binder.restoreCallingIdentity(ident); - } + previouslyEnabled = setInputMethodEnabledLocked(id, enabled); } final PrintWriter pr = shellCommand.getOutPrintWriter(); pr.print("Input method "); @@ -4805,7 +4813,9 @@ public class InputMethodManagerService extends IInputMethodManager.Stub @ShellCommandResult private int handleShellCommandSetInputMethod(@NonNull ShellCommand shellCommand) { final String id = shellCommand.getNextArgRequired(); - setInputMethod(null, id); + synchronized (mMethodMap) { + setInputMethodLocked(id, NOT_A_SUBTYPE_ID); + } final PrintWriter pr = shellCommand.getOutPrintWriter(); pr.print("Input method "); pr.print(id); @@ -4820,43 +4830,29 @@ public class InputMethodManagerService extends IInputMethodManager.Stub */ @BinderThread @ShellCommandResult - @RequiresPermission(Manifest.permission.WRITE_SECURE_SETTINGS) private int handleShellCommandResetInputMethod(@NonNull ShellCommand shellCommand) { synchronized (mMethodMap) { - if (!calledFromValidUserLocked()) { - shellCommand.getErrPrintWriter().print("Must be called from the foreground user or" - + " with INTERACT_ACROSS_USERS_FULL"); - return ShellCommandResult.FAILURE; - } - mContext.enforceCallingPermission(Manifest.permission.WRITE_SECURE_SETTINGS, null); final String nextIme; final List nextEnabledImes; - final long ident = Binder.clearCallingIdentity(); - try { - synchronized (mMethodMap) { - hideCurrentInputLocked(0, null); - unbindCurrentMethodLocked(); - // Reset the current IME - resetSelectedInputMethodAndSubtypeLocked(null); - // Also reset the settings of the current IME - mSettings.putSelectedInputMethod(null); - // Disable all enabled IMEs. - mSettings.getEnabledInputMethodListLocked().forEach( - imi -> setInputMethodEnabledLocked(imi.getId(), false)); - // Re-enable with default enabled IMEs. - InputMethodUtils.getDefaultEnabledImes(mContext, mMethodList).forEach( - imi -> setInputMethodEnabledLocked(imi.getId(), true)); - updateInputMethodsFromSettingsLocked(true /* enabledMayChange */); - InputMethodUtils.setNonSelectedSystemImesDisabledUntilUsed(mIPackageManager, - mSettings.getEnabledInputMethodListLocked(), - mSettings.getCurrentUserId(), - mContext.getBasePackageName()); - nextIme = mSettings.getSelectedInputMethod(); - nextEnabledImes = getEnabledInputMethodList(); - } - } finally { - Binder.restoreCallingIdentity(ident); - } + hideCurrentInputLocked(0, null); + unbindCurrentMethodLocked(); + // Reset the current IME + resetSelectedInputMethodAndSubtypeLocked(null); + // Also reset the settings of the current IME + mSettings.putSelectedInputMethod(null); + // Disable all enabled IMEs. + mSettings.getEnabledInputMethodListLocked().forEach( + imi -> setInputMethodEnabledLocked(imi.getId(), false)); + // Re-enable with default enabled IMEs. + InputMethodUtils.getDefaultEnabledImes(mContext, mMethodList).forEach( + imi -> setInputMethodEnabledLocked(imi.getId(), true)); + updateInputMethodsFromSettingsLocked(true /* enabledMayChange */); + InputMethodUtils.setNonSelectedSystemImesDisabledUntilUsed(mIPackageManager, + mSettings.getEnabledInputMethodListLocked(), + mSettings.getCurrentUserId(), + mContext.getBasePackageName()); + nextIme = mSettings.getSelectedInputMethod(); + nextEnabledImes = getEnabledInputMethodList(); final PrintWriter pr = shellCommand.getOutPrintWriter(); pr.println("Reset current and enabled IMEs"); pr.println("Newly selected IME:");