[PLGMAGTWOS-376] Please sanitise your inputs.

[Luciferiix]

Missing input sanitization and relying that i can't escape before passing on to Magento\Framework\Filesystem\Directory\WriteInterface in: https://github.com/MultiSafepay/Magento2Msp/blob/master/Controller/Fastcheckout/Success.php#L97

Name is: 'multisafepay-{{{{no-body-really-cares-apperently}}}}' Like: abc/../../../someFileThatNeedsToBeThere{{{{fancy-hacking-stuff-to-escape-the-suffix})}}

    /**
     * Get name of lock file
     *
     * @param string $name
     * @return string
     */
    private function getFilePath($name)
    {
        return DirectoryList::TMP . DIRECTORY_SEPARATOR . $name . self::LOCK_EXTENSION;
    }

If i can point to a relative old existing file here your going to delete it:

    /**
     * Check whether generation process has already locked
     *
     * @return bool
     */
    private function isProcessLocked()
    {
        if ($this->tmpDirectory->isExist($this->lockFilePath)) {
            try {
                $lockTime = (int) $this->tmpDirectory->readFile($this->lockFilePath);
                if ((time() - $lockTime) >= self::MAX_LOCK_TIME) {
                    $this->tmpDirectory->delete($this->lockFilePath);
                    return false;
                }
            } catch (FileSystemException $e) {
                return false;
            }
            return true;
        }
        return false;
    }

Conclusion: Is your Transaction key a number of fixed length ? Or does it have a subset a characters ?

Please check this first before passing it on to function in the namespace "Framework". They can't expect un-sanitised inputs.

[Ruud-MultiSafepay]

@Luciferiix Thank you for reporting. We have created an internal task for this report and will try to review as soon as possible. Internal reference: PLGMAGTWOS-376

[Jasper-MultiSafepay]

Analysis Impact is not severe for the following reasons:

• We're asking for write permissions on the var directory, not others. This leads to Framework already protecting with the following error. Path "/tmp/multisafepay-000000024.lock/../../../" cannot be used with directory "/home/vagrant/projects/magento2_2.3.0/var/" But we can even further lock it down by using DirectoryList::TMP instead of DirectoryList::VAR when asking for write permissions

Proposal

• We will sanitize and validate by checking $this->lockFilePath
• We will ask for write permissions inside DirectoryList::TMP, instead of DirectoryList::VAR

[Luciferiix]

About the impact i would be that shure. But i'm not that good at Triage.

The problem is not in the locking of files. Sanitising has to be done after getting the parameter value. It's a "thing" from the internet, please sanitise it first before try to fix a side effect somewhere else.

$params = $this->_requestHttp->getParams();
$this->_mspHelper->lockProcess('multisafepay-' . $params['transactionid']);

Don't worry this is a hard one. a 'normal' %00 (null) byte will throw exception, but as you can see we can get rid of the trailing ".lock". https://www.owasp.org/index.php/OWASP_Periodic_Table_of_Vulnerabilities_-_Null_Byte_Injection

The safest solution is in the controller at the source of the evil. (the internet)

$transactionId = $this->_requestHttp->getParams('transactionid');
$this->helper->validateTransactionId($transactionId);  // Throws a SecurityException

[Jasper-MultiSafepay]

We have released 1.7.0 which solves this issue.