Merge pull request #46571 from nextcloud/chore/migrate-to-filenameval…

…idator refactor: Migrate some legacy and core functions to `IFilenameValidator`
nextcloud · Jul 22, 2024 · 9ed2d3e · 9ed2d3e
2 parents c2a571e + 9716b0d
commit 9ed2d3e
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 27 deletions.
diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php
@@ -189,8 +189,7 @@ public function postAvatar(?string $path = null): JSONResponse {
 		} 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
@@ -1824,6 +1824,12 @@ private function getPartFileInfo(string $path): \OC\Files\FileInfo {
 	 * @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
@@ -8,7 +8,6 @@
  */
 namespace OC\Security;
 
-use OC\Files\Filesystem;
 use OC\Files\View;
 use OCP\ICertificate;
 use OCP\ICertificateManager;
@@ -150,20 +149,19 @@ public function createCertificateBundle(): void {
 	 * @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 @@ public function addCertificate(string $certificate, string $name): ICertificate
 	 * 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
@@ -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 @@ public static function computerFileSize(string $str): false|int|float {
 	 * @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 @@ public static function copyr($src, $dest) {
 					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
@@ -12,8 +12,10 @@
 
 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 @@ public function dangerousFileProvider() {
 	 * @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);
 	}