[ postfixadmin-Bugs-1838327 ] Inconsistent include statements
Brought to you by:
christian_boltz,
gingerdog
Menu
▾
▴
[ postfixadmin-Bugs-1838327 ] Inconsistent include statements
|
From: SourceForge.net <no...@so...> - 2007-11-26 23:59:03
|
Bugs item #1838327, was opened at 2007-11-26 01:29 Message generated for change (Comment added) made by christian_boltz You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=937964&aid=1838327&group_id=191583 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: Core Group: SVN (please specify revision!) >Status: Closed >Resolution: Fixed Priority: 1 Private: No Submitted By: Jan Örnstedt (ornstedt) Assigned to: Nobody/Anonymous (nobody) Summary: Inconsistent include statements Initial Comment: The code is quite inconsisten in the include statements. Se example below. I suggest to use one type of reference in all places and not mix them. I strongly suggest removing the variable from the include statement as this variable can be manipulated from a webform. See example below: edit-domain.php:include ("templates/header.tpl"); edit-domain.php:include ("templates/menu.tpl"); edit-domain.php:include ("templates/admin_edit-domain.tpl"); edit-domain.php:include ("templates/footer.tpl"); edit-mailbox.php:include ("$incpath/templates/header.tpl"); edit-mailbox.php:include ("$incpath/templates/menu.tpl"); edit-mailbox.php:include ("$incpath/templates/edit-mailbox.tpl"); edit-mailbox.php:include ("$incpath/templates/footer.tpl"); edit-vacation.php:include ("$incpath/templates/header.tpl"); edit-vacation.php:include ("$incpath/templates/menu.tpl"); edit-vacation.php:include ("$incpath/templates/edit-vacation.tpl"); edit-vacation.php:include ("$incpath/templates/footer.tpl"); fetchmail.php:include ("./templates/header.tpl"); fetchmail.php:include ("./templates/menu.tpl"); fetchmail.php:include ("./templates/fetchmail.tpl"); fetchmail.php:include ("./templates/footer.tpl"); Secondly I suggest changing thoose template names to have a php ending as you can today browse the source code in if you type in the file name... Perhaps add a .htaccess file. Or even better move the template directory outside the web root. ---------------------------------------------------------------------- >Comment By: Christian Boltz (christian_boltz) Date: 2007-11-27 00:59 Message: Logged In: YES user_id=593261 Originator: NO Some words on $incpath: - $incpath is secure because it is set in common.php, which is included by all files using this variable. So even register_globals = on would never have any effect - the reason for introducing $incpath was the merging of admin/ with / - since the merging is done, it is no longer needed - exception: common.php - users/ will break if you remove $incpath from it To make the story short: I removed $incpath from all files except common.php (see above) - commited to SVN r243. I also tested the affected files to verify that they still work. Regarding your example with $tempfile: If register_globals is on, PHP imports the global variables when _starting_ the script, not in the middle of it. So if your example script really modifies $tempfile in the middle, you must do something realy silly like while(list($varname, $varvalue) = each($_REQUEST)) { $$varname = $varvalue; } or simply $tempfile = $_POST['tempfile']. Needless to say that you can't secure such code with register_globals = off Regarding template files: Yes, on the long term we should rename them to *.php. However, I don't see security problems because they only contain some code to generate the output. Most users won't modify the templates, therefore an attacker could also download the same code on sourceforge ;-) In fact, simply renaming the templates would reduce security because the attacker could execute them... Since renaming template files is the only thing left in this report, I opened a new tracker item for it: http://sourceforge.net/tracker/index.php?func=detail&aid=1839070&group_id=191583&atid=937967 and will close this one as fixed. If you want to respond about the template renaming, please do it in the new tracker item. For all other parts, respond here. And please open separate tracker items for each problem you report next time ;-) ---------------------------------------------------------------------- Comment By: GingerDog (gingerdog) Date: 2007-11-26 08:41 Message: Logged In: YES user_id=1761957 Originator: NO Hi, I agree that the code is not consistent, and adding a .htaccess could be a good idea - however there should be no inherent security problem if someone can view the template; after all, they could view the source code to any part of the app via sourceforge.net... The variable (e.g. $incpath) can NOT be manipulated from the URL - as <code>register_globals</code> is not needed/required for PostfixAdmin, and should be turned off by default (it's also explicitly set in common.php). I have made a change to common.php which will result in postfixadmin aborting if register_globals is turned on. thanks David ---------------------------------------------------------------------- Comment By: Jan Örnstedt (ornstedt) Date: 2007-11-26 01:31 Message: Logged In: YES user_id=498787 Originator: YES $tempfile = "12345.tmp"; ... # do something with $tempfile here # and some form processing ... unlink ($tempfile); Even if you handle $tempfile safely before unlinking it, the last statement is still very dangerous. An attacker can craft his or her own form containing a field similar to: <input type=hidden name="tempfile" value="../../../etc/passwd"> PHP will insert the field name in the global namespace as $tempfile, thus overwriting the original value of the variable. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=937964&aid=1838327&group_id=191583 |
