WifiConfigStore: Limit integrity checks to single user devices
Bug: 138482990
Test: atest com.android.server.wifi
Test: Device boots up & verified that config store files are not
integrity protected.
Test: Will send for full wifi regression tests.
Change-Id: I1bf8ae320935cc1bf70625792c4c1a5e0d54f034
Merged-In: I1bf8ae320935cc1bf70625792c4c1a5e0d54f034
(cherry picked from commit 5be4058e19d5b5244224c6d8580b801567cb9ac7)
diff --git a/service/java/com/android/server/wifi/WifiConfigManager.java b/service/java/com/android/server/wifi/WifiConfigManager.java
index 1d287f0..0c175ac 100644
--- a/service/java/com/android/server/wifi/WifiConfigManager.java
+++ b/service/java/com/android/server/wifi/WifiConfigManager.java
@@ -3088,7 +3088,7 @@
if (mDeferredUserUnlockRead) {
Log.i(TAG, "Handling user unlock before loading from store.");
List<WifiConfigStore.StoreFile> userStoreFiles =
- WifiConfigStore.createUserFiles(mCurrentUserId);
+ WifiConfigStore.createUserFiles(mCurrentUserId, UserManager.get(mContext));
if (userStoreFiles == null) {
Log.wtf(TAG, "Failed to create user store files");
return false;
@@ -3127,7 +3127,7 @@
private boolean loadFromUserStoreAfterUnlockOrSwitch(int userId) {
try {
List<WifiConfigStore.StoreFile> userStoreFiles =
- WifiConfigStore.createUserFiles(userId);
+ WifiConfigStore.createUserFiles(userId, UserManager.get(mContext));
if (userStoreFiles == null) {
Log.e(TAG, "Failed to create user store files");
return false;
diff --git a/service/java/com/android/server/wifi/WifiConfigStore.java b/service/java/com/android/server/wifi/WifiConfigStore.java
index a618eb5..6989e72 100644
--- a/service/java/com/android/server/wifi/WifiConfigStore.java
+++ b/service/java/com/android/server/wifi/WifiConfigStore.java
@@ -27,6 +27,7 @@
import android.os.FileUtils;
import android.os.Handler;
import android.os.Looper;
+import android.os.UserManager;
import android.util.Log;
import android.util.SparseArray;
import android.util.Xml;
@@ -208,7 +209,7 @@
* @param clock clock instance to retrieve timestamps for alarms.
* @param wifiMetrics Metrics instance.
* @param sharedStore StoreFile instance pointing to the shared store file. This should
- * be retrieved using {@link #createSharedFile()} method.
+ * be retrieved using {@link #createSharedFile(UserManager)} method.
*/
public WifiConfigStore(Context context, Looper looper, Clock clock, WifiMetrics wifiMetrics,
StoreFile sharedStore) {
@@ -229,7 +230,8 @@
/**
* Set the user store files.
* (Useful for mocking in unit tests).
- * @param userStores List of {@link StoreFile} created using {@link #createUserFiles(int)}.
+ * @param userStores List of {@link StoreFile} created using {@link #createUserFiles(int,
+ * UserManager)}.
*/
public void setUserStores(@NonNull List<StoreFile> userStores) {
Preconditions.checkNotNull(userStores);
@@ -266,9 +268,11 @@
* @param storeBaseDir Base directory under which the store file is to be stored. The store file
* will be at <storeBaseDir>/wifi/WifiConfigStore.xml.
* @param fileId Identifier for the file. See {@link StoreFileId}.
+ * @param userManager Instance of UserManager to check if the device is in single user mode.
* @return new instance of the store file or null if the directory cannot be created.
*/
- private static @Nullable StoreFile createFile(File storeBaseDir, @StoreFileId int fileId) {
+ private static @Nullable StoreFile createFile(File storeBaseDir, @StoreFileId int fileId,
+ UserManager userManager) {
File storeDir = new File(storeBaseDir, STORE_DIRECTORY_NAME);
if (!storeDir.exists()) {
if (!storeDir.mkdir()) {
@@ -276,16 +280,24 @@
return null;
}
}
- return new StoreFile(new File(storeDir, STORE_ID_TO_FILE_NAME.get(fileId)), fileId);
+ File file = new File(storeDir, STORE_ID_TO_FILE_NAME.get(fileId));
+ DataIntegrityChecker dataIntegrityChecker = null;
+ // Turn on integrity checking only for single user mode devices.
+ if (userManager.hasUserRestriction(UserManager.DISALLOW_ADD_USER)) {
+ dataIntegrityChecker = new DataIntegrityChecker(file.getAbsolutePath());
+ }
+ return new StoreFile(file, fileId, dataIntegrityChecker);
}
/**
* Create a new instance of the shared store file.
*
+ * @param userManager Instance of UserManager to check if the device is in single user mode.
* @return new instance of the store file or null if the directory cannot be created.
*/
- public static @Nullable StoreFile createSharedFile() {
- return createFile(Environment.getDataMiscDirectory(), STORE_FILE_SHARED_GENERAL);
+ public static @Nullable StoreFile createSharedFile(UserManager userManager) {
+ return createFile(
+ Environment.getDataMiscDirectory(), STORE_FILE_SHARED_GENERAL, userManager);
}
/**
@@ -293,14 +305,16 @@
* The user store file is inside the user's encrypted data directory.
*
* @param userId userId corresponding to the currently logged-in user.
+ * @param userManager Instance of UserManager to check if the device is in single user mode.
* @return List of new instances of the store files created or null if the directory cannot be
* created.
*/
- public static @Nullable List<StoreFile> createUserFiles(int userId) {
+ public static @Nullable List<StoreFile> createUserFiles(int userId, UserManager userManager) {
List<StoreFile> storeFiles = new ArrayList<>();
for (int fileId : Arrays.asList(
STORE_FILE_USER_GENERAL, STORE_FILE_USER_NETWORK_SUGGESTIONS)) {
- StoreFile storeFile = createFile(Environment.getDataMiscCeDirectory(userId), fileId);
+ StoreFile storeFile =
+ createFile(Environment.getDataMiscCeDirectory(userId), fileId, userManager);
if (storeFile == null) {
return null;
}
@@ -502,7 +516,8 @@
* Handles a user switch. This method changes the user specific store files and reads from the
* new user's store files.
*
- * @param userStores List of {@link StoreFile} created using {@link #createUserFiles(int)}.
+ * @param userStores List of {@link StoreFile} created using {@link #createUserFiles(int,
+ * UserManager)}.
*/
public void switchUserStoresAndRead(@NonNull List<StoreFile> userStores)
throws XmlPullParserException, IOException {
@@ -655,19 +670,21 @@
*/
private String mFileName;
/**
- * The integrity file storing integrity checking data for the store file.
- */
- private DataIntegrityChecker mDataIntegrityChecker;
- /**
* {@link StoreFileId} Type of store file.
*/
private @StoreFileId int mFileId;
+ /**
+ * The integrity file storing integrity checking data for the store file.
+ * Note: This is only turned on for single user devices.
+ */
+ private @Nullable DataIntegrityChecker mDataIntegrityChecker;
- public StoreFile(File file, @StoreFileId int fileId) {
+ public StoreFile(File file, @StoreFileId int fileId,
+ @Nullable DataIntegrityChecker dataIntegrityChecker) {
mAtomicFile = new AtomicFile(file);
mFileName = mAtomicFile.getBaseFile().getAbsolutePath();
- mDataIntegrityChecker = new DataIntegrityChecker(mFileName);
mFileId = fileId;
+ mDataIntegrityChecker = dataIntegrityChecker;
}
/**
@@ -679,6 +696,7 @@
return mAtomicFile.exists();
}
+
/**
* Read the entire raw data from the store file and return in a byte array.
*
@@ -691,20 +709,24 @@
byte[] bytes = null;
try {
bytes = mAtomicFile.readFully();
- // Check that the file has not been altered since last writeBufferedRawData()
- if (!mDataIntegrityChecker.isOk(bytes)) {
- Log.wtf(TAG, "Data integrity problem with file: " + mFileName);
- return null;
- }
} catch (FileNotFoundException e) {
return null;
- } catch (DigestException e) {
- // When integrity checking is introduced. The existing data will have no related
- // integrity file for validation. Thus, we will assume the existing data is correct
- // and immediately create the integrity file.
- Log.i(TAG, "isOK() had no integrity data to check; thus vacuously "
- + "true. Running update now.");
- mDataIntegrityChecker.update(bytes);
+ }
+ if (mDataIntegrityChecker != null) {
+ // Check that the file has not been altered since last writeBufferedRawData()
+ try {
+ if (!mDataIntegrityChecker.isOk(bytes)) {
+ Log.wtf(TAG, "Data integrity problem with file: " + mFileName);
+ return null;
+ }
+ } catch (DigestException e) {
+ // When integrity checking is introduced. The existing data will have no
+ // related integrity file for validation. Thus, we will assume the existing
+ // data is correct and immediately create the integrity file.
+ Log.i(TAG, "isOK() had no integrity data to check; thus vacuously "
+ + "true. Running update now.");
+ mDataIntegrityChecker.update(bytes);
+ }
}
return bytes;
}
@@ -741,8 +763,10 @@
}
throw e;
}
- // There was a legitimate change and update the integrity checker.
- mDataIntegrityChecker.update(mWriteData);
+ if (mDataIntegrityChecker != null) {
+ // There was a legitimate change and update the integrity checker.
+ mDataIntegrityChecker.update(mWriteData);
+ }
// Reset the pending write data after write.
mWriteData = null;
}
diff --git a/service/java/com/android/server/wifi/WifiInjector.java b/service/java/com/android/server/wifi/WifiInjector.java
index 178c98f..474f27c 100644
--- a/service/java/com/android/server/wifi/WifiInjector.java
+++ b/service/java/com/android/server/wifi/WifiInjector.java
@@ -239,7 +239,7 @@
mWifiKeyStore = new WifiKeyStore(mKeyStore);
mWifiConfigStore = new WifiConfigStore(
mContext, clientModeImplLooper, mClock, mWifiMetrics,
- WifiConfigStore.createSharedFile());
+ WifiConfigStore.createSharedFile(UserManager.get(mContext)));
SubscriptionManager subscriptionManager =
mContext.getSystemService(SubscriptionManager.class);
// Config Manager
diff --git a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java
index 3b86be8..9d5ed04 100644
--- a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java
@@ -228,7 +228,8 @@
mSession = ExtendedMockito.mockitoSession()
.mockStatic(WifiConfigStore.class, withSettings().lenient())
.startMocking();
- when(WifiConfigStore.createUserFiles(anyInt())).thenReturn(mock(List.class));
+ when(WifiConfigStore.createUserFiles(anyInt(), any(UserManager.class)))
+ .thenReturn(mock(List.class));
when(mTelephonyManager.createForSubscriptionId(anyInt())).thenReturn(mDataTelephonyManager);
}
diff --git a/tests/wifitests/src/com/android/server/wifi/WifiConfigStoreTest.java b/tests/wifitests/src/com/android/server/wifi/WifiConfigStoreTest.java
index 64e762b..6fdcce8 100644
--- a/tests/wifitests/src/com/android/server/wifi/WifiConfigStoreTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/WifiConfigStoreTest.java
@@ -31,6 +31,7 @@
import com.android.internal.util.ArrayUtils;
import com.android.server.wifi.WifiConfigStore.StoreData;
import com.android.server.wifi.WifiConfigStore.StoreFile;
+import com.android.server.wifi.util.DataIntegrityChecker;
import com.android.server.wifi.util.XmlUtil;
import org.junit.After;
@@ -147,6 +148,7 @@
private TestLooper mLooper;
@Mock private Clock mClock;
@Mock private WifiMetrics mWifiMetrics;
+ @Mock private DataIntegrityChecker mDataIntegrityChecker;
private MockStoreFile mSharedStore;
private MockStoreFile mUserStore;
private MockStoreFile mUserNetworkSuggestionsStore;
@@ -379,6 +381,48 @@
}
/**
+ * Tests the read API behaviour after a write to the store files (with no integrity checks).
+ * Expected behaviour: The read should return the same data that was last written.
+ */
+ @Test
+ public void testReadAfterWriteWithNoIntegrityCheck() throws Exception {
+ // Recreate the mock store files with no data integrity checking.
+ mUserStores.clear();
+ mSharedStore = new MockStoreFile(WifiConfigStore.STORE_FILE_SHARED_GENERAL, null);
+ mUserStore = new MockStoreFile(WifiConfigStore.STORE_FILE_USER_GENERAL, null);
+ mUserNetworkSuggestionsStore =
+ new MockStoreFile(WifiConfigStore.STORE_FILE_USER_NETWORK_SUGGESTIONS, null);
+ mUserStores.add(mUserStore);
+ mUserStores.add(mUserNetworkSuggestionsStore);
+ mWifiConfigStore = new WifiConfigStore(mContext, mLooper.getLooper(), mClock, mWifiMetrics,
+ mSharedStore);
+
+ // Register data container.
+ mWifiConfigStore.registerStoreData(mSharedStoreData);
+ mWifiConfigStore.registerStoreData(mUserStoreData);
+
+ // Read both share and user config store.
+ mWifiConfigStore.switchUserStoresAndRead(mUserStores);
+
+ // Verify no data is read.
+ assertNull(mUserStoreData.getData());
+ assertNull(mSharedStoreData.getData());
+
+ // Write share and user data.
+ mUserStoreData.setData(TEST_USER_DATA);
+ mSharedStoreData.setData(TEST_SHARE_DATA);
+ mWifiConfigStore.write(true);
+
+ // Read and verify the data content in the data container.
+ mWifiConfigStore.read();
+ assertEquals(TEST_USER_DATA, mUserStoreData.getData());
+ assertEquals(TEST_SHARE_DATA, mSharedStoreData.getData());
+
+ verify(mWifiMetrics, times(2)).noteWifiConfigStoreReadDuration(anyInt());
+ verify(mWifiMetrics).noteWifiConfigStoreWriteDuration(anyInt());
+ }
+
+ /**
* Tests the read API behaviour when the shared store file is empty and the user store
* is not yet visible (user not yet unlocked).
* Expected behaviour: The read should return an empty store data instance when the file not
@@ -761,7 +805,12 @@
private boolean mStoreWritten;
MockStoreFile(@WifiConfigStore.StoreFileId int fileId) {
- super(new File("MockStoreFile"), fileId);
+ super(new File("MockStoreFile"), fileId, mDataIntegrityChecker);
+ }
+
+ MockStoreFile(@WifiConfigStore.StoreFileId int fileId,
+ DataIntegrityChecker dataIntegrityChecker) {
+ super(new File("MockStoreFile"), fileId, dataIntegrityChecker);
}
@Override
