From 9df63ce8151e473a5e0fb55c0cafb738fca1edaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Mon, 3 Jul 2023 10:48:05 +0200 Subject: [PATCH 1/3] fix: check for empty string if header exists --- changelog/unreleased/40856 | 6 +++ .../Security/SecurityMiddleware.php | 3 +- .../Security/SecurityMiddlewareTest.php | 54 ++++++++++++------- 3 files changed, 43 insertions(+), 20 deletions(-) create mode 100644 changelog/unreleased/40856 diff --git a/changelog/unreleased/40856 b/changelog/unreleased/40856 new file mode 100644 index 000000000000..abba7cae6050 --- /dev/null +++ b/changelog/unreleased/40856 @@ -0,0 +1,6 @@ +Bugfix: Request header can hold an empty string if not set + +Due to Apache rewrite rules originally not existing headers can hold an empty +string. + +https://github.com/owncloud/core/pull/40856 diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 82460d6a780d..60711217edf2 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -142,7 +142,8 @@ public function beforeController($controller, $methodName) { // CSRF check - also registers the CSRF token since the session may be closed later Util::callRegister(); if (!$this->reflector->hasAnnotation('NoCSRFRequired')) { - if (!$this->request->passesCSRFCheck() && $this->request->getHeader("Authorization") === null) { + $hasNoAuthHeader = ($this->request->getHeader("Authorization") === null || trim($this->request->getHeader("Authorization")) === ''); + if (!$this->request->passesCSRFCheck() && $hasNoAuthHeader) { throw new CrossSiteRequestForgeryException(); } } diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index 271fa500549d..07aee1ceb9ca 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -40,6 +40,7 @@ use OCP\ISession; use OCP\AppFramework\Controller; use OCP\IUserSession; +use ReflectionException; use Test\TestCase; use OCP\AppFramework\Http\Response; use OCP\IConfig; @@ -135,7 +136,7 @@ private function getMiddleware($isLoggedIn, $isAdminUser) { * @PublicPage * @NoCSRFRequired * @throws SecurityException - * @throws \ReflectionException + * @throws ReflectionException */ public function testSetNavigationEntry() { $this->navigationManager->expects($this->once()) @@ -150,7 +151,7 @@ public function testSetNavigationEntry() { * @param string $method * @param string $test * @param $status - * @throws \ReflectionException + * @throws ReflectionException */ private function ajaxExceptionStatus($method, $test, $status) { $isLoggedIn = false; @@ -178,7 +179,7 @@ private function ajaxExceptionStatus($method, $test, $status) { } /** - * @throws \ReflectionException + * @throws ReflectionException */ public function testAjaxStatusLoggedInCheck() { $this->ajaxExceptionStatus( @@ -190,7 +191,7 @@ public function testAjaxStatusLoggedInCheck() { /** * @NoCSRFRequired - * @throws \ReflectionException + * @throws ReflectionException */ public function testAjaxNotAdminCheck() { $this->ajaxExceptionStatus( @@ -202,7 +203,7 @@ public function testAjaxNotAdminCheck() { /** * @PublicPage - * @throws \ReflectionException + * @throws ReflectionException */ public function testAjaxStatusCSRFCheck() { $this->ajaxExceptionStatus( @@ -215,10 +216,7 @@ public function testAjaxStatusCSRFCheck() { /** * @PublicPage * @NoCSRFRequired - * @throws \ReflectionException - * @throws \ReflectionException - * @throws \ReflectionException - * @throws \ReflectionException + * @throws ReflectionException */ public function testAjaxStatusAllGood() { $this->ajaxExceptionStatus( @@ -247,7 +245,7 @@ public function testAjaxStatusAllGood() { * @PublicPage * @NoCSRFRequired * @throws SecurityException - * @throws \ReflectionException + * @throws ReflectionException */ public function testNoChecks() { $this->request->expects($this->never()) @@ -265,7 +263,7 @@ public function testNoChecks() { * @param string $expects * @param bool $shouldFail * @throws SecurityException - * @throws \ReflectionException + * @throws ReflectionException */ private function securityCheck($method, $expects, $shouldFail=false) { // admin check requires login @@ -292,10 +290,10 @@ private function securityCheck($method, $expects, $shouldFail=false) { /** * @PublicPage * @throws SecurityException - * @throws \ReflectionException + * @throws ReflectionException */ public function testCsrfCheck() { - $this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException::class); + $this->expectException(CrossSiteRequestForgeryException::class); $this->request->expects($this->once()) ->method('passesCSRFCheck') @@ -309,7 +307,7 @@ public function testCsrfCheck() { * @PublicPage * @NoCSRFRequired * @throws SecurityException - * @throws \ReflectionException + * @throws ReflectionException */ public function testNoCsrfCheck() { $this->request->expects($this->never()) @@ -323,7 +321,7 @@ public function testNoCsrfCheck() { /** * @PublicPage * @throws SecurityException - * @throws \ReflectionException + * @throws ReflectionException */ public function testFailCsrfCheck() { $this->request->expects($this->once()) @@ -334,11 +332,29 @@ public function testFailCsrfCheck() { $this->middleware->beforeController(__CLASS__, __FUNCTION__); } + /** + * @PublicPage + * @throws SecurityException + * @throws ReflectionException + */ + public function testFailCsrfCheckWithoutAuthHeader(): void { + $this->expectException(CrossSiteRequestForgeryException::class); + $this->request->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(false); + $this->request + ->method('getHeader') + ->willReturn(''); + + $this->reader->reflect(__CLASS__, __FUNCTION__); + $this->middleware->beforeController(__CLASS__, __FUNCTION__); + } + /** * @NoCSRFRequired * @NoAdminRequired * @throws SecurityException - * @throws \ReflectionException + * @throws ReflectionException */ public function testLoggedInCheck() { $this->securityCheck(__FUNCTION__, 'isLoggedIn'); @@ -348,7 +364,7 @@ public function testLoggedInCheck() { * @NoCSRFRequired * @NoAdminRequired * @throws SecurityException - * @throws \ReflectionException + * @throws ReflectionException */ public function testFailLoggedInCheck() { $this->securityCheck(__FUNCTION__, 'isLoggedIn', true); @@ -357,7 +373,7 @@ public function testFailLoggedInCheck() { /** * @NoCSRFRequired * @throws SecurityException - * @throws \ReflectionException + * @throws ReflectionException */ public function testIsAdminCheck() { $this->securityCheck(__FUNCTION__, 'isAdminUser'); @@ -366,7 +382,7 @@ public function testIsAdminCheck() { /** * @NoCSRFRequired * @throws SecurityException - * @throws \ReflectionException + * @throws ReflectionException */ public function testFailIsAdminCheck() { $this->securityCheck(__FUNCTION__, 'isAdminUser', true); From 878e654bc6b2c7a920a46d43df78378ab2a32829 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Mon, 10 Jul 2023 11:50:07 +0200 Subject: [PATCH 2/3] fix: no CSRF required on GET requests --- apps/files_sharing/lib/Controller/Share20OcsController.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/files_sharing/lib/Controller/Share20OcsController.php b/apps/files_sharing/lib/Controller/Share20OcsController.php index 38e9f141257c..ab9eb943f0b1 100644 --- a/apps/files_sharing/lib/Controller/Share20OcsController.php +++ b/apps/files_sharing/lib/Controller/Share20OcsController.php @@ -287,6 +287,7 @@ protected function formatShare(IShare $share, $received = false) { * Get a specific share by id * * @NoAdminRequired + * @NoCSRFRequired * * @param string $id * @return Result @@ -684,6 +685,7 @@ private function getSharedWithMe($node, $includeTags, $requestedShareTypes, $sta * the function will return an empty list. * * @NoAdminRequired + * @NoCSRFRequired * * - Get shares by the current user * - Get shares by the current user and reshares (?reshares=true) From 4c53f250a11cbf14e94405dc4281610d98658550 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Mon, 10 Jul 2023 17:23:21 +0200 Subject: [PATCH 3/3] fix: no CRSF on /privatedata/getattribute --- core/Controller/OcsController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/core/Controller/OcsController.php b/core/Controller/OcsController.php index e28fbd808d4c..e2b2e2e136cc 100644 --- a/core/Controller/OcsController.php +++ b/core/Controller/OcsController.php @@ -110,6 +110,7 @@ public function checkPerson($login, $password) { * test: curl http://login:passwd@oc/core/ocs/v1.php/privatedata/getattribute * * @NoAdminRequired + * @NoCSRFRequired * * @return Result */