Skip to content

Commit 772acf1

Browse files
ameysarameysar
committed
MAGETWO-83589: Vulnerability related to active login session
1 parent e669d9d commit 772acf1

File tree

5 files changed

+259
-21
lines changed

5 files changed

+259
-21
lines changed

‎app/code/Magento/Customer/Model/AccountManagement.php

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@
4848
use Magento\Store\Model\ScopeInterface;
4949
use Magento\Store\Model\StoreManagerInterface;
5050
use Psr\Log\LoggerInterface as PsrLogger;
51+
use Magento\Framework\Session\SessionManagerInterface;
52+
use Magento\Framework\Session\SaveHandlerInterface;
53+
use Magento\Customer\Model\ResourceModel\Visitor\CollectionFactory;
5154

5255
/**
5356
* Handle various customer account actions
@@ -243,6 +246,21 @@ class AccountManagement implements AccountManagementInterface
243246
*/
244247
private $transportBuilder;
245248

249+
/**
250+
* @var SessionManagerInterface
251+
*/
252+
private $sessionManager;
253+
254+
/**
255+
* @var SaveHandlerInterface
256+
*/
257+
private $saveHandler;
258+
259+
/**
260+
* @var CollectionFactory
261+
*/
262+
private $visitorCollectionFactory;
263+
246264
/**
247265
* @var DataObjectProcessor
248266
*/
@@ -335,6 +353,9 @@ class AccountManagement implements AccountManagementInterface
335353
* @param CredentialsValidator|null $credentialsValidator
336354
* @param DateTimeFactory|null $dateTimeFactory
337355
* @param AccountConfirmation|null $accountConfirmation
356+
* @param SessionManagerInterface|null $sessionManager
357+
* @param SaveHandlerInterface|null $saveHandler
358+
* @param CollectionFactory|null $visitorCollectionFactory
338359
* @SuppressWarnings(PHPMD.ExcessiveParameterList)
339360
*/
340361
public function __construct(
@@ -363,7 +384,10 @@ public function __construct(
363384
ExtensibleDataObjectConverter $extensibleDataObjectConverter,
364385
CredentialsValidator $credentialsValidator = null,
365386
DateTimeFactory $dateTimeFactory = null,
366-
AccountConfirmation $accountConfirmation = null
387+
AccountConfirmation $accountConfirmation = null,
388+
SessionManagerInterface $sessionManager = null,
389+
SaveHandlerInterface $saveHandler = null,
390+
CollectionFactory $visitorCollectionFactory = null
367391
) {
368392
$this->customerFactory = $customerFactory;
369393
$this->eventManager = $eventManager;
@@ -393,6 +417,12 @@ public function __construct(
393417
$this->dateTimeFactory = $dateTimeFactory ?: ObjectManager::getInstance()->get(DateTimeFactory::class);
394418
$this->accountConfirmation = $accountConfirmation ?: ObjectManager::getInstance()
395419
->get(AccountConfirmation::class);
420+
$this->sessionManager = $sessionManager
421+
?: ObjectManager::getInstance()->get(SessionManagerInterface::class);
422+
$this->saveHandler = $saveHandler
423+
?: ObjectManager::getInstance()->get(SaveHandlerInterface::class);
424+
$this->visitorCollectionFactory = $visitorCollectionFactory
425+
?: ObjectManager::getInstance()->get(CollectionFactory::class);
396426
}
397427

398428
/**
@@ -594,7 +624,10 @@ public function resetPassword($email, $resetToken, $newPassword)
594624
$customerSecure->setRpToken(null);
595625
$customerSecure->setRpTokenCreatedAt(null);
596626
$customerSecure->setPasswordHash($this->createPasswordHash($newPassword));
627+
$this->sessionManager->destroy();
628+
$this->destroyCustomerSessions($customer->getId());
597629
$this->customerRepository->save($customer);
630+
598631
return true;
599632
}
600633

@@ -896,7 +929,9 @@ private function changePasswordForCustomer($customer, $currentPassword, $newPass
896929
$customerSecure->setRpTokenCreatedAt(null);
897930
$this->checkPasswordStrength($newPassword);
898931
$customerSecure->setPasswordHash($this->createPasswordHash($newPassword));
932+
$this->destroyCustomerSessions($customer->getId());
899933
$this->customerRepository->save($customer);
934+
900935
return true;
901936
}
902937

@@ -1399,4 +1434,35 @@ private function getEmailNotification()
13991434
return $this->emailNotification;
14001435
}
14011436
}
1437+
1438+
/**
1439+
* Destroy all active customer sessions by customer id (current session will not be destroyed).
1440+
* Customer sessions which should be deleted are collecting from the "customer_visitor" table considering
1441+
* configured session lifetime.
1442+
*
1443+
* @param string|int $customerId
1444+
* @return void
1445+
*/
1446+
private function destroyCustomerSessions($customerId)
1447+
{
1448+
$sessionLifetime = $this->scopeConfig->getValue(
1449+
\Magento\Framework\Session\Config::XML_PATH_COOKIE_LIFETIME,
1450+
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
1451+
);
1452+
$dateTime = $this->dateTimeFactory->create();
1453+
$activeSessionsTime = $dateTime->setTimestamp($dateTime->getTimestamp() - $sessionLifetime)
1454+
->format(DateTime::DATETIME_PHP_FORMAT);
1455+
/** @var \Magento\Customer\Model\ResourceModel\Visitor\Collection $visitorCollection */
1456+
$visitorCollection = $this->visitorCollectionFactory->create();
1457+
$visitorCollection->addFieldToFilter('customer_id', $customerId);
1458+
$visitorCollection->addFieldToFilter('last_visit_at', ['from' => $activeSessionsTime]);
1459+
$visitorCollection->addFieldToFilter('session_id', ['neq' => $this->sessionManager->getSessionId()]);
1460+
/** @var \Magento\Customer\Model\Visitor $visitor */
1461+
foreach ($visitorCollection->getItems() as $visitor) {
1462+
$sessionId = $visitor->getSessionId();
1463+
$this->sessionManager->start();
1464+
$this->saveHandler->destroy($sessionId);
1465+
$this->sessionManager->writeClose();
1466+
}
1467+
}
14021468
}

‎app/code/Magento/Customer/Model/Visitor.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,9 @@ public function initByRequest($observer)
151151

152152
if ($this->session->getVisitorData()) {
153153
$this->setData($this->session->getVisitorData());
154+
if ($this->getSessionId() != $this->session->getSessionId()) {
155+
$this->setSessionId($this->session->getSessionId());
156+
}
154157
}
155158

156159
$this->setLastVisitAt((new \DateTime())->format(\Magento\Framework\Stdlib\DateTime::DATETIME_PHP_FORMAT));

‎app/code/Magento/Customer/Test/Unit/Model/AccountManagementTest.php

Lines changed: 175 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,21 @@ class AccountManagementTest extends \PHPUnit\Framework\TestCase
127127
*/
128128
private $accountConfirmation;
129129

130+
/**
131+
* @var \PHPUnit_Framework_MockObject_MockObject|\Magento\Framework\Session\SessionManagerInterface
132+
*/
133+
private $sessionManager;
134+
135+
/**
136+
* @var \PHPUnit_Framework_MockObject_MockObject|\Magento\Customer\Model\ResourceModel\Visitor\CollectionFactory
137+
*/
138+
private $visitorCollectionFactory;
139+
140+
/**
141+
* @var \PHPUnit_Framework_MockObject_MockObject|\Magento\Framework\Session\SaveHandlerInterface
142+
*/
143+
private $saveHandler;
144+
130145
/**
131146
* @SuppressWarnings(PHPMD.ExcessiveMethodLength)
132147
*/
@@ -179,6 +194,19 @@ protected function setUp()
179194
$this->dateTimeFactory = $this->createMock(DateTimeFactory::class);
180195
$this->accountConfirmation = $this->createMock(AccountConfirmation::class);
181196

197+
$this->visitorCollectionFactory = $this->getMockBuilder(
198+
\Magento\Customer\Model\ResourceModel\Visitor\CollectionFactory::class
199+
)
200+
->disableOriginalConstructor()
201+
->setMethods(['create'])
202+
->getMock();
203+
$this->sessionManager = $this->getMockBuilder(\Magento\Framework\Session\SessionManagerInterface::class)
204+
->disableOriginalConstructor()
205+
->getMock();
206+
$this->saveHandler = $this->getMockBuilder(\Magento\Framework\Session\SaveHandlerInterface::class)
207+
->disableOriginalConstructor()
208+
->getMock();
209+
182210
$this->objectManagerHelper = new ObjectManagerHelper($this);
183211
$this->accountManagement = $this->objectManagerHelper->getObject(
184212
\Magento\Customer\Model\AccountManagement::class,
@@ -207,7 +235,10 @@ protected function setUp()
207235
'objectFactory' => $this->objectFactory,
208236
'extensibleDataObjectConverter' => $this->extensibleDataObjectConverter,
209237
'dateTimeFactory' => $this->dateTimeFactory,
210-
'accountConfirmation' => $this->accountConfirmation
238+
'accountConfirmation' => $this->accountConfirmation,
239+
'sessionManager' => $this->sessionManager,
240+
'saveHandler' => $this->saveHandler,
241+
'visitorCollectionFactory' => $this->visitorCollectionFactory,
211242
]
212243
);
213244
$reflection = new \ReflectionClass(get_class($this->accountManagement));
@@ -1276,7 +1307,16 @@ private function reInitModel()
12761307
{
12771308
$this->customerSecure = $this->getMockBuilder(\Magento\Customer\Model\Data\CustomerSecure::class)
12781309
->disableOriginalConstructor()
1279-
->setMethods(['getRpToken', 'getRpTokenCreatedAt'])
1310+
->setMethods(
1311+
[
1312+
'getRpToken',
1313+
'getRpTokenCreatedAt',
1314+
'getPasswordHash',
1315+
'setPasswordHash',
1316+
'setRpToken',
1317+
'setRpTokenCreatedAt',
1318+
]
1319+
)
12801320
->getMock();
12811321

12821322
$this->customerSecure
@@ -1297,6 +1337,48 @@ private function reInitModel()
12971337
->getMock();
12981338

12991339
$this->prepareDateTimeFactory();
1340+
$this->sessionManager = $this->getMockBuilder(\Magento\Framework\Session\SessionManagerInterface::class)
1341+
->disableOriginalConstructor()
1342+
->setMethods(['destroy', 'start', 'writeClose'])
1343+
->getMockForAbstractClass();
1344+
$this->visitorCollectionFactory = $this->getMockBuilder(
1345+
\Magento\Customer\Model\ResourceModel\Visitor\CollectionFactory::class
1346+
)
1347+
->disableOriginalConstructor()
1348+
->setMethods(['create'])
1349+
->getMock();
1350+
$this->saveHandler = $this->getMockBuilder(\Magento\Framework\Session\SaveHandlerInterface::class)
1351+
->disableOriginalConstructor()
1352+
->setMethods(['destroy'])
1353+
->getMockForAbstractClass();
1354+
1355+
$dateTime = '2017-10-25 18:57:08';
1356+
$timestamp = '1508983028';
1357+
$dateTimeMock = $this->getMockBuilder(\DateTime::class)
1358+
->disableOriginalConstructor()
1359+
->setMethods(['format', 'getTimestamp', 'setTimestamp'])
1360+
->getMock();
1361+
1362+
$dateTimeMock->expects($this->any())
1363+
->method('format')
1364+
->with(\Magento\Framework\Stdlib\DateTime::DATETIME_PHP_FORMAT)
1365+
->willReturn($dateTime);
1366+
1367+
$dateTimeMock
1368+
->expects($this->any())
1369+
->method('getTimestamp')
1370+
->willReturn($timestamp);
1371+
1372+
$dateTimeMock
1373+
->expects($this->any())
1374+
->method('setTimestamp')
1375+
->willReturnSelf();
1376+
1377+
$dateTimeFactory = $this->getMockBuilder(DateTimeFactory::class)
1378+
->disableOriginalConstructor()
1379+
->setMethods(['create'])
1380+
->getMock();
1381+
$dateTimeFactory->expects($this->any())->method('create')->willReturn($dateTimeMock);
13001382

13011383
$this->objectManagerHelper = new ObjectManagerHelper($this);
13021384
$this->accountManagement = $this->objectManagerHelper->getObject(
@@ -1306,7 +1388,16 @@ private function reInitModel()
13061388
'customerRegistry' => $this->customerRegistry,
13071389
'customerRepository' => $this->customerRepository,
13081390
'customerModel' => $this->customer,
1309-
'dateTimeFactory' => $this->dateTimeFactory,
1391+
'dateTimeFactory' => $dateTimeFactory,
1392+
'stringHelper' => $this->string,
1393+
'scopeConfig' => $this->scopeConfig,
1394+
'sessionManager' => $this->sessionManager,
1395+
'visitorCollectionFactory' => $this->visitorCollectionFactory,
1396+
'saveHandler' => $this->saveHandler,
1397+
'encryptor' => $this->encryptor,
1398+
'dataProcessor' => $this->dataObjectProcessor,
1399+
'storeManager' => $this->storeManager,
1400+
'transportBuilder' => $this->transportBuilder,
13101401
]
13111402
);
13121403
$reflection = new \ReflectionClass(get_class($this->accountManagement));
@@ -1326,6 +1417,7 @@ public function testChangePassword()
13261417
$newPassword = 'abcdefg';
13271418
$passwordHash = '1a2b3f4c';
13281419

1420+
$this->reInitModel();
13291421
$customer = $this->getMockBuilder(\Magento\Customer\Api\Data\CustomerInterface::class)
13301422
->getMock();
13311423
$customer->expects($this->any())
@@ -1341,24 +1433,20 @@ public function testChangePassword()
13411433
$this->authenticationMock->expects($this->once())
13421434
->method('authenticate');
13431435

1344-
$customerSecure = $this->getMockBuilder(\Magento\Customer\Model\Data\CustomerSecure::class)
1345-
->setMethods(['setRpToken', 'setRpTokenCreatedAt', 'getPasswordHash'])
1346-
->disableOriginalConstructor()
1347-
->getMock();
1348-
$customerSecure->expects($this->once())
1436+
$this->customerSecure->expects($this->once())
13491437
->method('setRpToken')
13501438
->with(null);
1351-
$customerSecure->expects($this->once())
1439+
$this->customerSecure->expects($this->once())
13521440
->method('setRpTokenCreatedAt')
13531441
->willReturnSelf();
1354-
$customerSecure->expects($this->any())
1442+
$this->customerSecure->expects($this->any())
13551443
->method('getPasswordHash')
13561444
->willReturn($passwordHash);
13571445

13581446
$this->customerRegistry->expects($this->any())
13591447
->method('retrieveSecureData')
13601448
->with($customerId)
1361-
->willReturn($customerSecure);
1449+
->willReturn($this->customerSecure);
13621450

