Module FTP Management Allows Root Access as apache user.

[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]

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.

[apintocr]

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

[Caffe1neAdd1ct]

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

[Caffe1neAdd1ct]

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 :+1:

[apintocr]

Yup my solution is similar. Pull Request being Sent.

[Caffe1neAdd1ct]

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]

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]

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]

@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]

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

[eByte23]

@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 :+1:

[TGates71]

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]

@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 :)