173010 - [URL] Whitelist for launching safe external protocols and blocking unsafe

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Manko]

This bug isn't alternative for bug 167475 and bug 167473, but it's highly
desired addition to its.

According bugs 163648 and 172498, we need a whitelist for safe external
protocols (such as irc:, magnet:, realaudio:).

Any external protocol which isn't included in a whitelist must be ignored.

if software developers will want to introduce their own additional protocols,
they may register it in Mozilla whitelist during application's installation
process. But remembered user decisions for confirmation dialogs (see bug 167473)
must be saved in user profile in order to prevent developers from launching
their applications without user confirmation.

Please, enter here your suggestions for more safe and usable protocols.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

I have marked this bug security-confidential as it discusses pending security
issues.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think this should be secret. We're already adding a blacklist to protect
us against known-bad protocols which should keep us out of trouble for a while.
We need this bug to be public so that we can thrash out the issues:
-- should we have a whitelist?
-- what should the UI be?
-- what should be on the whitelist?

Comment 3

[Manko]

>-- should we have a whitelist?
Whitelist is better because we can't know what strange and dangerous protocols
will be introduced by Microsoft an could be exploited in future.

This was discussed in bug 163648 and I fully agree with Georgi Guninsky's
comments about this issue.

-- what should the UI be?
See my proposals in bug 167473

-- what should be on the whitelist?
Oh, this is a question :)
IMHO, irc:, magnet: and realaudio: (and, of course, mailto: and news:) are a
first candidates.

Comment 4

[Doug Turner (:dougt)]

to mitch for investigation.

Comment 5

[Mitchell Stoltz (not reading bugmail)]

I think we should have dialogs similar to the unknown content type dialog for
file downloads - an external protocol handler is, after all, very much like a
helper app. The dialog would ask:
- open this URL using Windows default helper app
- Open this URL using a different app
- don't open this type of URL at all
- do/don't ask me again

Comment 6

[Manko]

Mitchel, we already have bug 167473 for your considerations.

Comment 7

[Duncan Kimpton]

I feel my proposals at http://bugzilla.mozilla.org/show_bug.cgi?id=75915#c53
have some bearing.

Comment 8

[Andrew Hagen]

A fix for this bug would have prevented the security hole (the "shell" exploit)
that we recently had.

http://mozilla.org/security/shell.html

Operating system manufacturers will continue to add protocols that may very well
be highly questionable. It would be best to assume all protocols are dangerous
unless known to be safe.

Comment 9

[Daniel Veditz [:dveditz]]

Taking bug.

I doubt this will block 1.8a2 but should block 1.8 itself.

Comment 10

[Jesse Ruderman]

http://www.sacarny.com/blog/index.php?p=105 points out that if we add whitelist
UI, we need to keep the existing blacklist.

Comment 11

[chris hofmann]

UI freeze for preview release means this needs to get on aviary branch fairly soon.

Comment 12

[Aaron P]

If UI freeze is the only think keeping this out of Aviary, perhaps someone
should just make the UI, which would have the two fields (one for the whitelist
and one for the blacklist). The blacklist could be implemented, since it
currently exists; while the whitelist could just be greyed out until the feature
can be added (hopefully for the 1.0 full release).

Obviously, the best solution would be for someone to write the full whitelist
functionality, but it doesn't look like that is going to happen.

Comment 13

[Mike Connor [:mconnor]]

Dan, what's the status of this?  Its getting pretty late to take on something
that's going to be pretty major if done right.

Based on comment 11, minusing for 1.0 final, since if it doesn't make PR, it
waits for 1.5, which might be the best course of action unless we have time to
test the hell out of it.

Comment 14

[Sergey Sokoloff]

Consider adding ed2k: (under Windows) into your whitelist.

Comment 15

[Daniel Veditz [:dveditz]]

Attachment: Ask for permission before loading unknown schemes

This asks the user before loading any unknown external protocol handler. The
default can be changed via hidden pref to either NEVER load unknown handlers,
or to ALWAYS load unknown handlers (the default behavior up to now).

As is currently the case, any explicit pref setting for a specific protocol
will take precedence over the default behavior.

Bug 167475 would be a nice addition to this one (restrict such protocols to
explicit links, not images or frame source)

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 158392 [details] [diff] [review]
Ask for permission before loading unknown schemes

Seeking r= from mscott who was responsible for some of the early code in this
area.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 158392 [details] [diff] [review]
Ask for permission before loading unknown schemes

