Fix hardcoded encryption salt vulnerability in SecureStorage

[Copilot]

SecureStorage used a hardcoded all-zero salt (b"\x00" * 16) for key derivation, making all users with identical passwords share the same encryption key and vulnerable to rainbow table attacks.

Changes

• Generate random salt per file: Use os.urandom(16) for new storage files, store salt as first 16 bytes
• Load salt from existing files: Read salt prefix in _setup_encryptor(), track with _salt_loaded flag
• Backward compatibility: Fallback to hardcoded salt when new format decryption fails, migrate on next save
• Test coverage: Added 5 tests validating salt uniqueness, persistence, and migration

Implementation

File format: [16-byte salt][12-byte nonce][ciphertext][16-byte tag]

def _setup_encryptor(self):
    if os.path.exists(self.file_path):
        with open(self.file_path, 'rb') as f:
            self._salt = f.read(16)
            self._salt_loaded = len(self._salt) == 16
    else:
        self._salt = os.urandom(16)  # Random salt, not hardcoded
        self._salt_loaded = False
    
    kdf = ScryptKDF()
    self._key = kdf.derive_key(self._password, self._salt)

def load(self):
    with open(self.file_path, "rb") as f:
        if self._salt_loaded:
            f.seek(16)  # Skip salt prefix for new format
        encrypted_blob = f.read()
    
    try:
        self.data = json.loads(self._encryptor.decrypt(encrypted_blob, self._key))
    except (ValueError, InvalidTag):
        # Fallback: try old format with hardcoded salt
        # ... migration logic ...

Different files now produce different encryption keys even with the same password, restoring salt's security purpose.

Original prompt

Critical Security Fix: Hardcoded Encryption Salt

Problem

The SecureStorage class in src/device_fingerprinting/secure_storage.py uses a hardcoded all-zero salt (salt = b"\\x00" * 16) for key derivation, which completely defeats the purpose of salting and makes all users with the same password vulnerable to rainbow table attacks.

Security Impact

• All users with the same password have identical encryption keys
• Vulnerable to pre-computed rainbow table attacks
• Violates NIST SP 800-132 and OWASP cryptographic best practices
• No protection from key derivation function

Required Changes

File: src/device_fingerprinting/secure_storage.py

1. Generate a random 16-byte salt using os.urandom(16) on first use
2. Store the salt as the first 16 bytes of the encrypted file
3. Read the salt from the file when loading existing data
4. Update _setup_encryptor() to handle salt loading/generation
5. Update save() to write salt before encrypted data
6. Update load() to read salt before decrypting data
7. Add backward compatibility handling for files without salt
8. Add unit tests verifying different salts for same password

Implementation Details

def _setup_encryptor(self):
    """Sets up the encryptor with proper random salt."""
    if os.path.exists(self.file_path):
        # Load salt from existing file
        try:
            with open(self.file_path, 'rb') as f:
                self._salt = f.read(16)
                if len(self._salt) != 16:
                    # Corrupted file or old format - generate new salt
                    self._salt = os.urandom(16)
        except (IOError, OSError):
            self._salt = os.urandom(16)
    else:
        # Generate random salt for new files
        self._salt = os.urandom(16)
    
    kdf = ScryptKDF()
    self._key = kdf.derive_key(self._password, self._salt)
    self._encryptor = AESGCMEncryptor()

def save(self):
    """Saves data 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 data, reading salt from file."""
    if not self._encryptor:
        self._setup_encryptor()
    
    with open(self.file_path, "rb") as f:
        # Salt already read in _setup_encryptor
        f.seek(16)  # Skip salt
        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: {e}")

Testing Requirements

• Add test verifying different salts are generated for new files
• Add test verifying same password with different salts produces different keys
• Add test for loading files with stored salt
• Add test for backward compatibility with old files

Priority

🔴 CRITICAL - This is a fundamental cryptographic flaw

References

• OWASP Password Storage Cheat Sheet
• NIST SP 800-132

This pull request was created from Copilot chat.

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[Copilot]

Pull request overview

This PR fixes a critical security vulnerability in the SecureStorage class where a hardcoded all-zero salt was used for key derivation, making users with identical passwords share the same encryption key and vulnerable to rainbow table attacks.

Changes:

• Generate random salts using os.urandom(16) for new storage files and store them as the first 16 bytes of encrypted files
• Load salts from existing files in _setup_encryptor() with _salt_loaded flag to track format
• Implement backward compatibility fallback that migrates old format files (without salt prefix) to new format on next save
• Add comprehensive test coverage with 5 new tests validating salt uniqueness, persistence, and migration

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
src/device_fingerprinting/secure_storage.py	Implements random salt generation, file format changes to prepend salt, and backward compatibility logic for loading old-format files
tests/test_secure_storage.py	Adds 5 comprehensive tests covering different salts for new files, encryption differences, salt persistence, non-hardcoded salt validation, and backward compatibility migration

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The documentation comment should be updated to describe the new file format structure. Currently it just says "Saves the data to the file with salt prepended" but it would be helpful to document the complete format: [16-byte salt][12-byte nonce][ciphertext][16-byte tag].

