CVE-2016-9466: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: Nextcloud Server before 10.0.1 & ownCloud Server before 9.0.6 and 9.1.2 suffer from Reflected XSS in the Gallery application. The gallery app was not properly sanitizing exception messages from the Nextcloud/ownCloud server. Due to an endpoint where an attacker could influence the error message, this led to a reflected Cross-Site-Scripting vulnerability.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 6.1
Published: 2017-03-28T02:59Z
Repository: https://github.com/nextcloud/gallery
Language: PHP
Files changed: controller/configapicontroller.php, controller/configcontroller.php, controller/filesapicontroller.php, controller/filescontroller.php, controller/httperror.php, js/galleryview.js, tests/unit/controller/ConfigControllerTest.php, tests/unit/controller/FilesControllerTest.php, tests/unit/controller/HttpErrorTest.php
Commit date: 2016-09-13 23:43:50 +0200
Commit message: Do not display technical error messages

Diff: diff --git a/controller/configapicontroller.php b/controller/configapicontroller.php
index a331e3d..1ed0063 100644
--- a/controller/configapicontroller.php
+++ b/controller/configapicontroller.php
@@ -63,10 +63,10 @@ class ConfigApiController extends ApiController {
 	 */
 	public function get($extramediatypes = false) {
 		try {
 			return $this->getConfig($extramediatypes);
 		} catch (\Exception $exception) {
-			return $this->jsonError($exception);
+			return $this->jsonError($exception, $this->request, $this->logger);
 		}
 	}
 
 }
diff --git a/controller/configcontroller.php b/controller/configcontroller.php
index 568c57b..b535488 100644
--- a/controller/configcontroller.php
+++ b/controller/configcontroller.php
@@ -61,10 +61,10 @@ class ConfigController extends Controller {
 	 */
 	public function get($extramediatypes = false) {
 		try {
 			return $this->getConfig($extramediatypes);
 		} catch (\Exception $exception) {
-			return $this->jsonError($exception);
+			return $this->jsonError($exception, $this->request, $this->logger);
 		}
 	}
 
 }
