Merge pull request #27011 from owncloud/fix_owncloud_user_enumeration…

…_vulnerbility_#23595 fix user enumeration with logging
owncloud · Jan 25, 2017 · f15c3c2 · f15c3c2
2 parents 6c42cf4 + c303974
commit f15c3c2
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 40 deletions.
diff --git a/core/Application.php b/core/Application.php
@@ -72,7 +72,8 @@ public function __construct(array $urlParams= []){
 				$c->query('DefaultEmailAddress'),
 				$c->query('IsEncryptionEnabled'),
 				$c->query('Mailer'),
-				$c->query('TimeFactory')
+				$c->query('TimeFactory'),
+				$c->query('Logger')
 			);
 		});
 		$container->registerService('UserController', function(SimpleContainer $c) {

diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php
@@ -31,6 +31,7 @@
 use \OCP\AppFramework\Controller;
 use \OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\ILogger;
 use \OCP\IURLGenerator;
 use \OCP\IRequest;
 use \OCP\IL10N;
@@ -70,6 +71,8 @@ class LostController extends Controller {
 	protected $mailer;
 	/** @var ITimeFactory */
 	protected $timeFactory;
+	/** @var ILogger */
+	protected $logger;
 
 	/**
 	 * @param string $appName
@@ -84,6 +87,7 @@ class LostController extends Controller {
 	 * @param string $isDataEncrypted
 	 * @param IMailer $mailer
 	 * @param ITimeFactory $timeFactory
+	 * @param ILogger $logger
 	 */
 	public function __construct($appName,
 								IRequest $request,
@@ -96,7 +100,8 @@ public function __construct($appName,
 								$from,
 								$isDataEncrypted,
 								IMailer $mailer,
-								ITimeFactory $timeFactory) {
+								ITimeFactory $timeFactory,
+								ILogger $logger) {
 		parent::__construct($appName, $request);
 		$this->urlGenerator = $urlGenerator;
 		$this->userManager = $userManager;
@@ -108,6 +113,7 @@ public function __construct($appName,
 		$this->config = $config;
 		$this->mailer = $mailer;
 		$this->timeFactory = $timeFactory;
+		$this->logger = $logger;
 	}
 
 	/**
@@ -236,42 +242,41 @@ public function setPassword($token, $userId, $password, $proceed) {
 	 * @throws \Exception
 	 */
 	protected function sendEmail($user) {
-		if (!$this->userManager->userExists($user)) {
-			throw new \Exception($this->l10n->t('Couldn\'t send reset email. Please make sure your username is correct.'));
-		}
-
-		$userObject = $this->userManager->get($user);
-		$email = $userObject->getEMailAddress();
+		if ($this->userManager->userExists($user)) {
 
-		if (empty($email)) {
-			throw new \Exception(
-				$this->l10n->t('Could not send reset email because there is no email address for this username. Please contact your administrator.')
-			);
-		}
+			$userObject = $this->userManager->get($user);
+			$email = $userObject->getEMailAddress();
 
-		$token = $this->secureRandom->generate(21,
-			ISecureRandom::CHAR_DIGITS.
-			ISecureRandom::CHAR_LOWER.
-			ISecureRandom::CHAR_UPPER);
-		$this->config->setUserValue($user, 'owncloud', 'lostpassword', $this->timeFactory->getTime() .':'. $token);
+			if (!empty($email)) {
+				$token = $this->secureRandom->generate(21,
+					ISecureRandom::CHAR_DIGITS .
+					ISecureRandom::CHAR_LOWER .
+					ISecureRandom::CHAR_UPPER);
+				$this->config->setUserValue($user, 'owncloud', 'lostpassword', $this->timeFactory->getTime() . ':' . $token);
 
-		$link = $this->urlGenerator->linkToRouteAbsolute('core.lost.resetform', ['userId' => $user, 'token' => $token]);
+				$link = $this->urlGenerator->linkToRouteAbsolute('core.lost.resetform', ['userId' => $user, 'token' => $token]);
 
-		$tmpl = new \OC_Template('core', 'lostpassword/email');
-		$tmpl->assign('link', $link);
-		$msg = $tmpl->fetchPage();
+				$tmpl = new \OC_Template('core', 'lostpassword/email');
+				$tmpl->assign('link', $link);
+				$msg = $tmpl->fetchPage();
 
-		try {
-			$message = $this->mailer->createMessage();
-			$message->setTo([$email => $user]);
-			$message->setSubject($this->l10n->t('%s password reset', [$this->defaults->getName()]));
-			$message->setPlainBody($msg);
-			$message->setFrom([$this->from => $this->defaults->getName()]);
-			$this->mailer->send($message);
-		} catch (\Exception $e) {
-			throw new \Exception($this->l10n->t(
-				'Couldn\'t send reset email. Please contact your administrator.'
-			));
+				try {
+					$message = $this->mailer->createMessage();
+					$message->setTo([$email => $user]);
+					$message->setSubject($this->l10n->t('%s password reset', [$this->defaults->getName()]));
+					$message->setPlainBody($msg);
+					$message->setFrom([$this->from => $this->defaults->getName()]);
+					$this->mailer->send($message);
+				} catch (\Exception $e) {
+					throw new \Exception($this->l10n->t(
+						'Couldn\'t send reset email. Please contact your administrator.'
+					));
+				}
+			} else {
+				$this->logger->error('Could not send reset email because there is no email address for this username. User: {user}', ['app' => 'core', 'user' => $user]);
+			}
+		} else {
+			$this->logger->error('Could not send reset email because User does not exist. User: {user}', ['app' => 'core', 'user' => $user]);
 		}
 	}
 

diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php
@@ -26,6 +26,7 @@
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IConfig;
 use OCP\IL10N;
+use OCP\ILogger;
 use OCP\IRequest;
 use OCP\IURLGenerator;
 use OCP\IUser;
@@ -63,6 +64,8 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 	private $timeFactory;
 	/** @var IRequest */
 	private $request;
+	/** @var ILogger */
+	private $logger;
 
 	protected function setUp() {
 
@@ -98,6 +101,8 @@ protected function setUp() {
 			->disableOriginalConstructor()->getMock();
 		$this->request = $this->getMockBuilder('OCP\IRequest')
 			->disableOriginalConstructor()->getMock();
+		$this->logger = $this->getMockBuilder('OCP\ILogger')
+			->disableOriginalConstructor()->getMock();
 		$this->lostController = new LostController(
 			'Core',
 			$this->request,
@@ -110,7 +115,8 @@ protected function setUp() {
 			'lostpassword-noreply@localhost',
 			true,
 			$this->mailer,
-			$this->timeFactory
+			$this->timeFactory,
+			$this->logger
 		);
 	}
 
@@ -236,7 +242,7 @@ public function testResetFormValidToken() {
 	}
 
 	public function testEmailUnsucessful() {
-		$existingUser = 'ExistingUser';
+		$existingUser = 'ExistingUser1';
 		$nonExistingUser = 'NonExistingUser';
 		$this->userManager
 			->expects($this->any())
@@ -249,21 +255,25 @@ public function testEmailUnsucessful() {
 		// With a non existing user
 		$response = $this->lostController->email($nonExistingUser);
 		$expectedResponse = [
-			'status' => 'error',
-			'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.'
+			'status' => 'success'
 		];
+		$this->logger->expects($this->any())
+			->method('error')
+			->with('Could not send reset email because User does not exist. User: {user}');
 		$this->assertSame($expectedResponse, $response);
 
 		// With no mail address
 		$this->config
 			->expects($this->any())
 			->method('getUserValue')
 			->with($existingUser, 'settings', 'email')
-			->will($this->returnValue(null));
+			->will($this->returnValue(''));
 		$response = $this->lostController->email($existingUser);
+		$this->logger->expects($this->any())
+			->method('error')
+			->with('Could not send reset email because there is no email address for this username. User: {user}');
 		$expectedResponse = [
-			'status' => 'error',
-			'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.'
+			'status' => 'success'
 		];
 		$this->assertSame($expectedResponse, $response);
 	}