From ecffe3ecbf4cb01055bd2f852d95396f2475fc01 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 13 Aug 2021 13:37:55 -0700 Subject: [PATCH 1/5] [RESTRICT AUTOMERGE] StorageManagerService: don't ignore failures to prepare user storage We must never leave directories unencrypted. Bug: 164488924 Bug: 224585613 Change-Id: I9a38ab5cca1ae9c9ebff81fca04615fd83ebe4b2 (cherry picked from commit 50946dd15fd14cbf92b5c7e32ac7a0f088b8b302) Merged-In: I9a38ab5cca1ae9c9ebff81fca04615fd83ebe4b2 --- .../core/java/com/android/server/StorageManagerService.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 678387c540edc..ebe49cbdf257a 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -3380,8 +3380,12 @@ class StorageManagerService extends IStorageManager.Stub mInstaller.tryMountDataMirror(volumeUuid); } } - } catch (Exception e) { + } catch (RemoteException | Installer.InstallerException e) { Slog.wtf(TAG, e); + // Make sure to re-throw this exception; we must not ignore failure + // to prepare the user storage as it could indicate that encryption + // wasn't successfully set up. + throw new RuntimeException(e); } } From 69c3ce70c6dcabf57219d338af86e569ea672ef5 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 24 Jan 2022 20:33:11 +0000 Subject: [PATCH 2/5] [RESTRICT AUTOMERGE] UserDataPreparer: reboot to recovery if preparing user storage fails StorageManager.prepareUserStorage() can throw an exception if a directory cannot be encrypted, for example due to already being nonempty. In this case, usage of the directory must not be allowed to proceed. UserDataPreparer currently handles this by deleting the user's directories, but the error is still ultimately suppressed and starting the user is still allowed to proceed. The correct behavior in this case is to reboot into recovery to ask the user to factory reset the device. This is already what happens when 'init' fails to encrypt a directory with the system DE policy. However, this was overlooked for the user directories. Start doing this. Bug: 164488924 Bug: 224585613 Change-Id: Ib5e91d2510b25780d7a161b91b5cee2f6f7a2e54 (cherry picked from commit 5256365e65882b81509ec2f6b9dfe2dcf0025254) Merged-In: Ib5e91d2510b25780d7a161b91b5cee2f6f7a2e54 --- .../core/java/com/android/server/pm/UserDataPreparer.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/services/core/java/com/android/server/pm/UserDataPreparer.java b/services/core/java/com/android/server/pm/UserDataPreparer.java index 045a295da965f..504769064808c 100644 --- a/services/core/java/com/android/server/pm/UserDataPreparer.java +++ b/services/core/java/com/android/server/pm/UserDataPreparer.java @@ -22,6 +22,7 @@ import android.content.Context; import android.content.pm.UserInfo; import android.os.Environment; import android.os.FileUtils; +import android.os.RecoverySystem; import android.os.storage.StorageManager; import android.os.storage.VolumeInfo; import android.os.SystemProperties; @@ -115,6 +116,13 @@ class UserDataPreparer { // Try one last time; if we fail again we're really in trouble prepareUserDataLI(volumeUuid, userId, userSerial, flags | StorageManager.FLAG_STORAGE_DE, false); + } else { + try { + Log.e(TAG, "prepareUserData failed", e); + RecoverySystem.rebootPromptAndWipeUserData(mContext, "prepareUserData failed"); + } catch (IOException e2) { + throw new RuntimeException("error rebooting into recovery", e2); + } } } } From ecf569bd1623231984e9ec9823edb82f52d7846a Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 4 Mar 2022 00:07:29 +0000 Subject: [PATCH 3/5] [RESTRICT AUTOMERGE] UserDataPreparer: reboot to recovery for system user only With the next CL, old devices might contain a combination of old users with prepareUserStorage error checking disabled and new users with prepareUserStorage error checking enabled. Factory resetting the whole device when any user fails to prepare may be too aggressive. Also, UserDataPreparer already destroys the affected user's storage when it fails to prepare, which seems to be fairly effective at breaking things for that user (absent proper error handling by upper layers). Therefore, let's only factory reset the device if the failing user is the system user. Bug: 164488924 Bug: 224585613 Change-Id: Ia1db01ab4ec6b3b17d725f391c3500d92aa00f97 (cherry picked from commit 4c76da76c9831266e4e63c0618150bed10a929a7) Merged-In: Ia1db01ab4ec6b3b17d725f391c3500d92aa00f97 --- .../core/java/com/android/server/pm/UserDataPreparer.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/pm/UserDataPreparer.java b/services/core/java/com/android/server/pm/UserDataPreparer.java index 504769064808c..95482d7c7f1aa 100644 --- a/services/core/java/com/android/server/pm/UserDataPreparer.java +++ b/services/core/java/com/android/server/pm/UserDataPreparer.java @@ -118,8 +118,11 @@ class UserDataPreparer { flags | StorageManager.FLAG_STORAGE_DE, false); } else { try { - Log.e(TAG, "prepareUserData failed", e); - RecoverySystem.rebootPromptAndWipeUserData(mContext, "prepareUserData failed"); + Log.wtf(TAG, "prepareUserData failed for user " + userId, e); + if (userId == UserHandle.USER_SYSTEM) { + RecoverySystem.rebootPromptAndWipeUserData(mContext, + "prepareUserData failed for system user"); + } } catch (IOException e2) { throw new RuntimeException("error rebooting into recovery", e2); } From e03e987337accde646e4e86c1fdfe02c0d78d743 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 4 Mar 2022 00:07:43 +0000 Subject: [PATCH 4/5] [RESTRICT AUTOMERGE] Ignore errors preparing user storage for existing users Unfortunately we can't rule out the existence of devices where the user storage wasn't properly prepared, due to StorageManagerService previously ignoring errors from mVold.prepareUserStorage, combined with OEMs potentially creating files in per-user directories too early. And forcing these broken devices to be factory reset upon taking an OTA is not currently considered to be acceptable. One option is to only check for prepareUserStorage errors on devices that launched with T or later. However, this is a serious issue and it would be strongly preferable to do more than that. Therefore, this CL makes it so that errors are checked for all new users, rather than all new devices. A field ignorePrepareStorageErrors is added to the user record; it is only ever set to true implicitly, when reading a user record from disk that lacks this field. This field is used by StorageManagerService to decide whether to check for errors. Bug: 164488924 Bug: 224585613 Test: Intentionally made a device affected by this issue by reverting the CLs that introduced the error checks, and changing vold to inject an error into prepareUserStorage. Then, flashed a build with this CL without wiping userdata. The device still boots, as expected, and the log shows that the error was intentionally ignored. Tested that if a second user is added, the error is *not* ignored and the second user's storage is destroyed before it can be used. Finally, wiped the device and verified that it won't boot up anymore, as expected since error checking is enabled for the system user in that case. Change-Id: I9bdd1a4bf5b14542adb901f264a91d489115c89b (cherry picked from commit 60d8318c47b7b659716d71243d087b34ab327f64) Merged-In: I9bdd1a4bf5b14542adb901f264a91d489115c89b --- .../java/android/os/UserManagerInternal.java | 8 ++++ .../android/server/StorageManagerService.java | 11 ++++- .../android/server/pm/UserManagerService.java | 42 +++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/services/core/java/android/os/UserManagerInternal.java b/services/core/java/android/os/UserManagerInternal.java index fbe8c04bd59cd..8361e0fbf0f6a 100644 --- a/services/core/java/android/os/UserManagerInternal.java +++ b/services/core/java/android/os/UserManagerInternal.java @@ -271,4 +271,12 @@ public abstract class UserManagerInternal { * Gets all {@link UserInfo UserInfos}. */ public abstract @NonNull UserInfo[] getUserInfos(); + + /** + * Returns {@code true} if the system should ignore errors when preparing + * the storage directories for the user with ID {@code userId}. This will + * return {@code false} for all new users; it will only return {@code true} + * for users that already existed on-disk from an older version of Android. + */ + public abstract boolean shouldIgnorePrepareStorageErrors(int userId); } diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index ebe49cbdf257a..64aa443bdda2a 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -3380,11 +3380,20 @@ class StorageManagerService extends IStorageManager.Stub mInstaller.tryMountDataMirror(volumeUuid); } } - } catch (RemoteException | Installer.InstallerException e) { + } catch (Exception e) { Slog.wtf(TAG, e); // Make sure to re-throw this exception; we must not ignore failure // to prepare the user storage as it could indicate that encryption // wasn't successfully set up. + // + // Very unfortunately, these errors need to be ignored for broken + // users that already existed on-disk from older Android versions. + UserManagerInternal umInternal = LocalServices.getService(UserManagerInternal.class); + if (umInternal.shouldIgnorePrepareStorageErrors(userId)) { + Slog.wtf(TAG, "ignoring error preparing storage for existing user " + userId + + "; device may be insecure!"); + return; + } throw new RuntimeException(e); } } diff --git a/services/core/java/com/android/server/pm/UserManagerService.java b/services/core/java/com/android/server/pm/UserManagerService.java index 27924a68ff485..66fc608ead4a7 100644 --- a/services/core/java/com/android/server/pm/UserManagerService.java +++ b/services/core/java/com/android/server/pm/UserManagerService.java @@ -197,6 +197,8 @@ public class UserManagerService extends IUserManager.Stub { private static final String TAG_SEED_ACCOUNT_OPTIONS = "seedAccountOptions"; private static final String TAG_LAST_REQUEST_QUIET_MODE_ENABLED_CALL = "lastRequestQuietModeEnabledCall"; + private static final String TAG_IGNORE_PREPARE_STORAGE_ERRORS = + "ignorePrepareStorageErrors"; private static final String ATTR_KEY = "key"; private static final String ATTR_VALUE_TYPE = "type"; private static final String ATTR_MULTIPLE = "m"; @@ -297,6 +299,14 @@ public class UserManagerService extends IUserManager.Stub { private long mLastRequestQuietModeEnabledMillis; + /** + * {@code true} if the system should ignore errors when preparing the + * storage directories for this user. This is {@code false} for all new + * users; it will only be {@code true} for users that already existed + * on-disk from an older version of Android. + */ + private boolean mIgnorePrepareStorageErrors; + void setLastRequestQuietModeEnabledMillis(long millis) { mLastRequestQuietModeEnabledMillis = millis; } @@ -305,6 +315,14 @@ public class UserManagerService extends IUserManager.Stub { return mLastRequestQuietModeEnabledMillis; } + boolean getIgnorePrepareStorageErrors() { + return mIgnorePrepareStorageErrors; + } + + void setIgnorePrepareStorageErrors() { + mIgnorePrepareStorageErrors = true; + } + void clearSeedAccountData() { seedAccountName = null; seedAccountType = null; @@ -2928,6 +2946,10 @@ public class UserManagerService extends IUserManager.Stub { serializer.endTag(/* namespace */ null, TAG_LAST_REQUEST_QUIET_MODE_ENABLED_CALL); } + serializer.startTag(/* namespace */ null, TAG_IGNORE_PREPARE_STORAGE_ERRORS); + serializer.text(String.valueOf(userData.getIgnorePrepareStorageErrors())); + serializer.endTag(/* namespace */ null, TAG_IGNORE_PREPARE_STORAGE_ERRORS); + serializer.endTag(null, TAG_USER); serializer.endDocument(); @@ -3039,6 +3061,7 @@ public class UserManagerService extends IUserManager.Stub { Bundle legacyLocalRestrictions = null; RestrictionsSet localRestrictions = null; Bundle globalRestrictions = null; + boolean ignorePrepareStorageErrors = true; // default is true for old users XmlPullParser parser = Xml.newPullParser(); parser.setInput(is, StandardCharsets.UTF_8.name()); @@ -3126,6 +3149,11 @@ public class UserManagerService extends IUserManager.Stub { if (type == XmlPullParser.TEXT) { lastRequestQuietModeEnabledTimestamp = Long.parseLong(parser.getText()); } + } else if (TAG_IGNORE_PREPARE_STORAGE_ERRORS.equals(tag)) { + type = parser.next(); + if (type == XmlPullParser.TEXT) { + ignorePrepareStorageErrors = Boolean.parseBoolean(parser.getText()); + } } } } @@ -3152,6 +3180,9 @@ public class UserManagerService extends IUserManager.Stub { userData.persistSeedData = persistSeedData; userData.seedAccountOptions = seedAccountOptions; userData.setLastRequestQuietModeEnabledMillis(lastRequestQuietModeEnabledTimestamp); + if (ignorePrepareStorageErrors) { + userData.setIgnorePrepareStorageErrors(); + } synchronized (mRestrictionsLock) { if (baseRestrictions != null) { @@ -4771,6 +4802,9 @@ public class UserManagerService extends IUserManager.Stub { pw.println(); } } + + pw.println(" Ignore errors preparing storage: " + + userData.getIgnorePrepareStorageErrors()); } } pw.println(); @@ -5183,6 +5217,14 @@ public class UserManagerService extends IUserManager.Stub { return allInfos; } } + + @Override + public boolean shouldIgnorePrepareStorageErrors(int userId) { + synchronized (mUsersLock) { + UserData userData = mUsers.get(userId); + return userData != null && userData.getIgnorePrepareStorageErrors(); + } + } } /** From 0762961674f1454b7c7012a0ab53c427570e836c Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Sat, 26 Mar 2022 01:08:07 +0000 Subject: [PATCH 5/5] [RESTRICT AUTOMERGE] Log to EventLog on prepareUserStorage failure Bug: 224585613 Change-Id: Id6dfb4f4c48d5cf4e71f54bdb6d0d6eea527caf5 (cherry picked from commit fbb632ea95ac5b6d9efa89e09d0988a9df4f19e4) Merged-In: Id6dfb4f4c48d5cf4e71f54bdb6d0d6eea527caf5 --- .../core/java/com/android/server/StorageManagerService.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 64aa443bdda2a..eb65cac43c628 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -131,6 +131,7 @@ import android.util.ArrayMap; import android.util.ArraySet; import android.util.AtomicFile; import android.util.DataUnit; +import android.util.EventLog; import android.util.FeatureFlagUtils; import android.util.Log; import android.util.Pair; @@ -3381,6 +3382,7 @@ class StorageManagerService extends IStorageManager.Stub } } } catch (Exception e) { + EventLog.writeEvent(0x534e4554, "224585613", -1, ""); Slog.wtf(TAG, e); // Make sure to re-throw this exception; we must not ignore failure // to prepare the user storage as it could indicate that encryption