Subscribe: RSS
  • Quote from: Everett at Feb 09, 2011, 09:12 PM


    $command = escapeshellcmd($command);


    Do you have a test case where this patch fails?

    Yes, please have a look at this computed command:
    convert -density 150 -background "\#FFFFFF" -thumbnail 100x75 -quality 75 -interlace line  "/var/www/domain.dev/userfiles/test.jpg\[0\]" jpeg:"/tmp/pThumb8XOQIq" 2>&1


    It fails because of three escaped characters: \#, \[ and \] - these are escaped by escapeshellcmd(). As I mentioned in my previous post, escapeshellcmd() escapes too many characters, which are fully valid in many of the convert commands, computed by phpThumb.
    • Thanks... but the patch would still prevent a hack, and that was the priority. I meant, "do you have an example where this patch fails to prevent a hack?" The failure with this library is that it gave virtually no consideration to filtering the input commands, and without knowing what all the valid inputs are to the function, it’s extremely difficult to know how to filter the inputs other than the heavy-handed solution using escapeshellcmd(). If you’re more familiar with the valid inputs to this function, then it would be useful to provide a list of sample commands so that the data could be properly filtered... but that’s more the job of the library author, who seems to have gone completely AWOL, and it’s a shame that the project isn’t in a publicly available repository (at least, nowhere that I could find).

      If we wanted, we could create our own "fork" of this project on GitHub or similar, but I’m not sure it’s worth that kind of effort... there are other thumbnail libraries available, and I don’t think this one is necessarily the best.
      • Everett, I see your point.

        Anyways, I just wanted to make sure people are aware that patching their phpThumb libraries the way it’s proposed here would fix the vulnerability, but will also break the functionality in some of the scenarios.
        • Yeah, that’s definitely a valid point: applying this fix is sorta gets you out of the pan and into the fire... fixing one problem causes another. The thought did cross my mind that the fix presented might destroy some legitimate functionality -- good to know that yes, this is indeed the case. I remain mostly ignorant of the possible arguments that this library might pass to the command line, and without knowing those, it’s hard to offer a cleaner patch. Thanks for for "surgeon general’s warning" here -- people absolutely need to know what the effects of applying this patch are.
          • My site was hacked, I have found these next files modified, I say that because maybe you* have remaining code on them:

            assets/export/xmlrpcu.php <-- data file, not from modx, a hacked file, to be deleted
            manager/includes/document.parser.class.inc.php <--- modified in the last line with a: eval(base64_decode(".....
            manager/includes/protect.inc.php <--- Same thing

            Check if your php files includes "base64_decode" lines, they may be hacked files, im going to write a own script to run in cronjob for detect new files and files that include these codes, maybe also a checker for the database too
            • Those files shouldn’t be world-writable, so that means somebody has gained access to your FTP login, or else has gained root access to your server.
                Studying MODX in the desert - http://sottwell.com
                Tips and Tricks from the MODX Forums and Slack Channels - http://modxcookbook.com
                Join the Slack Community - http://modx.org
              • They are not word-writable, only for my user, I dont think that anybody can have ssh access (i have also a warning about logins), It can’t be possible that was a security problem by any plugin or similar thing ?
                • Of course it’s possible that a plugin had a security problem. For example, the initial post here detailed a hack that allowed someone else to run arbitrary commands on your server. It’s as your user (or as PHP’s user), but in the end, it means that your files could be modified, added, etc. That’s why it’s so important that plugins be checked carefully for security holes.
                  • I had a client site hacked as well, I’m still trying to clean it up.
                    I took the rather extreme step of deleting all of the existing files from the server, uploading a new copy of the modx 1.0.5 and doing an advanced reinstall using the same database.

                    I’m still seeing 302 redirect attempts after I login with the manager, so there must be something leftover in the database. I’ve search via phpmyadmin but I don’t see anything, and I’m not really sure exactly what to search for.
                    • Replacing all files after a hack should be NORMAL practice, not an "extreme" step. You have to assume that *all* files are tainted. The same goes for the database. This is why it’s so important to have rolling backups -- if you have a backup of the site prior to the hack, restore to that. Otherwise, you can spend a lot of time weeding through the database stuff and even if you find some altered code, you can never be sure that you’ve found all of it.

                      If you have replaced ALL the files with freshly-downloaded MODx versions, did you also replace all the user-created files? If you want to go through the MODx database, I’d look carefully at the templates to see if they were including extra JS stuff and all all the Snippets and Plugins to see if they were executing any extra code. For all those MODx snippets, paste the original Snippet code back into your database copy to ensure that you have the expected code executing and not some malicious stuff.