From 0a4725b962b1838f3cd064d25bc2bceb3e59d525 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 17 Nov 2014 22:21:18 +1100 Subject: [PATCH 1/2] Disable spongycastle before verifying jar files. Fixes issue #111. This is a workaround rather than a full solution, but it solves the problem. There is a bug in Lollipop, whereby including spongycastle as a security provider causes problems verifying .jar files. As a result, it is now disabled when actually performing verification, so that the libraries provided by Android do the work, and then re-enabled afterwards. There is an ever so slight chance that the period when this is disabled may align with the period when spongycastle may actually be required (i.e. for signing a local repo index). This is a risk which I cnanot see how to avoid, and will likel7 cause either the signing to fail due to the unavailability of the relevant security classes. However this is very minimal, hard to reproduce (I couldn't get it to happen) and also has the effect of the local repo failing, rather than the updating of apps failing (which is arguably more important) and so is worth it in my opinion. See comments at https://gitlab.com/fdroid/fdroidclient/issues/111 for much greater detail. --- src/org/fdroid/fdroid/FDroidApp.java | 27 ++++++++++-- src/org/fdroid/fdroid/data/App.java | 44 +++++++++++-------- .../fdroid/localrepo/LocalRepoKeyStore.java | 6 --- .../fdroid/updater/SignedRepoUpdater.java | 13 +++++- 4 files changed, 62 insertions(+), 28 deletions(-) diff --git a/src/org/fdroid/fdroid/FDroidApp.java b/src/org/fdroid/fdroid/FDroidApp.java index e16a5728c..c9ff2796f 100644 --- a/src/org/fdroid/fdroid/FDroidApp.java +++ b/src/org/fdroid/fdroid/FDroidApp.java @@ -55,10 +55,8 @@ import org.fdroid.fdroid.localrepo.LocalRepoService; import org.fdroid.fdroid.net.IconDownloader; import org.fdroid.fdroid.net.WifiStateChangeService; -import javax.net.ssl.HttpsURLConnection; -import javax.net.ssl.SSLContext; -import javax.net.ssl.TrustManager; import java.io.File; +import java.security.Security; import java.util.Set; public class FDroidApp extends Application { @@ -71,6 +69,8 @@ public class FDroidApp extends Application { public static Repo repo = new Repo(); public static Set selectedApps = null; // init in SelectLocalAppsFragment + // Leaving the fully qualified class name here to help clarify the difference between spongy/bouncy castle. + private static org.spongycastle.jce.provider.BouncyCastleProvider spongyCastleProvider; private static Messenger localRepoServiceMessenger = null; private static boolean localRepoServiceIsBound = false; @@ -78,6 +78,11 @@ public class FDroidApp extends Application { BluetoothAdapter bluetoothAdapter = null; + static { + spongyCastleProvider = new org.spongycastle.jce.provider.BouncyCastleProvider(); + enableSpongyCastle(); + } + public static enum Theme { dark, light, lightWithDarkActionBar } @@ -108,6 +113,22 @@ public class FDroidApp extends Application { return curTheme; } + public static void enableSpongyCastle() { + Security.addProvider(spongyCastleProvider); + } + + public static void enableSpongyCastleOnLollipop() { + if (Build.VERSION.SDK_INT == 21) { + Security.addProvider(spongyCastleProvider); + } + } + + public static void disableSpongyCastleOnLollipop() { + if (Build.VERSION.SDK_INT == 21) { + Security.removeProvider(spongyCastleProvider.getName()); + } + } + @Override public void onCreate() { super.onCreate(); diff --git a/src/org/fdroid/fdroid/data/App.java b/src/org/fdroid/fdroid/data/App.java index fd4af837c..933aa1ed0 100644 --- a/src/org/fdroid/fdroid/data/App.java +++ b/src/org/fdroid/fdroid/data/App.java @@ -11,6 +11,7 @@ import android.text.TextUtils; import android.util.Log; import org.fdroid.fdroid.AppFilter; +import org.fdroid.fdroid.FDroidApp; import org.fdroid.fdroid.Utils; import java.io.File; @@ -263,25 +264,32 @@ public class App extends ValueObject implements Comparable { throw new CertificateEncodingException("null signed entry!"); } - InputStream tmpIn = apkJar.getInputStream(aSignedEntry); - byte[] buff = new byte[2048]; - while (tmpIn.read(buff, 0, buff.length) != -1) { - /* - * NOP - apparently have to READ from the JarEntry before you can - * call getCerficates() and have it return != null. Yay Java. - */ + // Due to a bug in android 5.0 lollipop, the inclusion of BouncyCastle causes + // breakage when verifying the signature of most .jars. For more + // details, check out https://gitlab.com/fdroid/fdroidclient/issues/111. + try { + FDroidApp.disableSpongyCastleOnLollipop(); + InputStream tmpIn = apkJar.getInputStream(aSignedEntry); + byte[] buff = new byte[2048]; + while (tmpIn.read(buff, 0, buff.length) != -1) { + /* + * NOP - apparently have to READ from the JarEntry before you can + * call getCerficates() and have it return != null. Yay Java. + */ + } + tmpIn.close(); + + if (aSignedEntry.getCertificates() == null + || aSignedEntry.getCertificates().length == 0) { + apkJar.close(); + throw new CertificateEncodingException("No Certificates found!"); + } + + Certificate signer = aSignedEntry.getCertificates()[0]; + rawCertBytes = signer.getEncoded(); + } finally { + FDroidApp.enableSpongyCastleOnLollipop(); } - tmpIn.close(); - - if (aSignedEntry.getCertificates() == null - || aSignedEntry.getCertificates().length == 0) { - apkJar.close(); - throw new CertificateEncodingException("No Certificates found!"); - } - - Certificate signer = aSignedEntry.getCertificates()[0]; - rawCertBytes = signer.getEncoded(); - apkJar.close(); /* diff --git a/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java b/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java index b48a684e0..aafe89707 100644 --- a/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java +++ b/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java @@ -35,12 +35,6 @@ import kellinwood.security.zipsigner.ZipSigner; public class LocalRepoKeyStore { private static final String TAG = "KerplappKeyStore"; - - static { - Security.insertProviderAt( - new org.spongycastle.jce.provider.BouncyCastleProvider(), 1); - } - public static final String INDEX_CERT_ALIAS = "fdroid"; public static final String HTTP_CERT_ALIAS = "https"; diff --git a/src/org/fdroid/fdroid/updater/SignedRepoUpdater.java b/src/org/fdroid/fdroid/updater/SignedRepoUpdater.java index c6c9a4e48..31b14e7ec 100644 --- a/src/org/fdroid/fdroid/updater/SignedRepoUpdater.java +++ b/src/org/fdroid/fdroid/updater/SignedRepoUpdater.java @@ -2,6 +2,7 @@ package org.fdroid.fdroid.updater; import android.content.Context; import android.util.Log; +import org.fdroid.fdroid.FDroidApp; import org.fdroid.fdroid.Hasher; import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; @@ -123,7 +124,17 @@ public class SignedRepoUpdater extends RepoUpdater { // Don't worry about checking the status code for 200. If it was a // successful download, then we will have a file ready to use: if (indexJar != null && indexJar.exists()) { - indexXml = extractIndexFromJar(indexJar); + + // Due to a bug in android 5.0 lollipop, the inclusion of BouncyCastle causes + // breakage when verifying the signature of the downloaded .jar. For more + // details, check out https://gitlab.com/fdroid/fdroidclient/issues/111. + try { + FDroidApp.disableSpongyCastleOnLollipop(); + indexXml = extractIndexFromJar(indexJar); + } finally { + FDroidApp.enableSpongyCastleOnLollipop(); + } + } return indexXml; } From 6d0207c32eab0d7e7fdd0e696458e77385b8807d Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Sat, 29 Nov 2014 03:58:43 +1100 Subject: [PATCH 2/2] More robust failure conditions for local repo keystore. Was experiencing some problems with local repo keystore on Android 2.3.4. Previously, the local repo keystore could fail to create, but would still return a LocalRepoKeyStore instance to be used. Now it throws a checked exception, and is dealt with in the relveant places. Also cleaned up some calls to "e.printStackTrace()" and replaced with more informative logging messages. --- .../fdroid/localrepo/LocalRepoKeyStore.java | 95 ++++++++++++++----- .../fdroid/localrepo/LocalRepoManager.java | 28 ++++-- .../fdroid/localrepo/LocalRepoService.java | 13 ++- src/org/fdroid/fdroid/net/LocalHTTPD.java | 6 +- .../fdroid/net/WifiStateChangeService.java | 34 ++++--- 5 files changed, 119 insertions(+), 57 deletions(-) diff --git a/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java b/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java index aafe89707..351a5b4d2 100644 --- a/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java +++ b/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java @@ -34,7 +34,9 @@ import kellinwood.security.zipsigner.ZipSigner; // TODO Address exception handling in a uniform way throughout public class LocalRepoKeyStore { - private static final String TAG = "KerplappKeyStore"; + + private static final String TAG = "org.fdroid.fdroid.localrepo.LocalRepoKeyStore"; + public static final String INDEX_CERT_ALIAS = "fdroid"; public static final String HTTP_CERT_ALIAS = "https"; @@ -49,26 +51,49 @@ public class LocalRepoKeyStore { private KeyManager[] keyManagers; private File keyStoreFile; - public static LocalRepoKeyStore get(Context context) { + public static LocalRepoKeyStore get(Context context) throws InitException { if (localRepoKeyStore == null) localRepoKeyStore = new LocalRepoKeyStore(context); return localRepoKeyStore; } - private LocalRepoKeyStore(Context context) { + public static class InitException extends Exception { + public InitException(String detailMessage) { + super(detailMessage); + } + } + + private LocalRepoKeyStore(Context context) throws InitException { try { - Log.d(TAG, "generating LocalRepoKeyStore instance"); File appKeyStoreDir = context.getDir("keystore", Context.MODE_PRIVATE); + + Log.d(TAG, "Generating LocalRepoKeyStore instance: " + appKeyStoreDir.getAbsolutePath()); this.keyStoreFile = new File(appKeyStoreDir, "kerplapp.bks"); + + Log.d(TAG, "Using default KeyStore type: " + KeyStore.getDefaultType()); this.keyStore = KeyStore.getInstance(KeyStore.getDefaultType()); - // If there isn't a persisted BKS keystore on disk we need to - // create a new empty keystore + if (keyStoreFile.exists()) { + try { + Log.d(TAG, "Keystore already exists, loading..."); + keyStore.load(new FileInputStream(keyStoreFile), "".toCharArray()); + } catch (IOException e) { + Log.e(TAG, "Error while loading existing keystore. Will delete and create a new one."); + + // NOTE: Could opt to delete and then re-create the keystore here, but that may + // be undesirable. For example - if you were to re-connect to an existing device + // that you have swapped apps with in the past, then you would really want the + // signature to be the same as last time. + throw new InitException("Could not initialize local repo keystore: " + e); + } + } + if (!keyStoreFile.exists()) { + // If there isn't a persisted BKS keystore on disk we need to + // create a new empty keystore // Init a new keystore with a blank passphrase + Log.d(TAG, "Keystore doesn't exist, creating..."); keyStore.load(null, "".toCharArray()); - } else { - keyStore.load(new FileInputStream(keyStoreFile), "".toCharArray()); } /* @@ -113,17 +138,23 @@ public class LocalRepoKeyStore { wrappedKeyManager }; } catch (UnrecoverableKeyException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } catch (KeyStoreException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } catch (NoSuchAlgorithmException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } catch (CertificateException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } catch (OperatorCreationException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } } @@ -144,7 +175,8 @@ public class LocalRepoKeyStore { FDroidApp.ipAddressString); addToStore(HTTP_CERT_ALIAS, kerplappKeypair, indexCert); } catch (Exception e) { - e.printStackTrace(); + Log.e(TAG, "Failed to setup HTTPS certificate: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } } @@ -174,21 +206,29 @@ public class LocalRepoKeyStore { zipSigner.signZip(input.getAbsolutePath(), output.getAbsolutePath()); } catch (ClassNotFoundException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (IllegalAccessException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (InstantiationException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (KeyStoreException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (UnrecoverableKeyException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (NoSuchAlgorithmException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (GeneralSecurityException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } } @@ -217,11 +257,14 @@ public class LocalRepoKeyStore { if (key instanceof PrivateKey) return keyStore.getCertificate(INDEX_CERT_ALIAS); } catch (KeyStoreException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to get certificate for local repo: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (UnrecoverableKeyException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to get certificate for local repo: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (NoSuchAlgorithmException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to get certificate for local repo: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } return null; } diff --git a/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java b/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java index cdd9faa97..165842d65 100644 --- a/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java +++ b/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java @@ -185,7 +185,8 @@ public class LocalRepoManager { symlinkIndexPageElsewhere("../../", repoCAPS); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Error writing local repo index: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } } @@ -249,13 +250,16 @@ public class LocalRepoManager { PackageInfo packageInfo = pm.getPackageInfo(packageName, PackageManager.GET_META_DATA); app.icon = getIconFile(packageName, packageInfo.versionCode).getName(); } catch (NameNotFoundException e) { - e.printStackTrace(); + Log.e(TAG, "Error adding app to local repo: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); return; } catch (CertificateEncodingException e) { - e.printStackTrace(); + Log.e(TAG, "Error adding app to local repo: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); return; } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Error adding app to local repo: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); return; } Log.i(TAG, "apps.put: " + packageName); @@ -317,7 +321,7 @@ public class LocalRepoManager { // TODO this needs to be ported to < android-8 @TargetApi(8) - private void writeIndexXML() throws TransformerException, ParserConfigurationException { + private void writeIndexXML() throws TransformerException, ParserConfigurationException, LocalRepoKeyStore.InitException { DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); DocumentBuilder builder = factory.newDocumentBuilder(); @@ -480,12 +484,11 @@ public class LocalRepoManager { writeIndexXML(); } catch (Exception e) { Toast.makeText(context, R.string.failed_to_create_index, Toast.LENGTH_LONG).show(); - e.printStackTrace(); + Log.e(TAG, Log.getStackTraceString(e)); return; } - BufferedOutputStream bo = new BufferedOutputStream( - new FileOutputStream(xmlIndexJarUnsigned)); + BufferedOutputStream bo = new BufferedOutputStream(new FileOutputStream(xmlIndexJarUnsigned)); JarOutputStream jo = new JarOutputStream(bo); BufferedInputStream bi = new BufferedInputStream(new FileInputStream(xmlIndex)); @@ -504,8 +507,13 @@ public class LocalRepoManager { jo.close(); bo.close(); - LocalRepoKeyStore.get(context).signZip(xmlIndexJarUnsigned, xmlIndexJar); + try { + LocalRepoKeyStore.get(context).signZip(xmlIndexJarUnsigned, xmlIndexJar); + } catch (LocalRepoKeyStore.InitException e) { + throw new IOException("Could not sign index - keystore failed to initialize"); + } finally { + xmlIndexJarUnsigned.delete(); + } - xmlIndexJarUnsigned.delete(); } } diff --git a/src/org/fdroid/fdroid/localrepo/LocalRepoService.java b/src/org/fdroid/fdroid/localrepo/LocalRepoService.java index a8ed9f59b..fa31666a0 100644 --- a/src/org/fdroid/fdroid/localrepo/LocalRepoService.java +++ b/src/org/fdroid/fdroid/localrepo/LocalRepoService.java @@ -23,6 +23,7 @@ import org.fdroid.fdroid.FDroidApp; import org.fdroid.fdroid.Preferences; import org.fdroid.fdroid.Preferences.ChangeListener; import org.fdroid.fdroid.R; +import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.net.LocalHTTPD; import org.fdroid.fdroid.net.WifiStateChangeService; import org.fdroid.fdroid.views.swap.SwapActivity; @@ -226,7 +227,8 @@ public class LocalRepoService extends Service { Log.w(TAG, "port " + prev + " occupied, trying on " + FDroidApp.port + "!"); startService(new Intent(LocalRepoService.this, WifiStateChangeService.class)); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Could not start local repo HTTP server: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } Looper.loop(); // start the message receiving loop } @@ -279,7 +281,8 @@ public class LocalRepoService extends Service { jmdns = JmDNS.create(); jmdns.registerService(pairService); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Error while registering jmdns service: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } } }).start(); @@ -300,11 +303,7 @@ public class LocalRepoService extends Service { pairService = null; } jmdns.unregisterAllServices(); - try { - jmdns.close(); - } catch (IOException e) { - e.printStackTrace(); - } + Utils.closeQuietly(jmdns); jmdns = null; } } diff --git a/src/org/fdroid/fdroid/net/LocalHTTPD.java b/src/org/fdroid/fdroid/net/LocalHTTPD.java index 1eaa2e67d..b5fd420f0 100644 --- a/src/org/fdroid/fdroid/net/LocalHTTPD.java +++ b/src/org/fdroid/fdroid/net/LocalHTTPD.java @@ -92,7 +92,11 @@ public class LocalHTTPD extends NanoHTTPD { localRepoKeyStore.getKeyManagers()); makeSecure(factory); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Could not enable HTTPS: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); + } catch (LocalRepoKeyStore.InitException e) { + Log.e(TAG, "Could not enable HTTPS: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } } diff --git a/src/org/fdroid/fdroid/net/WifiStateChangeService.java b/src/org/fdroid/fdroid/net/WifiStateChangeService.java index 6b4fda481..8c48da60e 100644 --- a/src/org/fdroid/fdroid/net/WifiStateChangeService.java +++ b/src/org/fdroid/fdroid/net/WifiStateChangeService.java @@ -82,9 +82,6 @@ public class WifiStateChangeService extends Service { return null; Context context = WifiStateChangeService.this.getApplicationContext(); - LocalRepoKeyStore localRepoKeyStore = LocalRepoKeyStore.get(context); - Certificate localCert = localRepoKeyStore.getCertificate(); - FDroidApp.repo.fingerprint = Utils.calcFingerprint(localCert); LocalRepoManager lrm = LocalRepoManager.get(context); lrm.setUriString(FDroidApp.repo.address); lrm.writeIndexPage(Utils.getSharingUri(context, FDroidApp.repo).toString()); @@ -92,17 +89,28 @@ public class WifiStateChangeService extends Service { if (isCancelled()) return null; - /* - * Once the IP address is known we need to generate a self - * signed certificate to use for HTTPS that has a CN field set - * to the ipAddressString. This must be run in the background - * because if this is the first time the singleton is run, it - * can take a while to instantiate. - */ - if (Preferences.get().isLocalRepoHttpsEnabled()) - localRepoKeyStore.setupHTTPSCertificate(); + try { + LocalRepoKeyStore localRepoKeyStore = LocalRepoKeyStore.get(context); + Certificate localCert = localRepoKeyStore.getCertificate(); + FDroidApp.repo.fingerprint = Utils.calcFingerprint(localCert); + + /* + * Once the IP address is known we need to generate a self + * signed certificate to use for HTTPS that has a CN field set + * to the ipAddressString. This must be run in the background + * because if this is the first time the singleton is run, it + * can take a while to instantiate. + */ + if (Preferences.get().isLocalRepoHttpsEnabled()) + localRepoKeyStore.setupHTTPSCertificate(); + + } catch (LocalRepoKeyStore.InitException e) { + Log.e(TAG, "Unable to configure a fingerprint or HTTPS for the local repo: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); + } + } catch (InterruptedException e) { - e.printStackTrace(); + Log.e(TAG, Log.getStackTraceString(e)); } return null; }