From d05498b9d8d30ca69eaafe920c5915ee472058eb Mon Sep 17 00:00:00 2001 From: Max Bires Date: Sat, 5 Jun 2021 15:16:47 -0700 Subject: [PATCH] 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. Bug: 190222116 Test: atest RemoteProvisionerUnitTests Change-Id: I917a61da612f21f9a0f783bea5d24270d4e1db42 --- .../java/android/security/GenerateRkpKey.java | 68 +++++++++++++------ .../AndroidKeyStoreKeyPairGeneratorSpi.java | 2 +- 2 files changed, 50 insertions(+), 20 deletions(-) diff --git a/keystore/java/android/security/GenerateRkpKey.java b/keystore/java/android/security/GenerateRkpKey.java index a1a7aa85519f..cc1ec1bada50 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,51 @@ 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) { + throw new RemoteException("Could not resolve GenerateRkpKeyService."); + } 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());