Closed Bug 133170 Opened 22 years ago Closed 22 years ago

XMLHttpRequest() fails when authentication needed and http redirect vulnerability

Categories

(Core :: XML, defect, P2)

x86
Windows 2000
defect

Tracking

()

VERIFIED FIXED
mozilla1.1alpha

People

(Reporter: titch, Assigned: hjtoi-bugzilla)

References

Details

Attachments

(4 files, 6 obsolete files)

2.02 KB, text/html
Details
19.02 KB, patch
hjtoi-bugzilla
: review+
jband_mozilla
: superreview+
chofmann
: approval+
Details | Diff | Splinter Review
18.07 KB, patch
hjtoi-bugzilla
: review+
jband_mozilla
: superreview+
Details | Diff | Splinter Review
28.82 KB, patch
security-bugs
: review+
security-bugs
: superreview+
Details | Diff | Splinter Review
XMLHttpRequest() currently has no way of specifying a username/password for use 
with a proxy. Maybe something like the solution for bug 122495 
http://bugzilla.mozilla.org/show_bug.cgi?id=122495 is needed.

This following Javascript code fragment, used from a chrome file, crashes with 
a warning:

function test() { 
  // Create a new http request
  var p = new XMLHttpRequest();
  p.open("GET", "http://ww.ireland.com", false);
  p.send(null);

  var d = p.responseText;
  dump("ResponseText: \n" + d + "\n");

  var s = p.status;
  dump("StatusText: \n" + s + "\n");	
	
  var h = p.getAllResponseHeaders();
  dump("ResponseHeaders: \n" + h + "\n");	
  return;
}

The following warning is displayed:
WARNING: notification callbacks should provide nsIAuthPrompt, file 
D:\mozilla\dev3\mozilla\netwerk\protocol\http\src\nsHttpChannel.cpp, line 1631
You can provide optional user and password parameters in the open() method.

What does IE do with your testcase?
Modified version of syncget.html so that it can be used by IE aswell as Moz
For testing I used the attached testcase.
I am behind a proxy which needs a username and password to get access.
I have set up my browsers not to go through the proxy when accessing localhost

BLANK: This means when I start up the browser with about:blank, so that it
doesn't ask me for my user/pass firstoff.

REMOTE: This means when I start up the browser with a remote webpage, so that it
asks me for user/pass before it will show it.

_IE6_
When starting from BLANK:
When asking for a localhost XML file it works fine.
When asking for a remote XML file, it pops up the dialog to enter user/pass and
then works fine.

When starting from REMOTE:
When asking for a localhost XML file it works fine.
When asking for a remote XML file it works fine - no popup.

_Moz 0.9.9_
When starting from BLANK:
When asking for a localhost XML file it works fine.
When asking for a remote XML file, it CRASHES:
"WARNING: notification callbacks should provide nsIAuthPrompt, file
D:\mozilla\dev3\mozilla\netwerk\protocol\http\src\nsHttpChannel.cpp, line 1631
warning: property cp1256 already exists"

When starting from REMOTE:
When asking for a localhost XML file it works fine.
When asking for a remote XML file it works fine - no popup.

Ok, I'll take your word for it. Confirming. We should definitely not crash.
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash
Hmm... I don't know how to test this. Any ideas?
Not really to be honest... but if you send on patches to me, I'll test them 
gladly. I would really like to see a fix for this bug if possible.
Priority: -- → P2
Target Milestone: --- → mozilla1.0.1
I am using Build 2002042908 on W2K and not able to make it crash. Instead, I
just see a response saying "you are not authorized" issued by the Proxy. The
same file in IE will prompt me for a username/password to get through the proxy.

For test, you can use an Apache 2 proxy I just set up at home. Configure Mozilla
to use the address of http://algonquin.tzo.com and the port of 8080.

username: test
password: mozilla

After I have authenticated to the proxy, then the file will work as expected
through Mozilla. You know if it is going through the proxy because there will be
a Via response header that looks something like this:
Via: 1.1 www.mozilla.org:8080

