315166 - <label class="text-link"> should be more careful with javascript:, data:, and chrome: URLs

Status: RESOLVED FIXED

(Toolkit Graveyard :: Security, enhancement)

Description

[Jesse Ruderman]

<label class="text-link"> should not allow links to chrome: URLs, and it should either disallow or be careful with javascript: and data: URLs.  If necessary, there can be "allowchrome" and "allowscript" attributes on links where the chrome code gets the URL from a trusted source.

See bug 315004 for an example of a security hole this would avoid.

See also bug 293363 for an even more general solution to this kind of problem.

Comment 1

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

Attachment: tentative patch

I decided to use a whitelist instead of a blacklist, since there are possibilities of abuse by using combinations of jar: and view-source: etc that I didn't want to get into. I don't know whether this is the right way to go, and I need to test it more, but comments are welcome.

Comment 2

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

Comment on attachment 202849 [details] [diff] [review]
tentative patch

Asaf pointed out that since this is only a safe-guard, trying to actively prevent people from shooting themselves in the foot is kinda silly.

Comment 3

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

Attachment: patch

This is probably better. I figured there should be some kind of indication for why the click failed, suggestions for improvements on that wording are welcome. This lets remote xul actually work, too.

Comment 4

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

Attachment: patch

Comment 5

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

Attachment: testcase

Comment 6

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

Attachment: patch

Comment 7

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

Attachment: better patch

After discussion with Jesse and Asaf, I think we all agreed that being able to open chrome, script, or data links using the text-link binding didn't really have a compelling use-case, and was possible by other means (onclick) for code that really did want to do it. This patch therefore disallows it entirely. This also uses checkLoadURI against "about:blank" instead of scheme comparisons, so it is more robust (handles nested protocols, for example).

Comment 8

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

Attachment: diff -uw

Comment 9

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

Attachment: better testcase

Comment 10

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

Attachment: better patch v2

Comment 11

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

Attachment: diff -w

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 207358 [details] [diff] [review]
better patch v2

r=me with a comment explaining when and why its safe to use win.open(href)

Comment 13

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

Attachment: updated patch

Comment 14

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

(In reply to comment #12)
> (From update of attachment 207358 [details] [diff] [review] [edit])
> r=me with a comment explaining when and why its safe to use win.open(href)

If newURI(href) fails (which is the only case where we'd reach the else block, other than for non-chrome), then window.open(href) shouldn't be an issue.

Comment 15

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

mozilla/toolkit/content/widgets/autocomplete.xml; new revision: 1.53;

Comment 16

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

mozilla/toolkit/content/widgets/text.xml; new revision: 1.18.2.4;

Comment 17

[Scott MacGregor]

hey Gavin, this patch broke text boxes in thunderbird. It looks like http urls are no longer getting sent to the desktop but instead are trying to get opened from a  window.open call which has no meaning in thunderbird (there is no navigator.xul to open). 

See:

http://forums.mozillazine.org/viewtopic.php?t=387499

for more details.

Comment 18

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

(In reply to comment #17)
> hey Gavin, this patch broke text boxes in thunderbird.

Is there a bug filed on that issue? If there is, please assign it to me.

Comment 19

[Scott MacGregor]

I filed Bug 328890.

Comment 20

[Daniel Veditz [:dveditz]]

Does this fix need to land on the 1.8.0 branch? chrome links seem to be blocked and data and javascript run with the correct privileges so I don't think it's necessary. Please nominate it if I'm misunderstanding something.

Comment 21

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

(In reply to comment #20)
> Does this fix need to land on the 1.8.0 branch?

No, this bug was more of a precautionary measure rather than addressing any specific problems, so I don't think it should land on the branch. I also think it should be removed from the security group. It's been fixed for a while, and it doesn't contain any security-sensitive information (certainly no more than bug 315004, which is already open).