Closed Bug 905493 Opened 11 years ago Closed 11 years ago

Setting document.location via an Xray doesn't work correctly (ends up with the wrong security context)

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
23 Branch
Platform:
x86_64
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27
Tracking Flags:
Tracking Status
firefox24 --- affected
firefox25 - ---
firefox26 - ---
firefox27 - ---
firefox-esr17 --- unaffected
firefox-esr24 - ---

People

(Reporter: eb30750, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(4 files)

Screenshot from 2013-08-14 23:05:55.png
15.68 KB, image/png
Details
Test XPI add on
6.34 KB, application/x-xpinstall
Details
Fix setting document.location via an Xray to not enter the content compartment.
12.16 KB, patch
peterv
: review+
Details | Diff | Splinter Review
test configurations files
6.11 KB, application/zip
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130807125118

Steps to reproduce:

add-on stopped working with FF 23.0


Actual results:

when accessing a file in the add-on content file via javascript error access denied is generated


Expected results:

This add-on has executed properly for months with the same javascript code.  The file is an embedded "blank" html file that I load into and tab window.  This overwrites any and all html from the target tab window.  The add-on javascript is executing as chrome.  The attached screenshot captures the add-on content file.  The file blank.html is being accessed via javascript as wndPtr.document.location = "chrome://CmeExtends/content/blank.html"; which forces the blank screen to overwrite the existing contents of the target tab window.
Which add-on?

Are you the dev of this add-on? If yes, provide a reduced testcase.
Component: Untriaged → Extension Compatibility
Flags: needinfo?(eb30750)
My add-on simply creates a new tab window and then loops through a few times loading URLs in the newly created tab window.  When the iteration is through the blank html URL is loaded that resides in the add-on itself.  

LOOP
if(j > 0) gBrowser.removeCurrentTab();     // close previously opened tab window

var newTab = gBrowser.addTab("");
gBrowser.selectedTab = newTab;
wndPtr = gBrowser.contentWindow;

wndPtr.document.location = urlList[j];    // load new HTML page
END LOOP

gBrowser.removeCurrentTab();
var newTab = gBrowser.addTab("");
gBrowser.selectedTab = newTab;
wndPtr = gBrowser.contentWindow; 
wndPtr.document.location = "chrome://CmeExtends/content/blank.html";  

// generate summary report in HTML
var frmStr = "<html><head><title>Data Report</title></head><body>";
frmStr += "<p><font size='+1'><b> Data Report</b></font></p>";

for(n=0; n < sumRec.length; n++)
	frmStr += "<p><font size='-1'>"+sumRec[n].print()+"</font></p>\n";

frmStr += "</body></html>";

// overwrite blank html page with HTML report
wndPtr.document.writeln(frmStr);
wndPtr.document.close();


I tried a variant of this code and skipped loading the blank.html document into the newly opened tab window.  I directly attempted to write HTML directly to the new tab window such as wndptr.doucment.write("<html><body></body></html>");  This returned the error "insecure action."  

In summary FF 23 is blocking me from accessing my add-on's own content folder and my chrome created tab window.
Flags: needinfo?(eb30750)
Is it possible to attach your testcase as simple extension (.xpi) showing the issue?
Flags: needinfo?(eb30750)
Attached file Test XPI add on — 
This add on will add one menu item to the "Tools" menu.  There are three submenus.  The first will complete successfully when loading a URL.  The next two will fail with security warnings. It will be necessary to bring up the error console to locate the origin of these warnings.
Flags: needinfo?(eb30750)
Can I get an update on the status of this bug?  Based on updates I have received related to another reported security bug, I believe that there is another bug that has reported a similar issue.  If indeed this is true then these bugs need to be consolidated and then only one issued needs to be resolved.
Attachment #794341 - Attachment mime type: application/octet-stream → application/x-xpinstall
STR:

1) Install the add-on attached to this bug
2) Enable the Menu Bar
3) Open the Error Console (or Browser Console)
4) Tools > Test (in red)

Result:
Clicking on "LoadURL //:chrome" doesn't load the chrome file "chrome://testextends/content/blank.html" and there is no JS dialog box ("got to load chrome URL" message).

