From d7efc99bdbc0bec9aecfb27f5905b4a1aa13d43e Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 15 Jun 2015 17:39:55 -0400 Subject: [PATCH] simplify RepoUpdater to remove cruft from previous code structure Before, there was an abstract RepoUpdater class with two subclasses, one for signed and unsigned. Now there is just a single class, and it only ever starts with the index.jar. So this removes lots of code that was there to handle that more complicated structure. For example, there is no longer the need to separately work on the index.xml vs index.jar. --- .../src/org/fdroid/fdroid/RepoUpdater.java | 89 +++++++------------ .../org/fdroid/fdroid/RepoUpdaterTest.java | 14 +-- 2 files changed, 40 insertions(+), 63 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java b/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java index 599ca7a41..ad0eda202 100644 --- a/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java +++ b/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java @@ -6,7 +6,6 @@ import android.content.pm.PackageManager; import android.os.Bundle; import android.support.annotation.NonNull; import android.support.annotation.Nullable; -import android.text.TextUtils; import android.util.Log; import org.fdroid.fdroid.data.Apk; @@ -68,39 +67,6 @@ public class RepoUpdater { public List getApks() { return apks; } - /** - * 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 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; - } - protected String getIndexAddress() { try { String versionName = context.getPackageManager().getPackageInfo(context.getPackageName(), 0).versionName; @@ -111,7 +77,7 @@ public class RepoUpdater { return repo.address + "/index.jar"; } - protected Downloader downloadIndex() throws UpdateException { + Downloader downloadIndex() throws UpdateException { Downloader downloader = null; try { downloader = DownloaderFactory.create( @@ -170,8 +136,17 @@ public class RepoUpdater { if (hasChanged) { + // 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: downloadedFile = downloader.getFile(); - indexFile = getIndexFromFile(downloadedFile); + + if (downloadedFile != null && downloadedFile.exists()) { + indexFile = getIndexFromJar(downloadedFile); + } + + if (indexXmlFile == null || !indexXmlFile.canRead()) { + throw new UpdateException(repo, "Failed to get index for " + repo.address); + } // Process the index... final SAXParser parser = SAXParserFactory.newInstance().newSAXParser(); @@ -200,7 +175,7 @@ public class RepoUpdater { } catch (SAXException | ParserConfigurationException | IOException e) { throw new UpdateException(repo, "Error parsing index for repo " + repo.address, e); } finally { - if (downloadedFile != null && downloadedFile != indexFile && downloadedFile.exists()) { + if (downloadedFile != null && downloadedFile.exists()) { downloadedFile.delete(); } if (indexFile != null && indexFile.exists()) { @@ -312,11 +287,27 @@ public class RepoUpdater { return match; } - protected File extractIndexFromJar(File indexJar) throws UpdateException { + /** + * 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 File getIndexFromJar(File f) throws UpdateException { + final Date updateTime = new Date(System.currentTimeMillis()); + Log.d(TAG, "Getting signed index from " + repo.address + " at " + + Utils.formatLogDate(updateTime)); + File indexFile = null; JarFile jarFile = null; try { - jarFile = new JarFile(indexJar, true); + // 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. + FDroidApp.disableSpongyCastleOnLollipop(); + + jarFile = new JarFile(f, true); JarEntry indexEntry = (JarEntry) jarFile.getEntry("index.xml"); indexFile = File.createTempFile("index-", "-extracted.xml", context.getCacheDir()); @@ -340,11 +331,7 @@ public class RepoUpdater { // Can only read certificates from jar after it has been read // completely, so we put it after the copy above... - if (isTofuRequest()) { - Log.i(TAG, "Implicitly trusting the signature of index.jar, because this is a TOFU request"); - // Note that later on in the process we will save the pubkey against they repo, so - // that future requests verify against the signature we got this time. - } else if (!verifyCerts(indexEntry)) { + if (!verifyCerts(indexEntry)) { indexFile.delete(); throw new UpdateException(repo, "Index signature mismatch"); } @@ -352,9 +339,9 @@ public class RepoUpdater { if (indexFile != null) { indexFile.delete(); } - throw new UpdateException( - repo, "Error opening signed index", e); + throw new UpdateException(repo, "Error opening signed index", e); } finally { + FDroidApp.enableSpongyCastleOnLollipop(); if (jarFile != null) { try { jarFile.close(); @@ -367,14 +354,4 @@ public class RepoUpdater { return indexFile; } - /** - * If the repo doesn't have a fingerprint, then this is a "Trust On First Use" (TOFU) - * request. In that case, we will not verify the certificate, but rather implicitly trust - * the file we downloaded. We'll extract the certificate from the jar, and then use that - * to verify future requests to the same repository. - */ - private boolean isTofuRequest() { - return TextUtils.isEmpty(repo.fingerprint); - } - } diff --git a/F-Droid/test/src/org/fdroid/fdroid/RepoUpdaterTest.java b/F-Droid/test/src/org/fdroid/fdroid/RepoUpdaterTest.java index 96b2ae6f5..e441fcbd0 100644 --- a/F-Droid/test/src/org/fdroid/fdroid/RepoUpdaterTest.java +++ b/F-Droid/test/src/org/fdroid/fdroid/RepoUpdaterTest.java @@ -40,7 +40,7 @@ public class RepoUpdaterTest extends InstrumentationTestCase { // these are supposed to succeed try { - testFile = repoUpdater.getIndexFromFile(simpleIndexJar); + testFile = repoUpdater.getIndexFromJar(simpleIndexJar); assertTrue(testFile.length() == simpleIndexXml.length()); assertEquals(FileUtils.readFileToString(testFile), FileUtils.readFileToString(simpleIndexXml)); @@ -55,7 +55,7 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "simpleIndexWithoutSignature.jar", testFilesDir)); + repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "simpleIndexWithoutSignature.jar", testFilesDir)); fail(); } catch (UpdateException e) { // success! @@ -67,7 +67,7 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedManifest.jar", testFilesDir)); + repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedManifest.jar", testFilesDir)); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -82,7 +82,7 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedSignature.jar", testFilesDir)); + repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedSignature.jar", testFilesDir)); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -97,7 +97,7 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedCertificate.jar", testFilesDir)); + repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedCertificate.jar", testFilesDir)); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -112,7 +112,7 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedEverything.jar", testFilesDir)); + repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedEverything.jar", testFilesDir)); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -127,7 +127,7 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "masterKeyIndex.jar", testFilesDir)); + repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "masterKeyIndex.jar", testFilesDir)); fail(); } catch (UpdateException | SecurityException e) { // success!