Merge pull request #16532 from owncloud/cors-no-cookie-auth

Disallow cookie auth for cors requests

apps/files/controller/apicontroller.php

@@ -92,7 +92,6 @@ class ApiController extends Controller {
 	 * replace the actual tag selection.
 	 *
 	 * @NoAdminRequired
-	 * @CORS
 	 *
 	 * @param string $path path
 	 * @param array|string $tags array of tags
@@ -126,7 +125,6 @@ class ApiController extends Controller {
 	 * Returns a list of all files tagged with the given tag.
 	 *
 	 * @NoAdminRequired
-	 * @CORS
 	 *
 	 * @param array|string $tagName tag name to filter by
 	 * @return DataResponse

lib/private/appframework/dependencyinjection/dicontainer.php

@@ -291,7 +291,8 @@ class DIContainer extends SimpleContainer implements IAppContainer {
 		$this->registerService('CORSMiddleware', function($c) {
 			return new CORSMiddleware(
 				$c['Request'],
-				$c['ControllerMethodReflector']
+				$c['ControllerMethodReflector'],
+				$c['OCP\IUserSession']
 			);
 		});
 
@@ -306,8 +307,8 @@ class DIContainer extends SimpleContainer implements IAppContainer {
 		$middleWares = &$this->middleWares;
 		$this->registerService('MiddlewareDispatcher', function($c) use (&$middleWares) {
 			$dispatcher = new MiddlewareDispatcher();
-			$dispatcher->registerMiddleware($c['SecurityMiddleware']);
 			$dispatcher->registerMiddleware($c['CORSMiddleware']);
+			$dispatcher->registerMiddleware($c['SecurityMiddleware']);
 
 			foreach($middleWares as $middleWare) {
 				$dispatcher->registerMiddleware($c[$middleWare]);

lib/private/appframework/middleware/security/corsmiddleware.php

@@ -24,30 +24,69 @@ namespace OC\AppFramework\Middleware\Security;
 
 use OC\AppFramework\Utility\ControllerMethodReflector;
 use OCP\IRequest;
+use OCP\IUserSession;
 use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\Middleware;
 
 /**
- * This middleware sets the correct CORS headers on a response if the 
+ * This middleware sets the correct CORS headers on a response if the
  * controller has the @CORS annotation. This is needed for webapps that want
- * to access an API and dont run on the same domain, see 
+ * to access an API and dont run on the same domain, see
  * https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS
  */
 class CORSMiddleware extends Middleware {
 
+	/**
+	 * @var IRequest
+	 */
 	private $request;
+
+	/**
+	 * @var ControllerMethodReflector
+	 */
 	private $reflector;
 
+	/**
+	 * @var IUserSession
+	 */
+	private $session;
+
 	/**
 	 * @param IRequest $request
 	 * @param ControllerMethodReflector $reflector
+	 * @param IUserSession $session
 	 */
-	public function __construct(IRequest $request, 
-	                            ControllerMethodReflector $reflector) {
+	public function __construct(IRequest $request,
+	                            ControllerMethodReflector $reflector,
+	                            IUserSession $session) {
 		$this->request = $request;
 		$this->reflector = $reflector;
+		$this->session = $session;
 	}
 
+	/**
+	 * This is being run in normal order before the controller is being
+	 * called which allows several modifications and checks
+	 *
+	 * @param Controller $controller the controller that is being called
+	 * @param string $methodName the name of the method that will be called on
+	 *                           the controller
+	 * @since 6.0.0
+	 */
+	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') &&
+			!$this->reflector->hasAnnotation('PublicPage'))  {
+			$user = $this->request->server['PHP_AUTH_USER'];
+			$pass = $this->request->server['PHP_AUTH_PW'];
+
+			$this->session->logout();
+			if(!$this->session->login($user, $pass)) {
+				throw new SecurityException('CORS requires basic auth');
+			}
+		}
+	}
 
 	/**
 	 * This is being run after a successful controllermethod call and allows
@@ -65,7 +104,7 @@ class CORSMiddleware extends Middleware {
 		if(isset($this->request->server['HTTP_ORIGIN']) &&
 			$this->reflector->hasAnnotation('CORS')) {
 
-			// allow credentials headers must not be true or CSRF is possible 
+			// allow credentials headers must not be true or CSRF is possible
 			// otherwise
 			foreach($response->getHeaders() as $header => $value ) {
 				if(strtolower($header) === 'access-control-allow-credentials' &&

tests/lib/appframework/middleware/security/CORSMiddlewareTest.php

@@ -21,10 +21,12 @@ use OCP\AppFramework\Http\Response;
 class CORSMiddlewareTest extends \Test\TestCase {
 
 	private $reflector;
+	private $session;
 
 	protected function setUp() {
 		parent::setUp();
 		$this->reflector = new ControllerMethodReflector();
+		$this->session = $this->getMock('\OCP\IUserSession');
 	}
 
 	/**
@@ -41,7 +43,7 @@ class CORSMiddlewareTest extends \Test\TestCase {
 			$this->getMock('\OCP\IConfig')
 		);
 		$this->reflector->reflect($this, __FUNCTION__);
-		$middleware = new CORSMiddleware($request, $this->reflector);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session);
 
 		$response = $middleware->afterController($this, __FUNCTION__, new Response());
 		$headers = $response->getHeaders();
@@ -59,7 +61,7 @@ class CORSMiddlewareTest extends \Test\TestCase {
 			$this->getMock('\OCP\Security\ISecureRandom'),
 			$this->getMock('\OCP\IConfig')
 		);
-		$middleware = new CORSMiddleware($request, $this->reflector);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session);
 
 		$response = $middleware->afterController($this, __FUNCTION__, new Response());
 		$headers = $response->getHeaders();
@@ -77,7 +79,7 @@ class CORSMiddlewareTest extends \Test\TestCase {
 			$this->getMock('\OCP\IConfig')
 		);
 		$this->reflector->reflect($this, __FUNCTION__);
-		$middleware = new CORSMiddleware($request, $this->reflector);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session);
 
 		$response = $middleware->afterController($this, __FUNCTION__, new Response());
 		$headers = $response->getHeaders();
@@ -100,11 +102,76 @@ class CORSMiddlewareTest extends \Test\TestCase {
 			$this->getMock('\OCP\IConfig')
 		);
 		$this->reflector->reflect($this, __FUNCTION__);
-		$middleware = new CORSMiddleware($request, $this->reflector);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session);
 
 		$response = new Response();
 		$response->addHeader('AcCess-control-Allow-Credentials ', 'TRUE');
 		$middleware->afterController($this, __FUNCTION__, $response);
 	}
 
+	/**
+	 * @CORS
+	 * @PublicPage
+	 */
+	public function testNoCORSShouldAllowCookieAuth() {
+		$request = new Request(
+			[],
+			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\IConfig')
+		);
+		$this->reflector->reflect($this, __FUNCTION__);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session);
+
+		$middleware->beforeController($this, __FUNCTION__, new Response());
+	}
+
+	/**
+	 * @CORS
+	 */
+	public function testCORSShouldRelogin() {
+		$request = new Request(
+			['server' => [
+				'PHP_AUTH_USER' => 'user',
+				'PHP_AUTH_PW' => 'pass'
+			]],
+			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\IConfig')
+		);
+		$this->session->expects($this->once())
+			->method('logout');
+		$this->session->expects($this->once())
+			->method('login')
+			->with($this->equalTo('user'), $this->equalTo('pass'))
+			->will($this->returnValue(true));
+		$this->reflector->reflect($this, __FUNCTION__);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session);
+
+		$middleware->beforeController($this, __FUNCTION__, new Response());
+	}
+
+	/**
+	 * @CORS
+	 * @expectedException \OC\AppFramework\Middleware\Security\SecurityException
+	 */
+	public function testCORSShouldNotAllowCookieAuth() {
+		$request = new Request(
+			['server' => [
+				'PHP_AUTH_USER' => 'user',
+				'PHP_AUTH_PW' => 'pass'
+			]],
+			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\IConfig')
+		);
+		$this->session->expects($this->once())
+			->method('logout');
+		$this->session->expects($this->once())
+			->method('login')
+			->with($this->equalTo('user'), $this->equalTo('pass'))
+			->will($this->returnValue(false));
+		$this->reflector->reflect($this, __FUNCTION__);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session);
+
+		$middleware->beforeController($this, __FUNCTION__, new Response());
+	}
+
 }