On Samstag, 14. Juni 2008, Tim Starling wrote:
Markus Krötzsch wrote:
Thanks Tim! I have now backported your fixes in the current SVN version to the latest Semantic MediaWiki release 1.1.1. The updated release 1.1.2 can be obtained via Sourceforge [1] or SVN [2]. Updating existing SMW 1.1* installations can thus be done without moving to the development version, and without any additional side effects on the running wiki.
We will take care of preventing the "register_globals" issue in all future ...
The scanner picked up no register_globals vulnerabilities at all in the core. Part of the reason for this is that we changed our practices years ago to avoid explicit inclusion, by using the autoloader for almost everything. Make global functions into static member functions, make constants into class constants. By getting rid of include statements except in the autoloader, where it is properly guarded, you eliminate arbitrary inclusion vulnerabilities.
Yes, I mentally scheduled that change already. We currently have explicit includes left only in few cases.
It's also more efficient, especially for installations with no bytecode cache, because you can often load less code on average.
Sure, that's why most classes in SMW are autoloaded by now. On the sites we profile, we once had achieved SMW init times of below 0.2% with that. Alas, the new message management architecture (wfLoadExtensionMessages) with its large single message file brought that up to 10-15% on non-cached sites (we had used deferred on-demand message loading with one file per language before, so 95% of messages were not loaded in a normal run). See http://semanticweb.org/profileinfo.php?expand=Setup.php for current figures.
Note that even ParserFunctions make up 7% of MediaWiki execution time there on average inits, probably also due to message loading. Maybe there is some other reason here (unavoidable object unstubbing or the like), but these profiling figures look somewhat bad. In comparison, the whole semantic database querying on that data-heavy site makes up only 1.66% of execution time (SMWSQLStore::getQueryResult (SMW)), even though queries happen on every fifth call in average.
SemanticMediaWiki has "@note AUTOLOADED" for autoloaded classes. I'd like to see those comments removed, since they will be unnecessary once every single class in the extension is registered in the autoloader.
OK, we will autoload everything (to the extent possible, see below). My original reason for putting the comments in was that some developers used explicit includes for classes that already were autoloaded. My feeling is that code is the only thing any developer is bound to read at some point. We can put infos on autoloading on some web-page instead, but I am not sure if everybody will notice it there.
I have had troubles using autoloading in one case. The affected PHP file requires other third-party PHP files to be available, so it is only ever used when admins explicitly request it (in that case, all dependencies should be met). But Special:Version somehow still triggered this PHP file to be "autoloaded" which eventually lead to include errors since most sites did not have the extra PHP code available. Could it be that some special pages trigger all autoloaded classes to be included?
Guarding with if(defined('MEDIAWIKI')) has proven to be a fairly poor solution, since apparently nobody understands what it is there for, and in some extensions it was applied sporadically or incorrectly. We can enforce the use of if(defined('MEDIAWIKI')) by regular vulnerability scanning, but obviously it would be better if we could encourage the use of inherently secure practices.
I completely agree. One option would be to have some security guidelines and coding conventions that everyone who applies for SVN access is asked to read first. Much confusion could be prevented if one would know what part of the MW devel docs are a must-read for new developers.
Using .inc as a file extension for included PHP files would fix the problem more effectively than either autoloading or guarding.
Well, this would be easy enough to do, but I see MediaWiki is not using that option either. Have there been any problems with that practice? Does it work fine on all sites?
-- Markus