From 38efe9102d3d19ef37ac9d7246727d79f145c63f Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 6 Apr 2023 20:58:23 -0400 Subject: [PATCH] Island: Use SHA256 to hash OTPs The encryption algorithm is not deterministic, making searching impossible. Salted SHA256 is considered to be secure enough for one-time passwords with a 2-minute TTL. Issue #3078 PR #3204 --- .../mongo_otp_repository.py | 33 ++++++++++++++----- .../test_mongo_otp_repository.py | 9 ++--- 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/monkey/monkey_island/cc/services/authentication_service/mongo_otp_repository.py b/monkey/monkey_island/cc/services/authentication_service/mongo_otp_repository.py index d98d1041549..652e80ea3dc 100644 --- a/monkey/monkey_island/cc/services/authentication_service/mongo_otp_repository.py +++ b/monkey/monkey_island/cc/services/authentication_service/mongo_otp_repository.py @@ -1,3 +1,5 @@ +import hashlib +import secrets from functools import lru_cache from typing import Any, Mapping @@ -12,7 +14,6 @@ StorageError, UnknownRecordError, ) -from monkey_island.cc.server_utils.encryption import ILockableEncryptor from .i_otp_repository import IOTPRepository @@ -21,21 +22,36 @@ class MongoOTPRepository(IOTPRepository): def __init__( self, mongo_client: MongoClient, - encryptor: ILockableEncryptor, ): - self._encryptor = encryptor + # SECURITY: A new salt is generated for each instance of this repository. This effectively + # makes all OTPS invalid on startup. + self._salt = secrets.token_bytes(16) + self._otp_collection = mongo_client.monkey_island.otp self._otp_collection.create_index("otp", unique=True) def insert_otp(self, otp: OTP, expiration: float): try: - encrypted_otp = self._encryptor.encrypt(otp.get_secret_value().encode()) self._otp_collection.insert_one( - {"otp": encrypted_otp, "expiration_time": expiration, "used": False} + {"otp": self._hash_otp(otp), "expiration_time": expiration, "used": False} ) except Exception as err: raise StorageError(f"Error inserting OTP: {err}") + def _hash_otp(self, otp: OTP) -> bytes: + # SECURITY: A single round of salted SHA256 is usually not considered sufficient for + # protecting passwords. However, OTPs have a very short life span (2 minutes at the time of + # this writing). Additionally, they can only be used once. Finally, they are 32 bytes long. + # At the present time, we consider this to be sufficient protection. I'm unaware of any + # technology in existence that can brute force SHA256 for (roughly) 48-byte inputs in under + # 2 minutes. + # + # Note that if any of these conditions change (timeouts become very long or OTPs become very + # short), this should be revisited. For now, we prefer the significantly faster performance + # of a single round of salted SHA256 over a more secure but slower algorithm. + otp_bytes = otp.get_secret_value().encode() + return hashlib.sha256(self._salt + otp_bytes).digest() + def set_used(self, otp: OTP): try: otp_id = self._get_otp_object_id(otp) @@ -71,11 +87,10 @@ def _get_otp_document(self, otp: OTP) -> Mapping[str, Any]: @lru_cache def _get_otp_object_id(self, otp: OTP) -> ObjectId: - otp_str = otp.get_secret_value() - try: - encrypted_otp = self._encryptor.encrypt(otp_str.encode()) - otp_dict = self._otp_collection.find_one({"otp": encrypted_otp}, [MONGO_OBJECT_ID_KEY]) + otp_dict = self._otp_collection.find_one( + {"otp": self._hash_otp(otp)}, [MONGO_OBJECT_ID_KEY] + ) except Exception as err: raise RetrievalError(f"Error retrieving OTP: {err}") diff --git a/monkey/tests/unit_tests/monkey_island/cc/services/authentication_service/test_mongo_otp_repository.py b/monkey/tests/unit_tests/monkey_island/cc/services/authentication_service/test_mongo_otp_repository.py index 00e4e9f5e65..03546c2092d 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/services/authentication_service/test_mongo_otp_repository.py +++ b/monkey/tests/unit_tests/monkey_island/cc/services/authentication_service/test_mongo_otp_repository.py @@ -11,7 +11,6 @@ StorageError, UnknownRecordError, ) -from monkey_island.cc.server_utils.encryption import ILockableEncryptor from monkey_island.cc.services.authentication_service.i_otp_repository import IOTPRepository from monkey_island.cc.services.authentication_service.mongo_otp_repository import MongoOTPRepository @@ -35,10 +34,8 @@ def mongo_client() -> mongomock.MongoClient: @pytest.fixture -def otp_repository( - mongo_client: mongomock.MongoClient, repository_encryptor: ILockableEncryptor -) -> IOTPRepository: - return MongoOTPRepository(mongo_client, repository_encryptor) +def otp_repository(mongo_client: mongomock.MongoClient) -> IOTPRepository: + return MongoOTPRepository(mongo_client) @pytest.fixture @@ -59,7 +56,7 @@ def error_raising_mongo_client() -> mongomock.MongoClient: def error_raising_otp_repository( error_raising_mongo_client, repository_encryptor ) -> IOTPRepository: - return MongoOTPRepository(error_raising_mongo_client, repository_encryptor) + return MongoOTPRepository(error_raising_mongo_client) def test_insert_otp(otp_repository: IOTPRepository):