Note - my home connection is from a flaky DSL provider. Let me know if I should
change the port number.

Thanks, lowering priority and removing crash keyword.

I think I have an idea how to fix this now. Note to self: Need to have
XMLHttoRequest implement nsIHttpEventSink(?) and interface requestor and in
GetInterface respond to prompt IID. Dunno if who implements the prompt yet.
Severity: critical → major
Status: NEW → ASSIGNED
Keywords: crash
you just need to provide a nsIAuthPrompt implementation.  the windowwatcher
service can provide this.
Even after bug 141061 was fixed it is still possible to read stuff from http
URLs. I'll fix that as well as part of this bug.

Bringing back to 1.0.
Keywords: nsbeta1+
Whiteboard: [ADT1]
Target Milestone: mozilla1.0.1 → mozilla1.0
Ok, this seems to fix the security issue with XMLHttpRequest and redirects. It
also brings up the auth prompt, but my first test seemed to indicate the
connection stopped there. I need to do more testing.

Darin, how do I set up Apache to ask for username & password in some directory?
I think there was some .file that I could place in the directory, but I can't
remember (and not sure if I need to do something to enable that first).
Attachment #81907 - Attachment is obsolete: true
Mitch, how does this look?
changing QA contact
QA Contact: petersen → rakeshmishra
Ok, I confirmed that the prompt also works.

To test redirect, go to http://sec.greymagic.com/adv/gm001-ns/ and put
http://slashdot.org/users.pl?op=edituser as the demo URL, and hit Snif. You
should not get any results back (and you should get security error in JS console).

To test auth prompt, edit the get.html test in
mozilla/extensions/xmlextras/tests to point to http://green/heikki/login/ When
you open get.html you should get auth prompt, and if you answer "mozilla"  to
both user & password the prompt should go away and you should get the responseText.
QA Contact: rakeshmishra → petersen
Whiteboard: [ADT1] → [ADT1][fixinhand]
heikki:

you need to create a .htaccess file in the directory you want to protect:

here's a sample:

  AuthType Basic
  AuthName "basic-realm1"
  AuthUserFile /etc/httpd/passwd/basic-realm1
  Require valid-user

use the command htpasswd to create and populate the password file.

you probably also have to tweak your httpd.conf file to allow the use of
.htaccess files.  for example:

make sure the directory your interested has "AllowOverride AuthConfig" set.
Whiteboard: [ADT1][fixinhand] → [ADT1]
QA Contact: petersen → rakeshmishra
Here are some more tests:

1) XMLHttpRequest opens a connection to same host (green in this case), which
redirects it to mozilla.org

http://green.nscp.aoltw.net/heikki/133170/xmlhttp.html

2) Auth test, give "mozilla" as login & password

http://green.nscp.aoltw.net/heikki/133170/xmlhttpauth.html

3) document.load() with redirect

http://green.nscp.aoltw.net/heikki/133170/load.html

4) document.load() with auth

http://green.nscp.aoltw.net/heikki/133170/loadauth.html
QA Contact: rakeshmishra → petersen
Whiteboard: [ADT1] → [ADT1][fixinhand]
QA Contact: petersen → rakeshmishra
This patch also contains redirect check for document.load(), but for some
reason it passes the CheckConnect() call (it did not matter if I gave it |cx|
or |nsnull| as first param). I don't know why that does not work here; it works
fine for XMLHttpRequest.

If you try http://green.nscp.aoltw.net/heikki/133170/loadmozilla.html you will
see that the same check in document.load() blocks the call, so I see no reason
why it shouldn't work in redirect.
Attachment #82268 - Attachment is obsolete: true
Keywords: topembed
Log in to Bugzilla and W3C member only pages. Then try the following URLs. You
will notice this allows the attacker access to information he should not have.
This is possible because we save the basic auth information (W3C test) & cookies
that store login (Bugzilla test), so the attacker can use your identity to get
access. These will both naturally be fixed by disabling the redirect, as the
patch he will do.