13631451
$this->scopeConfig->expects($this->any())
13641452
->method('getValue')
@@ -1388,9 +1476,85 @@ public function testChangePassword()
13881476
->method('save')
13891477
->with($customer);
13901478

1479+
$this->sessionManager->expects($this->atLeastOnce())->method('start');
1480+
$this->sessionManager->expects($this->atLeastOnce())->method('writeClose');
1481+
$this->sessionManager->expects($this->atLeastOnce())->method('getSessionId');
1482+
1483+
$visitor = $this->getMockBuilder(\Magento\Customer\Model\Visitor::class)
1484+
->disableOriginalConstructor()
1485+
->setMethods(['getSessionId'])
1486+
->getMock();
1487+
$visitor->expects($this->at(0))->method('getSessionId')->willReturn('session_id_1');
1488+
$visitor->expects($this->at(1))->method('getSessionId')->willReturn('session_id_2');
1489+
$visitorCollection = $this->getMockBuilder(
1490+
\Magento\Customer\Model\ResourceModel\Visitor\CollectionFactory::class
1491+
)
1492+
->disableOriginalConstructor()->setMethods(['addFieldToFilter', 'getItems'])->getMock();
1493+
$visitorCollection->expects($this->atLeastOnce())->method('addFieldToFilter')->willReturnSelf();
1494+
$visitorCollection->expects($this->atLeastOnce())->method('getItems')->willReturn([$visitor, $visitor]);
1495+
$this->visitorCollectionFactory->expects($this->atLeastOnce())->method('create')
1496+
->willReturn($visitorCollection);
1497+
$this->saveHandler->expects($this->at(0))->method('destroy')->with('session_id_1');
1498+
$this->saveHandler->expects($this->at(1))->method('destroy')->with('session_id_2');
1499+
13911500
$this->assertTrue($this->accountManagement->changePassword($email, $currentPassword, $newPassword));
13921501
}
13931502

