Fixes and clarifications to app signature (or better certificate) pinning

4 files changed, 50 insertions, 38 deletions

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainContract.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainContract.java
index 6af5f4217..d5283f01f 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainContract.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainContract.java
@@ -73,7 +73,7 @@ public class KeychainContract {
 
     interface ApiAppsColumns {
         String PACKAGE_NAME = "package_name";
-        String PACKAGE_SIGNATURE = "package_signature";
+        String PACKAGE_CERTIFICATE = "package_signature";
     }
 
     interface ApiAppsAccountsColumns {
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainDatabase.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainDatabase.java
index 4a162989f..ff661e494 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainDatabase.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainDatabase.java
@@ -148,7 +148,7 @@ public class KeychainDatabase extends SQLiteOpenHelper {
             "CREATE TABLE IF NOT EXISTS " + Tables.API_APPS + " ("
                 + BaseColumns._ID + " INTEGER PRIMARY KEY AUTOINCREMENT, "
                 + ApiAppsColumns.PACKAGE_NAME + " TEXT NOT NULL UNIQUE, "
-                + ApiAppsColumns.PACKAGE_SIGNATURE + " BLOB"
+                + ApiAppsColumns.PACKAGE_CERTIFICATE + " BLOB"
             + ")";
 
     private static final String CREATE_API_APPS_ACCOUNTS =
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
index 1d3934620..1cd1826a3 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
@@ -45,7 +45,6 @@ import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey.SecretKeyType;
 import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKeyRing;
 import org.sufficientlysecure.keychain.pgp.KeyRing;
 import org.sufficientlysecure.keychain.pgp.PgpConstants;
-import org.sufficientlysecure.keychain.pgp.PgpHelper;
 import org.sufficientlysecure.keychain.pgp.Progressable;
 import org.sufficientlysecure.keychain.pgp.UncachedKeyRing;
 import org.sufficientlysecure.keychain.pgp.UncachedPublicKey;
@@ -1415,7 +1414,7 @@ public class ProviderHelper {
     private ContentValues contentValueForApiApps(AppSettings appSettings) {
         ContentValues values = new ContentValues();
         values.put(ApiApps.PACKAGE_NAME, appSettings.getPackageName());
-        values.put(ApiApps.PACKAGE_SIGNATURE, appSettings.getPackageSignature());
+        values.put(ApiApps.PACKAGE_CERTIFICATE, appSettings.getPackageSignature());
         return values;
     }
 
@@ -1462,7 +1461,7 @@ public class ProviderHelper {
                 settings.setPackageName(cursor.getString(
                         cursor.getColumnIndex(KeychainContract.ApiApps.PACKAGE_NAME)));
                 settings.setPackageSignature(cursor.getBlob(
-                        cursor.getColumnIndex(KeychainContract.ApiApps.PACKAGE_SIGNATURE)));
+                        cursor.getColumnIndex(KeychainContract.ApiApps.PACKAGE_CERTIFICATE)));
             }
         } finally {
             if (cursor != null) {
@@ -1575,10 +1574,10 @@ public class ProviderHelper {
         return fingerprints;
     }
 
-    public byte[] getApiAppSignature(String packageName) {
+    public byte[] getApiAppCertificate(String packageName) {
         Uri queryUri = ApiApps.buildByPackageNameUri(packageName);
 
-        String[] projection = new String[]{ApiApps.PACKAGE_SIGNATURE};
+        String[] projection = new String[]{ApiApps.PACKAGE_CERTIFICATE};
 
         Cursor cursor = mContentResolver.query(queryUri, projection, null, null, null);
         try {
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/RemoteService.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/RemoteService.java
index 59dafb505..e4d4ac49a 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/RemoteService.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/RemoteService.java
@@ -37,6 +37,8 @@ import org.sufficientlysecure.keychain.provider.ProviderHelper;
 import org.sufficientlysecure.keychain.remote.ui.RemoteServiceActivity;
 import org.sufficientlysecure.keychain.util.Log;
 
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 
@@ -45,10 +47,10 @@ import java.util.Arrays;
  */
 public abstract class RemoteService extends Service {
 
-    public static class WrongPackageSignatureException extends Exception {
+    public static class WrongPackageCertificateException extends Exception {
         private static final long serialVersionUID = -8294642703122196028L;
 
-        public WrongPackageSignatureException(String message) {
+        public WrongPackageCertificateException(String message) {
             super(message);
         }
     }
@@ -74,9 +76,9 @@ public abstract class RemoteService extends Service {
                 String packageName = getCurrentCallingPackage();
                 Log.d(Constants.TAG, "isAllowed packageName: " + packageName);
 
-                byte[] packageSignature;
+                byte[] packageCertificate;
                 try {
-                    packageSignature = getPackageSignature(packageName);
+                    packageCertificate = getPackageCertificate(packageName);
                 } catch (NameNotFoundException e) {
                     Log.e(Constants.TAG, "Should not happen, returning!", e);
                     // return error
@@ -91,7 +93,7 @@ public abstract class RemoteService extends Service {
                 Intent intent = new Intent(getBaseContext(), RemoteServiceActivity.class);
                 intent.setAction(RemoteServiceActivity.ACTION_REGISTER);
                 intent.putExtra(RemoteServiceActivity.EXTRA_PACKAGE_NAME, packageName);
-                intent.putExtra(RemoteServiceActivity.EXTRA_PACKAGE_SIGNATURE, packageSignature);
+                intent.putExtra(RemoteServiceActivity.EXTRA_PACKAGE_SIGNATURE, packageCertificate);
                 intent.putExtra(RemoteServiceActivity.EXTRA_DATA, data);
 
                 PendingIntent pi = PendingIntent.getActivity(getBaseContext(), 0,
@@ -105,7 +107,7 @@ public abstract class RemoteService extends Service {
 
                 return result;
             }
-        } catch (WrongPackageSignatureException e) {
+        } catch (WrongPackageCertificateException e) {
             Log.e(Constants.TAG, "wrong signature!", e);
 
             Intent intent = new Intent(getBaseContext(), RemoteServiceActivity.class);
@@ -127,14 +129,24 @@ public abstract class RemoteService extends Service {
         }
     }
 
-    private byte[] getPackageSignature(String packageName) throws NameNotFoundException {
+    private byte[] getPackageCertificate(String packageName) throws NameNotFoundException {
         PackageInfo pkgInfo = getPackageManager().getPackageInfo(packageName,
                 PackageManager.GET_SIGNATURES);
-        Signature[] signatures = pkgInfo.signatures;
-        // TODO: Only first signature?!
-        byte[] packageSignature = signatures[0].toByteArray();
+        // NOTE: Silly Android API naming: Signatures are actually certificates
+        Signature[] certificates = pkgInfo.signatures;
+        ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+        for (Signature cert : certificates) {
+            try {
+                outputStream.write(cert.toByteArray());
+            } catch (IOException e) {
+                throw new RuntimeException("Should not happen! Writing ByteArrayOutputStream to concat certificates failed");
+            }
+        }
 
-        return packageSignature;
+        // Even if an apk has several certificates, these certificates should never change
+        // Google Play does not allow the introduction of new certificates into an existing apk
+        // Also see this attack: http://stackoverflow.com/a/10567852
+        return outputStream.toByteArray();
     }
 
     /**
@@ -144,9 +156,12 @@ public abstract class RemoteService extends Service {
      * @return package name
      */
     protected String getCurrentCallingPackage() {
-        // TODO:
-        // callingPackages contains more than one entry when sharedUserId has been used...
         String[] callingPackages = getPackageManager().getPackagesForUid(Binder.getCallingUid());
+
+        // NOTE: No support for sharedUserIds
+        // callingPackages contains more than one entry when sharedUserId has been used
+        // No plans to support sharedUserIds due to many bugs connected to them:
+        // http://java-hamster.blogspot.de/2010/05/androids-shareduserid.html
         String currentPkg = callingPackages[0];
         Log.d(Constants.TAG, "currentPkg: " + currentPkg);
 
@@ -155,12 +170,12 @@ public abstract class RemoteService extends Service {
 
     /**
      * DEPRECATED API
-     *
+     * <p/>
      * Retrieves AccountSettings from database for the application calling this remote service
      */
     protected AccountSettings getAccSettings(String accountName) {
         String currentPkg = getCurrentCallingPackage();
-        Log.d(Constants.TAG, "getAccSettings accountName: "+ accountName);
+        Log.d(Constants.TAG, "getAccSettings accountName: " + accountName);
 
         Uri uri = KeychainContract.ApiAccounts.buildByPackageAndAccountUri(currentPkg, accountName);
 
@@ -198,14 +213,14 @@ public abstract class RemoteService extends Service {
      *
      * @param allowOnlySelf allow only Keychain app itself
      * @return true if process is allowed to use this service
-     * @throws WrongPackageSignatureException
+     * @throws WrongPackageCertificateException
      */
-    private boolean isCallerAllowed(boolean allowOnlySelf) throws WrongPackageSignatureException {
+    private boolean isCallerAllowed(boolean allowOnlySelf) throws WrongPackageCertificateException {
         return isUidAllowed(Binder.getCallingUid(), allowOnlySelf);
     }
 
     private boolean isUidAllowed(int uid, boolean allowOnlySelf)
-            throws WrongPackageSignatureException {
+            throws WrongPackageCertificateException {
         if (android.os.Process.myUid() == uid) {
             return true;
         }
@@ -229,11 +244,9 @@ public abstract class RemoteService extends Service {
     /**
      * Checks if packageName is a registered app for the API. Does not return true for own package!
      *
-     * @param packageName
-     * @return
-     * @throws WrongPackageSignatureException
+     * @throws WrongPackageCertificateException
      */
-    private boolean isPackageAllowed(String packageName) throws WrongPackageSignatureException {
+    private boolean isPackageAllowed(String packageName) throws WrongPackageCertificateException {
         Log.d(Constants.TAG, "isPackageAllowed packageName: " + packageName);
 
         ArrayList<String> allowedPkgs = mProviderHelper.getRegisteredApiApps();
@@ -244,22 +257,22 @@ public abstract class RemoteService extends Service {
             Log.d(Constants.TAG, "Package is allowed! packageName: " + packageName);
 
             // check package signature
-            byte[] currentSig;
+            byte[] currentCert;
             try {
-                currentSig = getPackageSignature(packageName);
+                currentCert = getPackageCertificate(packageName);
             } catch (NameNotFoundException e) {
-                throw new WrongPackageSignatureException(e.getMessage());
+                throw new WrongPackageCertificateException(e.getMessage());
             }
 
-            byte[] storedSig = mProviderHelper.getApiAppSignature(packageName);
-            if (Arrays.equals(currentSig, storedSig)) {
+            byte[] storedCert = mProviderHelper.getApiAppCertificate(packageName);
+            if (Arrays.equals(currentCert, storedCert)) {
                 Log.d(Constants.TAG,
-                        "Package signature is correct! (equals signature from database)");
+                        "Package certificate is correct! (equals certificate from database)");
                 return true;
             } else {
-                throw new WrongPackageSignatureException(
-                        "PACKAGE NOT ALLOWED! Signature wrong! (Signature not " +
-                                "equals signature from database)");
+                throw new WrongPackageCertificateException(
+                        "PACKAGE NOT ALLOWED! Certificate wrong! (Certificate not " +
+                                "equals certificate from database)");
             }
         }