Last modified: 2012-12-17 10:11:33 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T26199, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 24199 - DynamicPageList2 has security issues
DynamicPageList2 has security issues
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
DynamicPageList2 (Other open bugs)
unspecified
All All
: Normal critical with 1 vote (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-01 04:04 UTC by Bawolff (Brian Wolff)
Modified: 2012-12-17 10:11 UTC (History)
5 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Bawolff (Brian Wolff) 2010-07-01 04:04:40 UTC
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.
Comment 1 p858snake 2010-07-01 05:53:38 UTC
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?
Comment 2 [[kgh]] 2010-08-20 21:54:40 UTC
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.
Comment 3 Bawolff (Brian Wolff) 2010-08-21 18:55:18 UTC
(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.
Comment 4 [[kgh]] 2010-08-23 21:14:38 UTC
(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?
Comment 5 Laurence 'GreenReaper' Parry 2010-08-23 21:27:21 UTC
> 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.
Comment 6 Bawolff (Brian Wolff) 2010-08-23 21:33:09 UTC
(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.
Comment 7 [[kgh]] 2010-08-23 21:34:49 UTC
(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
Comment 8 Jan Schejbal 2012-08-08 00:34:12 UTC
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.
Comment 9 p858snake 2012-08-08 07:01:10 UTC
(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.
Comment 10 Bawolff (Brian Wolff) 2012-08-08 12:09:10 UTC
(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.
Comment 11 jon_wiki 2012-12-16 01:10:01 UTC
Version 2.01 claims it "resolves all former security_issues"

Is this correct? 

(And does it work with MW 1.20.2?)

Cheers
Comment 12 jon_wiki 2012-12-16 01:11:51 UTC
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)
Comment 13 Bawolff (Brian Wolff) 2012-12-16 03:44:09 UTC
(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.
Comment 14 Bawolff (Brian Wolff) 2012-12-16 03:45:33 UTC
>*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)
Comment 15 Andre Klapper 2012-12-17 10:11:33 UTC
Yes. :)

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links