User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.75 Safari/537.36 Steps to reproduce: on windows it's possible to bypass proxy settings by typing/pasting something like "\\evil-attacker.com\" to the address bar. This issue also affects Tor Browser. Note that the user must type or paste the text; webpages cannot trigger this issue. Actual results: smb connection attempt is performed by the operating system, bypassing proxy settings Expected results: tcpdump/wireshark will show smb traffic outside the proxy

What at least should happen is: 1) No proxy bypass by just pasting/typing in the URL bar domain (without hitting the return key or doing something similar expressing that one really wants to kick off the request) 2) In case a proxy is configured fail hard unless a preference is flipped or some other means available that indicates a user really wants to bypass the proxy. Otherwise the behavior (as it is right now) might be very surprising and dangerous.

A SMB url is a a file url (it is converted to it). File URLs are going directly to the filesystem, and not via a proxy, unless the proxy is set in the system (which I'm not sure would cover this as well, but likely yes). If the browser is set to handle file: URLs we open it (via command line). If a page has a link to file: anchor then we don't load it but likely hit bug 1412081. Same for a redirect to file: URL.

Another example of this issue that got reported to us by qab is: 1) Have victim drag and drop an anchor tag pointing to 'about:memory?file=\\localhost\\q.json.gz' inside bookmarks bar. 2) Victim then clicks on bookmark to visit URL. 3) An unproxied connection is made to 'localhost'

