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

RESOLVED FIXED in mozilla1.8.1

Status

--
enhancement
RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: jruderman, Assigned: Gavin)

Tracking

({fixed1.8.1})

1.8 Branch
mozilla1.8.1
fixed1.8.1
Bug Flags:
blocking1.8.0.8 -

Details

(Whiteboard: [sg:want P3], URL)

Attachments

(2 attachments, 9 obsolete attachments)

(Reporter)

Description

13 years ago
<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.
Created attachment 202849 [details] [diff] [review]
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.
Assignee: dveditz → gavin.sharp
Status: NEW → ASSIGNED
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.
Attachment #202849 - Attachment is obsolete: true
Created attachment 203679 [details] [diff] [review]
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.
Created attachment 203681 [details] [diff] [review]
patch
Attachment #203679 - Attachment is obsolete: true
OS: MacOS X → All
Hardware: Macintosh → All
Attachment #203681 - Attachment is obsolete: true
Created attachment 203754 [details] [diff] [review]
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).
Attachment #203748 - Attachment is obsolete: true
Created attachment 203756 [details]
better testcase
Attachment #203682 - Attachment is obsolete: true
Created attachment 207358 [details] [diff] [review]
better patch v2
Attachment #203754 - Attachment is obsolete: true
Attachment #203755 - Attachment is obsolete: true
Attachment #207358 - Flags: second-review?(bugs.mano)
Attachment #207358 - Flags: first-review?(mconnor)
Whiteboard: [patch-r?]
(Reporter)

Updated

13 years ago
Whiteboard: [patch-r?] → [sg:want P3] [patch-r?]
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)
Attachment #207358 - Flags: second-review?(bugs.mano)
Attachment #207358 - Flags: first-review?(mconnor)
Attachment #207358 - Flags: first-review+
Attachment #207358 - Attachment is obsolete: true
Attachment #207359 - Attachment is obsolete: true
(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.
Whiteboard: [sg:want P3] [patch-r?] → [sg:want P3]
mozilla/toolkit/content/widgets/autocomplete.xml; new revision: 1.53;
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Attachment #210665 - Flags: branch-1.8.1?(mconnor)

Updated

13 years ago
Attachment #210665 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
mozilla/toolkit/content/widgets/text.xml; new revision: 1.18.2.4;
Keywords: fixed1.8.1
Target Milestone: --- → mozilla1.8.1

Comment 17

13 years ago
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.
(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

13 years ago
I filed Bug 328890.
Depends on: 328890
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.
Flags: blocking1.8.0.8-
(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).
Group: security
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.