Closed Bug 90757 Opened 19 years ago Closed 18 years ago

non-built-in DOM properties not subject to same-origin check


(Core :: Security: CAPS, defect, P2)

Windows NT





(Reporter: jruderman, Assigned: jst)



(Keywords: csectype-sop, Whiteboard: [ADT2 RTM][HAVE FIX][sg:branch])


(4 files, 2 obsolete files)

x is a window in another domain, with a form "av" containing a "q" field.

I can't access these:
1 x.foopy (undefined global)
2 x.document.links (builtin dom)

But I can access these:
3 (undefined dom)
4 x.document.av (altavista search form)
5 x.document.av.q (altavista search textbox)

However, I can't access:
6 x.document.av.q.value (builtin dom)
I shouldn't be able to access any of 1-6.
Attached file testcase
OK, but what can you actually do with those properties you can access? What
information can you steal?
I don't think you can get much information this way.  It probably lets an
attacker out whether a user is logged into bugzilla and some other sites, but
not much else.
So an attacker can see *if* you are logged into a site, but can't tell a) what
your name/pw is b) what you're looking at?
Right, unless the site creaates a form, form element, or image whose name is
derived from your username.  I don't think any sites do that.
Target Milestone: --- → mozilla0.9.4
Target is now 0.9.4, Priority P2.
Priority: -- → P2
Not critical for 0.9.4, moving to 0.9.5.
Group: netscapeconfidential?
Target Milestone: mozilla0.9.4 → mozilla0.9.5
time marches on. Retargeting to 0.9.6.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Moving the most time-critical bugs and minor security fixes to 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Mass change: It appears that several bugs got accidentally opened up as part of
a mass change - see bug 107718 (now fixed). Moving back to security-sensitive
group. These were open for about 10 days.
Group: security?
Target Milestone: mozilla0.9.7 → mozilla1.0
Target Milestone: mozilla1.0 → mozilla1.1
jst has agreed to look into this.
Assignee: mstoltz → jst
Whiteboard: [ADT2 RTM]
Target Milestone: mozilla1.1alpha → mozilla1.0.1
This patch looks like it changes a lot of key security code, and it does, but
mostly just to move the code around. The best thing to do when reviewing this
is to apply the changes and read the methods needsSecurityCheck(),
documentNeedsSecurityCheck(), windowNeedsSecurityCheck(), and
nsDOMClassInfo::doCheckPropertyAccess() and compare those to the old code.
There's one significant change in what the old needsSecurityCheck() code does,
it used to get the nsIScriptGlobal out of the wrapper, then get the
nsIScriptContext out of that and then get the JSContext out of the
nsIScriptContext only to compare it against the passed in cx, the new code
flips that change over and just gets the JSObject out of the wrapper that's
passed in and compares that to the global object in cx. The meaning of those
two checks should be identical.
Mitch, please review.
Oh, and this patch is trunk only, there's code in there that relies on
nsIDocument::GetContainer() which doesn't exist on the branch, and because of
that we won't be able to do security checks on documents that are not in a
window (i.e. the results of XMLHttpRequest or
DOMImplementation.createDocument()) since there's no way to reach the context
that the documents originated from. Same goes for the trunk now since we don't
give such documents a container yet, but that's coming later on the trunk... For
the branch we'll just take out the code that tries to get the script global out
of the document container and just let the code execute w/o doing a security
check, just like we do today...
Whiteboard: [ADT2 RTM] → [ADT2 RTM][HAVE FIX]
So, this patch is useless for the branch, as far as securing the document
object? Or is that only WRT documents not in a window?
Found a few problems with the original diff while sr'ing with rpotts.
Attachment #88722 - Attachment is obsolete: true
This is only useless on the branch wrt documents *not* in a window, documents
that are loaded into a window *will* get these security checks.
Comment on attachment 88741 [details] [diff] [review]
Add security checks to non-predefined document properties.

Rick says sr=rpotts.
Attachment #88741 - Flags: superreview+
Comment on attachment 88741 [details] [diff] [review]
Add security checks to non-predefined document properties.

Two minor nits:

+  // Don't check when getting the Components property, since we check
+  // its properties anyway. This will help performance.
+  if (id == STRING_TO_JSVAL(sComponents_id) &&
+      accessMode == nsIXPCSecurityManager::ACCESS_GET_PROPERTY && isWindow) {
+    return NS_OK;
+  }
Maybe move isWindow to the beginning of the if statement, so we don't do the
other checks for documents?

 but the
+	 // document must remain scriptable so we have no other choice
+	 // than to go on w/o doing a security check here.
You could say, "documents loaded through these methods have already been vetted
by the security manager before they were loaded, so allowing access here is
probably safe anyway."

Why did you remove the cached-context part of the needsSecurityCheck
optimization? Not that I'm complaining, I'm just curious.

> Maybe move isWindow to the beginning of the if statement, so we don't do the
> other checks for documents?

Moving the isWindow check to the beginning would make us have to check that in
every call when we only really care if we're accessing something.Components,
moving the check for id == Components makes us do only one check per call except
in the very rare case that someone actually accesses something.Components.

