154930 - document.domain abused to access hosts behind firewall

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Daniel Veditz [:dveditz]]

Spun off from bug 149943 comment #4 and independently reported by Adam Megacz of
XWT. See that comment and the advisory at the URL above (also stored as
attachment 89639 [details]).

Comment 1

[Daniel Veditz [:dveditz]]

Additional comments on this issue from bug 149943:

------- Additional Comment #42 From Georgi Guninski  2002-06-29 02:13 -------

Created an attachment (id=89676)
Testcases for malicous use of document.domain with malicous DNS server

This needs modification in a DNS server or /etc/hosts. Check comments in the
attachment.


------- Additional Comment #43 From Georgi Guninski 2002-06-29 02:52 -------

In the testcase above and for the suggested patch:
local.mall.xx == 1.1.1.1 (server somewhere in the internet)
mall.xx == 10.10.10.10 (web server in the intranet)

Adam suggest in comment #41 that users who don't have DNS and their proxies
serve the intranet are vulnerable no matter what IP/name checks the browser
makes. I believe in the above case almost nothing can be done undless a change
in the web server/proxy (though I am *not* dns expert).
But at least we should protect for other attacks.

What the browser should do is protect systems which have dns or at least use
/etc/hosts (I strongly doubt it a lot of users access web applications with URLs
like http://10.10.10.10/cgi-bin/report.cgi - at least they have an alias in
/etc/hosts)

My idea is the following:
A check should be made when assigning to document.domain.
An exploit condition arises when document.domain is being tried to be assigned
to a value which have an IP, i.e. there is a host at the value.
The main problem is that the DNS mall.xx claims that mall.xx == 10.10.10.10
because the DNS has control over mall.xx
But the DNS doesn't have control over the 10.10.10.10 == ?? (the reverse lookup)
So the browser asks for the name of 10.10.10.10 a trusted DNS
Local DNS or /etc/hosts returns "intranetserver1"
Obiously intranetserver1 != mall.xx so something suspicous is going on.


Possible problems:
1. 10.10.10.10 may not have a name. I strongly doubt a user will use a local web
server by its IP and in this case use of document.domain is illegal but who knows?
2. This may get tricky if a web server is served by different machines with
different IPs (is this "load balancing") ? Probably in this case the IPs shall
be in some range and this case is probably not so common behind firewalls.

Another possibility is adding a preference for this check.

Comment 2

[Adam Megacz]

There has been an update to the advisory; IE5+ can be used to attack
SOAP/XML-RPC Web Services as well. Does Moz have anything similar to XMLHTTP? If
so, it may be vulnerable as well.

http://www.xwt.org/sop.txt

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch - restrict use of document.domain

This is the fix we discussed - if the subject page has set document.domain and
the target page has not, deny access to properties even if the domains match.

For XMLHttpRequest and document.load, which call CheckSameOrigin, this patch
will ignore any setting of document.domain and use the original host-specific
subject principal instead.

Comment 4

[Bob Clary [:bc] (inactive)]

I applied the patch and am running the tests at
<http://mozilla-evangelism.bclary.com/evang500/test.html> for the top 500 list.
This allows you to visit each of the links on each home page. For those of you
with fast connections you can set the page interval to a reasonably small
interval and proceed through the pages faster. I will let you know if anything
jumps out in the javascript console but it will take some time over a dialup
connection.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 90142 [details] [diff] [review]
Patch - restrict use of document.domain

>     //-- If this is the first codebase set, remember it
>+    //   If not, remember that the codebase has been changed

might want to change the comment to "...has been explicitly set" since there
might be pages on "foo.com" cooperating with "sub.foo.com", that would now have
to set domain to "foo.com" even though that was the default (setting, but not
changing it).

