The default bug view has changed. See this FAQ.

loading unsafe about: URLs using HTML forms

RESOLVED FIXED

Status

()

Core
Security
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: shutdown, Assigned: dveditz)

Tracking

({fixed-aviary1.0.7, fixed1.7.12, fixed1.8})

Trunk
fixed-aviary1.0.7, fixed1.7.12, fixed1.8
Points:
---
Bug Flags:
blocking1.7.12 +
blocking-aviary1.0.7 +
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix], URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

12 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b4rc?
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?
Whiteboard: [sg:fix]

Updated

12 years ago
Flags: blocking1.8b4rc? → blocking1.8b4+
Who is going to patch this?

/be
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?
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?
(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...
> 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.
(Assignee)

Comment 7

12 years ago
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.
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?
(Assignee)

Updated

12 years ago
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
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.
Attachment #195933 - Flags: review?(darin)
If you see a good way of sharing that function (plus the nsIURI::GetPath call
thet precedes it), let me know...
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).
Attachment #195933 - Flags: superreview+

Comment 11

12 years ago
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
Attachment #195933 - Flags: review?(darin) → review+
Created attachment 195946 [details] [diff] [review]
using shared header
Attachment #195933 - Attachment is obsolete: true
Attachment #195946 - Flags: superreview?(bzbarsky)
Attachment #195946 - Flags: review?(darin)
Attachment #195946 - Flags: approval1.8b5?
Attachment #195946 - Flags: approval1.7.12?
Attachment #195946 - Flags: approval-aviary1.0.7?
Hrm, maybe NS_AboutURIToModuleName instead of NS_AboutURIToAboutModule?
Attachment #195946 - Flags: superreview?(bzbarsky) → superreview+

Comment 14

12 years ago
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
Attachment #195946 - Flags: review?(darin) → review+
should NS_AboutURIToAboutModule assert that the scheme is about?
Created attachment 195966 [details] [diff] [review]
with name change
Attachment #195946 - Attachment is obsolete: true
(Assignee)

Comment 17

12 years ago
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.
Attachment #195991 - Flags: superreview?(shaver)
Attachment #195991 - Flags: review?(dbaron)
Attachment #195991 - Flags: approval1.7.12?
Attachment #195991 - Flags: approval-aviary1.0.7?
Comment on attachment 195991 [details] [diff] [review]
2-line aviary branch patch

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

/be
Attachment #195991 - Flags: superreview?(shaver) → superreview+
Attachment #195991 - Flags: review?(dbaron) → review+
Attachment #195991 - Flags: approval1.7.12?
Attachment #195991 - Flags: approval1.7.12+
Attachment #195991 - Flags: approval-aviary1.0.7?
Attachment #195991 - Flags: approval-aviary1.0.7+
(Assignee)

Comment 19

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

We're going with the smaller patch for the old branches
Attachment #195946 - Flags: approval1.7.12?
Attachment #195946 - Flags: approval1.7.12-
Attachment #195946 - Flags: approval-aviary1.0.7?
Attachment #195946 - Flags: approval-aviary1.0.7-

Comment 20

12 years ago
I'm happy with the 2-liner FWIW for the old branches.  Let's go with dbaron's
latest patch on the trunk.
(Assignee)

Comment 21

12 years ago
Fix checked into aviary and 1.7 branches
Flags: blocking1.7.13+
Flags: blocking1.7.12?
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.8+
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.0.7+
Keywords: fixed-aviary1.0.7, fixed1.7.12
Attachment #195946 - Flags: approval1.8b5? → approval1.8b5+

Comment 22

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

Updated

12 years ago
Keywords: fixed1.8
(Assignee)

Updated

12 years ago
Group: security
Did this ever land on trunk too?  And on 1.8?  That is, can we resolve this?

Comment 24

12 years ago
(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?
(Assignee)

Updated

12 years ago
Blocks: 256195
(Assignee)

Comment 25

11 years ago
This landed on the trunk on 2005-09-13
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.