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

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

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 x.document.foo (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.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Mitchell Stoltz (not reading bugmail)]

OK, but what can you actually do with those properties you can access? What
information can you steal?

Comment 3

[Jesse Ruderman]

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.

Comment 4

[ckritzer (gone)]

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?

Comment 5

[Jesse Ruderman]

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.

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Target is now 0.9.4, Priority P2.

Comment 7

[Mitchell Stoltz (not reading bugmail)]

Not critical for 0.9.4, moving to 0.9.5.

Comment 8

[Mitchell Stoltz (not reading bugmail)]

time marches on. Retargeting to 0.9.6.

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Moving the most time-critical bugs and minor security fixes to 0.9.7

Comment 10

[Bradley Baetz (:bbaetz)]

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.

Comment 11

[Mitchell Stoltz (not reading bugmail)]

Mozilla1.1

Comment 12

[Mitchell Stoltz (not reading bugmail)]

jst has agreed to look into this.

Comment 13

[Johnny Stenback (:jst)]

Attachment: Add security checks to non-predefined document properties.

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.

Comment 14

[Johnny Stenback (:jst)]

Mitch, please review.

Comment 15

[Johnny Stenback (:jst)]

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

Comment 16

[Mitchell Stoltz (not reading bugmail)]

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?

Comment 17

[Johnny Stenback (:jst)]

Attachment: Add security checks to non-predefined document properties.

Found a few problems with the original diff while sr'ing with rpotts.

Comment 18

[Johnny Stenback (:jst)]

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 19

[Johnny Stenback (:jst)]

Comment on attachment 88741 [details] [diff] [review]
Add security checks to non-predefined document properties.

Rick says sr=rpotts.

Comment 20

[Mitchell Stoltz (not reading bugmail)]

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.

r=mstoltz.

Comment 21

[Johnny Stenback (:jst)]

> 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 = window.foo;
  b = document.bar;
  c = window.baz;

since in that case accessing the document property would invalidate the cx and
wrapper that we cache when accessing the property window.foo, 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...  

Comment 22

[Johnny Stenback (:jst)]

Attachment: Less regression prone version for the branch.

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.

Comment 23

[Johnny Stenback (:jst)]

Attachment: Updated patch for the trunk.

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

Comment 24

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 89338 [details] [diff] [review]
Less regression prone version for the branch.

r=mstoltz for the branch

Comment 25

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 89361 [details] [diff] [review]
Updated patch for the trunk.

and likewise for the trunk

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 89338 [details] [diff] [review]
Less regression prone version for the branch.

rpotts says sr=rpotts

Comment 27

[chris hofmann]

Comment on attachment 89361 [details] [diff] [review]
Updated patch for the trunk.

a=chofmann for the trunk landing.

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 89338 [details] [diff] [review]
Less regression prone version for the branch.

Moving approval to the correct diff.

Comment 29

[Johnny Stenback (:jst)]

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

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

How is this patch doing on the trunk?

Comment 31

[Johnny Stenback (:jst)]

Doing good for all I know, no regressions reported so far.

Comment 32

[Asa Dotzler [:asa]]

Is this still wanted on the branch? we're done with 1.0.1.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

a=[e-mail] 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).

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK jst, please polish this off. Thanks.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 89361 [details] [diff] [review]
Updated patch for the trunk.

sr=roc+moz

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

renewal of a=rjesup for 1.0 branch

Comment 38

[Daniel Veditz [:dveditz]]

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?

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

From comment 29, I thought this HAS been checked into the trunk, but it's not on
the branch.

Comment 40

[Johnny Stenback (:jst)]

Attachment: Updated diff for the trunk

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.

Comment 41

[chris hofmann]

so is it time to close this bug?

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

from the perspective of embedding and distribution customers needing the fix on
a stable release the branch is important... 

Comment 42

[Michael Buckland]

Dan will talk to jst per adt mtg

Comment 43

[Daniel Veditz [:dveditz]]

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.

adt1.0.2-

Comment 44

[Daniel Veditz [:dveditz]]

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?

Comment 45

[Mitchell Stoltz (not reading bugmail)]

Good idea. I'm marking this fixed. Johnny, please open a new bug on whatever
cleanup work is necessary for the branch.