(In reply to Georg Koppen from comment #3) > Another example of this issue that got reported to us by qab is: > > 1) Have victim drag and drop an anchor tag pointing to > 'about:memory?file=\\localhost\\q.json.gz' inside bookmarks bar. > 2) Victim then clicks on bookmark to visit URL. > 3) An unproxied connection is made to 'localhost' Note for me: this ends up in FileReader.createFromFileName, which ends up creating nsLocalFile and passing it to FileCreatorHelper::CreateFile.

So, after a chat with Bob Owen, based on his suggestion I will first try to slightly modify the content process sandboxing restrictions to deliberately break access to network drives and see how it works: 1. change https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp#400 and below to USER_INTERACTIVE 2. remove the check for aIsFileProcess at https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp#485 Thanks, Bob!

with this patch I can't access smb shares (\\remote-server.com\share\) from the content process (access denied). I can run a profile on a smb share (c:\local\firefox.exe -profile \\server\share\profile). I can also run firefox (through a .lnk) from a share (> \\server\share\firefox.exe -profile \\server\share\profile). that's all nice, but the system IS trying to connect the `remote-server.com` on 445 samba port - confirmed in wireshark. hence, this doesn't solve the original problem.

I can see the Windows systems makes an attempt to connect the remote server when we get (for instance) here: Stack 1: ntdll.dll!NtQueryAttributesFile() Unknown KernelBase.dll!GetFileAttributesW() Unknown > xul.dll!nsLocalFile::HasFileAttribute(16, 0x00000063073ed482) Line 2932 C++ xul.dll!nsLocalFile::IsDirectory(0x00000063073ed482) Line 2902 C++ xul.dll!net_GetURLSpecFromFile(0x00000231a4b20680, {...}) Line 146 C++ xul.dll!nsFileProtocolHandler::GetURLSpecFromFile(0x00000231a4b20680, {...}) Line 281 C++ xul.dll!NS_GetURLSpecFromFile(0x00000231a4b20680, {...}, 0x0000000000000000) Line 1299 C++ xul.dll!nsDefaultURIFixup::ConvertFileToStringURI({...}, {...}) Line 666 C++ xul.dll!nsDefaultURIFixup::FileURIFixup({...}, 0x00000063073ed7e8) Line 625 C++ xul.dll!nsDefaultURIFixup::GetFixupURIInfo({...}, 0, 0x00000063073edda0, 0x00000063073edbb0) Line 204 C++ xul.dll!nsDefaultURIFixup::CreateFixupURI({...}, 0, 0x00000063073edda0, 0x00000063073eddb8) Line 99 C++ 0 PUIU_createFixedURI(aSpec = "\\janbambas.cz\nonexistent-share\") ["resource:///modules/PlacesUIUtils.jsm":220] this = [object Object] 1 PUIU_markPageAsTyped(aURL = "\\janbambas.cz\nonexistent-share\") ["resource:///modules/PlacesUIUtils.jsm":429] this = [object Object] 2 addToUrlbarHistory(aUrlToAdd = "\\janbambas.cz\nonexistent-share\") ["chrome://browser/content/browser.js":4242] this = [object ChromeWindow] 3 _loadURL(url = "\\janbambas.cz\nonexistent-share\", browser = [object XULElement], postData = null, openUILinkWhere = "current", openUILinkParams = undefined, mayInheritPrincipal = false, triggeringPrincipal = undefined) ["chrome://browser/content/urlbarBindings.xml":846] this = [object XULElement] 4 handleCommand(event = [object KeyboardEvent]) ["chrome://browser/content/urlbarBindings.xml":808] this = [object XULElement] 5 anonymous(eventType = "textentered", param = [object KeyboardEvent]) ["chrome://global/content/bindings/autocomplete.xml line 422 > Function":3] this = [object XULElement] 6 anonymous([object KeyboardEvent]) ["self-hosted":1026] this = [object XULElement] 7 onTextEntered(event = [object KeyboardEvent]) ["chrome://global/content/bindings/autocomplete.xml":256] this = [object XULElement] 8 handleEnter(event = [object KeyboardEvent]) ["chrome://browser/content/urlbarBindings.xml":1519] this = [object XULElement] 9 handleKeyPress(aEvent = [object KeyboardEvent]) ["chrome://global/content/bindings/autocomplete.xml":503] this = [object XULElement] 10 onKeyPress(aEvent = [object KeyboardEvent]) ["chrome://browser/content/urlbarBindings.xml":288] this = [object XULElement] 11 onxblkeypress(event = [object KeyboardEvent]) ["chrome://global/content/bindings/autocomplete.xml":617] this = [object XULElement] Stack 2: C++: nsDefaultURIFixup::CreateFixupURI 0 formatValue() ["chrome://browser/content/urlbarBindings.xml":527] this = [object XULElement] 1 onxblblur(event = [object FocusEvent]) ["chrome://browser/content/urlbarBindings.xml":1681] this = [object XULElement] Hence, it seems like blocking inside nsLocalFileWin is the right approach.

(In reply to Honza Bambas (:mayhemer) from comment #8) > Created attachment 8971330 [details] [diff] [review] > wip1 (nsLocalFile* early blocker) just tested and it's still not strict enough :/

The problem is that the stack from comment 7 happens on the parent process, right at the moment you paste \\janbambas.cz\nonexistent-share\ to the address bar... This will need even more care.

Starting to think to allow access only to profile paths and the installation path, nothing else, when UNC is passed on both processes.

I'd love to find someone who could overlook this more globally... This simply blocks all UNC paths that are not the binary dir or any of the profile dirs (roaming/local). The trickiest part was to find the right place how to init the preferences and the whitelist. Tested on a win box running from a share and having a profile on a share. No remote SMB requests seen in wireshark, everything else seems to work. This is windows specific, but I didn't want to bother with #ifdefs when we are likely to use the same thing for bug 1412081 which is on osx and maybe on linux too? I think the pref name would likely change as well with that bug. I deliberately leave this out off all.js.

Would your patch help against the same problem in the Windows file picker dialog? (See: https://trac.torproject.org/projects/tor/ticket/18101#comment:70 for context) I guess, no, as the proxy bypasses seem to be able to occur as one types, but maybe the things that get pasted would be safe? (I am just curious and "no" is not a bad answer. :) )

Comment on attachment 8971659 [details] [diff] [review] v1 Review of attachment 8971659 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/io/FilePreferences.cpp @@ +22,5 @@ > + static Paths sPaths; > + return sPaths; > +} > + > +static void AllowPathPrefix(char const* directory) I would call it "AllowSpecialDirectory" since we allow the directory represented by this string, instead of using this string as the prefix. @@ +35,5 @@ > + if (NS_FAILED(file->GetTarget(path))) { > + return; > + } > + > + if (!StringBeginsWith(path, NS_LITERAL_STRING("\\\\"))) { Add a comment as to why we do this. I assume it's because the profile may not have a path that begins with \\ and we only want to block \\ paths. @@ +76,5 @@ > + return false; > + } > + > + // When we are here, the path has a form "\\path\prefixevil" > + // while we have an allowed prefix of "\\path\prefix". I see that we only do the check for the InitWithPath call. But I think you can also create an nsIFile for "\\path\" and use append()/appendNative() (and maybe others) to get to "\\path\prefixevil" Can you check if this is possible? We may have to place the check somewhere else, such as in ResolveAndStat()

(In reply to Georg Koppen from comment #13) > Would your patch help against the same problem in the Windows file picker > dialog? (See: > https://trac.torproject.org/projects/tor/ticket/18101#comment:70 for > context) I guess, no, as the proxy bypasses seem to be able to occur as one > types, but maybe the things that get pasted would be safe? (I am just > curious and "no" is not a bad answer. :) ) No. The patch only works for how Firefox from inside handles files. That issue seems to be related to an external widget code we have zero control over.

(In reply to Valentin Gosu [:valentin] from comment #14) > I would call it "AllowSpecialDirectory" since we allow the directory > represented by this string, instead of using this string as the prefix. Just AllowDirectory will do IMO then. > Add a comment as to why we do this. > I assume it's because the profile may not have a path that begins with \\ > and we only want to block \\ paths. Yep. > I see that we only do the check for the InitWithPath call. > But I think you can also create an nsIFile for "\\path\" and use > append()/appendNative() (and maybe others) to get to "\\path\prefixevil" > Can you check if this is possible? We may have to place the check somewhere > else, such as in ResolveAndStat() This is a very good point! I think it is, but I don't think we have to _move_ the check, we will simply have to put to more places... I will walk it again. There are winapis called outside ResolveAndStat that may trigger the leak as well.

(In reply to Honza Bambas (:mayhemer) from comment #16) > This is a very good point! I think it is, but I don't think we have to > _move_ the check, we will simply have to put to more places... I will walk > it again. There are winapis called outside ResolveAndStat that may trigger > the leak as well. Append can only go deeper (disallows "\.." component) and we always add '\' before the added name. Hence we are safe. You can't also create "\\evil" from e.g. "\" since InitWithPath disallows mWorkingPath to have a trailing slash and crippled "\\" will be discarded by my checks (if not later by something else). I found only one more problem - you can rename (try to) a file "\\server\whitelisted" to "\\server\evil". CopySingleFile (called from various places) now explicitly checks the destination path.

- added comments - added check to win!nsLocalFile::CopySingleFile for the destination path being in the whitelist

as to be landed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f0b2e835712f6b9b4f02f0dcb5622f4b0abdb69 with the pref on, just from curiosity: https://treeherder.mozilla.org/#/jobs?repo=try&revision=020722abec3785eda8ad463257e0a159abd068e6

Comment on attachment 8972049 [details] [diff] [review] v1.1 although this is sec-other (a+ not needed) asking sec-approval since this is sec-high for TOR. [Security approval request comment] How easily could an exploit be constructed based on the patch? - relatively easily Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? - potentially yes Which older supported branches are affected by this flaw? - all of them If not all supported branches, which bug introduced the flaw? - since ever Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? - old code, unchanged for several versions; risk: zero - off by default How likely is this patch to cause regressions; how much testing does it need? - this is off by default, so no-op at all - when on, I did a lot of local testing with running firefox (fresh profile) from a remote share + having a profile on a remote share. no regressions found, so it's not likely to cause regressions

(In reply to Honza Bambas (:mayhemer) from comment #20) > Comment on attachment 8972049 [details] [diff] [review] > v1.1 > > although this is sec-other (a+ not needed) asking sec-approval since this is > sec-high for TOR. Thanks for taking this into account. > > [Security approval request comment] > How easily could an exploit be constructed based on the patch? > > - relatively easily Honza: in the description it is said this issue can't be exploited by a webpage, i.e. remotely. Do you agree with that and the relative easiness of constructing an exploit you have in mind is "given some other means, like social engineering, this can be exploited easily"? In general, if this bug could get treated as a "normal" sec-high bug with getting into mozilla-central X days before the next release that would be ideal. I think we'd want to make some further tests with this preference on, if possible, and would ship a backport of this patch with the regular alpha and stable Tor Browser being based on ESR 52.9.

I'm giving sec-approval+ for checkin on May 29, three weeks into the next release cycle. That should limit the exposure window a bit.

(In reply to Georg Koppen from comment #21) > (In reply to Honza Bambas (:mayhemer) from comment #20) > > Comment on attachment 8972049 [details] [diff] [review] > > v1.1 > > > > although this is sec-other (a+ not needed) asking sec-approval since this is > > sec-high for TOR. > > Thanks for taking this into account. > > > > > [Security approval request comment] > > How easily could an exploit be constructed based on the patch? > > > > - relatively easily > > Honza: in the description it is said this issue can't be exploited by a > webpage, i.e. remotely. Do you agree with that and the relative easiness of > constructing an exploit you have in mind is "given some other means, like > social engineering, this can be exploited easily"? The "relatively easily" expression means that a potential attacker could guess what we are trying to fix here. It DOES NOT mean it's easy to perform the exploit. As stated in the description of this bug, you need to make people drop or somehow enter and navigate to a crafted file://server or \\server url or path. That is still IMO not that easy. > > In general, if this bug could get treated as a "normal" sec-high bug with > getting into mozilla-central X days before the next release that would be > ideal. I think we'd want to make some further tests with this preference on, > if possible, and would ship a backport of this patch with the regular alpha > and stable Tor Browser being based on ESR 52.9.

Honza, while working on another file bug, I occurred to me that relative paths may be used to bypass the filter. Normally for UNC paths you shouldn't be able to navigate to an upper directory, but I'm not sure how well that is enforced. so if the only whitelisted directory is "\\allowed" and we InitWithPath("\\allowed\..\evil\bla") then call Normalize, that might bypass the entire filter. I haven't looked very closely at the windows code, so we might have this covered, but can you check if that is possible?

(In reply to Valentin Gosu [:valentin] from comment #24) > Honza, To answer this, I think I'll write proper tests for the patch. There could also be some ways to do "\\allowed\file"->Parent()->Parent()->Append("evil"). Parent()->Append() could potentially bypass. The code is hard to follow. For comment 17 I was mostly hunting Append("..") and similar cases. Somehow I was persuaded that the working path with ".." is invalid, but can't find the check now.

(In reply to Honza Bambas (:mayhemer) from comment #25) > (In reply to Valentin Gosu [:valentin] from comment #24) > > Honza, > > To answer this, I think I'll write proper tests for the patch. There could > also be some ways to do > "\\allowed\file"->Parent()->Parent()->Append("evil"). Parent()->Append() > could potentially bypass. The code is hard to follow. > > For comment 17 I was mostly hunting Append("..") and similar cases. Somehow > I was persuaded that the working path with ".." is invalid, but can't find > the check now. It's mentioned in the nsIFile IDL that Append("..") should throw - which does so in nsLocalFile::AppendInternal (nsLocalFileWin.cpp). I'll post my WIP patch with tests in the other bug.

Valentin, this patch tries to address your concern about the relative paths that can be actually passed to InitWithPath and could bypass (event though, IMO just potentially sake of this bug) whitelisting/blacklisting. I wrote a 'Normalizer' that is a Tokenizer based class that tries to resolve all the relative and self ('.') directories in the path w/o using any system APIs (to prevent any potential network requests) I don't know if that's the proper way and if there are any corner cases, but I think for the purpose it should work. Now, we have to synchronize your patch for bug 1412081 and mine (they totally collide :)) I've also added a test, same location, but I think it should live in xpcom, we are not testing anything from the necko code. Your test and mine could just be merged together. Then, the change I have some complains about (disallow relative paths ALWAYS) should be replaced with the normalizer and we should test against the normalized path. What do you think?

(In reply to Al Billings [:abillings] from comment #22) > I'm giving sec-approval+ for checkin on May 29, three weeks into the next > release cycle. That should limit the exposure window a bit. We will apparently miss this date...

Comment on attachment 8981394 [details] [diff] [review] v2 - still just a wip Review of attachment 8981394 [details] [diff] [review]: ----------------------------------------------------------------- This looks great. Thanks Honza! ::: xpcom/io/FilePreferences.cpp @@ +78,5 @@ > + bool CheckCurrentDir(); > + bool CheckSeparator(); > + > + Token const separator; > + nsTArray<nsDependentSubstring> stack; nit: mStack & mSeparator.

last few nits fixed, this is ready for landing. the test has been moved to IMO more proper place. The test passes locally. No try push is intentional (nothing to actually test, the patch by default has zero influence and when the pref is flipped, there is likely no test to cover.)

Comment on attachment 8984221 [details] [diff] [review] v2.1 Note that this has already been sec-a+ in comment 22.

Approval Request Comment [Feature/Bug causing the regression]: none, since ever [User impact if declined]: for Firefox default build zero (feature off by default) [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes, locally only [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: yes, bug 1463786 [Is the change risky?]: no [Why is the change risky/not risky?]: off by default, no side effects [String changes made/needed]: none

must run on win only!

I will prepare patches for other branches tomorrow (Friday). esr52 needs more care :( tokenizer changes needed for this patch don't build with VS 14.x.

https://hg.mozilla.org/mozilla-central/rev/ef46ec074d6a https://hg.mozilla.org/mozilla-central/rev/5cd3f1a923ef

Comment on attachment 8984230 [details] [diff] [review] v2.1 for Beta (needs the test perma failure on !win merged in)

Approval Request Comment, see comment 33

[Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: this is TOR sec-high User impact if declined: possibility to track users tricked to open file://///evil-server/fake-share and expose their IP Fix Landed on Version: 62 Risk to taking this patch (and alternatives if risky): none for default Firefox settings users, since this is off by default String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment] I think comment 39 covers this. So, Tokenizer template to support also char16_t can't build on esr52 (win). I wanted to use the CharSeparatedTokenizer but that is SO stupid that I simply could not. So, I (re)wrote a specific parser for file path normalization... With such a tight schedule despite this has changed some core functions, I believe the test coverage (which I ran locally on m-c with this patch applied) makes sure this will keep to the original purpose w/o a re-review. Locally builds on ESR52 until the build fails from a different reason...

v2.1 patches all depend on the patch for bug 1463786, which applies cleanly on beta, esr60. v2.2 doesn't need any additional uplifts.

Comment on attachment 8984469 [details] [diff] [review] v2.1 for Beta Fixes a sec-high issue for Tor users, adds automated tests, and the code isn't used by default Firefox builds, so low-risk to our users. Approved for 61.0b13, ESR 60.1, and ESR 52.9. Georg, are the Tor browser folks able to verify that things work correctly for them after this lands?

https://hg.mozilla.org/releases/mozilla-beta/rev/0c31585602a7 https://hg.mozilla.org/releases/mozilla-esr60/rev/1248809b8616

Backed out from ESR60 for bustage. Also, it looks like your Try push for ESR52 had Windows GTest failures, so I'm holding off on landing there too. Build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=182571952&repo=mozilla-esr60 GTest failures: https://treeherder.mozilla.org/logviewer.html#?job_id=182555844&repo=try

(In reply to Ryan VanderMeulen [:RyanVM] from comment #44) > Backed out from ESR60 for bustage. Thanks and sorry. > Also, it looks like your Try push for > ESR52 had Windows GTest failures, so I'm holding off on landing there too. > > Build bustage: > https://treeherder.mozilla.org/logviewer.html#?job_id=182571952&repo=mozilla- > esr60 > > GTest failures: > https://treeherder.mozilla.org/logviewer.html#?job_id=182555844&repo=try Please disregard this try push. The ESR52 patch is now different (different tokenization code) and the try push is for an older non functioning version of the patch. I will merge my new and working ESR52 patch also to apply on ESR60, do try push for both, refer them here, and ask for approval again.

Comment on attachment 8984737 [details] [diff] [review] v2.2 for ESR60 Looks like this one still has the weird nsLinebreakConverter.cpp build error. https://treeherder.mozilla.org/#/jobs?repo=try&author=honzab.moz@firemni.cz&fromchange=66c423594f3c7dfec87a03d5d0342f5a4f356c71&selectedJob=182760814 I'll look into this later.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #42) > Georg, are the Tor browser folks able to verify that things work correctly > for them after this lands? Yes, although we won't have for that during the All Hands week.

Comment on attachment 8984738 [details] [diff] [review] v2.2 for ESR52 Looks good on Try, approved for ESR 52.9. Looking forward to that fixed-up ESR60 patch.

Hey Honza, have you been able to look at that ESR60 build failure on the Try push?

Ryan, can you please push this patch to try for me when you have time? I don't have my commit key setup correctly here. Thanks.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f91e10ccf2fd1dd07fdf08137c4bfdb085a864e Looks like debug builds are crashing in the xpcshell self-tests, but we don't have symbols for those runs for some reason :\

Here's an OSX xpcshell run which has more stack info: https://treeherder.mozilla.org/logviewer.html#?job_id=183237275&repo=try&lineNumber=8325

(In reply to Ryan VanderMeulen [:RyanVM] from comment #56) > Here's an OSX xpcshell run which has more stack info: > https://treeherder.mozilla.org/logviewer. > html#?job_id=183237275&repo=try&lineNumber=8325 i'm reading prefs too soon on the child process... i can't take care now. got cold from the hotel a/c, feel like ****, leaving tomorrow.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b824cec796dde53aafdf990359f101e6606576da I'm adding the new pref to the early list, that helps to prevent the crash acording https://hg.mozilla.org/releases/mozilla-esr60/annotate/b82802657783/modules/libpref/Preferences.cpp#l926 Ben, please look at the changes in ContentPrefs.cpp or feel free to redirect to anyone else.

Thanks!

Okay, I cherry-picked the esr52 patch to test it in our Tor Browser context. It does not compile with mingw-w64 at least: In file included from /var/tmp/build/firefox-f435669b1491/obj-mingw/xpcom/io/Unified_cpp_xpcom_io1.cpp:2:0: /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFileCommon.cpp: In function 'void SplitPath(char16_t*, nsTArray<char16_t*>&)': /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFileCommon.cpp:184:13: error: invalid use of incomplete type 'class nsTArray<char16_t*>' aNodeArray.AppendElement(aPath); ^ In file included from /var/tmp/build/firefox-f435669b1491/obj-mingw/dist/include/nsReadableUtils.h:19:0, from /var/tmp/build/firefox-f435669b1491/obj-mingw/dist/include/nsString.h:14, from /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFileWin.h:12, from /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFile.h:42, from /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFileCommon.cpp:9, from /var/tmp/build/firefox-f435669b1491/obj-mingw/xpcom/io/Unified_cpp_xpcom_io1.cpp:2: /var/tmp/build/firefox-f435669b1491/obj-mingw/dist/include/nsTArrayForwardDeclare.h:25:7: note: declaration of 'class nsTArray<char16_t*>' class nsTArray; ^ In file included from /var/tmp/build/firefox-f435669b1491/obj-mingw/xpcom/io/Unified_cpp_xpcom_io1.cpp:2:0: /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFileCommon.cpp:192:17: error: invalid use of incomplete type 'class nsTArray<char16_t*>' aNodeArray.AppendElement(cp); ^ In file included from /var/tmp/build/firefox-f435669b1491/obj-mingw/dist/include/nsReadableUtils.h:19:0, from /var/tmp/build/firefox-f435669b1491/obj-mingw/dist/include/nsString.h:14, from /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFileWin.h:12, from /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFile.h:42, from /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFileCommon.cpp:9, from /var/tmp/build/firefox-f435669b1491/obj-mingw/xpcom/io/Unified_cpp_xpcom_io1.cpp:2: /var/tmp/build/firefox-f435669b1491/obj-mingw/dist/include/nsTArrayForwardDeclare.h:25:7: note: declaration of 'class nsTArray<char16_t*>' class nsTArray; ^ In file included from /var/tmp/build/firefox-f435669b1491/obj-mingw/xpcom/io/Unified_cpp_xpcom_io1.cpp:2:0: /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFileCommon.cpp: In member function 'virtual nsresult nsLocalFile::GetRelativeDescriptor(nsIFile*, nsACString_internal&)': /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFileCommon.cpp:213:29: error: aggregate 'AutoTArray<char16_t*, 32u> thisNodes' has incomplete type and cannot be defined AutoTArray<char16_t*, 32> thisNodes; ^ /var/tmp/build/firefox-f435669b1491/xpcom/io/nsLocalFileCommon.cpp:214:29: error: aggregate 'AutoTArray<char16_t*, 32u> fromNodes' has incomplete type and cannot be defined AutoTArray<char16_t*, 32> fromNodes; ^ make[5]: *** [Unified_cpp_xpcom_io1.o] Error 1 Let me try the esr60 one with an updated toolchain.

Adding Jacek as he might know what's up with the mingw-w64 bits...

Interesting, for us it builds. To fix the build bustage, use the same trick I did here: https://bugzilla.mozilla.org/attachment.cgi?id=8985950&action=diff#a/xpcom/io/moz.build_sec1 just move the FilePreferences.cpp file from UNIFIED_SOURCES to SOURCES. Then please submit the patch that works for you here so we can land it on ESR52. Thanks and sorry for inconvenience.

No worries. The attached fixup unbreaks compilation for us. Here is a try build for it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac08fb6268fb459d73f837d26ecea7bd33d7c4ca Setting `network.file.disable_unc_paths` to `true` does not cause any crashes or issues for us, as far as my testing goes. I still need to find a Windows box where I can actually reproduce the original bug, though, to verify the fix. But I am pretty confident that this got thoroughly done while writing and reviewing the patch. Thus, I think we are good here for now. FWIW: the esr60 patch builds fine for us. Thanks!

Comment on attachment 8986398 [details] [diff] [review] fixup patch for mingw-w64 build of esr52 Review of attachment 8986398 [details] [diff] [review]: ----------------------------------------------------------------- Stealing review! r=me assuming this is for esr52 only.

Thanks! This should be in today's ESR 52.9 RC2 respin. https://hg.mozilla.org/releases/mozilla-esr52/rev/755067c14b06

(In reply to Georg Koppen from comment #64) > Created attachment 8986398 [details] [diff] [review] > fixup patch for mingw-w64 build of esr52 > > No worries. The attached fixup unbreaks compilation for us. Here is a try > build for it: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=ac08fb6268fb459d73f837d26ecea7bd33d7c4ca > > Setting `network.file.disable_unc_paths` to `true` does not cause any > crashes or issues for us, as far as my testing goes. I still need to find a > Windows box where I can actually reproduce the original bug, though, to > verify the fix. But I am pretty confident that this got thoroughly done > while writing and reviewing the patch. Thus, I think we are good here for > now. It's always good to run another round of testing. You may find paths to pass down to system APIs I may have missed. > > FWIW: the esr60 patch builds fine for us. > > Thanks!

(In reply to Honza Bambas (:mayhemer) from comment #67) > (In reply to Georg Koppen from comment #64) > > Created attachment 8986398 [details] [diff] [review] > > fixup patch for mingw-w64 build of esr52 > > > > No worries. The attached fixup unbreaks compilation for us. Here is a try > > build for it: > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=ac08fb6268fb459d73f837d26ecea7bd33d7c4ca > > > > Setting `network.file.disable_unc_paths` to `true` does not cause any > > crashes or issues for us, as far as my testing goes. I still need to find a > > Windows box where I can actually reproduce the original bug, though, to > > verify the fix. But I am pretty confident that this got thoroughly done > > while writing and reviewing the patch. Thus, I think we are good here for > > now. > > It's always good to run another round of testing. You may find paths to > pass down to system APIs I may have missed. Fair enough. We got a report by Xiaoyin Liu at our HackerOne bug bounty program indicating that there is indeed at least some corner case left. From the bug report: """ If I map a network location to a drive letter on Windows, Tor Browser doesn't treat such location as a network address, and when referring to such location from a local HTML file, a direct connection is made to a remote server that user has already configured. Two limitations of this attack: 1) the attack can be performed only by opening a local downloaded HTML file; 2) it only works when user has already configured a mapped network drive, and the IP is leaked to that network location, not an attacker controlled one. But I think it still has some impact: there're some popular 3rd party websites that supports WebDAV, like OneDrive. Thus if a user has configured OneDrive WebDAV, when he opens a local HTML file, his IP may potentially be leaked to Microsoft. Another attack scenario is that in case an attacker controls the WebDAV server that user has mapped, and with some social engineering, user has downloaded and opened attacker's HTML in Tor, then attacker can know the user's IP. """ So assuming any network location is mapped to Z: then <!DOCTYPE html> <html> <head> <meta charset="UTF-8"> <title>Test</title> <script src="file:///Z:/223.txt"></script> </head> <body></body></html> (PoC by Xiaoyin Liu) bypasses the restrictions implemented by the fix for this bug once this HTML file is loaded from disk or if the user pastes or types "Z:" into the URL bar. The current suggestion is to check whether a drive letter is actually a network drive and to apply the restrictions accordingly. Not sure if that's a viable way to go or not, though.

(In reply to Georg Koppen from comment #68) > So assuming any network location is mapped to Z: then > > <!DOCTYPE html> > <html> > <head> > <meta charset="UTF-8"> > <title>Test</title> > <script src="file:///Z:/223.txt"></script> > </head> > <body></body></html> > > (PoC by Xiaoyin Liu) > > bypasses the restrictions implemented by the fix for this bug once this HTML > file is loaded from disk or if the user pastes or types "Z:" into the URL > bar. The current suggestion is to check whether a drive letter is actually a > network drive and to apply the restrictions accordingly. Not sure if that's > a viable way to go or not, though. I was aware of this and pretty much ignored this issue, intentionally. When the user has setup a network drive on a remote server that is potentially evil it's out of our control. The IP is already leaked, outside Firefox. I don't know about a way to join the file access and the user's Firefox profile except doing a crafted tracking file name access, or something. As there is a need for the user to map the drive first (not that simple task), I don't think we should be conserned. We can think of a protection like this tho, just in case, but in a new bug. The code I've introduced here would have to be extended also for regular paths and every path would have to be checked for either of 1) being on a net drive (an API call needed for the drive latter) or 2) being a UNC path (a simple begins-with "\\" check). Please file a new bug.

We got a report about the patch not properly working in our context (see: https://trac.torproject.org/projects/tor/ticket/26874). I don't know how the original bug report could conclude that the issue is not affecting ESR60 but Richard looked at it and came up with a fixup: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_26874&id=44252a9962b7f69006f375100e53b71660385be8. We think that needs an ESR60 uplift, too. Honza: What do you think? Should we file a follow-up bug with the patch for ESR60 attached?

Also, this discussion should happen in bug 1412081, actually.

That check came from the patch landed in bug 1412081, not this one. Per comment 72, this discussion should be happening there.

Sorry, I can't access that link. Plus Honza said to disregard their previous comment so I thought here was where discussion should happen...

You should have access to the other bug. And comment 71 is the one I believe we should be disregarding :)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #78) > You should have access to the other bug. And comment 71 is the one I believe > we should be disregarding :) Yes comment 71 is to disregard, thanks Ryan, and sorry for the confusion.

Comment on attachment 9000005 [details] SMB Leak (Please move to the appropriate bug, if found suitable)