Closed Bug 1689598 Opened 4 years ago Closed 4 years ago

<A ref> link to c:\:$i30:$bitmap triggers a known and unfixed NTFS vulnerability (corrupting MFT table)

Categories

(Core :: Networking: File, defect)

Product:
Core
Component:
Networking: File
Version:
Firefox 85
Platform:
Desktop
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
87 Branch
Tracking Flags:
Tracking Status
relnote-firefox --- 85+
firefox-esr78 85+ verified
firefox84 --- wontfix
firefox85 + verified
firefox86 + verified
firefox87 + verified

People

(Reporter: yzhuang502, Assigned: Gijs)

References

Details

(Keywords: sec-moderate)

Attachments

(9 files)

Firefix.JPG
28.35 KB, image/jpeg
Details
firefox1.JPG
169.22 KB, image/jpeg
Details
firefox2.JPG
28.93 KB, image/jpeg
Details
firefox3.JPG
111.45 KB, image/jpeg
Details
firefox4.JPG
59.29 KB, image/jpeg
Details
procmon-log.zip
5.66 MB, application/x-zip-compressed
Details
1.html
170 bytes, text/html
Details
Bug 1689598
48 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
jcristau
: approval-mozilla-release+
dveditz
: sec-approval+
Details | Review
Patch for esr78
8.84 KB, patch
jcristau
: approval-mozilla-esr78+
Details | Diff | Splinter Review
Attached image Firefix.JPG —

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

  1. Fresh installion of Windows 10 Enterprise Edition Version 2004
  2. Fresh install Firefox 85 x86_64 (Build ID 20210118153634)
  3. Create below html file in a web server
      <html>
              <body>
                      Bug
                      <a href="C:\:$i30:$bitmap">WARNING: Don't click or else corrupt your harddisk</a>
              </body>
      </html>
  1. In Firefox, browse the above html via http URI (eg http://192.168.1.1/test.html)
  2. Click the link in html
  3. Reboot Windows 10

Actual results:

Reboot will trigger a NTFS check unexpected due to known and unifxed NTFS bug as at 29 Jan 2021
File system corruption or BSOD may happen too.

ref https://www.bleepingcomputer.com/news/security/windows-10-bug-corrupts-your-hard-drive-on-seeing-this-files-icon/

Expected results:

Access to local file resources (eg <a href="C::$i30:$bitmap">) is not expected from remote http or https request.

No issue from Chrome 88.0.4324.104 and Microsoft Chromium Edge 88.0.705.56

Similar issue was reported in Bug 1368682

Clicking the link does nothing, and an error appears in the console ("Prevented navigation to "c::$i30:$bitmap" due to an unknown protocol"), which is what I'd expect. (Tested on a Windows 7 VM; the comments in the article claim this is as an issue with NTFS so I would expect all Windows versions to have this issue, and I don't have a Windows 10 VM handy)
Even after a manual reboot I do not see a checkdsk screen or anything like that.

