Remove broken code for mounting encrypted OBB files
Mounting encrypted OBB files has never worked reliably across devices,
partly due to its reliance on Twofish encryption support in the kernel.
This is because Twofish support (CONFIG_CRYPTO_TWOFISH) has never been
required or even recommended for Android. It has never been enabled in
GKI, but even before GKI it wasn't required or recommended. Moreover,
this is now the only Android feature that still uses dm-crypt
(CONFIG_DM_CRYPT), and some devices don't have that enabled either.
Therefore, it appears that this feature is unused. That's perhaps not
surprising, considering that the documentation for OBBs
(https://developer.android.com/google/play/expansion-files) says that
they are deprecated, and also it explains OBBs as being app files that
are opaque to the platform; the ability of the platform to mount OBBs
that happen to be in a particular format is never mentioned. That means
that OBB mounting is probably rarely used even with unencrypted OBBs.
Finally, the usefulness of OBBs having their own encryption layer (in
addition to what the platform already provides via FBE) is not clear
either, especially with such an unusual choice of cipher.
To avoid the confusion that is being caused by having the broken code
for mounting encrypted OBBs still sitting around, let's remove it.
Test: atest StorageManagerTest # on Cuttlefish
Test: atest StorageManagerIntegrationTest # on Cuttlefish
Bug: 216475849
Change-Id: I6e6a6462ab8343299dc5e0145b87dc28b16b0bc1
diff --git a/core/java/android/os/storage/IStorageManager.aidl b/core/java/android/os/storage/IStorageManager.aidl
index 9385402..6c0a1f9 100644
--- a/core/java/android/os/storage/IStorageManager.aidl
+++ b/core/java/android/os/storage/IStorageManager.aidl
@@ -54,13 +54,13 @@
*/
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 77c794c..39f87d5 100644
--- a/core/java/android/os/storage/StorageManager.java
+++ b/core/java/android/os/storage/StorageManager.java
@@ -665,9 +665,7 @@
}
/**
- * 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 @@
* 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 373b8e4..0000000
--- a/core/tests/coretests/res/raw/obb_enc_file100_orig1.obb
+++ /dev/null
Binary files differ
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 aa531d8..0000000
--- a/core/tests/coretests/res/raw/obb_enc_file100_orig3.obb
+++ /dev/null
Binary files differ
diff --git a/core/tests/coretests/src/android/os/storage/StorageManagerBaseTest.java b/core/tests/coretests/src/android/os/storage/StorageManagerBaseTest.java
index 16dcff5..e56c0ad 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 @@
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 @@
* 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 @@
* @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 @@
* 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 62f2ac2..ecd2f76 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 @@
}
/**
- * 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 @@
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 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 @@
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 fd6e6e9..055dbb2 100644
--- a/libs/storage/IMountService.cpp
+++ b/libs/storage/IMountService.cpp
@@ -442,14 +442,13 @@
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 2463e02..5b07318 100644
--- a/libs/storage/include/storage/IMountService.h
+++ b/libs/storage/include/storage/IMountService.h
@@ -64,8 +64,8 @@
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 2272525..9e0a6eb 100644
--- a/native/android/storage_manager.cpp
+++ b/native/android/storage_manager.cpp
@@ -140,8 +140,7 @@
}
}
- 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 @@
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_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 9546496..9266bb4 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.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.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 @@
}
@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 @@
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 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 @@
"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)