From 0c4e6a8da3405f742e5cef8afdf579d58b6f1246 Mon Sep 17 00:00:00 2001 From: Dianne Hackborn Date: Fri, 13 May 2016 17:37:08 -0700 Subject: [PATCH] Fix issue #27532364: Security Vulnerability in IIntentSender.send We need to make IIntentSender oneway... but when the system is calling that for itself, it needs to be able to return a result code. Solution: instead of directly calling the interface, we have a new IPC through the activity manager. If the thing being used is the activity manager impl, it can do the synchronous send and return the result directly in place. If not, you only get asynchronous sending and thus never a failure result back (too bad for you!). Change-Id: I4096e5b00063e8dba66230585a2dfe67e35e8092 --- cmds/pm/src/com/android/commands/pm/Pm.java | 3 +- .../android/app/ActivityManagerNative.java | 48 +++++++++++++++++++ core/java/android/app/IActivityManager.java | 5 ++ core/java/android/app/PendingIntent.java | 3 +- core/java/android/content/IIntentSender.aidl | 4 +- core/java/android/content/IntentSender.java | 7 +-- .../server/am/ActivityManagerService.java | 26 ++++++++++ .../server/am/PendingIntentRecord.java | 24 ++++++---- .../server/pm/PackageManagerShellCommand.java | 3 +- 9 files changed, 103 insertions(+), 20 deletions(-) diff --git a/cmds/pm/src/com/android/commands/pm/Pm.java b/cmds/pm/src/com/android/commands/pm/Pm.java index 19cb927bd582e..4470eda0a2fa5 100644 --- a/cmds/pm/src/com/android/commands/pm/Pm.java +++ b/cmds/pm/src/com/android/commands/pm/Pm.java @@ -301,14 +301,13 @@ public final class Pm { private IIntentSender.Stub mLocalSender = new IIntentSender.Stub() { @Override - public int send(int code, Intent intent, String resolvedType, + public void send(int code, Intent intent, String resolvedType, IIntentReceiver finishedReceiver, String requiredPermission, Bundle options) { try { mResult.offer(intent, 5, TimeUnit.SECONDS); } catch (InterruptedException e) { throw new RuntimeException(e); } - return 0; } }; diff --git a/core/java/android/app/ActivityManagerNative.java b/core/java/android/app/ActivityManagerNative.java index a4f404f18ed6b..6ba6b0f07bc5c 100644 --- a/core/java/android/app/ActivityManagerNative.java +++ b/core/java/android/app/ActivityManagerNative.java @@ -2977,6 +2977,22 @@ public abstract class ActivityManagerNative extends Binder implements IActivityM reply.writeNoException(); return true; } + case SEND_INTENT_SENDER_TRANSACTION: { + data.enforceInterface(IActivityManager.descriptor); + IIntentSender sender = IIntentSender.Stub.asInterface(data.readStrongBinder()); + int scode = data.readInt(); + Intent intent = data.readInt() != 0 ? Intent.CREATOR.createFromParcel(data) : null; + String resolvedType = data.readString(); + IIntentReceiver finishedReceiver = IIntentReceiver.Stub.asInterface( + data.readStrongBinder()); + String requiredPermission = data.readString(); + Bundle options = data.readInt() != 0 ? Bundle.CREATOR.createFromParcel(data) : null; + int result = sendIntentSender(sender, scode, intent, resolvedType, finishedReceiver, + requiredPermission, options); + reply.writeNoException(); + reply.writeInt(result); + return true; + } } return super.onTransact(code, data, reply, flags); @@ -6973,5 +6989,37 @@ class ActivityManagerProxy implements IActivityManager reply.recycle(); } + public int sendIntentSender(IIntentSender target, int code, Intent intent, String resolvedType, + IIntentReceiver finishedReceiver, String requiredPermission, Bundle options) + throws RemoteException { + Parcel data = Parcel.obtain(); + Parcel reply = Parcel.obtain(); + data.writeInterfaceToken(IActivityManager.descriptor); + data.writeStrongBinder(target.asBinder()); + data.writeInt(code); + if ((intent!=null)) { + data.writeInt(1); + intent.writeToParcel(data, 0); + } + else { + data.writeInt(0); + } + data.writeString(resolvedType); + data.writeStrongBinder((((finishedReceiver!=null))?(finishedReceiver.asBinder()):(null))); + data.writeString(requiredPermission); + if ((options!=null)) { + data.writeInt(1); + options.writeToParcel(data, 0); + } + else { + data.writeInt(0); + } + mRemote.transact(SEND_INTENT_SENDER_TRANSACTION, data, reply, 0); + final int res = reply.readInt(); + data.recycle(); + reply.recycle(); + return res; + } + private IBinder mRemote; } diff --git a/core/java/android/app/IActivityManager.java b/core/java/android/app/IActivityManager.java index 2fcad0d3ced03..81788da78e83a 100644 --- a/core/java/android/app/IActivityManager.java +++ b/core/java/android/app/IActivityManager.java @@ -653,6 +653,10 @@ public interface IActivityManager extends IInterface { public void startConfirmDeviceCredentialIntent(Intent intent) throws RemoteException; + public int sendIntentSender(IIntentSender target, int code, Intent intent, String resolvedType, + IIntentReceiver finishedReceiver, String requiredPermission, Bundle options) + throws RemoteException; + /* * Private non-Binder interfaces */ @@ -1038,4 +1042,5 @@ public interface IActivityManager extends IInterface { int NOTIFY_LOCKED_PROFILE = IBinder.FIRST_CALL_TRANSACTION + 373; int START_CONFIRM_DEVICE_CREDENTIAL_INTENT = IBinder.FIRST_CALL_TRANSACTION + 374; int SEND_IDLE_JOB_TRIGGER_TRANSACTION = IBinder.FIRST_CALL_TRANSACTION + 375; + int SEND_INTENT_SENDER_TRANSACTION = IBinder.FIRST_CALL_TRANSACTION + 376; } diff --git a/core/java/android/app/PendingIntent.java b/core/java/android/app/PendingIntent.java index 412b098a58725..cb15392b6a8a1 100644 --- a/core/java/android/app/PendingIntent.java +++ b/core/java/android/app/PendingIntent.java @@ -803,7 +803,8 @@ public final class PendingIntent implements Parcelable { String resolvedType = intent != null ? intent.resolveTypeIfNeeded(context.getContentResolver()) : null; - int res = mTarget.send(code, intent, resolvedType, + int res = ActivityManagerNative.getDefault().sendIntentSender( + mTarget, code, intent, resolvedType, onFinished != null ? new FinishedDispatcher(this, onFinished, handler) : null, diff --git a/core/java/android/content/IIntentSender.aidl b/core/java/android/content/IIntentSender.aidl index f3affa7dc8539..45c62d4916258 100644 --- a/core/java/android/content/IIntentSender.aidl +++ b/core/java/android/content/IIntentSender.aidl @@ -21,7 +21,7 @@ import android.content.Intent; import android.os.Bundle; /** @hide */ -interface IIntentSender { - int send(int code, in Intent intent, String resolvedType, +oneway interface IIntentSender { + void send(int code, in Intent intent, String resolvedType, IIntentReceiver finishedReceiver, String requiredPermission, in Bundle options); } diff --git a/core/java/android/content/IntentSender.java b/core/java/android/content/IntentSender.java index 13a767e5dbac5..32ca6c2d4bac6 100644 --- a/core/java/android/content/IntentSender.java +++ b/core/java/android/content/IntentSender.java @@ -17,10 +17,6 @@ package android.content; import android.app.ActivityManagerNative; -import android.content.Context; -import android.content.Intent; -import android.content.IIntentSender; -import android.content.IIntentReceiver; import android.os.Bundle; import android.os.RemoteException; import android.os.Handler; @@ -191,7 +187,8 @@ public class IntentSender implements Parcelable { String resolvedType = intent != null ? intent.resolveTypeIfNeeded(context.getContentResolver()) : null; - int res = mTarget.send(code, intent, resolvedType, + int res = ActivityManagerNative.getDefault().sendIntentSender(mTarget, + code, intent, resolvedType, onFinished != null ? new FinishedDispatcher(this, onFinished, handler) : null, diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 313ca9a95415c..e0afc69fdc41d 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -7043,6 +7043,32 @@ public final class ActivityManagerService extends ActivityManagerNative return rec; } + @Override + public int sendIntentSender(IIntentSender target, int code, Intent intent, String resolvedType, + IIntentReceiver finishedReceiver, String requiredPermission, Bundle options) { + if (target instanceof PendingIntentRecord) { + return ((PendingIntentRecord)target).sendWithResult(code, intent, resolvedType, + finishedReceiver, requiredPermission, options); + } else { + try { + target.send(code, intent, resolvedType, null, requiredPermission, options); + } catch (RemoteException e) { + } + // Platform code can rely on getting a result back when the send is done, but if + // this intent sender is from outside of the system we can't rely on it doing that. + // So instead we don't give it the result receiver, and instead just directly + // report the finish immediately. + if (finishedReceiver != null) { + try { + finishedReceiver.performReceive(intent, 0, + null, null, false, false, UserHandle.getCallingUserId()); + } catch (RemoteException e) { + } + } + return 0; + } + } + @Override public void cancelIntentSender(IIntentSender sender) { if (!(sender instanceof PendingIntentRecord)) { diff --git a/services/core/java/com/android/server/am/PendingIntentRecord.java b/services/core/java/com/android/server/am/PendingIntentRecord.java index b8f45bc342a8f..1f8d26bc55d06 100644 --- a/services/core/java/com/android/server/am/PendingIntentRecord.java +++ b/services/core/java/com/android/server/am/PendingIntentRecord.java @@ -198,16 +198,21 @@ final class PendingIntentRecord extends IIntentSender.Stub { ref = new WeakReference(this); } - public int send(int code, Intent intent, String resolvedType, IIntentReceiver finishedReceiver, - String requiredPermission, Bundle options) throws TransactionTooLargeException { + public void send(int code, Intent intent, String resolvedType, IIntentReceiver finishedReceiver, + String requiredPermission, Bundle options) { + sendInner(code, intent, resolvedType, finishedReceiver, + requiredPermission, null, null, 0, 0, 0, options, null); + } + + public int sendWithResult(int code, Intent intent, String resolvedType, + IIntentReceiver finishedReceiver, String requiredPermission, Bundle options) { return sendInner(code, intent, resolvedType, finishedReceiver, requiredPermission, null, null, 0, 0, 0, options, null); } int sendInner(int code, Intent intent, String resolvedType, IIntentReceiver finishedReceiver, String requiredPermission, IBinder resultTo, String resultWho, int requestCode, - int flagsMask, int flagsValues, Bundle options, IActivityContainer container) - throws TransactionTooLargeException { + int flagsMask, int flagsValues, Bundle options, IActivityContainer container) { if (intent != null) intent.setDefusable(true); if (options != null) options.setDefusable(true); @@ -253,6 +258,7 @@ final class PendingIntentRecord extends IIntentSender.Stub { if (userId == UserHandle.USER_CURRENT) { userId = owner.mUserController.getCurrentOrTargetUserIdLocked(); } + int res = 0; switch (key.type) { case ActivityManager.INTENT_SENDER_ACTIVITY: if (options == null) { @@ -312,21 +318,23 @@ final class PendingIntentRecord extends IIntentSender.Stub { resolvedType, key.packageName, userId); } catch (RuntimeException e) { Slog.w(TAG, "Unable to send startService intent", e); + } catch (TransactionTooLargeException e) { + res = ActivityManager.START_CANCELED; } break; } - if (sendFinish) { + if (sendFinish && res != ActivityManager.START_CANCELED) { try { finishedReceiver.performReceive(new Intent(finalIntent), 0, null, null, false, false, key.userId); } catch (RemoteException e) { } } - + Binder.restoreCallingIdentity(origId); - - return 0; + + return res; } } return ActivityManager.START_CANCELED; diff --git a/services/core/java/com/android/server/pm/PackageManagerShellCommand.java b/services/core/java/com/android/server/pm/PackageManagerShellCommand.java index 4de8b57d4087b..1eeff14568c60 100644 --- a/services/core/java/com/android/server/pm/PackageManagerShellCommand.java +++ b/services/core/java/com/android/server/pm/PackageManagerShellCommand.java @@ -1463,14 +1463,13 @@ class PackageManagerShellCommand extends ShellCommand { private IIntentSender.Stub mLocalSender = new IIntentSender.Stub() { @Override - public int send(int code, Intent intent, String resolvedType, + public void send(int code, Intent intent, String resolvedType, IIntentReceiver finishedReceiver, String requiredPermission, Bundle options) { try { mResult.offer(intent, 5, TimeUnit.SECONDS); } catch (InterruptedException e) { throw new RuntimeException(e); } - return 0; } };