From 157b1e242f067fa778a70bb8747b14e2f3ed7703 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Thu, 7 May 2015 16:52:57 -0400 Subject: [PATCH] remove support for unsigned repos This has been discussed quite a bit now. It is very easy to generate a signed repo on the server, and supporting unsigned repos adds complexity and security issues, including "BZ-01-002 TOFU Requests too easy to recognize and intercept" from the audit. https://gitlab.com/fdroid/fdroidserver/merge_requests/48 closes #12 https://gitlab.com/fdroid/fdroidclient/issues/12 --- .../src/org/fdroid/fdroid/UpdateService.java | 2 +- .../fdroid/fdroid/updater/RepoUpdater.java | 142 ++++++++++++++-- .../fdroid/updater/SignedRepoUpdater.java | 153 ------------------ .../fdroid/updater/UnsignedRepoUpdater.java | 28 ---- ...oUpdaterTest.java => RepoUpdaterTest.java} | 6 +- 5 files changed, 131 insertions(+), 200 deletions(-) delete mode 100644 F-Droid/src/org/fdroid/fdroid/updater/SignedRepoUpdater.java delete mode 100644 F-Droid/src/org/fdroid/fdroid/updater/UnsignedRepoUpdater.java rename F-Droid/test/src/org/fdroid/fdroid/updater/{SignedRepoUpdaterTest.java => RepoUpdaterTest.java} (96%) diff --git a/F-Droid/src/org/fdroid/fdroid/UpdateService.java b/F-Droid/src/org/fdroid/fdroid/UpdateService.java index 8b36429dc..5e87f38fa 100644 --- a/F-Droid/src/org/fdroid/fdroid/UpdateService.java +++ b/F-Droid/src/org/fdroid/fdroid/UpdateService.java @@ -376,7 +376,7 @@ public class UpdateService extends IntentService implements ProgressListener { } sendStatus(STATUS_INFO, getString(R.string.status_connecting_to_repo, repo.address)); - RepoUpdater updater = RepoUpdater.createUpdaterFor(getBaseContext(), repo); + RepoUpdater updater = new RepoUpdater(getBaseContext(), repo); updater.setProgressListener(this); try { updater.update(); diff --git a/F-Droid/src/org/fdroid/fdroid/updater/RepoUpdater.java b/F-Droid/src/org/fdroid/fdroid/updater/RepoUpdater.java index 9f5c84614..4101eedca 100644 --- a/F-Droid/src/org/fdroid/fdroid/updater/RepoUpdater.java +++ b/F-Droid/src/org/fdroid/fdroid/updater/RepoUpdater.java @@ -2,9 +2,12 @@ package org.fdroid.fdroid.updater; import android.content.ContentValues; import android.content.Context; +import android.content.pm.PackageManager; import android.os.Bundle; import android.util.Log; +import org.fdroid.fdroid.FDroidApp; +import org.fdroid.fdroid.Hasher; import org.fdroid.fdroid.ProgressListener; import org.fdroid.fdroid.RepoXMLHandler; import org.fdroid.fdroid.Utils; @@ -20,31 +23,29 @@ import org.xml.sax.XMLReader; import java.io.BufferedReader; import java.io.File; +import java.io.FileOutputStream; import java.io.FileReader; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.security.cert.Certificate; import java.util.ArrayList; import java.util.Date; import java.util.List; +import java.util.jar.JarEntry; +import java.util.jar.JarFile; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; -abstract public class RepoUpdater { +public class RepoUpdater { private static final String TAG = "fdroid.RepoUpdater"; public static final String PROGRESS_TYPE_PROCESS_XML = "processingXml"; - public static final String PROGRESS_DATA_REPO_ADDRESS = "repoAddress"; - public static RepoUpdater createUpdaterFor(Context ctx, Repo repo) { - if (repo.fingerprint == null && repo.pubkey == null) { - return new UnsignedRepoUpdater(ctx, repo); - } - return new SignedRepoUpdater(ctx, repo); - } - protected final Context context; protected final Repo repo; private List apps = new ArrayList<>(); @@ -70,15 +71,47 @@ abstract public class RepoUpdater { public List getApks() { return apks; } /** - * For example, you may want to unzip a jar file to get the index inside, - * or if the file is not compressed, you can just return a reference to - * the downloaded file. + * All repos are represented by a signed jar file, {@code index.jar}, which contains + * a single file, {@code index.xml}. This takes the {@code index.jar}, verifies the + * signature, then returns the unzipped {@code index.xml}. * * @throws UpdateException All error states will come from here. */ - protected abstract File getIndexFromFile(File downloadedFile) throws UpdateException; + protected File getIndexFromFile(File downloadedFile) throws UpdateException { + final Date updateTime = new Date(System.currentTimeMillis()); + Log.d(TAG, "Getting signed index from " + repo.address + " at " + + Utils.formatLogDate(updateTime)); - protected abstract String getIndexAddress(); + final File indexJar = downloadedFile; + File indexXml = null; + + // 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()) { + + // 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; + } + + protected String getIndexAddress() { + try { + String versionName = context.getPackageManager().getPackageInfo(context.getPackageName(), 0).versionName; + return repo.address + "/index.jar?client_version=" + versionName; + } catch (PackageManager.NameNotFoundException e) { + e.printStackTrace(); + } + return repo.address + "/index.jar"; + } protected Downloader downloadIndex() throws UpdateException { Downloader downloader = null; @@ -128,7 +161,6 @@ abstract public class RepoUpdater { return count; } - public void update() throws UpdateException { File downloadedFile = null; @@ -261,4 +293,84 @@ abstract public class RepoUpdater { } } + private boolean verifyCerts(JarEntry item) throws UpdateException { + final Certificate[] certs = item.getCertificates(); + if (certs == null || certs.length == 0) { + throw new UpdateException(repo, "No signature found in index"); + } + + Log.d(TAG, "Index has " + certs.length + " signature(s)"); + boolean match = false; + for (final Certificate cert : certs) { + String certdata = Hasher.hex(cert); + if (repo.pubkey == null && repo.fingerprint != null) { + String certFingerprint = Utils.calcFingerprint(cert); + Log.d(TAG, "No public key for repo " + repo.address + " yet, but it does have a fingerprint, so comparing them."); + Log.d(TAG, "Repo fingerprint: " + repo.fingerprint); + Log.d(TAG, "Cert fingerprint: " + certFingerprint); + if (repo.fingerprint.equalsIgnoreCase(certFingerprint)) { + repo.pubkey = certdata; + usePubkeyInJar = true; + } + } + if (repo.pubkey != null && repo.pubkey.equals(certdata)) { + Log.d(TAG, "Checking repo public key against cert found in jar."); + match = true; + break; + } + } + return match; + } + + protected File extractIndexFromJar(File indexJar) throws UpdateException { + File indexFile = null; + JarFile jarFile = null; + try { + jarFile = new JarFile(indexJar, true); + JarEntry indexEntry = (JarEntry) jarFile.getEntry("index.xml"); + + indexFile = File.createTempFile("index-", "-extracted.xml", context.getCacheDir()); + InputStream input = null; + OutputStream output = null; + try { + /* + * JarFile.getInputStream() provides the signature check, even + * though the Android docs do not mention this, the Java docs do + * and Android seems to implement it the same: + * http://docs.oracle.com/javase/6/docs/api/java/util/jar/JarFile.html#getInputStream(java.util.zip.ZipEntry) + * https://developer.android.com/reference/java/util/jar/JarFile.html#getInputStream(java.util.zip.ZipEntry) + */ + input = jarFile.getInputStream(indexEntry); + output = new FileOutputStream(indexFile); + Utils.copy(input, output); + } finally { + Utils.closeQuietly(output); + Utils.closeQuietly(input); + } + + // Can only read certificates from jar after it has been read + // completely, so we put it after the copy above... + if (!verifyCerts(indexEntry)) { + indexFile.delete(); + throw new UpdateException(repo, "Index signature mismatch"); + } + } catch (IOException e) { + if (indexFile != null) { + indexFile.delete(); + } + throw new UpdateException( + repo, "Error opening signed index", e); + } finally { + if (jarFile != null) { + try { + jarFile.close(); + } catch (IOException ioe) { + // ignore + } + } + } + + return indexFile; + } + } diff --git a/F-Droid/src/org/fdroid/fdroid/updater/SignedRepoUpdater.java b/F-Droid/src/org/fdroid/fdroid/updater/SignedRepoUpdater.java deleted file mode 100644 index 142c5d102..000000000 --- a/F-Droid/src/org/fdroid/fdroid/updater/SignedRepoUpdater.java +++ /dev/null @@ -1,153 +0,0 @@ -package org.fdroid.fdroid.updater; - -import android.content.Context; -import android.content.pm.PackageManager; -import android.util.Log; - -import org.fdroid.fdroid.FDroidApp; -import org.fdroid.fdroid.Hasher; -import org.fdroid.fdroid.R; -import org.fdroid.fdroid.Utils; -import org.fdroid.fdroid.data.Repo; - -import java.io.File; -import java.io.FileOutputStream; -import java.io.IOException; -import java.io.InputStream; -import java.io.OutputStream; -import java.security.cert.Certificate; -import java.util.Date; -import java.util.jar.JarEntry; -import java.util.jar.JarFile; - -public class SignedRepoUpdater extends RepoUpdater { - - private static final String TAG = "fdroid.SignedRepoUpdater"; - - public SignedRepoUpdater(Context ctx, Repo repo) { - super(ctx, repo); - } - - private boolean verifyCerts(JarEntry item) throws UpdateException { - final Certificate[] certs = item.getCertificates(); - if (certs == null || certs.length == 0) { - throw new UpdateException(repo, "No signature found in index"); - } - - Log.d(TAG, "Index has " + certs.length + " signature(s)"); - boolean match = false; - for (final Certificate cert : certs) { - String certdata = Hasher.hex(cert); - if (repo.pubkey == null && repo.fingerprint != null) { - String certFingerprint = Utils.calcFingerprint(cert); - Log.d(TAG, "No public key for repo " + repo.address + " yet, but it does have a fingerprint, so comparing them."); - Log.d(TAG, "Repo fingerprint: " + repo.fingerprint); - Log.d(TAG, "Cert fingerprint: " + certFingerprint); - if (repo.fingerprint.equalsIgnoreCase(certFingerprint)) { - repo.pubkey = certdata; - usePubkeyInJar = true; - } - } - if (repo.pubkey != null && repo.pubkey.equals(certdata)) { - Log.d(TAG, "Checking repo public key against cert found in jar."); - match = true; - break; - } - } - return match; - } - - protected File extractIndexFromJar(File indexJar) throws UpdateException { - File indexFile = null; - JarFile jarFile = null; - try { - jarFile = new JarFile(indexJar, true); - JarEntry indexEntry = (JarEntry)jarFile.getEntry("index.xml"); - - indexFile = File.createTempFile("index-", "-extracted.xml", context.getCacheDir()); - InputStream input = null; - OutputStream output = null; - try { - /* - * JarFile.getInputStream() provides the signature check, even - * though the Android docs do not mention this, the Java docs do - * and Android seems to implement it the same: - * http://docs.oracle.com/javase/6/docs/api/java/util/jar/JarFile.html#getInputStream(java.util.zip.ZipEntry) - * https://developer.android.com/reference/java/util/jar/JarFile.html#getInputStream(java.util.zip.ZipEntry) - */ - input = jarFile.getInputStream(indexEntry); - output = new FileOutputStream(indexFile); - Utils.copy(input, output); - } finally { - Utils.closeQuietly(output); - Utils.closeQuietly(input); - } - - // Can only read certificates from jar after it has been read - // completely, so we put it after the copy above... - if (!verifyCerts(indexEntry)) { - indexFile.delete(); - throw new UpdateException(repo, "Index signature mismatch"); - } - } catch (IOException e) { - if (indexFile != null) { - indexFile.delete(); - } - throw new UpdateException( - repo, "Error opening signed index", e); - } finally { - if (jarFile != null) { - try { - jarFile.close(); - } catch (IOException ioe) { - // ignore - } - } - } - - return indexFile; - } - - @Override - protected String getIndexAddress() { - try { - String versionName = context.getPackageManager().getPackageInfo(context.getPackageName(), 0).versionName; - return repo.address + "/index.jar?client_version=" + versionName; - } catch (PackageManager.NameNotFoundException e) { - e.printStackTrace(); - } - return repo.address + "/index.jar"; - } - - /** - * As this is a signed repo - we download the jar file, - * check the signature, and extract the index file - */ - @Override - protected File getIndexFromFile(File downloadedFile) throws - UpdateException { - final Date updateTime = new Date(System.currentTimeMillis()); - Log.d(TAG, "Getting signed index from " + repo.address + " at " + - Utils.formatLogDate(updateTime)); - - final File indexJar = downloadedFile; - File indexXml = null; - - // 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()) { - - // 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; - } -} diff --git a/F-Droid/src/org/fdroid/fdroid/updater/UnsignedRepoUpdater.java b/F-Droid/src/org/fdroid/fdroid/updater/UnsignedRepoUpdater.java deleted file mode 100644 index b3b4503bd..000000000 --- a/F-Droid/src/org/fdroid/fdroid/updater/UnsignedRepoUpdater.java +++ /dev/null @@ -1,28 +0,0 @@ -package org.fdroid.fdroid.updater; - -import android.content.Context; -import android.util.Log; - -import org.fdroid.fdroid.data.Repo; - -import java.io.File; - -public class UnsignedRepoUpdater extends RepoUpdater { - - private static final String TAG = "fdroid.UnsignedRepoUpdater"; - - public UnsignedRepoUpdater(Context ctx, Repo repo) { - super(ctx, repo); - } - - @Override - protected File getIndexFromFile(File file) throws UpdateException { - Log.d(TAG, "Getting unsigned index from " + getIndexAddress()); - return file; - } - - @Override - protected String getIndexAddress() { - return repo.address + "/index.xml"; - } -} diff --git a/F-Droid/test/src/org/fdroid/fdroid/updater/SignedRepoUpdaterTest.java b/F-Droid/test/src/org/fdroid/fdroid/updater/RepoUpdaterTest.java similarity index 96% rename from F-Droid/test/src/org/fdroid/fdroid/updater/SignedRepoUpdaterTest.java rename to F-Droid/test/src/org/fdroid/fdroid/updater/RepoUpdaterTest.java index da1602faa..40f4b4de0 100644 --- a/F-Droid/test/src/org/fdroid/fdroid/updater/SignedRepoUpdaterTest.java +++ b/F-Droid/test/src/org/fdroid/fdroid/updater/RepoUpdaterTest.java @@ -14,8 +14,8 @@ import java.io.File; import java.io.IOException; @TargetApi(8) -public class SignedRepoUpdaterTest extends InstrumentationTestCase { - private static final String TAG = "SignedRepoUpdaterTest"; +public class RepoUpdaterTest extends InstrumentationTestCase { + private static final String TAG = "RepoUpdaterTest"; private Context context; private RepoUpdater repoUpdater; @@ -29,7 +29,7 @@ public class SignedRepoUpdaterTest extends InstrumentationTestCase { testFilesDir = TestUtils.getWriteableDir(getInstrumentation()); Repo repo = new Repo(); repo.pubkey = this.simpleIndexPubkey; - repoUpdater = RepoUpdater.createUpdaterFor(context, repo); + repoUpdater = new RepoUpdater(context, repo); } public void testExtractIndexFromJar() {