>+    {   // If the subject has changed its principal by setting document.domain
>+        // then the object's page must also have done so in order to be considered
>+        // the same origin. This prevents DNS spoofing based on document.domain 

You've implemented "if subject is set, target must be set"; an alternate would
be "if either is set, both must be set", that is, check both every time. The
first blocks the current known attack, are we sure there is no attack going the
other way?

So you have a page on foo.com that thinks it's writing into another page on
foo.com, but really it's evil.foo.com with document.domain="foo.com". That
would probably require that the DNS for foo.com is controlled by someone at
odds with the site author, who doesn't otherwise have the ability to just go
and change the foo.com page. Seems unlikely, but wanted to raise it. (the known
attack is reading the victim's page; in the unchecked scenario it would
probably be an innocent logged-on page fooled into writing info into the
attacker's page.)

Your patch is faster if we're not worried about the other case.

r=dveditz

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 90142 [details] [diff] [review]
Patch - restrict use of document.domain

nit: use the same style as the other member variables:

     nsCOMPtr<nsIPrincipal> mOriginalCodebase;
+    PRBool codebaseWasChanged;

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 90142 [details] [diff] [review]
Patch - restrict use of document.domain

+    // Get the original URI from the source principal
+    // This has the effect of ignoring any change to document.domain
+    // which must be done to avoid DNS spooing (bug 154930)

Typo in "spooing", and add a period after "principal".

sr=jst if you fix that and the other issues mentioned in this bug.

Comment 8

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch v2.0 - addresses review comments

I went ahead and used Dan's suggestion of checking both the subject and the
object for explicit domain-setting every time. I think the preformance impact
will be minimal, but we'll see.

Comment 9

[Adam Megacz]

Does this prevent the attack in the section of the advisory titled "Quick-Swap
DNS"? With this patch, I believe you can still do it by simply not bothering to
use document.domain. Perhaps that merits another bug report.

I have also realized that you can set the timeout for a DNS record to 0 (no
caching allowed). If the attacker does this, he knows that the first incoming
DNS request is the attacker page load (so it should return the IP of the
attacker's server), and the second incoming request is the intranet page load
(so it should return 10.x.x.x).

This eliminates the need to wait for a timeout to expire before loading the
intranet page, making the Quick-Swap DNS attack just as effective as the
original document.domain attack.

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Adam,
   I believe bug 149943 addresses "Quick-Swap DNS" by "pinning" a hostname to
the first IP address received for that host for the duration of a browser
session. Please tell me if you disagree.

Meanwhile, I have checked in patch v2.0 to the trunk. Marking Fixed.

Comment 11

[Adam Megacz]

> I believe bug 149943 addresses "Quick-Swap DNS" by "pinning" a hostname to
> the first IP address received for that host for the duration of a browser
> session. Please tell me if you disagree.

How is this implemented for clients behind proxies (no DNS access)?

Comment 12

[Daniel Veditz [:dveditz]]

If you're using proxies every address is the address of the proxy--there's not
much we can do about that. If you're going to configure a complex network
configuration then security has to be built into that configuration as a whole,
not just one part of it such as the browser. For example, many firewalls can be
configured to reject responses from external DNS servers that claim "internal"
addresses.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 90552 [details] [diff] [review]
Patch v2.0 - addresses review comments

>+        // If both or neither explicitly set their domain, allow the access
>+        if (!(subjectSetDomain || objectSetDomain) ||
>+            (subjectSetDomain && objectSetDomain))
>+            return NS_OK;

You probably could have gotten away with "if ( subjectSetDomain !=
objectSetDomain )"

r=dveditz

Comment 14

[Adam Megacz]

Agreed; this is not Mozilla's fault.

However, I think that this vulnerability dramatically changes the security
architecture of the Internet. Running a proxy without DNS is now an insecure
configuration.

A lot of people have set up networks assuming that that is not the case.

Comment 15

[georgi - hopefully not receiving bugspam]

I don't see what Mozilla can do if it has no knowledge of DNS.
People who use such DNSless configurations may consider reconfiguring the proxy
so it doesn't serve intranet pages or configure their intranets web servers so
if the HTTP Host: field do not match the actual web server name the request is
not fullfilled.

Comment 16

[Mitchell Stoltz (not reading bugmail)]

dougt, dveditz, and bsharma have verified this fix using the DNS that Ben set
up. We're going to make a testcase for general use inside Netscape, but in the
meantime I'm marking this Verified so we can move on.

Comment 17

[scottputterman]

adding adt1.0.1+.  Please get drivers approval before checking into the branch.

Comment 18

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 19

[scottputterman]

Is this waiting for 149943 in order to land?

Comment 20

[Mitchell Stoltz (not reading bugmail)]

Actually, this depends on changes made for bug 152725.

Comment 21

[Mitchell Stoltz (not reading bugmail)]

Fixed on branch.

Comment 22

[bsharma]

Verified on 2002-07-25-branch build on Win 2000

An exception is thrown in the JS console.

Worked with dougt on this.

Comment 23

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch for 0.9.4 branch

Comment 24

[Daniel Veditz [:dveditz]]

Attachment: make extra document.domain checks depend on pref (1.0 branch only)

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 95931 [details] [diff] [review]
make extra document.domain checks depend on pref (1.0 branch only)

I moved this patch from evangelism bug 160339, where it got r=syd.

This fix broke a few more major sites than predicted, this patch adds a
preference setting for users or their admins to choose between exploitable
compatibility or safety. This is for the near-term 1.0 branch only, 1.1 and the
trunk will continue on in the safer configuration.

When MS releases their fix the evangelism on the issue will get a lot easier;
meanwhile administrators will have to do something to protect against all the
other non-fixed browsers.

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 95931 [details] [diff] [review]
make extra document.domain checks depend on pref (1.0 branch only)

@@ -670,7 +670,12 @@
	 return NS_ERROR_FAILURE;

     if (isSameOrigin)
-    {	 // If either the subject or the object has changed its principal by
+    {
+	 // isSameOrigin is all we need if we're not doing strict checking
+	 if ( !mStrictDomainCheckEnabled )
+	     return NS_OK;

Nit-of-the-day: Loose the spaces directly inside the parens here to match the
other code in this file.

- In all.js:

+// Strict document.domain checking closes a potential security hole through
+// which sites behind a firewall can be reached (see bug 154930). Turning this
+// stricter checking on currently breaks some popular sites (see bug 160339).
+pref("javascript.strict_domain_checking",   false);

I'm not sure we want to put references to the bugs in the comment that'll be in
everybody's all.js files, but given that this bug is not marked as
Security-sensitive I guess it doesn't matter that much. If nobody else has
strong feelings about this, I'm ok either way.

sr=jst

Comment 27

[Christopher Blizzard (:blizzard)]

Comment on attachment 95931 [details] [diff] [review]
make extra document.domain checks depend on pref (1.0 branch only)

a=blizzard for the 1.0 branch, as discussed in email.

Comment 28

[scottputterman]

this has adt approval as well. thanks for adding the pref.

Comment 29

[Daniel Veditz [:dveditz]]

The 1.0 branch now has the original fix effectively backed out but it can be
re-enabled by those who want it by setting the pref
"javscript.strict_domain_checking" to true. The fix will remain in 1.1 and trunk
builds.

Comment 30

[Steven Grimm]

I filed bug 183143 on this subject.  I think there's a more nuanced way to get
the security benefits of this fix without breaking standards compliance or
forcing site authors to set their document domains on zillions of otherwise
non-Javascripty pages.