drgreen/android_frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove broken code for mounting encrypted OBB files

Browse Source 
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
This commit is contained in:
Eric Biggers 2022-03-01 21:19:10 +00:00
parent 7ee20f2830
commit 8bc9340b4c
10 changed files with 41 additions and 129 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
core/java/android/os/storage/IStorageManager.aidl
View File

@ -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.

13
core/java/android/os/storage/StorageManager.java
View File

@ -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) {

BIN
core/tests/coretests/res/raw/obb_enc_file100_orig1.obb
View File

Binary file not shown.

BIN
core/tests/coretests/res/raw/obb_enc_file100_orig3.obb
View File

Binary file not shown.

26
core/tests/coretests/src/android/os/storage/StorageManagerBaseTest.java
View File

@ -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,

59
core/tests/coretests/src/android/os/storage/StorageManagerIntegrationTest.java
View File

@ -82,58 +82,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.
*/
@ -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);
}

3
libs/storage/IMountService.cpp
View File

@ -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);

4
libs/storage/include/storage/IMountService.h
View File

@ -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;

13
native/android/storage_manager.cpp
View File

@ -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,

40
services/core/java/com/android/server/StorageManagerService.java
View File

@ -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)