Error console: 
15:30:47.206 Security Error: Content at about:blank may not load or link to chrome://testextends/content/blank.html.
15:30:47.206 Error: Access to 'chrome://testextends/content/blank.html' from script denied overlay.js:26

Regression range:
good=2013-05-05
bad=2013-05-06
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c8e47b184aba&tochange=b109e2dbf03b

There are possible suspected bugs like bug 855971 and bug 820846...
Status: UNCONFIRMED → NEW
Component: Extension Compatibility → Add-ons Manager
Ever confirmed: true
Keywords: regressionwindow-wanted
Product: Firefox → Toolkit
Regressed by:
adaaf6641785	Peter Van der Beken — Bug 855971 - Switch HTMLDocument to WebIDL bindings. r=bz.
Blocks: 855971
status-firefox24: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox25: --- → ?
tracking-firefox26: --- → ?
tracking-firefox27: --- → ?
tracking-firefox-esr24: --- → ?
Component: Add-ons Manager → DOM
Keywords: regressionwindow-wanted → regression
OS: Linux → All
Product: Toolkit → Core
Strange behavior:
It works if use wndPtr.location instead of wndPtr.document.location
Because Window is not converted to WebIDL yet?
So this is a bug in the interaction of Xrays with the unforgeable holder on the document proxy, as far as I can see.

We land in mozilla::dom::HTMLDocumentBinding::DOMProxyHandler::getOwnPropertyDescriptor from Xray code, enter the compartment of the content-side expando object (which is also the unforgeable holder), get the stuff for the "location" property.  This hands out a property descriptor which has content JSFunctions for the getter and setter, of course.  Then we exit the content compartment.

Finally, we wrap this descriptor into the xray compartment before returning it.  But all this does is give us a cross-compartment wrapper to the content setter function.  Later this is called, which enters the content compartment and calls the content setter, so in nsLocation::SetHref we're in the content compartment and the security check fails.  This is obviously bogus.

What we probably need to do instead is that either mozilla::dom::HTMLDocumentBinding::ResolveOwnProperty or mozilla::dom::XrayResolveOwnProperty or the code in getOwnPropertyDescriptor needs to explicitly walk the sUnforgeableAttributes list and look for the id there, just like XrayResolveNativeProperty would for a non-proxy.

Putting this in mozilla::dom::XrayResolveOwnProperty would make sense to me...  In that case, could we rip out the (broken anyway) xray bits in getOwnPropertyDescriptor?  Or would that still be reachable somehow?
Flags: needinfo?(bobbyholley+bmo)
PJAL, I expect that in the meantime you should be able to replace this line:

  wndPtr.document.location = "chrome://TestExtends/content/blank.html";

with

  wndPtr.document.location.href = "chrome://TestExtends/content/blank.html";

and it would work correctly.
Summary: file access denied in add-on → Setting document.location via an Xray doesn't work correctly (ends up with the wrong security context)
> could we rip out the (broken anyway) xray bits in getOwnPropertyDescriptor?

I meant the unforgeable holder bits.  Obviously we'd still need the named getter mess.
Where the code snippets in comment #2 show "wndPtr.document.location = urlList[j];" urlList is just a place holder to shorten the bug report.  The real code is shown below.  Why would "wndPtr.document.location =" execute correctly when iterating through the indirect text from a table and load URLs correctly but the same code fail when using direct text?  In the code snippet wndptr.document.location executes successfully when iterating through a table of URLs.  It seems to me that is the access to the Content folder within my own add-on that is getting blocked.  I don't see how this explains the fact that changing the code to wndptr.location should matter.

 if(curIndx < symbTblArr.length)
	{
	wndPtr.document.location = baseLink + symbTblArr[curIndx].link;
	}
  else
	{
	wndPtr.document.location = "chrome://CmeExtends/content/blank.html";
        }
PJAL, the bug is that the document.location setter thinks its being called from the untrusted about:blank page, not from chrome code.  And untrusted pages are not allowed to load chrome:// URIs, so the load of the chrome:// URI fails.  Other URIs (e.g. http:// URIs) would work fine.  So it really depends on what the URI being loaded looks like.

