From 9716b0d7350aa28481a5a9642b74da847c0fa211 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 17 Jul 2024 02:03:18 +0200 Subject: [PATCH] refactor: Migrate some legacy and core functions to `IFilenameValidator` Signed-off-by: Ferdinand Thiessen --- core/Controller/AvatarController.php | 3 +- lib/private/Files/View.php | 6 ++++ lib/private/Security/CertificateManager.php | 29 ++++++++++--------- lib/private/legacy/OC_Helper.php | 12 ++++++-- tests/lib/Security/CertificateManagerTest.php | 15 ++++------ 5 files changed, 38 insertions(+), 27 deletions(-) diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index f29c9c5ab4f..dac2df37ac3 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -189,8 +189,7 @@ class AvatarController extends Controller { } elseif (!is_null($files)) { if ( $files['error'][0] === 0 && - is_uploaded_file($files['tmp_name'][0]) && - !\OC\Files\Filesystem::isFileBlacklisted($files['tmp_name'][0]) + is_uploaded_file($files['tmp_name'][0]) ) { if ($files['size'][0] > 20 * 1024 * 1024) { return new JSONResponse( diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 26d419842d6..c74206f6ee3 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1824,6 +1824,12 @@ class View { * @throws InvalidPathException */ public function verifyPath($path, $fileName): void { + // All of the view's functions disallow '..' in the path so we can short cut if the path is invalid + if (!Filesystem::isValidPath($path ?: '/')) { + $l = \OCP\Util::getL10N('lib'); + throw new InvalidPathException($l->t('Path contains invalid segments')); + } + try { /** @type \OCP\Files\Storage $storage */ [$storage, $internalPath] = $this->resolvePath($path); diff --git a/lib/private/Security/CertificateManager.php b/lib/private/Security/CertificateManager.php index f9fd2b160b8..00babff735f 100644 --- a/lib/private/Security/CertificateManager.php +++ b/lib/private/Security/CertificateManager.php @@ -8,7 +8,6 @@ declare(strict_types=1); */ namespace OC\Security; -use OC\Files\Filesystem; use OC\Files\View; use OCP\ICertificate; use OCP\ICertificateManager; @@ -150,20 +149,19 @@ class CertificateManager implements ICertificateManager { * @throws \Exception If the certificate could not get added */ public function addCertificate(string $certificate, string $name): ICertificate { - if (!Filesystem::isValidPath($name) or Filesystem::isFileBlacklisted($name)) { - throw new \Exception('Filename is not valid'); - } + $path = $this->getPathToCertificates() . 'uploads/' . $name; + $directory = dirname($path); + + $this->view->verifyPath($directory, basename($path)); $this->bundlePath = null; - $dir = $this->getPathToCertificates() . 'uploads/'; - if (!$this->view->file_exists($dir)) { - $this->view->mkdir($dir); + if (!$this->view->file_exists($directory)) { + $this->view->mkdir($directory); } try { - $file = $dir . $name; $certificateObject = new Certificate($certificate, $name); - $this->view->file_put_contents($file, $certificate); + $this->view->file_put_contents($path, $certificate); $this->createCertificateBundle(); return $certificateObject; } catch (\Exception $e) { @@ -175,14 +173,17 @@ class CertificateManager implements ICertificateManager { * Remove the certificate and re-generate the certificate bundle */ public function removeCertificate(string $name): bool { - if (!Filesystem::isValidPath($name)) { + $path = $this->getPathToCertificates() . 'uploads/' . $name; + + try { + $this->view->verifyPath(dirname($path), basename($path)); + } catch (\Exception) { return false; } - $this->bundlePath = null; - $path = $this->getPathToCertificates() . 'uploads/'; - if ($this->view->file_exists($path . $name)) { - $this->view->unlink($path . $name); + $this->bundlePath = null; + if ($this->view->file_exists($path)) { + $this->view->unlink($path); $this->createCertificateBundle(); } return true; diff --git a/lib/private/legacy/OC_Helper.php b/lib/private/legacy/OC_Helper.php index 0c913709111..7db60363ff3 100644 --- a/lib/private/legacy/OC_Helper.php +++ b/lib/private/legacy/OC_Helper.php @@ -6,6 +6,7 @@ * SPDX-License-Identifier: AGPL-3.0-only */ use bantu\IniGetWrapper\IniGetWrapper; +use OC\Files\FilenameValidator; use OC\Files\Filesystem; use OCP\Files\Mount\IMountPoint; use OCP\IBinaryFinder; @@ -116,6 +117,10 @@ class OC_Helper { * @return void */ public static function copyr($src, $dest) { + if (!file_exists($src)) { + return; + } + if (is_dir($src)) { if (!is_dir($dest)) { mkdir($dest); @@ -126,8 +131,11 @@ class OC_Helper { self::copyr("$src/$file", "$dest/$file"); } } - } elseif (file_exists($src) && !\OC\Files\Filesystem::isFileBlacklisted($src)) { - copy($src, $dest); + } else { + $validator = \OCP\Server::get(FilenameValidator::class); + if (!$validator->isForbidden($src)) { + copy($src, $dest); + } } } diff --git a/tests/lib/Security/CertificateManagerTest.php b/tests/lib/Security/CertificateManagerTest.php index fc5ae1f03f2..e123ebeda25 100644 --- a/tests/lib/Security/CertificateManagerTest.php +++ b/tests/lib/Security/CertificateManagerTest.php @@ -12,8 +12,10 @@ namespace Test\Security; use OC\Files\View; use OC\Security\CertificateManager; +use OCP\Files\InvalidPathException; use OCP\IConfig; use OCP\Security\ISecureRandom; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; /** @@ -25,12 +27,9 @@ class CertificateManagerTest extends \Test\TestCase { use \Test\Traits\UserTrait; use \Test\Traits\MountProviderTrait; - /** @var CertificateManager */ - private $certificateManager; - /** @var String */ - private $username; - /** @var ISecureRandom */ - private $random; + private CertificateManager $certificateManager; + private string $username; + private ISecureRandom&MockObject $random; protected function setUp(): void { parent::setUp(); @@ -117,9 +116,7 @@ class CertificateManagerTest extends \Test\TestCase { * @param string $filename */ public function testAddDangerousFile($filename) { - $this->expectException(\Exception::class); - $this->expectExceptionMessage('Filename is not valid'); - + $this->expectException(InvalidPathException::class); $this->certificateManager->addCertificate(file_get_contents(__DIR__ . '/../../data/certificates/expiredCertificate.crt'), $filename); }