319091 - disabledtext accepts HTML and thus <script>

Status: VERIFIED WONTFIX

(Bugzilla :: User Accounts, defect)

Description

[Max Kanat-Alexander]

From Tim Brown <[e-mail]>:

Bugzilla 2.18.4, 2.20

Injection of malicious Javascript is possible by modification of the disabledtext parameter of editusers.cgi.  The contents of this will then be executed on the users browser when they next attempt to login.

---------
I'm honestly not *too* worred about this, since only admins with editusers can changed disabledtext, and having editusers basically makes you a super-admin.

That is, this bug is similar to saying "An admin can install Bugzilla and put a malicious <script> tag on the front page if he wants."

Still, I don't see any reason for disabledtext to allow HTML.

Comment 1

[Marc Schumann [:Wurblzap]]

WONTFIX imo. I want the disabledtext to allow live HTML, think
'You've been blocked because of <strong>Ad Spamming</strong> bug 1234.
<a href="/blockpolicy-adspam.html">Please see our policy about this.</a>'.

We allow it for the similar reasoning as for why we allow live HTML in descriptions for classifications, products and components, thus trusting editcomponents people: we trust editusers people not to attempt malicious XSS.

Comment 2

[Frédéric Buclin]

I agree with Marc. Suggesting WONTFIX too. What we really should do is to have a whitelist of allowed HTML tags, as suggested by justdave. But even in this case, I could add a link to a hacked page containing JS in it on the same server, see bug 38862 for a testcase.


<justdave> what we should really do for those fields is put a whitelist of allowed HTML tags and refuse the update if they put ones in that aren't on that list.
<justdave> as for why it's a security issue, that's only because we don't have any form submission security
<justdave> (to ensure that the submit actually came from our form)

Comment 3

[Max Kanat-Alexander]

OK. I think that perhaps we should keep this around as a "blocker will fix" sort of thing on whitelisting HTML tags.

Comment 4

[Gervase Markham [:gerv]]

disabledtext can only be changed by people who are trusted. I can't see ever "doing an HTML parser and whitelist of tags for disabledtext" being at the top of anyone's priorities. I also suggest WONTFIX.

Gerv

Comment 5

[Max Kanat-Alexander]

OK. Consensus is very apparently WONTFIX. :-)

Comment 6

[Zach Lipton [:zach]]

While you can use modules like HTML::StripScripts to implement a whitelist, it is still highly non-trivial to get right. Even if you disallow <script>, you still have to be careful to disallow onLoad, onClick, and all the other event handlers which can be added as html attributes. 

Comment 7

[Vlad Dascalu]

Adding reporter for bug access.

Comment 8

[Tim Brown]

(In reply to comment #6)
> While you can use modules like HTML::StripScripts to implement a whitelist, it
> is still highly non-trivial to get right. Even if you disallow <script>, you
> still have to be careful to disallow onLoad, onClick, and all the other event
> handlers which can be added as html attributes. 
> 

It's hardly a whitelist if you start talking about disallowing <script>.  The concept of a whitelist is that you explicitly state everything you *will* allow.  You may disagree, but I don't think it acceptable to allow arbitrary code.  I'll give you an example, someone compromises a Bugzilla database, and disables every user with code such as <script>...code to perform local exploit of IE/Mozilla...</script>.  Users try to log in, and they themselves get compromised.  At the very least, Bugzilla is a method for the delivery of a payload, albeit that in this case to inject the payload admin privileges is required.  Of course admin privileges might be quite easy to obtain given the session handling weaknesses in Bugzilla or the existence of https://bugzilla.mozilla.org/show_bug.cgi?id=319085.