Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 306261 - loading unsafe about: URLs using HTML forms
: loading unsafe about: URLs using HTML forms
: fixed-aviary1.0.7, fixed1.7.12, fixed1.8
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
: David Keeler [:keeler] (use needinfo?)
data:text/html,<form action="about:bl...
Depends on:
Blocks: sbb?
  Show dependency treegraph
Reported: 2005-08-28 13:13 PDT by shutdown
Modified: 2006-03-12 18:52 PST (History)
17 users (show)
dveditz: blocking1.7.12+
dveditz: blocking‑aviary1.0.7+
cbeard: blocking1.8b5+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

what about this approach? (6.03 KB, patch)
2005-09-13 13:24 PDT, David Baron :dbaron: ⌚️UTC-7
darin.moz: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
using shared header (12.11 KB, patch)
2005-09-13 14:17 PDT, David Baron :dbaron: ⌚️UTC-7
darin.moz: review+
bzbarsky: superreview+
dveditz: approval‑aviary1.0.7-
dveditz: approval1.7.12-
dbaron: approval1.8b5+
Details | Diff | Splinter Review
with name change (12.08 KB, patch)
2005-09-13 16:28 PDT, David Baron :dbaron: ⌚️UTC-7
no flags Details | Diff | Splinter Review
2-line aviary branch patch (1.08 KB, patch)
2005-09-13 18:50 PDT, Daniel Veditz [:dveditz]
dbaron: review+
brendan: superreview+
dbaron: approval‑aviary1.0.7+
dbaron: approval1.7.12+
Details | Diff | Splinter Review

Description shutdown 2005-08-28 13:13:56 PDT
Spin off from bug 300830 comment 15.
EqualsLiteral for distinguishing "about safe" can be security problem.

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] 2005-08-29 14:33:41 PDT
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] 2005-08-30 12:14:09 PDT
Who is going to patch this?

Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2005-08-30 12:48:55 PDT
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
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2005-09-09 14:35:53 PDT
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) 2005-09-09 15:50:10 PDT
(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 [:bz] (still a bit busy) 2005-09-09 16:06:52 PDT
> 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] 2005-09-12 19:34:28 PDT
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-7 2005-09-13 13:24:41 PDT
Created attachment 195933 [details] [diff] [review]
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-7 2005-09-13 13:29:18 PDT
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 [:bz] (still a bit busy) 2005-09-13 13:36:45 PDT
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 2005-09-13 13:58:09 PDT
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.

Comment 12 David Baron :dbaron: ⌚️UTC-7 2005-09-13 14:17:11 PDT
Created attachment 195946 [details] [diff] [review]
using shared header
Comment 13 David Baron :dbaron: ⌚️UTC-7 2005-09-13 14:21:48 PDT
Hrm, maybe NS_AboutURIToModuleName instead of NS_AboutURIToAboutModule?
Comment 14 Darin Fisher 2005-09-13 14:37:31 PDT
Comment on attachment 195946 [details] [diff] [review]
using shared header

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

>+ * The Original Code is __________________________________________.


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

Hmm.. how about NS_GetAboutModuleName?

Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-13 15:42:36 PDT
should NS_AboutURIToAboutModule assert that the scheme is about?
Comment 16 David Baron :dbaron: ⌚️UTC-7 2005-09-13 16:28:17 PDT
Created attachment 195966 [details] [diff] [review]
with name change
Comment 17 Daniel Veditz [:dveditz] 2005-09-13 18:50:45 PDT
Created attachment 195991 [details] [diff] [review]
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] 2005-09-13 18:55:43 PDT
Comment on attachment 195991 [details] [diff] [review]
2-line aviary branch patch

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

Comment 19 Daniel Veditz [:dveditz] 2005-09-13 19:03:39 PDT
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 2005-09-13 19:05:48 PDT
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] 2005-09-13 19:08:07 PDT
Fix checked into aviary and 1.7 branches
Comment 22 Simon Fraser 2005-09-13 22:22:13 PDT
Camino uses about:bookmarks and about:history. Does anything need to change there?
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2005-10-02 07:24:17 PDT
Did this ever land on trunk too?  And on 1.8?  That is, can we resolve this?
Comment 24 Daniel Cater 2005-10-30 09:11:44 PST
(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] 2006-01-05 01:28:18 PST
This landed on the trunk on 2005-09-13

Note You need to log in before you can comment on or make changes to this bug.