Fix security vulnerability that creates user with no restrictions when accountOptions are too long.
Bug: 293602970
Test: atest UserManagerTest#testAddUserAccountData_validStringValuesAreSaved_validBundleIsSaved && atest UserManagerTest#testAddUserAccountData_invalidStringValuesAreTruncated_invalidBundleIsDropped
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:8dc6feaee7c0a5cea093b5280acaad862921cf3e)
Merged-In: I23c971f671546ac085060add89485cfac6691ca3
Change-Id: I23c971f671546ac085060add89485cfac6691ca3
diff --git a/core/java/android/os/PersistableBundle.java b/core/java/android/os/PersistableBundle.java
index acfd15c..7915507 100644
--- a/core/java/android/os/PersistableBundle.java
+++ b/core/java/android/os/PersistableBundle.java
@@ -294,6 +294,43 @@
XmlUtils.writeMapXml(mMap, out, this);
}
+ /**
+ * Checks whether all keys and values are within the given character limit.
+ * Note: Maximum character limit of String that can be saved to XML as part of bundle is 65535.
+ * Otherwise IOException is thrown.
+ * @param limit length of String keys and values in the PersistableBundle, including nested
+ * PersistableBundles to check against.
+ *
+ * @hide
+ */
+ public boolean isBundleContentsWithinLengthLimit(int limit) {
+ unparcel();
+ if (mMap == null) {
+ return true;
+ }
+ for (int i = 0; i < mMap.size(); i++) {
+ if (mMap.keyAt(i) != null && mMap.keyAt(i).length() > limit) {
+ return false;
+ }
+ final Object value = mMap.valueAt(i);
+ if (value instanceof String && ((String) value).length() > limit) {
+ return false;
+ } else if (value instanceof String[]) {
+ String[] stringArray = (String[]) value;
+ for (int j = 0; j < stringArray.length; j++) {
+ if (stringArray[j] != null
+ && stringArray[j].length() > limit) {
+ return false;
+ }
+ }
+ } else if (value instanceof PersistableBundle
+ && !((PersistableBundle) value).isBundleContentsWithinLengthLimit(limit)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
/** @hide */
static class MyReadMapCallback implements XmlUtils.ReadMapCallback {
@Override
diff --git a/core/java/android/os/UserManager.java b/core/java/android/os/UserManager.java
index 0ffdfc6..00b8f9b 100644
--- a/core/java/android/os/UserManager.java
+++ b/core/java/android/os/UserManager.java
@@ -101,6 +101,21 @@
/** The userType of UserHandle.myUserId(); empty string if not a profile; null until cached. */
private String mProfileTypeOfProcessUser = null;
+ /** Maximum length of username.
+ * @hide
+ */
+ public static final int MAX_USER_NAME_LENGTH = 100;
+
+ /** Maximum length of user property String value.
+ * @hide
+ */
+ public static final int MAX_ACCOUNT_STRING_LENGTH = 500;
+
+ /** Maximum length of account options String values.
+ * @hide
+ */
+ public static final int MAX_ACCOUNT_OPTIONS_LENGTH = 1000;
+
/**
* User type representing a {@link UserHandle#USER_SYSTEM system} user that is a human user.
* This type of user cannot be created; it can only pre-exist on first boot.
@@ -3633,15 +3648,15 @@
* time, the preferred user name and account information are used by the setup process for that
* user.
*
- * @param userName Optional name to assign to the user.
+ * @param userName Optional name to assign to the user. Character limit is 100.
* @param accountName Optional account name that will be used by the setup wizard to initialize
- * the user.
+ * the user. Character limit is 500.
* @param accountType Optional account type for the account to be created. This is required
- * if the account name is specified.
+ * if the account name is specified. Character limit is 500.
* @param accountOptions Optional bundle of data to be passed in during account creation in the
* new user via {@link AccountManager#addAccount(String, String, String[],
* Bundle, android.app.Activity, android.accounts.AccountManagerCallback,
- * Handler)}.
+ * Handler)}. Character limit is 1000.
* @return An Intent that can be launched from an Activity.
* @see #USER_CREATION_FAILED_NOT_PERMITTED
* @see #USER_CREATION_FAILED_NO_MORE_USERS
diff --git a/core/java/com/android/internal/app/ConfirmUserCreationActivity.java b/core/java/com/android/internal/app/ConfirmUserCreationActivity.java
index ee4d46d..86aa0df 100644
--- a/core/java/com/android/internal/app/ConfirmUserCreationActivity.java
+++ b/core/java/com/android/internal/app/ConfirmUserCreationActivity.java
@@ -114,6 +114,14 @@
if (cantCreateUser) {
setResult(UserManager.USER_CREATION_FAILED_NOT_PERMITTED);
return null;
+ } else if (!(isUserPropertyWithinLimit(mUserName, UserManager.MAX_USER_NAME_LENGTH)
+ && isUserPropertyWithinLimit(mAccountName, UserManager.MAX_ACCOUNT_STRING_LENGTH)
+ && isUserPropertyWithinLimit(mAccountType, UserManager.MAX_ACCOUNT_STRING_LENGTH))
+ || (mAccountOptions != null && !mAccountOptions.isBundleContentsWithinLengthLimit(
+ UserManager.MAX_ACCOUNT_OPTIONS_LENGTH))) {
+ setResult(UserManager.USER_CREATION_FAILED_NOT_PERMITTED);
+ Log.i(TAG, "User properties must not exceed their character limits");
+ return null;
} else if (cantCreateAnyMoreUsers) {
setResult(UserManager.USER_CREATION_FAILED_NO_MORE_USERS);
return null;
@@ -141,4 +149,8 @@
}
finish();
}
+
+ private boolean isUserPropertyWithinLimit(String property, int limit) {
+ return property == null || property.length() <= limit;
+ }
}
diff --git a/services/core/java/com/android/server/pm/UserManagerService.java b/services/core/java/com/android/server/pm/UserManagerService.java
index be68545..04ef75b 100644
--- a/services/core/java/com/android/server/pm/UserManagerService.java
+++ b/services/core/java/com/android/server/pm/UserManagerService.java
@@ -255,8 +255,6 @@
private static final int USER_VERSION = 9;
- private static final int MAX_USER_STRING_LENGTH = 500;
-
private static final long EPOCH_PLUS_30_YEARS = 30L * 365 * 24 * 60 * 60 * 1000L; // ms
static final int WRITE_USER_MSG = 1;
@@ -3407,16 +3405,18 @@
if (userData.persistSeedData) {
if (userData.seedAccountName != null) {
serializer.attribute(null, ATTR_SEED_ACCOUNT_NAME,
- truncateString(userData.seedAccountName));
+ truncateString(userData.seedAccountName,
+ UserManager.MAX_ACCOUNT_STRING_LENGTH));
}
if (userData.seedAccountType != null) {
serializer.attribute(null, ATTR_SEED_ACCOUNT_TYPE,
- truncateString(userData.seedAccountType));
+ truncateString(userData.seedAccountType,
+ UserManager.MAX_ACCOUNT_STRING_LENGTH));
}
}
if (userInfo.name != null) {
serializer.startTag(null, TAG_NAME);
- serializer.text(truncateString(userInfo.name));
+ serializer.text(truncateString(userInfo.name, UserManager.MAX_USER_NAME_LENGTH));
serializer.endTag(null, TAG_NAME);
}
synchronized (mRestrictionsLock) {
@@ -3456,11 +3456,11 @@
serializer.endDocument();
}
- private String truncateString(String original) {
- if (original == null || original.length() <= MAX_USER_STRING_LENGTH) {
+ private String truncateString(String original, int limit) {
+ if (original == null || original.length() <= limit) {
return original;
}
- return original.substring(0, MAX_USER_STRING_LENGTH);
+ return original.substring(0, limit);
}
/*
@@ -3868,8 +3868,7 @@
boolean preCreate, @Nullable String[] disallowedPackages,
@NonNull TimingsTraceAndSlog t, @Nullable Object token)
throws UserManager.CheckedUserOperationException {
-
- String truncatedName = truncateString(name);
+ String truncatedName = truncateString(name, UserManager.MAX_USER_NAME_LENGTH);
final UserTypeDetails userTypeDetails = mUserTypes.get(userType);
if (userTypeDetails == null) {
Slog.e(LOG_TAG, "Cannot create user of invalid user type: " + userType);
@@ -5410,9 +5409,14 @@
Slog.e(LOG_TAG, "No such user for settings seed data u=" + userId);
return;
}
- userData.seedAccountName = truncateString(accountName);
- userData.seedAccountType = truncateString(accountType);
- userData.seedAccountOptions = accountOptions;
+ userData.seedAccountName = truncateString(accountName,
+ UserManager.MAX_ACCOUNT_STRING_LENGTH);
+ userData.seedAccountType = truncateString(accountType,
+ UserManager.MAX_ACCOUNT_STRING_LENGTH);
+ if (accountOptions != null && accountOptions.isBundleContentsWithinLengthLimit(
+ UserManager.MAX_ACCOUNT_OPTIONS_LENGTH)) {
+ userData.seedAccountOptions = accountOptions;
+ }
userData.persistSeedData = persist;
}
if (persist) {
diff --git a/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java b/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java
index 06b7112..3730fe3 100644
--- a/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java
+++ b/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java
@@ -19,6 +19,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.junit.Assume.assumeTrue;
import static org.testng.Assert.assertThrows;
@@ -33,6 +34,7 @@
import android.content.pm.UserInfo;
import android.content.res.Resources;
import android.os.Bundle;
+import android.os.PersistableBundle;
import android.os.UserHandle;
import android.os.UserManager;
import android.platform.test.annotations.Postsubmit;
@@ -1114,6 +1116,106 @@
assertThat(userInfo.name).isEqualTo(newName);
}
+ @Test
+ public void testAddUserAccountData_validStringValuesAreSaved_validBundleIsSaved() {
+ assumeManagedUsersSupported();
+
+ String userName = "User";
+ String accountName = "accountName";
+ String accountType = "accountType";
+ String arrayKey = "StringArrayKey";
+ String stringKey = "StringKey";
+ String intKey = "IntKey";
+ String nestedBundleKey = "PersistableBundleKey";
+ String value1 = "Value 1";
+ String value2 = "Value 2";
+ String value3 = "Value 3";
+
+ UserInfo userInfo = mUserManager.createUser(userName,
+ UserManager.USER_TYPE_FULL_SECONDARY, 0);
+
+ PersistableBundle accountOptions = new PersistableBundle();
+ String[] stringArray = {value1, value2};
+ accountOptions.putInt(intKey, 1234);
+ PersistableBundle nested = new PersistableBundle();
+ nested.putString(stringKey, value3);
+ accountOptions.putPersistableBundle(nestedBundleKey, nested);
+ accountOptions.putStringArray(arrayKey, stringArray);
+
+ mUserManager.clearSeedAccountData();
+ mUserManager.setSeedAccountData(mContext.getUserId(), accountName,
+ accountType, accountOptions);
+
+ //assert userName accountName and accountType were saved correctly
+ assertTrue(mUserManager.getUserInfo(userInfo.id).name.equals(userName));
+ assertTrue(mUserManager.getSeedAccountName().equals(accountName));
+ assertTrue(mUserManager.getSeedAccountType().equals(accountType));
+
+ //assert bundle with correct values was added
+ assertThat(mUserManager.getSeedAccountOptions().containsKey(arrayKey)).isTrue();
+ assertThat(mUserManager.getSeedAccountOptions().getPersistableBundle(nestedBundleKey)
+ .getString(stringKey)).isEqualTo(value3);
+ assertThat(mUserManager.getSeedAccountOptions().getStringArray(arrayKey)[0])
+ .isEqualTo(value1);
+
+ mUserManager.removeUser(userInfo.id);
+ }
+
+ @Test
+ public void testAddUserAccountData_invalidStringValuesAreTruncated_invalidBundleIsDropped() {
+ assumeManagedUsersSupported();
+
+ String tooLongString = generateLongString();
+ String userName = "User " + tooLongString;
+ String accountType = "Account Type " + tooLongString;
+ String accountName = "accountName " + tooLongString;
+ String arrayKey = "StringArrayKey";
+ String stringKey = "StringKey";
+ String intKey = "IntKey";
+ String nestedBundleKey = "PersistableBundleKey";
+ String value1 = "Value 1";
+ String value2 = "Value 2";
+
+ UserInfo userInfo = mUserManager.createUser(userName,
+ UserManager.USER_TYPE_FULL_SECONDARY, 0);
+
+ PersistableBundle accountOptions = new PersistableBundle();
+ String[] stringArray = {value1, value2};
+ accountOptions.putInt(intKey, 1234);
+ PersistableBundle nested = new PersistableBundle();
+ nested.putString(stringKey, tooLongString);
+ accountOptions.putPersistableBundle(nestedBundleKey, nested);
+ accountOptions.putStringArray(arrayKey, stringArray);
+ mUserManager.clearSeedAccountData();
+ mUserManager.setSeedAccountData(mContext.getUserId(), accountName,
+ accountType, accountOptions);
+
+ //assert userName was truncated
+ assertTrue(mUserManager.getUserInfo(userInfo.id).name.length()
+ == UserManager.MAX_USER_NAME_LENGTH);
+
+ //assert accountName and accountType got truncated
+ assertTrue(mUserManager.getSeedAccountName().length()
+ == UserManager.MAX_ACCOUNT_STRING_LENGTH);
+ assertTrue(mUserManager.getSeedAccountType().length()
+ == UserManager.MAX_ACCOUNT_STRING_LENGTH);
+
+ //assert bundle with invalid values was dropped
+ assertThat(mUserManager.getSeedAccountOptions() == null).isTrue();
+
+ mUserManager.removeUser(userInfo.id);
+ }
+
+ private String generateLongString() {
+ String partialString = "Test Name Test Name Test Name Test Name Test Name Test Name Test "
+ + "Name Test Name Test Name Test Name "; //String of length 100
+ StringBuilder resultString = new StringBuilder();
+ for (int i = 0; i < 600; i++) {
+ resultString.append(partialString);
+ }
+ return resultString.toString();
+ }
+
private boolean isPackageInstalledForUser(String packageName, int userId) {
try {
return mPackageManager.getPackageInfoAsUser(packageName, 0, userId) != null;