From 6e83277162b418dbc2bcf7e8ce9c74c56de5b2c3 Mon Sep 17 00:00:00 2001 From: karakayasemi Date: Tue, 25 Sep 2018 20:03:14 +0300 Subject: [PATCH] do not allow multiple uses of the same key --- appinfo/database.xml | 5 ++ lib/Db/TotpSecret.php | 26 ++++++- lib/Db/TotpSecretMapper.php | 2 +- lib/Service/Totp.php | 22 ++++-- tests/unit/Service/TotpTest.php | 121 ++++++++++++++++++++++++++++++++ 5 files changed, 169 insertions(+), 7 deletions(-) create mode 100644 tests/unit/Service/TotpTest.php diff --git a/appinfo/database.xml b/appinfo/database.xml index 00fb3af..006e9c5 100644 --- a/appinfo/database.xml +++ b/appinfo/database.xml @@ -27,6 +27,11 @@ clob true + + last_validated_key + text + 32 + verified boolean diff --git a/lib/Db/TotpSecret.php b/lib/Db/TotpSecret.php index 6e5c782..0d33a8b 100644 --- a/lib/Db/TotpSecret.php +++ b/lib/Db/TotpSecret.php @@ -2,6 +2,7 @@ /** * @author Christoph Wurst + * @author Semih Serhat Karakaya * * Two-factor TOTP * @@ -26,7 +27,6 @@ /** * @method string getUserId() * @method void setUserId(string $userId) - * @method string getSecret() * @method void setSecret(string $secret) * @method boolean getVerified() * @method void setVerified(bool $verified) @@ -39,7 +39,31 @@ class TotpSecret extends Entity { /** @var string */ protected $secret; + /** @var string */ + protected $lastValidatedKey; + /** @var boolean */ protected $verified; + /** + * @return string + */ + public function getSecret() { + return $this->secret; + } + + /** + * @return string + */ + public function getLastValidatedKey() { + return $this->lastValidatedKey; + } + + /** + * @param string $key + */ + public function setLastValidatedKey($key) { + $this->lastValidatedKey = $key; + } + } diff --git a/lib/Db/TotpSecretMapper.php b/lib/Db/TotpSecretMapper.php index 29feaf4..740beb3 100644 --- a/lib/Db/TotpSecretMapper.php +++ b/lib/Db/TotpSecretMapper.php @@ -43,7 +43,7 @@ public function getSecret(IUser $user) { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); - $qb->select('id', 'user_id', 'secret', 'verified') + $qb->select('id', 'user_id', 'secret', 'verified', 'last_validated_key') ->from('twofactor_totp_secrets') ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($user->getUID()))); $result = $qb->execute(); diff --git a/lib/Service/Totp.php b/lib/Service/Totp.php index 252399b..b6fdaae 100644 --- a/lib/Service/Totp.php +++ b/lib/Service/Totp.php @@ -40,9 +40,13 @@ class Totp implements ITotp { /** @var ICrypto */ private $crypto; - public function __construct(TotpSecretMapper $secretMapper, ICrypto $crypto) { + /** @var Otp */ + private $otp; + + public function __construct(TotpSecretMapper $secretMapper, ICrypto $crypto, Otp $otp) { $this->secretMapper = $secretMapper; $this->crypto = $crypto; + $this->otp = $otp; } public function hasSecret(IUser $user) { @@ -109,10 +113,18 @@ public function validateSecret(IUser $user, $key) { throw new NoTotpSecretFoundException(); } - $secret = $this->crypto->decrypt($dbSecret->getSecret()); - - $otp = new Otp(); - return $otp->checkTotp(Base32::decode($secret), $key, 3); + /** + * Check last validated key to prevent consecutive usage of the same key + */ + if ($dbSecret->getLastValidatedKey() !== $key) { + $secret = $this->crypto->decrypt($dbSecret->getSecret()); + if ($this->otp->checkTotp(Base32::decode($secret), $key, 3) === true) { + $dbSecret->setLastValidatedKey($key); + $this->secretMapper->update($dbSecret); + return true; + } + } + return false; } /** diff --git a/tests/unit/Service/TotpTest.php b/tests/unit/Service/TotpTest.php new file mode 100644 index 0000000..4976cb8 --- /dev/null +++ b/tests/unit/Service/TotpTest.php @@ -0,0 +1,121 @@ + + * + * Two-factor TOTP + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OCA\TwoFactor_Totp\Tests\Service; + +use OCA\TwoFactor_Totp\Db\TotpSecret; +use OCA\TwoFactor_Totp\Db\TotpSecretMapper; +use OCA\TwoFactor_Totp\Exception\NoTotpSecretFoundException; +use OCA\TwoFactor_Totp\Service\Totp; +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\IUser; +use OCP\Security\ICrypto; +use Otp\Otp; +use Test\TestCase; + +/** + * Class TotpTest + */ +class TotpTest extends TestCase { + + /** @var TotpSecretMapper | \PHPUnit_Framework_MockObject_MockObject */ + private $secretMapper; + + /** @var ICrypto | \PHPUnit_Framework_MockObject_MockObject */ + private $crypto; + + /** @var Otp | \PHPUnit_Framework_MockObject_MockObject */ + private $otp; + + /** @var Totp */ + private $totp; + + protected function setUp() { + parent::setUp(); + + $this->secretMapper = $this->createMock(TotpSecretMapper::class); + $this->crypto = $this->createMock(ICrypto::class); + $this->otp = $this->createMock(Otp::class); + + $this->totp = new Totp($this->secretMapper, $this->crypto, $this->otp); + + } + + /** + * @dataProvider validationProvider + * @param string $lastKey + * @param string $key + * @param boolean $validationResult + * @param boolean $expectedResult + */ + public function testValidateSecret($lastKey, $key, $validationResult, $expectedResult) { + /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); + $dbSecret = $this->createMock(TotpSecret::class); + + $dbSecret->expects($this->once()) + ->method('getLastValidatedKey') + ->will($this->returnValue($lastKey)); + $this->secretMapper->expects($this->once()) + ->method('getSecret') + ->with($user) + ->will($this->returnValue($dbSecret)); + + if($lastKey !== $key) { + $dbSecret->expects($this->once()) + ->method('getSecret') + ->will($this->returnValue('secret')); + $this->otp->expects($this->once()) + ->method('checkTotp') + ->will($this->returnValue($validationResult)); + } + if($expectedResult === true) { + $dbSecret->expects($this->once()) + ->method('setLastValidatedKey') + ->with($key); + $this->secretMapper->expects($this->once()) + ->method('update') + ->with($dbSecret); + } + $this->assertEquals($this->totp->validateSecret($user, $key), $expectedResult); + } + + public function validationProvider() { + return [ + [ '000000', '000000', true, false ], + [ '000000', '000000', false, false ], + [ '000000', '000001', false, false ], + [ '000000', '000001', true, true ] + ]; + } + + public function testValidateSecretNoSecret() { + /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); + + $this->secretMapper->expects($this->once()) + ->method('getSecret') + ->with($user) + ->will($this->throwException(new DoesNotExistException(''))); + $this->expectException(NoTotpSecretFoundException::class); + $this->totp->validateSecret($user, 'testkey'); + } + +} \ No newline at end of file