From 37b3f1ff57381ae4ae8b89e211e74ce1de2cc66c Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 26 Jan 2015 07:56:05 +1100 Subject: [PATCH] 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