306261 - loading unsafe about: URLs using HTML forms

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[shutdown]

Spin off from bug 300830 comment 15.
EqualsLiteral for distinguishing "about safe" can be security problem.
http://lxr.mozilla.org/mozilla/source/caps/src/nsScriptSecurityManager.cpp#1177

See the URL field. It will load "about:blank?" which is unsafe about: URL.
Once unsafe about: URL is laoded, an attacker can load
"about:plugins" and abuse its privileges by XSS attacks.

Comment 1

[Daniel Veditz [:dveditz]]

Confirming that an unsafe about:blank? (and others) can be loaded this way.
Combined with another XSS/same-origin/checkLoadURI bug this could lead to
arbitrary execution.

Comment 2

[Brendan Eich [:brendan]]

Who is going to patch this?

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

First question is what the real problem is.

The immediate issue problem is that "about:blank?" is not considered an "about
safe" URI but that untrusted content can still load it, right?  So the two
options are to make it "about safe" or to prevent content from loading it.

The latter looks more correct to me.  Is there any way other than a form
submission to cause this load to happen?  It looks like forms do a CheckLoadURI
check when creating the action URL, but not after they've appended the various
form params.  Perhaps we should be doing this check on the actual URL we plan to
load?

Comment 4

[Boris Zbarsky [:bzbarsky]]

So I'm leaning towards just changing nsFormSubmission to use nsIURL::SetQuery to
set the query string instead of calling SetPath and manually dealing with
fragment identifiers and so on.

Note that this DOES mean that it will no longer be possible to send query params
with a GET to a non-nsIURL URI.  Which seems ok to me, really...  except I guess
it does make it impossible to get with a query to schemes we don't recognize.  :(

Perhaps we should leave the current code for that case and do nsIURL for every
scheme that we do recognize?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #4)
> Perhaps we should leave the current code for that case and do nsIURL for every
> scheme that we do recognize?

what was wrong with calling CheckLoadURI on the right URI? :)

I'm not sure that special-casing external protocols in nsFormSubmission is a
good idea...

Comment 6

[Boris Zbarsky [:bzbarsky]]

> what was wrong with calling CheckLoadURI on the right URI? :)

Nothing offhand, I guess; just have to sort out the many form submission
codepaths once you get into the different submission types and see which ones to
call in.  That' and the fact that the current code screws up if a URI has a ';'
in it (that part won't get submitted, and arguably it should be).  Using
SetQuery would get around that.

Comment 7

[Daniel Veditz [:dveditz]]

Nominating for 1.0.7 -- if we don't fix the split-window issues in this branch
release then this potentially could be combined with one of those into an
arbitrary execution exploit.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: what about this approach?

This doesn't fix the bug as reported but I'd think it ought to fix the security
problem.  That said, I don't really see a way to test the security problem
provided here.	This makes the check in nsScriptSecurityManager match the way
nsAboutProtocolHandler chooses which about module to use.  It seems easier to
copy the code than to find a way to share it, although that is a little
dangerous since it could get out of sync.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If you see a good way of sharing that function (plus the nsIURI::GetPath call
thet precedes it), let me know...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195933 [details] [diff] [review]
what about this approach?

sr=bzbarky, but for trunk I think we should (additionaly):

1)  Move a lot of the about stuff in the script security manager into the about
protocol handler and expose it via an interface.
2)  Fix form submission to use nsIURL when it can, so that it doesn't screw up
except in cases where it can't avoid it (needs followup bug).

Comment 11

[Darin Fisher]

Comment on attachment 195933 [details] [diff] [review]
what about this approach?

>Index: caps/src/nsScriptSecurityManager.cpp

>+// This needs to stay in sync with the function ConvertAboutPathToModule
>+// in nsScriptSecurityManager.cpp.
>+static void
>+ConvertAboutPathToModule(nsCString& aPath)

That comment is wrong.	I think you meant to refer to the
nsAboutProtocolHandler code, right?


>+{
>+    PRInt32 f = aPath.FindCharInSet(NS_LITERAL_CSTRING("#?"));
>+    if (f != kNotFound) {
>+        aPath.Truncate(f);
>+    }
>+
>+    // convert to lowercase, as all about: modules are lowercase
>+    ToLowerCase(aPath);
>+}

A function like this could be defined in a header file if you
wanted to share the code.


>+        // This needs to stay in sync with what
>+        // nsAboutProtocolHandler::NewChannel does.
>+        ConvertAboutPathToModule(path);
>         if (path.EqualsLiteral("blank")   ||
>             path.EqualsLiteral("mozilla") ||
>             path.EqualsLiteral("logo")    ||
>             path.EqualsLiteral("license") ||
>             path.EqualsLiteral("licence") ||
>             path.EqualsLiteral("credits") ||
>-            Substring(path,0,9).EqualsLiteral("neterror?"))
>+            path.EqualsLiteral("neterror"))
>         {
>             aScheme = NS_LITERAL_CSTRING("about safe");
>             return NS_OK;
>         }

perhaps nsAboutProtocolHandler could implement an interface
that would expose a method that would run through similar
code.  that way, you'd be able to avoid duplicating logic
like this.


r=darin

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: using shared header

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hrm, maybe NS_AboutURIToModuleName instead of NS_AboutURIToAboutModule?

Comment 14

[Darin Fisher]

Comment on attachment 195946 [details] [diff] [review]
using shared header

>Index: netwerk/protocol/about/public/nsAboutProtocolUtils.h

>+ * The Original Code is __________________________________________.

oops


>+NS_AboutURIToAboutModule(nsIURI *aAboutURI, nsCString& aModule)

Hmm.. how about NS_GetAboutModuleName?


r=darin

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

should NS_AboutURIToAboutModule assert that the scheme is about?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: with name change

Comment 17

[Daniel Veditz [:dveditz]]

Attachment: 2-line aviary branch patch

1.8 has a problem that the about redirector didn't match the caps logic, thus
we need dbaron's patch. But in the old days the about redirector didn't allow
query strings either, and caps matched it. Except in the about:blank case,
which has its own logic and didn't prevent about:blank? (and perhaps was even
done on purpose, maybe for debugging forms during webpage development?).

Since web pages *can* in fact load about:blank? through forms let's tell caps
about them. Preventing about:blank? from loading through form submits would be
far riskier.

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 195991 [details] [diff] [review]
2-line aviary branch patch

shaver says "sr=me! *hic* (dirty martini gulp)".

/be

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 195946 [details] [diff] [review]
using shared header

We're going with the smaller patch for the old branches

Comment 20

[Darin Fisher]

I'm happy with the 2-liner FWIW for the old branches.  Let's go with dbaron's
latest patch on the trunk.

Comment 21

[Daniel Veditz [:dveditz]]

Fix checked into aviary and 1.7 branches

Comment 22

[Simon Fraser [no longer active]]

Camino uses about:bookmarks and about:history. Does anything need to change there?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Did this ever land on trunk too?  And on 1.8?  That is, can we resolve this?

Comment 24

[Daniel C]

(In reply to comment #23)
> Did this ever land on trunk too?  And on 1.8?  That is, can we resolve this?
> 

Ping. Can anyone answer?

Comment 25

[Daniel Veditz [:dveditz]]

This landed on the trunk on 2005-09-13