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.
This commit is contained in:
Peter Serwylo 2015-01-26 15:00:53 +11:00
parent afef5ea233
commit 08af7ee157
11 changed files with 314 additions and 110 deletions

View File

@ -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;

View File

@ -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) {

View File

@ -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<Apk> {
@ -33,7 +32,7 @@ public class Apk extends ValueObject implements Comparable<Apk> {
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

View File

@ -224,7 +224,7 @@ public class App extends ValueObject implements Comparable<App> {
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;

View File

@ -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);
}
}

View File

@ -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<String, App> apps = new HashMap<String, App>();
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;
}

View File

@ -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

View File

@ -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);
}
}
}

View File

@ -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 <T extends Comparable> void assertContainsOnly(List<T> actualList, T[] expectedArray) {
List<T> expectedList = new ArrayList<T>(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;
}
}

View File

@ -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);
}
}

View File

@ -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!
}
}