KeyChain: Do not validate policy-provided aliases
The KeyChainActivity validated, before granting access to an alias,
that it is user-selectable. This is a defense-in-depth mechanism
to avoid granting access to non-user-selectable keys due to bugs
or race conditions.
However, that check does not make sense if the alias was chosen
programatically by the DeviceAdminReceiver implementation.
Avoid performing the user-selectability check for policy-provided
aliases by propagating the origin of the alias and skipping the
check if it was provided programatically.
Part of the fix for b/69337278
Bug: 69337278
Test: m -j RunKeyChainRoboTests && cts-tradefed run commandAndExit cts-dev -a armeabi-v7a -m CtsDevicePolicyManagerTestCases -t com.android.cts.devicepolicy.DeviceOwnerTest#testKeyManagement
Change-Id: I4a22e193eaf73595745ac41d9b53a064d3f41830
diff --git a/src/com/android/keychain/KeyChainActivity.java b/src/com/android/keychain/KeyChainActivity.java
index 2eb7c89..7360531 100644
--- a/src/com/android/keychain/KeyChainActivity.java
+++ b/src/com/android/keychain/KeyChainActivity.java
@@ -171,7 +171,7 @@
@Override public void alias(String alias) {
// Use policy-suggested alias if provided
if (alias != null) {
- finish(alias);
+ finishWithAliasFromPolicy(alias);
return;
}
@@ -466,6 +466,14 @@
}
private void finish(String alias) {
+ finish(alias, false);
+ }
+
+ private void finishWithAliasFromPolicy(String alias) {
+ finish(alias, true);
+ }
+
+ private void finish(String alias, boolean isAliasFromPolicy) {
if (alias == null) {
setResult(RESULT_CANCELED);
} else {
@@ -477,7 +485,7 @@
= IKeyChainAliasCallback.Stub.asInterface(
getIntent().getIBinderExtra(KeyChain.EXTRA_RESPONSE));
if (keyChainAliasResponse != null) {
- new ResponseSender(keyChainAliasResponse, alias).execute();
+ new ResponseSender(keyChainAliasResponse, alias, isAliasFromPolicy).execute();
return;
}
finish();
@@ -486,16 +494,25 @@
private class ResponseSender extends AsyncTask<Void, Void, Void> {
private IKeyChainAliasCallback mKeyChainAliasResponse;
private String mAlias;
- private ResponseSender(IKeyChainAliasCallback keyChainAliasResponse, String alias) {
+ private boolean mFromPolicy;
+
+ private ResponseSender(IKeyChainAliasCallback keyChainAliasResponse, String alias,
+ boolean isFromPolicy) {
mKeyChainAliasResponse = keyChainAliasResponse;
mAlias = alias;
+ mFromPolicy = isFromPolicy;
}
@Override protected Void doInBackground(Void... unused) {
try {
if (mAlias != null) {
KeyChain.KeyChainConnection connection = KeyChain.bind(KeyChainActivity.this);
try {
- if (!connection.getService().isUserSelectable(mAlias)) {
+ // This is a safety check to make sure an alias was not somehow chosen by
+ // the user but is not user-selectable.
+ // However, if the alias was selected by the Device Owner / Profile Owner
+ // (by implementing DeviceAdminReceiver), then there's no need to check
+ // this.
+ if (!mFromPolicy && (!connection.getService().isUserSelectable(mAlias))) {
Log.w(TAG, String.format("Alias %s not user-selectable.", mAlias));
//TODO: Should we invoke the callback with null here to indicate error?
return null;
diff --git a/src/com/android/keychain/KeyChainService.java b/src/com/android/keychain/KeyChainService.java
index 09bdc6c..419da2e 100644
--- a/src/com/android/keychain/KeyChainService.java
+++ b/src/com/android/keychain/KeyChainService.java
@@ -206,7 +206,7 @@
if (!Credentials.deleteAllTypesForAlias(mKeyStore, alias)) {
return false;
}
- mGrantsDb.removeGrantsForAlias(alias);
+ mGrantsDb.removeAliasInformation(alias);
broadcastKeychainChange();
broadcastLegacyStorageChange();
return true;
@@ -220,7 +220,7 @@
@Override public boolean reset() {
// only Settings should be able to reset
checkSystemCaller();
- mGrantsDb.removeAllGrants();
+ mGrantsDb.removeAllAliasesInformation();
boolean ok = true;
synchronized (mTrustedCertificateStore) {
// delete user-installed CA certs
diff --git a/src/com/android/keychain/internal/GrantsDatabase.java b/src/com/android/keychain/internal/GrantsDatabase.java
index 28605b5..1e333c1 100644
--- a/src/com/android/keychain/internal/GrantsDatabase.java
+++ b/src/com/android/keychain/internal/GrantsDatabase.java
@@ -142,14 +142,16 @@
}
}
- public void removeGrantsForAlias(String alias) {
+ public void removeAliasInformation(String alias) {
final SQLiteDatabase db = mDatabaseHelper.getWritableDatabase();
db.delete(TABLE_GRANTS, SELECTION_GRANTS_BY_ALIAS, new String[] {alias});
+ db.delete(TABLE_SELECTABLE, SELECTION_GRANTS_BY_ALIAS, new String[] {alias});
}
- public void removeAllGrants() {
+ public void removeAllAliasesInformation() {
final SQLiteDatabase db = mDatabaseHelper.getWritableDatabase();
db.delete(TABLE_GRANTS, null /* whereClause */, null /* whereArgs */);
+ db.delete(TABLE_SELECTABLE, null /* whereClause */, null /* whereArgs */);
}
public void purgeOldGrants(PackageManager pm) {
@@ -203,7 +205,7 @@
}
boolean isSelectable = Boolean.parseBoolean(res.getString(0));
- if (!res.isAfterLast()) {
+ if (res.getCount() > 1) {
// BUG! Should not have more than one result for any given alias.
Log.w(TAG, String.format("Have more than one result for alias %s", alias));
}