#36 Inconsistent include statements

closed-fixed
nobody
Core (82)
1
2013-12-01
2007-11-26
No

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.

Discussion

  • Jan Örnstedt

    Jan Örnstedt - 2007-11-26

    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.

     
  • GingerDog

    GingerDog - 2007-11-26

    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

     
  • GingerDog

    GingerDog - 2007-11-26
    • priority: 5 --> 1
     
  • Christian Boltz

    Christian Boltz - 2007-11-26

    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 ;-)

     
  • Christian Boltz

    Christian Boltz - 2007-11-26
    • status: open --> closed-fixed
     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks