BobRay/UpgradeMODX

Security: upgrade script available for the public/anonymous

wuuti opened this issue · 3 comments

wuuti commented

The upgrade.php script is available for anyone. It shouldn't. Instead it should only show up if a user of the administrator group is logged in. Otherwise it should camouflage its own existence by redirecting with a 404 not found to the sites not found page (or whatever is configured).
Alongside the user logout should take place after the upgrade button inside the script is clicked. Even better, after the successful download of the package and right before any changes to the system.

I question whether this is necessary. The only permanent version of the script is in the core directory, which shouldn't be available to the public. The temporary version used for the install can only be created by action of an administrator and deletes itself immediately after running, so it only exists very briefly when an admin user decides to upgrade the site.

wuuti commented

Yeah, that's the same if you do the setup manually. The argumentation there is the same, the setup directory only exists during administrative setup tasks and should be deleted when update is done.
But that's a weak argument imho: the admin may be interrupted during his setup tasks, or the whole procedure just takes longer than the expected time (that may be enough time for script kiddies to automatically detect the script/setup and do something nasty).

Or - and that was the case for me (see issue #12 about blank page), the update procedure fails for some reason. These leaves the script behind - the admin may never see that there, don't give upgrademodx another try and do it manually or with other tools again. Which occasionally may result in an installation where a not-secured script prowls in your root directory. That's not what anyone would want.

Every part of a system which is capable of changing the system should only be accessible with the proper permissions - regardless of how long the part may exist.

The new version attempts to remove all created files if there is an error.