http://green.nscp.aoltw.net/heikki/133170/xmlhttpbugzilla.html
http://green.nscp.aoltw.net/heikki/133170/xmlhttpw3cauth.html
Also please note that although the authentication prompt and the security fix
aren't directly related, adding the auth prompt can improve security by alerting
the user to the fact that something is trying to log him into a site he was not
trying to get into (the prompt displays URL & group on server it is trying to
log into). This prevents a hacker from using a timer based get (for example try
to get page every 5 minutes until succeed) on a page that uses basic
authentication, because the user would be peppered by these dialogs. Without
that part of the fix those calls would just silently fail if auth information
was not available. IE6 obviously supports that auth dialog as well.
Michael, we still have the issue with http redirecting to another http URL,
which will be fixed by the patch here.
Summary: XMLHttpRequest() fails when authentication needed → XMLHttpRequest() fails when authentication needed and http redirect vulnerability
Whiteboard: [ADT1][fixinhand] → [ADT1][fixinhand][m5+]
Need this for RC2. 
Blocks: 138000
Bummer, Mitch realized that the approach I have does not work for asynchronous
case (document.load is async.). He has an idea how to make it work, though. Here
is a test for asynchronous XMLHttpRequest:

http://green.nscp.aoltw.net/heikki/133170/xmlhttpasync.html
Whiteboard: [ADT1][fixinhand][m5+] → [ADT1][m5+]
Ray, it looks like we will need to change the security checks for XMLHttpRequest
a little to fix this problem. We shouldn't affect SOAP, but I wanted to let you
know just in case...
This patch was done against the 1.0 branch, but it should apply to the trunk as
well. It includes Heikki's earlier patch but adds a new security check function
which correctly handles both synchronous and asnchronous cases. This still
needs testing - we should test cross-site access with privileges, synchronous
and asynchronous.

r=mstoltz on Heikki's part of the patch, Heikki, can you review the rest?
Attachment #82306 - Attachment is obsolete: true
The same patch, ported to the 6.2.1 branch. Appears to pass Heikki's testcases
in comment 17.
Comment on attachment 82755 [details] [diff] [review]
Equivalent patch for NS6.2.1 branch

+    if (!sourcePrincipal ||
+	 NS_SUCCEEDED(sourcePrincipal->Equals(mSystemPrincipal, &equals))
+		      && equals)

I'd like to see some grouping parens to make the intent clearer.

This stuff looks reasonable to me. I'd like to see some r= from 
qualified reviewers before this goes in. I'd also like some
assurance that the tests cover the range of cases that might
include possible regressions - redirects that suddenly fail
or unnecessary re-auth dialogs where they had not previously 
been, etc.

Given that, sr=jband
Attachment #82755 - Flags: superreview+
heikki - if you need add'l tests, pls email allclientqa@netscape.com.  Due to
some time constraints and people working different hours, I want to ensure that
you can get the widest range of help in addition to the Mozilla community
helping.  Thanks.
Comment on attachment 82728 [details] [diff] [review]
New patch for Moz 1.0 branch, fixes async case

a=chofmann for the 1.0 branch.
Attachment #82728 - Flags: approval+
Comment on attachment 82728 [details] [diff] [review]
New patch for Moz 1.0 branch, fixes async case

sr=jband With the same comments as on the other patch.
Attachment #82728 - Flags: superreview+
Comment on attachment 82728 [details] [diff] [review]
New patch for Moz 1.0 branch, fixes async case

I second jband's plea to add some parenthesis to make the grouping clear in
nsScriptSecurityManager::CheckSameOrigin().

There is also a small bloat issue: make mCrossSiteAccessEnabled be of type
PRPackedBool, and move it next to the other packed booleans.

r=heikki
Attachment #82728 - Flags: review+
Comment on attachment 82755 [details] [diff] [review]
Equivalent patch for NS6.2.1 branch

With same nits r=heikki

Btw, I should note that caps is strange territory for me, but what I
undrerstood seemed good.
Attachment #82755 - Flags: review+
Tests to run:

See comments #17, #18, #19, #23 (or just go to
http://green.nscp.aoltw.net/heikki/133170/ and try them all).

As far as regression tests go:

Online tests are at: http://www.mozilla.org/xmlextras/tests.html

Additional tests at extensions/xmlextras/tests/ (try all the post tests, SOAP
tests, and if you get any SOAP tests from the Evangelism team/Ray Whitmer).
There are also some tests in content/xml/tests/load/.

Due to personal circumstances I can not do much to help right now, but Mitch
should be able to finish the checkins etc. 
Keywords: adt1.0.0, approval
Whiteboard: [ADT1][m5+] → [ADT1][m5+] [ETA 05/08] [Needs a=]
adding adt1.0.0+.  Mitch, could you check this into the branch tonight?
Keywords: adt1.0.0adt1.0.0+
Bonsai shows a checkin from Mitch 5/7 23:27, updating keyword foo.
Keywords: fixed1.0.0
Whiteboard: [ADT1][m5+] [ETA 05/08] [Needs a=] → [ADT1][m5+]
Trying to verify on the branch build 2002-05-08-08-1.0.0. As per comment# 15
,going to http://sec.greymagic.com/adv/gm001-ns/ and putting
http://slashdot.org/users.pl?op=edituser as demo URL and hitting Sniff. 
I get as result "File not found" and I do not get any security error in th JS
console.Is this how it should behave .
verified on the branch build 2002-05-08-08-1.0.0 on Windows XP , with the URLs
under comment #36 . Adding the keyword verified 1.0.0.
Keywords: verified1.0.0
did this hit the trunk yet?  let's get it there and mark this fixed.
thanks
Verified on the 6.2.3 branch build 2002-05-08-14-6.2.3 on Windows 2000 , with
the test cases under comment # 36.
Work fine.
Any reason for the lack of approval of the 6.2.1 base patch? It's been a few
days.

I'm not pushing anyone, just a concerned Netscape 6.2.2 user waiting for a
patch. :)

Could this comment about documen.load() be related? He says it no longer passes 
cookies. http://bugzilla.mozilla.org/show_bug.cgi?id=136891#c21
Fernando: A patch to 6.2.2 is forthcoming. "Has-Approval" is irrelevant; that
only applies to Mozilla. If you're concerned in the meantime, disable
JavaScript, at least when you visit any sites you don't trust.
Keywords: topembedtopembed+
Keywords: nsbeta1+
Whiteboard: [ADT1][m5+]
Target Milestone: mozilla1.0 → mozilla1.1alpha
OK, here's the patch for the trunk with some changes in caps. I have refactored
code to avoid doing unnecessary codebase principal creation, and to generate
better exceptions / console messages with localization. This will be checked in
along with the earlier 1.0 branch patch in this bug, and the patches from bug
113351 and bug 147754.
Comment on attachment 86700 [details] [diff] [review]
New patch for the trunk (the caps part)

>Index: caps/idl/nsIScriptSecurityManager.idl
>===================================================================
>Index: caps/include/nsScriptSecurityManager.h
>===================================================================
>+    SecurityCompareURIs(nsIURI* aSourceURI,
>+                        nsIURI* aTargetURI,
>+                        PRBool* result);
>+

This name is not very descriptive. CompareProtocolHostPort() would
describe what it does although the name is not "pretty".

>Index: caps/src/caps.properties
>===================================================================
>+CheckLoadURIError = The link to %s was blocked by the security manager. Remote content may not link to local content.

I don't think it is necessarily local content, for example if you have a link
pointing to imap://...?

>Index: caps/src/nsCertificatePrincipal.cpp
>===================================================================
>Index: caps/src/nsCodebasePrincipal.cpp
>===================================================================
> nsCodebasePrincipal::Equals(nsIPrincipal *other, PRBool *result)

In general the style is to have parameters start with 'a'.

