From cf5b33fd6eab58bc51236573ecc5974ff706e4f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 7 Feb 2020 16:03:09 +0100 Subject: [PATCH 1/2] Allow accessing CORS routes if on ApiControllers if a proper CSRF token is provided MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .../Middleware/Security/CORSMiddleware.php | 7 ++++++- .../Middleware/Security/SecurityMiddleware.php | 16 ++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index 1883756954b58..cece4dc743c6e 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -26,6 +26,7 @@ namespace OC\AppFramework\Middleware\Security; +use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Authentication\Exceptions\PasswordLoginForbiddenException; @@ -80,7 +81,7 @@ public function __construct(IRequest $request, * @throws SecurityException * @since 6.0.0 */ - public function beforeController($controller, $methodName){ + public function beforeController($controller, $methodName) { // ensure that @CORS annotated API routes are not used in conjunction // with session authentication since this enables CSRF attack vectors if ($this->reflector->hasAnnotation('CORS') && @@ -88,6 +89,10 @@ public function beforeController($controller, $methodName){ $user = $this->request->server['PHP_AUTH_USER']; $pass = $this->request->server['PHP_AUTH_PW']; + // Allow to use the current session if a CSRF token is provided + if ($this->request->passesCSRFCheck()) { + return; + } $this->session->logout(); try { if (!$this->session->logClientIn($user, $pass, $this->request, $this->throttler)) { diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 16f1fb35a82a4..923649772fd5f 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -46,6 +46,7 @@ use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; +use OCP\AppFramework\ApiController; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\RedirectResponse; @@ -170,13 +171,16 @@ public function beforeController($controller, $methodName) { * * Additionally we allow Bearer authenticated requests to pass on OCS routes. * This allows oauth apps (e.g. moodle) to use the OCS endpoints + * CORS routes are also allowed to pass since the authentication and possible required + * CSRF token check is handled in the CORSMiddleware */ - if(!$this->request->passesCSRFCheck() && !( - $controller instanceof OCSController && ( - $this->request->getHeader('OCS-APIREQUEST') === 'true' || - strpos($this->request->getHeader('Authorization'), 'Bearer ') === 0 - ) - )) { + if (!$this->request->passesCSRFCheck() + && !($controller instanceof ApiController && $this->reflector->hasAnnotation('CORS')) + && !($controller instanceof OCSController && ( + $this->request->getHeader('OCS-APIREQUEST') === 'true' || + strpos($this->request->getHeader('Authorization'), 'Bearer ') === 0) + ) + ) { throw new CrossSiteRequestForgeryException(); } } From f1b387686af3858eba02c1da84e7d8a01a35a334 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 7 Feb 2020 16:03:55 +0100 Subject: [PATCH 2/2] Properly check for empty basic auth when trying to log in a user on CORS annotated endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .../AppFramework/Middleware/Security/CORSMiddleware.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index cece4dc743c6e..d60e002f97bd2 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -26,7 +26,6 @@ namespace OC\AppFramework\Middleware\Security; -use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Authentication\Exceptions\PasswordLoginForbiddenException; @@ -95,7 +94,7 @@ public function beforeController($controller, $methodName) { } $this->session->logout(); try { - if (!$this->session->logClientIn($user, $pass, $this->request, $this->throttler)) { + if (!empty($user) && !empty($pass) && !$this->session->logClientIn($user, $pass, $this->request, $this->throttler)) { throw new SecurityException('CORS requires basic auth', Http::STATUS_UNAUTHORIZED); } } catch (PasswordLoginForbiddenException $ex) {