From e14a2efcaded63ee6d0946d46a605c858941e3ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Sun, 10 May 2015 03:31:19 +0200 Subject: Fixes and clarifications to app signature (or better certificate) pinning --- .../keychain/provider/KeychainContract.java | 2 +- .../keychain/provider/KeychainDatabase.java | 2 +- .../keychain/provider/ProviderHelper.java | 9 ++- .../keychain/remote/RemoteService.java | 75 +++++++++++++--------- 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 - * + *

* 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 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)"); } } -- cgit v1.2.3