From e06f53372649b766cefe0cc408381f497426a2b8 Mon Sep 17 00:00:00 2001 From: Janis Danisevskis Date: Thu, 8 Jun 2017 17:53:34 -0700 Subject: [PATCH] Refurbish granting mechanism Keystore stores key blobs in with filenames that include the symbolic name and the uid of the owner. This behaviour should have been completely opaque to the user keystore. However, the granting mechanism, by which an app can allow another app to use one of its keys, leaked the internal structure in that the grantee had to specify the key name with the granter's uid prefix in order to use the granted key. This in turn collided with prefix handling in other parts of the framework. This patch refurbishes the granting mechanism such that keystore can choose a name for the grant. It uses the original symbolic key name as prefix and appends _KEYSTOREGRANT_ where the grant_no is chosen as first free slot starting from 0. Each uid has its own grant_no space. This changes the grant call such that it now returns a string, which is the alias name of the newly created grant. The string is empty if the grant operation failed. As before apps can still mask granted keys by importing a key with the exact same name including the added suffix. But everybody deserves the right to shoot themselves in the foot if they really want to. Bug: 37264540 Bug: 62237038 Test: run cts-dev --module CtsDevicePolicyManagerTestCases --test com.android.cts.devicepolicy.DeviceOwnerTest#testKeyManagement because it grants a key Merged-In: I047512ba345c25e6e691e78f7a37fc3f97b95d32 Change-Id: I047512ba345c25e6e691e78f7a37fc3f97b95d32 --- .../android/security/IKeystoreService.aidl | 2 +- keystore/java/android/security/KeyStore.java | 8 +++++--- .../src/android/security/KeyStoreTest.java | 18 +++++++++--------- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/core/java/android/security/IKeystoreService.aidl b/core/java/android/security/IKeystoreService.aidl index bfc8636c70a40..42282ac2858b1 100644 --- a/core/java/android/security/IKeystoreService.aidl +++ b/core/java/android/security/IKeystoreService.aidl @@ -48,7 +48,7 @@ interface IKeystoreService { byte[] sign(String name, in byte[] data); int verify(String name, in byte[] data, in byte[] signature); byte[] get_pubkey(String name); - int grant(String name, int granteeUid); + String grant(String name, int granteeUid); int ungrant(String name, int granteeUid); long getmtime(String name, int uid); int duplicate(String srcKey, int srcUid, String destKey, int destUid); diff --git a/keystore/java/android/security/KeyStore.java b/keystore/java/android/security/KeyStore.java index bfd1422ddca4a..ccf9de0abdb4a 100644 --- a/keystore/java/android/security/KeyStore.java +++ b/keystore/java/android/security/KeyStore.java @@ -341,12 +341,14 @@ public class KeyStore { } } - public boolean grant(String key, int uid) { + public String grant(String key, int uid) { try { - return mBinder.grant(key, uid) == NO_ERROR; + String grantAlias = mBinder.grant(key, uid); + if (grantAlias == "") return null; + return grantAlias; } catch (RemoteException e) { Log.w(TAG, "Cannot connect to keystore", e); - return false; + return null; } } diff --git a/keystore/tests/src/android/security/KeyStoreTest.java b/keystore/tests/src/android/security/KeyStoreTest.java index 319cf32397a11..1d5bf1205bf94 100644 --- a/keystore/tests/src/android/security/KeyStoreTest.java +++ b/keystore/tests/src/android/security/KeyStoreTest.java @@ -483,7 +483,7 @@ public class KeyStoreTest extends ActivityUnitTestCase { mKeyStore.generate(TEST_KEYNAME, KeyStore.UID_SELF, NativeConstants.EVP_PKEY_RSA, RSA_KEY_SIZE, KeyStore.FLAG_ENCRYPTED, null)); - assertTrue("Should be able to grant key to other user", + assertNotNull("Should be able to grant key to other user", mKeyStore.grant(TEST_KEYNAME, 0)); } @@ -493,19 +493,19 @@ public class KeyStoreTest extends ActivityUnitTestCase { assertTrue("Should be able to import key for testcase", mKeyStore.importKey(TEST_KEYNAME, PRIVKEY_BYTES, KeyStore.UID_SELF, KeyStore.FLAG_ENCRYPTED)); - assertTrue("Should be able to grant key to other user", mKeyStore.grant(TEST_KEYNAME, 0)); + assertNotNull("Should be able to grant key to other user", mKeyStore.grant(TEST_KEYNAME, 0)); } public void testGrant_NoKey_Failure() throws Exception { assertTrue("Should be able to unlock keystore for test", mKeyStore.onUserPasswordChanged(TEST_PASSWD)); - assertFalse("Should not be able to grant without first initializing the keystore", + assertNull("Should not be able to grant without first initializing the keystore", mKeyStore.grant(TEST_KEYNAME, 0)); } public void testGrant_NotInitialized_Failure() throws Exception { - assertFalse("Should not be able to grant without first initializing the keystore", + assertNull("Should not be able to grant without first initializing the keystore", mKeyStore.grant(TEST_KEYNAME, 0)); } @@ -517,7 +517,7 @@ public class KeyStoreTest extends ActivityUnitTestCase { mKeyStore.generate(TEST_KEYNAME, KeyStore.UID_SELF, NativeConstants.EVP_PKEY_RSA, RSA_KEY_SIZE, KeyStore.FLAG_ENCRYPTED, null)); - assertTrue("Should be able to grant key to other user", + assertNotNull("Should be able to grant key to other user", mKeyStore.grant(TEST_KEYNAME, 0)); assertTrue("Should be able to ungrant key to other user", @@ -531,7 +531,7 @@ public class KeyStoreTest extends ActivityUnitTestCase { assertTrue("Should be able to import key for testcase", mKeyStore.importKey(TEST_KEYNAME, PRIVKEY_BYTES, KeyStore.UID_SELF, KeyStore.FLAG_ENCRYPTED)); - assertTrue("Should be able to grant key to other user", + assertNotNull("Should be able to grant key to other user", mKeyStore.grant(TEST_KEYNAME, 0)); assertTrue("Should be able to ungrant key to other user", @@ -563,7 +563,7 @@ public class KeyStoreTest extends ActivityUnitTestCase { mKeyStore.generate(TEST_KEYNAME, KeyStore.UID_SELF, NativeConstants.EVP_PKEY_RSA, RSA_KEY_SIZE, KeyStore.FLAG_ENCRYPTED, null)); - assertTrue("Should be able to grant key to other user", + assertNotNull("Should be able to grant key to other user", mKeyStore.grant(TEST_KEYNAME, 0)); assertTrue("Should be able to ungrant key to other user", @@ -581,10 +581,10 @@ public class KeyStoreTest extends ActivityUnitTestCase { mKeyStore.generate(TEST_KEYNAME, KeyStore.UID_SELF, NativeConstants.EVP_PKEY_RSA, RSA_KEY_SIZE, KeyStore.FLAG_ENCRYPTED, null)); - assertTrue("Should be able to grant key to other user", + assertNotNull("Should be able to grant key to other user", mKeyStore.grant(TEST_KEYNAME, 0)); - assertTrue("Should be able to grant key to other user a second time", + assertNotNull("Should be able to grant key to other user a second time", mKeyStore.grant(TEST_KEYNAME, 0)); assertTrue("Should be able to ungrant key to other user",