Stop saving password metrics to disk
Previously, DevicePolicyManager saved password stats (number of letters,
number of symbols, etc) to disk for FDE devices. This made it possible
for the isActivePasswordSufficient() API to return a result before the
password was entered for the first time after a reboot. Access to these
stats would significantly narrow the space of possible passwords an
attacker would need to explore.
Going forward, every time either the password or the password
requirements change, a flag will be persisted indicating whether the
current password meets the requirements. Before the password is entered
for the first time after a reboot, isActivePasswordSufficient() simply
returns the value of this flag. (After the password is entered for the
first time, isActivePasswordSufficient() uses password stats saved in
memory, as is the case today.)
This creates a window where isActivePasswordSufficient() may return an
incorrect value before the password is entered for the first time, if
the requirements are changed after startup so that the current password
no longer meets the requirements. This has been deemed an acceptable
compromise in order to avoid storing potentially sensitive data.
Test: runtest -c com.android.server.devicepolicy.DevicePolicyManagerTest
frameworks-services
Bug: 34218769
Change-Id: I5d3cd008a9ee2787bcb10ed5455bb61c6014ae00
diff --git a/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java b/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java
index e9e3a18..2470e87 100644
--- a/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java
+++ b/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java
@@ -16,16 +16,14 @@
package android.app.admin;
-import org.junit.Test;
-import org.junit.runner.RunWith;
+import static org.junit.Assert.assertEquals;
import android.os.Parcel;
import android.support.test.filters.SmallTest;
import android.support.test.runner.AndroidJUnit4;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import org.junit.Test;
+import org.junit.runner.RunWith;
/** Unit tests for {@link PasswordMetrics}. */
@RunWith(AndroidJUnit4.class)
@@ -43,20 +41,6 @@
assertEquals(0, metrics.numeric);
assertEquals(0, metrics.symbols);
assertEquals(0, metrics.nonLetter);
- assertTrue("default constructor does not produce default metrics", metrics.isDefault());
- }
-
- @Test
- public void testIsNotDefault() {
- final PasswordMetrics metrics = new PasswordMetrics(
- DevicePolicyManager.PASSWORD_QUALITY_NUMERIC, 12);
- assertFalse("non-default metrics are repoted as default", metrics.isDefault());
- }
-
- @Test
- public void testComputeForEmptyPassword() {
- final PasswordMetrics metrics = PasswordMetrics.computeForPassword("");
- assertTrue("empty password has default metrics", metrics.isDefault());
}
@Test
diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java
index 911bb2a..136d335 100644
--- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java
+++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java
@@ -254,6 +254,8 @@
private static final String TAG_PASSWORD_TOKEN_HANDLE = "password-token";
+ private static final String TAG_PASSWORD_VALIDITY = "password-validity";
+
private static final int REQUEST_EXPIRE_PASSWORD = 5571;
private static final long MS_PER_DAY = TimeUnit.DAYS.toMillis(1);
@@ -478,6 +480,8 @@
public static class DevicePolicyData {
@NonNull PasswordMetrics mActivePasswordMetrics = new PasswordMetrics();
int mFailedPasswordAttempts = 0;
+ boolean mPasswordStateHasBeenSetSinceBoot = false;
+ boolean mPasswordValidAtLastCheckpoint = false;
int mUserHandle;
int mPasswordOwner = -1;
@@ -2574,19 +2578,14 @@
out.endTag(null, "failed-password-attempts");
}
- // Don't save metrics for FBE devices
- final PasswordMetrics metrics = policy.mActivePasswordMetrics;
- if (!mInjector.storageManagerIsFileBasedEncryptionEnabled() && !metrics.isDefault()) {
- out.startTag(null, "active-password");
- out.attribute(null, "quality", Integer.toString(metrics.quality));
- out.attribute(null, "length", Integer.toString(metrics.length));
- out.attribute(null, "uppercase", Integer.toString(metrics.upperCase));
- out.attribute(null, "lowercase", Integer.toString(metrics.lowerCase));
- out.attribute(null, "letters", Integer.toString(metrics.letters));
- out.attribute(null, "numeric", Integer.toString(metrics.numeric));
- out.attribute(null, "symbols", Integer.toString(metrics.symbols));
- out.attribute(null, "nonletter", Integer.toString(metrics.nonLetter));
- out.endTag(null, "active-password");
+ // For FDE devices only, we save this flag so we can report on password sufficiency
+ // before the user enters their password for the first time after a reboot. For
+ // security reasons, we don't want to store the full set of active password metrics.
+ if (!mInjector.storageManagerIsFileBasedEncryptionEnabled()) {
+ out.startTag(null, TAG_PASSWORD_VALIDITY);
+ out.attribute(null, ATTR_VALUE,
+ Boolean.toString(policy.mPasswordValidAtLastCheckpoint));
+ out.endTag(null, TAG_PASSWORD_VALIDITY);
}
for (int i = 0; i < policy.mAcceptedCaCertificates.size(); i++) {
@@ -2864,19 +2863,14 @@
} else if (TAG_INITIALIZATION_BUNDLE.equals(tag)) {
policy.mInitBundle = PersistableBundle.restoreFromXml(parser);
} else if ("active-password".equals(tag)) {
- if (mInjector.storageManagerIsFileBasedEncryptionEnabled()) {
- // Remove this from FBE devices
- needsRewrite = true;
- } else {
- final PasswordMetrics m = policy.mActivePasswordMetrics;
- m.quality = Integer.parseInt(parser.getAttributeValue(null, "quality"));
- m.length = Integer.parseInt(parser.getAttributeValue(null, "length"));
- m.upperCase = Integer.parseInt(parser.getAttributeValue(null, "uppercase"));
- m.lowerCase = Integer.parseInt(parser.getAttributeValue(null, "lowercase"));
- m.letters = Integer.parseInt(parser.getAttributeValue(null, "letters"));
- m.numeric = Integer.parseInt(parser.getAttributeValue(null, "numeric"));
- m.symbols = Integer.parseInt(parser.getAttributeValue(null, "symbols"));
- m.nonLetter = Integer.parseInt(parser.getAttributeValue(null, "nonletter"));
+ // Remove password metrics from saved settings, as we no longer wish to store
+ // these on disk
+ needsRewrite = true;
+ } else if (TAG_PASSWORD_VALIDITY.equals(tag)) {
+ if (!mInjector.storageManagerIsFileBasedEncryptionEnabled()) {
+ // This flag is only used for FDE devices
+ policy.mPasswordValidAtLastCheckpoint = Boolean.parseBoolean(
+ parser.getAttributeValue(null, ATTR_VALUE));
}
} else if (TAG_PASSWORD_TOKEN_HANDLE.equals(tag)) {
policy.mPasswordTokenHandle = Long.parseLong(
@@ -3453,11 +3447,24 @@
who, DeviceAdminInfo.USES_POLICY_LIMIT_PASSWORD, parent);
if (ap.minimumPasswordMetrics.quality != quality) {
ap.minimumPasswordMetrics.quality = quality;
+ updatePasswordValidityCheckpointLocked(mInjector.userHandleGetCallingUserId());
saveSettingsLocked(mInjector.userHandleGetCallingUserId());
}
}
}
+ /**
+ * Updates flag in memory that tells us whether the user's password currently satisfies the
+ * requirements set by all of the user's active admins. This should be called before
+ * {@link #saveSettingsLocked} whenever the password or the admin policies have changed.
+ */
+ @GuardedBy("DevicePolicyManagerService.this")
+ private void updatePasswordValidityCheckpointLocked(int userHandle) {
+ DevicePolicyData policy = getUserData(userHandle);
+ policy.mPasswordValidAtLastCheckpoint = isActivePasswordSufficientForUserLocked(
+ policy, policy.mUserHandle, false);
+ }
+
@Override
public int getPasswordQuality(ComponentName who, int userHandle, boolean parent) {
if (!mHasFeature) {
@@ -3540,6 +3547,7 @@
who, DeviceAdminInfo.USES_POLICY_LIMIT_PASSWORD, parent);
if (ap.minimumPasswordMetrics.length != length) {
ap.minimumPasswordMetrics.length = length;
+ updatePasswordValidityCheckpointLocked(mInjector.userHandleGetCallingUserId());
saveSettingsLocked(mInjector.userHandleGetCallingUserId());
}
}
@@ -3584,6 +3592,7 @@
who, DeviceAdminInfo.USES_POLICY_LIMIT_PASSWORD, parent);
if (ap.passwordHistoryLength != length) {
ap.passwordHistoryLength = length;
+ updatePasswordValidityCheckpointLocked(mInjector.userHandleGetCallingUserId());
saveSettingsLocked(mInjector.userHandleGetCallingUserId());
}
}
@@ -3796,6 +3805,7 @@
who, DeviceAdminInfo.USES_POLICY_LIMIT_PASSWORD, parent);
if (ap.minimumPasswordMetrics.upperCase != length) {
ap.minimumPasswordMetrics.upperCase = length;
+ updatePasswordValidityCheckpointLocked(mInjector.userHandleGetCallingUserId());
saveSettingsLocked(mInjector.userHandleGetCallingUserId());
}
}
@@ -3837,6 +3847,7 @@
who, DeviceAdminInfo.USES_POLICY_LIMIT_PASSWORD, parent);
if (ap.minimumPasswordMetrics.lowerCase != length) {
ap.minimumPasswordMetrics.lowerCase = length;
+ updatePasswordValidityCheckpointLocked(mInjector.userHandleGetCallingUserId());
saveSettingsLocked(mInjector.userHandleGetCallingUserId());
}
}
@@ -3881,6 +3892,7 @@
who, DeviceAdminInfo.USES_POLICY_LIMIT_PASSWORD, parent);
if (ap.minimumPasswordMetrics.letters != length) {
ap.minimumPasswordMetrics.letters = length;
+ updatePasswordValidityCheckpointLocked(mInjector.userHandleGetCallingUserId());
saveSettingsLocked(mInjector.userHandleGetCallingUserId());
}
}
@@ -3928,6 +3940,7 @@
who, DeviceAdminInfo.USES_POLICY_LIMIT_PASSWORD, parent);
if (ap.minimumPasswordMetrics.numeric != length) {
ap.minimumPasswordMetrics.numeric = length;
+ updatePasswordValidityCheckpointLocked(mInjector.userHandleGetCallingUserId());
saveSettingsLocked(mInjector.userHandleGetCallingUserId());
}
}
@@ -3975,6 +3988,7 @@
who, DeviceAdminInfo.USES_POLICY_LIMIT_PASSWORD, parent);
if (ap.minimumPasswordMetrics.symbols != length) {
ap.minimumPasswordMetrics.symbols = length;
+ updatePasswordValidityCheckpointLocked(mInjector.userHandleGetCallingUserId());
saveSettingsLocked(mInjector.userHandleGetCallingUserId());
}
}
@@ -4022,6 +4036,7 @@
who, DeviceAdminInfo.USES_POLICY_LIMIT_PASSWORD, parent);
if (ap.minimumPasswordMetrics.nonLetter != length) {
ap.minimumPasswordMetrics.nonLetter = length;
+ updatePasswordValidityCheckpointLocked(mInjector.userHandleGetCallingUserId());
saveSettingsLocked(mInjector.userHandleGetCallingUserId());
}
}
@@ -4093,6 +4108,16 @@
DevicePolicyData policy, int userHandle, boolean parent) {
enforceUserUnlocked(userHandle, parent);
+ if (!mInjector.storageManagerIsFileBasedEncryptionEnabled()
+ && !policy.mPasswordStateHasBeenSetSinceBoot) {
+ // Before user enters their password for the first time after a reboot, return the
+ // value of this flag, which tells us whether the password was valid the last time
+ // settings were saved. If DPC changes password requirements on boot so that the
+ // current password no longer meets the requirements, this value will be stale until
+ // the next time the password is entered.
+ return policy.mPasswordValidAtLastCheckpoint;
+ }
+
final int requiredPasswordQuality = getPasswordQuality(null, userHandle, parent);
if (policy.mActivePasswordMetrics.quality < requiredPasswordQuality) {
return false;
@@ -5436,6 +5461,7 @@
DevicePolicyData policy = getUserData(userHandle);
synchronized (this) {
policy.mActivePasswordMetrics = metrics;
+ policy.mPasswordStateHasBeenSetSinceBoot = true;
}
}
@@ -5460,6 +5486,7 @@
try {
synchronized (this) {
policy.mFailedPasswordAttempts = 0;
+ updatePasswordValidityCheckpointLocked(userId);
saveSettingsLocked(userId);
updatePasswordExpirationsLocked(userId);
setExpirationAlarmCheckLocked(mContext, userId, /* parent */ false);
diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java
index c2b0ea5..fb74d05 100644
--- a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java
+++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java
@@ -26,6 +26,7 @@
import android.app.admin.DeviceAdminReceiver;
import android.app.admin.DevicePolicyManager;
import android.app.admin.DevicePolicyManagerInternal;
+import android.app.admin.PasswordMetrics;
import android.content.BroadcastReceiver;
import android.content.ComponentName;
import android.content.Context;
@@ -3836,6 +3837,80 @@
assertTrue(dpm.clearResetPasswordToken(admin1));
}
+ public void testIsActivePasswordSufficient() throws Exception {
+ mContext.binder.callingUid = DpmMockContext.CALLER_SYSTEM_USER_UID;
+ mContext.packageName = admin1.getPackageName();
+ setupDeviceOwner();
+
+ dpm.setPasswordQuality(admin1, DevicePolicyManager.PASSWORD_QUALITY_COMPLEX);
+ dpm.setPasswordMinimumLength(admin1, 8);
+ dpm.setPasswordMinimumLetters(admin1, 6);
+ dpm.setPasswordMinimumLowerCase(admin1, 3);
+ dpm.setPasswordMinimumUpperCase(admin1, 1);
+ dpm.setPasswordMinimumNonLetter(admin1, 1);
+ dpm.setPasswordMinimumNumeric(admin1, 1);
+ dpm.setPasswordMinimumSymbols(admin1, 0);
+
+ PasswordMetrics passwordMetricsNoSymbols = new PasswordMetrics(
+ DevicePolicyManager.PASSWORD_QUALITY_COMPLEX, 9,
+ 8, 2,
+ 6, 1,
+ 0, 1);
+
+ setActivePasswordState(passwordMetricsNoSymbols);
+ assertTrue(dpm.isActivePasswordSufficient());
+
+ initializeDpms();
+ reset(mContext.spiedContext);
+ assertTrue(dpm.isActivePasswordSufficient());
+
+ // This call simulates the user entering the password for the first time after a reboot.
+ // This causes password metrics to be reloaded into memory. Until this happens,
+ // dpm.isActivePasswordSufficient() will continue to return its last checkpointed value,
+ // even if the DPC changes password requirements so that the password no longer meets the
+ // requirements. This is a known limitation of the current implementation of
+ // isActivePasswordSufficient() - see b/34218769.
+ setActivePasswordState(passwordMetricsNoSymbols);
+ assertTrue(dpm.isActivePasswordSufficient());
+
+ dpm.setPasswordMinimumSymbols(admin1, 1);
+ // This assertion would fail if we had not called setActivePasswordState() again after
+ // initializeDpms() - see previous comment.
+ assertFalse(dpm.isActivePasswordSufficient());
+
+ initializeDpms();
+ reset(mContext.spiedContext);
+ assertFalse(dpm.isActivePasswordSufficient());
+
+ PasswordMetrics passwordMetricsWithSymbols = new PasswordMetrics(
+ DevicePolicyManager.PASSWORD_QUALITY_COMPLEX, 9,
+ 7, 2,
+ 5, 1,
+ 1, 2);
+
+ setActivePasswordState(passwordMetricsWithSymbols);
+ assertTrue(dpm.isActivePasswordSufficient());
+ }
+
+ private void setActivePasswordState(PasswordMetrics passwordMetrics) {
+ int userHandle = UserHandle.getUserId(mContext.binder.callingUid);
+ final long ident = mContext.binder.clearCallingIdentity();
+ try {
+ dpm.setActivePasswordState(passwordMetrics, userHandle);
+ dpm.reportPasswordChanged(userHandle);
+
+ final Intent intent = new Intent(DeviceAdminReceiver.ACTION_PASSWORD_CHANGED);
+ intent.setComponent(admin1);
+ intent.putExtra(Intent.EXTRA_USER, UserHandle.of(mContext.binder.callingUid));
+
+ verify(mContext.spiedContext, times(1)).sendBroadcastAsUser(
+ MockUtils.checkIntent(intent),
+ MockUtils.checkUserHandle(userHandle));
+ } finally {
+ mContext.binder.restoreCallingIdentity(ident);
+ }
+ }
+
public void testIsCurrentInputMethodSetByOwnerForDeviceOwner() throws Exception {
final String currentIme = Settings.Secure.DEFAULT_INPUT_METHOD;
final Uri currentImeUri = Settings.Secure.getUriFor(currentIme);