Harden config import against malicious payloads - #3177
Open
ethicnology wants to merge 4 commits into
Open
Conversation
Owner
|
Thanks for the PR. Not sure a whitelist is a good idea since we have to remember to maintain it and keep it in sync with new items. Also items not in the whitelist are dropped quietly so that would add to the confusion if the list is not maintained. Instead of this approach, maybe we should have a detailed confirmation dialog that lists out all settings that are about to be imported, with a checkbox for the user to manually opt out? Maybe high risk items like |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Importing an Obtainium backup (file) or config link (
obtainium://app/…)applied the payload with almost no validation:
settingsobjectwas written verbatim to SharedPreferences — including
externalInstallerPackage/externalInstallerComponent(which wouldredirect all future installs to an attacker-chosen app) and
-credskeys (overwriting stored tokens).
idmatches an existing onereplaced it — including its source URL — with no notice. A shared
"add this app" link could quietly repoint a trusted app to a malicious
source, making the next "update" serve attacker content.
versionExtractionRegEx,apkFilterRegEx,customLinkFilterRegex) from an imported config runon every update check; a catastrophic-backtracking pattern hangs them.
What this PR changes
Suggested reading order — each commit stands alone:
fix: only import allowlisted settings keys from backups— importedsettings are applied only if they are in an explicit allowlist of
user-facing preferences. Credentials, the external installer identity,
device-specific state (
exportDir) and onboarding flags are ignored(and logged).
fix: tolerate map-shaped additionalSettings in app import—robustness:
App.fromJsonand the JSON migrations now acceptadditionalSettingsboth as a JSON-encoded string (export format) andas an already-decoded map (hand-crafted or third-party payloads),
instead of failing the whole import.
feat: warn about overwrites and insecure settings before importing—both import paths (file picker and
obtainium://deep links) now show ablocking warning dialog when the payload would overwrite an existing app
(showing the URL change), uses a cleartext
http://URL, enablesallowInsecure, or contains a potentially unsafe regex. The user cancancel or proceed.
fix: strip potentially unsafe regexes from imported configs—regex values that fail a best-effort safety check (compilation + timing
against short adversarial probes) are removed from imported apps and
logged, so a malicious config can't hang update checks.
What it deliberately does NOT change
importable, and clean configs produce no warnings.
manually-added apps. New UI strings are
en.jsononly (other locales viathe usual translation process).