Explorar o código

Merge commit from fork

* Fix Path Traversal vulnerability in UserDAO methods

* Add tests and changelog for UserDAO path traversal fix

* make fix-all

* Fix PHPStan

---------

Co-authored-by: Alexandre Alapetite <alexandre@alapetite.fr>
Bartłomiej Dmitruk hai 2 meses
pai
achega
26c1102567
Modificáronse 3 ficheiros con 88 adicións e 0 borrados
  1. 1 0
      CHANGELOG.md
  2. 9 0
      app/Models/UserDAO.php
  3. 78 0
      tests/app/Models/UserDAOTest.php

+ 1 - 0
CHANGELOG.md

@@ -14,6 +14,7 @@ See also [the FreshRSS releases](https://github.com/FreshRSS/FreshRSS/releases).
 	* Disable counting articles in user labels for Ajax requests (unused) [#8352](https://github.com/FreshRSS/FreshRSS/pull/8352)
 * Security
 	* Change `Content-Disposition: inline` to `attachment` in `f.php` [#8344](https://github.com/FreshRSS/FreshRSS/pull/8344)
+	* Fix Path Traversal vulnerability in `UserDAO` methods (`exists`, `mtime`, `ctime`) [GHSA-p8fh-pp43-9372](https://github.com/FreshRSS/FreshRSS/security/advisories/GHSA-p8fh-pp43-9372)
 * Extensions
 	* Update `.gitignore` to ignore installed extensions [#8372](https://github.com/FreshRSS/FreshRSS/pull/8372)
 * Misc.

+ 9 - 0
app/Models/UserDAO.php

@@ -49,6 +49,9 @@ class FreshRSS_UserDAO extends Minz_ModelPdo {
 	}
 
 	public static function exists(string $username): bool {
+		if (!FreshRSS_user_Controller::checkUsername($username)) {
+			return false;
+		}
 		return is_dir(USERS_PATH . '/' . $username);
 	}
 
@@ -64,6 +67,9 @@ class FreshRSS_UserDAO extends Minz_ModelPdo {
 
 	/** Time of the last modification action by the user (e.g., mark an article as read) */
 	public static function mtime(string $username): int {
+		if (!FreshRSS_user_Controller::checkUsername($username)) {
+			return 0;
+		}
 		return @filemtime(USERS_PATH . '/' . $username . '/config.php') ?: 0;
 	}
 
@@ -79,6 +85,9 @@ class FreshRSS_UserDAO extends Minz_ModelPdo {
 
 	/** Time of the last new content automatically received by the user (e.g., cron job, WebSub) */
 	public static function ctime(string $username): int {
+		if (!FreshRSS_user_Controller::checkUsername($username)) {
+			return 0;
+		}
 		return @filemtime(USERS_PATH . '/' . $username . '/' . LOG_FILENAME) ?: 0;
 	}
 }

+ 78 - 0
tests/app/Models/UserDAOTest.php

@@ -0,0 +1,78 @@
+<?php
+declare(strict_types=1);
+
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+class UserDAOTest extends TestCase {
+
+	#[DataProvider('pathTraversalPayloadsProvider')]
+	public function testExistsRejectsPathTraversal(string $payload): void {
+		self::assertFalse(FreshRSS_UserDAO::exists($payload));
+	}
+
+	#[DataProvider('pathTraversalPayloadsProvider')]
+	public function testMtimeRejectsPathTraversal(string $payload): void {
+		self::assertSame(0, FreshRSS_UserDAO::mtime($payload));
+	}
+
+	#[DataProvider('pathTraversalPayloadsProvider')]
+	public function testCtimeRejectsPathTraversal(string $payload): void {
+		self::assertSame(0, FreshRSS_UserDAO::ctime($payload));
+	}
+
+	/**
+	 * @return array<string,array<int,string>>
+	 */
+	public static function pathTraversalPayloadsProvider(): array {
+		return [
+			'parent directory' => ['../'],
+			'double parent directory' => ['../../'],
+			'traversal to app' => ['../../app'],
+			'traversal to etc' => ['../../../etc'],
+			'traversal with null byte' => ["../\0"],
+			'absolute path' => ['/etc/passwd'],
+			'dot only' => ['.'],
+			'double dot' => ['..'],
+			'slash in name' => ['user/config'],
+			'backslash traversal' => ['..\\..\\app'],
+			'encoded traversal' => ['%2e%2e%2f'],
+			'mixed traversal' => ['valid/../invalid'],
+			'empty string' => [''],
+		];
+	}
+
+	#[DataProvider('validUsernamesProvider')]
+	public function testExistsAcceptsValidUsernames(string $username): void {
+		$result = FreshRSS_UserDAO::exists($username);
+		self::assertIsBool($result);
+	}
+
+	#[DataProvider('validUsernamesProvider')]
+	public function testMtimeAcceptsValidUsernames(string $username): void {
+		$result = FreshRSS_UserDAO::mtime($username);
+		self::assertIsInt($result);
+	}
+
+	#[DataProvider('validUsernamesProvider')]
+	public function testCtimeAcceptsValidUsernames(string $username): void {
+		$result = FreshRSS_UserDAO::ctime($username);
+		self::assertIsInt($result);
+	}
+
+	/**
+	 * @return array<string,array<int,string>>
+	 */
+	public static function validUsernamesProvider(): array {
+		return [
+			'simple' => ['alice'],
+			'with numbers' => ['user123'],
+			'with underscore' => ['test_user'],
+			'with dot' => ['user.name'],
+			'with hyphen' => ['user-name'],
+			'with at' => ['user@domain'],
+			'single char' => ['a'],
+			'max length' => [str_repeat('a', 39)],
+		];
+	}
+}