250730 - Bugzilla's URL field allows javascript: and data: URLs

Status: RESOLVED DUPLICATE of bug 276907

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Jesse Ruderman]

Bugzilla's URL field allows javascript: and data: URLs.  It's convenient, but
it's a security hole.

Impact is similar to 38862.

Comment 1

[Gervase Markham [:gerv]]

True - although this is perhaps less serious, because people would have to click
the hyperlinked words "URL" to trigger the code, and they can actually see text
beginning "javascript:" or "data:" in the URL textbox, which looks suspicious
(even if you hide the nastier stuff off the end, as Jesse has now done).
Clicking an attachment, on the other hand, is a blind action.

Gerv

Comment 2

[Dave Miller [:justdave]]

My proposal for this, rather than disabling it, is to make the content of the
URL field normal data displayed on the page inline rather than (or in addition
to) the edit box.  Just ensure that the entire thing is plainly visible.  Jesse
mentioned himself that he uses the field for that purpose to demonstrate bugs
(heck, he even did it on this one), so it's definitely got a heck of a lot of
legitimate use as well.  Alternately (or in addition?) Perhaps we can display a
warning next to the field in red text if it starts with javascript: or data:  ?

Comment 3

[Gervase Markham [:gerv]]

> My proposal for this, rather than disabling it, is to make the content of the
> URL field normal data displayed on the page inline rather than (or in addition
> to) the edit box.  Just ensure that the entire thing is plainly visible.

What, always? That would be ugly. And it's rather a sledgehammer solution for a
nut problem.

How about just not autolinkifying the words "URL" when it begins "javascript:"
or "data:"? Then, there's no way to activate the code apart from copying and
pasting into a new browser window. And, if you do that, you should know what you
are doing.

Gerv

Comment 4

[Jesse Ruderman]

Not everyone knows that pasting a javascript: or data: URL into the address bar
gives code in the URL access to the current page.  On the other hand, a bug
report could just as easily include a javascript: or data: URL in a comment.

Comment 5

[Gervase Markham [:gerv]]

Exactly. We can't really protect people at that level - it's the same as "Hey,
cool things happen if you copy this and paste it into the URL bar of your
banking window..."

The no-hyperlinking fix seems elegant and non-intrusive to me.

Gerv

Comment 6

[Dave Miller [:justdave]]

that sounds like a plan to me, too...  it's the least invasive of the stuff we
can do.  I guess that makes bug 232457 a WONTFIX, too. :)

For the record, here's what the comments auto-linking currently links:

afs cid ftp gopher http https irc mid news nntp prospero telnet view-source wais
mailto

Comment 7

[Gervase Markham [:gerv]]

So are we blacklisting javascript: and data:, or are we whitelisting the ones
the comments autolinkify? I'd prefer a blacklist, just because it doesn't
arbitrarily restrict Bugzilla's function. Is that secure?

Gerv

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Does this do the trick? We don't need to worry about leading whitespace - it's
trimmed on submit.

Gerv

Comment 9

[Dave Miller [:justdave]]

shell:  ???  ;)

Guess those type are up to the browser to block...

Comment 10

[Dave Miller [:justdave]]

Comment on attachment 153135 [details] [diff] [review]
Patch v.1

would it be safer to include the colon in the match string?

Comment 11

[Gervase Markham [:gerv]]

justdave: we could do. I doubt it makes much odds; I can't see a
javascript-wibble protocol getting invented any time soon :-) Any other review
comments?

Gerv

Comment 12

[Joel Peshkin]

What keeps this check from failing to detect " javascript"??  Wouldn't leading
whitespace throw it off?

Comment 13

[Gervase Markham [:gerv]]

No; I tested that. URLs don't work with leading whitespace anyway :-) Try
putting one into your test Bugzilla and see. 

I can't remember if it gets trimmed before the check, or it retains the space
and doesn't work. Either way, it's not a problem.

Gerv

Comment 14

[Dave Miller [:justdave]]

Holding approvals for security advisory.

Patch v1 applies cleanly to both 2.18 and the trunk, but it doesn't work on
2.16.  Anyone want to cook up a patch for 2.16?

Comment 15

[Dave Miller [:justdave]]

we're still waiting for a patch against 2.16 on this.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Attachment: version of patch for 2.16

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 158931 [details] [diff] [review]
version of patch for 2.16

This is just the previously reviewed patch modified to work on 2.16.  Vladd,
can you review this trivial equivalent to the 2.18/trunk patch you've already
reviewed?

Comment 18

[Vlad Dascalu]

Sure, done, thanks!

Comment 19

[Dave Miller [:justdave]]

OK, I'm not putting this in 2.16.7 because bug 38862 got mentioned in comment 0,
and that bug isn't ready to land yet.  I can't open this one until that one is
ready to be opened.  Impact of both is pretty much the same anyway, so they
might as well land together.

Comment 20

[Dave Miller [:justdave]]

This isn't going to make 2.16.8 beacuse bug 38862 is blocking it, and it's not
ready to go yet.

Comment 21

[Gervase Markham [:gerv]]

Dave: that doesn't seem like a good reason to me. The only reason this bug is
marked as depending on bug 38862 is because it got mentioned in the initial
comment. If that's really a problem, let's open another bug for this bug, copy
the patch over, and then open that one up at release time and keep this one closed.

It seems perverse to not protect users against one problem just because we can't
protect them against another...

Gerv

Comment 22

[Dave Miller [:justdave]]

fair enough, go for it.

Comment 23

[Gervase Markham [:gerv]]

*** This bug has been marked as a duplicate of 276907 ***

Comment 24

[Dave Miller [:justdave]]

Wish my head was clearer in the holidays.  All we have mentioned here is a bug
number.  We'll live.  The discussion here is much clearer than the brief "here's
the problem, here's the patch" on the clone bug.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> It seems perverse to not protect users against one problem just because we
> can't protect them against another...

It seems perverse to reduce usability while not offering real protection because the attacker will just use a different, equally obvious, vector...