Fix hardcoded encryption salt vulnerability in SecureStorage

src/device_fingerprinting/secure_storage.py

@@ -41,6 +41,8 @@ def __init__(
         self.key_iterations = key_iterations
         self._password = password
         self._encryptor = None
+        self._salt = None
+        self._salt_loaded = False
         self.data: Dict[str, Any] = {}
 
         if not self._password and keyring:
@@ -83,44 +85,83 @@ def _set_password_in_keyring(self, password: str):
             pass
 
     def _setup_encryptor(self):
-        """Sets up the encryptor instance variable."""
-        # Derive a key from the password
-        # In a real application, the salt should be stored with the encrypted data
-        salt = b"\\x00" * 16
+        """Sets up the encryptor with proper random salt."""
+        if os.path.exists(self.file_path):
+            # Try to load salt from existing file (new format)
+            try:
+                with open(self.file_path, 'rb') as f:
+                    self._salt = f.read(16)
+                    if len(self._salt) == 16:
+                        self._salt_loaded = True
+                    else:
+                        # File too short - generate new salt
+                        self._salt = os.urandom(16)
+                        self._salt_loaded = False
+            except (IOError, OSError):
+                self._salt = os.urandom(16)
+                self._salt_loaded = False
+        else:
+            # Generate random salt for new files
+            self._salt = os.urandom(16)
+            self._salt_loaded = False
+
         kdf = ScryptKDF()
-        self._key = kdf.derive_key(self._password, salt)
+        self._key = kdf.derive_key(self._password, self._salt)
         self._encryptor = AESGCMEncryptor()
 
     def save(self):
         """
-        Saves the data to the file.
+        Saves the data to the file with salt prepended.
         """
         if not self._encryptor:
             self._setup_encryptor()
 
         json_data = json.dumps(self.data).encode("utf-8")
         encrypted_blob = self._encryptor.encrypt(json_data, self._key)
 
+        # Write salt + encrypted data
         with open(self.file_path, "wb") as f:
+            f.write(self._salt)
             f.write(encrypted_blob)
 
     def load(self):
         """
-        Loads and decrypts the data from the file.
+        Loads and decrypts the data from the file, reading salt from file.
+        Supports both new format (with salt) and old format (without salt).
         """
         if not self._encryptor:
             self._setup_encryptor()
 
         with open(self.file_path, "rb") as f:
+            if self._salt_loaded:
+                # New format: skip salt prefix
+                f.seek(16)
+            # Old format or file too short: read from beginning
             encrypted_blob = f.read()
 
         try:
             decrypted_data = self._encryptor.decrypt(encrypted_blob, self._key)
             self.data = json.loads(decrypted_data)
         except (ValueError, InvalidTag) as e:
-            raise IOError(
-                f"Failed to decrypt or load data. Incorrect password or corrupted file. Reason: {e}"
-            )
+            # Try old format (no salt prefix) for backward compatibility
+            with open(self.file_path, "rb") as f:
+                encrypted_blob_old = f.read()
+
+            # Use hardcoded salt for old format
+            old_salt = b"\x00" * 16
+            kdf = ScryptKDF()
+            old_key = kdf.derive_key(self._password, old_salt)
+
+            try:
+                decrypted_data = self._encryptor.decrypt(encrypted_blob_old, old_key)
+                self.data = json.loads(decrypted_data)
+                # Successfully loaded old format - generate new salt for migration on next save
+                self._salt = os.urandom(16)
+                self._key = kdf.derive_key(self._password, self._salt)
+            except (ValueError, InvalidTag):
+                raise IOError(
+                    f"Failed to decrypt or load data. Incorrect password or corrupted file. Reason: {e}"
+                )
         except json.JSONDecodeError:
             raise IOError("File is corrupted and does not contain valid JSON.")
 

tests/test_secure_storage.py

@@ -1,8 +1,10 @@
 import unittest
 import os
+import json
 from unittest.mock import patch, MagicMock
 
 from device_fingerprinting.secure_storage import SecureStorage
+from device_fingerprinting.crypto import AESGCMEncryptor, ScryptKDF
 
 
 class TestSecureStorage(unittest.TestCase):
@@ -101,6 +103,155 @@ def test_context_manager_saves_on_exit(self):
         with SecureStorage(self.test_file, self.password) as reloaded_store:
             self.assertEqual(reloaded_store.get_item("auto_saved"), "data")
 
+    def test_different_salts_for_new_files(self):
+        """Test that different files get different random salts."""
+        test_file1 = "test_salt_1.bin"
+        test_file2 = "test_salt_2.bin"
+
+        try:
+            # Create two separate storage files with same password
+            with SecureStorage(test_file1, self.password) as store1:
+                store1.set_item("key", "value")
+
+            with SecureStorage(test_file2, self.password) as store2:
+                store2.set_item("key", "value")
+
+            # Read the first 16 bytes (salt) from each file
+            with open(test_file1, "rb") as f:
+                salt1 = f.read(16)
+
+            with open(test_file2, "rb") as f:
+                salt2 = f.read(16)
+
+            # Salts should be different
+            self.assertNotEqual(salt1, salt2)
+            self.assertEqual(len(salt1), 16)
+            self.assertEqual(len(salt2), 16)
+        finally:
+            if os.path.exists(test_file1):
+                os.remove(test_file1)
+            if os.path.exists(test_file2):
+                os.remove(test_file2)
+
+    def test_same_password_different_salts_produces_different_keys(self):
+        """Test that same password with different salts produces different encryption results."""
+        test_file1 = "test_key_1.bin"
+        test_file2 = "test_key_2.bin"
+
+        try:
+            # Create two storage files with same password and same data
+            with SecureStorage(test_file1, self.password) as store1:
+                store1.set_item("test", "same_data")
+
+            with SecureStorage(test_file2, self.password) as store2:
+                store2.set_item("test", "same_data")
+
+            # Read the encrypted content (after the salt)
+            with open(test_file1, "rb") as f:
+                f.seek(16)  # Skip salt
+                encrypted1 = f.read()
+
+            with open(test_file2, "rb") as f:
+                f.seek(16)  # Skip salt
+                encrypted2 = f.read()
+
+            # Encrypted content should be different due to different salts
+            self.assertNotEqual(encrypted1, encrypted2)
+        finally:
+            if os.path.exists(test_file1):
+                os.remove(test_file1)
+            if os.path.exists(test_file2):
+                os.remove(test_file2)
+
+    def test_salt_persists_across_save_load_cycles(self):
+        """Test that salt is stored and correctly loaded from file."""
+        # Create and save data
+        with SecureStorage(self.test_file, self.password) as store:
+            store.set_item("persistent", "data")
+
+        # Read the salt from file
+        with open(self.test_file, "rb") as f:
+            original_salt = f.read(16)
+
+        # Load the file again and save without changes
+        with SecureStorage(self.test_file, self.password) as store:
+            # Access data to ensure it loaded correctly
+            self.assertEqual(store.get_item("persistent"), "data")
+            # Add new data
+            store.set_item("more", "data")
+
+        # Read the salt again - should be the same
+        with open(self.test_file, "rb") as f:
+            new_salt = f.read(16)
+
+        self.assertEqual(original_salt, new_salt)
+
+        # Verify data is still accessible
+        with SecureStorage(self.test_file, self.password) as store:
+            self.assertEqual(store.get_item("persistent"), "data")
+            self.assertEqual(store.get_item("more"), "data")
+
+    def test_salt_is_random_not_hardcoded(self):
+        """Test that salt is not the hardcoded all-zero value."""
+        with SecureStorage(self.test_file, self.password) as store:
+            store.set_item("test", "value")
+
+        with open(self.test_file, "rb") as f:
+            salt = f.read(16)
+
+        # Salt should NOT be all zeros
+        hardcoded_salt = b"\x00" * 16
+        self.assertNotEqual(salt, hardcoded_salt)
+
+        # Salt should have some randomness (very unlikely to have all same bytes)
+        # This is a probabilistic test, but with 16 random bytes, 
+        # having all same is virtually impossible
+        unique_bytes = len(set(salt))
+        self.assertGreater(unique_bytes, 1)
+
+    def test_backward_compatibility_with_old_format(self):
+        """Test that files created with old format (hardcoded salt) can still be loaded."""
+        # Create an old format file (no salt prefix, using hardcoded salt)
+        old_format_file = "test_old_format.bin"
+
+        try:
+            # Simulate old format: encrypt data with hardcoded salt
+            old_salt = b"\x00" * 16
+            kdf = ScryptKDF()
+            old_key = kdf.derive_key(self.password, old_salt)
+            encryptor = AESGCMEncryptor()
+
+            test_data = {"legacy": "data", "version": "old"}
+            json_data = json.dumps(test_data).encode("utf-8")
+            encrypted_blob = encryptor.encrypt(json_data, old_key)
+
+            # Write old format file (no salt prefix)
+            with open(old_format_file, "wb") as f:
+                f.write(encrypted_blob)
+
+            # Now try to load with new SecureStorage (should handle backward compatibility)
+            with SecureStorage(old_format_file, self.password) as store:
+                self.assertEqual(store.get_item("legacy"), "data")
+                self.assertEqual(store.get_item("version"), "old")
+                # Add new data to trigger migration
+                store.set_item("migrated", "new_data")
+
+            # After migration, file should have new format with random salt
+            with open(old_format_file, "rb") as f:
+                new_salt = f.read(16)
+
+            # New salt should not be the hardcoded one
+            self.assertNotEqual(new_salt, old_salt)
+
+            # Verify data is still accessible after migration
+            with SecureStorage(old_format_file, self.password) as store:
+                self.assertEqual(store.get_item("legacy"), "data")
+                self.assertEqual(store.get_item("migrated"), "new_data")
+
+        finally:
+            if os.path.exists(old_format_file):
+                os.remove(old_format_file)
+
 
 if __name__ == "__main__":
     unittest.main()