Last Comment Bug 120373 - [FIX]nsIScriptSecurityManager::CheckLoadURI isn't scalable to new protocols
: [FIX]nsIScriptSecurityManager::CheckLoadURI isn't scalable to new protocols
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: All All
: P1 enhancement with 1 vote (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
: benc
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
: 101928 (view as bug list)
Depends on: 361231 368160
Blocks: clu JEP/caps
  Show dependency treegraph
 
Reported: 2002-01-16 11:57 PST by timeless
Modified: 2011-05-02 15:10 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (46.78 KB, patch)
2006-09-21 10:26 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Same as diff -w (45.88 KB, patch)
2006-09-21 10:33 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Previous patch had a typo (46.69 KB, patch)
2006-09-21 22:21 PDT, Boris Zbarsky [:bz]
dveditz: review+
Details | Diff | Splinter Review
diff -w (45.79 KB, patch)
2006-09-21 22:24 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Updated to comments (79.09 KB, patch)
2006-10-04 22:52 PDT, Boris Zbarsky [:bz]
dveditz: review+
Details | Diff | Splinter Review
Same as diff -w (78.03 KB, patch)
2006-10-04 22:53 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Folder file with testcase messages (12.59 KB, text/plain)
2006-10-18 20:02 PDT, Boris Zbarsky [:bz]
no flags Details
Patch with "addbook" world-accessible again, and updated to tip (77.99 KB, patch)
2006-10-18 20:11 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Same as diff -w (76.98 KB, patch)
2006-10-18 20:26 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Updated to dveditz's comments (78.82 KB, patch)
2006-10-19 21:19 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Updated to Places changes (78.99 KB, patch)
2006-10-25 10:55 PDT, Boris Zbarsky [:bz]
darin.moz: superreview+
Details | Diff | Splinter Review

Description timeless 2002-01-16 11:57:28 PST
If we're going to dream about pipes, i have some:
3. be able to add new protocols (wgate needs this) w/o rebuilding this file or
maintaining a patch to it.
2. adding logic to 868-874 that blocked http, https, and ftp would satisfy some
people's wishes that we not load any content from the network while reading
mail. The trick is to do that cleanly, #1 would allow that as would some variant.
1. be able to add protocols to the blacklist and whitelist using prefs.js

Bonus points, aim wouldn't have to be listed in this file (aim shouldn't be here
for the same reasons that wgate's protocol doesn't belong here). And https could
be moved out of this file.

So ... instead of doing what we're doing now, could the protocol handlers
themselves report whether they're safe to the security manager, so that the
security manager wouldn't have to hard code the list at all?
Comment 1 Mitchell Stoltz (not reading bugmail) 2002-01-18 11:51:48 PST
Yeah, we've talked about that. It seems like a good thing to do. As for using
CheckLoadURI to implement the anti-web-bug solution, I'm not sure that's the
best way to go about it. In any case, we've got some other bugs that deal with
the web-bug problem, and some other solutions proposed, so let's leave that out
of this bug. Allowing protocol handlers to specify their own security
requirements is definitely something I'd like to see happen.
Comment 2 Brendan Eich [:brendan] 2002-01-18 12:31:44 PST
Why can't we do the protocol handler query instead of hardcoding thing in 0.9.9?

/be
Comment 3 Mitchell Stoltz (not reading bugmail) 2002-01-22 10:55:41 PST
Because "we" have some other priorities and might not get to this, and I'm
trying to be a little more realistic about how many bugs I can fix in a
milestone. My first priorities are performance and fixing vulnerabilities. If
you think this should be a higher priority, please let me know.
Comment 4 Mitchell Stoltz (not reading bugmail) 2003-02-13 15:26:13 PST
Let's try to do this for 1.4 alpha.
Comment 5 Mitchell Stoltz (not reading bugmail) 2003-02-13 15:31:11 PST
*** Bug 101928 has been marked as a duplicate of this bug. ***
Comment 6 Boris Zbarsky [:bz] 2004-02-04 17:25:52 PST
What sort of security info would we want to expose on the protocol handlers?
Comment 7 Christopher Aillon (sabbatical, not receiving bugmail) 2004-02-04 17:50:55 PST
off the top of my head:

data such as whether the protocol is public (http:), protected (file:), private
(chrome:), or "non-linkable from anywhere" (not sure of a better term for it)
(e.g. mailbox:, imap:) would be nice.  It would also be nice to know what
protocols are "dynamic" things (though dynamic really isn't a good term -- most
of the web is dynamic -- please come up with a better one) such as data: or
javascript:.  Also news: seems to be special.  the browser can load news: urls,
but mail can't if nsIScriptSecurityManager::DISALLOW_FROM_MAIL is passed to
checkLoadURI, so perhaps a "browser only" flag?
Comment 8 Boris Zbarsky [:bz] 2006-09-21 10:26:25 PDT
Created attachment 239527 [details] [diff] [review]
Proposed fix

Summary of changes:

1)  Add flags to nsIProtocolHandler to represent the "protected", "private",
    and "not linkable" states from comment 7 (which correspond to the the
    existing PrefControlled, ChromeProtocol, DenyProtocol values in
    nsScriptSecurityManager).  Having none of the flags set means the protocol
    is public, which is the current default anyway.
2)  Rename the CheckLoadURI constants to not refer to particular protocols and
    rev the IID of nsIScriptSecurityManager.  The latter is not strictly
    needed, since the constants have the same numeric values and mean the same
    thing, but it's safer this way, imo.  If people want, I can keep the old
    constant names as aliases for the new ones, so that existing JS code will
    continue to work instead of suddenly passing |undefined|, which might
    translate to STANDARD...
3)  Rewrite CheckLoadURI in terms of the new protocol flags and some of the
    existing ones.
4)  Update secman callers to use the new flag names.
5)  Update protocol handlers to duplicate the functionality in the table.
6)  Remove the URI_HAS_NO_SECURITY_CONTEXT flag from the external protocol
    handler -- that flag is basically used for inheriting the security context,
    and that makes no sense for the external protocol handler.  Without this
    change, loads of external URIs would be blocked everywhere we block
    javascript:/data: right now, which seems undesirable.  Maybe we should
    rename the protocol flag to URI_INHERITS_SECURITY_CONTEXT, btw?  It's
    newly-introduced, so this should be safe...

As far as the hardcoded list goes, the following things are non-obvious from the patch:

A) "pop" is AllowProtocol while "pop3" is DenyProtocol.  I left that as-is, but
   is that right?  Note that nsPop3Service is used as the protocol handler for
   both types of URIs.  And that some other security code in mailnews 
   (nsMsgContentPolicy comes to mind) knows about "pop" but not "pop3"?  I'll
   try to get a mailnews peer to comment on this, esp. since I have no way to
   test POP mail.
B) There is no more "keyword" protocol handler, so that part just got removed.
C) There is no more "res" protocol handler, so that part just got removed.
D) I changed "x-jsd" from ChromeProtocol to DenyProtocol -- I can't see a
   reason for web pages to be able to load scripts or stylesheets or XBL
   bindings from x-jsd URLs.  This change we might want on branches, even.
Comment 9 Boris Zbarsky [:bz] 2006-09-21 10:29:23 PDT
Comment on attachment 239527 [details] [diff] [review]
Proposed fix

mscott, could you perhaps check over the mailnews changes?  I'm particularly worried about the pop/pop3 thing (and about the fact that random web content is currently allowed to link to pop: URIs, with no explanations given in the checkin comment or the originating bug).
Comment 10 Boris Zbarsky [:bz] 2006-09-21 10:31:22 PDT
Comment on attachment 239527 [details] [diff] [review]
Proposed fix

Silver, could you review the jsd changes?  They should be ok even in old Geckos, since "foo | undefined" == "foo".  But just in case.  Also I did change the loading permissions on x-jsd; if I'm wrong about who should be able to access it, I'd like to know.
Comment 11 Boris Zbarsky [:bz] 2006-09-21 10:33:16 PDT
Created attachment 239529 [details] [diff] [review]
Same as diff -w

This makes some of the nsScriptSecurityManager changes easier to review, I think.
Comment 12 Boris Zbarsky [:bz] 2006-09-21 10:37:20 PDT
> A) "pop" is AllowProtocol while "pop3" is DenyProtocol.  I left that as-is, 

Given that the same protocol handler is used for both, that's false.  In fact, they are both DenyProtocol with my patch.
Comment 13 Boris Zbarsky [:bz] 2006-09-21 22:21:33 PDT
Created attachment 239614 [details] [diff] [review]
Previous patch had a typo
Comment 14 Boris Zbarsky [:bz] 2006-09-21 22:24:13 PDT
Created attachment 239615 [details] [diff] [review]
diff -w
Comment 15 Ben Bucksch (:BenB) 2006-09-21 22:50:17 PDT
First, thanks a lot for fixing this!

Is there a good reason to default to open/access? Can we default to no access unless the protocol writer made an explicit decision to allow things? Effects are for (1) psychology, (2) mistakes, and (3) compatibility for new flags as you mentioned and as the IDL comments say.

nsIScriptSecurityManager Comments: I would keep/add examples of what you mean with the new generalizations.
Comment 16 Boris Zbarsky [:bz] 2006-09-21 22:58:09 PDT
> Is there a good reason to default to open/access?

Sort of.  That's what we do now; changing it would require changing a lot of extensions, embedding apps, etc.

Furthermore, if the "don't allow loading me" value is indicated by absence of all flags, then it's not clear what's to be done with a URI chain in which some URIs have no flags and some URIs have the ALLOW_ACCESS flag.  Should we allow access?  I'd say no.  But there's no really good way right now to test for a URI chain in which each URI is _missing_ certain flags or in which each URI has certain flags, though I guess we could add such a way.

I do agree it would be better security in general if we required an ALLOW_ACCESS flag from protocols...

I'm not sure how having such a flag would improve compatibility for new flags, though.  Can you explain?
Comment 17 Boris Zbarsky [:bz] 2006-09-22 10:55:37 PDT
Oh, I recall the #1 reason for not making URI_IS_CHROME the default.  nsIProtocolHandler is frozen. So existing code that implements it must continue working.  For example, if I have a protocol handler that I build against Gecko 1.0 and then ship, it must continue working the same way against Gecko 1.9.  Since Gecko 1.0 defaulted protocol handlers to Allow, we have to continue doing the same.

One of those times I keep wishing that necko had been designed with security from the ground up.  :(
Comment 18 Ben Bucksch (:BenB) 2006-09-22 11:16:30 PDT
I answered to comment 16 per mail. In paritcular, it's very confusing which flag an insecure protocol would use.

Comment 17: Does that mean we have to live with insecure defaults forever? I personally think that security is reason to break frozen interfaces, albeit with reason. Even Microsoft (compatibility fetishist #1) broke apps in SP2 for security.
A way would be to introduce a number of flags that clearly label the security level the protocols provide. If they are missing, print a warning that the developer will see.
Comment 19 Boris Zbarsky [:bz] 2006-09-22 11:38:45 PDT
> In paritcular, it's very confusing which flag an insecure protocol would use.

I can add comments that say that a protocol should be URI_IS_SYSTEM unless it has good reasons not to be.

> Comment 17: Does that mean we have to live with insecure defaults forever?

Until we decide to completely break all compat, yes.  That is, announce that all code linked against earlier Gecko versions, even using only frozen interfaces, is not guaranteed to work against the new Gecko version (which will probably be called something like 2.0 or 3.0 or whatever).
Comment 20 Daniel Veditz [:dveditz] 2006-09-25 00:53:24 PDT
Comment on attachment 239614 [details] [diff] [review]
Previous patch had a typo

>+     * "Automatic" loads originating from URIs with this protocol are not
>+     * allowed.  The decision as to what constitutes an "automatic" load is
>+     * made externally.
>+     */
>+    const unsigned long URI_FORBIDS_OUTGOING_AUTOLOADS = (1<<5);

This one is confusing. Apart from concerns about the name (I can't think of a better one) the comments should note that it's paired with the nsIScriptSecurityManager::LOAD_IS_AUTOMATIC_NEW_DOCUMENT CheckLoadURI flag. The comments are way too non-commital about what the flag means, no one will be able to figure out whether or not their new protocol handler should set this flag or not. They could at least give the hint that it's defacto talking about things like redirects.

Also maybe this one should be the last new one added rather than the first. The others make more sense, save the confusion to last :-)

>+    // Indicate that the load is an "automatic" (declarative, not
>+    // user-triggered or scripted) load of a new document.
>+    const unsigned long LOAD_IS_AUTOMATIC_NEW_DOCUMENT = 1 << 0;

Likewise here you could mention the tie to URI_FORBIDS_OUTGOING_AUTOLOADS

>-    const unsigned long DISALLOW_SCRIPT_OR_DATA = 1 << 2;
>+    // Don't allow URLs which would inherit the caller's principal to load.
>+    const unsigned long DISALLOW_INHERIT_PRINCIPAL = 1 << 2;

I'm slightly concerned about this name change. I like it, but worry about extensions that may continue to use the old name and become unsafe (I'm not worried about the DISALLOW_FROM_MAILNEWS name change). I also worry slightly that the abstract name will confuse people and they'll just go for the concrete DISALLOW_SCRIPT, when really this one would be better. At least mention in the DISALLOW_SCRIPT comment that this one is preferred.

I like your proposed name change from URI_HAS_NO_SECURITY_CONTEXT to URI_INHERITS_SECURITY_CONTEXT (though not reflected in the patch) to better match this flag name.

>+    //-- Some callers do not allow loading javascript:
>+    if ((aFlags & nsIScriptSecurityManager::DISALLOW_SCRIPT) &&
>+         targetScheme.EqualsLiteral("javascript"))

After all this work it seems a shame to hardcode this scheme in here like this, although one can hope we wouldn't ever invent another script URI scheme.

>+    rv = DenyAccessIfURIHasFlags(aTargetURI, nsIProtocolHandler::URI_IS_SYSTEM);

What's the performance impact of all these calls to DenyAccessIfURIHasFlags? seems like it's got to be more expensive than the straight string compares that were here before.

>+    if (NS_FAILED(rv)) {
>+        // Deny access, since the origin principal is not system
>+        ReportError(nsnull, errorTag, sourceURI, aTargetURI);

You don't report errors in the two earlier calls to DenyAccessEtc, the checks for LOAD_IS_AUTOMATIC_NEW_DOCUMENT and DISALLOW_INHERIT_PRINCIPAL, nor in the DISALLOW_SCRIPT case. What makes this one special?

> Index: extensions/datetime/nsDateTimeHandler.cpp

I'd just as soon see this one ripped out of the tree, but I guess that's a different bug.

r=dveditz
Comment 21 Ben Bucksch (:BenB) 2006-09-25 06:32:04 PDT
I promised bz to report about our mail discussions:
I suggested very concrete, hands-on (real-world) examples, to make up for the very abstract generalizations. For example, I only understood that nsNestedIDL is when I read jar: and view-source: in the check-in comment. The same applies to CheckLoadURI and nsIProtocolHandler. We need to keep in mind that these are used by third party extension writers who usually have only a slight idea about security in Mozilla, and extension security is *our* business.

Secondly, I suggested to extend the flags in nsIProtocolHandler to (a) explicitly mark the security they provide, and (b) more fine-grained and (c) with different naming.

With the current patch, if the protocol does provide adequate security for the rough Internet, it should simply not set any flags - I think that should be done explicitly, for code-readability, and because people tend to do nothing when they are not sure. Here, if they set no flag, and there are edge cases which can be abused, we have a hole - that's not good. Suggestions below.

That's made worse by the naming, which can be very confusing, even misleading for some people. When I have insecure JS that may be abused by the web, I should make sure it is *not* chrome (nor with system principal), but rather treated as webpage. So, I would infer that for my insecure protocol, I should avoid by all means to set URI_IS_CHROME/SYSTEM. However, here it's the opposite (and I had to let bz confirm it to me to believe it): SYSTEM is *good* for insecure stuff, because it won't be accessible to webpages. So, the wording is a huge problem. Given that most users of these flags don't know how caps work, I would instead suggest a naming that goes after the intent, what it's supposed to do or when it's to be used, rather than the internal workings. For example: URI_IS_WEBSAFE or URI_IS_SECURE instead of the current 'no flag at all'. Then URI_IS_NOT_WEBSAFE or URI_IS_DANGEROUS or URI_CAN_TRASH_YOUR_SYSTEM for URI_IS_SYSTEM. All of these should come with comments with detailed explanation and concrete, real-world examples, esp. common ones, the 80%-use-cases. Maybe for theory's sake also examples for the extreme edges of what's covered.
Additionally, I think we need more flags, because the URI_IS_WEBSAFE, URI_IS_DANGEROUS, URI_NO_AUTOLOAD is not differentiated enough. For example, a URI_EXPENSIVE: mailto: probably should not be allowed to be called by JS or otherwise on short succession (e.g. in a frameset with 100 mailto: "frames"). Many protocol handlers would probably have similar requirements: Anything that consumes considerable system resources i.e. anything that starts a new program, or anything that opens a new window, e.g. <aim:greybat> or even <translate:http:...> calling translator.exe. Maybe a timer and some user action should be required between 2 of these loads (I wouldn't want to have 100 composer windows on my screen even after I'm away from screen). Or URI_ONLY_USER_TRIGGERED: There are also URIs which should only be called on explicit user action, e.g. <tel:+900-....>. You can argue that this is different from URI_EXPENSIVE, because it's fine to call translate: in a frameset of a webmailer, or to call mailto: from a script once, to construct a message, given that the user has to confirm the sending in the mailer. BTW, if we implement this, these kinds of explanations and examples I mention here should also be in the comments of the IDL. So, in summary, I think there are many more flags needed, and we should document them well, and we should ensure that each protocol is explicitly classified by at least one of these flags.

We can ensure the latter without breaking the frozen nsIProtocolHandler e.g. by:
   * require explicit flagging of the security the protocol provides,
     at least URI_IS_WEBSAFE (instead of current absence of flags)
   * clearly warn the developer when the flags are missing,
     in a way that they are sure to read (e.g. dialog, or crash if needed)
   * but still default to access in releases
Most importantly, it forces developers to know and think about the problem at all. But it would keep the frozen behaviour and keep existing binaries working for users, thus not forcing a new extension release, but push developers to update the code immediately. This update would be backwards-compatible.
That's a pattern that could generally be used for now-unwanted behaviour of frozen interfaces, BTW.

However, third party developers may be alerted to a potential security problem the first time and not have time to immediately check or fix their source, which may have far-reaching implications up to a re-write of all of their code. They shouldn't set URI_IS_WEBSAFE, just because they need to be reachable from http, although they are not secure. For them, we could maybe add URI_NEEDS_TO_BE_WEB_ACCESSIBLE_DESPITE_UNKNOWN_SECURITY, which would be more or less the same as URI_IS_WEBSAFE technically, but act as red flag (which also could be searched for). We could also default to that when there's no explicit flag set, but it doesn't make much of a difference.

Concrete suggestions:
In CheckLoadURI, if no specific deny flag matched, check that URI_IS_WEBSAFE exists (*everywhere* in the nested URI - yes, there is currently no such function, but it should be trivial to add). If it does not, trigger a clear warning (dialog or crash) in DEBUG only and assume URI_IS_WEBSAFE (or URI_NEEDS_TO_BE_WEB_ACCESSIBLE_DESPITE_UNKNOWN_SECURITY). Extension writers should add the correct flag with the next release, but users would see no difference.

nsIProtocolHandler comments/naming suggestions:

/* The following specifies which level of security the URI handler requires/provides.
 * A wrong flag here will cause a security hole.
 *
 * You must specify at least one of the flags:
 *   URI_WEBSAFE
 *   URI_DANGEROUS
 *   URI_LOAD_IS_PREF_CONTROLLED
(*   ...)
 * Specifying no flag is deprecated and will cause red flags,
 * but will for now default to URI_IS_WEBSAFE, for
 * backwards-compatibility. However, this is dangerous and
 * will cause a security hole if the protocol handler
 * is not actually websafe.
 */

/**
 * This URI handler is secure enough to be called from arbitrary,
 * untrusted web content.
 * If you use this, make sure that there is no way,
 * under no circumstance, in no edgecase,
 * your code or anything it calls could be used to
 * write files to disk, execute native commands etc..
 * Look out for escaping of shell parameters, calls to chrome-JS,
 * the behavour or any external programs you may call
 * in addition to the typical buffer overflows etc..
 * Mozilla's internal http: handler uses this.
 */
("in addition to the typical buffer overflows" is intentionally discomforting the reader to make him realize that providing such security is hard and usually requires specialized knowledge, to avoid to usual reaction "yeah, no problem here, I'm just calling shell trillian -to <var>" - ops, he forgot the quotes!)
const unsigned long URI_IS_WEBSAFE = (1<<6);

/**
 * Use this if you absolutely need your uri to be accessible from
 * arbitrary web content, but are not certain that you fulfill the
 * requirements of URI_IS_WEBSAFE. This is technically
 * currently the same as URI_IS_WEBSAFE, and thus carries
 * the same risks. But the intent is to differentiate between
 * really safe and checked protocols and those in urgent need
 * of care. I.e. this is a red flag.
 */
const unsigned long URI_NEEDS_TO_BE_WEB_ACCESSIBLE_DESPITE_UNKNOWN_SECURITY = (1<<11);

/**
 * chrome: URIs, i.e. part of the application's user interface
 * XXX Please explain better when this would be used.
 */
const unsigned long URI_IS_CHROME = (1<<7);
/**
* Use this for URIs which allow the caller to trigger arbitrary
* native commands (directly or indirectly),
* write to non-safe local files or otherwise call all-powerful
* or dangerous functions.
* Such URIs are not allowed to be loaded by non-system callers,
* i.e. web content cannot reach them.
* If you are not sure about the security of your code, use this.
*/
const unsigned long URI_IS_DANGEROUS = (1<<6);
const unsigned long URI_IS_SYSTEM = (1<<6);

/**
* Whether this URI can be called is controlled by the pref ...
* TODO describe preference
*/
const unsigned long URI_PREF_CONTROLLED = (1<<8);

/**
 * "Automatic" loads (e.g. redirects) originating *from* these URIs
 * are not allowed. (This is unlike the other flags which
 * control whether calls *to* this URI are allowed.)
 * For example, imap: sets this flag to prevent emails
 * triggering redirects.
 * What constitutes an autoload is defined by the caller
 * of CheckLoadURI setting the
 * LOAD_IS_AUTOMATIC_NEW_DOCUMENT flag
 *
 * XXX is this only about redirects?
 * If so, replace "AUTOLOAD" with "REDIRECT"?
*/
const unsigned long URI_NO_AUTOLOADS_FROM_HERE = (1<<5);

/**
 * This URI e.g. starts a new application or opens new windows.
 * Thus, it may no be called in a loop, in short succession or
 * many times in a row.
 * For example, prevent mailto: or aim: opening lots of windows
 * at once, at every mouse move or while the user is away.
 * Or many <translate:http:...> loads triggering translate.exe may
 * make the computer unresponsive, but it's OK once or twice
 * in a frameset.
 */
const unsigned long URI_IS_EXPENSIVE = (1<<9);

/**
 * URI load must be triggered by explicit and conscious user action
 * For example, you don't want <tel:+900-...> be triggered by a frameset.
 * Note that this does not relieve you from making your protocol
 * otherwise secure (e.g. prevent to execute arbitrary native commands),
 * so this flag is just an addition to others above.
 */
const unsigned long URI_ONLY_USER_TRIGGERED = (1<<10);


I also mentioned to bz that the name of NS_URIChainHasFlags() was confusing to me, because it checks nsNestedURIs, but for me, a chain is completely different from nesting, a chain goes to the right while nesting goes inside. I'd rename it to NS_URIOrNestedURIHasFlags() (ugly) or similar.

A nit: You may want to rename aFlags (CheckLoadURI parameter) and hasFlags (target URI flags) to loadFlags, targetFlags and sourceFlags.
Maybe you don't even need DenyAccessIfURIHasFlags(), given how short it is and sparely it's used?

Offtopic: It's clear that nsIProtocolHandler is a misnomer, if it's dealing with URIs? (protocol != URI) Point this out in a comment? Rename it? Maybe rename it now to circumvent the frozen interface problem? 
Comment 22 Boris Zbarsky [:bz] 2006-09-25 07:15:35 PDT
Re comment 20:

> the comments should note that it's paired with the nsIScriptSecurityManager

I'll try to see what I can do here that Darin will accept.... Sadly, necko has no dependency on the security manager.  :(  I'll definitely talk about redirects and meta refresh and such.

> Likewise here you could mention the tie to URI_FORBIDS_OUTGOING_AUTOLOADS

Will do.

> I'm slightly concerned about this name change. I like it, but worry about
> extensions that may continue to use the old name and become unsafe

We could leave the old name as an alias for this one, I guess....  Not that hard to do that.  Let me know.

> At least mention in the DISALLOW_SCRIPT comment that this one is preferred.

Er... but it's not, in general.  They just do different things.  I'll add much more exhaustive comments about what they do and how they differ.

> After all this work it seems a shame to hardcode this scheme in here

Yeah.  I could add another flag to handle that case, but I really doubt we'll ever want another script URI scheme, like you said.  So I figured I'd save the flag bit.  ;)

> What's the performance impact of all these calls to DenyAccessIfURIHasFlags?

I'm not exactly sure, but I can test.  I do wish there were less COM involved in all this, for sure.  :(  But I also don't think this code is on any critical codepaths, with the possible exception of kicking off image loads.

> You don't report errors in the two earlier calls to DenyAccessEtc

Yeah.  I kept exactly the error reporting we used to have, for now.  In general, I would like to rework the error reporting, because right now the caller has no control over whether errors should be reported and chrome needs that control in some cases.  But that will be a separate patch.

Re comment 21:
> mailto: probably should not be allowed to be called by JS or
> otherwise on short succession

Followup bug on this.  I'm not trying to solve all possible security problems; just provide the infrastructure for doing it later.  ;)

> (e.g. dialog, or crash if needed)

Neither option is acceptable, as I said in mail.  I welcome proposals that would really work, but I did spend a few weeks thinking about this issue and couldn't figure out a way to make it work.

> If it does not, trigger a clear warning (dialog or crash) in DEBUG only

That wouldn't solve the extension problem (since extension writers rarely run debug builds), while causing pain for actual Mozilla developers testing code against extensions written against frozen APIs....

> Maybe you don't even need DenyAccessIfURIHasFlags()

Factoring it out made the code a lot more readable.  So I think we do, at least for my sanity.

> It's clear that nsIProtocolHandler is a misnomer, if it's dealing with URIs? 

No, it deals with a protocol.  In Gecko you need a URI to access a protocol.

> Maybe rename it now to circumvent the frozen interface problem? 

I _really_ have no idea what that means.

I'll make some comment changes later this week or early next week, if I have time.
Comment 23 Ben Bucksch (:BenB) 2006-09-25 11:54:37 PDT
> > mailto: probably should not be allowed to be called by JS or

> Followup bug on this.

Yeah, sure. Just wanted to illustrate the idea of fine-grained categorization of security levels.
We can't add new properties to frozen IDLs, right?

> > (e.g. dialog, or crash if needed)
> Neither option is acceptable, as I said in mail.

Print a warning in the JS console, if you prefer.

> > It's clear that nsIProtocolHandler is a misnomer, if it's dealing with URIs? 

> No, it deals with a protocol.

If I want to create an <aim:greyrat> URI to trigger AIM to open a new message window (which is not a protocol, just a URI handler), which interface would I use?
Comment 24 Boris Zbarsky [:bz] 2006-09-25 15:05:46 PDT
> We can't add new properties to frozen IDLs, right?

Yep.

> Print a warning in the JS console, if you prefer.

That we could do, sure.  That might be doable.

> which interface would I use?

nsIIOService.
Comment 25 Scott MacGregor 2006-09-26 10:55:58 PDT
I'll get to this today Boris.
Comment 26 Darin Fisher 2006-09-26 22:14:38 PDT
Comment on attachment 239614 [details] [diff] [review]
Previous patch had a typo

Some nit-picks on the flag names and descriptions...


>Index: netwerk/base/public/nsIProtocolHandler.idl
...
>     /**
>+     * "Automatic" loads originating from URIs with this protocol are not
>+     * allowed.  The decision as to what constitutes an "automatic" load is
>+     * made externally.
>+     */
>+    const unsigned long URI_FORBIDS_OUTGOING_AUTOLOADS = (1<<5);

This flag needs more explanation.  I've read the comment and name many
times, and I still don't know when or how I would use this if I were
implementing a protocol handler.


>+    /**
>+     * The URIs for this protocol are not allowed to be loaded by non-system
>+     * callers.
>+     */
>+    const unsigned long URI_IS_SYSTEM = (1<<6);

"non-system" means non-system principal?


>+    /**
>+     * The URIs for this protocol are "chrome" URIs.  That is, they are part of
>+     * the application's user interface.
>+     */
>+    const unsigned long URI_IS_CHROME = (1<<7);

This does not mean "chrome:" necessarily, so perhaps you should point that
out more clearly.  It seems like this could be confusing.  There should be
some kind of documentation somewhere (maybe referenced from here) that
explains some of these concepts.


>+    /**
>+     * Loading of URIs for this protocol from other origins is controlled by
>+     * preferences of some sort.
>+     */
>+    const unsigned long URI_LOAD_IS_PREF_CONTROLLED = (1<<8);

The flag name doesn't capture the "from other origins" bit that seems so
significant.  Hmm...


>Index: caps/idl/nsIScriptSecurityManager.idl

>+    // Indicate that the load is an "automatic" (declarative, not
>+    // user-triggered or scripted) load of a new document.
>+    const unsigned long LOAD_IS_AUTOMATIC_NEW_DOCUMENT = 1 << 0;

I'm confused as well by this flag description.  "the load is an automatic load of a new document" ... ??
Comment 27 Boris Zbarsky [:bz] 2006-10-04 22:52:54 PDT
Created attachment 241290 [details] [diff] [review]
Updated to comments

Better documentation, better flag names.  Changed the security manager to only look at the explicit "who's allowed to load me" flags on the innermost URI (otherwise, e.g. about:blank comes up as DANGEROUS_TO_LOAD).  Still looking for the INHERIT and AUTOMATIC flags on the whole URI chain, just in case.

I think I got all our in-tree protocol handlers.
Comment 28 Boris Zbarsky [:bz] 2006-10-04 22:53:35 PDT
Created attachment 241291 [details] [diff] [review]
Same as diff -w
Comment 29 Boris Zbarsky [:bz] 2006-10-04 22:54:19 PDT
Comment on attachment 241290 [details] [diff] [review]
Updated to comments

I have 2 mailnews issues now.  pop vs pop3, and the "cid" handler...  Note that with this patch as-is, all "cid" URIs will be denied.
Comment 30 Scott MacGregor 2006-10-09 17:04:12 PDT
(In reply to comment #29)
> (From update of attachment 241290 [details] [diff] [review] [edit])
> I have 2 mailnews issues now.  pop vs pop3, and the "cid" handler...  Note that
> with this patch as-is, all "cid" URIs will be denied.
> 

cid is defined in RFC 2111: http://www.ietf.org/rfc/rfc2111.txt

message bodies use cid urls to refer to other parts contained in the message. For instance:

<img src="cid:somerandompartID.com"> where somerandompartID.com refers to another mime part in the message that actually contains the image part.

Denying it the ability to run script sounds ok to me.

POP3 is a normal mail URL like imap:, mailbox:, etc. and should be Deny like the rest of the mail protocols.

We don't register a protocol handler for POP: urls with the component manager. I think it's used internally as a special URL explicilty constructed by libmime inside the message pane the user clicks on to to fetch the rest of the message body for large messages. But I'm not familiar with it and need to look at it some more.
Comment 31 Robert Sayre 2006-10-09 17:10:40 PDT
(In reply to comment #30)
> 
> cid is defined in RFC 2111: http://www.ietf.org/rfc/rfc2111.txt
> 

Obsoleted by RFC 2392.
Comment 32 Boris Zbarsky [:bz] 2006-10-09 17:18:33 PDT
> Denying it the ability to run script sounds ok to me.

I'm not denying it the ability to run script.  I'm denying untrusted callers the ability to load cid: URIs....

If there's a testcase somewhere (eg folder file I could stick in local folders in Seamonkey or TBird with steps to follow) that I could use to make sure I'm not breaking whatever we use cid: for now, that would be great.

> <img src="cid:somerandompartID.com"> 

Right.  But do we actually support that?  If so, I think my patch would break it, since the untrusted image would not be able to link to the "cid:" link.
Comment 33 Ben Bucksch (:BenB) 2006-10-09 17:35:01 PDT
> > <img src="cid:somerandompartID.com"> 

> Right.  But do we actually support that?

Yes. Both in reading and sending: "Insert image".
There are also (working) cases where you can click on links, IIRC.

> If there's a testcase somewhere

I just sent you one (actually two mails: ignore the first one).
Comment 34 Boris Zbarsky [:bz] 2006-10-09 18:40:13 PDT
> Yes. Both in reading and sending: "Insert image".

I just tried the reading (with the testcase you sent me).  It looks like mailnews rewrites the <img> tag somewhere -- by the time the DOM sees the tag, the URI is mailbox:// or imap: (depending on whether I load it from a local folder or from my inbox).

> There are also (working) cases where you can click on links, IIRC.

Testcase?
Comment 35 Boris Zbarsky [:bz] 2006-10-09 18:45:23 PDT
OK, so there is a bit of a problem with "addbook:" URIs.  The issue is that the way mailnews treats vcards is to add HTML to the mail message that presents the vcard as a link to an "addbook:" URI.  But this HTML has the security context of the mail message now!

So if I leave addbook: as world-accessible (as it is now), any email message or website can trigger such a URI...  If I make it URI_IS_DANGEROUS_TO_LOAD (which it should be, imo), then things break because we're putting parts of our browser UI in the mail message itself...

By the way, Ben, bienvenu, are you OK with me attaching those mails you sent me with the testcases to this bug?
Comment 36 David :Bienvenu 2006-10-09 21:50:27 PDT
sure, you can put my message in the bug. 
Comment 37 Boris Zbarsky [:bz] 2006-10-18 20:02:50 PDT
Created attachment 242719 [details]
Folder file with testcase messages
Comment 38 Boris Zbarsky [:bz] 2006-10-18 20:11:01 PDT
Created attachment 242720 [details] [diff] [review]
Patch with "addbook" world-accessible again, and updated to tip
Comment 39 Boris Zbarsky [:bz] 2006-10-18 20:26:24 PDT
Created attachment 242721 [details] [diff] [review]
Same as diff -w
Comment 40 Daniel Veditz [:dveditz] 2006-10-19 13:59:52 PDT
Comment on attachment 241290 [details] [diff] [review]
Updated to comments

>Index: netwerk/base/public/nsIProtocolHandler.idl
>+    /**
>+     * All protocol handlers must set one of the following four flags.

ObNit: Can you do something to set off this comment more, at least this
initial sentence. Otherwise it visually bleeds into the comments for the
next flag value. The doxygen /** probably does nothing here since there's
no code before the next /** section, or else it'll do the wrong thing
and coalesce the two blocks.

(I'm thinking ugly, for one example among many:
      /* +-----------------------------------------+
       * | All protocol handlers -MUST- set one of |
       * | the following four flags.               |
       * +-----------------------------------------+
       * <rest of comment>

>+     * If none of these four flags are set [...] there may
>+     * be run-time warning messages [...]

Might want to add that in a future version (Mozilla 2? unspecified?)
the warning will become an error.

>+     * The URIs for this protocol can be loaded by anyone.  For example, any
>+     * website should be allowed to trigger a load a URI for this protocol.
>+     * Typicaly protocols like "http" will set this flag.

more nits: "trigger a load a URI"  (missing 'of'?)
typo: "Typicaly", but I'd drop it anyway. Maybe replace with "Standard"
and/or "web-safe", maybe nothing.

>+     * The URIs for this protocol are not allowed to be loaded by anyone other
>+     * than sufficiently privileged code (for example, code which has the
>+     * system principal).  Various internal protocols should set this flag.

I'm not liking "sufficiently". maybe something like
  The URIs for this protocol are UNSAFE if loaded by untrusted (web) content
  and may only be loaded by privileged code. Various...

>+     * The URIs for this protocol point to resources that are part of the
>+     * application's user interface.  There are cases when such resources may
>+     * be made accessible to untrusted content such as web pages, so this is
>+     * less restrictive than URI_DANGEROUS_TO_LOAD but more restrictive than
>+     * URI_LOADABLE_BY_ANYONE.  See the documentation for
>+     * nsIScriptSecurityManager::CheckLoadURI.
>+     */
>+    const unsigned long URI_IS_UI_RESOURCE = (1<<8);

This explanation is a little confusing. If you're going to reference
nsIScriptSecurityManager should you mention ALLOW_CHROME specifically?

>+    // When using this, make sure that you actually want DISALLOW_SCRIPT, not
>+    // DISALLOW_INHERIT_PRINCIPAL
>     const unsigned long DISALLOW_SCRIPT = 1 << 3;

Thank you.

>Index: caps/src/nsScriptSecurityManager.cpp
>===================================================================
>+        NS_URIChainHasFlags(aURI, aURIFlags, &uriHasFlags);

I mostly agree with BenB about this name; NS_NestedURIHasFlags? Don't
care too much, though. Worried more out the perf implications of drilling
down into the URI chain multiple times for each of several flags, but
I guess we'll see.

>+        // resource: and chrome: are equivalent, securitywise
>+        // That's bogus!!  Fix this.  But watch out for
>+        // the view-source stylesheet?
>+        PRBool sourceIsChrome;

What breaks if you don't do this part? If a chrome: document wants to
link to resource: it's got system privileges and would have already
been allowed. We do have some default stylesheets (html.css, ua.css)
that are resource: urls with chrome bindings or imports. Other than this
resource: urls are no more privileged than file urls

>+                             nsIProtocolHandler::URI_IS_LOCAL_FILE,
>+                             &hasFlags);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if (hasFlags) {
>+        // resource: and chrome: are equivalent, securitywise
>+        // That's bogus!!  Fix this.  But watch out for
>+        // the view-source stylesheet?

Do you have to do this? Why should unprivileged chrome: have access to
chrome links? It looks like this was added in bug 168136 to get the mail
filter viewer working in bug 167891. At the time there was no "withPrincipal"
form of CheckLoadURI, and there was no "system principal can do anything it
wants" check at the top.

I bet we don't need this anymore, and wouldn't want it in any remaining
places where it would actually do something. But, for the sake of not holding
up the structural change this *is* equivalent to the existing code so let's
leave it. I filed bug 357195 on changing this in the future.

>+    // OK, everyone is allowed to load this.  Check whether we need to warn
...
>+#ifdef DEBUG
>+            fprintf(stderr, "%s\n", NS_ConvertUTF16toUTF8(message).get());
>+#endif

Couldn't you just have stuck an NS_WARNING after the "if (!hasFlags)"?
I don't see why the debug message needs to be localized. Maybe even
an NS_ASSERTION because we don't want unflagged protocol handlers
in our own tree.

Maybe a comment that unflagged handlers are deprecated, and at some point
will be an error. The assertion kind of does that.

>-    // If we reach here, we have an unknown protocol. Warn, but allow.
>-    // This is risky from a security standpoint, but allows flexibility

Nice that this goes away -- there are no more unknown protocols.
Either they're internal and have flags, or they get the generic
unknown external handler flags.

>+ProtocolFlagError = Warning: Protocol handler for '%S' doesn't advertise a security policy.

Do you want to stick "deprected" in there? Or just trust that developers
will look up the code where this is coming from an fix it?

>+  // Is URI_STD true of all GnomeVFS URI types?
>+  *aProtocolFlags = URI_STD | URI_LOADABLE_BY_ANYONE;

isn't moz-gnomevfs kind of an internal thing? If it were standard we
wouldn't have put the moz- prefix on it. with 'smb' being one of the
supported protocols that's like the file://host/path format, maybe
this should be URI_IS_LOCAL_FILE? maybe not strictly "local",
but probably an internal rather than public resource.

> NS_IMETHODIMP nsAddbookProtocolHandler::GetProtocolFlags(PRUint32 *aUritype)
> {
>-  *aUritype = URI_STD;
>+  *aUritype = URI_STD | URI_DANGEROUS_TO_LOAD;

As you point out this breaks VCard display. This should be loadable by anyone
for now... it's an annoyance/DOS but not otherwise exploitable.

> NS_IMETHODIMP nsCidProtocolHandler::GetProtocolFlags(PRUint32 *aProtocolFlags)
> {
>+  // XXXbz so why does this protocol handler exist, exactly?
>   return NS_ERROR_NOT_IMPLEMENTED;

To keep them from leaking out to the unknown protocol handler? Dunno, good question.

> NS_IMETHODIMP nsIconProtocolHandler::GetProtocolFlags(PRUint32 *result) 
> {
>-  *result = URI_NORELATIVE | URI_NOAUTH;
>+  *result = URI_NORELATIVE | URI_NOAUTH | URI_IS_UI_RESOURCE;

I'm fine with this, but moz-icon wasn't in the previous caps list meaning
it was loadable by everyone. May have some breakage from people who shouldn't
have been using this in the first place.

>   scheme: "moz-test",
>+                 Components.interfaces.nsIProtocolHandler.URI_DANGEROUS_TO_LOAD,

really? Is the test harness privileged?

r=dveditz, but I'd like answers/changes for at least gnomevfs and addbook
Comment 41 Boris Zbarsky [:bz] 2006-10-19 21:01:27 PDT
> ObNit: Can you do something to set off this comment more

Will do.

> Might want to add that in a future version (Mozilla 2? unspecified?)
> the warning will become an error.

OK.

> more nits: "trigger a load a URI"  (missing 'of'?)

Yes. Will fix.

>  The URIs for this protocol are UNSAFE if loaded by untrusted (web) content
>  and may only be loaded by privileged code. Various...

Good idea.  Will do.

> This explanation is a little confusing. If you're going to reference
> nsIScriptSecurityManager should you mention ALLOW_CHROME specifically?

If that's OK with darin, sure.

> I mostly agree with BenB about this name; NS_NestedURIHasFlags?

I'm not adding this method in this patch.  If we want a separate bug on renaming the method and changing all existing consumers, great.  But this patch is already big enough.

> Worried more out the perf implications

Very few URIs are actually nested.  I think it'll be ok...

> What breaks if you don't do this part?

No idea.  The goal was to not change existing functionality -- this patch is big and scary enough as is.  :)  Thanks for filing a followup on this!

I should note that I asked in mozilla.dev.security on Sept 22 about resource: and what it's about and what the security model is and got only silence as an answer.  :(

> I don't see why the debug message needs to be localized.

It doesn't need to, but since we're already doing it, why not?

> Maybe even an NS_ASSERTION

I want to be able to actually debug extensions in debug builds... (have to do it pretty often for various Gecko issues, memory leaks, etc).  If the extensions are following the published frozen nsIProtocolHandler API, we don't really want to be asserting continuously (and covering up whatever real problem is being debugged).  Imo.

That, and I really do subscribe to the "assert when the state should really not be hit, period", which is not what the situation is here.

> Do you want to stick "deprected" in there?

Sure.  Will do.

> isn't moz-gnomevfs kind of an internal thing? 

Hmm... Looks like it is, yes.  I must have gotten it confused with helper apps for protocols.  I can't figure out how this stuff is used, and it's not documented anywhere, so I'll make it DANGEROUS_TO_LOAD and if something breaks people can file bugs.  :)

> As you point out this breaks VCard display

Yes, that's why I posted the updated patch yesterday.... Not sure why you ended up reviewing the old version.

> I'm fine with this, but moz-icon wasn't in the previous caps list meaning
> it was loadable by everyone.

I'm pretty sure that's a security bug, fwiw.  I don't think the URI parsers for moz-icon were ever designed to deal with arbitrary input, for example.

> really? Is the test harness privileged?

It's running in xpcshell.  So yes.
Comment 42 Boris Zbarsky [:bz] 2006-10-19 21:18:50 PDT
Oh, I see what happened with the old patch.  We mid-aired, somehow... and on patches there is no mid-air detection (e.g. the "obsolete" flag also went away).
Comment 43 Boris Zbarsky [:bz] 2006-10-19 21:19:09 PDT
Created attachment 242832 [details] [diff] [review]
Updated to dveditz's comments
Comment 44 Boris Zbarsky [:bz] 2006-10-25 10:55:42 PDT
Created attachment 243491 [details] [diff] [review]
Updated to Places changes
Comment 45 Boris Zbarsky [:bz] 2006-11-10 20:36:45 PST
Fixed on trunk.  No effect on Tp, happy to report.
Comment 46 James Ross 2006-11-10 20:45:47 PST
(In reply to comment #10)
> (From update of attachment 239527 [details] [diff] [review] [edit])
> Silver, could you review the jsd changes?

It would have helped enormously had you remembered to CC me when you made this comment. FWIW, the code itself looks fine for both Vnk and CZ. However, I will have to test the x-jsd change hasn't broken it, when trunk decides to start building.
Comment 47 Boris Zbarsky [:bz] 2006-11-10 20:52:22 PST
> It would have helped enormously had you remembered to CC me when you made this
> comment.

Er.... I set a "review?" flag on you when I made that comment.  See the change history for this bug.  Did you not get the mail?
Comment 48 James Ross 2006-11-10 20:54:47 PST
I certainly don't remember getting any mail for it and, more to the point, I never reviewed the changes, according to the bug's history/comments. I'm only here now because I saw an unexpected check-in in mozilla/extensions/irc being picked up by my own automatic build system.

I'll let you know if x-jsd has been broken, though.
Comment 49 Boris Zbarsky [:bz] 2006-11-10 20:58:46 PST
Yeah, I know you never reviewed.  There was a limit to what I was willing to wait on for code that:

1)  Had already gotten review in general.
2)  I had tested as best I could.
3)  Was pretty obviously correct in its final incarnation (even taking into
    account the "need to work with Gecko 1.0" stuff that jsd has).
Comment 50 Boris Zbarsky [:bz] 2007-01-24 22:35:48 PST
This caused bug 368160.
Comment 51 Eric Shepherd [:sheppy] 2007-09-28 12:41:09 PDT
Is there someone familiar with this material that could contribute some documentation for MDC, or at least could write up some notes from which I could distill some documentation?  That would be faster than trying to parse all these comments and the code out and come up with them myself.

If nobody can do that, then I'll deal with it but it may take quite a while before it happens. :)
Comment 52 Boris Zbarsky [:bz] 2007-09-30 21:05:24 PDT
I mailed Eric a starting blurb.
Comment 53 Eric Shepherd [:sheppy] 2007-10-09 10:20:38 PDT
I've created a new nsIProtocolHandler reference document on MDC, which includes this information.  Please feel free to twiddle if there's more information required.

http://developer.mozilla.org/en/docs/nsIProtocolHandler

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