Module FTP Management Allows Root Access as apache user. #189

Closed
apintocr opened this Issue Jul 15, 2015 · 18 comments

Projects

None yet

4 participants

@apintocr

By manipulating the data sent to the server a user can create a FTP account that allows to access the root folder of the server (running as apache user).

Details and Proof of Concept already sent to the Dev Team.

@Caffe1neAdd1ct
Member

Thanks for the update, i recieved an email from Tom on the support team.

I'm currently both not actively working on the project and occupied at work, however if one of the other developers doesn't pick this up soon I will personally make time to fix this.

a simple check using real_path against the user's web directory should stop this. For example the htaccess module i re-wrote does similar sanity checks.

@Caffe1neAdd1ct Caffe1neAdd1ct added the bug label Jul 15, 2015
@apintocr

I'm already Hot Fixing this.
Should be ready in moments.

@Caffe1neAdd1ct
Member

Great, let me know and i can pull in for you.

@Caffe1neAdd1ct
Member

Personally i would pull the fileInPathCheck method from the protected_directories module into the ftp_management module:

static function fileInPathCheck($path, $username)
    {
        $realPath = realpath($path);

        if(!$realPath)
        {
            return false;
        }

        if( 0 !== strpos($realPath, ctrl_options::GetSystemOption('hosted_dir') . $username . '/'))
        {
            return false;
        }

        return $realPath;
    }

And call that throughout any path creations inside the ExectureCreateFTP method. Many ways to fix this though 👍

@apintocr

Yup my solution is similar. Pull Request being Sent.

@Caffe1neAdd1ct
Member

Cool thanks, watching for it now. My solution was looking like this:

// Check to see if its a new home directory or use a current one or domain folder
            if ($home == 1) {

                $homedirectory_to_use = '/' . str_replace('.', '_', $username);
                $full_path = ctrl_options::GetSystemOption('hosted_dir') . $currentuser['username'] . $homedirectory_to_use . '/';

                if(!self::fileInPathCheck($full_path, $currentuser['username'])) {
                    return false;
                }

                // Create the new home directory... (If it doesnt already exist.)
                if (!file_exists($full_path)) {
                    @mkdir($full_path, 777);
                    @chmod($full_path, 0777);
                }
            } else if ($home == 3) {
                $homedirectory_to_use = '/' . $domainDestination;
                $full_path = ctrl_options::GetSystemOption('hosted_dir') . $currentuser['username'] . $homedirectory_to_use . '/';
                if(!self::fileInPathCheck($full_path, $currentuser['username'])) {
                    return false;
                }
            } else {
                $homedirectory_to_use = '/' . $destination;
                $full_path = ctrl_options::GetSystemOption('hosted_dir') . $currentuser['username'] . $homedirectory_to_use . '/';
                if(!self::fileInPathCheck($full_path, $currentuser['username'])) {
                    return false;
                }
            }

with the additional function in the previous comment, however i currently have no development server available to test (blind commits never a good idea.. )

@eByte23
Contributor
eByte23 commented Jul 15, 2015

The pr does not fix all the issue in this file.

  1. Any user can currently reset the password of any ftp user
  2. Any user can currently delete any ftp user

Also this is total not need...

 static function ExecuteResetPassword($ft_id_pk, $password)
    {
        global $zdbh;
        global $controller;
        runtime_hook::Execute('OnBeforeResetFTPPassword');
      //from here
        $rowftpsql = "SELECT * FROM x_ftpaccounts WHERE ft_id_pk=:ftIdPk";
        $rowftpfind = $zdbh->prepare($rowftpsql);
        $rowftpfind->bindParam(':ftIdPk', $ft_id_pk);
        $rowftpfind->execute();
        $rowftp = $rowftpfind->fetch();
 //here not needed at all
        $sql = $zdbh->prepare("UPDATE x_ftpaccounts SET ft_password_vc=:password WHERE ft_id_pk=:ftpid");
        $sql->bindParam(':password', $password);
        $sql->bindParam(':ftpid', $ft_id_pk);
        $sql->execute();
        self::$reset = true;
        // Include FTP server specific file here.
        $FtpModuleFile = 'modules/' . $controller->GetControllerRequest('URL', 'module') . '/code/' . ctrl_options::GetSystemOption('ftp_php');
        if (file_exists($FtpModuleFile)) {
            include($FtpModuleFile);
        }
        $retval = TRUE;
        runtime_hook::Execute('OnAfterResetFTPPassword');
        return $retval;
    }

That was already exisiting

@Caffe1neAdd1ct
Member

Looks like we need to limit the update query on current logged in user as i'm betting thats a key on the ftp table.

WHERE ft_acc_fk=:userid

and add

$sql->bindParam(':userid', $uid);
@apintocr

@eByte23 My goal was to fix the fact that any user could add a FTP directory any level above his own base directory.

I never claimed to fix all the issues on this file...

The useless code is probably due to whitespaces on file endings that my editor automatically removed (this is common practice and it is a good thing).

On a side note @eByte23 I'm very sad to see your arrogant nature criticizing my PR defects (that are not even critical...). What about a "Thank You for your PR. Now theres only the password reset issue remaining."?

@Caffe1neAdd1ct
Member

@eByte23 it might have been more appropriate to raise a separate issue for this.

@apintocr do you have time to have a quick look at the separate issue of password resets?

@apintocr

I'll look into it as I have a some freetime today.

@Caffe1neAdd1ct
Member

@apintocr thank you very much, i envy your free time!

@Caffe1neAdd1ct Caffe1neAdd1ct added this to the 1.0.1 milestone Jul 15, 2015
@Caffe1neAdd1ct Caffe1neAdd1ct self-assigned this Jul 15, 2015
@eByte23
Contributor
eByte23 commented Jul 15, 2015

@apintocr I wasn't being arrogant I was letting you know it didn't fix all issue on the page.
Also I would assume that when someone is working a page they would like to fix the issues that are similar or surrounding.

I wasn't commenting on any of your 'coding practices' I said that the page contained unused or useless code.(not your fault) was just mentioning.

If I wanted to be a pr nazi I would but I'm not.

@apintocr

@eByte23 sorry if I misunderstood you (internet sometimes leads to this...)
It just looked arrogant to me, but I believe it was not intentional and I apologize to you for my comment.
I'm Sorry.

@Caffe1neAdd1ct I envy my free time too, specially because I have the bad habit of filling my free time with work really fast 👍

@apintocr apintocr closed this Jul 15, 2015
@TGates71
Member

Thanks @Caffe1neAdd1ct for stopping in and helping with some positive suggestions! (That's why I tagged you ;) ) Also thanks to @apintocr :)

@apintocr

@Caffe1neAdd1ct tip was crucial for a fast PR.
Thank you.

@Caffe1neAdd1ct
Member

@TGates71 can you pop a forum announcement up about this and get people to start patching their servers asap? I'm only on my mobile at the moment

@apintocr

@Caffe1neAdd1ct / @TGates71 I've made a quick fix script and a forum announcement.
http://forums.sentora.org/showthread.php?tid=1767&pid=11302#pid11302

I hope you are ok with this, if not just delete the thread, I'll not be angry :)

@apintocr apintocr reopened this Jul 15, 2015
@apintocr apintocr closed this Jul 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment