Merge "Remove broken code for mounting encrypted OBB files"

10 files changed, 41 insertions, 129 deletions

diff --git a/core/java/android/os/storage/IStorageManager.aidl b/core/java/android/os/storage/IStorageManager.aidl
index 9385402c3d72..6c0a1f99e112 100644
--- a/core/java/android/os/storage/IStorageManager.aidl
+++ b/core/java/android/os/storage/IStorageManager.aidl
@@ -54,13 +54,13 @@ interface IStorageManager {
      */
     void shutdown(IStorageShutdownObserver observer) = 19;
     /**
-     * Mounts an Opaque Binary Blob (OBB) with the specified decryption key and
-     * only allows the calling process's UID access to the contents.
-     * StorageManagerService will call back to the supplied IObbActionListener to inform
-     * it of the terminal state of the call.
+     * Mounts an Opaque Binary Blob (OBB). Only allows the calling process's UID
+     * access to the contents. StorageManagerService will call back to the
+     * supplied IObbActionListener to inform it of the terminal state of the
+     * call.
      */
-    void mountObb(in String rawPath, in String canonicalPath, in String key,
-            IObbActionListener token, int nonce, in ObbInfo obbInfo) = 21;
+    void mountObb(in String rawPath, in String canonicalPath, IObbActionListener token,
+            int nonce, in ObbInfo obbInfo) = 21;
     /**
      * Unmounts an Opaque Binary Blob (OBB). When the force flag is specified,
      * any program using it will be forcibly killed to unmount the image.
diff --git a/core/java/android/os/storage/StorageManager.java b/core/java/android/os/storage/StorageManager.java
index 77c794cd17a8..39f87d558098 100644
--- a/core/java/android/os/storage/StorageManager.java
+++ b/core/java/android/os/storage/StorageManager.java
@@ -665,9 +665,7 @@ public class StorageManager {
     }
 
     /**
-     * Mount an Opaque Binary Blob (OBB) file. If a <code>key</code> is
-     * specified, it is supplied to the mounting process to be used in any
-     * encryption used in the OBB.
+     * Mount an Opaque Binary Blob (OBB) file.
      * <p>
      * The OBB will remain mounted for as long as the StorageManager reference
      * is held by the application. As soon as this reference is lost, the OBBs
@@ -680,19 +678,22 @@ public class StorageManager {
      * application's OBB that shares its UID.
      *
      * @param rawPath the path to the OBB file
-     * @param key secret used to encrypt the OBB; may be <code>null</code> if no
-     *            encryption was used on the OBB.
+     * @param key must be <code>null</code>. Previously, some Android device
+     *            implementations accepted a non-<code>null</code> key to mount
+     *            an encrypted OBB file. However, this never worked reliably and
+     *            is no longer supported.
      * @param listener will receive the success or failure of the operation
      * @return whether the mount call was successfully queued or not
      */
     public boolean mountObb(String rawPath, String key, OnObbStateChangeListener listener) {
         Preconditions.checkNotNull(rawPath, "rawPath cannot be null");
+        Preconditions.checkArgument(key == null, "mounting encrypted OBBs is no longer supported");
         Preconditions.checkNotNull(listener, "listener cannot be null");
 
         try {
             final String canonicalPath = new File(rawPath).getCanonicalPath();
             final int nonce = mObbActionListener.addListener(listener);
-            mStorageManager.mountObb(rawPath, canonicalPath, key, mObbActionListener, nonce,
+            mStorageManager.mountObb(rawPath, canonicalPath, mObbActionListener, nonce,
                     getObbInfo(canonicalPath));
             return true;
         } catch (IOException e) {
diff --git a/core/tests/coretests/res/raw/obb_enc_file100_orig1.obb b/core/tests/coretests/res/raw/obb_enc_file100_orig1.obb
deleted file mode 100644
index 373b8e4bd01f..000000000000
--- a/core/tests/coretests/res/raw/obb_enc_file100_orig1.obb
+++ /dev/null
diff --git a/core/tests/coretests/res/raw/obb_enc_file100_orig3.obb b/core/tests/coretests/res/raw/obb_enc_file100_orig3.obb
deleted file mode 100644
index aa531d8b35bd..000000000000
--- a/core/tests/coretests/res/raw/obb_enc_file100_orig3.obb
+++ /dev/null
diff --git a/core/tests/coretests/src/android/os/storage/StorageManagerBaseTest.java b/core/tests/coretests/src/android/os/storage/StorageManagerBaseTest.java
index 16dcff5e0c8c..e56c0ad0138a 100644
--- a/core/tests/coretests/src/android/os/storage/StorageManagerBaseTest.java
+++ b/core/tests/coretests/src/android/os/storage/StorageManagerBaseTest.java
@@ -46,11 +46,7 @@ public class StorageManagerBaseTest extends InstrumentationTestCase {
     protected static String OBB_FILE_1_CONTENTS_1 = "OneToOneThousandInts.bin";
     protected static String OBB_FILE_2 = "obb_file2.obb";
     protected static String OBB_FILE_3 = "obb_file3.obb";
-    protected static String OBB_FILE_1_PASSWORD = "password1";
-    protected static String OBB_FILE_1_ENCRYPTED = "obb_enc_file100_orig1.obb";
     protected static String OBB_FILE_2_UNSIGNED = "obb_file2_nosign.obb";
-    protected static String OBB_FILE_3_PASSWORD = "password3";
-    protected static String OBB_FILE_3_ENCRYPTED = "obb_enc_file100_orig3.obb";
     protected static String OBB_FILE_3_BAD_PACKAGENAME = "obb_file3_bad_packagename.obb";
 
     protected static boolean FORCE = true;
@@ -180,22 +176,21 @@ public class StorageManagerBaseTest extends InstrumentationTestCase {
      * Mounts an OBB file
      *
      * @param obbFilePath The full path to the OBB file to mount
-     * @param key (optional) The key to use to unencrypt the OBB; pass null for no encryption
      * @param expectedState The expected state resulting from trying to mount the OBB
      * @return A {@link String} representing the normalized path to OBB file that was mounted
      */
-    protected String mountObb(String obbFilePath, String key, int expectedState) {
-        return doMountObb(obbFilePath, key, expectedState);
+    protected String mountObb(String obbFilePath, int expectedState) {
+        return doMountObb(obbFilePath, expectedState);
     }
 
     /**
-     * Mounts an OBB file with default options (no encryption, mounting succeeds)
+     * Mounts an OBB file with default options.
      *
      * @param obbFilePath The full path to the OBB file to mount
      * @return A {@link String} representing the normalized path to OBB file that was mounted
      */
     protected String mountObb(String obbFilePath) {
-        return doMountObb(obbFilePath, null, OnObbStateChangeListener.MOUNTED);
+        return doMountObb(obbFilePath, OnObbStateChangeListener.MOUNTED);
     }
 
     /**
@@ -232,13 +227,13 @@ public class StorageManagerBaseTest extends InstrumentationTestCase {
      * @return true if the listener was signaled of a state change by the system; else a fail()
      *      is triggered if we timed out
      */
-    protected String doMountObb_noThrow(String obbFilePath, String key, int expectedState) {
-        Log.i(LOG_TAG, "doMountObb() on " + obbFilePath + " using key: " + key);
+    protected String doMountObb_noThrow(String obbFilePath, int expectedState) {
+        Log.i(LOG_TAG, "doMountObb() on " + obbFilePath);
         assertTrue ("Null path was passed in for OBB file!", obbFilePath != null);
         assertTrue ("Null path was passed in for OBB file!", obbFilePath != null);
 
         ObbListener obbListener = new ObbListener();
-        boolean success = mSm.mountObb(obbFilePath, key, obbListener);
+        boolean success = mSm.mountObb(obbFilePath, null, obbListener);
         success &= obbFilePath.equals(doWaitForObbStateChange(obbListener));
         success &= (expectedState == obbListener.state());
 
@@ -260,17 +255,16 @@ public class StorageManagerBaseTest extends InstrumentationTestCase {
      * Mounts an OBB file without throwing and synchronously waits for it to finish mounting
      *
      * @param obbFilePath The full path to the OBB file to mount
-     * @param key (optional) The key to use to unencrypt the OBB; pass null for no encryption
      * @param expectedState The expected state resulting from trying to mount the OBB
      * @return A {@link String} representing the actual normalized path to OBB file that was
      *      mounted, or null if the mounting failed
      */
-    protected String doMountObb(String obbFilePath, String key, int expectedState) {
-        Log.i(LOG_TAG, "doMountObb() on " + obbFilePath + " using key: " + key);
+    protected String doMountObb(String obbFilePath, int expectedState) {
+        Log.i(LOG_TAG, "doMountObb() on " + obbFilePath);
         assertTrue ("Null path was passed in for OBB file!", obbFilePath != null);
 
         ObbListener obbListener = new ObbListener();
-        assertTrue("mountObb call failed", mSm.mountObb(obbFilePath, key, obbListener));
+        assertTrue("mountObb call failed", mSm.mountObb(obbFilePath, null, obbListener));
         assertTrue("Failed to get OBB mount status change for file: " + obbFilePath,
                 doWaitForObbStateChange(obbListener));
         assertEquals("OBB mount state not what was expected!", expectedState,
diff --git a/core/tests/coretests/src/android/os/storage/StorageManagerIntegrationTest.java b/core/tests/coretests/src/android/os/storage/StorageManagerIntegrationTest.java
index 62f2ac28a601..ecd2f76a5160 100644
--- a/core/tests/coretests/src/android/os/storage/StorageManagerIntegrationTest.java
+++ b/core/tests/coretests/src/android/os/storage/StorageManagerIntegrationTest.java
@@ -83,58 +83,6 @@ public class StorageManagerIntegrationTest extends StorageManagerBaseTest {
     }
 
     /**
-     * Tests mounting a single encrypted OBB file and verifies its contents.
-     */
-    @LargeTest
-    public void testMountSingleEncryptedObb() throws Exception {
-        final File file = createObbFile(OBB_FILE_3_ENCRYPTED, R.raw.obb_enc_file100_orig3);
-        String filePath = file.getAbsolutePath();
-        mountObb(filePath, OBB_FILE_3_PASSWORD, OnObbStateChangeListener.MOUNTED);
-        verifyObb3Contents(filePath);
-        unmountObb(filePath, DONT_FORCE);
-    }
-
-    /**
-     * Tests mounting a single encrypted OBB file using an invalid password.
-     */
-    @LargeTest
-    public void testMountSingleEncryptedObbInvalidPassword() throws Exception {
-        final File file = createObbFile("bad password@$%#@^*(!&)", R.raw.obb_enc_file100_orig3);
-        String filePath = file.getAbsolutePath();
-        mountObb(filePath, OBB_FILE_1_PASSWORD, OnObbStateChangeListener.ERROR_COULD_NOT_MOUNT);
-    }
-
-    /**
-     * Tests simultaneously mounting 2 encrypted OBBs with different keys and verifies contents.
-     */
-    @LargeTest
-    public void testMountTwoEncryptedObb() throws Exception {
-        File file3 = null;
-        File file1 = null;
-        try {
-            file3 = createObbFile(OBB_FILE_3_ENCRYPTED, R.raw.obb_enc_file100_orig3);
-            String filePath3 = file3.getAbsolutePath();
-            mountObb(filePath3, OBB_FILE_3_PASSWORD, OnObbStateChangeListener.MOUNTED);
-            verifyObb3Contents(filePath3);
-
-            file1 = createObbFile(OBB_FILE_1_ENCRYPTED, R.raw.obb_enc_file100_orig1);
-            String filePath1 = file1.getAbsolutePath();
-            mountObb(filePath1, OBB_FILE_1_PASSWORD, OnObbStateChangeListener.MOUNTED);
-            verifyObb1Contents(filePath1);
-
-            unmountObb(filePath3, DONT_FORCE);
-            unmountObb(filePath1, DONT_FORCE);
-        } finally {
-            if (file3 != null) {
-                file3.delete();
-            }
-            if (file1 != null) {
-                file1.delete();
-            }
-        }
-    }
-
-    /**
      * Tests mounting a single OBB that isn't signed.
      */
     @LargeTest
@@ -142,7 +90,7 @@ public class StorageManagerIntegrationTest extends StorageManagerBaseTest {
         final File file = createObbFile(OBB_FILE_2_UNSIGNED, R.raw.obb_file2_nosign);
         String filePath = file.getAbsolutePath();
         try {
-            mountObb(filePath, OBB_FILE_2_UNSIGNED, OnObbStateChangeListener.ERROR_INTERNAL);
+            mountObb(filePath, OnObbStateChangeListener.ERROR_INTERNAL);
             fail("mountObb should've failed with an exception");
         } catch (IllegalArgumentException e) {
             // Expected
@@ -156,8 +104,7 @@ public class StorageManagerIntegrationTest extends StorageManagerBaseTest {
     public void testMountBadPackageNameObb() throws Exception {
         final File file = createObbFile(OBB_FILE_3_BAD_PACKAGENAME, R.raw.obb_file3_bad_packagename);
         String filePath = file.getAbsolutePath();
-        mountObb(filePath, OBB_FILE_3_BAD_PACKAGENAME,
-                OnObbStateChangeListener.ERROR_PERMISSION_DENIED);
+        mountObb(filePath, OnObbStateChangeListener.ERROR_PERMISSION_DENIED);
     }
 
     /**
@@ -169,7 +116,7 @@ public class StorageManagerIntegrationTest extends StorageManagerBaseTest {
         String filePath = file.getAbsolutePath();
         mountObb(filePath);
         verifyObb1Contents(filePath);
-        mountObb(filePath, null, OnObbStateChangeListener.ERROR_ALREADY_MOUNTED);
+        mountObb(filePath, OnObbStateChangeListener.ERROR_ALREADY_MOUNTED);
         verifyObb1Contents(filePath);
         unmountObb(filePath, DONT_FORCE);
     }
diff --git a/libs/storage/IMountService.cpp b/libs/storage/IMountService.cpp
index fd6e6e932ebc..055dbb2b5c5e 100644
--- a/libs/storage/IMountService.cpp
+++ b/libs/storage/IMountService.cpp
@@ -442,14 +442,13 @@ public:
         reply.readExceptionCode();
     }
 
-    void mountObb(const String16& rawPath, const String16& canonicalPath, const String16& key,
+    void mountObb(const String16& rawPath, const String16& canonicalPath,
             const sp<IObbActionListener>& token, int32_t nonce, const sp<ObbInfo>& obbInfo)
     {
         Parcel data, reply;
         data.writeInterfaceToken(IMountService::getInterfaceDescriptor());
         data.writeString16(rawPath);
         data.writeString16(canonicalPath);
-        data.writeString16(key);
         data.writeStrongBinder(IInterface::asBinder(token));
         data.writeInt32(nonce);
         obbInfo->writeToParcel(&data);
diff --git a/libs/storage/include/storage/IMountService.h b/libs/storage/include/storage/IMountService.h
index 2463e023efc1..5b07318f4432 100644
--- a/libs/storage/include/storage/IMountService.h
+++ b/libs/storage/include/storage/IMountService.h
@@ -64,8 +64,8 @@ public:
     virtual void shutdown(const sp<IMountShutdownObserver>& observer) = 0;
     virtual void finishMediaUpdate() = 0;
     virtual void mountObb(const String16& rawPath, const String16& canonicalPath,
-            const String16& key, const sp<IObbActionListener>& token,
-            const int32_t nonce, const sp<ObbInfo>& obbInfo) = 0;
+            const sp<IObbActionListener>& token, const int32_t nonce,
+            const sp<ObbInfo>& obbInfo) = 0;
     virtual void unmountObb(const String16& filename, const bool force,
             const sp<IObbActionListener>& token, const int32_t nonce) = 0;
     virtual bool isObbMounted(const String16& filename) = 0;
diff --git a/native/android/storage_manager.cpp b/native/android/storage_manager.cpp
index 22725254fef6..9e0a6eb476d3 100644
--- a/native/android/storage_manager.cpp
+++ b/native/android/storage_manager.cpp
@@ -140,8 +140,7 @@ public:
         }
     }
 
-    void mountObb(const char* rawPath, const char* key, AStorageManager_obbCallbackFunc func,
-            void* data) {
+    void mountObb(const char* rawPath, AStorageManager_obbCallbackFunc func, void* data) {
         // Resolve path before sending to MountService
         char canonicalPath[PATH_MAX];
         if (realpath(rawPath, canonicalPath) == NULL) {
@@ -158,9 +157,7 @@ public:
         ObbCallback* cb = registerObbCallback(func, data);
         String16 rawPath16(rawPath);
         String16 canonicalPath16(canonicalPath);
-        String16 key16(key);
-        mMountService->mountObb(rawPath16, canonicalPath16, key16, mObbActionListener,
-                cb->nonce, obbInfo);
+        mMountService->mountObb(rawPath16, canonicalPath16, mObbActionListener, cb->nonce, obbInfo);
     }
 
     void unmountObb(const char* filename, const bool force, AStorageManager_obbCallbackFunc func, void* data) {
@@ -207,7 +204,11 @@ void AStorageManager_delete(AStorageManager* mgr) {
 
 void AStorageManager_mountObb(AStorageManager* mgr, const char* filename, const char* key,
         AStorageManager_obbCallbackFunc cb, void* data) {
-    mgr->mountObb(filename, key, cb, data);
+    if (key != nullptr && key[0] != '\0') {
+        ALOGE("mounting encrypted OBBs is no longer supported");
+        return;
+    }
+    mgr->mountObb(filename, cb, data);
 }
 
 void AStorageManager_unmountObb(AStorageManager* mgr, const char* filename, const int force,
diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java
index 9546496fd8e8..9266bb431aa5 100644
--- a/services/core/java/com/android/server/StorageManagerService.java
+++ b/services/core/java/com/android/server/StorageManagerService.java
@@ -173,9 +173,6 @@ import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.PrintWriter;
-import java.math.BigInteger;
-import java.security.GeneralSecurityException;
-import java.security.spec.KeySpec;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -194,10 +191,6 @@ import java.util.concurrent.TimeoutException;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
-import javax.crypto.SecretKey;
-import javax.crypto.SecretKeyFactory;
-import javax.crypto.spec.PBEKeySpec;
-
 /**
  * Service responsible for various storage media. Connects to {@code vold} to
  * watch for and manage dynamically added storage, such as SD cards and USB mass
@@ -3073,8 +3066,8 @@ class StorageManagerService extends IStorageManager.Stub
     }
 
     @Override
-    public void mountObb(String rawPath, String canonicalPath, String key,
-            IObbActionListener token, int nonce, ObbInfo obbInfo) {
+    public void mountObb(String rawPath, String canonicalPath, IObbActionListener token,
+            int nonce, ObbInfo obbInfo) {
         Objects.requireNonNull(rawPath, "rawPath cannot be null");
         Objects.requireNonNull(canonicalPath, "canonicalPath cannot be null");
         Objects.requireNonNull(token, "token cannot be null");
@@ -3083,7 +3076,7 @@ class StorageManagerService extends IStorageManager.Stub
         final int callingUid = Binder.getCallingUid();
         final ObbState obbState = new ObbState(rawPath, canonicalPath,
                 callingUid, token, nonce, null);
-        final ObbAction action = new MountObbAction(obbState, key, callingUid, obbInfo);
+        final ObbAction action = new MountObbAction(obbState, callingUid, obbInfo);
         mObbActionHandler.sendMessage(mObbActionHandler.obtainMessage(OBB_RUN_ACTION, action));
 
         if (DEBUG_OBB)
@@ -4428,13 +4421,11 @@ class StorageManagerService extends IStorageManager.Stub
     }
 
     class MountObbAction extends ObbAction {
-        private final String mKey;
         private final int mCallingUid;
         private ObbInfo mObbInfo;
 
-        MountObbAction(ObbState obbState, String key, int callingUid, ObbInfo obbInfo) {
+        MountObbAction(ObbState obbState, int callingUid, ObbInfo obbInfo) {
             super(obbState);
-            mKey = key;
             mCallingUid = callingUid;
             mObbInfo = obbInfo;
         }
@@ -4457,29 +4448,8 @@ class StorageManagerService extends IStorageManager.Stub
                         "Attempt to mount OBB which is already mounted: " + mObbInfo.filename);
             }
 
-            final String hashedKey;
-            final String binderKey;
-            if (mKey == null) {
-                hashedKey = "none";
-                binderKey = "";
-            } else {
-                try {
-                    SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
-
-                    KeySpec ks = new PBEKeySpec(mKey.toCharArray(), mObbInfo.salt,
-                            PBKDF2_HASH_ROUNDS, CRYPTO_ALGORITHM_KEY_SIZE);
-                    SecretKey key = factory.generateSecret(ks);
-                    BigInteger bi = new BigInteger(key.getEncoded());
-                    hashedKey = bi.toString(16);
-                    binderKey = hashedKey;
-                } catch (GeneralSecurityException e) {
-                    throw new ObbException(ERROR_INTERNAL, e);
-                }
-            }
-
             try {
-                mObbState.volId = mVold.createObb(mObbState.canonicalPath, binderKey,
-                        mObbState.ownerGid);
+                mObbState.volId = mVold.createObb(mObbState.canonicalPath, mObbState.ownerGid);
                 mVold.mount(mObbState.volId, 0, -1, null);
 
                 if (DEBUG_OBB)