1503+
public function testResetPassword()
1504+
{
1505+
$customerEmail = 'customer@example.com';
1506+
$customerId = '1';
1507+
$resetToken = 'newStringToken';
1508+
$newPassword = 'new_password';
1509+
1510+
$this->reInitModel();
1511+
$customer = $this->getMockBuilder(\Magento\Customer\Api\Data\CustomerInterface::class)->getMock();
1512+
$customer->expects($this->any())->method('getId')->willReturn($customerId);
1513+
$this->customerRepository->expects($this->atLeastOnce())->method('get')->with($customerEmail)
1514+
->willReturn($customer);
1515+
$this->customer->expects($this->atLeastOnce())->method('getResetPasswordLinkExpirationPeriod')
1516+
->willReturn(100000);
1517+
$this->string->expects($this->any())->method('strlen')->willReturnCallback(
1518+
function ($string) {
1519+
return strlen($string);
1520+
}
1521+
);
1522+
$this->customerRegistry->expects($this->atLeastOnce())->method('retrieveSecureData')
1523+
->willReturn($this->customerSecure);
1524+
1525+
$this->customerSecure->expects($this->once())
1526+
->method('setRpToken')
1527+
->with(null);
1528+
$this->customerSecure->expects($this->once())
1529+
->method('setRpTokenCreatedAt')
1530+
->with(null);
1531+
$this->customerSecure->expects($this->any())
1532+
->method('setPasswordHash')
1533+
->willReturn(null);
1534+
1535+
$this->sessionManager->expects($this->atLeastOnce())->method('destroy');
1536+
$this->sessionManager->expects($this->atLeastOnce())->method('start');
1537+
$this->sessionManager->expects($this->atLeastOnce())->method('writeClose');
1538+
$this->sessionManager->expects($this->atLeastOnce())->method('getSessionId');
1539+
$visitor = $this->getMockBuilder(\Magento\Customer\Model\Visitor::class)
1540+
->disableOriginalConstructor()
1541+
->setMethods(['getSessionId'])
1542+
->getMock();
1543+
$visitor->expects($this->at(0))->method('getSessionId')->willReturn('session_id_1');
1544+
$visitor->expects($this->at(1))->method('getSessionId')->willReturn('session_id_2');
1545+
$visitorCollection = $this->getMockBuilder(
1546+
\Magento\Customer\Model\ResourceModel\Visitor\CollectionFactory::class
1547+
)
1548+
->disableOriginalConstructor()->setMethods(['addFieldToFilter', 'getItems'])->getMock();
1549+
$visitorCollection->expects($this->atLeastOnce())->method('addFieldToFilter')->willReturnSelf();
1550+
$visitorCollection->expects($this->atLeastOnce())->method('getItems')->willReturn([$visitor, $visitor]);
1551+
$this->visitorCollectionFactory->expects($this->atLeastOnce())->method('create')
1552+
->willReturn($visitorCollection);
1553+
$this->saveHandler->expects($this->at(0))->method('destroy')->with('session_id_1');
1554+
$this->saveHandler->expects($this->at(1))->method('destroy')->with('session_id_2');
1555+
$this->assertTrue($this->accountManagement->resetPassword($customerEmail, $resetToken, $newPassword));
1556+
}
1557+
13941558
/**
13951559
* @return void
13961560
*/

0 commit comments

Comments
 (0)