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?

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.

Why can't we do the protocol handler query instead of hardcoding thing in 0.9.9? /be

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.

Let's try to do this for 1.4 alpha.

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

What sort of security info would we want to expose on the protocol handlers?

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?

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 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 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.

This makes some of the nsScriptSecurityManager changes easier to review, I think.

> 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.

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.

> 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?

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. :(

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.

> 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 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

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?

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.

> > 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?

> 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.

I'll get to this today Boris.

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" ... ??

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 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.

(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.

(In reply to comment #30) > > cid is defined in RFC 2111: http://www.ietf.org/rfc/rfc2111.txt > Obsoleted by RFC 2392.

> 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.

> > <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).

> 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?

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?

sure, you can put my message in the bug.

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

> 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.

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).

Fixed on trunk. No effect on Tp, happy to report.

(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.

> 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?

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.

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).

This caused bug 368160.

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. :)

I mailed Eric a starting blurb.

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