>Index: caps/src/nsScriptSecurityManager.cpp
>===================================================================
>+nsScriptSecurityManager::SecurityCompareURIs(nsIURI* aSourceURI,
>+                                             nsIURI* aTargetURI,
>+                                             PRBool* result)
>+{
>+    nsresult rv;

Move rv down to where you use it for the first time.

It would also be a good idea to set result to false in the beginning.

>+    if (aSourceURI == aTargetURI)
>+	{

Indentation...

>+    if (aTargetURI == nsnull) 
>+        // return false
>+        return NS_OK;

In general this is dangerous, because it is easy to later change this
code so that it would require braces without noticing that.

>+    if (!sourceBaseURI || !targetBaseURI)
>+        return NS_ERROR_FAILURE;

You did not set result. Would be fixed by initializing it in the beginning.

>+            *result = targetSpec.Equals(sourceSpec);
>+        } 
>+		else
>+		{

Indentation seems weird again. Also you could consider changing the
whole file to 2 space indents (would help reading code like this
in hear in Emacs etc.)

I didn't have time to read the rest carefully, so just some brief observations:

* ReportError second argument is PRUnichar, check if nsAString would work.
* ReportError checks the localization string format for correct number of
  %-characters. I think this is the only code that does that. How about
  making some new functions into string bundle, or modifying the existing
  methods, to do these checks? That way everyone could get the benefits of
  this work.
Attached patch caps patch for the trunk v2 (obsolete) — Splinter Review
OK, I addressed some of Heikki's concerns and also changed the return values
from CheckSameOrigin and CheckSameOriginURI - I was forgetting to return an
error when the check failed (whoops).
- I want to keep the name of SecurityCompareURIs as it is, because there's no
reason callers need to know the exact nature of the check (that it compares
scheme, host, and port). It's a URL comparison for security, that's what's
important.
- Style point: I like to declare |nsresult rv| at the top of a function so that
I don't have to move the declaration around every time I use rv in a different
place.
- Style point: I like 4-space indents better, and this whole module is written
that way, so I don't want to change it now. I did weed out a couple of hard
tabs that were still lurking in the file.
- The second parameter of ReportError could be nsAString, but that would be
harder to use, since I need to pass it to a function that takes PRUnichar*, and
I don't think you can do a .get() on an nsAString. I think the current setup is
simpler, even though it doesn't use the nsString classes.
Attachment #86700 - Attachment is obsolete: true
Comment on attachment 86898 [details] [diff] [review]
caps patch for the trunk v2

+nsCodebasePrincipal::Equals(nsIPrincipal *aOther, PRBool *result)
 {
-    // Equals is defined as object equality or same origin
+    nsresult rv;
     *result = PR_FALSE;
-    if (this == other) 
-	{
-	 *result = PR_TRUE;
-	 return NS_OK;
-    }
...

Are you sure you want to loose this fast path optimization, I know you're doing
a similar optimization further down when you have the URI's, how commonly would
this case arise? I would suggest leaving it there, but move it above the
assignment of PR_FALSE into *result. And I don't see rv being needed here, the
one place where it's used you can just do a nsCOMPtr null check.

- In nsScriptSecurityManager::SecurityCompareURIs(), are we guaranteed that all
URI's that are ever passed to this method are normalized so that the protocol
is lower case n' so on?

- Nit of the day:

+		 if(!*result && (sourcePort == -1 || targetPort == -1))

Add a space after the "if" for consistency with the rest of the code.

- In nsScriptSecurityManager::ReportError():

+    while (p != end)
+    {
+	 if (*p == '%')
+	 {
+	     ++p;
+	     if (foundPercent || *p != 's')

Accessing *p here w/o checking if p != end after the increment is not safe.

Also:

+    PRUnichar* message = 
+	 nsTextFormatter::smprintf(msgText, spec.get());
...
+	 NS_ENSURE_TRUE(console, NS_ERROR_FAILURE);
...
+    nsTextFormatter::smprintf_free(message);

The NS_ENSURE_TRUE(...) would cause |message| to be leaked. There's also a leak
inside the #ifdef DEBUG block, which doesn't really matter, but should still be
fixed...

Attach a new patch with those issues and I'll have one last look...
Attachment #86898 - Flags: needs-work+
Attached patch Caps patch for the trunk v3 (obsolete) — Splinter Review
OK, I've addressed those issues.
Attachment #86898 - Attachment is obsolete: true
Comment on attachment 87146 [details] [diff] [review]
Caps patch for the trunk v3

+	     ++p;
+	     if ((p != end) && (foundPercent || *p != 's'))
+	     {
+		 NS_ERROR("Malformed format string in error message
localization");
+		 return NS_ERROR_FAILURE;
+	     }

Actually, don't you want that to be:

  if (p == end || foundPercent || *p != s)

This way you'll catch invalid strings that end with a '%' character.

sr=jst if you make the above change, assuming you agree with the above.
Attachment #87146 - Flags: superreview+
> +/* Static function for comparing two URLs - for security purposes,
> + * two URLs are equivalent if their scheme, host, and port are equal.

"URIs" instead of "URLs"?

> +   if (NS_SUCCEEDED(rv) && targetScheme.EqualsIgnoreCase(sourceScheme.get())) 

targetScheme.Equals(sourceScheme, nsCaseInsensitiveCStringComparator()),
please.  EqualsIgnoreCase is supposed to go away, iirc.

> +        if (targetScheme.Equals("file"))

Should that not be a case-insensitive comparison?

> +        else if (targetScheme.EqualsIgnoreCase("imap")    ||
> +                 targetScheme.EqualsIgnoreCase("mailbox") ||
> +                 targetScheme.EqualsIgnoreCase("news"))

This may benefit from NS_LITERAL_CSTRING and using a comparator (though the perf
difference is likely not very big, if any).

> +            *result = targetSpec.Equals(sourceSpec);

Are mail URIs case-sensitive?

> + *result = NS_SUCCEEDED(rv) && targetHost.EqualsIgnoreCase(sourceHost.get());

Again, probably better to use a comparator.

> +        rv = ioService->GetProtocolHandler(sourceScheme.get(),
> +                                           getter_AddRefs(protocolHandler));
> +        if (NS_FAILED(rv))
> +        return rv;

Do we really want to fail in this case?  Or return a PR_FALSE for the check and
succeed?

> +    PRBool equals = PR_FALSE;

How about naming this "systemPrincipal" or something descriptive like that?

> +ReportError(cx, NS_LITERAL_STRING("CheckSameOriginError").get(), aTargetURI);

It would be nice to report the sourceURI here as well.....

> +    rv = bundle->GetStringFromName(messageTag, getter_Copies(msgText));

Is there a reason not to use FormatStringFromName here?

Thanks Boris, I took your suggestions. Please give me your final review ASAP.
Attachment #87146 - Attachment is obsolete: true
Comment on attachment 87428 [details] [diff] [review]
Caps patch for the trunk v4

> +    if (aSourceURI == aTargetURI)
> +	{
> +        *result = PR_TRUE;

The indentation on the "{" looks like a tab...

> +    PRBool equals = PR_FALSE;
> +    rv = sourcePrincipal->Equals(mSystemPrincipal, &equals);

Again, that bool could have a better name like |isSystemPrincipal|.

> +const PRUnichar *formatStrings[] = { NS_ConvertASCIItoUCS2(sourceSpec).get(),
> +                                     NS_ConvertASCIItoUCS2(targetSpec).get() };

This is no good.  You're saving pointers to temporaries that go away right
after this line.  You want to do something like:

ucsSourceSpec NS_ConvertASCIItoUCS2(sourceSpec);
ucsTargetSpec NS_ConvertASCIItoUCS2(targetSpec);
const PRUnichar *formatStrings[] = { ucsSourceSpec.get(), ucsTargetSpec.get()
};

Fix those issues, and r=bzbarsky.
Attachment #87428 - Flags: needs-work+
Comment on attachment 87428 [details] [diff] [review]
Caps patch for the trunk v4

OK, carrying over r and sr
Attachment #87428 - Flags: superreview+
Attachment #87428 - Flags: needs-work+
Attachment #87428 - Flags: approval+
Attachment #87428 - Flags: approval+ → review+
Fix checked in on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Verified fixed on the trunk build 2002-12-11-08-trunk on Windows 2000 , with
the test cases under comment # 36.

marking verified 

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: