Page MenuHomePhabricator

DynamicPageList2 has security issues
Closed, ResolvedPublic

Description

I fixed some XSS vulnerabilities in r68811 - However I still feel there are problems with this extension.

*The playing with $wgRawHtml - this in itself is not a security vulnerability, but makes it easy to give yourself problems. before r68811 the following:
<DPL>

category = Africa
 count= 2
resultsfooter=<html><script>alert('d')</script></html>

</DPL>
Did bad things because resultsfooter was interpreted as if $wgRawHtml was on. I think I got most of those types of issues in r68811, but I am not really familiar with the extension's options at all, so its quite likely i missed something (esp for the find and replace options).
**The approach of using wiki-syntax mixed with <html> sections seems like a bad idea. It seems as if it'd be better to use either wiki-syntax only or html only then you wouldn't have to worry about escaping for both ways (but thats just my opinion after reading the code for 10 minutes, perhaps there is valid reason to do that)...
*The ordercollation option does not seem to be escaped when put in the sql...

This is just after a brief scan through the code when trying to fix Bug 22675 - I wouldn't be surprised if there are other issues.


Version: unspecified
Severity: critical

Details

Reference
bz24199

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:08 PM
bzimport set Reference to bz24199.
bzimport added a subscriber: Unknown Object (MLST).

Could we perhaps merge the two DPLs together, and bring over what ever is missing from the WMF in a sane manner then just disable those functions by default?

It seems as something happened to $wgRawHtml in MW 1.16.0 Either it is now depreciated or it is broken, since extensions using it (true) do not work any longer. In the first case Manual:$wgRawHtml should be updated in the latter a new bug should be filed. However the release notes of ME 1.16.0 do not mention $wgRawHtml. Still I am not aware what happended exactly.

(In reply to comment #2)

It seems as something happened to $wgRawHtml in MW 1.16.0 Either it is now
depreciated or it is broken, since extensions using it (true) do not work any
longer. In the first case Manual:$wgRawHtml should be updated in the latter a
new bug should be filed. However the release notes of ME 1.16.0 do not mention
$wgRawHtml. Still I am not aware what happended exactly.

It changed at which point the variable was looked at (its looked at when the parser is initialized, not at parse time), so extensions abusing it in certain ways stopped working. I think this change happened at r61913.

Personally I think extensions messing with it seems like an inherently bad idea, and I can't think of one good reason that an extension should set it.

(In reply to comment #3)

It changed at which point the variable was looked at (its looked at when the
parser is initialized, not at parse time), so extensions abusing it in certain
ways stopped working. I think this change happened at r61913.

Personally I think extensions messing with it seems like an inherently bad
idea, and I can't think of one good reason that an extension should set it.

Ah, I see. Thanks for your information. Shouldn't this be worth a bug requesting the depreciation of $wgRawHtml since there seems to be a consensus on this?

Shouldn't this be worth a bug requesting the depreciation of $wgRawHtml
since there seems to be a consensus on this?

Not sure how you came to that conclusion. A lot of people out there use $wgRawHtml for one reason or another. Perhaps in an ideal world they would have found a more secure way to do whatever they wanted to do, but if it doesn't exist or they can't figure out how to get it working, this is a mechanism for them to achieve their goal. They are warned of the potential consequences and offered other options where available.

(In reply to comment #4)

Ah, I see. Thanks for your information. Shouldn't this be worth a bug
requesting the depreciation of $wgRawHtml since there seems to be a consensus
on this?

(note: I'm not someone of importance, so my opinion doesn't matter, but...) $wgRawHtml does what its supposed to (allow normal editors to add <html> sections). I do not believe it was ever supposed to be set by extensions (there are other ways for extensions to output html), and its functionality has not changed.

(In reply to comment #5)

Not sure how you came to that conclusion. A lot of people out there use
$wgRawHtml for one reason or another. Perhaps in an ideal world they would have
found a more secure way to do whatever they wanted to do, but if it doesn't
exist or they can't figure out how to get it working, this is a mechanism for
them to achieve their goal. They are warned of the potential consequences and
offered other options where available.

EC I have to admit that I was a bit provocative with my reply. I just was not sure
what to think about this from what I read. Now it is clear. Thank you for your
reply

jan-bugreport wrote:

I was able to perform XSS on revision 72454 and have no reason to believe this wouldn't work with current versions. I do not want to publicly disclose the exploit. That $wgRawHtml hack really needs to go away. Setting such a global variable and never changing it back (!) sounds like a great way to cause nasty security issues everywhere.

I have set severity=critical, priority=normal, please correct it if that was wrong.

(In reply to comment #8)

I do not want to publicly disclose the
exploit.

Please file a bug under the "Security" product about that so its private and only visible to our security group.

(In reply to comment #9)

(In reply to comment #8)

I do not want to publicly disclose the
exploit.

Please file a bug under the "Security" product about that so its private and
only visible to our security group.

Note, its not exactly secret that their are open XSS issues with this extension. They are very obvious when you look at the code (hence the giant notice on the extension description page).

I somewhat doubt the "Security" group plans to rewrite the entire extension.

jon_wiki wrote:

Version 2.01 claims it "resolves all former security_issues"

Is this correct?

(And does it work with MW 1.20.2?)

Cheers

jon_wiki wrote:

I should add that it is the extension called "Extension:DynamicPageList (third-party)" which makes this claim - I understand that DPL2 (the category on bugzilla) is the old name for this. URL: http://www.mediawiki.org/wiki/Extension:DynamicPageList_(third-party)

(In reply to comment #11)

Version 2.01 claims it "resolves all former security_issues"

Is this correct?

(And does it work with MW 1.20.2?)

Cheers

It resolved the known issues as far as i know. Nobody has exactly done a security audit on the extension, but the old issues that were reported have been fixed.

With that in mind, this bug could probably be closed as fixed.

*The ordercollation option does not seem to be escaped when put in the sql...

Actually I didn't double check that issue was actually fixed when I briefly looked through the code several months ago (Not saying it isn't fixed, I just haven't checked)