The window.location setter doesn't have the same "thinks it's being called from untrusted code" bug, which is why using it would also help.
(In reply to Boris Zbarsky [:bz] from comment #10)
> enter the compartment of the content-side expando object (which is also the unforgeable holder)

I don't follow this part. From what I see in the code, the unforgeable holder is a per-prototype thing, whereas the expando object is a per-object thing. In particular, since the EXPANDO_OBJECT is distinct from the XRAY_EXPANDO_OBJECT (I suggested at one point that these should be merged), I don't see any reason why we ought to be touching the expando object at all

, get the stuff for the "location"
> property.  This hands out a property descriptor which has content
> JSFunctions for the getter and setter, of course.  Then we exit the content
> compartment.

Yes, this seems wrong.
> 
> What we probably need to do instead is that either
> mozilla::dom::HTMLDocumentBinding::ResolveOwnProperty or
> mozilla::dom::XrayResolveOwnProperty or the code in getOwnPropertyDescriptor
> needs to explicitly walk the sUnforgeableAttributes list and look for the id
> there, just like XrayResolveNativeProperty would for a non-proxy.

That sounds reasonable, yes. But note that I never reviewed this code.

> Putting this in mozilla::dom::XrayResolveOwnProperty would make sense to
> me...  In that case, could we rip out the (broken anyway) xray bits in
> getOwnPropertyDescriptor?  Or would that still be reachable somehow?

I'd think so, given my point above that we should never be pulling a damn thing from the other compartment (except perhaps things off of the expando object, which are nominally in the target compartment purely for convenience).
Flags: needinfo?(bobbyholley+bmo)
> From what I see in the code, the unforgeable holder is a per-prototype thing

Yes.  Sorry, I should have been more careful here.  We actually enter the compartment of js::GetGlobalForObjectCrossCompartment(js::UncheckedUnwrap(proxy)), but only when xpc::WrapperFactory::IsXrayWrapper(proxy).  Which is the content compartment, in the end.
Yeah, walking sUnforgeableAttributes in XrayResolveOwnProperty and not doing anything for Xrays in getOwnPropertyDescriptor seems like the right fix here. We should then also get rid of the code that walks sUnforgeableAttributes in XrayResolveProperty.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [need review]
Please renominate with justification for tracking - not clear what the criticality is here.
tracking-firefox25: ? → -
tracking-firefox26: ? → -
tracking-firefox27: ? → -
tracking-firefox-esr24: ? → -
This only affects addons that set document.location on content documents and set it to a URI content can't normally load.

I expect this is a pretty rare situation... not worth backporting, esp. since there are at least two simple workarounds.
Comment on attachment 808610 [details] [diff] [review]
Fix setting document.location via an Xray to not enter the content compartment.

Review of attachment 808610 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/BindingUtils.cpp
@@ +972,5 @@
> +                               JS::MutableHandle<JSPropertyDescriptor> desc,
> +                               const NativeProperties* nativeProperties)
> +{
> +  return !nativeProperties || !nativeProperties->unforgeableAttributes ||
> +    XrayResolveAttribute(cx, wrapper, obj, id,

Line this up with the !
Attachment #808610 - Flags: review?(peterv) → review+
Done, and https://hg.mozilla.org/integration/mozilla-inbound/rev/e03276bf4eaa
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla27
The problem was in fact with this patch.

We can't remove the unforgeable stuff in getOwnPropertyDescriptor: that's needed in the non-Xray case, of course.  I'm going to put it back, but condition it on !isXray.

Wes, thank you for backing this out, and sorry for the trouble.  :(
(In reply to Boris Zbarsky [:bz] from comment #24)
> We can't remove the unforgeable stuff in getOwnPropertyDescriptor: that's
> needed in the non-Xray case, of course.  I'm going to put it back, but
> condition it on !isXray.

Hrmpf.
https://hg.mozilla.org/mozilla-central/rev/4675e87c49b0
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
I have attached three zip files that contain variations of iFrame testing that I have performed over the past two days.  What I have found is that the FF domain security does not meet the W3C standards.  As long as the iFrame html document is at the same level or in a folder below the host html document FF will allow access to the iFrame contents.  However, if the host html document is in a folder at the same level as that of the iFrame html document then FF will yield a permission denied error and return a "null" when using getElementById for the document contents.  I tested the non working iFrame configuration using IE8 and it worked correctly the first time.

This discovery I believe is at the root of another bug I reported, 908973, that states many financial web pages that work on Safari and IE do not work on FF.  The responder to this reported bug says that I may need to contact my financial providers and report this as a bug when I clearly stated that these same financial web pages work perfectly with Safari and IE.  Most likely these financial web pages and others I have found embed their host html documents in a folder that is different that the folder than hosts their iFrame html document (or they dynamically add the iFrame).  Of course, this bug is even more ridiculous since this the Add-On I actually create a html fragment that contains a new iFrame that I append to my host html document and after it is appended FF retunrs "null" when I execute a getElementById on the appended iFrame.  This is the premise that this bug, 905493 was reported.  

Therefore, I believe bug 908973 can be merged into this bug and then it can be closed.  Second, this is a very serious problem as thousands of users are experiencing incorrect web page rendering by FF.  The FF domain security is inferior and does not meet the standard.  To me this makes FF pretty useless.  Domain security and FF has been an issue for years as it broke a perfectly working plugin of mine a few years ago and forced me to rewrite an entire workaround.  There is absolutely no reason for me to develop web pages using FF as a test bed until this matter is resolved and there is no reason for me to use FF to access by online accounts until this matter is resolved.  this needs to be FIXED ASAP.

Testing was performed using FF 25.0 and IE8 on Fedora 3.11.8-100.fc18.x86_64

    
    

    
  
I am reporting this as a domain security matter as since two different directories within the same higher directory should have access to each other's documents since they are on the "same level."  This coincides with the HTML security "within the same domain" such as all files located at www.mywebsite.com/cgi-bin, etc.  At the file system level this translates to file:///home/myaccount/documents, etc. Or C://users/documents, etc.

    
    

    
  
PJAL, the issue you're talking about has nothing to do with this bug as originally reported.  It sounds like you filed bug 908973 on it, so let's follow up there.
That said:

1)  There is no W3C standard for the security model of file:// URLs.  IE, Firefox, Safari,
    and Chrome all implement slightly different security policies for such URLs.
2)  The security model of file:// URLs is wholly distinct from that of http:// web pages,
    and hence any issues you observe with file:// URLs are not related to issues you
    observe with web pages.  The latter should get a separate bug as well, with steps to 
    reproduce.

    
    

    
  
That said, I _assume_ comment 29 was talking about file:// URLs, since the behavior it describes is the one file:// URLs implement.  If it's about http:// URLs, then as I said is a bug with clear steps to reproduce the problem for an http:// URL.

    
    

    
  
Why would it matter if the folder hierarchy is on an http server or my local file system?  It seems to me that whatever same domain security model is implemented that it would be consistent meaning if I have an html file on an http server at level /www/cgi-bin and this html page has an iframe that's src is at /root/www it should result in a permission denied error.  The same error should be generated if I have a webpage at file:///home/User123 that has an iframe that attempts to load its src from file:///home/root.  The security mechanism should realize the relative levels are in violation of the same level policy and generate an error.

    
    

    
  
> it should result in a permission denied error. 

Unfortunately the web doesn't work that way.  What it does have are these things called "domains" (which file:// URIs typically do not have), which are used as the security boundaries on the web.

Again, that has nothing to do with this bug.

    
    

    
  
I have installed FF 26.0 and retested my add-on.  The good news is that I can load the iframe with a local html file that is stored in the content extension folder.  I have removed all of my workarounds and the code works as before with regards to loading an iframe with a local file.  However, when attempting to load the local file (blank.html) using wndptr.document.location as shown below the code stops executing once the html file is loaded.  When I stay wndPtr.location as the target it works.  

wndPtr.document.location = "chrome://CmeExtends/content/blank.html"; (code halts after the local html file loads)

wndPtr.location = "chrome://CmeExtends/content/blank.html"; (this was the workaround and it still loads and code continues to execute)

wndptr is defined by

var newTab = gBrowser.addTab("");
gBrowser.selectedTab = newTab;
wndPtr = gBrowser.contentWindow;

    
    

    
  
PJAL, this bug is fixed in Firefox 27.  Given the existence of workarounds and the riskiness of the patch, it was not backported to what was then-aurora Firefox 26.
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: