670514 - (CVE-2012-1945) Arbitrary File + Directory read via .lnk files on Windows Share

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Paul Stone]

Attachment: PoC

If a user can be persuaded to visit an HTML file hosted on a public Windows share (see below) then that page can use .lnk files to read arbitrary files and directory listings from their machine.

Imagine we have index.html and foo.lnk in the same directory, with foo.lnk pointing to C:\boot.ini. The HTML file is loaded via a file URL then loads foo.link into an iframe. The contents of boot.ini are displayed but document.location of the iframe is still foo.lnk. However, if the outer page tries to access the iframe, it gets a permission denied error as Firefox still seems to realise that the outer page and inner page are from different origins/directories.

The trick is to have two link files:
baz.lnk -> index.html
foo.lnk -> C:\boot.ini

First we load baz.lnk, then replace baz.lnk with foo.lnk. The HTML page then loads baz.lnk (i.e. itself) using XHR or an iframe. Because the url of the requesting page matches the page loaded, Firefox assumes they're the same origin and allows the contents to be read. Both files and directories can be read in this manner. My PoC uses static .lnk files to read some system directories and the Firefox profile.ini file. However it would be possible to traverse the whole filesystem by grabbing a directory listing and then dynamically generating .lnk files on the server.

There are various method to get the user to load content from a public share (e.g. file://///myhost.com/share/index.html):
1. Use a Java applet to force a drop of the URL (see bug 546909) - PoC uses this
2. Put an innocent looking unhyperlinked HTTP url on the page and get the user to copy/paste it into the location bar. When the user has selected the URL, sneakily change the current selection to the file: URL so that gets copied instead (PoC also has this as an alternative option)
3. Possibly send the file URL in an email. I've not really looked into this, but I seem to recall Outlook will open up \\myhost.com\share\index.html in Firefox if it's the default browser.

This attack could either work within an intranet or possibly over the internet providing your ISP/Corporate firewall doesn't block the relevant ports. My ISP does, but I know not all do. I've attached the PoC as a zip file, as I don't currently have anywhere to host this publically. It's been tested on a few Windows XP machines, not sure if Win7 has any extra security measures to prevent things like this.

Finally, this bug was found in my own time, so I promise it won't be blogged, tweeted or publicised in any way before it's fixed.

Comment 1

[Paul Stone]

Attachment: PoC v2

Ok, the previous attachment won't work as-is. You need to create the index.lnk and reset.lnk files yourself. The ones in the previous version pointed to the IP of my test VM so they won't work. I've updated the README.txt file with instructions.

Comment 2

[Paul Stone]

A fourth way to get the public share to load is to offer a .lnk file for download over HTTP. Firefox will show the normal save file dialog, allowing the user to either open the shortcut directly or save it to disk. If Firefox is the default browser, then a shortcut pointing to an HTML file will open up the file in Firefox. 

Apart from the the file reading vulnerability, there seems to be a lot of other problems caused by .lnk files and/or connecting to public shares. For example, the Metasplot SMB sniffer module could be used to grab password hashes. It's also possible to create a .lnk to cmd.exe with arguments. Firefox allows you to save and open that shortcut without any of the normal warnings you get for executable files. Granted, you could argue that these are problems with Windows - but Firefox probably shouldn't expose them so easily.

Comment 3

[Benjamin Smedberg]

Does anyone know whether we can just rip out .lnk file handling altogether?

Brian, welcome to your first security bug! ;-)

Comment 4

[Paul Stone]

I noticed that Chrome seems to rename .lnk files downloaded over HTTP to .download. This prevents Windows from trying to automatically resolve the shortcut - with Firefox the shortcut is resolved as soon as it's downloaded because the Download manager fetches the icon and file size. 

Chrome does allow viewing files on Windows shares via a file:// URL and will follow .lnk files. However it always resolves the filename so the target of the .lnk file is always displayed in the location bar.

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #3)
> Does anyone know whether we can just rip out .lnk file handling altogether?
> 
> Brian, welcome to your first security bug! ;-)

We seem to treat them as generic files. I placed one up on a web server, clicked on it, was prompted to save as, which I did. Double clicking on that Windows prompted with the standard security dialog about downloaded content then launched the app. 

