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

RESOLVED FIXED

Status

()

Core
Security: CAPS
P2
normal
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: jst)

Tracking

({csectype-sop})

Trunk
x86
Windows NT
csectype-sop
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ADT2 RTM][HAVE FIX][sg:branch])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
Created attachment 42260 [details]
testcase
OK, but what can you actually do with those properties you can access? What
information can you steal?
(Reporter)

Comment 3

17 years ago
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

17 years ago
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?
(Reporter)

Comment 5

17 years ago
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.
Status: NEW → ASSIGNED
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
Mozilla1.1
Target Milestone: mozilla1.0 → mozilla1.1
jst has agreed to look into this.
Assignee: mstoltz → jst
Status: ASSIGNED → NEW
Keywords: mozilla1.0.1, nsbeta1
Whiteboard: [ADT2 RTM]
Target Milestone: mozilla1.1alpha → mozilla1.0.1
(Assignee)

Comment 13

16 years ago
Created attachment 88722 [details] [diff] [review]
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.
(Assignee)

Comment 14

16 years ago
Mitch, please review.
(Assignee)

Comment 15

16 years ago
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...
Status: NEW → ASSIGNED
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?
(Assignee)

Comment 17

16 years ago
Created attachment 88741 [details] [diff] [review]
Add security checks to non-predefined document properties.

Found a few problems with the original diff while sr'ing with rpotts.
Attachment #88722 - Attachment is obsolete: true
(Assignee)

Comment 18

16 years ago
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.
(Assignee)

Comment 19

16 years ago
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.

r=mstoltz.
(Assignee)

Comment 21

16 years ago
> 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...  
(Assignee)

Comment 22

16 years ago
Created attachment 89338 [details] [diff] [review]
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.
(Assignee)

Comment 23

16 years ago
Created attachment 89361 [details] [diff] [review]
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().
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+
(Assignee)

Comment 26

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

rpotts says sr=rpotts
Attachment #89338 - Flags: superreview+

Comment 27

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

a=chofmann for the trunk landing.
Attachment #89361 - Flags: approval+
(Assignee)

Comment 28

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

Moving approval to the correct diff.
Attachment #89338 - Flags: approval+
(Assignee)

Updated

16 years ago
Attachment #89361 - Flags: approval+
(Assignee)

Comment 29

16 years ago
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.
How is this patch doing on the trunk?
(Assignee)

Comment 31

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

Comment 32

16 years ago
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.
a=rjesup@wgate.com 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).
Keywords: mozilla1.0.1 → mozilla1.0.2+
OK jst, please polish this off. Thanks.
Comment on attachment 89361 [details] [diff] [review]
Updated patch for the trunk.

sr=roc+moz
Attachment #89361 - Flags: superreview+
Keywords: patch
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.
(Assignee)

Comment 40

16 years ago
Created attachment 104576 [details] [diff] [review]
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

16 years ago
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

16 years ago
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.

adt1.0.2-
Keywords: adt1.0.2 → adt1.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?
(Assignee)

Updated

16 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Group: security
(Reporter)

Updated

5 years ago
Keywords: csec-sop
You need to log in before you can comment on or make changes to this bug.