diff --git a/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java b/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java index ad0eda202..0b6e3a0d0 100644 --- a/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java +++ b/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java @@ -18,13 +18,11 @@ import org.xml.sax.InputSource; import org.xml.sax.SAXException; import org.xml.sax.XMLReader; -import java.io.BufferedReader; +import java.io.BufferedInputStream; 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.CodeSigner; import java.security.cert.Certificate; import java.util.ArrayList; import java.util.Date; @@ -48,13 +46,18 @@ public class RepoUpdater { private List apps = new ArrayList<>(); private List apks = new ArrayList<>(); private RepoUpdateRememberer rememberer = null; - protected boolean usePubkeyInJar = false; protected boolean hasChanged = false; @Nullable protected ProgressListener progressListener; - public RepoUpdater(@NonNull Context ctx, @NonNull Repo repo) { - this.context = ctx; - this.repo = repo; + /** + * Updates an app repo as read out of the database into a {@link Repo} instance. + * + * @param context + * @param repo a {@link Repo} read out of the local database + */ + public RepoUpdater(@NonNull Context context, @NonNull Repo repo) { + this.context = context; + this.repo = repo; } public void setProgressListener(ProgressListener progressListener) { @@ -107,86 +110,74 @@ public class RepoUpdater { return downloader; } - private int estimateAppCount(File indexFile) { - int count = -1; - try { - // A bit of a hack, this might return false positives if an apps description - // or some other part of the XML file contains this, but it is a pretty good - // estimate and makes the progress counter more informative. - // As with asking the server about the size of the index before downloading, - // this also has a time tradeoff. It takes about three seconds to iterate - // through the file and count 600 apps on a slow emulator (v17), but if it is - // taking two minutes to update, the three second wait may be worth it. - final String APPLICATION = " 1) { + throw new UpdateException(repo, "index.jar must be signed by a single code signer!"); + } + List certs = codeSigners[0].getSignerCertPath().getCertificates(); + if (certs.size() != 1) { + throw new UpdateException(repo, "index.jar code signers must only have a single certificate!"); + } + Certificate cert = certs.get(0); - 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; + // though its called repo.pubkey, its actually a X509 certificate + String pubkey = Hasher.hex(cert); + + /* The first time a repo is added, it can be added with the signing key's + fingerprint. In that case, check that fingerprint against what is + actually in the index.jar itself */ + if (repo.pubkey == null && repo.fingerprint != null) { + String certFingerprint = Utils.calcFingerprint(cert); + if (repo.fingerprint.equalsIgnoreCase(certFingerprint)) { + storePubKey = true; + } else { + throw new UpdateException(repo, "Supplied certificate fingerprint does not match!"); } } - return match; - } - - /** - * 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 { - // 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()); - 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 { - FDroidApp.enableSpongyCastleOnLollipop(); - if (jarFile != null) { - try { - jarFile.close(); - } catch (IOException ioe) { - // ignore - } - } + /* This storePubKey business makes me uncomfortable, but there seems no way around it + * since writing the pubkey to the database happens far from here in RepoUpdateRememberer */ + if (storePubKey) { + repo.pubkey = pubkey; + return; + } else if (repo.pubkey != null && repo.pubkey.equals(pubkey)) { + return; // we have a match! } - - return indexFile; + throw new UpdateException(repo, "Signing certificate does not match!"); } } diff --git a/F-Droid/src/org/fdroid/fdroid/RepoXMLHandler.java b/F-Droid/src/org/fdroid/fdroid/RepoXMLHandler.java index 8655f9edc..bf2853dc4 100644 --- a/F-Droid/src/org/fdroid/fdroid/RepoXMLHandler.java +++ b/F-Droid/src/org/fdroid/fdroid/RepoXMLHandler.java @@ -32,6 +32,9 @@ import org.xml.sax.helpers.DefaultHandler; import java.util.ArrayList; import java.util.List; +/** + * Parses the index.xml into Java data structures. + */ public class RepoXMLHandler extends DefaultHandler { // The repo we're processing. @@ -49,27 +52,18 @@ public class RepoXMLHandler extends DefaultHandler { private int version = -1; private int maxage = -1; - // After processing the XML, this will be null if the index specified a - // public key - otherwise a public key. This is used for TOFU where an - // index.xml is read on the first connection, and a signed index.jar is - // expected on all subsequent connections. + /** the pubkey stored in the header of index.xml */ private String pubkey; private String name; private String description; private String hashType; - private int progressCounter = 0; - private final ProgressListener progressListener; - - private int totalAppCount; - - public RepoXMLHandler(Repo repo, ProgressListener listener) { + public RepoXMLHandler(Repo repo) { this.repo = repo; pubkey = null; name = null; description = null; - progressListener = listener; } public List getApps() { return apps; } @@ -265,16 +259,6 @@ public class RepoXMLHandler extends DefaultHandler { } else if (localName.equals("application") && curapp == null) { curapp = new App(); curapp.id = attributes.getValue("", "id"); - /* show progress for the first 25, then start skipping every 25 */ - if (totalAppCount < 25 || progressCounter % (totalAppCount / 25) == 0) { - Bundle data = new Bundle(1); - data.putString(RepoUpdater.PROGRESS_DATA_REPO_ADDRESS, repo.address); - progressListener.onProgress( - new ProgressListener.Event( - RepoUpdater.PROGRESS_TYPE_PROCESS_XML, - progressCounter, totalAppCount, data)); - } - progressCounter++; } else if (localName.equals("package") && curapp != null && curapk == null) { curapk = new Apk(); curapk.id = curapp.id; @@ -287,10 +271,6 @@ public class RepoXMLHandler extends DefaultHandler { curchars.setLength(0); } - public void setTotalAppCount(int totalAppCount) { - this.totalAppCount = totalAppCount; - } - private String cleanWhiteSpace(String str) { return str.replaceAll("\n", " ").replaceAll(" ", " "); } diff --git a/F-Droid/src/org/fdroid/fdroid/Utils.java b/F-Droid/src/org/fdroid/fdroid/Utils.java index 42cd96ef3..cce9d61cd 100644 --- a/F-Droid/src/org/fdroid/fdroid/Utils.java +++ b/F-Droid/src/org/fdroid/fdroid/Utils.java @@ -45,7 +45,6 @@ import java.io.Closeable; import java.io.File; import java.io.FileInputStream; import java.io.FileOutputStream; -import java.io.FileReader; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -260,36 +259,6 @@ public final class Utils { return getMinMaxSdkVersion(context, packageName, "maxSdkVersion"); } - public static int countSubstringOccurrence(File file, String substring) throws IOException { - int count = 0; - FileReader input = null; - try { - int currentSubstringIndex = 0; - char[] buffer = new char[4096]; - - input = new FileReader(file); - int numRead = input.read(buffer); - while(numRead != -1) { - - for (char c : buffer) { - if (c == substring.charAt(currentSubstringIndex)) { - currentSubstringIndex++; - if (currentSubstringIndex == substring.length()) { - count++; - currentSubstringIndex = 0; - } - } else { - currentSubstringIndex = 0; - } - } - numRead = input.read(buffer); - } - } finally { - closeQuietly(input); - } - return count; - } - // return a fingerprint formatted for display public static String formatFingerprint(Context context, String fingerprint) { if (TextUtils.isEmpty(fingerprint) diff --git a/F-Droid/test/src/org/fdroid/fdroid/RepoUpdaterTest.java b/F-Droid/test/src/org/fdroid/fdroid/RepoUpdaterTest.java index e441fcbd0..1c5a95999 100644 --- a/F-Droid/test/src/org/fdroid/fdroid/RepoUpdaterTest.java +++ b/F-Droid/test/src/org/fdroid/fdroid/RepoUpdaterTest.java @@ -5,12 +5,11 @@ import android.annotation.TargetApi; import android.content.Context; import android.test.InstrumentationTestCase; -import org.apache.commons.io.FileUtils; import org.fdroid.fdroid.RepoUpdater.UpdateException; import org.fdroid.fdroid.data.Repo; import java.io.File; -import java.io.IOException; +import java.util.UUID; @TargetApi(8) public class RepoUpdaterTest extends InstrumentationTestCase { @@ -34,17 +33,12 @@ public class RepoUpdaterTest extends InstrumentationTestCase { public void testExtractIndexFromJar() { if (!testFilesDir.canWrite()) return; - File simpleIndexXml = TestUtils.copyAssetToDir(context, "simpleIndex.xml", testFilesDir); File simpleIndexJar = TestUtils.copyAssetToDir(context, "simpleIndex.jar", testFilesDir); - File testFile = null; // these are supposed to succeed try { - testFile = repoUpdater.getIndexFromJar(simpleIndexJar); - assertTrue(testFile.length() == simpleIndexXml.length()); - assertEquals(FileUtils.readFileToString(testFile), - FileUtils.readFileToString(simpleIndexXml)); - } catch (IOException | UpdateException e) { + repoUpdater.processDownloadedFile(simpleIndexJar, UUID.randomUUID().toString()); + } catch (UpdateException e) { e.printStackTrace(); fail(); } @@ -55,7 +49,8 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "simpleIndexWithoutSignature.jar", testFilesDir)); + File jarFile = TestUtils.copyAssetToDir(context, "simpleIndexWithoutSignature.jar", testFilesDir); + repoUpdater.processDownloadedFile(jarFile, UUID.randomUUID().toString()); fail(); } catch (UpdateException e) { // success! @@ -67,7 +62,8 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedManifest.jar", testFilesDir)); + File jarFile = TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedManifest.jar", testFilesDir); + repoUpdater.processDownloadedFile(jarFile, UUID.randomUUID().toString()); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -82,7 +78,8 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedSignature.jar", testFilesDir)); + File jarFile = TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedSignature.jar", testFilesDir); + repoUpdater.processDownloadedFile(jarFile, UUID.randomUUID().toString()); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -97,7 +94,8 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedCertificate.jar", testFilesDir)); + File jarFile = TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedCertificate.jar", testFilesDir); + repoUpdater.processDownloadedFile(jarFile, UUID.randomUUID().toString()); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -112,7 +110,8 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedEverything.jar", testFilesDir)); + File jarFile = TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedEverything.jar", testFilesDir); + repoUpdater.processDownloadedFile(jarFile, UUID.randomUUID().toString()); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -127,7 +126,8 @@ public class RepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromJar(TestUtils.copyAssetToDir(context, "masterKeyIndex.jar", testFilesDir)); + File jarFile = TestUtils.copyAssetToDir(context, "masterKeyIndex.jar", testFilesDir); + repoUpdater.processDownloadedFile(jarFile, UUID.randomUUID().toString()); fail(); } catch (UpdateException | SecurityException e) { // success!