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

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

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

> 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

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.

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

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

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

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

shell: ??? ;) Guess those type are up to the browser to block...

Comment on attachment 153135 [details] [diff] [review] Patch v.1 would it be safer to include the colon in the match string?

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

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

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

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?

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

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?

Sure, done, thanks!

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.

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

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

fair enough, go for it.

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

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.

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