This documentation would help future maintainers understand the file format and is especially important for a security-critical component.

Suggested change

	Saves the data to the file with salt prepended.
	Encrypts and saves the current data to ``self.file_path``.
	The file format is:
	[16-byte salt][12-byte nonce][ciphertext][16-byte tag]
	where:
	- ``salt`` is the 16-byte random value used for key derivation.
	- ``nonce`` is the 12-byte AES-GCM nonce.
	- ``ciphertext`` is the encrypted JSON-serialized payload.
	- ``tag`` is the 16-byte AES-GCM authentication tag.

[Copilot]

In the backward compatibility scenario, after successfully loading old format data and generating a new salt, the code updates 'self._salt' and 'self._key' but doesn't set 'self._salt_loaded = True'. This means on the next save() call within the same session, it will write the new salt correctly. However, this state inconsistency could be confusing.

Consider setting 'self._salt_loaded = True' after successful migration (line 160) to maintain consistent state, indicating that we now have a proper salt that will be persisted.

Suggested change

	self._key = kdf.derive_key(self._password, self._salt)
	self._key = kdf.derive_key(self._password, self._salt)
	self._salt_loaded = True

[Copilot]

The logic for determining whether a file is in the new format has an edge case issue. If an existing file has exactly 16 bytes or fewer (but not zero), those bytes will be interpreted as a salt, and then the code will attempt to decrypt an empty or very short blob. This could happen with a corrupted file or during a write interruption.

Consider explicitly checking the minimum file size. A valid new format file should be at least 16 (salt) + 12 (nonce) + 16 (tag) = 44 bytes. Files shorter than this cannot be valid new format files and should either generate a new salt or raise an error rather than trying to use the first 16 bytes as a salt.

[Copilot]

The exception handling in the fallback logic loses the original exception context. When the old format decryption also fails, the error message references 'e' from the outer exception handler, but this could be confusing if both new and old format decryption fail for different reasons.

Consider capturing the original exception in a variable before attempting the fallback, and providing both exceptions in the final error message to give better debugging information. For example: "Failed to decrypt with both new format (Reason: {new_error}) and old format (Reason: {old_error})".

[Copilot]

This probabilistic test assertion could theoretically fail with valid random data. While extremely unlikely, it's possible (though with probability less than 1 in 10^38) for os.urandom(16) to return 16 identical bytes.

Consider either removing this assertion or replacing it with a comment explaining that this is a probabilistic check. The primary assertion on line 204 (checking it's not all zeros) is sufficient to verify the security fix. Alternatively, you could check that the salt is not equal to any known weak patterns rather than checking uniqueness of bytes.

Suggested change

	# 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)
	# Salt should NOT be all zeros (regression test for hardcoded salt bug)
	hardcoded_salt = b"\x00" * 16
	self.assertNotEqual(salt, hardcoded_salt)
	# NOTE: We intentionally avoid additional probabilistic assertions here
	# (for example, checking that all bytes are not identical), because such
	# checks can legitimately fail with extremely low probability when using
	# a secure random source. The non-zero check above is sufficient to verify
	# that the previous hardcoded-salt behavior has been fixed.

[Copilot]

The backward compatibility fallback has a logical issue. When '_salt_loaded' is True but the file is actually in the old format (e.g., an old format file that happens to start with 16 bytes that look like a salt), the code will skip those first 16 bytes, potentially reading the wrong data for decryption.

This edge case could occur if an old-format encrypted file happens to start with 16 bytes that don't trigger an immediate decryption error. The fallback on line 147 will then read from the beginning again, which is correct, but the logic assumes that any decryption failure with '_salt_loaded=True' means we should try old format. A more robust approach would be to check file size or add a magic header to distinguish formats definitively.

Suggested change

	# 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):
	# Only try old format (no salt prefix) when salt was not loaded,
	# to avoid incorrectly treating new-format files as old-format.
	if not self._salt_loaded:
	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}"
	)
	else:
	# For files where a salt was already loaded, decryption failure
	# indicates an incorrect password or corrupted file; do not
	# attempt legacy format decryption.

[Copilot]

This test assumes that different salts will always produce different encrypted outputs, but this is testing the wrong layer. The encrypted content differs primarily because AES-GCM uses a random nonce for each encryption (generated in AESGCMEncryptor.encrypt()), not because of different salts. Different salts produce different keys, but the nonce randomization is what makes the ciphertext different.

While this test will pass (because both salt and nonce contribute to differences), the test name and comment suggest it's testing salt-based differences specifically. Consider renaming to clarify that you're testing that the overall encryption produces different results, or add a more direct test that verifies different salts produce different keys by using the KDF directly.