diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index f06c80995..6434eaf7c 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -89,7 +89,6 @@ import org.fdroid.fdroid.data.RepoProvider; import org.fdroid.fdroid.installer.Installer; import org.fdroid.fdroid.installer.Installer.InstallFailedException; import org.fdroid.fdroid.installer.Installer.InstallerCallback; -import org.fdroid.fdroid.net.ApkDownloader; import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.DownloaderService; @@ -304,7 +303,7 @@ public class AppDetails extends AppCompatActivity { private App app; private PackageManager packageManager; - private ApkDownloader downloadHandler; + private String activeDownloadUrlString; private LocalBroadcastManager localBroadcastManager; private boolean startingIgnoreAll; @@ -324,17 +323,15 @@ public class AppDetails extends AppCompatActivity { */ private static class ConfigurationChangeHelper { - public final ApkDownloader downloader; + public final String urlString; public final App app; - ConfigurationChangeHelper(ApkDownloader downloader, App app) { - this.downloader = downloader; + ConfigurationChangeHelper(String urlString, App app) { + this.urlString = urlString; this.app = app; } } - private boolean inProcessOfChangingConfiguration; - /** * Attempt to extract the packageName from the intent which launched this activity. * @return May return null, if we couldn't find the packageName. This should @@ -375,8 +372,8 @@ public class AppDetails extends AppCompatActivity { ConfigurationChangeHelper previousData = (ConfigurationChangeHelper) getLastCustomNonConfigurationInstance(); if (previousData != null) { Utils.debugLog(TAG, "Recreating view after configuration change."); - downloadHandler = previousData.downloader; - if (downloadHandler != null) { + activeDownloadUrlString = previousData.urlString; + if (activeDownloadUrlString != null) { Utils.debugLog(TAG, "Download was in progress before the configuration change, so we will start to listen to its events again."); } app = previousData.app; @@ -438,10 +435,9 @@ public class AppDetails extends AppCompatActivity { * Remove progress listener, suppress progress bar, set downloadHandler to null. */ private void cleanUpFinishedDownload() { - if (downloadHandler != null) { - headerFragment.removeProgress(); - downloadHandler = null; - } + activeDownloadUrlString = null; + headerFragment.removeProgress(); + unregisterDownloaderReceivers(); } protected void onStop() { @@ -458,7 +454,6 @@ public class AppDetails extends AppCompatActivity { setIgnoreUpdates(app.packageName, app.ignoreAllUpdates, app.ignoreThisUpdate); } unregisterDownloaderReceivers(); - headerFragment.removeProgress(); } private void unregisterDownloaderReceivers() { @@ -469,8 +464,8 @@ public class AppDetails extends AppCompatActivity { } private void registerDownloaderReceivers() { - if (downloadHandler != null) { // if a download is active - String url = downloadHandler.urlString; + if (activeDownloadUrlString != null) { // if a download is active + String url = activeDownloadUrlString; localBroadcastManager.registerReceiver(startedReceiver, DownloaderService.getIntentFilter(url, Downloader.ACTION_STARTED)); localBroadcastManager.registerReceiver(progressReceiver, @@ -550,17 +545,12 @@ public class AppDetails extends AppCompatActivity { @Override public Object onRetainCustomNonConfigurationInstance() { - inProcessOfChangingConfiguration = true; - return new ConfigurationChangeHelper(downloadHandler, app); + return new ConfigurationChangeHelper(activeDownloadUrlString, app); } @Override protected void onDestroy() { - if (downloadHandler != null && !inProcessOfChangingConfiguration) { - DownloaderService.cancel(context, downloadHandler.urlString); - cleanUpFinishedDownload(); - } - inProcessOfChangingConfiguration = false; + cleanUpFinishedDownload(); super.onDestroy(); } @@ -853,10 +843,11 @@ public class AppDetails extends AppCompatActivity { } private void startDownload(Apk apk, String repoAddress) { - downloadHandler = new ApkDownloader(getBaseContext(), app, apk, repoAddress); + String urlString = Utils.getApkUrl(repoAddress, apk); + activeDownloadUrlString = urlString; registerDownloaderReceivers(); - downloadHandler.download(); headerFragment.startProgress(); + DownloaderService.queue(this, activeDownloadUrlString); } public void removeApk(String packageName) { @@ -1466,14 +1457,11 @@ public class AppDetails extends AppCompatActivity { @Override public void onClick(View view) { AppDetails appDetails = (AppDetails) getActivity(); - if (appDetails == null || appDetails.downloadHandler == null) { + if (appDetails == null || appDetails.activeDownloadUrlString == null) { return; } - DownloaderService.cancel(getContext(), appDetails.downloadHandler.urlString); - appDetails.cleanUpFinishedDownload(); - setProgressVisible(false); - updateViews(); + DownloaderService.cancel(getContext(), appDetails.activeDownloadUrlString); } public void updateViews() { @@ -1485,7 +1473,7 @@ public class AppDetails extends AppCompatActivity { TextView statusView = (TextView) view.findViewById(R.id.status); btMain.setVisibility(View.VISIBLE); - if (appDetails.downloadHandler != null) { + if (appDetails.activeDownloadUrlString != null) { btMain.setText(R.string.downloading); btMain.setEnabled(false); } else if (!app.isInstalled() && app.suggestedVercode > 0 && diff --git a/app/src/main/java/org/fdroid/fdroid/FDroidApp.java b/app/src/main/java/org/fdroid/fdroid/FDroidApp.java index 57862494b..d086d8e3f 100644 --- a/app/src/main/java/org/fdroid/fdroid/FDroidApp.java +++ b/app/src/main/java/org/fdroid/fdroid/FDroidApp.java @@ -234,7 +234,6 @@ public class FDroidApp extends Application { // been installed, but this causes problems for proprietary gapps // users since the introduction of verification (on pre-4.2 Android), // because the install intent says it's finished when it hasn't. - Utils.deleteFiles(Utils.getApkDownloadDir(this), null, ".apk"); if (!Preferences.get().shouldCacheApks()) { Utils.deleteFiles(Utils.getApkCacheDir(this), null, ".apk"); } diff --git a/app/src/main/java/org/fdroid/fdroid/Utils.java b/app/src/main/java/org/fdroid/fdroid/Utils.java index 1f66c3c05..94ae4ed64 100644 --- a/app/src/main/java/org/fdroid/fdroid/Utils.java +++ b/app/src/main/java/org/fdroid/fdroid/Utils.java @@ -318,7 +318,11 @@ public final class Utils { } /** - * See {@link Utils#getApkDownloadDir(android.content.Context)} for why this is "unsafe". + * This location is only for caching, do not install directly from this location + * because if the file is on the External Storage, any other app could swap out + * the APK while the install was in process, allowing malware to install things. + * Using {@link org.fdroid.fdroid.installer.Installer#installPackage(File, String)} + * is fine since that does the right thing. */ public static SanitizedFile getApkCacheDir(Context context) { final SanitizedFile apkCacheDir = new SanitizedFile(StorageUtils.getCacheDirectory(context, true), "apks"); @@ -328,24 +332,6 @@ public final class Utils { return apkCacheDir; } - /** - * 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 getApkDownloadDir(Context context) { - final SanitizedFile apkCacheDir = new SanitizedFile(StorageUtils.getCacheDirectory(context, false), "temp"); - 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; - } - public static String calcFingerprint(String keyHexString) { if (TextUtils.isEmpty(keyHexString) || keyHexString.matches(".*[^a-fA-F0-9].*")) { diff --git a/app/src/main/java/org/fdroid/fdroid/installer/Installer.java b/app/src/main/java/org/fdroid/fdroid/installer/Installer.java index 9517c6d78..092f62e41 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/Installer.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/Installer.java @@ -23,14 +23,25 @@ import android.app.Activity; import android.content.Context; import android.content.Intent; import android.content.pm.PackageManager; +import android.text.TextUtils; import android.util.Log; +import org.apache.commons.io.FileUtils; +import org.fdroid.fdroid.AndroidXMLDecompress; import org.fdroid.fdroid.BuildConfig; +import org.fdroid.fdroid.Hasher; import org.fdroid.fdroid.Preferences; import org.fdroid.fdroid.Utils; +import org.fdroid.fdroid.compat.FileCompat; +import org.fdroid.fdroid.data.Apk; +import org.fdroid.fdroid.data.ApkProvider; +import org.fdroid.fdroid.data.SanitizedFile; import org.fdroid.fdroid.privileged.install.InstallExtensionDialogActivity; import java.io.File; +import java.io.IOException; +import java.security.NoSuchAlgorithmException; +import java.util.Map; /** * Abstract Installer class. Also provides static methods to automatically @@ -120,16 +131,16 @@ public abstract class Installer { if (android.os.Build.VERSION.SDK_INT >= 14) { // Default installer on Android >= 4.0 try { - Utils.debugLog(TAG, "try default installer for Android >= 4"); + Utils.debugLog(TAG, "try default installer for android >= 14"); return new DefaultSdk14Installer(activity, pm, callback); } catch (InstallFailedException e) { Log.e(TAG, "Android not compatible with DefaultInstallerSdk14!", e); } } else { - // Default installer on Android < 4.0 + // Default installer on Android < 4.0 (android-14) try { - Utils.debugLog(TAG, "try default installer for Android < 4"); + Utils.debugLog(TAG, "try default installer for android < 14"); return new DefaultInstaller(activity, pm, callback); } catch (InstallFailedException e) { @@ -141,39 +152,87 @@ public abstract class Installer { return null; } - public void installPackage(File apkFile, String packageName) throws InstallFailedException { - // check if file exists... + /** + * Checks the APK file against the provided hash, returning whether it is a match. + */ + private static boolean verifyApkFile(File apkFile, String hash, String hashType) + throws NoSuchAlgorithmException { if (!apkFile.exists()) { - Log.e(TAG, "Couldn't find file " + apkFile + " to install."); - return; + return false; } + Hasher hasher = new Hasher(hashType, apkFile); + if (hasher != null && hasher.match(hash)) { + return true; + } + return false; + } - // special case: F-Droid Privileged Extension - if (packageName != null && packageName.equals(PrivilegedInstaller.PRIVILEGED_EXTENSION_PACKAGE_NAME)) { + /** + * This is the safe, single point of entry for submitting an APK file to be installed. + */ + public void installPackage(File apkFile, String packageName) throws InstallFailedException { + SanitizedFile apkToInstall = null; + try { + Map attributes = AndroidXMLDecompress.getManifestHeaderAttributes(apkFile.getAbsolutePath()); - // extension must be signed with the same public key as main F-Droid - // NOTE: Disabled for debug builds to be able to use official extension from repo - ApkSignatureVerifier signatureVerifier = new ApkSignatureVerifier(mContext); - if (!BuildConfig.DEBUG && !signatureVerifier.hasFDroidSignature(apkFile)) { - throw new SecurityException("APK signature of extension not correct!"); + /* This isn't really needed, but might as well since we have the data already */ + if (attributes.containsKey("packageName")) { + if (!TextUtils.equals(packageName, (String) attributes.get("packageName"))) { + throw new InstallFailedException(apkFile + " has packageName that clashes with " + packageName); + } } - Activity activity; - try { - activity = (Activity) mContext; - } catch (ClassCastException e) { - Utils.debugLog(TAG, "F-Droid Privileged can only be updated using an activity!"); + if (!attributes.containsKey("versionCode")) { + throw new InstallFailedException(apkFile + " is missing versionCode!"); + } + int versionCode = (Integer) attributes.get("versionCode"); + Apk apk = ApkProvider.Helper.find(mContext, packageName, versionCode, new String[]{ + ApkProvider.DataColumns.HASH, + ApkProvider.DataColumns.HASH_TYPE, + }); + /* Always copy the APK to the safe location inside of the protected area + * of the app to prevent attacks based on other apps swapping the file + * out during the install process. Most likely, apkFile was just downloaded, + * so it should still be in the RAM disk cache */ + apkToInstall = SanitizedFile.knownSanitized(File.createTempFile("install-", ".apk", mContext.getFilesDir())); + FileUtils.copyFile(apkFile, apkToInstall); + if (!verifyApkFile(apkToInstall, apk.hash, apk.hashType)) { + FileUtils.deleteQuietly(apkFile); + throw new InstallFailedException(apkFile + " failed to verify!"); + } + apkFile = null; // ensure this is not used now that its copied to apkToInstall + + // special case: F-Droid Privileged Extension + if (packageName != null && packageName.equals(PrivilegedInstaller.PRIVILEGED_EXTENSION_PACKAGE_NAME)) { + + // extension must be signed with the same public key as main F-Droid + // NOTE: Disabled for debug builds to be able to use official extension from repo + ApkSignatureVerifier signatureVerifier = new ApkSignatureVerifier(mContext); + if (!BuildConfig.DEBUG && !signatureVerifier.hasFDroidSignature(apkToInstall)) { + throw new InstallFailedException("APK signature of extension not correct!"); + } + + Activity activity = (Activity) mContext; + Intent installIntent = new Intent(activity, InstallExtensionDialogActivity.class); + installIntent.setAction(InstallExtensionDialogActivity.ACTION_INSTALL); + installIntent.putExtra(InstallExtensionDialogActivity.EXTRA_INSTALL_APK, apkToInstall.getAbsolutePath()); + activity.startActivity(installIntent); return; } - Intent installIntent = new Intent(activity, InstallExtensionDialogActivity.class); - installIntent.setAction(InstallExtensionDialogActivity.ACTION_INSTALL); - installIntent.putExtra(InstallExtensionDialogActivity.EXTRA_INSTALL_APK, apkFile.getAbsolutePath()); - activity.startActivity(installIntent); - 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(apkToInstall, true, false); + installPackageInternal(apkToInstall); - installPackageInternal(apkFile); + } catch (NumberFormatException | NoSuchAlgorithmException | IOException e) { + throw new InstallFailedException(e); + } catch (ClassCastException e) { + throw new InstallFailedException("F-Droid Privileged can only be updated using an activity!"); + } } public void deletePackage(String packageName) throws InstallFailedException { diff --git a/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java deleted file mode 100644 index 0673a7fe4..000000000 --- a/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java +++ /dev/null @@ -1,160 +0,0 @@ -/* - * Copyright (C) 2010-2012 Ciaran Gultnieks - * Copyright (C) 2011 Henrik Tunedal - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License - * as published by the Free Software Foundation; either version 3 - * of the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, - * MA 02110-1301, USA. - */ - -package org.fdroid.fdroid.net; - -import android.content.BroadcastReceiver; -import android.content.Context; -import android.content.Intent; -import android.net.Uri; -import android.support.annotation.NonNull; -import android.support.v4.content.LocalBroadcastManager; -import android.util.Log; -import android.widget.Toast; - -import org.fdroid.fdroid.Hasher; -import org.fdroid.fdroid.Preferences; -import org.fdroid.fdroid.R; -import org.fdroid.fdroid.Utils; -import org.fdroid.fdroid.data.Apk; -import org.fdroid.fdroid.data.App; -import org.fdroid.fdroid.data.SanitizedFile; - -import java.io.File; -import java.security.NoSuchAlgorithmException; - -/** - * Downloads and verifies (against the Apk.hash) the apk file. - * If the file has previously been downloaded, it will make use of that - * instead, without going to the network to download a new one. - */ -public class ApkDownloader { - - private static final String TAG = "ApkDownloader"; - - public final String urlString; - - @NonNull private final Apk curApk; - @NonNull private final Context context; - @NonNull private SanitizedFile localFile; - @NonNull private final SanitizedFile potentiallyCachedFile; - private final LocalBroadcastManager localBroadcastManager; - - public ApkDownloader(@NonNull final Context context, @NonNull final App app, @NonNull final Apk apk, @NonNull final String repoAddress) { - this.context = context; - curApk = apk; - potentiallyCachedFile = new SanitizedFile(Utils.getApkCacheDir(context), apk.apkName); - urlString = Utils.getApkUrl(repoAddress, apk); - localBroadcastManager = LocalBroadcastManager.getInstance(context); - } - - private Hasher createHasher(File apkFile) { - Hasher hasher; - try { - hasher = new Hasher(curApk.hashType, apkFile); - } catch (NoSuchAlgorithmException e) { - Log.e(TAG, "Error verifying hash of cached apk at " + apkFile + ". " + - "I don't understand what the " + curApk.hashType + " hash algorithm is :("); - hasher = null; - } - return hasher; - } - - private boolean hashMatches(@NonNull final File apkFile) { - if (!apkFile.exists()) { - return false; - } - Hasher hasher = createHasher(apkFile); - return hasher != null && hasher.match(curApk.hash); - } - - /** - * If an existing cached version exists, and matches the hash of the apk we - * want to download, then we will return true. Otherwise, we return false - * (and remove the cached file - if it exists and didn't match the correct hash). - */ - private boolean verifyOrDelete(@NonNull final File apkFile) { - if (apkFile.exists()) { - if (hashMatches(apkFile)) { - Utils.debugLog(TAG, "Using cached apk at " + apkFile); - return true; - } - Utils.debugLog(TAG, "Not using cached apk at " + apkFile + "(hash doesn't match, will delete file)"); - delete(apkFile); - } - return false; - } - - private void delete(@NonNull final File file) { - if (file.exists()) { - if (!file.delete()) { - Log.w(TAG, "Could not delete file " + file); - } - } - } - - private void sendDownloadComplete() { - Utils.debugLog(TAG, "Download finished: " + localFile); - localBroadcastManager.unregisterReceiver(downloadCompleteReceiver); - } - - public void download() { - // Can we use the cached version? - if (verifyOrDelete(potentiallyCachedFile)) { - delete(localFile); - Utils.copyQuietly(potentiallyCachedFile, localFile); - sendDownloadComplete(); - return; - } - - Utils.debugLog(TAG, "Downloading apk from " + urlString + " to " + localFile); - localBroadcastManager.registerReceiver(downloadCompleteReceiver, - DownloaderService.getIntentFilter(urlString, Downloader.ACTION_COMPLETE)); - - DownloaderService.queue(context, urlString); - } - - private void sendProgressEvent(String status) { - Intent intent = new Intent(status); - intent.setData(Uri.parse(urlString)); - intent.putExtra(Downloader.EXTRA_DOWNLOAD_PATH, localFile.getAbsolutePath()); - localBroadcastManager.sendBroadcast(intent); - } - - // TODO move this code to somewhere more appropriate - BroadcastReceiver downloadCompleteReceiver = new BroadcastReceiver() { - @Override - public void onReceive(Context context, Intent intent) { - localFile = SanitizedFile.knownSanitized(intent.getStringExtra(Downloader.EXTRA_DOWNLOAD_PATH)); - if (!verifyOrDelete(localFile)) { - sendProgressEvent(Downloader.ACTION_INTERRUPTED); - Toast.makeText(context, R.string.corrupt_download, Toast.LENGTH_LONG).show(); - return; - } - - if (Preferences.get().shouldCacheApks()) { - Utils.debugLog(TAG, "Copying .apk file to cache at " + potentiallyCachedFile.getAbsolutePath()); - Utils.copyQuietly(localFile, potentiallyCachedFile); - } - - sendDownloadComplete(); - } - }; -} diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index 7f308433a..1b0f68ed6 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -34,7 +34,6 @@ import android.text.TextUtils; import android.util.Log; import org.fdroid.fdroid.Utils; -import org.fdroid.fdroid.compat.FileCompat; import org.fdroid.fdroid.data.SanitizedFile; import java.io.File; @@ -194,12 +193,6 @@ public class DownloaderService extends Service { } }); downloader.download(); - // 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); sendBroadcast(uri, Downloader.ACTION_COMPLETE, localFile); } catch (InterruptedException e) { sendBroadcast(uri, Downloader.ACTION_INTERRUPTED, localFile); diff --git a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java index 2137ccf5e..219f4c0b4 100644 --- a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java +++ b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java @@ -45,7 +45,6 @@ import org.fdroid.fdroid.installer.Installer; import org.fdroid.fdroid.localrepo.LocalRepoManager; import org.fdroid.fdroid.localrepo.SwapService; import org.fdroid.fdroid.localrepo.peers.Peer; -import org.fdroid.fdroid.net.ApkDownloader; import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.DownloaderService; @@ -783,8 +782,8 @@ public class SwapWorkflowActivity extends AppCompatActivity { } public void install(@NonNull final App app) { - final Apk apkToInstall = ApkProvider.Helper.find(this, app.packageName, app.suggestedVercode); - final ApkDownloader downloader = new ApkDownloader(this, app, apkToInstall, apkToInstall.repoAddress); + final Apk apk = ApkProvider.Helper.find(this, app.packageName, app.suggestedVercode); + String urlString = Utils.getApkUrl(apk.repoAddress, apk); downloadCompleteReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { @@ -793,8 +792,8 @@ public class SwapWorkflowActivity extends AppCompatActivity { } }; localBroadcastManager.registerReceiver(downloadCompleteReceiver, - DownloaderService.getIntentFilter(downloader.urlString, Downloader.ACTION_COMPLETE)); - downloader.download(); + DownloaderService.getIntentFilter(urlString, Downloader.ACTION_COMPLETE)); + DownloaderService.queue(this, urlString); } private void handleDownloadComplete(File apkFile, String packageName) {