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
--
Markus Krötzsch
Institut AIFB, Universität Karlsruhe (TH), 76128 Karlsruhe
phone +49 (0)721 608 7362 fax +49 (0)721 608 5998
mak(a)aifb.uni-karlsruhe.de www
http://korrekt.org