From 24ed40bd341bc46559f1486a0b074be3274c76aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Mon, 6 Jun 2016 14:35:25 +0200 Subject: [PATCH 1/5] Move Apk verification and file sanitizing into own class * use getPackageArchiveInfo of AOSP instead of AndroidXMLDecompress * verify in InstallManagerService instead of Installer subclasses --- .../fdroid/fdroid/AndroidXMLDecompress.java | 172 ------------------ .../fdroid/fdroid/installer/ApkVerifier.java | 139 ++++++++++++++ .../fdroid/installer/DefaultInstaller.java | 13 +- .../fdroid/installer/ExtensionInstaller.java | 17 +- .../installer/InstallManagerService.java | 21 ++- .../fdroid/fdroid/installer/Installer.java | 88 --------- .../fdroid/installer/PrivilegedInstaller.java | 12 +- 7 files changed, 163 insertions(+), 299 deletions(-) delete mode 100644 app/src/main/java/org/fdroid/fdroid/AndroidXMLDecompress.java create mode 100644 app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java diff --git a/app/src/main/java/org/fdroid/fdroid/AndroidXMLDecompress.java b/app/src/main/java/org/fdroid/fdroid/AndroidXMLDecompress.java deleted file mode 100644 index 35fcc6a4b..000000000 --- a/app/src/main/java/org/fdroid/fdroid/AndroidXMLDecompress.java +++ /dev/null @@ -1,172 +0,0 @@ -/* - * Copyright (C) 2016 Hans-Christoph Steiner - * - * 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. - */ - -/* - Copyright (c) 2016, Liu Dong - All rights reserved. - - Redistribution and use in source and binary forms, with or without - modification, are permitted provided that the following conditions are met: - - * Redistributions of source code must retain the above copyright notice, this - list of conditions and the following disclaimer. - - * Redistributions in binary form must reproduce the above copyright notice, - this list of conditions and the following disclaimer in the documentation - and/or other materials provided with the distribution. - - * Neither the name of apk-parser nor the names of its - contributors may be used to endorse or promote products derived from - this software without specific prior written permission. - - THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" - AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE - DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE - FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL - DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR - SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER - CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, - OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE - OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ - -package org.fdroid.fdroid; - -import java.io.IOException; -import java.io.InputStream; -import java.util.HashMap; -import java.util.Map; -import java.util.zip.ZipEntry; -import java.util.zip.ZipFile; - -/** - * Parse the 'compressed' binary form of Android XML docs such as for - * {@code AndroidManifest.xml} in APK files. This is a very truncated - * version of apk-parser since currently, we only need the header from - * the binary XML AndroidManifest.xml. apk-parser provides full APK - * parsing, which is a lot more than what is needed. - * - * @see apk-parser - * @see Android Internals: Binary XML - * @see a binary XML parser - */ -public class AndroidXMLDecompress { - private static final int START_TAG = 0x00100102; - - /** - * Just get the XML attributes from the {@code } element. - * - * @return A key value map of the attributes, with values as {@link Object}s - */ - public static Map getManifestHeaderAttributes(String filename) throws IOException { - byte[] binaryXml = getManifestFromFilename(filename); - int numbStrings = littleEndianWord(binaryXml, 4 * 4); - int stringIndexTableOffset = 0x24; - int stringTableOffset = stringIndexTableOffset + numbStrings * 4; - int xmlTagOffset = littleEndianWord(binaryXml, 3 * 4); - for (int i = xmlTagOffset; i < binaryXml.length - 4; i += 4) { - if (littleEndianWord(binaryXml, i) == START_TAG) { - xmlTagOffset = i; - break; - } - } - int offset = xmlTagOffset; - - // we only need the first start tag - if (offset < binaryXml.length) { - int tag0 = littleEndianWord(binaryXml, offset); - - if (tag0 == START_TAG) { - int numbAttrs = littleEndianWord(binaryXml, offset + 7 * 4); - offset += 9 * 4; - - HashMap attributes = new HashMap<>(3); - for (int i = 0; i < numbAttrs; i++) { - int attributeNameStringIndex = littleEndianWord(binaryXml, offset + 1 * 4); - int attributeValueStringIndex = littleEndianWord(binaryXml, offset + 2 * 4); - int attributeResourceId = littleEndianWord(binaryXml, offset + 4 * 4); - offset += 5 * 4; - - String attributeName = getString(binaryXml, stringIndexTableOffset, stringTableOffset, attributeNameStringIndex); - Object attributeValue; - if (attributeValueStringIndex != -1) { - attributeValue = getString(binaryXml, stringIndexTableOffset, stringTableOffset, attributeValueStringIndex); - } else { - attributeValue = attributeResourceId; - } - attributes.put(attributeName, attributeValue); - } - return attributes; - } - } - return new HashMap<>(0); - } - - private static byte[] getManifestFromFilename(String filename) throws IOException { - InputStream is = null; - ZipFile zip = null; - int size = 0; - - if (filename.endsWith(".apk")) { - zip = new ZipFile(filename); - ZipEntry ze = zip.getEntry("AndroidManifest.xml"); - is = zip.getInputStream(ze); - size = (int) ze.getSize(); - } else { - throw new RuntimeException("This only works on APK files!"); - } - byte[] buf = new byte[size]; - is.read(buf); - - is.close(); - zip.close(); - return buf; - } - - private static String getString(byte[] bytes, int stringIndexTableOffset, int stringTableOffset, int stringIndex) { - if (stringIndex < 0) { - return null; - } - int stringOffset = stringTableOffset + littleEndianWord(bytes, stringIndexTableOffset + stringIndex * 4); - return getStringAt(bytes, stringOffset); - } - - private static String getStringAt(byte[] bytes, int stringOffset) { - int length = bytes[stringOffset + 1] << 8 & 0xff00 | bytes[stringOffset] & 0xff; - byte[] chars = new byte[length]; - for (int i = 0; i < length; i++) { - chars[i] = bytes[stringOffset + 2 + i * 2]; - } - return new String(chars); - } - - /** - * Return the little endian 32-bit word from the byte array at offset - */ - private static int littleEndianWord(byte[] bytes, int offset) { - return bytes[offset + 3] - << 24 & 0xff000000 - | bytes[offset + 2] - << 16 & 0xff0000 - | bytes[offset + 1] - << 8 & 0xff00 - | bytes[offset] & 0xFF; - } -} diff --git a/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java b/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java new file mode 100644 index 000000000..298631413 --- /dev/null +++ b/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java @@ -0,0 +1,139 @@ +/* + * Copyright (C) 2016 Dominik Schürmann + * + * 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.installer; + +import android.content.Context; +import android.content.pm.PackageInfo; +import android.content.pm.PackageManager; +import android.net.Uri; + +import org.apache.commons.io.FileUtils; +import org.fdroid.fdroid.Hasher; +import org.fdroid.fdroid.data.Apk; +import org.fdroid.fdroid.data.SanitizedFile; + +import java.io.File; +import java.io.IOException; +import java.security.NoSuchAlgorithmException; + +public class ApkVerifier { + + Context context; + Uri localApkUri; + Apk apk; + PackageManager pm; + + ApkVerifier(Context context, Uri localApkUri, Apk apk) { + this.context = context; + this.localApkUri = localApkUri; + this.apk = apk; + this.pm = context.getPackageManager(); + } + + public void basicVerify() throws ApkVerificationException { + PackageInfo localApkInfo = pm.getPackageArchiveInfo( + localApkUri.getPath(), PackageManager.GET_PERMISSIONS); + if (localApkInfo == null) { + throw new ApkVerificationException("parsing apk failed!"); + } + + // check if the apk has the expected packageName + if (localApkInfo.packageName == null || !apk.packageName.equals(localApkInfo.packageName)) { + throw new ApkVerificationException("apk has unexpected packageName!"); + } + + if (localApkInfo.versionCode < 0) { + throw new ApkVerificationException("apk has no valid versionCode!"); + } + } + + public Uri getSafeUri() throws ApkVerificationException { + File apkFile = new File(localApkUri.getPath()); + + SanitizedFile sanitizedApkFile = null; + try { + + /* 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 */ + sanitizedApkFile = SanitizedFile.knownSanitized(File.createTempFile("install-", ".apk", + context.getFilesDir())); + FileUtils.copyFile(apkFile, sanitizedApkFile); + if (!verifyApkFile(sanitizedApkFile, apk.hash, apk.hashType)) { + FileUtils.deleteQuietly(apkFile); + throw new ApkVerificationException(apkFile + " failed to verify!"); + } + apkFile = null; // ensure this is not used now that its copied to apkToInstall + + // 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. + sanitizedApkFile.setReadable(true, false); + + } catch (IOException | NoSuchAlgorithmException e) { + throw new ApkVerificationException(e); + } finally { + // 20 minutes the start of the install process, delete the file + final File apkToDelete = sanitizedApkFile; + new Thread() { + @Override + public void run() { + android.os.Process.setThreadPriority(android.os.Process.THREAD_PRIORITY_LOWEST); + try { + Thread.sleep(1200000); + } catch (InterruptedException e) { + e.printStackTrace(); + } finally { + FileUtils.deleteQuietly(apkToDelete); + } + } + }.start(); + } + + return Uri.fromFile(sanitizedApkFile); + } + + /** + * Checks the APK file against the provided hash, returning whether it is a match. + */ + static boolean verifyApkFile(File apkFile, String hash, String hashType) + throws NoSuchAlgorithmException { + if (!apkFile.exists()) { + return false; + } + Hasher hasher = new Hasher(hashType, apkFile); + return hasher.match(hash); + } + + public static class ApkVerificationException extends Exception { + + public ApkVerificationException(String message) { + super(message); + } + + public ApkVerificationException(Throwable cause) { + super(cause); + } + } + +} diff --git a/app/src/main/java/org/fdroid/fdroid/installer/DefaultInstaller.java b/app/src/main/java/org/fdroid/fdroid/installer/DefaultInstaller.java index b3bcf6218..7f48e7ca3 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/DefaultInstaller.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/DefaultInstaller.java @@ -23,7 +23,6 @@ import android.app.PendingIntent; import android.content.Context; import android.content.Intent; import android.net.Uri; -import android.util.Log; import org.fdroid.fdroid.Utils; @@ -50,20 +49,10 @@ public class DefaultInstaller extends Installer { Utils.debugLog(TAG, "DefaultInstaller uri: " + localApkUri + " file: " + new File(localApkUri.getPath())); - Uri sanitizedUri; - try { - sanitizedUri = Installer.prepareApkFile(context, localApkUri, packageName); - } catch (Installer.InstallFailedException e) { - Log.e(TAG, "prepareApkFile failed", e); - sendBroadcastInstall(downloadUri, Installer.ACTION_INSTALL_INTERRUPTED, - e.getMessage()); - return; - } - Intent installIntent = new Intent(context, DefaultInstallerActivity.class); installIntent.setAction(DefaultInstallerActivity.ACTION_INSTALL_PACKAGE); installIntent.putExtra(Installer.EXTRA_DOWNLOAD_URI, downloadUri); - installIntent.setData(sanitizedUri); + installIntent.setData(localApkUri); PendingIntent installPendingIntent = PendingIntent.getActivity( context.getApplicationContext(), diff --git a/app/src/main/java/org/fdroid/fdroid/installer/ExtensionInstaller.java b/app/src/main/java/org/fdroid/fdroid/installer/ExtensionInstaller.java index adb61d16d..b9902bd58 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/ExtensionInstaller.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/ExtensionInstaller.java @@ -23,7 +23,6 @@ import android.app.PendingIntent; import android.content.Context; import android.content.Intent; import android.net.Uri; -import android.util.Log; import org.fdroid.fdroid.BuildConfig; import org.fdroid.fdroid.privileged.install.InstallExtensionDialogActivity; @@ -47,25 +46,17 @@ public class ExtensionInstaller extends Installer { @Override protected void installPackage(Uri localApkUri, Uri downloadUri, String packageName) { - Uri sanitizedUri; - try { - sanitizedUri = Installer.prepareApkFile(context, localApkUri, packageName); - } catch (InstallFailedException e) { - Log.e(TAG, "prepareApkFile failed", e); - sendBroadcastInstall(downloadUri, Installer.ACTION_INSTALL_INTERRUPTED, e.getMessage()); - return; - } - // 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 + // NOTE: Disabled for debug builds to be able to test official extension from repo ApkSignatureVerifier signatureVerifier = new ApkSignatureVerifier(context); - if (!BuildConfig.DEBUG && !signatureVerifier.hasFDroidSignature(new File(sanitizedUri.getPath()))) { + if (!BuildConfig.DEBUG && + !signatureVerifier.hasFDroidSignature(new File(localApkUri.getPath()))) { sendBroadcastInstall(downloadUri, Installer.ACTION_INSTALL_INTERRUPTED, "APK signature of extension not correct!"); } Intent installIntent = new Intent(context, InstallExtensionDialogActivity.class); installIntent.setAction(InstallExtensionDialogActivity.ACTION_INSTALL); - installIntent.setData(sanitizedUri); + installIntent.setData(localApkUri); PendingIntent installPendingIntent = PendingIntent.getActivity( context.getApplicationContext(), diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java index c0e2fab35..d80c4de84 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -15,6 +15,7 @@ import android.support.v4.app.NotificationCompat; import android.support.v4.app.TaskStackBuilder; import android.support.v4.content.LocalBroadcastManager; import android.text.TextUtils; +import android.util.Log; import org.fdroid.fdroid.AppDetails; import org.fdroid.fdroid.R; @@ -186,7 +187,7 @@ public class InstallManagerService extends Service { private static boolean apkIsCached(File apkFile, Apk apkToCheck) { try { return apkFile.length() == apkToCheck.size && - Installer.verifyApkFile(apkFile, apkToCheck.hash, apkToCheck.hashType); + ApkVerifier.verifyApkFile(apkFile, apkToCheck.hash, apkToCheck.hashType); } catch (NoSuchAlgorithmException e) { e.printStackTrace(); return false; @@ -225,7 +226,6 @@ public class InstallManagerService extends Service { BroadcastReceiver completeReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { - // elsewhere called urlString Uri downloadUri = intent.getData(); String urlString = downloadUri.toString(); File localFile = new File(intent.getStringExtra(Downloader.EXTRA_DOWNLOAD_PATH)); @@ -237,7 +237,22 @@ public class InstallManagerService extends Service { registerInstallerReceivers(downloadUri); Apk apk = ACTIVE_APKS.get(urlString); - InstallerService.install(context, localApkUri, downloadUri, apk.packageName); + + Uri sanitizedUri; + try { + ApkVerifier apkVerifier = new ApkVerifier(context, localApkUri, apk); + apkVerifier.basicVerify(); + sanitizedUri = apkVerifier.getSafeUri(); + } catch (ApkVerifier.ApkVerificationException e) { + Log.e(TAG, "ApkVerifier failed", e); + String title = String.format( + getString(R.string.install_error_notify_title), + apk.packageName); + notifyError(urlString, title, e.getMessage()); + return; + } + + InstallerService.install(context, sanitizedUri, downloadUri, apk.packageName); } }; BroadcastReceiver interruptedReceiver = new BroadcastReceiver() { 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 660508358..fc4ae1012 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/Installer.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/Installer.java @@ -28,22 +28,13 @@ import android.os.PatternMatcher; import android.support.v4.content.LocalBroadcastManager; import android.text.TextUtils; -import org.apache.commons.io.FileUtils; -import org.fdroid.fdroid.AndroidXMLDecompress; -import org.fdroid.fdroid.Hasher; import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.ApkProvider; -import org.fdroid.fdroid.data.SanitizedFile; import org.fdroid.fdroid.privileged.views.AppDiff; import org.fdroid.fdroid.privileged.views.AppSecurityPermissions; import org.fdroid.fdroid.privileged.views.InstallConfirmActivity; import org.fdroid.fdroid.privileged.views.UninstallDialogActivity; -import java.io.File; -import java.io.IOException; -import java.security.NoSuchAlgorithmException; -import java.util.Map; - /** * Handles the actual install process. Subclasses implement the details. */ @@ -92,73 +83,6 @@ public abstract class Installer { localBroadcastManager = LocalBroadcastManager.getInstance(context); } - static Uri prepareApkFile(Context context, Uri uri, String packageName) - throws InstallFailedException { - - File apkFile = new File(uri.getPath()); - - SanitizedFile sanitizedApkFile = null; - try { - Map attributes = AndroidXMLDecompress.getManifestHeaderAttributes(apkFile.getAbsolutePath()); - - /* This isn't really needed, but might as well since we have the data already */ - if (attributes.containsKey("packageName") && !TextUtils.equals(packageName, (String) attributes.get("packageName"))) { - throw new InstallFailedException(uri + " has packageName that clashes with " + packageName); - } - - if (!attributes.containsKey("versionCode")) { - throw new InstallFailedException(uri + " is missing versionCode!"); - } - int versionCode = (Integer) attributes.get("versionCode"); - Apk apk = ApkProvider.Helper.find(context, 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 */ - sanitizedApkFile = SanitizedFile.knownSanitized(File.createTempFile("install-", ".apk", - context.getFilesDir())); - FileUtils.copyFile(apkFile, sanitizedApkFile); - if (!verifyApkFile(sanitizedApkFile, 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 - - // 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. - sanitizedApkFile.setReadable(true, false); - - } catch (NumberFormatException | IOException | NoSuchAlgorithmException e) { - throw new InstallFailedException(e); - } catch (ClassCastException e) { - throw new InstallFailedException("F-Droid Privileged can only be updated using an activity!"); - } finally { - // 20 minutes the start of the install process, delete the file - final File apkToDelete = sanitizedApkFile; - new Thread() { - @Override - public void run() { - android.os.Process.setThreadPriority(android.os.Process.THREAD_PRIORITY_LOWEST); - try { - Thread.sleep(1200000); - } catch (InterruptedException e) { - e.printStackTrace(); - } finally { - FileUtils.deleteQuietly(apkToDelete); - } - } - }.start(); - } - - return Uri.fromFile(sanitizedApkFile); - } - /** * Returns permission screen for given apk. * @@ -227,18 +151,6 @@ public abstract class Installer { return intent; } - /** - * Checks the APK file against the provided hash, returning whether it is a match. - */ - static boolean verifyApkFile(File apkFile, String hash, String hashType) - throws NoSuchAlgorithmException { - if (!apkFile.exists()) { - return false; - } - Hasher hasher = new Hasher(hashType, apkFile); - return hasher.match(hash); - } - void sendBroadcastInstall(Uri downloadUri, String action, PendingIntent pendingIntent) { sendBroadcastInstall(downloadUri, action, pendingIntent, null); diff --git a/app/src/main/java/org/fdroid/fdroid/installer/PrivilegedInstaller.java b/app/src/main/java/org/fdroid/fdroid/installer/PrivilegedInstaller.java index 66bf38e88..ec8039880 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/PrivilegedInstaller.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/PrivilegedInstaller.java @@ -300,16 +300,6 @@ public class PrivilegedInstaller extends Installer { protected void installPackage(final Uri localApkUri, final Uri downloadUri, String packageName) { sendBroadcastInstall(downloadUri, Installer.ACTION_INSTALL_STARTED); - final Uri sanitizedUri; - try { - sanitizedUri = Installer.prepareApkFile(context, localApkUri, packageName); - } catch (Installer.InstallFailedException e) { - Log.e(TAG, "prepareApkFile failed", e); - sendBroadcastInstall(downloadUri, Installer.ACTION_INSTALL_INTERRUPTED, - e.getMessage()); - return; - } - ServiceConnection mServiceConnection = new ServiceConnection() { public void onServiceConnected(ComponentName name, IBinder service) { IPrivilegedService privService = IPrivilegedService.Stub.asInterface(service); @@ -335,7 +325,7 @@ public class PrivilegedInstaller extends Installer { return; } - privService.installPackage(sanitizedUri, ACTION_INSTALL_REPLACE_EXISTING, + privService.installPackage(localApkUri, ACTION_INSTALL_REPLACE_EXISTING, null, callback); } catch (RemoteException e) { Log.e(TAG, "RemoteException", e); From 1652c32d5124ec1240cf94f059197a112a56536e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Mon, 6 Jun 2016 18:06:39 +0200 Subject: [PATCH 2/5] Move permission getter in Apk class Also fix permissions list based on the fact that F-Droid deletes android.permission. prefix only for default Android permissions, not custom ones. --- .../java/org/fdroid/fdroid/AppDetails.java | 7 ++-- .../java/org/fdroid/fdroid/Permission.java | 14 +------ .../main/java/org/fdroid/fdroid/data/Apk.java | 39 +++++++++++++++++++ .../fdroid/privileged/views/AppDiff.java | 13 +------ 4 files changed, 46 insertions(+), 27 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index 1f3f0db53..71fa35b30 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -92,6 +92,7 @@ import org.fdroid.fdroid.installer.InstallerService; import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.DownloaderService; +import java.util.ArrayList; import java.util.Iterator; import java.util.List; @@ -1375,14 +1376,12 @@ public class AppDetails extends AppCompatActivity { private void buildPermissionInfo() { final TextView permissionListView = (TextView) llViewMorePermissions.findViewById(R.id.permissions_list); - CommaSeparatedList permsList = appDetails.getApks().getItem(0).permissions; + ArrayList permsList = appDetails.getApks().getItem(0).getFullPermissionList(); if (permsList == null) { permissionListView.setText(R.string.no_permissions); } else { - Iterator permissions = permsList.iterator(); StringBuilder sb = new StringBuilder(); - while (permissions.hasNext()) { - final String permissionName = permissions.next(); + for (String permissionName : permsList) { try { final Permission permission = new Permission(getActivity(), permissionName); // TODO: Make this list RTL friendly diff --git a/app/src/main/java/org/fdroid/fdroid/Permission.java b/app/src/main/java/org/fdroid/fdroid/Permission.java index 986290b61..8a688854e 100644 --- a/app/src/main/java/org/fdroid/fdroid/Permission.java +++ b/app/src/main/java/org/fdroid/fdroid/Permission.java @@ -4,7 +4,7 @@ import android.content.Context; import android.content.pm.PackageManager; import android.content.pm.PermissionInfo; -class Permission { +public class Permission { private final PackageManager packageManager; private final PermissionInfo permissionInfo; @@ -13,17 +13,7 @@ class Permission { throws PackageManager.NameNotFoundException { this.packageManager = context.getPackageManager(); this.permissionInfo = this.packageManager.getPermissionInfo( - fdroidToAndroid(permission), PackageManager.GET_META_DATA); - } - - /** - * It appears that all of the permissions in android.Manifest.permissions - * are prefixed with "android.permission." and then the constant name. - * FDroid just includes the constant name in the apk list, so we prefix it - * with "android.permission." - */ - private static String fdroidToAndroid(String permission) { - return "android.permission." + permission; + permission, PackageManager.GET_META_DATA); } public CharSequence getName() { diff --git a/app/src/main/java/org/fdroid/fdroid/data/Apk.java b/app/src/main/java/org/fdroid/fdroid/data/Apk.java index 1f6b92220..5d90b4e2a 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Apk.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Apk.java @@ -5,9 +5,11 @@ import android.content.ContentValues; import android.database.Cursor; import android.os.Build; import android.os.Parcelable; +import android.util.Log; import org.fdroid.fdroid.Utils; +import java.util.ArrayList; import java.util.Date; public class Apk extends ValueObject implements Comparable { @@ -141,6 +143,43 @@ public class Apk extends ValueObject implements Comparable { return repoAddress + "/" + apkName.replace(" ", "%20"); } + public ArrayList getFullPermissionList() { + if (this.permissions == null) { + return null; + } + + ArrayList permissionsFull = new ArrayList<>(); + for (String perm : this.permissions) { + permissionsFull.add(fdroidToAndroidPermission(perm)); + } + return permissionsFull; + } + + public String[] getFullPermissionsArray() { + ArrayList fullPermissions = getFullPermissionList(); + if (fullPermissions == null) { + return null; + } + + return fullPermissions.toArray(new String[fullPermissions.size()]); + } + + /** + * It appears that the default Android permissions in android.Manifest.permissions + * are prefixed with "android.permission." and then the constant name. + * FDroid just includes the constant name in the apk list, so we prefix it + * with "android.permission." + * + * see https://gitlab.com/fdroid/fdroidserver/blob/master/fdroidserver/update.py#L535# + */ + public static String fdroidToAndroidPermission(String permission) { + if (!permission.contains(".")) { + return "android.permission." + permission; + } + + return permission; + } + @Override public String toString() { return packageName + " (version " + versionCode + ")"; diff --git a/app/src/main/java/org/fdroid/fdroid/privileged/views/AppDiff.java b/app/src/main/java/org/fdroid/fdroid/privileged/views/AppDiff.java index ee61311b3..58d10cfdb 100644 --- a/app/src/main/java/org/fdroid/fdroid/privileged/views/AppDiff.java +++ b/app/src/main/java/org/fdroid/fdroid/privileged/views/AppDiff.java @@ -24,6 +24,7 @@ import android.content.pm.PackageInfo; import android.content.pm.PackageManager; import android.os.Build; +import org.fdroid.fdroid.Permission; import org.fdroid.fdroid.data.Apk; import java.util.ArrayList; @@ -45,17 +46,7 @@ public class AppDiff { mPkgInfo = new PackageInfo(); mPkgInfo.packageName = apk.packageName; mPkgInfo.applicationInfo = new ApplicationInfo(); - - if (apk.permissions == null) { - mPkgInfo.requestedPermissions = null; - } else { - // TODO: duplicate code with Permission.fdroidToAndroid - ArrayList permissionsFixed = new ArrayList<>(); - for (String perm : apk.permissions.toArrayList()) { - permissionsFixed.add("android.permission." + perm); - } - mPkgInfo.requestedPermissions = permissionsFixed.toArray(new String[permissionsFixed.size()]); - } + mPkgInfo.requestedPermissions = apk.getFullPermissionsArray(); init(); } From 4bed9d67c53a2ebbdc0b856b7a1dcde96dfac28b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Wed, 8 Jun 2016 13:54:56 +0200 Subject: [PATCH 3/5] Verify permissions of downloaded apk --- .../main/java/org/fdroid/fdroid/data/Apk.java | 12 +++--- .../fdroid/fdroid/installer/ApkVerifier.java | 38 ++++++++++++++++--- .../installer/InstallManagerService.java | 2 +- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/Apk.java b/app/src/main/java/org/fdroid/fdroid/data/Apk.java index 5d90b4e2a..81d84d6ce 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Apk.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Apk.java @@ -5,12 +5,12 @@ import android.content.ContentValues; import android.database.Cursor; import android.os.Build; import android.os.Parcelable; -import android.util.Log; import org.fdroid.fdroid.Utils; import java.util.ArrayList; import java.util.Date; +import java.util.HashSet; public class Apk extends ValueObject implements Comparable { @@ -145,7 +145,7 @@ public class Apk extends ValueObject implements Comparable { public ArrayList getFullPermissionList() { if (this.permissions == null) { - return null; + return new ArrayList<>(); } ArrayList permissionsFull = new ArrayList<>(); @@ -157,13 +157,13 @@ public class Apk extends ValueObject implements Comparable { public String[] getFullPermissionsArray() { ArrayList fullPermissions = getFullPermissionList(); - if (fullPermissions == null) { - return null; - } - return fullPermissions.toArray(new String[fullPermissions.size()]); } + public HashSet getFullPermissionsSet() { + return new HashSet<>(getFullPermissionList()); + } + /** * It appears that the default Android permissions in android.Manifest.permissions * are prefixed with "android.permission." and then the constant name. diff --git a/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java b/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java index 298631413..c818df9a5 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java @@ -23,31 +23,37 @@ import android.content.Context; import android.content.pm.PackageInfo; import android.content.pm.PackageManager; import android.net.Uri; +import android.util.Log; import org.apache.commons.io.FileUtils; import org.fdroid.fdroid.Hasher; +import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.SanitizedFile; import java.io.File; import java.io.IOException; import java.security.NoSuchAlgorithmException; +import java.util.Arrays; +import java.util.HashSet; public class ApkVerifier { + private static final String TAG = "ApkVerifier"; + Context context; Uri localApkUri; - Apk apk; + Apk expectedApk; PackageManager pm; - ApkVerifier(Context context, Uri localApkUri, Apk apk) { + ApkVerifier(Context context, Uri localApkUri, Apk expectedApk) { this.context = context; this.localApkUri = localApkUri; - this.apk = apk; + this.expectedApk = expectedApk; this.pm = context.getPackageManager(); } - public void basicVerify() throws ApkVerificationException { + public void verifyApk() throws ApkVerificationException { PackageInfo localApkInfo = pm.getPackageArchiveInfo( localApkUri.getPath(), PackageManager.GET_PERMISSIONS); if (localApkInfo == null) { @@ -55,13 +61,33 @@ public class ApkVerifier { } // check if the apk has the expected packageName - if (localApkInfo.packageName == null || !apk.packageName.equals(localApkInfo.packageName)) { + if (localApkInfo.packageName == null || !localApkInfo.packageName.equals(expectedApk.packageName)) { throw new ApkVerificationException("apk has unexpected packageName!"); } if (localApkInfo.versionCode < 0) { throw new ApkVerificationException("apk has no valid versionCode!"); } + + // verify permissions, important for unattended installer + HashSet localPermissions = getLocalPermissionsSet(localApkInfo); + HashSet expectedPermissions = expectedApk.getFullPermissionsSet(); + Utils.debugLog(TAG, "localPermissions: " + localPermissions); + Utils.debugLog(TAG, "expectedPermissions: " + expectedPermissions); + if (!localPermissions.equals(expectedPermissions)) { + throw new ApkVerificationException("permissions of apk not equals expected permissions!"); + } + + // TODO: check target sdk + } + + private HashSet getLocalPermissionsSet(PackageInfo localApkInfo) { + String[] localPermissions = localApkInfo.requestedPermissions; + if (localPermissions == null) { + return new HashSet<>(); + } + + return new HashSet<>(Arrays.asList(localApkInfo.requestedPermissions)); } public Uri getSafeUri() throws ApkVerificationException { @@ -77,7 +103,7 @@ public class ApkVerifier { sanitizedApkFile = SanitizedFile.knownSanitized(File.createTempFile("install-", ".apk", context.getFilesDir())); FileUtils.copyFile(apkFile, sanitizedApkFile); - if (!verifyApkFile(sanitizedApkFile, apk.hash, apk.hashType)) { + if (!verifyApkFile(sanitizedApkFile, expectedApk.hash, expectedApk.hashType)) { FileUtils.deleteQuietly(apkFile); throw new ApkVerificationException(apkFile + " failed to verify!"); } diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java index d80c4de84..1486b0215 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -241,7 +241,7 @@ public class InstallManagerService extends Service { Uri sanitizedUri; try { ApkVerifier apkVerifier = new ApkVerifier(context, localApkUri, apk); - apkVerifier.basicVerify(); + apkVerifier.verifyApk(); sanitizedUri = apkVerifier.getSafeUri(); } catch (ApkVerifier.ApkVerificationException e) { Log.e(TAG, "ApkVerifier failed", e); From 739bd00257957aa5ea3082ce68b03189ff51314c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Wed, 8 Jun 2016 14:01:17 +0200 Subject: [PATCH 4/5] Documentation and cleanup --- .../java/org/fdroid/fdroid/AppDetails.java | 2 -- .../fdroid/fdroid/installer/ApkVerifier.java | 23 +++++++++++++------ .../fdroid/installer/ExtensionInstaller.java | 2 -- .../fdroid/privileged/views/AppDiff.java | 6 ----- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index 71fa35b30..ead493e76 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -78,7 +78,6 @@ import com.nostra13.universalimageloader.core.DisplayImageOptions; import com.nostra13.universalimageloader.core.ImageLoader; import com.nostra13.universalimageloader.core.assist.ImageScaleType; -import org.fdroid.fdroid.Utils.CommaSeparatedList; import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.ApkProvider; import org.fdroid.fdroid.data.App; @@ -93,7 +92,6 @@ import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.DownloaderService; import java.util.ArrayList; -import java.util.Iterator; import java.util.List; public class AppDetails extends AppCompatActivity { diff --git a/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java b/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java index c818df9a5..7b38093b2 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/ApkVerifier.java @@ -23,7 +23,7 @@ import android.content.Context; import android.content.pm.PackageInfo; import android.content.pm.PackageManager; import android.net.Uri; -import android.util.Log; +import android.text.TextUtils; import org.apache.commons.io.FileUtils; import org.fdroid.fdroid.Hasher; @@ -37,14 +37,19 @@ import java.security.NoSuchAlgorithmException; import java.util.Arrays; import java.util.HashSet; +/** + * This ApkVerifier verifies that the downloaded apk corresponds to the Apk information + * displayed to the user. This is especially important in case an unattended installer + * has been used which displays permissions before download. + */ public class ApkVerifier { private static final String TAG = "ApkVerifier"; - Context context; - Uri localApkUri; - Apk expectedApk; - PackageManager pm; + private final Context context; + private final Uri localApkUri; + private final Apk expectedApk; + private final PackageManager pm; ApkVerifier(Context context, Uri localApkUri, Apk expectedApk) { this.context = context; @@ -54,14 +59,15 @@ public class ApkVerifier { } public void verifyApk() throws ApkVerificationException { + // parse downloaded apk file locally PackageInfo localApkInfo = pm.getPackageArchiveInfo( localApkUri.getPath(), PackageManager.GET_PERMISSIONS); if (localApkInfo == null) { - throw new ApkVerificationException("parsing apk failed!"); + throw new ApkVerificationException("parsing apk file failed!"); } // check if the apk has the expected packageName - if (localApkInfo.packageName == null || !localApkInfo.packageName.equals(expectedApk.packageName)) { + if (!TextUtils.equals(localApkInfo.packageName, expectedApk.packageName)) { throw new ApkVerificationException("apk has unexpected packageName!"); } @@ -78,7 +84,10 @@ public class ApkVerifier { throw new ApkVerificationException("permissions of apk not equals expected permissions!"); } + int localTargetSdkVersion = localApkInfo.applicationInfo.targetSdkVersion; + Utils.debugLog(TAG, "localTargetSdkVersion: " + localTargetSdkVersion); // TODO: check target sdk + } private HashSet getLocalPermissionsSet(PackageInfo localApkInfo) { diff --git a/app/src/main/java/org/fdroid/fdroid/installer/ExtensionInstaller.java b/app/src/main/java/org/fdroid/fdroid/installer/ExtensionInstaller.java index b9902bd58..a17565dd3 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/ExtensionInstaller.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/ExtensionInstaller.java @@ -38,8 +38,6 @@ import java.io.File; */ public class ExtensionInstaller extends Installer { - private static final String TAG = "ExtensionInstaller"; - ExtensionInstaller(Context context) { super(context); } diff --git a/app/src/main/java/org/fdroid/fdroid/privileged/views/AppDiff.java b/app/src/main/java/org/fdroid/fdroid/privileged/views/AppDiff.java index 58d10cfdb..3753093d2 100644 --- a/app/src/main/java/org/fdroid/fdroid/privileged/views/AppDiff.java +++ b/app/src/main/java/org/fdroid/fdroid/privileged/views/AppDiff.java @@ -18,18 +18,12 @@ package org.fdroid.fdroid.privileged.views; -import android.annotation.TargetApi; import android.content.pm.ApplicationInfo; import android.content.pm.PackageInfo; import android.content.pm.PackageManager; -import android.os.Build; -import org.fdroid.fdroid.Permission; import org.fdroid.fdroid.data.Apk; -import java.util.ArrayList; - -@TargetApi(Build.VERSION_CODES.M) public class AppDiff { private final PackageManager mPm; From c1abd0936226c82142c22e78185bebac1c9e68df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Thu, 9 Jun 2016 12:36:43 +0200 Subject: [PATCH 5/5] Remove AndroidXMLDecompressTest --- .../fdroid/AndroidXMLDecompressTest.java | 51 ------------------- 1 file changed, 51 deletions(-) delete mode 100644 app/src/test/java/org/fdroid/fdroid/AndroidXMLDecompressTest.java diff --git a/app/src/test/java/org/fdroid/fdroid/AndroidXMLDecompressTest.java b/app/src/test/java/org/fdroid/fdroid/AndroidXMLDecompressTest.java deleted file mode 100644 index ead537730..000000000 --- a/app/src/test/java/org/fdroid/fdroid/AndroidXMLDecompressTest.java +++ /dev/null @@ -1,51 +0,0 @@ -package org.fdroid.fdroid; - -import org.junit.Test; - -import java.io.File; -import java.io.FilenameFilter; -import java.io.IOException; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.Map; - -public class AndroidXMLDecompressTest { - - String[] testDirNames = { - System.getProperty("user.dir") + "/src/test/assets", - System.getProperty("user.dir") + "/build/outputs/apk", - System.getenv("HOME") + "/fdroid/repo", - }; - - FilenameFilter apkFilter = new FilenameFilter() { - @Override - public boolean accept(File dir, String filename) { - return filename.endsWith(".apk"); - } - }; - - @Test - public void testParseVersionCode() throws IOException { - for (File f : getFilesToTest()) { - System.out.println("\n" + f); - Map map = AndroidXMLDecompress.getManifestHeaderAttributes(f.getAbsolutePath()); - for (String key : map.keySet()) { - System.out.println(key + "=\"" + map.get(key) + "\""); - } - } - } - - private List getFilesToTest() { - ArrayList apkFiles = new ArrayList(5); - for (String dirName : testDirNames) { - System.out.println("looking in " + dirName); - File dir = new File(dirName); - File[] files = dir.listFiles(apkFilter); - if (files != null) { - apkFiles.addAll(Arrays.asList(files)); - } - } - return apkFiles; - } -}