- In nsExtProtocolChannel::SetNotificationCallbacks():

 {
+    mCallbacks = aNotificationCallbacks;
	return NS_OK;	    // don't fail when trying to set this

Wanna replace that tab with two spaces while you're here?

sr=jst

Comment 18

[Boris Zbarsky [:bzbarsky]]

I see a few problems with this patch...

1)  The dialog comes up sync off an AsyncOpen() call.  That's a violation of the
    API, really.  The dialog needs to come off an event (perhaps just have the
    event do the LoadURL() call).
2)  This patch reuses the existing protocol handler preferences.  This has some
    unintended side effects, I think.  For example, if I have a protocol we
    can't handle internally and I click on a link of that type and say it's ok
    to use the external app every time and _then_ I install an extension that
    adds an internal handler for the protocol, we will continue using the
    external app, ignoring the internal handler.  There are similar issues with
    the "false" value for the pref... All of this is compounded by the lack of
    UI to manage said prefs.
3)  The prompt doesn't tell the user what app will be used.  I think it should;
    as it is, the user has no basis for deciding whether to allow the load...

Comment 19

[Darin Fisher]

> 1)  The dialog comes up sync off an AsyncOpen() call.  That's a violation of the
>     API, really.  The dialog needs to come off an event (perhaps just have the
>     event do the LoadURL() call).

Yeah, this is a major problem.  Launching a XUL dialog from LoadURL means
spinning up an event queue, etc.  That could lead to the caller of AsyncOpen
being re-entered unexpectedly.  AsyncOpen is sometimes called deep in some
stack, and re-entering that stack could cause all sorts of problems.  Let's not
risk that.  Instead, we should post a PLEvent to the current event queue, and
call LoadURL from that PLEvent.  It should be fine for ::Open to do the modal
dialog stuff... callers of nsIChannel::open already have to accept such dialogs.


> 2)  This patch reuses the existing protocol handler preferences.  This has some
>     unintended side effects, I think.  For example, if I have a protocol we
>     can't handle internally and I click on a link of that type and say it's ok
>     to use the external app every time and _then_ I install an extension that
>     adds an internal handler for the protocol, we will continue using the
>     external app, ignoring the internal handler.  There are similar issues with
>     the "false" value for the pref... All of this is compounded by the lack of
>     UI to manage said prefs.

Yeah, it seems like we need UI to manage these settings just like we have UI to
manage the handlers for unknown mime types.  We have the same issue today with
extensions adding content handlers.  Of course, it is possible that an extension
could reset the pref.


> 3)  The prompt doesn't tell the user what app will be used.  I think it should;
>     as it is, the user has no basis for deciding whether to allow the load...

I agree with this point too.  It probably wouldn't be that hard to add something
like this... we'd probably have to add an API that nsOSHelperAppService would
implement.

Comment 20

[Scott MacGregor]

Comment on attachment 158392 [details] [diff] [review]
Ask for permission before loading unknown schemes

minus'ing after reading comments from Boris and Darin.

Comment 21

[Daniel Veditz [:dveditz]]

Attachment: updated to handle AsyncOpen correctly

Updated to address the AsyncOpen concern.

I think this is the right set of prefs to use rather than make up a second "ask
me" set that would only take effect after nsIOService looks at the first set.
The lack of a managing UI is a little problem, but not one that needs to keep
this security feature out of the tree (and could even be added as an extension
later). I'll file a bug for this.

Showing the appname would be nice, but we currently don't have a clue what the
OS is going to use. As Darin said, we'd need a new entry point on the OS helper
service, and then have to write the implementation for all platforms.  It's not
realistic to get that into 1.0PR, but simply showing the suspicious link is
easy. I'm not sure how useful knowing the app would be, at least we would still
want to show the link because I'd much rather know someone's trying an
aim:GoAway?hahaha link on me than just that it's launching AIM. I will file a
bug here, too.

Ignore minor tab cleanups.

Comment 22

[Darin Fisher]

Comment on attachment 158476 [details] [diff] [review]
updated to handle AsyncOpen correctly

>Index: mozilla/uriloader/exthandler/nsExternalProtocolHandler.cpp

