From 37b3f1ff57381ae4ae8b89e211e74ce1de2cc66c Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 26 Jan 2015 07:56:05 +1100 Subject: [PATCH 1/4] Cache .apks in internal storage before installing. This prevents an app with "write external storage" permission from being able to switch the legit app with a dodgey one between F-Droid requesting an install, and the package manager actually showing the install dialog to the user. In order to make the file in private internal storage readable by the package manager, its parent directories need to be world-executable, and the file itself needs to be world-readable. It seems that the "/data/data/org.fdroid.fdroid/cache" dir provided by the Context is already world executable, but the "apks" subdirectory does not default to this. Also, to be compatible with android-8, a Runtime.getRuntime().exec() call was added for such devices, which invokes /system/bin/chmod. The effect of this was to require some level of file sanitization to be made available using the Java type system to prevent command injection attacks from weird apk names (as people are free to download metadata from random internet people). --- F-Droid/src/org/fdroid/fdroid/Utils.java | 31 +++++++- .../org/fdroid/fdroid/compat/FileCompat.java | 70 +++++++++++++++++++ .../org/fdroid/fdroid/data/SanitizedFile.java | 15 ++++ .../org/fdroid/fdroid/net/ApkDownloader.java | 15 +++- .../org/fdroid/fdroid/SanitizedFileTest.java | 0 5 files changed, 126 insertions(+), 5 deletions(-) create mode 100644 F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java create mode 100644 F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java create mode 100644 F-Droid/test/src/org/fdroid/fdroid/SanitizedFileTest.java diff --git a/F-Droid/src/org/fdroid/fdroid/Utils.java b/F-Droid/src/org/fdroid/fdroid/Utils.java index 7358104bd..f345a8113 100644 --- a/F-Droid/src/org/fdroid/fdroid/Utils.java +++ b/F-Droid/src/org/fdroid/fdroid/Utils.java @@ -29,7 +29,10 @@ import android.text.TextUtils; import android.util.DisplayMetrics; import android.util.Log; import com.nostra13.universalimageloader.utils.StorageUtils; +import org.fdroid.fdroid.compat.FileCompat; +import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.Repo; +import org.fdroid.fdroid.data.SanitizedFile; import org.xml.sax.XMLReader; import org.xmlpull.v1.XmlPullParser; import org.xmlpull.v1.XmlPullParserException; @@ -125,6 +128,21 @@ public final class Utils { } } + /** + * Read the input stream until it reaches the end, ignoring any exceptions. + */ + public static void consumeStream(InputStream stream) { + final byte buffer[] = new byte[256]; + try { + int read; + do { + read = stream.read(buffer); + } while (read != -1); + } catch (IOException e) { + // Ignore... + } + } + public static boolean symlink(File inFile, File outFile) { int exitCode = -1; try { @@ -311,12 +329,21 @@ public final class Utils { return b.build(); } + /** + * The directory where .apk files are downloaded (and stored - if the relevant property is enabled). + * This must be on internal storage, to prevent other apps with "write external storage" from being + * able to change the .apk file between F-Droid requesting the Package Manger to install, and the + * Package Manager receiving that request. + */ public static File getApkCacheDir(Context context) { - File apkCacheDir = new File( - StorageUtils.getCacheDirectory(context, true), "apks"); + SanitizedFile apkCacheDir = new SanitizedFile(StorageUtils.getCacheDirectory(context, false), "apks"); if (!apkCacheDir.exists()) { apkCacheDir.mkdir(); } + + // All parent directories of the .apk file need to be executable for the package installer + // to be able to have permission to read our world-readable .apk files. + FileCompat.setExecutable(apkCacheDir, true, false); return apkCacheDir; } diff --git a/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java b/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java new file mode 100644 index 000000000..9784bc8f2 --- /dev/null +++ b/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java @@ -0,0 +1,70 @@ +package org.fdroid.fdroid.compat; + +import android.annotation.TargetApi; +import android.util.Log; +import org.fdroid.fdroid.Utils; +import org.fdroid.fdroid.data.SanitizedFile; + +import java.io.IOException; + +public class FileCompat { + + private static final String TAG = "org.fdroid.fdroid.compat.FileCompat"; + + @TargetApi(9) + public static boolean setReadable(SanitizedFile file, boolean readable, boolean ownerOnly) { + + if (Compatibility.hasApi(9)) { + return file.setReadable(readable, ownerOnly); + } else { + String mode; + if (readable) { + mode = ownerOnly ? "0600" : "0644"; + } else { + mode = "0000"; + } + return setMode(file, mode); + } + + } + + private static boolean setMode(SanitizedFile file, String mode) { + + // The "file" must be a sanitized file, and hence only contain A-Za-z0-9.-_ already, + // but it makes no assurances about the parent directory. + String[] args = { + "/system/bin/chmod", + mode, + file.getAbsolutePath() + }; + + try { + Log.d(TAG, "Executing following command: " + args[0] + " " + args[1] + " " + args[2]); + Process proc = Runtime.getRuntime().exec(args); + Utils.consumeStream(proc.getInputStream()); + Utils.consumeStream(proc.getErrorStream()); + return true; + } catch (IOException e) { + return false; + } + + } + + @TargetApi(9) + public static boolean setExecutable(SanitizedFile file, boolean readable, boolean ownerOnly) { + + if (Compatibility.hasApi(9)) { + return file.setExecutable(readable, ownerOnly); + } else { + String mode; + if ( readable ) { + mode = ownerOnly ? "0700" : "0711"; + } else { + mode = ownerOnly ? "0600" : "0600"; + } + return setMode( file, mode ); + } + + } + +} diff --git a/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java b/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java new file mode 100644 index 000000000..8e60837e6 --- /dev/null +++ b/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java @@ -0,0 +1,15 @@ +package org.fdroid.fdroid.data; + +import java.io.File; + +/** + * File guaranteed to have a santitized name (though not a sanitized path to the parent dir). + * Useful so that we can use Java's type system to enforce that the file we are accessing + * doesn't contain illegal characters. + * Sanitized names are those which only have the following characters: [A-Za-z0-9.-_] + */ +public class SanitizedFile extends File { + public SanitizedFile(File parent, String name) { + super(parent, name.replaceAll("[^A-Za-z0-9.-_]", "")); + } +} diff --git a/F-Droid/src/org/fdroid/fdroid/net/ApkDownloader.java b/F-Droid/src/org/fdroid/fdroid/net/ApkDownloader.java index c8c6bcc15..e989da6d6 100644 --- a/F-Droid/src/org/fdroid/fdroid/net/ApkDownloader.java +++ b/F-Droid/src/org/fdroid/fdroid/net/ApkDownloader.java @@ -25,7 +25,9 @@ import android.util.Log; import org.fdroid.fdroid.Hasher; import org.fdroid.fdroid.ProgressListener; +import org.fdroid.fdroid.compat.FileCompat; import org.fdroid.fdroid.data.Apk; +import org.fdroid.fdroid.data.SanitizedFile; import java.io.File; import java.io.IOException; @@ -59,7 +61,7 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { private Apk curApk; private String repoAddress; - private File localFile; + private SanitizedFile localFile; private ProgressListener listener; private AsyncDownloadWrapper dlWrapper = null; @@ -80,13 +82,13 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { public ApkDownloader(Apk apk, String repoAddress, File destDir) { curApk = apk; this.repoAddress = repoAddress; - localFile = new File(destDir, curApk.apkName); + localFile = new SanitizedFile(destDir, curApk.apkName); } /** * The downloaded APK. Valid only when getStatus() has returned STATUS.DONE. */ - public File localFile() { + public SanitizedFile localFile() { return localFile; } @@ -239,6 +241,13 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { return; } + // Need the apk to be world readable, so that the installer is able to read it. + // Note that saving it into external storage for the purpose of letting the installer + // have access is insecure, because apps with permission to write to the external + // storage can overwrite the app between F-Droid asking for it to be installed and + // the installer actually installing it. + FileCompat.setReadable(localFile, true, false); + Log.d("FDroid", "Download finished: " + localFile); sendCompleteMessage(); } diff --git a/F-Droid/test/src/org/fdroid/fdroid/SanitizedFileTest.java b/F-Droid/test/src/org/fdroid/fdroid/SanitizedFileTest.java new file mode 100644 index 000000000..e69de29bb From afef5ea233af9b54db94798f39a4b2aef7b3b764 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 26 Jan 2015 13:53:15 +1100 Subject: [PATCH 2/4] Added test for SanitizedFile class. --- .../org/fdroid/fdroid/data/SanitizedFile.java | 2 +- .../org/fdroid/fdroid/SanitizedFileTest.java | 44 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java b/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java index 8e60837e6..4a3ed7651 100644 --- a/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java +++ b/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java @@ -10,6 +10,6 @@ import java.io.File; */ public class SanitizedFile extends File { public SanitizedFile(File parent, String name) { - super(parent, name.replaceAll("[^A-Za-z0-9.-_]", "")); + super(parent, name.replaceAll("[^A-Za-z0-9-._]", "")); } } diff --git a/F-Droid/test/src/org/fdroid/fdroid/SanitizedFileTest.java b/F-Droid/test/src/org/fdroid/fdroid/SanitizedFileTest.java index e69de29bb..6cde747fc 100644 --- a/F-Droid/test/src/org/fdroid/fdroid/SanitizedFileTest.java +++ b/F-Droid/test/src/org/fdroid/fdroid/SanitizedFileTest.java @@ -0,0 +1,44 @@ +package org.fdroid.fdroid; + +import android.test.AndroidTestCase; +import org.fdroid.fdroid.data.SanitizedFile; + +import java.io.File; + +public class SanitizedFileTest extends AndroidTestCase { + + public void testSanitizedFile() { + + File directory = new File("/tmp/blah"); + + String safeFile = "safe"; + String nonEvilFile = "$%^safe-and_bleh.boo*@~"; + String evilFile = ";rm /etc/shadow;"; + + File safeNotSanitized = new File(directory, safeFile); + File nonEvilNotSanitized = new File(directory, nonEvilFile); + File evilNotSanitized = new File(directory, evilFile); + + assertEquals("/tmp/blah/safe", safeNotSanitized.getAbsolutePath()); + assertEquals("/tmp/blah/$%^safe-and_bleh.boo*@~", nonEvilNotSanitized.getAbsolutePath()); + assertEquals("/tmp/blah/;rm /etc/shadow;", evilNotSanitized.getAbsolutePath()); + + assertEquals("safe", safeNotSanitized.getName()); + assertEquals("$%^safe-and_bleh.boo*@~", nonEvilNotSanitized.getName()); + assertEquals("shadow;", evilNotSanitized.getName()); // Should be ;rm /etc/shadow; but the forward slashes are naughty. + + SanitizedFile safeSanitized = new SanitizedFile(directory, safeFile); + SanitizedFile nonEvilSanitized = new SanitizedFile(directory, nonEvilFile); + SanitizedFile evilSanitized = new SanitizedFile(directory, evilFile); + + assertEquals("/tmp/blah/safe", safeSanitized.getAbsolutePath()); + assertEquals("/tmp/blah/safe-and_bleh.boo", nonEvilSanitized.getAbsolutePath()); + assertEquals("/tmp/blah/rmetcshadow", evilSanitized.getAbsolutePath()); + + assertEquals("safe", safeSanitized.getName()); + assertEquals("safe-and_bleh.boo", nonEvilSanitized.getName()); + assertEquals("rmetcshadow", evilSanitized.getName()); + + } + +} From 08af7ee15708e6199a1bb2440450746612ac98e9 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 26 Jan 2015 15:00:53 +1100 Subject: [PATCH 3/4] Change symlink implementation to use best available impl per platform. * Android-21 introduced an API for symlinking. * Android-19 has an API which can be used via reflection. * Earlier versions use Runtime.exec('/system/bin/ln') This also extends the SanitizedFile stuff so that the android < 19 can safely use Runtime.exec() with less fear of command injection vulnerabilities. Finally, some tests for the SanitizedFile and symlink stuff was added. --- F-Droid/src/org/fdroid/fdroid/Utils.java | 38 +--------- .../org/fdroid/fdroid/compat/FileCompat.java | 52 ++++++++++++++ F-Droid/src/org/fdroid/fdroid/data/Apk.java | 3 +- F-Droid/src/org/fdroid/fdroid/data/App.java | 2 +- .../org/fdroid/fdroid/data/SanitizedFile.java | 48 +++++++++++++ .../fdroid/localrepo/LocalRepoManager.java | 43 ++++++------ F-Droid/test/project.properties | 7 ++ .../src/org/fdroid/fdroid/FileCompatTest.java | 66 +++++++++++++++++ .../test/src/org/fdroid/fdroid/TestUtils.java | 70 +++++++++++++++++++ .../fdroid/compat/FileCompatForTest.java | 33 +++++++++ .../fdroid/updater/SignedRepoUpdaterTest.java | 62 +++------------- 11 files changed, 314 insertions(+), 110 deletions(-) create mode 100644 F-Droid/test/src/org/fdroid/fdroid/FileCompatTest.java create mode 100644 F-Droid/test/src/org/fdroid/fdroid/compat/FileCompatForTest.java diff --git a/F-Droid/src/org/fdroid/fdroid/Utils.java b/F-Droid/src/org/fdroid/fdroid/Utils.java index f345a8113..01430b396 100644 --- a/F-Droid/src/org/fdroid/fdroid/Utils.java +++ b/F-Droid/src/org/fdroid/fdroid/Utils.java @@ -118,14 +118,10 @@ public final class Utils { } /** - * use symlinks if they are available, otherwise fall back to copying + * Attempt to symlink, but if that fails, it will make a copy of the file. */ - public static boolean symlinkOrCopyFile(File inFile, File outFile) { - if (new File("/system/bin/ln").exists()) { - return symlink(inFile, outFile); - } else { - return copy(inFile, outFile); - } + public static boolean symlinkOrCopyFile(SanitizedFile inFile, SanitizedFile outFile) { + return FileCompat.symlink(inFile, outFile) || copy(inFile, outFile); } /** @@ -143,34 +139,6 @@ public final class Utils { } } - public static boolean symlink(File inFile, File outFile) { - int exitCode = -1; - try { - Process sh = Runtime.getRuntime().exec("sh"); - OutputStream out = sh.getOutputStream(); - String command = "/system/bin/ln -s " + inFile + " " + outFile - + "\nexit\n"; - out.write(command.getBytes("ASCII")); - - final char buf[] = new char[40]; - InputStreamReader reader = new InputStreamReader(sh.getInputStream()); - while (reader.read(buf) != -1) - throw new IOException("stdout: " + new String(buf)); - reader = new InputStreamReader(sh.getErrorStream()); - while (reader.read(buf) != -1) - throw new IOException("stderr: " + new String(buf)); - - exitCode = sh.waitFor(); - } catch (IOException e) { - e.printStackTrace(); - return false; - } catch (InterruptedException e) { - e.printStackTrace(); - return false; - } - return exitCode == 0; - } - public static boolean copy(File inFile, File outFile) { InputStream input = null; OutputStream output = null; diff --git a/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java b/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java index 9784bc8f2..b0275d847 100644 --- a/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java +++ b/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java @@ -1,16 +1,68 @@ package org.fdroid.fdroid.compat; import android.annotation.TargetApi; +import android.os.Build; +import android.system.ErrnoException; import android.util.Log; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.SanitizedFile; import java.io.IOException; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; public class FileCompat { private static final String TAG = "org.fdroid.fdroid.compat.FileCompat"; + public static boolean symlink(SanitizedFile source, SanitizedFile dest) { + + if (Compatibility.hasApi(21)) { + symlinkOs(source, dest); + } else if (Compatibility.hasApi(19)) { + symlinkLibcore(source, dest); + } else { + symlinkRuntime(source, dest); + } + + return dest.exists(); + } + + @TargetApi(Build.VERSION_CODES.LOLLIPOP) + protected static void symlinkOs(SanitizedFile source, SanitizedFile dest) { + try { + android.system.Os.symlink(source.getAbsolutePath(), dest.getAbsolutePath()); + } catch (ErrnoException e) { + // Do nothing... + } + } + + protected static void symlinkRuntime(SanitizedFile source, SanitizedFile dest) { + String commands[] = { + "/system/bin/ln", + source.getAbsolutePath(), + dest.getAbsolutePath() + }; + try { + Log.d(TAG, "Executing command: " + commands[0] + " " + commands[1] + " " + commands[2]); + Process proc = Runtime.getRuntime().exec(commands); + Utils.consumeStream(proc.getInputStream()); + Utils.consumeStream(proc.getErrorStream()); + } catch (IOException e) { + // Do nothing + } + } + + protected static void symlinkLibcore(SanitizedFile source, SanitizedFile dest) { + try { + Object os = Class.forName("libcore.io.Libcore").getField("os").get(null); + Method symlink = os.getClass().getMethod("symlink", String.class, String.class); + symlink.invoke(os, source.getAbsolutePath(), dest.getAbsolutePath()); + } catch (InvocationTargetException | NoSuchMethodException | ClassNotFoundException | IllegalAccessException | NoSuchFieldException e) { + // Do nothing + } + } + @TargetApi(9) public static boolean setReadable(SanitizedFile file, boolean readable, boolean ownerOnly) { diff --git a/F-Droid/src/org/fdroid/fdroid/data/Apk.java b/F-Droid/src/org/fdroid/fdroid/data/Apk.java index 6177f6e9a..f70166a60 100644 --- a/F-Droid/src/org/fdroid/fdroid/data/Apk.java +++ b/F-Droid/src/org/fdroid/fdroid/data/Apk.java @@ -4,7 +4,6 @@ import android.content.ContentValues; import android.database.Cursor; import org.fdroid.fdroid.Utils; -import java.io.File; import java.util.Date; public class Apk extends ValueObject implements Comparable { @@ -33,7 +32,7 @@ public class Apk extends ValueObject implements Comparable { public boolean compatible; public String apkName; // F-Droid style APK name - public File installedFile; // the .apk file on this device's filesystem + public SanitizedFile installedFile; // the .apk file on this device's filesystem // If not null, this is the name of the source tarball for the // application. Null indicates that it's a developer's binary diff --git a/F-Droid/src/org/fdroid/fdroid/data/App.java b/F-Droid/src/org/fdroid/fdroid/data/App.java index 2c8ed0fb9..61b364f5f 100644 --- a/F-Droid/src/org/fdroid/fdroid/data/App.java +++ b/F-Droid/src/org/fdroid/fdroid/data/App.java @@ -224,7 +224,7 @@ public class App extends ValueObject implements Comparable { this.name = (String) appInfo.loadLabel(pm); - File apkFile = new File(appInfo.publicSourceDir); + SanitizedFile apkFile = SanitizedFile.knownSanitized(appInfo.publicSourceDir); Apk apk = new Apk(); apk.version = packageInfo.versionName; apk.vercode = packageInfo.versionCode; diff --git a/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java b/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java index 4a3ed7651..cdf4e8981 100644 --- a/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java +++ b/F-Droid/src/org/fdroid/fdroid/data/SanitizedFile.java @@ -9,7 +9,55 @@ import java.io.File; * Sanitized names are those which only have the following characters: [A-Za-z0-9.-_] */ public class SanitizedFile extends File { + + /** + * The "name" argument is assumed to be a file name, _not including any path separators_. + * If it is a relative path to be appended to "parent", such as "/blah/sneh.txt", then + * the forward slashes will be removed and it will be assumed you meant "blahsneh.txt". + */ public SanitizedFile(File parent, String name) { super(parent, name.replaceAll("[^A-Za-z0-9-._]", "")); } + + /** + * Used by the {@link org.fdroid.fdroid.data.SanitizedFile#knownSanitized(java.io.File)} + * method, but intentionally kept private so people don't think that any sanitization + * will occur by passing a file in - because it wont. + */ + private SanitizedFile(File file) { + super(file.getAbsolutePath()); + } + + /** + * This is dangerous, but there will be some cases when all we have is an absolute file + * path that wasn't given to us from user input. One example would be asking Android for + * the path to an installed .apk on disk. In such situations, we can't meaningfully + * sanitize it, but will still need to pass to a function which only allows SanitizedFile's + * as arguments (because they interact with, e.g. shells). + * + * To illustrate, imagine perfectly valid file path: "/tmp/../secret/file.txt", + * one cannot distinguish between: + * + * "/tmp/" (known safe directory) + "../secret/file.txt" (suspicious looking file name) + * + * and + * + * "/tmp/../secret/" (known safe directory) + "file.txt" (known safe file name) + * + * I guess the best this method offers us is the ability to uniquely trace the different + * ways in which files are created and handled. It should make it easier to find and + * prevent suspect usages of methods which only expect SanitizedFile's, but are given + * a SanitizedFile returned from this method that really originated from user input. + */ + public static SanitizedFile knownSanitized(String path) { + return new SanitizedFile(new File(path)); + } + + /** + * @see {@link org.fdroid.fdroid.data.SanitizedFile#knownSanitized(String)} + */ + public static SanitizedFile knownSanitized(File file) { + return new SanitizedFile(file); + } + } diff --git a/F-Droid/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java b/F-Droid/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java index 3fa7c36bc..45fa2e9e5 100644 --- a/F-Droid/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java +++ b/F-Droid/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java @@ -25,6 +25,7 @@ import org.fdroid.fdroid.Preferences; import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.App; +import org.fdroid.fdroid.data.SanitizedFile; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -81,13 +82,13 @@ public class LocalRepoManager { private Map apps = new HashMap(); - public final File xmlIndex; - private File xmlIndexJar = null; - private File xmlIndexJarUnsigned = null; - public final File webRoot; - public final File fdroidDir; - public final File repoDir; - public final File iconsDir; + public final SanitizedFile xmlIndex; + private SanitizedFile xmlIndexJar = null; + private SanitizedFile xmlIndexJarUnsigned = null; + public final SanitizedFile webRoot; + public final SanitizedFile fdroidDir; + public final SanitizedFile repoDir; + public final SanitizedFile iconsDir; private static LocalRepoManager localRepoManager; @@ -104,14 +105,14 @@ public class LocalRepoManager { prefs = PreferenceManager.getDefaultSharedPreferences(c); fdroidPackageName = c.getPackageName(); - webRoot = c.getFilesDir(); + webRoot = SanitizedFile.knownSanitized(c.getFilesDir()); /* /fdroid/repo is the standard path for user repos */ - fdroidDir = new File(webRoot, "fdroid"); - repoDir = new File(fdroidDir, "repo"); - iconsDir = new File(repoDir, "icons"); - xmlIndex = new File(repoDir, "index.xml"); - xmlIndexJar = new File(repoDir, "index.jar"); - xmlIndexJarUnsigned = new File(repoDir, "index.unsigned.jar"); + fdroidDir = new SanitizedFile(webRoot, "fdroid"); + repoDir = new SanitizedFile(fdroidDir, "repo"); + iconsDir = new SanitizedFile(repoDir, "icons"); + xmlIndex = new SanitizedFile(repoDir, "index.xml"); + xmlIndexJar = new SanitizedFile(repoDir, "index.jar"); + xmlIndexJarUnsigned = new SanitizedFile(repoDir, "index.unsigned.jar"); if (!fdroidDir.exists()) if (!fdroidDir.mkdir()) @@ -136,8 +137,8 @@ public class LocalRepoManager { try { appInfo = pm.getApplicationInfo(fdroidPackageName, PackageManager.GET_META_DATA); - File apkFile = new File(appInfo.publicSourceDir); - File fdroidApkLink = new File(webRoot, "fdroid.client.apk"); + SanitizedFile apkFile = SanitizedFile.knownSanitized(appInfo.publicSourceDir); + SanitizedFile fdroidApkLink = new SanitizedFile(webRoot, "fdroid.client.apk"); fdroidApkLink.delete(); if (Utils.symlinkOrCopyFile(apkFile, fdroidApkLink)) fdroidClientURL = "/" + fdroidApkLink.getName(); @@ -191,14 +192,14 @@ public class LocalRepoManager { } private void symlinkIndexPageElsewhere(String symlinkPrefix, File directory) { - File index = new File(directory, "index.html"); + SanitizedFile index = new SanitizedFile(directory, "index.html"); index.delete(); - Utils.symlinkOrCopyFile(new File(symlinkPrefix + "index.html"), index); + Utils.symlinkOrCopyFile(new SanitizedFile(new File(symlinkPrefix), "index.html"), index); for(String fileName : WEB_ROOT_ASSET_FILES) { - File file = new File(directory, fileName); + SanitizedFile file = new SanitizedFile(directory, fileName); file.delete(); - Utils.symlinkOrCopyFile(new File(symlinkPrefix + fileName), file); + Utils.symlinkOrCopyFile(new SanitizedFile(new File(symlinkPrefix), fileName), file); } } @@ -227,7 +228,7 @@ public class LocalRepoManager { App app = apps.get(packageName); if (app.installedApk != null) { - File outFile = new File(repoDir, app.installedApk.apkName); + SanitizedFile outFile = new SanitizedFile(repoDir, app.installedApk.apkName); if (Utils.symlinkOrCopyFile(app.installedApk.installedFile, outFile)) continue; } diff --git a/F-Droid/test/project.properties b/F-Droid/test/project.properties index 6e18427a4..e611232d2 100644 --- a/F-Droid/test/project.properties +++ b/F-Droid/test/project.properties @@ -12,3 +12,10 @@ # Project target. target=android-21 + +# With a target SDK of android-21 (5.0/Lollipop) and a Java 1.7 compiler, you +# can use Java 1.7 features like the <> diamond operator, multi-catch, strings +# in switches, etc. +java.encoding=UTF-8 +java.source=1.7 +java.target=1.7 diff --git a/F-Droid/test/src/org/fdroid/fdroid/FileCompatTest.java b/F-Droid/test/src/org/fdroid/fdroid/FileCompatTest.java new file mode 100644 index 000000000..4f1c48110 --- /dev/null +++ b/F-Droid/test/src/org/fdroid/fdroid/FileCompatTest.java @@ -0,0 +1,66 @@ +package org.fdroid.fdroid; + +import android.os.Build; +import android.test.InstrumentationTestCase; +import android.util.Log; +import org.fdroid.fdroid.compat.FileCompatForTest; +import org.fdroid.fdroid.data.SanitizedFile; + +import java.io.File; + +public class FileCompatTest extends InstrumentationTestCase { + + private static final String TAG = "org.fdroid.fdroid.FileCompatTest"; + + private File dir; + private SanitizedFile sourceFile; + private SanitizedFile destFile; + + public void setUp() { + dir = TestUtils.getWriteableDir(getInstrumentation()); + sourceFile = SanitizedFile.knownSanitized(TestUtils.copyAssetToDir(getInstrumentation().getContext(), "simpleIndex.jar", dir)); + destFile = new SanitizedFile(dir, "dest.txt"); + assertTrue(!destFile.exists()); + assertTrue(sourceFile.getAbsolutePath() + " should exist.", sourceFile.exists()); + } + + public void tearDown() { + assertTrue("Can't delete " + sourceFile.getAbsolutePath() + ".", sourceFile.delete()); + assertTrue("Can't delete " + destFile.getAbsolutePath() + ".", destFile.delete()); + } + + public void testSymlinkRuntime() { + SanitizedFile destFile = new SanitizedFile(dir, "dest.txt"); + assertFalse(destFile.exists()); + + FileCompatForTest.symlinkRuntimeTest(sourceFile, destFile); + assertTrue(destFile.getAbsolutePath() + " should exist after symlinking", destFile.exists()); + } + + public void testSymlinkLibcore() { + + if (Build.VERSION.SDK_INT >= 19) { + SanitizedFile destFile = new SanitizedFile(dir, "dest.txt"); + assertFalse(destFile.exists()); + + FileCompatForTest.symlinkLibcoreTest(sourceFile, destFile); + assertTrue(destFile.getAbsolutePath() + " should exist after symlinking", destFile.exists()); + } else { + Log.w(TAG, "Cannot test symlink-libcore on this device. Requires android-19, but this has android-" + Build.VERSION.SDK_INT); + } + } + + public void testSymlinkOs() { + + if (Build.VERSION.SDK_INT >= 21 ) { + SanitizedFile destFile = new SanitizedFile(dir, "dest.txt"); + assertFalse(destFile.exists()); + + FileCompatForTest.symlinkOsTest(sourceFile, destFile); + assertTrue(destFile.getAbsolutePath() + " should exist after symlinking", destFile.exists()); + } else { + Log.w(TAG, "Cannot test symlink-os on this device. Requires android-21, but only has android-" + Build.VERSION.SDK_INT); + } + } + +} diff --git a/F-Droid/test/src/org/fdroid/fdroid/TestUtils.java b/F-Droid/test/src/org/fdroid/fdroid/TestUtils.java index e4927a3fe..1f6b65c45 100644 --- a/F-Droid/test/src/org/fdroid/fdroid/TestUtils.java +++ b/F-Droid/test/src/org/fdroid/fdroid/TestUtils.java @@ -1,18 +1,24 @@ package org.fdroid.fdroid; +import android.app.Instrumentation; import android.content.*; import android.net.Uri; +import android.os.Environment; +import android.util.Log; import junit.framework.AssertionFailedError; import mock.MockInstallablePackageManager; import org.fdroid.fdroid.data.ApkProvider; import org.fdroid.fdroid.data.AppProvider; +import java.io.*; import java.util.ArrayList; import java.util.Collections; import java.util.List; public class TestUtils { + private static final String TAG = "org.fdroid.fdroid.TestUtils"; + public static void assertContainsOnly(List actualList, T[] expectedArray) { List expectedList = new ArrayList(expectedArray.length); Collections.addAll(expectedList, expectedArray); @@ -175,4 +181,68 @@ public class TestUtils { } + public static File copyAssetToDir(Context context, String assetName, File directory) { + File tempFile; + InputStream input = null; + OutputStream output = null; + try { + tempFile = File.createTempFile(assetName + "-", ".testasset", directory); + Log.d(TAG, "Copying asset file " + assetName + " to directory " + directory); + input = context.getResources().getAssets().open(assetName); + output = new FileOutputStream(tempFile); + Utils.copy(input, output); + } catch (IOException e) { + e.printStackTrace(); + return null; + } finally { + Utils.closeQuietly(output); + Utils.closeQuietly(input); + } + return tempFile; + } + + /** + * Prefer internal over external storage, because external tends to be FAT filesystems, + * which don't support symlinks (which we test using this method). + */ + public static File getWriteableDir(Instrumentation instrumentation) { + Context context = instrumentation.getContext(); + Context targetContext = instrumentation.getTargetContext(); + File dir = context.getCacheDir(); + Log.d(TAG, "Looking for writeable dir, trying context.getCacheDir()" ); + if (dir == null || !dir.canWrite()) { + Log.d(TAG, "Looking for writeable dir, trying context.getFilesDir()"); + dir = context.getFilesDir(); + } + if (dir == null || !dir.canWrite()) { + Log.d(TAG, "Looking for writeable dir, trying targetContext.getCacheDir()"); + dir = targetContext.getCacheDir(); + } + if (dir == null || !dir.canWrite()) { + Log.d(TAG, "Looking for writeable dir, trying targetContext.getFilesDir()"); + dir = targetContext.getFilesDir(); + } + if (dir == null || !dir.canWrite()) { + Log.d(TAG, "Looking for writeable dir, trying context.getExternalCacheDir()"); + dir = context.getExternalCacheDir(); + } + if (dir == null || !dir.canWrite()) { + Log.d(TAG, "Looking for writeable dir, trying context.getExternalFilesDir(null)"); + dir = context.getExternalFilesDir(null); + } + if (dir == null || !dir.canWrite()) { + Log.d(TAG, "Looking for writeable dir, trying targetContext.getExternalCacheDir()"); + dir = targetContext.getExternalCacheDir(); + } + if (dir == null || !dir.canWrite()) { + Log.d(TAG, "Looking for writeable dir, trying targetContext.getExternalFilesDir(null)"); + dir = targetContext.getExternalFilesDir(null); + } + if (dir == null || !dir.canWrite()) { + Log.d(TAG, "Looking for writeable dir, trying Environment.getExternalStorageDirectory()"); + dir = Environment.getExternalStorageDirectory(); + } + Log.d(TAG, "Writeable dir found: " + dir); + return dir; + } } diff --git a/F-Droid/test/src/org/fdroid/fdroid/compat/FileCompatForTest.java b/F-Droid/test/src/org/fdroid/fdroid/compat/FileCompatForTest.java new file mode 100644 index 000000000..f5ac4607f --- /dev/null +++ b/F-Droid/test/src/org/fdroid/fdroid/compat/FileCompatForTest.java @@ -0,0 +1,33 @@ +package org.fdroid.fdroid.compat; + +import android.annotation.TargetApi; +import android.os.Build; +import android.system.ErrnoException; +import org.fdroid.fdroid.Utils; +import org.fdroid.fdroid.data.SanitizedFile; + +import java.io.IOException; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; + +/** + * Used to expose the protected methods from FileCompat in a public manner so + * that they can be called from a test harness. + */ +public class FileCompatForTest extends FileCompat { + + @TargetApi(Build.VERSION_CODES.LOLLIPOP) + public static void symlinkOsTest(SanitizedFile source, SanitizedFile dest) { + symlinkOs(source, dest); + } + + public static void symlinkRuntimeTest(SanitizedFile source, SanitizedFile dest) { + symlinkRuntime(source, dest); + } + + public static void symlinkLibcoreTest(SanitizedFile source, SanitizedFile dest) { + symlinkLibcore(source, dest); + } + + +} diff --git a/F-Droid/test/src/org/fdroid/fdroid/updater/SignedRepoUpdaterTest.java b/F-Droid/test/src/org/fdroid/fdroid/updater/SignedRepoUpdaterTest.java index dc98ab5e8..e11f9639c 100644 --- a/F-Droid/test/src/org/fdroid/fdroid/updater/SignedRepoUpdaterTest.java +++ b/F-Droid/test/src/org/fdroid/fdroid/updater/SignedRepoUpdaterTest.java @@ -8,6 +8,7 @@ import android.test.InstrumentationTestCase; import android.util.Log; import org.apache.commons.io.FileUtils; +import org.fdroid.fdroid.TestUtils; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Repo; import org.fdroid.fdroid.updater.RepoUpdater.UpdateException; @@ -31,56 +32,17 @@ public class SignedRepoUpdaterTest extends InstrumentationTestCase { @Override protected void setUp() { context = getInstrumentation().getContext(); - Context target = getInstrumentation().getTargetContext(); - /* find something that works, many of these often fail on emulators */ - testFilesDir = context.getFilesDir(); - if (testFilesDir == null || !testFilesDir.canWrite()) - testFilesDir = context.getCacheDir(); - if (testFilesDir == null || !testFilesDir.canWrite()) - testFilesDir = context.getExternalCacheDir(); - if (testFilesDir == null || !testFilesDir.canWrite()) - testFilesDir = context.getExternalFilesDir(null); - if (testFilesDir == null || !testFilesDir.canWrite()) - testFilesDir = target.getFilesDir(); - if (testFilesDir == null || !testFilesDir.canWrite()) - testFilesDir = target.getCacheDir(); - if (testFilesDir == null || !testFilesDir.canWrite()) - testFilesDir = target.getExternalCacheDir(); - if (testFilesDir == null || !testFilesDir.canWrite()) - testFilesDir = target.getExternalFilesDir(null); - if (testFilesDir == null || !testFilesDir.canWrite()) - testFilesDir = Environment.getExternalStorageDirectory(); - Log.i(TAG, "testFilesDir: " + testFilesDir); + testFilesDir = TestUtils.getWriteableDir(getInstrumentation()); Repo repo = new Repo(); repo.pubkey = this.simpleIndexPubkey; repoUpdater = RepoUpdater.createUpdaterFor(context, repo); } - private File getTestFile(String fileName) { - File indexFile; - InputStream input = null; - OutputStream output = null; - try { - indexFile = File.createTempFile(fileName + "-", ".xml", testFilesDir); - Log.i(TAG, "getTestFile indexFile " + indexFile); - input = context.getResources().getAssets().open(fileName); - output = new FileOutputStream(indexFile); - Utils.copy(input, output); - } catch (IOException e) { - e.printStackTrace(); - return null; - } finally { - Utils.closeQuietly(output); - Utils.closeQuietly(input); - } - return indexFile; - } - public void testExtractIndexFromJar() { if (!testFilesDir.canWrite()) return; - File simpleIndexXml = getTestFile("simpleIndex.xml"); - File simpleIndexJar = getTestFile("simpleIndex.jar"); + File simpleIndexXml = TestUtils.copyAssetToDir(context, "simpleIndex.xml", testFilesDir); + File simpleIndexJar = TestUtils.copyAssetToDir(context, "simpleIndex.jar", testFilesDir); File testFile = null; // these are supposed to succeed @@ -103,7 +65,7 @@ public class SignedRepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(getTestFile("simpleIndexWithoutSignature.jar")); + repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "simpleIndexWithoutSignature.jar", testFilesDir)); fail(); } catch (UpdateException e) { // success! @@ -115,7 +77,7 @@ public class SignedRepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(getTestFile("simpleIndexWithCorruptedManifest.jar")); + repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedManifest.jar", testFilesDir)); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -130,7 +92,7 @@ public class SignedRepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(getTestFile("simpleIndexWithCorruptedSignature.jar")); + repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedSignature.jar", testFilesDir)); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -145,7 +107,7 @@ public class SignedRepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(getTestFile("simpleIndexWithCorruptedCertificate.jar")); + repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedCertificate.jar", testFilesDir)); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -160,7 +122,7 @@ public class SignedRepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(getTestFile("simpleIndexWithCorruptedEverything.jar")); + repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedEverything.jar", testFilesDir)); fail(); } catch (UpdateException e) { e.printStackTrace(); @@ -175,11 +137,9 @@ public class SignedRepoUpdaterTest extends InstrumentationTestCase { return; // this is supposed to fail try { - repoUpdater.getIndexFromFile(getTestFile("masterKeyIndex.jar")); + repoUpdater.getIndexFromFile(TestUtils.copyAssetToDir(context, "masterKeyIndex.jar", testFilesDir)); fail(); - } catch (UpdateException e) { - // success! - } catch (SecurityException e) { + } catch (UpdateException | SecurityException e) { // success! } } From 2fe91bc35eb77820a508bf882e76e2bd1e8d7cff Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 27 Jan 2015 23:24:00 +1100 Subject: [PATCH 4/4] Prevent VerifyError when loading FileCompat. Classes which contain calls to platform specific methods cause problems, because the dexer will go looking for that method even if you put a guard condition checking the build number. However, if you lazily load a class depdending on the version number, then older API devices wont try and load it, and no VerifyError occurs. --- .../org/fdroid/fdroid/compat/FileCompat.java | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java b/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java index b0275d847..7cc6c411e 100644 --- a/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java +++ b/F-Droid/src/org/fdroid/fdroid/compat/FileCompat.java @@ -19,7 +19,7 @@ public class FileCompat { if (Compatibility.hasApi(21)) { symlinkOs(source, dest); - } else if (Compatibility.hasApi(19)) { + } else if (Compatibility.hasApi(15)) { symlinkLibcore(source, dest); } else { symlinkRuntime(source, dest); @@ -28,13 +28,27 @@ public class FileCompat { return dest.exists(); } - @TargetApi(Build.VERSION_CODES.LOLLIPOP) - protected static void symlinkOs(SanitizedFile source, SanitizedFile dest) { - try { - android.system.Os.symlink(source.getAbsolutePath(), dest.getAbsolutePath()); - } catch (ErrnoException e) { - // Do nothing... + /** + * Moved into a separate class rather than just a method, so that phones without API 21 will + * not attempt to load this class at runtime. Otherwise, using the Os.symlink method will cause + * a VerifyError to be thrown at runtime when the FileCompat class is first used. + */ + private static class Symlink21 { + + @TargetApi(Build.VERSION_CODES.LOLLIPOP) + public void symlink(SanitizedFile source, SanitizedFile dest) { + try { + android.system.Os.symlink(source.getAbsolutePath(), dest.getAbsolutePath()); + } catch (ErrnoException e) { + // Do nothing... + } } + + } + + @TargetApi(21) + protected static void symlinkOs(SanitizedFile source, SanitizedFile dest) { + new Symlink21().symlink(source, dest); } protected static void symlinkRuntime(SanitizedFile source, SanitizedFile dest) { @@ -58,8 +72,10 @@ public class FileCompat { Object os = Class.forName("libcore.io.Libcore").getField("os").get(null); Method symlink = os.getClass().getMethod("symlink", String.class, String.class); symlink.invoke(os, source.getAbsolutePath(), dest.getAbsolutePath()); - } catch (InvocationTargetException | NoSuchMethodException | ClassNotFoundException | IllegalAccessException | NoSuchFieldException e) { - // Do nothing + } catch (Exception e) { + // Should catch more specific exceptions than just "Exception" here, but there are + // some which come from libcore.io.Libcore, which we don't have access to at compile time. + Log.e(TAG, "Could not symlink " + source.getAbsolutePath() + " to " + dest.getAbsolutePath() + ": " + e.getMessage()); } }