In your testing, if you replace the link with a "benign" file path, does clicking the link work (ie load a directory listing)? Because it does not for me (and it's not supposed to; web content should not be allowed to link to files), and if it does not, I don't see how we would be triggering this bug. :-\

Flags: needinfo?(yzhuang502)

The comment in the article says this

"The researcher told BleepingComputer that the flaw became exploitable starting around Windows 10 build 1803, the Windows 10 April 2018 Update, and continues to work in the latest version."

Hence, the issue should be affecting around Win 10 build 1803 or later, my reported issue will not be reproduced in Win 7.

I have just tested on Win 7 SP1 32 bit VM and I can confirm no issue in Win 7, same as your test result.

However, please try to test in a Win 10 VM preferably the latest build version.
I was testing on a Windows 10 version 2004, which issue can be reproduced.

In Win 10 test, I got the same "Prevented navigation to "c::$i30:$bitmap" due to an unknown protocol" console output.
But still after manual reboot, the chkdsk thing occurs during reboot process.

I was expected web content not allow to link to files too.

Latest Chrome and Chromium Edge do not have the same issue with the html file when I performed same test in Win10.

So, please try a test in a Win 10 VM

Flags: needinfo?(yzhuang502)

I'm not in a position to quickly obtain a test setup for this. I'm checking if we have other people who are.

Is there any way you can obtain more information here, e.g. using FileMon/ProcMon or similar to get a stack for the file access?

Because in general, this doesn't make a lot of sense - web content processes (which would be what loads the HTML file and attempts to deal with the link click) are sandboxed and should not be able to access arbitrary local files, and on top of that, the security error indicates we didn't even get to the point of considering it as a file URI (ie we still thought of "c:" as a protocol, which wouldn't result in file lookups), so it is a little difficult to see how this could even be happening.

Flags: needinfo?(yzhuang502)
Attached image firefox1.JPG —

Procmon showing Firefox is trying to create C::$i30:$bitmap.

Flags: needinfo?(yzhuang502)
Attached image firefox2.JPG —

More details

Attached image firefox3.JPG —

More detail

Attached image firefox4.JPG —

To view the event, it is required to un-filter $bitmap events in procmon

Attached file procmon-log.zip —

Procmon capture. The event is collected at 8:36:32.8892686 AM

Are you sure that's the testcase you used--href="C::$i30:$bitmap"--just a raw Windows path for the link? Everywhere else I've seen this (for example, the article from 2 weeks ago you linked to) has a file:/// url which makes a lot more sense. The windows path ought to get you an unknown protocol prompt. And a file: url shouldn't "work" when it's loaded from a remote server as you specified (but could still trigger this bug if loaded from a local file).

Maybe our file: handler should bail out for any path that starts with '$' after the drive letter. Firefox is a web browser, not a file browser, so if we block all access to windows metafiles it's not really a problem. Some of those files can't be accessed without special tools anyway. An approach like that would fix bug 1368682 at the same time.

Group: firefox-core-security → network-core-security
Component: Untriaged → Networking: File
Product: Firefox → Core
See Also: → 1368682

At least one issue that seems to have thrown us off here is that the path in comment #0 as provided by the reporter is actually: C:\:$i30:$bitmap, but markdown ate the backslash.

The procmon logs are helpful; I can now reproduce the same thing on Windows 7 (without any corruption), but the stacks both from the attached log and when I reproduce are useless, and don't go beyond ntoskrnl - they don't reference any Firefox code.

Attached file 1.html —

This is the raw html file I was using.

To trigger the issue.
I put the html file into nginx web server, then access it in Firefox via http://<ip address>/1.html

Many thanks to mhowell for helping out with the win10 testing, we managed to reproduce in the end.

On my win7 VM, I can also reproduce the CreateFile call happening with C:\arbitrarypath -- which is a bit less destructive and will hopefully help diagnose/fix this. ;-)

This access happens in the child process for the tab with the link. I am not able to reproduce similar access if I change the link to point to file:///C:\arbitrarypath, which is... odd. Still no useful stacks out of procmon. I will keep looking for a bit. I'm surprised this access is allowed from the sandbox at all, tbh... but then again, the equivalent of a stat with nefarious side effects is not very common, I guess...

Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

So the rabbithole is peculiar here.

When the link click happens, we do security checks on the link (from both some JS code and later from C++ in/around nsDocShell's OnLinkClickSync.

The security checks in some cases get a plain string, and have to evaluate if loading this plain string, when parsed into a URL, should be allowed from the page that contained the string. Those checks not only check what happens with the literal string we have, but also what happens if we were to "fix" the protocol of the string (which normally happens for address bar loads, but also in some other cases). This fixes some scheme typos, makes sure that just "example.com" (without the http protocol) works, etc. It can also fix C:\foo to file:///C:/foo.

These checks are useful because they mean that we still end up treating C:\foo the same as file:///C:/foo, which is how it would get treated in the URL bar if dragged there, etc.

Unfortunately, in the process of doing this, we run this code:

      // Test if this is a valid path by trying to create a local file
      // object. The URL of that is returned if successful.
      let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
      file.initWithPath(uriString);
      return Services.io.newFileURI(file);

This looks innocent enough, but newFileURI needs to know if the file for which we're creating a URI is a file or a directory, in order to determine whether to suffix a /, which in turn gets used to resolve relative paths correctly (so is important in some settings). Of course, we don't care about any of that in this particular case, but the utility function in question has no idea about that - once we're in the newFileURI implementation we're like 4 levels deep away from the security check. We don't want to duplicate the logic because then we risk diverging in how the check and the execution behave and then we'll have other issues.

To know if something is a directory, we have to ask the nsIFile instance, are you a directory? To know that, it does a CreateFile Windows API call that looks up this information. Which is how we end up accessing this magic path.

To fix this, I have a two-fold patch:

  1. in the URI fixup code, use slightly different utility methods to create the file URL, and just assume that the path we get is for a file, rather than that it could be for a directory. If we're using fixup, this shouldn't matter - once we do the actual load, if that happens, we'll get the "right" URL internally anyway. This is also a slight perf improvement - repetitive stat calls aren't useful.
  2. make nsLocalFile refuse to initWithPath if the path ends with the names of any of the NTFS metadata files (see https://en.wikipedia.org/wiki/NTFS#Metafiles ).

Julien: I think we might want to take this into a dot-release for 85. I defer to :dveditz as to whether this is enough of a dot-release driver on its own. Microsoft has publicly stated that all that the bug does is set a corruption flag, not actually corrupt things - but others have reported that they have bricked their machine with this stuff. MS have also stated they're working on a fix.

We're patching separately because it is unexpected that a link click on a website on its own would trigger this (short of downloading a file and/or opening a zip file, as various blog posts have alluded to), and if more / other similar issues arise, we shouldn't be more vulnerable than other browsers.

status-firefox84: --- → affected
status-firefox85: --- → affected
status-firefox86: --- → affected
status-firefox87: --- → affected
status-firefox-esr78: --- → affected
Flags: needinfo?(jcristau)
Flags: needinfo?(dveditz)
OS: Unspecified → Windows
Hardware: Unspecified → Desktop
Attached file Bug 1689598 —

I found this interesting article saying this NTFS bug is caused by a case-sensitive comparison for "$i30" in ntfs.sys.

Anyway, I agree that it's safer for us to disallow access to NTFS special paths not only for $I30.

Comment on attachment 9200134 [details]
Bug 1689598

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Between the keywords in the list of forbidden files, and the issue with windows paths being a google search away, I imagine pretty easily (even without the tests, which I don't propose landing at the time of 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?: Yes
  • Which older supported branches are affected by this flaw?: everything
  • 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?: The integration with current code is fairly limited, so I don't anticipate issues; https://bugzilla.mozilla.org/show_bug.cgi?id=1496578 landed on 77 and the nsLocalFileWin code is really old.

In terms of risk, these are reasonably targeted fixes that shouldn't affect normal use.

  • How likely is this patch to cause regressions; how much testing does it need?: It's possible, I guess? But at this point, a number of different people have looked at the patch, and although that has definitely improved patch quality substantially, nobody has raised "but this is going to break critical usecases" kind of flags. So, hopefully, not super likely. It would be useful for QA to kick the tires and confirm the original bug no longer reproduces...
Attachment #9200134 - Flags: sec-approval?

Comment on attachment 9200134 [details]
Bug 1689598

sec-approval=dveditz, WITHOUT the test until later of course

Flags: needinfo?(dveditz)
Attachment #9200134 - Flags: sec-approval? → sec-approval+

We need to include this fix on ESR. Setting the target to 86+ because that's the next train I know about. If we decide we need to fix this in an 85 point release we probably want to do the same for ESR.

tracking-firefox-esr78: ? → 86+

Denial of Service is usually sec-moderate, though this is by far the most desastrous DoS I could think of (e.g., corruping the system disk).

I thought we required file access to go through the parent process. Do I misremember stuff or did this also expose a hole in the sandbox?

Flags: needinfo?(bobowencode)
Keywords: sec-moderate
Blocks: 1690245

I've just checked, and the patch grafts cleanly to release and beta, trivial context conflicts for esr - I'll attach an esr patch. I've triggered autoland.

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

We need to include this fix on ESR. Setting the target to 86+ because that's the next train I know about. If we decide we need to fix this in an 85 point release we probably want to do the same for ESR.

As for the dot release, this particular issue should be fixed by MS in the next patch tuesday (next week), I would hope -- but obviously I have no special insight in what Microsoft is and isn't going to do about this, or how quick their update uptake is for those.

Comment on attachment 9200134 [details]
Bug 1689598

Beta/Release Uplift Approval Request

  • User impact if declined: Alarming Win10 notification when clicking a malicious link, that prompts a machine reboot and checkdsk. In a minority of cases, this appears to lead to actual disk corruption and therefore bricked machines.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Run attached testcase on Windows 10 in a virtual machine or some other environment where you've minimized risk of data loss and click the link.
  • List of other uplifts needed: n/a
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Messing with local file handling code isn't exactly the least-risky thing to do on a branch, but it's the thorough fix. I've tried to mitigate this by writing thorough tests at the same time (though we can't land then until we ship the change...).

A less risky option would be only taking the URIFixup.jsm change, but that risks having to take follow-up fixes for other issues in this general area as/when they arise.

  • String changes made/needed: nope
Attachment #9200134 - Flags: approval-mozilla-release?
Attachment #9200134 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Attached patch Patch for esr78 — — Splinter Review

[Approval Request Comment]
Fix Landed on Version: 87

See above for other questions.

Attachment #9200649 - Flags: approval-mozilla-esr78?

(In reply to Frederik Braun [:freddy] from comment #19)

Denial of Service is usually sec-moderate, though this is by far the most desastrous DoS I could think of (e.g., corruping the system disk).

I thought we required file access to go through the parent process. Do I misremember stuff or did this also expose a hole in the sandbox?

The Windows content process still has fairly wide read access to general system files and limited read access to some of the current firefox profile.
So I guess the particular path used to trigger the Windows bug was not blocked.

Hopefully we're getting closer to improving on the file access required within the content process, it would be interesting to see if that would have stopped this issue.

Flags: needinfo?(bobowencode)

(In reply to Bob Owen (:bobowen) from comment #23)

Hopefully we're getting closer to improving on the file access required within the content process, it would be interesting to see if that would have stopped this issue.

FWIW, we're wondering if bug 1670242 would stop this.

https://hg.mozilla.org/integration/autoland/rev/ca17538f31ff62a0c6769ff0cd8d2af317a4549e
https://hg.mozilla.org/mozilla-central/rev/ca17538f31ff

Group: network-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox87: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch

(In reply to Frederik Braun [:freddy] from comment #25)

(In reply to Bob Owen (:bobowen) from comment #23)

Hopefully we're getting closer to improving on the file access required within the content process, it would be interesting to see if that would have stopped this issue.

FWIW, we're wondering if bug 1670242 would stop this.

I don't think so, that bug is about principals, and this bug is about URIs. We won't be able to restrict creation of x-origin URIs on a per-process basis. Even blocking file: URI creation would be a bit weird - chrome: and resource: URIs map to those, and they need to work in child processes...

Comment on attachment 9200134 [details]
Bug 1689598

Approved for 86 beta 6, thanks.

Attachment #9200134 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Reproduced the initial issue using a Windows 10 Home in VM, windows repair message appeared right after I visited the URL also after restarting it it jumped into repair mode. I confirm that using latest Nightly from today and latest Beta from treeherder I don't have the problems I had with Firefox 85.0.
Leaving qe-verify+ for verification on the remaining affected versions if the fix will be uplifted.

status-firefox86: fixed → verified
status-firefox87: fixed → verified

I'd prefer to let this ride to 86 in a couple of weeks, as this is kind of scary to take straight to release.

status-firefox85: affected → wontfix
Attachment #9200134 - Flags: approval-mozilla-release? → approval-mozilla-release-

Dan convinced me this isn't all that scary.

status-firefox85: wontfix → affected
tracking-firefox-esr78: 86+ → 85+

Comment on attachment 9200134 [details]
Bug 1689598

approved for 85.0.1

Attachment #9200134 - Flags: approval-mozilla-release- → approval-mozilla-release+

Comment on attachment 9200649 [details] [diff] [review]
Patch for esr78

approved for 78.7.1

Attachment #9200649 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+

Adding to the 85.0.1 and 78.7.1 release notes:
"Prevent access to NTFS special paths that could lead to filesystem corruption."

relnote-firefox: --- → 85+

Also verified that the issue does not persist using dot release builds RC 85.0.1 and 78.7.1esr on the same Win 10 VM machine.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
status-firefox85: fixed → verified
status-firefox-esr78: fixed → verified
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: