467852 - View Source Linkification should only linkify "safe" URLs.

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Curtis Bartley [:cbartley]]

View Source should only linkify "safe" URLs.  In particular, javascript: URLs should not be linked, and nested URIs that ultimately resolve to "javascript:" URLs should not be linked.

Note that simple "view-source:javascript" links don't work in the view-source window, but they shouldn't be linked anyway.

In general linkification should be restricted to a whitelist of supported schemes, e.g. http, https, file, ftp.

Comment 1

[Tony Chung [:tchung]]

do we have a whitelist of these supported protocols someplace?

Comment 2

[Curtis Bartley [:cbartley]]

(In reply to comment #1)
> do we have a whitelist of these supported protocols someplace?

Not to my knowledge.  I was thinking we could just create a list of the obviously safe ones and that would easily hit 99+ percent of the real world cases.

Of course it might be possible to piggyback on top of existing URI security infrastructure rather than relying on an explicit whitelist.  I'm hoping someone more knowledgeable will comment.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Depends on what you mean by "safe", I guess.

You could use nsINetUtil.URIChainHasFlags(uri, URI_INHERITS_SECURITY_CONTEXT) which is basically equivalent to "is innermost URI a javascript:, data: or wyciwyg: URI" in practice (ignoring custom protocol handlers), but maybe checking for URI_LOADABLE_BY_ANYONE would be better.

Comment 4

[Boris Zbarsky [:bzbarsky]]

What exact problems are we trying to address?  Like Gavin says, the definition of "safe" here is crucial.

I am strongly opposed to having a list of protocols here (or anywhere else).  If we need to add flags to nsIProtocolHandler, we should do that.

Comment 5

[Curtis Bartley [:cbartley]]

(In reply to comment #4)
> What exact problems are we trying to address?  Like Gavin says, the definition
> of "safe" here is crucial.
> 
> I am strongly opposed to having a list of protocols here (or anywhere else). 
> If we need to add flags to nsIProtocolHandler, we should do that.

Now that I think about it, I'm conflating two different (but similar) issues in my head.

1. The security issue.  In particular we don't want javascript: URLs running inside the view source window, because a) that doesn't make sense anyway, and b) they will be (I think) running inside a chrome window.  There might be other security issues that I don't understand here with other protocol types.

2. Currently View Source linkification creates links which are prefixed with "view-source:" (e.g. view-source:http://www.example.com/) so that if the user clicks on a link to an HTML page, it will be opened as source rather than rendered.  It might be debatable whether this is really what the user wants most of the time, but as long as the new page opens inside the view source window, I think defaulting to source view makes the most sense.

The current code suffers from the problem that it prefixes all URLs, regardless of scheme, so you can get things like "view-source:mailto:[e-mail]".  This doesn't seem to hurt anything (a view-source:mailto: link works just like a mailto link), but it sure seems icky.  In particular there might be other schemes where prefixing view-source: doesn't make sense and where the behavior is less friendly.

So anyway, if #2 is an issue, it's not really a security issue and should probably be spun out as a separate bug.  I'm elaborating here since it seems like the right way to handle this issue is probably a list of protocols/schemes that should be view-source:'d, which bz contends is bad in general.

I've been thinking about the two issues as one because the solution I was thinking of (a scheme whitelist) seemed to solve both of them.

I'm thinking now that I should spin issue #2 out to a separate bug and we can address the solution to that problem over there.

Opinions?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Issue #2 is a matter of checking for the URI_DOES_NOT_RETURN_DATA protocol flag.  If it's set, don't linkify (since view-source of such a URI is not reasonable).

Issue #1 for javascript: is a non-issue due to the fix for bug 204779 (and bug 290982).  We'll linkify the url but do nothing when the link is clicked.  If desired, we coudl move the check to newURI from channel init, and then even the linkification won't happen.  This is worth doing and writing a unit test for, imo.

We'd need to figure out what other issues one can expect to decide what to do with them.

Comment 7

[Curtis Bartley [:cbartley]]

This issue was raised in the security review.

Comment 8

[Curtis Bartley [:cbartley]]

Attachment: Use URI_INHERITS_SECURITY_CONTEXT and URI_DOES_NOT_RETURN_DATA flags to determine when and how to create view-source links

Use URI_INHERITS_SECURITY_CONTEXT and URI_DOES_NOT_RETURN_DATA flags to determine when and how to create view-source links.  In particular "javascript:" and "data:" links are not linkified and "mailto:" links are not prefixed with "view-source:".

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm still confused.  Why are we checking the INHERITS_SECURITY_CONTEXT bit?  What's the threat?  What are we protecting against?  Why shouldn't this work for data:?

Comment 10

[Curtis Bartley [:cbartley]]

(In reply to comment #9)
> I'm still confused.  Why are we checking the INHERITS_SECURITY_CONTEXT bit? 
> What's the threat?  What are we protecting against?  Why shouldn't this work
> for data:?

1. We don't even want to linkify certain types of URLs -- most notably javascript: URLs -- since there's nothing reasonable we can do with them anyway.  The INHERITS_SECURITY_CONTEXT prevents that without explicitly testing for "javascript:" URLs.
2. One issue that was raised in the security review is that although it looked like javascript: URLs were blocked even in the original implementation, there was always the risk that a new protocol type might be introduced later that wasn't blocked even though it should be.  Another layer of defense here seemed warranted, and at the very least shouldn't hurt anything.

Now as to whether data: URLs should be excluded, I don't know.  Perhaps INHERITS_SECURITY_CONTEXT isn't the right flag to be checking.  Gavin suggested URI_LOADABLE_BY_ANYONE as an alternative, but when I tested using that flag, javascript: URLs weren't blocked.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> The INHERITS_SECURITY_CONTEXT prevents that without explicitly testing
> for "javascript:" URLs.

That's too wide a net.  It also catches data: URLs, and possibly others, which we _can_ do useful things with.  I would argue that the only thing you really want to block here is "protocols that will execute script", and the right place to block those is not in this code but in the view-source: handler (where we already block javascript:).  If we're worried about other such schemes being added, we should be changing the check there.  If we don't want the links to look clickable, we should be moving the check to newURI, as I suggested above.

Comment 12

[Boris Zbarsky [:bzbarsky]]

And I can't tell you what the right thing is to be checking, or even whether we have such a right thing, until we articulate exactly what the problem with javascript: URLs here is.

Comment 13

[Curtis Bartley [:cbartley]]

(In reply to comment #12)
> And I can't tell you what the right thing is to be checking, or even whether we
> have such a right thing, until we articulate exactly what the problem with
> javascript: URLs here is.

1. I want to make sure that there is no way for linkification to produce a link which will run script when the user clicks on it.  I don't know if it's possible for such a script to do anything malicious if it runs in the context of the viewsource window, but since I can't think of any useful reason for a script to be running here, I'd prefer to simply make it impossible.

2. If we know that a particular link generated by linkification is going to be prevented from doing anything by a security check somewhere, I'd prefer to not generate the link at all, simply as a matter of good UI practice.  So for example, I already know that empirically "view-source:javascript" URLs don't do anything.  Nevertheless I'd prefer not to generate such links since I know a priori they're not going to work.

3. In cases of URLs like mailto:, it makes sense to linkify them, but it doesn't really make sense to prefix them with view-source: since there's no source to view.  Also, I know empirically that a "view-source:mailto:" URL works just like a "mailto:" URL, but I don't know if view-source: is explicitly designed to work with mailto: URLs or if it just sort of coincidentally works the way I want.  If we know that slapping a view-source: in front of a URL makes no behavioral difference, it seems like good form to just use the undecorated source URL for linkification, although I could imagine plausible arguments otherwise.  Hopefully the URI_DOES_NOT_RETURN_DATA test does what I want here.

4. I'd like to think that the only dangerous URLs are javascript URLs or javascript URLs embedded in another type of nested URL.  Unfortunately I have this uneasy feeling that there are other well known URL attack vectors that I'm just not familiar with.  I don't know the full breadth of URL schemes supported by Firefox and that hampers me here.  As a consequence I tend towards trying to find a conservative solution, at least for 3.1.  Find what we can definitely safely allow, and make sure we just allow that for now.  If I understood the security architecture around URIs better, I might lean more towards a generic solution that leverages the existing architecture.  The flags in nsIProtocolHandler.idl are pretty well documented in one sense, but on the other hand, if you take URI_INHERITS_SECURITY_CONTEXT as an example, the documentation is only meaningful if you understand what a security context is and how it works inside the application.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Curtis, thanks for explaining what the concerns are.

> I want to make sure that there is no way for linkification to produce a link
> which will run script when the user clicks on it.

OK.  This sounds like a good goal.  We can either check for javascript: explicitly (as we already do) or add a protocol handler flag for this check.

> simply as a matter of good UI practice.

Fully agreed, and this is why I think we should change view-source to do the check in newURI instead of newChannel.  That will work not only for view-source linkification but in all other places where such a URI is used in a link.

> 3. In cases of URLs like mailto:

This is covered by the URI_DOES_NOT_RETURN_DATA flag, and I agree that we should not linkify things with that flag.  I have no problems with that part of this patch.

> I don't know the full breadth of URL schemes supported by Firefox

No one does, since extensions can implement URL schemes.  We can only assume that such extensions will set the right protocol handler flags for their protocol; if they don't we're in for a world of hurt in all sorts of ways.

It sounds to me like we should add a flag for "opening this channel will execute code provided in the URI" and use it here.  I'd really like us to allow browsing to linkified data: URIs, since that's actually a pretty common use case for a lot of our developers.

Comment 15

[Curtis Bartley [:cbartley]]

> Fully agreed, and this is why I think we should change view-source to do the
> check in newURI instead of newChannel.  That will work not only for view-source
> linkification but in all other places where such a URI is used in a link.

Just so I'm clear on this -- you're suggesting a modification of the *implementation* of NS_NewURI, right?

> It sounds to me like we should add a flag for "opening this channel will
> execute code provided in the URI" and use it here.  I'd really like us to allow
> browsing to linkified data: URIs, since that's actually a pretty common use
> case for a lot of our developers.

This would be another nsIProtocolHandler flag like URI_DOES_NOT_RETURN_DATA and URI_INHERITS_SECURITY_CONTEXT, correct?

If we add such a flag, then as I understand it we'd need to look at the schemes we currently support and see if any of them need to set this new flag.  Presumably most schemes obviously wouldn't need it so this might not be a big deal, but we'd need to devote at least some thought to it to make sure we didn't miss a case.

Can we make a change like this and expect to get it approved for Firefox 3.1?  (I'm just asking because I'm a newbie, and it's hard for me to judge.)

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Just so I'm clear on this -- you're suggesting a modification of the
> *implementation* of NS_NewURI, right?

The implementation of nsViewSourceHandler::NewURI, yes.

> This would be another nsIProtocolHandler flag like URI_DOES_NOT_RETURN_DATA
> and URI_INHERITS_SECURITY_CONTEXT, correct?

Yep.

> we'd need to look at the schemes we currently support and see if any of them
> need to set this new flag. 

In theory, yes.  In practice, javascript: is the only scheme that would need it in our core code.

> Can we make a change like this and expect to get it approved for Firefox 3.1? 

I think so, yes.

Comment 17

[Curtis Bartley [:cbartley]]

Attachment: Use URI_DOES_NOT_RETURN_DATA flag to determine how to create view-source links

This is one half of the previous patch (patch 353716).  It uses the URI_DOES_NOT_RETURN_DATA flag to determine when a URL should be prefixed with "view-source:" when we do linkification.  For example, we want "view-source:http://www.example.com", but "mailto:[e-mail]".

I'm splitting the patch up because we're discussing a better way to do the other half of the patch.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 354267 [details] [diff] [review]
Use URI_DOES_NOT_RETURN_DATA flag to determine how to create view-source links

>diff -r 537be0b8a439 parser/htmlparser/src/nsViewSourceHTML.cpp
>+  // URLs that don't return data (e.g. mailto: URLs) should just be returned
>+  // undecorated.  URLs that do return data (e.g. http: URLs) should be prefixed
>+  // with "view-source:".
>+  if (UriDoesNotReturnData(hrefURI)) {
>+    // do nothing
>+  } else {
>+    viewSourceUrl.AssignLiteral("view-source:");    
>+  }

Is there any reason to not invert the sense of the if statement to avoid the empty body? Perhaps inverting the sense of the function name would help, too.

>+PRBool CViewSourceHTML::UriDoesNotReturnData(nsIURI* uri) {
>+  PRBool doesNotReturnData = PR_FALSE;
>+  nsresult rv = 
>+  NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, &doesNotReturnData);

Nit: please indent this one tab (since it's a continuation). Also, I'd do something like:

nsresult rv =
  NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA,
                      &doesNotReturnData);

to preserve the 80th column.

Comment 19

[Curtis Bartley [:cbartley]]

Comment on attachment 354267 [details] [diff] [review]
Use URI_DOES_NOT_RETURN_DATA flag to determine how to create view-source links

> >diff -r 537be0b8a439 parser/htmlparser/src/nsViewSourceHTML.cpp
> >+  // URLs that don't return data (e.g. mailto: URLs) should just be returned
> >+  // undecorated.  URLs that do return data (e.g. http: URLs) should be prefixed
> >+  // with "view-source:".
> >+  if (UriDoesNotReturnData(hrefURI)) {
> >+    // do nothing
> >+  } else {
> >+    viewSourceUrl.AssignLiteral("view-source:");    
> >+  }

Is there any reason to not invert the sense of the if statement to avoid the
empty body? Perhaps inverting the sense of the function name would help, too.

*** Done. ***

> >+PRBool CViewSourceHTML::UriDoesNotReturnData(nsIURI* uri) {
> >+  PRBool doesNotReturnData = PR_FALSE;
> >+  nsresult rv = 
> >+  NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, &doesNotReturnData);

Nit: please indent this one tab (since it's a continuation). Also, I'd do
something like:

nsresult rv =
  NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA,
                      &doesNotReturnData);

to preserve the 80th column.

*** Done. ****

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 354758 [details] [diff] [review]
Use URI_DOES_NOT_RETURN_DATA flag to determine how to create view-source links (mk II)

The double negative still rankles a little, but that's OK since it's following the flag name. r=mrbkap

Comment 21

[Curtis Bartley [:cbartley]]

Attachment: Use URI_XXX flags to determine how and when to create view-source links

OK, this patch uses the URI_DOES_NOT_RETURN_DATA flag to identify links that should be used as-is, rather than prefixed with "view-source:".  This part of the patch is unchanged from the previous patch.  It also contains logic which ignores "javascript:" URLs altogether, so they're not linkified at all.  This logic uses the new URI_MAY_EXECUTE_SCRIPT flag, rather than testing for "javascript:" explicitly.

I think that the only thing here that might be controversial is the name and implied semantics of the new flag.

Bz, what do you think?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Maybe call it URI_OPENING_EXECUTES_SCRIPT?  And s/may// in the comment?  No need for the "canonical example" part of that comment, I think.

Also, in failure cases you'd want to assume "true" for "can execute script", right?

And while you're here, are you willing to file a followup on making the view-source protocol handler change I talked about above?  That is, moving the check to newURI and making it use this flag?

Comment 23

[Curtis Bartley [:cbartley]]

Attachment: Use URI_XXX flags to determine how and when to create view-source links (v2)

Renamed the flag to URI_OPENING_EXECUTES_SCRIPT.

Also inlined the UriDoesNotReturnData() and UriMayExecuteScript() functions to make error handling more explicit.

Comment 24

[Curtis Bartley [:cbartley]]

(In reply to comment #22)

> And while you're here, are you willing to file a followup on making the
> view-source protocol handler change I talked about above?  That is, moving the
> check to newURI and making it use this flag?

Bug 472547.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 355825 [details] [diff] [review]
Use URI_XXX flags to determine how and when to create view-source links (v2)

r=mrbkap, I think bz should sr (though I'll let cbartley ask for himself so he gets the mail).

Comment 26

[Johnathan Nightingale [:johnath]]

Checked in on mozilla-central per Curtis' request.

http://hg.mozilla.org/mozilla-central/rev/0db00d61c314

Will still need landing on branch after baking.

Comment 27

[Curtis Bartley [:cbartley]]

Patch applied cleanly to 1.9.1 as of a few minutes ago.

Comment 28

[Nochum Sossonko [:Natch]]

Comment on attachment 355825 [details] [diff] [review]
Use URI_XXX flags to determine how and when to create view-source links (v2)

This is blocking 1.9.1, doesn't need approval.

Comment 29

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7716444f4f91