>+PRBool nsExtProtocolChannel::PromptForScheme(nsIURI* aURI,
>+                                             nsACString& aScheme)
>+{
>+  // deny the load if we aren't able to ask but prefs say we should
>+  nsresult rv;
>+  NS_ASSERTION(mCallbacks, "Notification Callbacks not set!");
>+  if (!mCallbacks)
>+    return PR_FALSE;

This may be the one case that breaks something.  It's possible that some
consumer may have not provided a prompt or notification callbacks.  We'd
of course like to fix any of those cases, so maybe it is for the best that
we block by default and hope for bug reports during the PR cycle.


>+  NS_ASSERTION(prompt, "No prompt interface on channel");
>+  if (!prompt)
>+    return PR_FALSE;

you could also write:

    if (!prompt) {
      NS_ERROR("No prompt interface on channel");
      return PR_FALSE;
    }

same applies elsewhere in this patch.


>Index: mozilla/docshell/resources/locale/en-US/appstrings.properties

>+externalProtocolPrompt=An external application must be launched to handle %1$S: links. Requested link:\n\n\n%2$S\n\n\nIf you were not expectng this request it may be an attempt to exploit a weakness in that other program. Cancel this request unless you are sure it is not malicious.\n

"If you were not expectng this request it may..." should be:
"If you were not expecting this request, it may..."


sr=darin

Comment 23

[Darin Fisher]

I think that UI for this security feature should be a requirement for Firefox
1.0.  We should probably leverage the "banner"-based UI notification, and
provide a management interface similar to what is currently provided for other
things like XPI whitelisting.  Only, this would apply to URL schemes instead of
hostnames.

Comment 24

[Scott MacGregor]

Comment on attachment 158476 [details] [diff] [review]
updated to handle AsyncOpen correctly

plussing after talking to jst (who already reviewed the original patch) and
shaver. 

Dan can you make sure we file a spin off 1.0 blocker for adding the UI per
Darin's comments?

Comment 25

[Daniel Veditz [:dveditz]]

Fixed typo and formatting nits

filed bug 258799 and bug 258802 on requested enhancements to the UI

Fix checked into aviary, waiting for bustage to clear on trunk.

Not sure if we want this fix on the 1.7.x branch since it introduces new
strings. We could maybe tweak the patch to have hardcoded english defaults if
the stringbundle contents are missing...

Comment 26

[Boris Zbarsky [:bzbarsky]]

I believe this falls into the "core functionality that should be the same
between 1.7 and aviary" category.  If we're so desperate to have this on aviary,
we need to take this on 1.7.

Comment 27

[Daniel Veditz [:dveditz]]

fixed on trunk

Comment 28

[Daniel Veditz [:dveditz]]

Attachment: 1.7 branch patch (handles missing localized strings)

Comment 29

[Bill Gianopoulos [:WG9s]]

re comment #23   If the UI is a requirement for 1.0, then any text it is going
touse needs to be defined realy quickly because we are up againt the l10n freeze
for the 1.0 release.

Comment 30

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

Comment on attachment 158476 [details] [diff] [review]
updated to handle AsyncOpen correctly

Index: mozilla/uriloader/exthandler/nsExternalProtocolHandler.cpp

+    PRBool PromptForScheme(nsIURI *aURI, nsACString& aScheme);

since aScheme seems to be an in parameter it should be const. but the scheme
could be gotten off the URI too, why pass it here?

+  NS_ENSURE_ARG_POINTER(aNotificationCallbacks);

this should not be a runtime check, since people who pass NULL to a get
function totally deserve what they get. also, scripts can't do that.

+  return NS_OK;       // don't fail when trying to set this

the comment seems to be pointless now that this function actually does
something

+static void *PR_CALLBACK handleExtProtoEvent(PLEvent *event)
+{
+  nsExtProtocolChannel *channel =
+      NS_STATIC_CAST(nsExtProtocolChannel*, PL_GetEventOwner(event));
+
+  if (channel)

when can this be null? same in destroyExtProtoEvent

Comment 31

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

since this bug is fixed, I filed Bug 259084 about the issues from comment 30.

Comment 32

[Darin Fisher]

Comment on attachment 158515 [details] [diff] [review]
1.7 branch patch (handles missing localized strings)

looks fine to me, r=darin

Comment 33

[Ben Bucksch (:BenB)]

*** Bug 163767 has been marked as a duplicate of this bug. ***

Comment 34

[Manko]

OK, great work!

Is bug 167473 fixed by this?

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 158515 [details] [diff] [review]
1.7 branch patch (handles missing localized strings)

a=asa for 1.7.x checkin.

Comment 36

[OstGote!]

A patch for bug 173010 and bug 263546 was checked in on MOZILLA_1_7_BRANCH on
2004-10-20 09:26. So is this fixed1.7.5?

Comment 37

[Asa Dotzler [:asa]]

1.7.5 has shipped. Moving request to 1.7.6.

Comment 38

[Daniel Veditz [:dveditz]]

This one's fixed, don't need blocking flag.