Note: There are a few cases of duplicates in user autocompletion which are being worked on.

loading unsafe about: URLs using HTML forms

RESOLVED FIXED

Status

()

Core
Security
RESOLVED FIXED
12 years ago
12 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.
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

Comment 3

12 years ago
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

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

Comment 6

12 years ago
> 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.
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?
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 10

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

Updated

12 years ago
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
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+
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.
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
Group: security

Comment 23

12 years ago
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?
Blocks: 256195
This landed on the trunk on 2005-09-13
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.