Fixing the race condition in GenerateRkpKey

This file was written on the assumption that bindService was
synchronous, which it isn't. This change adds a CountDownLatch to force
the class to wait for the binding to finish. If the relevant key
generation service is not present on the system, then this
functionality will just silently be skipped over.

Bug: 190222116
Test: atest RemoteProvisionerUnitTests
Change-Id: Ie34997a08aa743642c66a20c4b756cd47bff4af1
Merged-In: Ie34997a08aa743642c66a20c4b756cd47bff4af1

2 files changed, 51 insertions, 20 deletions

diff --git a/keystore/java/android/security/GenerateRkpKey.java b/keystore/java/android/security/GenerateRkpKey.java
index a1a7aa85519f..053bec74405e 100644
--- a/keystore/java/android/security/GenerateRkpKey.java
+++ b/keystore/java/android/security/GenerateRkpKey.java
@@ -22,6 +22,10 @@ import android.content.Intent;
 import android.content.ServiceConnection;
 import android.os.IBinder;
 import android.os.RemoteException;
+import android.util.Log;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 
 /**
  * GenerateKey is a helper class to handle interactions between Keystore and the RemoteProvisioner
@@ -41,14 +45,25 @@ import android.os.RemoteException;
  * @hide
  */
 public class GenerateRkpKey {
+    private static final String TAG = "GenerateRkpKey";
+
+    private static final int NOTIFY_EMPTY = 0;
+    private static final int NOTIFY_KEY_GENERATED = 1;
+    private static final int TIMEOUT_MS = 1000;
 
     private IGenerateRkpKeyService mBinder;
     private Context mContext;
+    private CountDownLatch mCountDownLatch;
 
     private ServiceConnection mConnection = new ServiceConnection() {
         @Override
         public void onServiceConnected(ComponentName className, IBinder service) {
             mBinder = IGenerateRkpKeyService.Stub.asInterface(service);
+            mCountDownLatch.countDown();
+        }
+
+        @Override public void onBindingDied(ComponentName className) {
+            mCountDownLatch.countDown();
         }
 
         @Override
@@ -64,36 +79,52 @@ public class GenerateRkpKey {
         mContext = context;
     }
 
-    /**
-     * Fulfills the use case of (2) described in the class documentation. Blocks until the
-     * RemoteProvisioner application can get new attestation keys signed by the server.
-     */
-    public void notifyEmpty(int securityLevel) throws RemoteException {
+    private void bindAndSendCommand(int command, int securityLevel) throws RemoteException {
         Intent intent = new Intent(IGenerateRkpKeyService.class.getName());
         ComponentName comp = intent.resolveSystemService(mContext.getPackageManager(), 0);
+        if (comp == null) {
+            // On a system that does not use RKP, the RemoteProvisioner app won't be installed.
+            return;
+        }
         intent.setComponent(comp);
-        if (comp == null || !mContext.bindService(intent, mConnection, Context.BIND_AUTO_CREATE)) {
-            throw new RemoteException("Failed to bind to GenerateKeyService");
+        mCountDownLatch = new CountDownLatch(1);
+        if (!mContext.bindService(intent, mConnection, Context.BIND_AUTO_CREATE)) {
+            throw new RemoteException("Failed to bind to GenerateRkpKeyService");
+        }
+        try {
+            mCountDownLatch.await(TIMEOUT_MS, TimeUnit.MILLISECONDS);
+        } catch (InterruptedException e) {
+            Log.e(TAG, "Interrupted: ", e);
         }
         if (mBinder != null) {
-            mBinder.generateKey(securityLevel);
+            switch (command) {
+                case NOTIFY_EMPTY:
+                    mBinder.generateKey(securityLevel);
+                    break;
+                case NOTIFY_KEY_GENERATED:
+                    mBinder.notifyKeyGenerated(securityLevel);
+                    break;
+                default:
+                    Log.e(TAG, "Invalid case for command");
+            }
+        } else {
+            Log.e(TAG, "Binder object is null; failed to bind to GenerateRkpKeyService.");
         }
         mContext.unbindService(mConnection);
     }
 
     /**
-     * FUlfills the use case of (1) described in the class documentation. Non blocking call.
+     * Fulfills the use case of (2) described in the class documentation. Blocks until the
+     * RemoteProvisioner application can get new attestation keys signed by the server.
+     */
+    public void notifyEmpty(int securityLevel) throws RemoteException {
+        bindAndSendCommand(NOTIFY_EMPTY, securityLevel);
+    }
+
+    /**
+     * Fulfills the use case of (1) described in the class documentation. Non blocking call.
      */
     public void notifyKeyGenerated(int securityLevel) throws RemoteException {
-        Intent intent = new Intent(IGenerateRkpKeyService.class.getName());
-        ComponentName comp = intent.resolveSystemService(mContext.getPackageManager(), 0);
-        intent.setComponent(comp);
-        if (comp == null || !mContext.bindService(intent, mConnection, Context.BIND_AUTO_CREATE)) {
-            throw new RemoteException("Failed to bind to GenerateKeyService");
-        }
-        if (mBinder != null) {
-            mBinder.notifyKeyGenerated(securityLevel);
-        }
-        mContext.unbindService(mConnection);
+        bindAndSendCommand(NOTIFY_KEY_GENERATED, securityLevel);
     }
 }
diff --git a/keystore/java/android/security/keystore2/AndroidKeyStoreKeyPairGeneratorSpi.java b/keystore/java/android/security/keystore2/AndroidKeyStoreKeyPairGeneratorSpi.java
index dc7f3dda35c0..c048f3bffc75 100644
--- a/keystore/java/android/security/keystore2/AndroidKeyStoreKeyPairGeneratorSpi.java
+++ b/keystore/java/android/security/keystore2/AndroidKeyStoreKeyPairGeneratorSpi.java
@@ -580,7 +580,7 @@ public abstract class AndroidKeyStoreKeyPairGeneratorSpi extends KeyPairGenerato
             } catch (RemoteException e) {
                 // This is not really an error state, and necessarily does not apply to non RKP
                 // systems or hybrid systems where RKP is not currently turned on.
-                Log.d(TAG, "Couldn't connect to the RemoteProvisioner backend.");
+                Log.d(TAG, "Couldn't connect to the RemoteProvisioner backend.", e);
             }
             success = true;
             return new KeyPair(publicKey, publicKey.getPrivateKey());