We have file extension restrictions that bring up the save as dialog on downloads, but I don't think we restrict any specific type of file. Maybe a "rename extension .xyz" filter down in nsExternalAppHandler might be the best bet.

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #5)
> (In reply to comment #3)
> > Does anyone know whether we can just rip out .lnk file handling altogether?
> > 
> > Brian, welcome to your first security bug! ;-)
> 
> We seem to treat them as generic files. I placed one up on a web server,
> clicked on it, was prompted to save as, which I did. Double clicking on that
> Windows prompted with the standard security dialog about downloaded content
> then launched the app. 
> 
> We have file extension restrictions that bring up the save as dialog on
> downloads, but I don't think we restrict any specific type of file. Maybe a
> "rename extension .xyz" filter down in nsExternalAppHandler might be the
> best bet.

Actually I meant the service - 

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2215

Comment 7

[Jim Mathies [:jimm]]

(In reply to comment #4)
> Chrome does allow viewing files on Windows shares via a file:// URL and will
> follow .lnk files. However it always resolves the filename so the target of
> the .lnk file is always displayed in the location bar.

This something we might want to add, but I'd say we should spin it off into another feature related bug. Currently browsing a local share results in the same save as dialog you get for the web, so lnk files in this case don't work anyway with fx.

Comment 8

[Paul Stone]

Just to be clear (and apologies for mixing separate issues in one bug) Firefox will load the target of .lnk files over file:// directly (without a download dialog) if the target is an HTML file, directory or other file type that Firefox can display.

Comment 9

[Brian R. Bondy [:bbondy]]

Thanks Paul, I was able to reproduce the problem as per your README.txt.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Fix for Arbitrary File + Directory read via .lnk files on Windows Share

Everything seems to work now.

The trick is early resolution of the lnk URI into the target path URI before the origins are compared in nsNetUtil.h's NS_SecurityCompareURIs.  

The lnk file cannot be resolved at the time of NS_SecurityCompareURIs because it is too late and the URIs are the same at that point and resolve to the same target.  The best place I could find to do this resolving was in nsDefaultURIFixup::FileURIFixup.

I don't have try server access yet (Bug 671744), but when I do I will make sure it builds on non Windows platforms and passes all tests on all platforms.

Comment 11

[Boris Zbarsky [:bzbarsky]]

The attached patch only affects files loaded via the url bar.  Seems like the bug would still be there if the file is loaded via some other means, no?

I don't see how this is a problem for XHR, because XHR should be doing a security check on the link-redirect, unless we've stopped treating these as redirects?

For subframes, though, it's a problem; perhaps the file special-casing in CheckMayLoad should be taught about .lnk files?

> The lnk file cannot be resolved at the time of NS_SecurityCompareURIs 

Why not?  Seems like it sure can.  But see above...

Comment 12

[Brian R. Bondy [:bbondy]]

Thanks for the quick response.  

> bug would still be there if the file is loaded via some other means, no?

I tried from the bookmarks bar, URL bar, and via command line to load the URL and the changed function resolves the path in all of those cases.  Let me know if I missed any cases to try.

> Why not?  Seems like it sure can.

Because at this point the LNK file will resolve to the same target of the replaced file, so if the check was put there, the origin check would return same origin.

Comment 13

[Brian R. Bondy [:bbondy]]

bz, I was working off of the attached proof of concept zip file and its readme file. I'll try the iframe example mentioned above as well and then let you know or submit a new patch.

Comment 14

[Boris Zbarsky [:bzbarsky]]

The things you tried are all the same thing: something that passes a string URI into docshell.

Try something like <a href>.

> Because at this point the LNK file will resolve to the same target of the
> replaced file

So the problem is that the principal points to the .lnk file, not what the link resolved to or something?

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, so we do the redirect thing for .url files.  But I assume we treat .lnk files as just normal files and the OS is what does the magic?

In that case the basic issue is that we should be storing the fully-resolved filename somewhere and using it for security checks.... except that may break other things.  dveditz, thoughts?

Comment 16

[Brian R. Bondy [:bbondy]]

> So the problem is that the principal points to the .lnk file, not what the link resolved to or something?

Yes correct.

> Try something like <a href>.

I tried created an html file that had such a link to the lnk but that also hits the changed code.  Maybe there is another way I can get it to not go to the docshell?

> ... I was working off of the attached proof of concept zip file...

It should be the same because the proof of concept simply does an XMLHttpRequest

Comment 17

[Brian R. Bondy [:bbondy]]

Actually sorry, the <a href> does call the function with the lnk file but reproduces the original problem, so I will investigate that more. 

On a side note, with Google Chrome (who doesn't have the problem), and with the patch, when you enter an lnk file it will update the URL bar to the resolved path.

With FF5 it will keep the lnk file as the path in the URL bar.

Comment 18

[Brian R. Bondy [:bbondy]]

> we should be storing the fully-resolved filename somewhere and using it for security checks

Is there any reason we want to preserve the lnk file?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> but that also hits the changed code.

The original file does.  But clicking the anchor shouldn't.

Just curious... what's the Unix behavior here with symlinks?  This is really the same issue.  Do we have the same problem there?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Is there any reason we want to preserve the lnk file?

Well, what should relative URIs do?  What should the base be?

Comment 21

[Robert Strong (they/them - no direct email)]

(In reply to comment #19)
> > but that also hits the changed code.
> 
> The original file does.  But clicking the anchor shouldn't.
> 
> Just curious... what's the Unix behavior here with symlinks?  This is really
> the same issue.  Do we have the same problem there?
Mac .webloc files are much more similar to Windows .lnk files

Comment 22

[Brian R. Bondy [:bbondy]]

> what's the Unix behavior here with symlinks? 

I think the problem would be present both in mac and linux so the fix did use the platform independent IsSymLink.  I will try those platforms though.

> Well, what should relative URIs do?  What should the base be?

With an <a href> to a relative .lnk file, it goes into the function as the already resolved fully spec'ed HTTP URL. But the fix doesn't work for clicking on links as you mentioned though so maybe this will be a problem in the new location I have a fix in.

> The original file does.  But clicking the anchor shouldn't.

It hits that function when clicking on the anchor with an input URL of the a href target (lnk file).  Not sure why, something from JS.  But as mentioned my patch doesn't fix that case so I will fix it in any case somewhere else.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Not sure why, something from JS. 

So not the actual codepath that leads up to the load, is the point.  Almost certainly the "show the url on hover" code.

I think the right thing to do here is to first figure out what our security policy and relative link behavior should be for the symlink case.  Then we can figure out how to implement that...

Comment 24

[Brian R. Bondy [:bbondy]]

> So not the actual codepath that leads up to the load

Ya, I had just tested initially with a breakpoint on the function but then realized that after.  You were right :)

>  figure out what our security policy and relative link behavior should be for the symlink case

I will wait for dveditz to comment before proceeding on this task.

Comment 25

[Paul Stone]

I just tested this on Linux with symlinks and it does indeed work. However it'd be rather more tricky to exploit than on Windows - lnk files are normal files that can be accessed remotely. Symlinks are a filesystem feature so it would be difficult for an attacker to place one on the user's machine.

Comment 26

[Boris Zbarsky [:bzbarsky]]

You can have symlinks on remote filesystems, last I checked.

Comment 27

[Paul Stone]

(In reply to comment #26)
> You can have symlinks on remote filesystems, last I checked.

I guess symlinks would work over an NFS mount, but there's no way (or at least I hope not) to force a user to mount a remote NFS share though Firefox. Firefox does support smb: and sftp: on Gnome now bug 494163 is fixed. However remote symlinks over those protocols would be resolved by the remote filesystem, not the user's system.

Unless there's another way I'm not thinking of? Anyway I guess it's best to resolve symlinks and lnk files in the same way just to simplify things.

Comment 28

[Paul Stone]

Hmm, apparently Samba implements some UNIX extensions which allow remote symlinks: http://wiki.samba.org/index.php/UNIX_Extensions#Minshall.2BFrench_symlinks

I'll have a play and report back.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 547405 [details] [diff] [review]
Fix for Arbitrary File + Directory read via .lnk files on Windows Share

This isn't the right fix.

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: Fix for Arbitrary File + Directory read via .lnk files

It is fixed now, I think correctly.

- The patch will work in all cases for loads even from a href links.
- If a .lnk URI is detected on the nsFileChannel, the patch will set the original URI to where the source data of the link first came from.  E.g. to the LNK's resolved target.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I'd like to understand the answers to my questions in comment 23 before reviewing, please.

Comment 32

[Brian R. Bondy [:bbondy]]

Ok thanks bz, np. 
I think the action item is for dveditz so I will feedback? him on this patch. 
Please let me know if there are any other action items for me before that feedback. 

For normal files I believe the security policy is that they are considered the same origin if they have the same URI.
For symlinks, the security policy was the same as for normal files just mentioned. 

What I think the policy should be for symlinks, and what this fix accomplishes, is that the origin is considered the same if the target of symlinks are the same.  This is because a symlink can point to different resources between requests while having the same URI.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> For normal files I believe the security policy is that they are considered the
> same origin if they have the same URI.

There are some complications involving subdirectory loads sourced from a file.  It's not as simple as "same URI".  And once you start talking subdirectories, where your file "really" lives matters.

Also, the relative URI resolution question still needs answering.

(Fwiw, I believe the patch is wrong on its face because it violates the originalURI contract, but I would like to understand what behavior we want before trying to suggest ways to achieve it....)

Comment 34

[Brian R. Bondy [:bbondy]]

Regarding the relative test:

Test case:

1.
file:///C:/a/b/c/test.lnk
Points to:
c:\test.html

2.
Inside test.html you have <a href="test2.html>link</a>

3. 
Does test2.html resolve to:
file:///C:/a/b/c/test2.html
or 
file:///C:/a/test2.html

Windows Results when dropping c:\a\b\c\test.lnk: 
- Chrome12 loads file:///C:/a/test.html clicking on relative link brings you to file:///C:/a/test2.html
- IE9 loads file C:\a\test2.html clicking on relative link brings you to C:\a\test2.html
- Opera11.5 loads file://localhost/C:/a/test.html clicking on relative link brings you to file://localhost/C:/a/test2.html
- Safari5.1: Reveals the path in explorer: C:\a\b\c\test.lnk
- Firefox5: loads file file:///C:/a/b/c/test.lnk (which contains test.html), clicking on link brings you to file:///C:/a/b/c/test2.html
- Firefox Nightly post-patch: loads file file:///C:/a/test.html, clicking on link brings you to file:///C:/a/test2.html

Linux results when dropping /home/bbondy/a/b/c/test.html: (which is a symlink to /home/bbondy/a/test.html)
- Konqueror loads file file:///home/bbondy/a/b/c/test.html, clicking on link brings you to file:///c:/a/b/c/test2.html
- Chrome12 loads file file:///home/bbondy/a/b/c/test.html, clicking on link brings you to file:///c:/a/b/c/test2.html
- Opera11.5 loads file localhost/home/bbondy/a/b/c/test.html, clicking on link brings you to localhost/home/bbondy/a/b/c/test2.html
- Firefox5 loads file file:///home/bbondy/a/b/c/test.html, clicking on link brings you to file:///c:/a/b/c/test2.html

Mac OS X symlinks (ln -s):
- Same as linux as expected

Mac OS X alias files:
- Chrome12: Downloads right away and when you open with Chrome it brings you to file:///Users/bbondy/a/test.html, clicking on link brings you to file://Users/bbondy/a/test2.html
- Opera11.5 downloads the file, when you open with Opera it asks to download it again
- Safari5.1: Opens Finder with the alias selected
- Firefox5: Brings up download dialog for /users/bbondy/a/b/c, select open with firefox, brings you to file:///Users/bbondy//a/test.html, clicking on link brings you to file:///Users/bbondy/a/test2.html

Depending on feedback on security policy we have several possible ways to proceed:
- Change the original URI to be the LNK target URI, which is what the current patch does, (but this is probably not desired, bz mention it violates the originalURI contract)
- Treat lnk as a redirects
- Get the link target's principal via getChannelPrincipal
- Something else

Need to know:
- What the security policy should be
- How relative link behavior should work, as we did before, or as the other browsers do in Windows?

Comment 35

[Boris Zbarsky [:bzbarsky]]

I guess the other question is whether we should aim for consistent behavior for .lnk and Unix symlinks or just do what other browsers do and be inconsistent....

Comment 36

[Paul Stone]

Although bug 494163 is fixed, gio integration isn't actually enabled in any Firefox builds I could find (nightlies, Ubuntu or Fedora). So it seems that this bug is currently only exploitable on Windows. That said, I expect gio will be turned on at some point in the future so I suppose the behaviour for symlinks should follow what .lnk does.

Comment 37

[Brian R. Bondy [:bbondy]]

I emailed on Aug 31st for feedback but have not gotten a response yet.
Boris is there someone else we can ask for the needed feedback on this?

Recall, needed feedback is:
- Comment 34 section "Need to know:"
- Comemnt 35

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Boris is there someone else we can ask for the needed feedback on this?

I don't know.

Did you send non-bugmail to dveditz already?  If not, could you do that please?

Comment 39

[Brian R. Bondy [:bbondy]]

Ya I've sent a non bugmail one on Aug 31st. Also I've tried to find dveditz on IRC but cannot find him. 

If there's anything else I can do to move this bug forward in the meantime please let me know.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Mail Lucas Adamski and ask him to make sure dveditz or someone else on the security team takes a look at this?

Comment 41

[Brian R. Bondy [:bbondy]]

Emailed.

Comment 42

[Daniel Veditz [:dveditz]]

The patch behavior for Windows seems right and consistency with other browsers is a plus. I'm OK with the behavior for Mac alias files, which I suspect this patch doesn't touch in any way (I assume we get the local file URL from the OS after it handles the alias).

The pre-existing behavior for symlinks seems correct. Haven't tested, but this patch looks like it changes that and that seems wrong. I'm perfectly fine treating traditional symlinks one way (as expected by linux users) and treating Windows .lnk files more like a redirect.

Changing the OriginalURI worries me because we rely on that to mean different things in other contexts. It's taking me a while going through the code to reassure myself that it's not going to mess with other cases.

I really wish we simply didn't support UNC paths (I already hear the screaming protests). Our code basically assumes "file:" means "local", but UNC blows a gaping hole in that assumption every time. I'd be OK continuing to pretend mapped drive letters are "local" because at least the user "mounted" it in a way.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Changing the original URI is not OK.  We can change the URI and set the "redirected" flag, though.

Comment 44

[Daniel Veditz [:dveditz]]

Comment on attachment 549410 [details] [diff] [review]
Fix for Arbitrary File + Directory read via .lnk files

So bz's last comment is an r- ?

Comment 45

[Boris Zbarsky [:bzbarsky]]

I think so, yes, but there was no point worrying about the code details until we had a clear plan for the desired behavior.

Comment 46

[Brian R. Bondy [:bbondy]]

Just finishing up on a silent update task, will get back on this late this week or early next. Thanks for the feedback dveditz/bz.

Comment 47

[Brian R. Bondy [:bbondy]]

Comment on attachment 549410 [details] [diff] [review]
Fix for Arbitrary File + Directory read via .lnk files

Cancelling review for now until I get a new patch with dveditz’s feedback.  I'll have time to look at this again after I focus on the silent update service which we're trying to get in v10.

Comment 48

[Paul Stone]

Brian, are you likely to have time to look at this bug soon? If not, is there anyone else who might be able to take it on?

Comment 49

[Brian R. Bondy [:bbondy]]

I can retake this bug within the next couple weeks and try a new patch. If you find someone who can do it before that please feel free to re-assign.

Comment 50

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

Implemented review comments as per Comment 43.
I think this is the correct handling now. 

Sorry for taking so long to get back to this, I had to re-setup the environment for this bug.  Should there be more review comments to implement, I'll make them my highest priority.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 605002 [details] [diff] [review]
Patch v3.

Why is REDIRECT_INTERNAL involved?  How does that fix the bug?

Shouldn't you just be setting the LOAD_REPLACE load flag?

Comment 53

[Brian R. Bondy [:bbondy]]

> Why is REDIRECT_INTERNAL involved? 

I must have misunderstood Comment 43 when you said: 

> We can change the URI and set the "redirected" flag, though.

Out of the redirect flags I could find, I thought that was the most appropriate one.

> How does that fix the bug?

Setting the flag isn't the part that fixes the bug, setting the URI to the resolved target is.

> Shouldn't you just be setting the LOAD_REPLACE load flag?

I'm very familiar with writing network protocol implementations, but not with mozilla/netwerk code yet, so I don't know the answer to the question. It sounds like you are suggesting that though, so I will attach a patch that way instead.  Thanks for the info.

Comment 54

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4.

The description of LOAD_REPLACE in nsIChannel.idl seems appropriate.
Updated patch as per review comments.

Comment 55

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the lag here.  I've been trying to understand why setting the URI to the resolved target but NOT setting LOAD_REPLACE was fixing the bug.  Why is that, exactly?  The principal should be based on the originalURI if LOAD_REPLACE is not set, which should be the .lnk URI, still.

That part really confuses me, and I'd like to understand what's going on with that.  where's the NS_SecurityCompareURIs from comment 10 happening?

Comment 57

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5

So the reason why it was working without the LOAD_REPLACE flag was because SetURI also sets the OriginalURI.  I was calling SetURI with the target of the lnk file.  The real original URI was lost. 

With this new patch, I retain the real original URI and specify LOAD_REPLACE.
I verified that removing the LOAD_REPLACE flag makes it so I can reproduce the original reported issue.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 615531 [details] [diff] [review]
Patch v5

The nsNetUtil code should probably preserve LOAD_REPLACE in all cases, not just for "file" scheme.  That's come up as an issue in the past, actually.

r=me with that changed.  Thank you for hunting down what was going on here!

If you can manage to write a test for this (esp. one that checks the originalURI bit), that would be great.

Comment 59

[Brian R. Bondy [:bbondy]]

Sounds good thanks for the review. 

I'll attach a new patch with the test for review and I'll make the change to preserve the LOAD_REPLACE flag in all cases and carry forward the existing r+ to that.

Comment 60

[Brian R. Bondy [:bbondy]]

Attachment: Patch v6

> The nsNetUtil code should probably preserve LOAD_REPLACE in all cases, 
> not just for "file" scheme.  That's come up as an issue in the past, actually.

Done.  Carrying forward r+.
I'll attach the test patch as a new patch to this bug before landing.

Comment 61

[Brian R. Bondy [:bbondy]]

Attachment: Test patch. v1.

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 620384 [details] [diff] [review]
Test patch. v1.

r=me

Comment 63

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/60613f18435b
http://hg.mozilla.org/mozilla-central/rev/c045085c0436

Comment 64

[Brian R. Bondy [:bbondy]]

I don't think this needs to go in aurora/beta but please let me know if you disagree dveditz.

Comment 65

[Daniel Veditz [:dveditz]]

The bias for high/critical bugs should be to ship them as soon as possible, unless something like patch risk outweighs that good. This does not seem to be a particularly risky patch.

We will need to include the fixes for the two regressions, bug 751905 and bug 754894

Comment 66

[Alex Keybl [:akeybl]]

Given Dan's Comment 65, please nominate for aurora/beta approval and land the patch prior to 5/21.

Also adding qawanted to perform some exploratory testing (once landed) around opening links to files on Windows Shares, to ensure we don't regress anything here.

Comment 67

[Brian R. Bondy [:bbondy]]

Comment on attachment 620323 [details] [diff] [review]
Patch v6

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown

User impact if declined: If a user can be persuaded to visit an HTML file hosted on a public Windows share then that page can use .lnk files to read arbitrary files and directory listings from their machine.

Testing completed (on m-c, etc.): The problem seems to be gone and initial regressions have been fixed. 

Risk to taking this patch (and alternatives if risky): I personally think the changes are somewhat risky and this security problem has always been there. I personally don't think this should go in (especially beta), but am requesting because I was asked to by dveditz and akeybl. 

String or UUID changes made by this patch: none

If this is marked for approval on Aurora or Beta I will bundle in the fixes to the regressions found in Bug 751905 and Bug 754894.

Comment 68

[Brian R. Bondy [:bbondy]]

You mentioned this would need to land prior to 5/21.  If possible please let me know as soon as possible because I'll be going away for the long weekend.

Comment 69

[Brian R. Bondy [:bbondy]]

I am working Friday by the way, so I didn't mean today in Comment 68.

Comment 70

[Alex Keybl [:akeybl]]

Comment on attachment 620323 [details] [diff] [review]
Patch v6

[Triage Comment]
Given Brian's concerns, we'll make sure to perform targeted QA around opening links from Windows Shares. Any further guidance in testing would be appreciated.

Approving for Aurora 14 and Beta 13. If landed on 5/22, this can still make it into our next beta.

An ESR patch will need to follow.

Comment 71

[Alex Keybl [:akeybl]]

(In reply to Alex Keybl [:akeybl] from comment #70)
> An ESR patch will need to follow.

(prior to 6/1)

Comment 72

[Brian R. Bondy [:bbondy]]

> Given Brian's concerns, we'll make sure to perform targeted QA around 
> opening links from Windows Shares. Any further guidance in testing 
> would be appreciated.

The change that was somewhat risky in particular was preserving the load replace flag on *all* channels.  The last bug wasn't discovered for 11 days after landing.  The problems could really show up anywhere so I can't come up with any specific guidance for finding regressions.  

QA should also please test to make sure they can reproduce the PoC on a build before this fix, but can no longer reproduce on a build after this fix.

Comment 73

[Brian R. Bondy [:bbondy]]

Attachment: Bundled Beta and Aurora patch - this bug + Bug 751905 + Bug 754894

Here's the bundled patch. 
Carrying forward beta+ and aurora+ (I can't mark them myself though).  I will probably use it for esr as well if it applies cleanly at a later date before june 1st.

Comment 74

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/cf5c4540fe22

I could not push to beta because the tree is closed at this time.  If someone notices it open please feel free to push the change set from aurora to beta as well.  Otherwise I should get to it tomorrow night.

Comment 75

[Brian R. Bondy [:bbondy]]

For anyone following along that doesn't already know, beta is still closed so could not land there still.

Comment 76

[Al Billings [:abillings - ex-MoCo]]

(In reply to Brian R. Bondy [:bbondy] from comment #75)
> For anyone following along that doesn't already know, beta is still closed
> so could not land there still.

How about ESR?

Comment 77

[Brian R. Bondy [:bbondy]]

Comment 73

Comment 78

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-beta/rev/7a2c1909e205

Comment 79

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-esr10/rev/8ef222645a1a

Comment 80

[u279076]

Removing qawanted as this will now fall into our daily [qa+] verification bucket. Note to verifiers to use the PoC as well as do some exploratory around Windows shares.

Comment 81

[u279076]

@Paul, it's going to take considerable effort for me to get an environment set up to even be able to test this fix. Would you kindly be able to verify the fix in Firefox 13, 14, 15, and latest-mozilla-esr10?
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-esr10/

Comment 82

[Paul Stone]

Just wanted to check - Firefox 13 = Beta, 14 = Aurora, 15 = Nightly?

Comment 83

[Brian R. Bondy [:bbondy]]

Yup until June 5th (or close to that day)
https://wiki.mozilla.org/Releases

After June 5th everything shifts one to the left and Nightly becomes 16.

Comment 84

[Paul Stone]

I've confirmed that the POC no longer works in the current ESR, Nightly, Beta and Nightly releases. Visiting .lnk files now causes a 'redirect' to the target of the lnk file.

Comment 85

[Brian R. Bondy [:bbondy]]

Thanks for verifying Paul!

Comment 86

[u279076]

Thanks Paul, did you have a chance to check Aurora?

Comment 87

[Paul Stone]

Yeah I did check Aurora too, just forgot to include it in the list

Comment 88

[u279076]

Thanks again, Paul.

Comment 89

[Huzaifa Sidhpurwala]

Maybe i am mis-understanding this issue here, but it seems that this bug affects only the Windows version of Firefox right?
Linux versions are not affected!

Comment 90

[Al Billings [:abillings - ex-MoCo]]

(In reply to Huzaifa Sidhpurwala from comment #89)
> Maybe i am mis-understanding this issue here, but it seems that this bug
> affects only the Windows version of Firefox right?
> Linux versions are not affected!

See comment 22, 27, 28, etc for discussion.

Comment 91

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719

Comment 92

[txema.gonz]

(In reply to Brian R. Bondy [:bbondy] from comment #22)
> > what's the Unix behavior here with symlinks? 
> 
> I think the problem would be present both in mac and linux so the fix did
> use the platform independent IsSymLink.  I will try those platforms though.
> 
> > Well, what should relative URIs do?  What should the base be?
> 
> With an <a href> to a relative .lnk file, it goes into the function as the
> already resolved fully spec'ed HTTP URL. But the fix doesn't work for
> clicking on links as you mentioned though so maybe this will be a problem in
> the new location I have a fix in.
> 
> > The original file does.  But clicking the anchor shouldn't.
> 
> It hits that function when clicking on the anchor with an input URL of the a
> href target (lnk file).  Not sure why, something from JS.  But as mentioned
> my patch doesn't fix that case so I will fix it in any case somewhere else.

I see this question is fixed, but I have come across some side effects. 

Developing in *NIX systems a TDD System (https://github.com/txemagon/lluvia-Project) we finally decided to host the framework inside its own folder and create a symbolic link into every package. Now developing with version greater than 12 is impossible.

I posted a question in https://support.mozilla.org/es/questions/951431 and a possible solution: letting the user to allow symlinks (hosted in github)

Finally, following the kind instructions of Daniel Holbert, I've landed here in this post. I'm pretty sure the solution is not going to be tough, but I feel this is too much for me.

Thanks for reconsidering the status of this bug.