diff --git a/controller/filesapicontroller.php b/controller/filesapicontroller.php
index 8a10d71..6fe341b 100644
--- a/controller/filesapicontroller.php
+++ b/controller/filesapicontroller.php
@@ -92,11 +92,11 @@ class FilesApiController extends ApiController {
 		$featuresArray = explode(';', $features);
 		$mediaTypesArray = explode(';', $mediatypes);
 		try {
 			return $this->getFilesAndAlbums($location, $featuresArray, $etag, $mediaTypesArray);
 		} catch (\Exception $exception) {
-			return $this->jsonError($exception);
+			return $this->jsonError($exception, $this->request, $this->logger);
 		}
 	}
 
 	/**
 	 * @NoAdminRequired
diff --git a/controller/filescontroller.php b/controller/filescontroller.php
index eee2a4a..7ce6ca9 100644
--- a/controller/filescontroller.php
+++ b/controller/filescontroller.php
@@ -95,11 +95,11 @@ class FilesController extends Controller {
 		$featuresArray = explode(';', $features);
 		$mediaTypesArray = explode(';', $mediatypes);
 		try {
 			return $this->getFilesAndAlbums($location, $featuresArray, $etag, $mediaTypesArray);
 		} catch (\Exception $exception) {
-			return $this->jsonError($exception);
+			return $this->jsonError($exception, $this->request, $this->logger);
 		}
 	}
 
 	/**
 	 * @NoAdminRequired
diff --git a/controller/httperror.php b/controller/httperror.php
index 58b289c..50cd336 100644
--- a/controller/httperror.php
+++ b/controller/httperror.php
@@ -14,10 +14,12 @@
 
 namespace OCA\Gallery\Controller;
 
 use Exception;
 
+use OCP\ILogger;
+use OCP\IRequest;
 use OCP\IURLGenerator;
 
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Http\RedirectResponse;
@@ -34,21 +36,33 @@ use OCA\Gallery\Service\ForbiddenServiceException;
  */
 trait HttpError {
 
 	/**
 	 * @param \Exception $exception
+	 * @param IRequest $request
+	 * @param ILogger $logger
 	 *
 	 * @return JSONResponse
 	 */
-	public function jsonError(Exception $exception) {
-		$message = $exception->getMessage();
+	public function jsonError(Exception $exception,
+							  IRequest $request,
+							  ILogger $logger) {
 		$code = $this->getHttpStatusCode($exception);
 
+		// If the exception is not of type ForbiddenServiceException only show a
+		// generic error message to avoid leaking information.
+		if(!($exception instanceof ForbiddenServiceException)) {
+			$logger->logException($exception, ['app' => 'gallery']);
+			$message = sprintf('An error occurred. Request ID: %s', $request->getId());
+		} else {
+			$message = $exception->getMessage() . ' (' . $code . ')';
+		}
+
 		return new JSONResponse(
 			[
-				'message' => $message . ' (' . $code . ')',
-				'success' => false
+				'message' => $message,
+				'success' => false,
 			],
 			$code
 		);
 	}
 
diff --git a/js/galleryview.js b/js/galleryview.js
index 55e5589..eb147c7 100644
--- a/js/galleryview.js
+++ b/js/galleryview.js
@@ -199,11 +199,11 @@
 			this.removeLoading();
 
 			if (!_.isUndefined(errorMessage) && errorMessage !== null) {
 				message += '<h2>' + t('gallery',
 						'Album cannot be shown') + '</h2>';
-				message += '<p>' + errorMessage + '</p>';
+				message += '<p>' + escapeHTML(errorMessage) + '</p>';
 				uploadAllowed = false;
 			} else {
 				message += '<h2>' + t('gallery',
 						'No media files found') + '</h2>';
 				// We can't upload yet on the public side
diff --git a/tests/unit/controller/ConfigControllerTest.php b/tests/unit/controller/ConfigControllerTest.php
index cf57055..0144cc9 100644
--- a/tests/unit/controller/ConfigControllerTest.php
+++ b/tests/unit/controller/ConfigControllerTest.php
@@ -168,14 +168,16 @@ class ConfigControllerTest extends \Test\TestCase {
 		$slideshow = true;
 		$exceptionMessage = 'Aïe!';
 		$this->configService->expects($this->any())
 							->method('getFeaturesList')
 							->willThrowException(new ServiceException($exceptionMessage));
-		// Default status code when something breaks
-		$status = Http::STATUS_INTERNAL_SERVER_ERROR;
+		$this->request
+			->expects($this->once())
+			->method('getId')
+			->willReturn('1234');
 		$errorMessage = [
-			'message' => $exceptionMessage  . ' (' . $status . ')',
+			'message' => 'An error occurred. Request ID: 1234',
 			'success' => false
 		];
 		/** @type JSONResponse $response */
 		$response = $this->controller->get($slideshow);
 
diff --git a/tests/unit/controller/FilesControllerTest.php b/tests/unit/controller/FilesControllerTest.php
index bc2099c..7062474 100644
--- a/tests/unit/controller/FilesControllerTest.php
+++ b/tests/unit/controller/FilesControllerTest.php
@@ -260,12 +260,16 @@ class FilesControllerTest extends \Test\GalleryUnitTest {
 									  [$features]
 								  )
 								  ->willThrowException(new ServiceException($exceptionMessage));
 		// Default status code when something breaks
 		$status = Http::STATUS_INTERNAL_SERVER_ERROR;
+		$this->request
+			->expects($this->once())
+			->method('getId')
+			->willReturn('1234');
 		$errorMessage = [
-			'message' => $exceptionMessage . ' (' . $status . ')',
+			'message' => 'An error occurred. Request ID: 1234',
 			'success' => false
 		];
 		/** @type JSONResponse $response */
 		$response = $this->controller->getList($location, $features, $etag, $mediatypes);
 
diff --git a/tests/unit/controller/HttpErrorTest.php b/tests/unit/controller/HttpErrorTest.php
index f82d959..0796f27 100644
--- a/tests/unit/controller/HttpErrorTest.php
+++ b/tests/unit/controller/HttpErrorTest.php
@@ -18,10 +18,12 @@ use OCP\AppFramework\Http\RedirectResponse;
 
 use OCA\Gallery\Environment\NotFoundEnvException;
 use OCA\Gallery\Service\NotFoundServiceException;
 use OCA\Gallery\Service\ForbiddenServiceException;
 use OCA\Gallery\Service\InternalServerErrorServiceException;
+use OCP\ILogger;
+use OCP\IRequest;
 
 /**
  * Class HttpErrorTest
  *
  * @package OCA\Gallery\Controller
@@ -33,27 +35,27 @@ class HttpErrorTest extends \Test\TestCase {
 
 	/**
 	 * @return array
 	 */
 	public function providesExceptionData() {
-		$notFoundEnvMessage = 'Not found in env';
+		$notFoundEnvMessage = 'An error occurred. Request ID: 1234';
 		$notFoundEnvException = new NotFoundEnvException($notFoundEnvMessage);
 		$notFoundEnvStatus = Http::STATUS_NOT_FOUND;
 
-		$notFoundServiceMessage = 'Not found in service';
+		$notFoundServiceMessage = 'An error occurred. Request ID: 1234';
 		$notFoundServiceException = new NotFoundServiceException($notFoundServiceMessage);
 		$notFoundServiceStatus = Http::STATUS_NOT_FOUND;
 
 		$forbiddenServiceMessage = 'Forbidden in service';
 		$forbiddenServiceException = new ForbiddenServiceException($forbiddenServiceMessage);
 		$forbiddenServiceStatus = Http::STATUS_FORBIDDEN;
 
-		$errorServiceMessage = 'Broken service';
+		$errorServiceMessage = 'An error occurred. Request ID: 1234';
 		$errorServiceException = new InternalServerErrorServiceException($errorServiceMessage);
 		$errorServiceStatus = Http::STATUS_INTERNAL_SERVER_ERROR;
 
-		$coreServiceMessage = 'Broken core';
+		$coreServiceMessage = 'An error occurred. Request ID: 1234';
 		$coreServiceException = new \Exception($coreServiceMessage);
 		$coreServiceStatus = Http::STATUS_INTERNAL_SERVER_ERROR;
 
 		return [
 			[$notFoundEnvException, $notFoundEnvMessage, $notFoundEnvStatus],
@@ -70,16 +72,36 @@ class HttpErrorTest extends \Test\TestCase {
 	 * @param \Exception $exception
 	 * @param String $message
 	 * @param String $status
 	 */
 	public function testJsonError($exception, $message, $status) {
-		$httpError = $this->getMockForTrait('\OCA\Gallery\Controller\HttpError');
+		$request = $this->createMock(IRequest::class);
+		$logger = $this->createMock(ILogger::class);
+
+		if($exception instanceof ForbiddenServiceException) {
+			$amount = 0;
+			$message = $message . ' (' . $status . ')';
+		} else {
+			$amount = 1;
+		}
+
+		$logger
+			->expects($this->exactly($amount))
+			->method('logException')
+			->with($exception, ['app' => 'gallery']);
+		$request
+			->expects($this->exactly($amount))
+			->method('getId')
+			->willReturn('1234');
+
+		/** @var HttpError $httpError */
+		$httpError = $this->getMockForTrait(HttpError::class);
 		/** @type JSONResponse $response */
-		$response = $httpError->jsonError($exception);
+		$response = $httpError->jsonError($exception, $request, $logger);
 
-		$this->assertEquals(
-			['message' => $message . ' (' . $status . ')', 'success' => false], $response->getData()
+		$this->assertSame(
+			['message' => $message, 'success' => false], $response->getData()
 		);
 		$this->assertEquals($status, $response->getStatus());
 	}
 
 	/**

Vulnerable code: 			return $this->jsonError($exception);
			return $this->jsonError($exception);
			return $this->jsonError($exception);
			return $this->jsonError($exception);
	public function jsonError(Exception $exception) {
		$message = $exception->getMessage();
				'message' => $message . ' (' . $code . ')',
				'success' => false
				message += '<p>' + errorMessage + '</p>';
		// Default status code when something breaks
		$status = Http::STATUS_INTERNAL_SERVER_ERROR;
			'message' => $exceptionMessage  . ' (' . $status . ')',
			'message' => $exceptionMessage . ' (' . $status . ')',
		$notFoundEnvMessage = 'Not found in env';
		$notFoundServiceMessage = 'Not found in service';
		$errorServiceMessage = 'Broken service';
		$coreServiceMessage = 'Broken core';
		$httpError = $this->getMockForTrait('\OCA\Gallery\Controller\HttpError');
		$response = $httpError->jsonError($exception);
		$this->assertEquals(
			['message' => $message . ' (' . $status . ')', 'success' => false], $response->getData()
Fixed code: 			return $this->jsonError($exception, $this->request, $this->logger);
			return $this->jsonError($exception, $this->request, $this->logger);
			return $this->jsonError($exception, $this->request, $this->logger);
			return $this->jsonError($exception, $this->request, $this->logger);
use OCP\ILogger;
use OCP\IRequest;
	 * @param IRequest $request
	 * @param ILogger $logger
	public function jsonError(Exception $exception,
							  IRequest $request,
							  ILogger $logger) {
		// If the exception is not of type ForbiddenServiceException only show a
		// generic error message to avoid leaking information.
		if(!($exception instanceof ForbiddenServiceException)) {
			$logger->logException($exception, ['app' => 'gallery']);
			$message = sprintf('An error occurred. Request ID: %s', $request->getId());
		} else {
			$message = $exception->getMessage() . ' (' . $code . ')';
		}

				'message' => $message,
				'success' => false,
				message += '<p>' + escapeHTML(errorMessage) + '</p>';
		$this->request
			->expects($this->once())
			->method('getId')
			->willReturn('1234');
			'message' => 'An error occurred. Request ID: 1234',
		$this->request
			->expects($this->once())
			->method('getId')
			->willReturn('1234');
			'message' => 'An error occurred. Request ID: 1234',
use OCP\ILogger;
use OCP\IRequest;
		$notFoundEnvMessage = 'An error occurred. Request ID: 1234';
		$notFoundServiceMessage = 'An error occurred. Request ID: 1234';
		$errorServiceMessage = 'An error occurred. Request ID: 1234';
		$coreServiceMessage = 'An error occurred. Request ID: 1234';
		$request = $this->createMock(IRequest::class);
		$logger = $this->createMock(ILogger::class);

		if($exception instanceof ForbiddenServiceException) {
			$amount = 0;
			$message = $message . ' (' . $status . ')';
		} else {
			$amount = 1;
		}

		$logger
			->expects($this->exactly($amount))
			->method('logException')
			->with($exception, ['app' => 'gallery']);
		$request
			->expects($this->exactly($amount))
			->method('getId')
			->willReturn('1234');

		/** @var HttpError $httpError */
		$httpError = $this->getMockForTrait(HttpError::class);
		$response = $httpError->jsonError($exception, $request, $logger);
		$this->assertSame(
			['message' => $message, 'success' => false], $response->getData()