The reason I don't want to mess with the cached cx and wrapper in the document
security checks is that if I'd use the same cache for caching the context and
the *document* wrapper when someone touches a property on the document we'd end
up invalidating the cache which is really meant to speed up access to window
properties. So unless we create a new cache for document wrappers (whose
lifetime is really hard to know and control) we'd end up getting no gain at all
from the cache in a case like this:

  a =;
  b =;
  c = window.baz;

since in that case accessing the document property would invalidate the cx and
wrapper that we cache when accessing the property, and likewise
accessing the document property bar would invalidate the cache for when we
access window.baz. Our cache would be invalid in all those three calls (ignoring
what happens when actually assigning to those variables).

So I'm arguing that the performance of accessing document properties is not
nearly as critical as the performance when accessing window properties. And
besides, now the code inside the cache checks is so fast that it's arguable if
the cache really buys us anything at all performance wise.

New simplified (and less regression prone) branch patch on its way...  
This patch does the same thing that the previous one does but it does so w/o
touching the critical security optimizations that we have for access to window
properties. This patch duplicates more code and is not as clean, but it's a
safer approach.
This is the same as the first diff in this bug except that it addresses Mitch's
comments and adds a cache for the JS object in a window's wrapper which gives
us a ~10% speedup of windowNeedsSecurityCheck().
Attachment #88741 - Attachment is obsolete: true
Comment on attachment 89338 [details] [diff] [review]
Less regression prone version for the branch.

r=mstoltz for the branch
Attachment #89338 - Flags: review+
Comment on attachment 89361 [details] [diff] [review]
Updated patch for the trunk.

and likewise for the trunk
Attachment #89361 - Flags: review+
Comment on attachment 89338 [details] [diff] [review]
Less regression prone version for the branch.

rpotts says sr=rpotts
Attachment #89338 - Flags: superreview+
Comment on attachment 89361 [details] [diff] [review]
Updated patch for the trunk.

a=chofmann for the trunk landing.
Attachment #89361 - Flags: approval+
Comment on attachment 89338 [details] [diff] [review]
Less regression prone version for the branch.

Moving approval to the correct diff.
Attachment #89338 - Flags: approval+
Attachment #89361 - Flags: approval+
Comment on attachment 89338 [details] [diff] [review]
Less regression prone version for the branch.

This fix was checked in on the trunk. Once it has baked there we might want
this on the branch, and once we know this is good, or we know that we don't
need this on the branch we can land the real fix (last attachment) on the
How is this patch doing on the trunk?
Doing good for all I know, no regressions reported so far.
Is this still wanted on the branch? we're done with 1.0.1.

I've asked valeski and jesup for approval to check into the 1.0 branch.

If they grant approval, please check into the branch and also check in the final
trunk patch, then mark this bug FIXED.

If they don't grant approval, then just check in the final trunk patch and mark
this FIXED. for 1.0 branch.  Change mozilla1.0.2+ to fixed1.0.2 when
checked in.  Make sure it gets added to any list Asa is keeping of fixed
security bugs in 1.0.2 (probably fixed1.0.2 is all that is needed).
OK jst, please polish this off. Thanks.
Comment on attachment 89361 [details] [diff] [review]
Updated patch for the trunk.

Attachment #89361 - Flags: superreview+
Whiteboard: [ADT2 RTM][HAVE FIX] → [ADT2 RTM][HAVE FIX][sg:branch]
renewal of a=rjesup for 1.0 branch
What's the status on this bug? It's got an ancient patch, stale 1.0.2 branch
approval, apparently not fixed on the trunk? Is this ready to go, is it going?
Keywords: adt1.0.2
From comment 29, I thought this HAS been checked into the trunk, but it's not on
the branch.
This bug is fixed on the trunk, I can't remember if it was fixed on the branch
or not, but AFAIK the trunk is what really matters at this point.

This diff doesn't change how we do security checks, it just eliminates some
duplicated code, and adds some more caching to avoid doing unnecessary security
checks. IOW, the security holes are plugged already, this just cleans up code
and speeds things up a bit.
so is it time to close this bug?

when you say "branch is not important" does that mean the vulerability is closed

from the perspective of embedding and distribution customers needing the fix on
a stable release the branch is important... 
Dan will talk to jst per adt mtg
Johnny explained this applies to user-created properties on the document object
and not the window object, which a site author would have to go out of their way
to do. Maybe a way for two cooperating sites to communicate, but not a serious
privacy problem that needs to be fixed on the branch.

Keywords: adt1.0.2adt1.0.2-
If this has been fixed on the trunk and won't be fixed on the 1.0 branch
shouldn't we open this up? If it's being left open for code cleanup could we
close this one so it doesn't look like an unfixed security problem, and move the
cleanup into a new bug?
Target Milestone: mozilla1.0.1 → ---
Good idea. I'm marking this fixed. Johnny, please open a new bug on whatever
cleanup work is necessary for the branch.
Closed: 18 years ago
Resolution: --- → FIXED
Group: security
Keywords: csec-sop
You need to log in before you can comment on or make changes to this bug.