Merge pull request from GHSA-wxrm-jhpf-vp6v

Fix preferences import vulnerability

app/src/main/java/org/schabi/newpipe/error/UserAction.java

@@ -6,6 +6,7 @@ package org.schabi.newpipe.error;
 public enum UserAction {
     USER_REPORT("user report"),
     UI_ERROR("ui error"),
+    DATABASE_IMPORT_EXPORT("database import or export"),
     SUBSCRIPTION_CHANGE("subscription change"),
     SUBSCRIPTION_UPDATE("subscription update"),
     SUBSCRIPTION_GET("get subscription"),

app/src/main/java/org/schabi/newpipe/settings/BackupRestoreSettingsFragment.java

@@ -21,9 +21,15 @@ import androidx.core.content.ContextCompat;
 import androidx.preference.Preference;
 import androidx.preference.PreferenceManager;
 
+import com.grack.nanojson.JsonParserException;
+
 import org.schabi.newpipe.NewPipeDatabase;
 import org.schabi.newpipe.R;
+import org.schabi.newpipe.error.ErrorInfo;
 import org.schabi.newpipe.error.ErrorUtil;
+import org.schabi.newpipe.error.UserAction;
+import org.schabi.newpipe.settings.export.BackupFileLocator;
+import org.schabi.newpipe.settings.export.ImportExportManager;
 import org.schabi.newpipe.streams.io.NoFileManagerSafeGuard;
 import org.schabi.newpipe.streams.io.StoredFileHelper;
 import org.schabi.newpipe.util.NavigationHelper;
@@ -42,7 +48,7 @@ public class BackupRestoreSettingsFragment extends BasePreferenceFragment {
 
     private final SimpleDateFormat exportDateFormat =
             new SimpleDateFormat("yyyyMMdd_HHmmss", Locale.US);
-    private ContentSettingsManager manager;
+    private ImportExportManager manager;
     private String importExportDataPathKey;
     private final ActivityResultLauncher<Intent> requestImportPathLauncher =
             registerForActivityResult(new ActivityResultContracts.StartActivityForResult(),
@@ -57,8 +63,7 @@ public class BackupRestoreSettingsFragment extends BasePreferenceFragment {
                                     @Nullable final String rootKey) {
         final File homeDir = ContextCompat.getDataDir(requireContext());
         Objects.requireNonNull(homeDir);
-        manager = new ContentSettingsManager(new NewPipeFileLocator(homeDir));
+        manager = new ImportExportManager(new BackupFileLocator(homeDir));
-        manager.deleteSettingsFile();
 
         importExportDataPathKey = getString(R.string.import_export_data_path);
 
@@ -165,7 +170,7 @@ public class BackupRestoreSettingsFragment extends BasePreferenceFragment {
             Toast.makeText(requireContext(), R.string.export_complete_toast, Toast.LENGTH_SHORT)
                     .show();
         } catch (final Exception e) {
-            ErrorUtil.showUiErrorSnackbar(this, "Exporting database", e);
+            showErrorSnackbar(e, "Exporting database and settings");
         }
     }
 
@@ -182,6 +187,7 @@ public class BackupRestoreSettingsFragment extends BasePreferenceFragment {
                 throw new IOException("Could not create databases dir");
             }
 
+            // replace the current database
             if (!manager.extractDb(file)) {
                 Toast.makeText(requireContext(), R.string.could_not_import_all_files,
                                 Toast.LENGTH_LONG)
@@ -189,9 +195,13 @@ public class BackupRestoreSettingsFragment extends BasePreferenceFragment {
             }
 
             // if settings file exist, ask if it should be imported.
-            if (manager.extractSettings(file)) {
+            final boolean hasJsonPrefs = manager.exportHasJsonPrefs(file);
+            if (hasJsonPrefs || manager.exportHasSerializedPrefs(file)) {
                 new androidx.appcompat.app.AlertDialog.Builder(requireContext())
                         .setTitle(R.string.import_settings)
+                        .setMessage(hasJsonPrefs ? null : requireContext()
+                                .getString(R.string.import_settings_vulnerable_format))
+                        .setOnDismissListener(dialog -> finishImport(importDataUri))
                         .setNegativeButton(R.string.cancel, (dialog, which) -> {
                             dialog.dismiss();
                             finishImport(importDataUri);
@@ -201,7 +211,16 @@ public class BackupRestoreSettingsFragment extends BasePreferenceFragment {
                             final Context context = requireContext();
                             final SharedPreferences prefs = PreferenceManager
                                     .getDefaultSharedPreferences(context);
-                            manager.loadSharedPreferences(prefs);
+                            try {
+                                if (hasJsonPrefs) {
+                                    manager.loadJsonPrefs(file, prefs);
+                                } else {
+                                    manager.loadSerializedPrefs(file, prefs);
+                                }
+                            } catch (IOException | ClassNotFoundException | JsonParserException e) {
+                                createErrorNotification(e, "Importing preferences");
+                                return;
+                            }
                             cleanImport(context, prefs);
                             finishImport(importDataUri);
                         })
@@ -210,7 +229,7 @@ public class BackupRestoreSettingsFragment extends BasePreferenceFragment {
                 finishImport(importDataUri);
             }
         } catch (final Exception e) {
-            ErrorUtil.showUiErrorSnackbar(this, "Importing database", e);
+            showErrorSnackbar(e, "Importing database and settings");
         }
     }
 
@@ -247,7 +266,7 @@ public class BackupRestoreSettingsFragment extends BasePreferenceFragment {
     }
 
     /**
-     * Save import path and restart system.
+     * Save import path and restart app.
      *
      * @param importDataUri The import path to save
      */
@@ -268,4 +287,15 @@ public class BackupRestoreSettingsFragment extends BasePreferenceFragment {
                 .putString(importExportDataPathKey, importExportDataUri.toString());
         editor.apply();
     }
+
+    private void showErrorSnackbar(final Throwable e, final String request) {
+        ErrorUtil.showSnackbar(this, new ErrorInfo(e, UserAction.DATABASE_IMPORT_EXPORT, request));
+    }
+
+    private void createErrorNotification(final Throwable e, final String request) {
+        ErrorUtil.createNotification(
+                requireContext(),
+                new ErrorInfo(e, UserAction.DATABASE_IMPORT_EXPORT, request)
+        );
+    }
 }

app/src/main/java/org/schabi/newpipe/settings/ContentSettingsManager.kt

@@ -1,120 +0,0 @@
-package org.schabi.newpipe.settings
-
-import android.content.SharedPreferences
-import android.util.Log
-import org.schabi.newpipe.MainActivity.DEBUG
-import org.schabi.newpipe.streams.io.SharpOutputStream
-import org.schabi.newpipe.streams.io.StoredFileHelper
-import org.schabi.newpipe.util.ZipHelper
-import java.io.IOException
-import java.io.ObjectInputStream
-import java.io.ObjectOutputStream
-import java.util.zip.ZipOutputStream
-
-class ContentSettingsManager(private val fileLocator: NewPipeFileLocator) {
-    companion object {
-        const val TAG = "ContentSetManager"
-    }
-
-    /**
-     * Exports given [SharedPreferences] to the file in given outputPath.
-     * It also creates the file.
-     */
-    @Throws(Exception::class)
-    fun exportDatabase(preferences: SharedPreferences, file: StoredFileHelper) {
-        file.create()
-        ZipOutputStream(SharpOutputStream(file.stream).buffered())
-            .use { outZip ->
-                ZipHelper.addFileToZip(outZip, fileLocator.db.path, "newpipe.db")
-
-                try {
-                    ObjectOutputStream(fileLocator.settings.outputStream()).use { output ->
-                        output.writeObject(preferences.all)
-                        output.flush()
-                    }
-                } catch (e: IOException) {
-                    if (DEBUG) {
-                        Log.e(TAG, "Unable to exportDatabase", e)
-                    }
-                }
-
-                ZipHelper.addFileToZip(outZip, fileLocator.settings.path, "newpipe.settings")
-            }
-    }
-
-    fun deleteSettingsFile() {
-        fileLocator.settings.delete()
-    }
-
-    /**
-     * Tries to create database directory if it does not exist.
-     *
-     * @return Whether the directory exists afterwards.
-     */
-    fun ensureDbDirectoryExists(): Boolean {
-        return fileLocator.dbDir.exists() || fileLocator.dbDir.mkdir()
-    }
-
-    fun extractDb(file: StoredFileHelper): Boolean {
-        val success = ZipHelper.extractFileFromZip(file, fileLocator.db.path, "newpipe.db")
-        if (success) {
-            fileLocator.dbJournal.delete()
-            fileLocator.dbWal.delete()
-            fileLocator.dbShm.delete()
-        }
-
-        return success
-    }
-
-    fun extractSettings(file: StoredFileHelper): Boolean {
-        return ZipHelper.extractFileFromZip(file, fileLocator.settings.path, "newpipe.settings")
-    }
-
-    /**
-     * Remove all shared preferences from the app and load the preferences supplied to the manager.
-     */
-    fun loadSharedPreferences(preferences: SharedPreferences) {
-        try {
-            val preferenceEditor = preferences.edit()
-
-            ObjectInputStream(fileLocator.settings.inputStream()).use { input ->
-                preferenceEditor.clear()
-                @Suppress("UNCHECKED_CAST")
-                val entries = input.readObject() as Map<String, *>
-                for ((key, value) in entries) {
-                    when (value) {
-                        is Boolean -> {
-                            preferenceEditor.putBoolean(key, value)
-                        }
-                        is Float -> {
-                            preferenceEditor.putFloat(key, value)
-                        }
-                        is Int -> {
-                            preferenceEditor.putInt(key, value)
-                        }
-                        is Long -> {
-                            preferenceEditor.putLong(key, value)
-                        }
-                        is String -> {
-                            preferenceEditor.putString(key, value)
-                        }
-                        is Set<*> -> {
-                            // There are currently only Sets with type String possible
-                            @Suppress("UNCHECKED_CAST")
-                            preferenceEditor.putStringSet(key, value as Set<String>?)
-                        }
-                    }
-                }
-                preferenceEditor.commit()
-            }
-        } catch (e: IOException) {
-            if (DEBUG) {
-                Log.e(TAG, "Unable to loadSharedPreferences", e)
-            }
-        } catch (e: ClassNotFoundException) {
-            if (DEBUG) {
-                Log.e(TAG, "Unable to loadSharedPreferences", e)
-            }
-        }
-    }
-}

app/src/main/java/org/schabi/newpipe/settings/NewPipeFileLocator.kt

@@ -1,21 +0,0 @@
-package org.schabi.newpipe.settings
-
-import java.io.File
-
-/**
- * Locates specific files of NewPipe based on the home directory of the app.
- */
-class NewPipeFileLocator(private val homeDir: File) {
-
-    val dbDir by lazy { File(homeDir, "/databases") }
-
-    val db by lazy { File(homeDir, "/databases/newpipe.db") }
-
-    val dbJournal by lazy { File(homeDir, "/databases/newpipe.db-journal") }
-
-    val dbShm by lazy { File(homeDir, "/databases/newpipe.db-shm") }
-
-    val dbWal by lazy { File(homeDir, "/databases/newpipe.db-wal") }
-
-    val settings by lazy { File(homeDir, "/databases/newpipe.settings") }
-}

app/src/main/java/org/schabi/newpipe/settings/export/BackupFileLocator.kt

@@ -0,0 +1,28 @@
+package org.schabi.newpipe.settings.export
+
+import java.io.File
+
+/**
+ * Locates specific files of NewPipe based on the home directory of the app.
+ */
+class BackupFileLocator(private val homeDir: File) {
+    companion object {
+        const val FILE_NAME_DB = "newpipe.db"
+        @Deprecated(
+            "Serializing preferences with Java's ObjectOutputStream is vulnerable to injections",
+            replaceWith = ReplaceWith("FILE_NAME_JSON_PREFS")
+        )
+        const val FILE_NAME_SERIALIZED_PREFS = "newpipe.settings"
+        const val FILE_NAME_JSON_PREFS = "preferences.json"
+    }
+
+    val dbDir by lazy { File(homeDir, "/databases") }
+
+    val db by lazy { File(dbDir, FILE_NAME_DB) }
+
+    val dbJournal by lazy { File(dbDir, "$FILE_NAME_DB-journal") }
+
+    val dbShm by lazy { File(dbDir, "$FILE_NAME_DB-shm") }
+
+    val dbWal by lazy { File(dbDir, "$FILE_NAME_DB-wal") }
+}

app/src/main/java/org/schabi/newpipe/settings/export/ImportExportManager.kt

@@ -0,0 +1,180 @@
+package org.schabi.newpipe.settings.export
+
+import android.content.SharedPreferences
+import com.grack.nanojson.JsonArray
+import com.grack.nanojson.JsonParser
+import com.grack.nanojson.JsonParserException
+import com.grack.nanojson.JsonWriter
+import org.schabi.newpipe.streams.io.SharpOutputStream
+import org.schabi.newpipe.streams.io.StoredFileHelper
+import org.schabi.newpipe.util.ZipHelper
+import java.io.FileNotFoundException
+import java.io.IOException
+import java.io.ObjectOutputStream
+import java.util.zip.ZipOutputStream
+
+class ImportExportManager(private val fileLocator: BackupFileLocator) {
+    companion object {
+        const val TAG = "ImportExportManager"
+    }
+
+    /**
+     * Exports given [SharedPreferences] to the file in given outputPath.
+     * It also creates the file.
+     */
+    @Throws(Exception::class)
+    fun exportDatabase(preferences: SharedPreferences, file: StoredFileHelper) {
+        file.create()
+        ZipOutputStream(SharpOutputStream(file.stream).buffered()).use { outZip ->
+            // add the database
+            ZipHelper.addFileToZip(
+                outZip,
+                BackupFileLocator.FILE_NAME_DB,
+                fileLocator.db.path,
+            )
+
+            // add the legacy vulnerable serialized preferences (will be removed in the future)
+            ZipHelper.addFileToZip(
+                outZip,
+                BackupFileLocator.FILE_NAME_SERIALIZED_PREFS
+            ) { byteOutput ->
+                ObjectOutputStream(byteOutput).use { output ->
+                    output.writeObject(preferences.all)
+                    output.flush()
+                }
+            }
+
+            // add the JSON preferences
+            ZipHelper.addFileToZip(
+                outZip,
+                BackupFileLocator.FILE_NAME_JSON_PREFS
+            ) { byteOutput ->
+                JsonWriter
+                    .indent("")
+                    .on(byteOutput)
+                    .`object`(preferences.all)
+                    .done()
+            }
+        }
+    }
+
+    /**
+     * Tries to create database directory if it does not exist.
+     *
+     * @return Whether the directory exists afterwards.
+     */
+    fun ensureDbDirectoryExists(): Boolean {
+        return fileLocator.dbDir.exists() || fileLocator.dbDir.mkdir()
+    }
+
+    /**
+     * Extracts the database from the given file to the app's database directory.
+     * The current app's database will be overwritten.
+     * @param file the .zip file to extract the database from
+     * @return true if the database was successfully extracted, false otherwise
+     */
+    fun extractDb(file: StoredFileHelper): Boolean {
+        val success = ZipHelper.extractFileFromZip(
+                file,
+                BackupFileLocator.FILE_NAME_DB,
+                fileLocator.db.path,
+        )
+
+        if (success) {
+            fileLocator.dbJournal.delete()
+            fileLocator.dbWal.delete()
+            fileLocator.dbShm.delete()
+        }
+
+        return success
+    }
+
+    @Deprecated(
+        "Serializing preferences with Java's ObjectOutputStream is vulnerable to injections",
+        replaceWith = ReplaceWith("exportHasJsonPrefs")
+    )
+    fun exportHasSerializedPrefs(zipFile: StoredFileHelper): Boolean {
+        return ZipHelper.zipContainsFile(zipFile, BackupFileLocator.FILE_NAME_SERIALIZED_PREFS)
+    }
+
+    fun exportHasJsonPrefs(zipFile: StoredFileHelper): Boolean {
+        return ZipHelper.zipContainsFile(zipFile, BackupFileLocator.FILE_NAME_JSON_PREFS)
+    }
+
+    /**
+     * Remove all shared preferences from the app and load the preferences supplied to the manager.
+     */
+    @Deprecated(
+        "Serializing preferences with Java's ObjectOutputStream is vulnerable to injections",
+        replaceWith = ReplaceWith("loadJsonPrefs")
+    )
+    @Throws(IOException::class, ClassNotFoundException::class)
+    fun loadSerializedPrefs(zipFile: StoredFileHelper, preferences: SharedPreferences) {
+        ZipHelper.extractFileFromZip(zipFile, BackupFileLocator.FILE_NAME_SERIALIZED_PREFS) {
+            PreferencesObjectInputStream(it).use { input ->
+                @Suppress("UNCHECKED_CAST")
+                val entries = input.readObject() as Map<String, *>
+
+                val editor = preferences.edit()
+                editor.clear()
+
+                for ((key, value) in entries) {
+                    when (value) {
+                        is Boolean -> editor.putBoolean(key, value)
+                        is Float -> editor.putFloat(key, value)
+                        is Int -> editor.putInt(key, value)
+                        is Long -> editor.putLong(key, value)
+                        is String -> editor.putString(key, value)
+                        is Set<*> -> {
+                            // There are currently only Sets with type String possible
+                            @Suppress("UNCHECKED_CAST")
+                            editor.putStringSet(key, value as Set<String>?)
+                        }
+                    }
+                }
+
+                if (!editor.commit()) {
+                    throw IOException("Unable to commit loadSerializedPrefs")
+                }
+            }
+        }.let { fileExists ->
+            if (!fileExists) {
+                throw FileNotFoundException(BackupFileLocator.FILE_NAME_SERIALIZED_PREFS)
+            }
+        }
+    }
+
+    /**
+     * Remove all shared preferences from the app and load the preferences supplied to the manager.
+     */
+    @Throws(IOException::class, JsonParserException::class)
+    fun loadJsonPrefs(zipFile: StoredFileHelper, preferences: SharedPreferences) {
+        ZipHelper.extractFileFromZip(zipFile, BackupFileLocator.FILE_NAME_JSON_PREFS) {
+            val jsonObject = JsonParser.`object`().from(it)
+
+            val editor = preferences.edit()
+            editor.clear()
+
+            for ((key, value) in jsonObject) {
+                when (value) {
+                    is Boolean -> editor.putBoolean(key, value)
+                    is Float -> editor.putFloat(key, value)
+                    is Int -> editor.putInt(key, value)
+                    is Long -> editor.putLong(key, value)
+                    is String -> editor.putString(key, value)
+                    is JsonArray -> {
+                        editor.putStringSet(key, value.mapNotNull { e -> e as? String }.toSet())
+                    }
+                }
+            }
+
+            if (!editor.commit()) {
+                throw IOException("Unable to commit loadJsonPrefs")
+            }
+        }.let { fileExists ->
+            if (!fileExists) {
+                throw FileNotFoundException(BackupFileLocator.FILE_NAME_JSON_PREFS)
+            }
+        }
+    }
+}

app/src/main/java/org/schabi/newpipe/settings/export/PreferencesObjectInputStream.java

@@ -0,0 +1,58 @@
+package org.schabi.newpipe.settings.export;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.ObjectInputStream;
+import java.io.ObjectStreamClass;
+import java.util.Set;
+
+/**
+ * An {@link ObjectInputStream} that only allows preferences-related types to be deserialized, to
+ * prevent injections. The only allowed types are: all primitive types, all boxed primitive types,
+ * null, strings. HashMap, HashSet and arrays of previously defined types are also allowed. Sources:
+ * <a href="https://wiki.sei.cmu.edu/confluence/display/java/SER00-J.+Enable+serialization+compatibility+during+class+evolution">
+ * cmu.edu
+ * </a>,
+ * <a href="https://cheatsheetseries.owasp.org/cheatsheets/Deserialization_Cheat_Sheet.html#harden-your-own-javaioobjectinputstream">
+ * OWASP cheatsheet
+ * </a>,
+ * <a href="https://commons.apache.org/proper/commons-io/apidocs/src-html/org/apache/commons/io/serialization/ValidatingObjectInputStream.html#line-118">
+ * Apache's {@code ValidatingObjectInputStream}
+ * </a>
+ */
+public class PreferencesObjectInputStream extends ObjectInputStream {
+
+    /**
+     * Primitive types, strings and other built-in types do not pass through resolveClass() but
+     * instead have a custom encoding; see
+     * <a href="https://docs.oracle.com/javase/6/docs/platform/serialization/spec/protocol.html#10152">
+     * official docs</a>.
+     */
+    private static final Set<String> CLASS_WHITELIST = Set.of(
+            "java.lang.Boolean",
+            "java.lang.Byte",
+            "java.lang.Character",
+            "java.lang.Short",
+            "java.lang.Integer",
+            "java.lang.Long",
+            "java.lang.Float",
+            "java.lang.Double",
+            "java.lang.Void",
+            "java.util.HashMap",
+            "java.util.HashSet"
+    );
+
+    public PreferencesObjectInputStream(final InputStream in) throws IOException {
+        super(in);
+    }
+
+    @Override
+    protected Class<?> resolveClass(final ObjectStreamClass desc)
+            throws ClassNotFoundException, IOException {
+        if (CLASS_WHITELIST.contains(desc.getName())) {
+            return super.resolveClass(desc);
+        } else {
+            throw new ClassNotFoundException("Class not allowed: " + desc.getName());
+        }
+    }
+}

app/src/main/java/org/schabi/newpipe/util/ZipHelper.java

@@ -1,18 +1,21 @@
 package org.schabi.newpipe.util;
 
 import org.schabi.newpipe.streams.io.SharpInputStream;
+import org.schabi.newpipe.streams.io.StoredFileHelper;
 
 import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipInputStream;
 import java.util.zip.ZipOutputStream;
 
-import org.schabi.newpipe.streams.io.StoredFileHelper;
-
 /**
  * Created by Christian Schabesberger on 28.01.18.
  * Copyright 2018 Christian Schabesberger <chris.schabesberger@mailbox.org>
@@ -34,73 +37,154 @@ import org.schabi.newpipe.streams.io.StoredFileHelper;
  */
 
 public final class ZipHelper {
-    private ZipHelper() { }
 
     private static final int BUFFER_SIZE = 2048;
 
+    @FunctionalInterface
+    public interface InputStreamConsumer {
+        void acceptStream(InputStream inputStream) throws IOException;
+    }
+
+    @FunctionalInterface
+    public interface OutputStreamConsumer {
+        void acceptStream(OutputStream outputStream) throws IOException;
+    }
+
+
+    private ZipHelper() { }
+
+
     /**
-     * This function helps to create zip files.
+     * This function helps to create zip files. Caution this will overwrite the original file.
-     * Caution this will override the original file.
      *
-     * @param outZip The ZipOutputStream where the data should be stored in
+     * @param outZip     the ZipOutputStream where the data should be stored in
-     * @param file   The path of the file that should be added to zip.
+     * @param nameInZip  the path of the file inside the zip
-     * @param name   The path of the file inside the zip.
+     * @param fileOnDisk the path of the file on the disk that should be added to zip
-     * @throws Exception
      */
-    public static void addFileToZip(final ZipOutputStream outZip, final String file,
+    public static void addFileToZip(final ZipOutputStream outZip,
-                                    final String name) throws Exception {
+                                    final String nameInZip,
+                                    final String fileOnDisk) throws IOException {
+        try (FileInputStream fi = new FileInputStream(fileOnDisk)) {
+            addFileToZip(outZip, nameInZip, fi);
+        }
+    }
+
+    /**
+     * This function helps to create zip files. Caution this will overwrite the original file.
+     *
+     * @param outZip         the ZipOutputStream where the data should be stored in
+     * @param nameInZip      the path of the file inside the zip
+     * @param streamConsumer will be called with an output stream that will go to the output file
+     */
+    public static void addFileToZip(final ZipOutputStream outZip,
+                                    final String nameInZip,
+                                    final OutputStreamConsumer streamConsumer) throws IOException {
+        final byte[] bytes;
+        try (ByteArrayOutputStream byteOutput = new ByteArrayOutputStream()) {
+            streamConsumer.acceptStream(byteOutput);
+            bytes = byteOutput.toByteArray();
+        }
+
+        try (ByteArrayInputStream byteInput = new ByteArrayInputStream(bytes)) {
+            ZipHelper.addFileToZip(outZip, nameInZip, byteInput);
+        }
+    }
+
+    /**
+     * This function helps to create zip files. Caution this will overwrite the original file.
+     *
+     * @param outZip      the ZipOutputStream where the data should be stored in
+     * @param nameInZip   the path of the file inside the zip
+     * @param inputStream the content to put inside the file
+     */
+    public static void addFileToZip(final ZipOutputStream outZip,
+                                    final String nameInZip,
+                                    final InputStream inputStream) throws IOException {
         final byte[] data = new byte[BUFFER_SIZE];
-        try (FileInputStream fi = new FileInputStream(file);
+        try (BufferedInputStream bufferedInputStream =
-             BufferedInputStream inputStream = new BufferedInputStream(fi, BUFFER_SIZE)) {
+                     new BufferedInputStream(inputStream, BUFFER_SIZE)) {
-            final ZipEntry entry = new ZipEntry(name);
+            final ZipEntry entry = new ZipEntry(nameInZip);
             outZip.putNextEntry(entry);
             int count;
-            while ((count = inputStream.read(data, 0, BUFFER_SIZE)) != -1) {
+            while ((count = bufferedInputStream.read(data, 0, BUFFER_SIZE)) != -1) {
                 outZip.write(data, 0, count);
             }
         }
     }
 
     /**
-     * This will extract data from ZipInputStream.
+     * This will extract data from ZipInputStream. Caution this will overwrite the original file.
-     * Caution this will override the original file.
      *
-     * @param zipFile The zip file
+     * @param zipFile    the zip file to extract from
-     * @param file The path of the file on the disk where the data should be extracted to.
+     * @param nameInZip  the path of the file inside the zip
-     * @param name The path of the file inside the zip.
+     * @param fileOnDisk the path of the file on the disk where the data should be extracted to
      * @return will return true if the file was found within the zip file
-     * @throws Exception
      */
-    public static boolean extractFileFromZip(final StoredFileHelper zipFile, final String file,
+    public static boolean extractFileFromZip(final StoredFileHelper zipFile,
-                                             final String name) throws Exception {
+                                             final String nameInZip,
-        try (ZipInputStream inZip = new ZipInputStream(new BufferedInputStream(
+                                             final String fileOnDisk) throws IOException {
-                new SharpInputStream(zipFile.getStream())))) {
+        return extractFileFromZip(zipFile, nameInZip, input -> {
-            final byte[] data = new byte[BUFFER_SIZE];
-            boolean found = false;
-            ZipEntry ze;
-
-            while ((ze = inZip.getNextEntry()) != null) {
-                if (ze.getName().equals(name)) {
-                    found = true;
             // delete old file first
-                    final File oldFile = new File(file);
+            final File oldFile = new File(fileOnDisk);
             if (oldFile.exists()) {
                 if (!oldFile.delete()) {
-                            throw new Exception("Could not delete " + file);
+                    throw new IOException("Could not delete " + fileOnDisk);
                 }
             }
 
-                    try (FileOutputStream outFile = new FileOutputStream(file)) {
+            final byte[] data = new byte[BUFFER_SIZE];
-                        int count = 0;
+            try (FileOutputStream outFile = new FileOutputStream(fileOnDisk)) {
-                        while ((count = inZip.read(data)) != -1) {
+                int count;
+                while ((count = input.read(data)) != -1) {
                     outFile.write(data, 0, count);
                 }
             }
+        });
+    }
 
-                    inZip.closeEntry();
+    /**
+     * This will extract data from ZipInputStream.
+     *
+     * @param zipFile        the zip file to extract from
+     * @param nameInZip      the path of the file inside the zip
+     * @param streamConsumer will be called with the input stream from the file inside the zip
+     * @return will return true if the file was found within the zip file
+     */
+    public static boolean extractFileFromZip(final StoredFileHelper zipFile,
+                                             final String nameInZip,
+                                             final InputStreamConsumer streamConsumer)
+            throws IOException {
+        try (ZipInputStream inZip = new ZipInputStream(new BufferedInputStream(
+                new SharpInputStream(zipFile.getStream())))) {
+            ZipEntry ze;
+            while ((ze = inZip.getNextEntry()) != null) {
+                if (ze.getName().equals(nameInZip)) {
+                    streamConsumer.acceptStream(inZip);
+                    return true;
                 }
             }
-            return found;
+
+            return false;
+        }
+    }
+
+    /**
+     * @param zipFile   the zip file
+     * @param fileInZip the filename to check
+     * @return whether the provided filename is in the zip; only the first level is checked
+     */
+    public static boolean zipContainsFile(final StoredFileHelper zipFile, final String fileInZip)
+            throws Exception {
+        try (ZipInputStream inZip = new ZipInputStream(new BufferedInputStream(
+                new SharpInputStream(zipFile.getStream())))) {
+            ZipEntry ze;
+
+            while ((ze = inZip.getNextEntry()) != null) {
+                if (ze.getName().equals(fileInZip)) {
+                    return true;
+                }
+            }
+            return false;
         }
     }
 

app/src/main/res/values/strings.xml

@@ -856,4 +856,5 @@
     </plurals>
     <string name="show_more">Show more</string>
     <string name="show_less">Show less</string>
+    <string name="import_settings_vulnerable_format">The settings in the export being imported use a vulnerable format that was deprecated since NewPipe 0.27.0. Make sure the export being imported is from a trusted source, and prefer using only exports obtained from NewPipe 0.27.0 or newer in the future. Support for importing settings in this vulnerable format will soon be removed completely, and then old versions of NewPipe will not be able to import settings of exports from new versions anymore.</string>
 </resources>

app/src/test/java/org/schabi/newpipe/settings/ImportAllCombinationsTest.kt

@@ -0,0 +1,184 @@
+package org.schabi.newpipe.settings
+
+import android.content.SharedPreferences
+import org.junit.Assert
+import org.junit.Test
+import org.mockito.Mockito
+import org.schabi.newpipe.settings.export.BackupFileLocator
+import org.schabi.newpipe.settings.export.ImportExportManager
+import org.schabi.newpipe.streams.io.StoredFileHelper
+import us.shandian.giga.io.FileStream
+import java.io.File
+import java.io.IOException
+import java.nio.file.Files
+
+class ImportAllCombinationsTest {
+
+    companion object {
+        private val classloader = ImportExportManager::class.java.classLoader!!
+    }
+
+    private enum class Ser(val id: String) {
+        YES("ser"),
+        VULNERABLE("vulnser"),
+        NO("noser");
+    }
+
+    private data class FailData(
+        val containsDb: Boolean,
+        val containsSer: Ser,
+        val containsJson: Boolean,
+        val filename: String,
+        val throwable: Throwable,
+    )
+
+    private fun testZipCombination(
+        containsDb: Boolean,
+        containsSer: Ser,
+        containsJson: Boolean,
+        filename: String,
+        runTest: (test: () -> Unit) -> Unit,
+    ) {
+        val zipFile = File(classloader.getResource(filename)?.file!!)
+        val zip = Mockito.mock(StoredFileHelper::class.java, Mockito.withSettings().stubOnly())
+        Mockito.`when`(zip.stream).then { FileStream(zipFile) }
+
+        val fileLocator = Mockito.mock(
+            BackupFileLocator::class.java,
+            Mockito.withSettings().stubOnly()
+        )
+        val db = File.createTempFile("newpipe_", "")
+        val dbJournal = File.createTempFile("newpipe_", "")
+        val dbWal = File.createTempFile("newpipe_", "")
+        val dbShm = File.createTempFile("newpipe_", "")
+        Mockito.`when`(fileLocator.db).thenReturn(db)
+        Mockito.`when`(fileLocator.dbJournal).thenReturn(dbJournal)
+        Mockito.`when`(fileLocator.dbShm).thenReturn(dbShm)
+        Mockito.`when`(fileLocator.dbWal).thenReturn(dbWal)
+
+        if (containsDb) {
+            runTest {
+                Assert.assertTrue(ImportExportManager(fileLocator).extractDb(zip))
+                Assert.assertFalse(dbJournal.exists())
+                Assert.assertFalse(dbWal.exists())
+                Assert.assertFalse(dbShm.exists())
+                Assert.assertTrue("database file size is zero", Files.size(db.toPath()) > 0)
+            }
+        } else {
+            runTest {
+                Assert.assertFalse(ImportExportManager(fileLocator).extractDb(zip))
+                Assert.assertTrue(dbJournal.exists())
+                Assert.assertTrue(dbWal.exists())
+                Assert.assertTrue(dbShm.exists())
+                Assert.assertEquals(0, Files.size(db.toPath()))
+            }
+        }
+
+        val preferences = Mockito.mock(SharedPreferences::class.java, Mockito.withSettings().stubOnly())
+        var editor = Mockito.mock(SharedPreferences.Editor::class.java)
+        Mockito.`when`(preferences.edit()).thenReturn(editor)
+        Mockito.`when`(editor.commit()).thenReturn(true)
+
+        when (containsSer) {
+            Ser.YES -> runTest {
+                Assert.assertTrue(ImportExportManager(fileLocator).exportHasSerializedPrefs(zip))
+                ImportExportManager(fileLocator).loadSerializedPrefs(zip, preferences)
+
+                Mockito.verify(editor, Mockito.times(1)).clear()
+                Mockito.verify(editor, Mockito.times(1)).commit()
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putBoolean(Mockito.anyString(), Mockito.anyBoolean())
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putString(Mockito.anyString(), Mockito.anyString())
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putInt(Mockito.anyString(), Mockito.anyInt())
+            }
+            Ser.VULNERABLE -> runTest {
+                Assert.assertTrue(ImportExportManager(fileLocator).exportHasSerializedPrefs(zip))
+                Assert.assertThrows(ClassNotFoundException::class.java) {
+                    ImportExportManager(fileLocator).loadSerializedPrefs(zip, preferences)
+                }
+
+                Mockito.verify(editor, Mockito.never()).clear()
+                Mockito.verify(editor, Mockito.never()).commit()
+            }
+            Ser.NO -> runTest {
+                Assert.assertFalse(ImportExportManager(fileLocator).exportHasSerializedPrefs(zip))
+                Assert.assertThrows(IOException::class.java) {
+                    ImportExportManager(fileLocator).loadSerializedPrefs(zip, preferences)
+                }
+
+                Mockito.verify(editor, Mockito.never()).clear()
+                Mockito.verify(editor, Mockito.never()).commit()
+            }
+        }
+
+        // recreate editor mock so verify() behaves correctly
+        editor = Mockito.mock(SharedPreferences.Editor::class.java)
+        Mockito.`when`(preferences.edit()).thenReturn(editor)
+        Mockito.`when`(editor.commit()).thenReturn(true)
+
+        if (containsJson) {
+            runTest {
+                Assert.assertTrue(ImportExportManager(fileLocator).exportHasJsonPrefs(zip))
+                ImportExportManager(fileLocator).loadJsonPrefs(zip, preferences)
+
+                Mockito.verify(editor, Mockito.times(1)).clear()
+                Mockito.verify(editor, Mockito.times(1)).commit()
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putBoolean(Mockito.anyString(), Mockito.anyBoolean())
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putString(Mockito.anyString(), Mockito.anyString())
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putInt(Mockito.anyString(), Mockito.anyInt())
+            }
+        } else {
+            runTest {
+                Assert.assertFalse(ImportExportManager(fileLocator).exportHasJsonPrefs(zip))
+                Assert.assertThrows(IOException::class.java) {
+                    ImportExportManager(fileLocator).loadJsonPrefs(zip, preferences)
+                }
+
+                Mockito.verify(editor, Mockito.never()).clear()
+                Mockito.verify(editor, Mockito.never()).commit()
+            }
+        }
+    }
+
+    @Test
+    fun `Importing all possible combinations of zip files`() {
+        val failedAssertions = mutableListOf<FailData>()
+        for (containsDb in listOf(true, false)) {
+            for (containsSer in Ser.entries) {
+                for (containsJson in listOf(true, false)) {
+                    val filename = "settings/${if (containsDb) "db" else "nodb"}_${
+                    containsSer.id}_${if (containsJson) "json" else "nojson"}.zip"
+                    testZipCombination(containsDb, containsSer, containsJson, filename) { test ->
+                        try {
+                            test()
+                        } catch (e: Throwable) {
+                            failedAssertions.add(
+                                FailData(
+                                    containsDb, containsSer, containsJson,
+                                    filename, e
+                                )
+                            )
+                        }
+                    }
+                }
+            }
+        }
+
+        if (failedAssertions.isNotEmpty()) {
+            for (a in failedAssertions) {
+                println(
+                    "Assertion failed with containsDb=${a.containsDb}, containsSer=${
+                    a.containsSer}, containsJson=${a.containsJson}, filename=${a.filename}:"
+                )
+                a.throwable.printStackTrace()
+                println()
+            }
+            Assert.fail("${failedAssertions.size} assertions failed")
+        }
+    }
+}

app/src/test/java/org/schabi/newpipe/settings/ImportExportManagerTest.kt

@@ -1,8 +1,10 @@
 package org.schabi.newpipe.settings
 
 import android.content.SharedPreferences
+import com.grack.nanojson.JsonParser
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
+import org.junit.Assert.assertThrows
 import org.junit.Assert.assertTrue
 import org.junit.Assume
 import org.junit.Before
@@ -17,6 +19,8 @@ import org.mockito.Mockito.verify
 import org.mockito.Mockito.`when`
 import org.mockito.Mockito.withSettings
 import org.mockito.junit.MockitoJUnitRunner
+import org.schabi.newpipe.settings.export.BackupFileLocator
+import org.schabi.newpipe.settings.export.ImportExportManager
 import org.schabi.newpipe.streams.io.StoredFileHelper
 import us.shandian.giga.io.FileStream
 import java.io.File
@@ -25,27 +29,25 @@ import java.nio.file.Files
 import java.util.zip.ZipFile
 
 @RunWith(MockitoJUnitRunner::class)
-class ContentSettingsManagerTest {
+class ImportExportManagerTest {
 
     companion object {
-        private val classloader = ContentSettingsManager::class.java.classLoader!!
+        private val classloader = ImportExportManager::class.java.classLoader!!
     }
 
-    private lateinit var fileLocator: NewPipeFileLocator
+    private lateinit var fileLocator: BackupFileLocator
     private lateinit var storedFileHelper: StoredFileHelper
 
     @Before
     fun setupFileLocator() {
-        fileLocator = Mockito.mock(NewPipeFileLocator::class.java, withSettings().stubOnly())
+        fileLocator = Mockito.mock(BackupFileLocator::class.java, withSettings().stubOnly())
         storedFileHelper = Mockito.mock(StoredFileHelper::class.java, withSettings().stubOnly())
     }
 
     @Test
     fun `The settings must be exported successfully in the correct format`() {
         val db = File(classloader.getResource("settings/newpipe.db")!!.file)
-        val newpipeSettings = File.createTempFile("newpipe_", "")
         `when`(fileLocator.db).thenReturn(db)
-        `when`(fileLocator.settings).thenReturn(newpipeSettings)
 
         val expectedPreferences = mapOf("such pref" to "much wow")
         val sharedPreferences =
@@ -54,11 +56,11 @@ class ContentSettingsManagerTest {
 
         val output = File.createTempFile("newpipe_", "")
         `when`(storedFileHelper.stream).thenReturn(FileStream(output))
-        ContentSettingsManager(fileLocator).exportDatabase(sharedPreferences, storedFileHelper)
+        ImportExportManager(fileLocator).exportDatabase(sharedPreferences, storedFileHelper)
 
         val zipFile = ZipFile(output)
         val entries = zipFile.entries().toList()
-        assertEquals(2, entries.size)
+        assertEquals(3, entries.size)
 
         zipFile.getInputStream(entries.first { it.name == "newpipe.db" }).use { actual ->
             db.inputStream().use { expected ->
@@ -70,26 +72,11 @@ class ContentSettingsManagerTest {
             val actualPreferences = ObjectInputStream(actual).readObject()
             assertEquals(expectedPreferences, actualPreferences)
         }
+
+        zipFile.getInputStream(entries.first { it.name == "preferences.json" }).use { actual ->
+            val actualPreferences = JsonParser.`object`().from(actual)
+            assertEquals(expectedPreferences, actualPreferences)
         }
-
-    @Test
-    fun `Settings file must be deleted`() {
-        val settings = File.createTempFile("newpipe_", "")
-        `when`(fileLocator.settings).thenReturn(settings)
-
-        ContentSettingsManager(fileLocator).deleteSettingsFile()
-
-        assertFalse(settings.exists())
-    }
-
-    @Test
-    fun `Deleting settings file must do nothing if none exist`() {
-        val settings = File("non_existent")
-        `when`(fileLocator.settings).thenReturn(settings)
-
-        ContentSettingsManager(fileLocator).deleteSettingsFile()
-
-        assertFalse(settings.exists())
     }
 
     @Test
@@ -98,7 +85,7 @@ class ContentSettingsManagerTest {
         Assume.assumeTrue(dir.delete())
         `when`(fileLocator.dbDir).thenReturn(dir)
 
-        ContentSettingsManager(fileLocator).ensureDbDirectoryExists()
+        ImportExportManager(fileLocator).ensureDbDirectoryExists()
         assertTrue(dir.exists())
     }
 
@@ -107,7 +94,7 @@ class ContentSettingsManagerTest {
         val dir = Files.createTempDirectory("newpipe_").toFile()
         `when`(fileLocator.dbDir).thenReturn(dir)
 
-        ContentSettingsManager(fileLocator).ensureDbDirectoryExists()
+        ImportExportManager(fileLocator).ensureDbDirectoryExists()
         assertTrue(dir.exists())
     }
 
@@ -122,9 +109,9 @@ class ContentSettingsManagerTest {
         `when`(fileLocator.dbShm).thenReturn(dbShm)
         `when`(fileLocator.dbWal).thenReturn(dbWal)
 
-        val zip = File(classloader.getResource("settings/newpipe.zip")?.file!!)
+        val zip = File(classloader.getResource("settings/db_ser_json.zip")?.file!!)
         `when`(storedFileHelper.stream).thenReturn(FileStream(zip))
-        val success = ContentSettingsManager(fileLocator).extractDb(storedFileHelper)
+        val success = ImportExportManager(fileLocator).extractDb(storedFileHelper)
 
         assertTrue(success)
         assertFalse(dbJournal.exists())
@@ -141,9 +128,9 @@ class ContentSettingsManagerTest {
         val dbShm = File.createTempFile("newpipe_", "")
         `when`(fileLocator.db).thenReturn(db)
 
-        val emptyZip = File(classloader.getResource("settings/empty.zip")?.file!!)
+        val emptyZip = File(classloader.getResource("settings/nodb_noser_nojson.zip")?.file!!)
         `when`(storedFileHelper.stream).thenReturn(FileStream(emptyZip))
-        val success = ContentSettingsManager(fileLocator).extractDb(storedFileHelper)
+        val success = ImportExportManager(fileLocator).extractDb(storedFileHelper)
 
         assertFalse(success)
         assertTrue(dbJournal.exists())
@@ -154,41 +141,44 @@ class ContentSettingsManagerTest {
 
     @Test
     fun `Contains setting must return true if a settings file exists in the zip`() {
-        val settings = File.createTempFile("newpipe_", "")
+        val zip = File(classloader.getResource("settings/db_ser_json.zip")?.file!!)
-        `when`(fileLocator.settings).thenReturn(settings)
-
-        val zip = File(classloader.getResource("settings/newpipe.zip")?.file!!)
         `when`(storedFileHelper.stream).thenReturn(FileStream(zip))
-        val contains = ContentSettingsManager(fileLocator).extractSettings(storedFileHelper)
+        assertTrue(ImportExportManager(fileLocator).exportHasSerializedPrefs(storedFileHelper))
-
-        assertTrue(contains)
     }
 
     @Test
-    fun `Contains setting must return false if a no settings file exists in the zip`() {
+    fun `Contains setting must return false if no settings file exists in the zip`() {
-        val settings = File.createTempFile("newpipe_", "")
+        val emptyZip = File(classloader.getResource("settings/nodb_noser_nojson.zip")?.file!!)
-        `when`(fileLocator.settings).thenReturn(settings)
-
-        val emptyZip = File(classloader.getResource("settings/empty.zip")?.file!!)
         `when`(storedFileHelper.stream).thenReturn(FileStream(emptyZip))
-        val contains = ContentSettingsManager(fileLocator).extractSettings(storedFileHelper)
+        assertFalse(ImportExportManager(fileLocator).exportHasSerializedPrefs(storedFileHelper))
-
-        assertFalse(contains)
     }
 
     @Test
     fun `Preferences must be set from the settings file`() {
-        val settings = File(classloader.getResource("settings/newpipe.settings")!!.path)
+        val zip = File(classloader.getResource("settings/db_ser_json.zip")?.file!!)
-        `when`(fileLocator.settings).thenReturn(settings)
+        `when`(storedFileHelper.stream).thenReturn(FileStream(zip))
 
         val preferences = Mockito.mock(SharedPreferences::class.java, withSettings().stubOnly())
         val editor = Mockito.mock(SharedPreferences.Editor::class.java)
         `when`(preferences.edit()).thenReturn(editor)
+        `when`(editor.commit()).thenReturn(true)
 
-        ContentSettingsManager(fileLocator).loadSharedPreferences(preferences)
+        ImportExportManager(fileLocator).loadSerializedPrefs(storedFileHelper, preferences)
 
         verify(editor, atLeastOnce()).putBoolean(anyString(), anyBoolean())
         verify(editor, atLeastOnce()).putString(anyString(), anyString())
         verify(editor, atLeastOnce()).putInt(anyString(), anyInt())
     }
+
+    @Test
+    fun `Importing preferences with a serialization injected class should fail`() {
+        val emptyZip = File(classloader.getResource("settings/db_vulnser_json.zip")?.file!!)
+        `when`(storedFileHelper.stream).thenReturn(FileStream(emptyZip))
+
+        val preferences = Mockito.mock(SharedPreferences::class.java, withSettings().stubOnly())
+
+        assertThrows(ClassNotFoundException::class.java) {
+            ImportExportManager(fileLocator).loadSerializedPrefs(storedFileHelper, preferences)
+        }
+    }
 }

app/src/test/resources/settings/README.md

@@ -0,0 +1,4 @@
+`*.zip` files in this folder are NewPipe database exports, in all possible configurations:
+- `db` / `nodb` indicates if there is a `newpipe.db` database included or not
+- `ser` / `vulnser` / `noser` indicates if there is a `newpipe.settings` Java-serialized preferences file included, if it is included and contains an injection attack, of if it is not included
+- `json` / `nojson` indicates if there is a `preferences.json` JSON preferences file included or not

checkstyle/checkstyle.xml

@@ -39,11 +39,13 @@
     <module name="Translation"/>
 
     <!-- Checks for Size Violations.                    -->
-    <!-- See https://checkstyle.org/config_sizes.html -->
+    <!-- See https://checkstyle.sourceforge.io/checks/sizes/index.html -->
     <module name="FileLength"/>
     <module name="LineLength">
       <property name="max" value="100"/>
       <property name="fileExtensions" value="java"/>
+      <!-- Also allow links in javadocs to be longer (the default would just cover imports) -->
+      <property name="ignorePattern" value="^((package|import) .*)|( *\* &lt;a href ?\= ?&quot;.*&quot;&gt;)$"/>
     </module>
 
     <!-- Checks for whitespace                               -->