Add a secure cache to Windows Hello to make it usable (amount of prompts) #105
Conversation
comes up with the RequestSignAsync call
This reverts commit 234a3fa.
Needed for more than one vault Fixes HMAC verification failure
|
""" WalkthroughThe changes update two areas of the codebase. In the Java module descriptor ( In the native code ( In the Java class Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (2)
28-28: Concurrent caching strategy is sound but consider cache eviction.
Defining a global cache (keyCache) guarded bycacheMutexis correct to avoid data races. However, you may want to periodically invalidate or limit the cache size to prevent indefinite growth or stale entries persisting in memory.Also applies to: 30-47, 49-53, 55-55
200-234: Signing the challenge and populating the cache is correct.
The logic for creating or opening the Windows Hello credential, signing the challenge, and then protecting/storing it in the cache is appropriate. Consider clarifying behavior for user-canceled prompts (line 215) if enhanced UX is desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/module-info.java(1 hunks)src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (1)
Learnt from: purejava
PR: cryptomator/integrations-win#83
File: src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp:0-0
Timestamp: 2025-04-03T12:51:14.634Z
Learning: In `src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp`, additional error handling and input validation in the `deriveEncryptionKey` function may not be necessary unless specific issues are identified.
🔇 Additional comments (8)
src/main/java/module-info.java (1)
23-23: Provision of multiple keychain providers looks good.
AddingWindowsHelloKeychainAccessalongsideWindowsProtectedKeychainAccessappears consistent with the PR’s objective to provide both functionalities for keychain access.src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (7)
9-9: Includes for cryptographic operations and hashing are appropriate.
These newly added headers (<wincrypt.h>,<unordered_map>,<mutex>, and<functional>) are necessary to implement the secure caching and memory protection functionalities.Also applies to: 10-11, 19-19
58-58: Memory protection functions are correctly implemented.
UsingCryptProtectMemoryandCryptUnprotectMemoryto protect sensitive data is a reputable approach. Ensure these functions remain compatible with your targeted Windows versions.Also applies to: 59-65, 67-73
178-178: Passing keyId by reference is a performance and clarity improvement.
Changingbool deriveEncryptionKey(const std::wstring keyId, ...)tobool deriveEncryptionKey(const std::wstring& keyId, ...)avoids unnecessary copies.
184-185: Securely retrieving and unprotecting data from cache is well-handled.
The lock-based retrieval ensures thread-safe access, and the explicit unprotection step prevents storing plaintext in memory. This flow aligns with secure caching requirements.Also applies to: 187-198
236-238: Converting signature data into an IBuffer is implemented correctly.
This step is straightforward and consistent with your subsequent HKDF usage.
241-242: Zeroing out sensitive data helps prevent lingering in memory.
OverwritingsignatureDataafter deriving the key is a good security practice, reducing the window of exposure.
244-244: Final return statement concludes the flow gracefully.
No issues noted; the function clearly returns success at this point.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (2)
165-169: Consider adding cache invalidation mechanism.While the current implementation works well for caching Windows Hello signatures, there's no mechanism to invalidate or expire cached entries. Consider adding time-based expiration or a way to clear the cache when certain security events occur.
static std::mutex cacheMutex; -static std::unordered_map<std::wstring, std::vector<uint8_t>> keyCache; +// Cache entry with timestamp for potential expiration +struct CacheEntry { + std::vector<uint8_t> data; + std::chrono::system_clock::time_point timestamp; +}; +static std::unordered_map<std::wstring, CacheEntry> keyCache; + +// Helper to clear cache (can be exposed via JNI if needed) +void clearCache() { + std::lock_guard<std::mutex> lock(cacheMutex); + keyCache.clear(); +}And then in the cache lookup code:
auto it = keyCache.find(keyId); if (it != keyCache.end()) { - signatureData = it->second; + // Optional: Check if entry has expired (e.g., after 30 minutes) + auto now = std::chrono::system_clock::now(); + if (now - it->second.timestamp > std::chrono::minutes(30)) { + keyCache.erase(it); + } else { + signatureData = it->second.data; + if (!UnprotectMemory(signatureData)) { + throw std::runtime_error("Failed to unprotect memory."); + } + foundInCache = true; + } - if (!UnprotectMemory(signatureData)) { - throw std::runtime_error("Failed to unprotect memory."); - } - foundInCache = true; }And when storing:
{ std::lock_guard<std::mutex> lock(cacheMutex); - keyCache[keyId] = protectedCopy; + keyCache[keyId] = {protectedCopy, std::chrono::system_clock::now()}; }
173-174: Consider adding a JNI method to pre-cache keys.Since the core functionality already supports caching, consider exposing a JNI method that allows pre-caching keys during application startup or at strategic points in the user workflow to further reduce prompts.
This would allow the Java layer to proactively trigger the Windows Hello prompt at the most appropriate time for the user, further improving usability while maintaining security.
// Example JNI method to pre-cache a key jboolean JNICALL Java_org_cryptomator_windows_keychain_WindowsHello_00024Native_preCacheKey (JNIEnv* env, jobject obj, jbyteArray keyId, jbyteArray challenge) { queueSecurityPromptFocus(); try { std::vector<uint8_t> challengeVec = jbyteArrayToVector(env, challenge); winrt::init_apartment(winrt::apartment_type::single_threaded); auto toReleaseKeyId = (LPCWSTR)env->GetByteArrayElements(keyId, NULL); const std::wstring keyIdentifier(toReleaseKeyId); IBuffer key; bool result = deriveEncryptionKey(keyIdentifier, challengeVec, key); env->ReleaseByteArrayElements(keyId, (jbyte*)toReleaseKeyId, JNI_ABORT); return result ? JNI_TRUE : JNI_FALSE; } catch (...) { return JNI_FALSE; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (2)
Learnt from: purejava
PR: cryptomator/integrations-win#83
File: src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp:0-0
Timestamp: 2024-11-16T17:26:11.536Z
Learning: In `src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp`, additional error handling and input validation in the `deriveEncryptionKey` function may not be necessary unless specific issues are identified.
Learnt from: purejava
PR: cryptomator/integrations-win#83
File: src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp:0-0
Timestamp: 2024-11-18T20:38:43.547Z
Learning: In `deriveEncryptionKey` in `src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp`, additional null checks on `result.Credential()` are unnecessary because the Windows API guarantees it is non-null or throws an exception otherwise.
🔇 Additional comments (9)
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (9)
9-11: Appropriate includes added for new functionality.The additions of
wincrypt.h,unordered_map, andmutexare appropriate for implementing the secure caching mechanism with thread safety.
27-28: Thread-safe cache implementation.Good implementation of a thread-safe cache using a mutex for synchronization and an unordered map for storage. The key design is appropriate - using the Windows Hello
keyIdas the map key and storing the protected signature data as the value.
31-38: Memory protection implementation looks good.The
ProtectMemoryfunction properly usesCryptProtectMemorywith the correct protection level (CRYPTPROTECTMEMORY_SAME_PROCESS). The function includes appropriate error handling and empty data validation.
40-46: Memory unprotection implementation looks good.The
UnprotectMemoryfunction correctly implements the inverse operation usingCryptUnprotectMemorywith consistent protection level and error handling.
151-151: Function signature improvement.Good change to pass the
keyIdparameter by const reference, which avoids unnecessary copying of the string.
157-171: Thread-safe cache retrieval implementation.Well-structured implementation that:
- Uses a lock guard for thread safety
- Attempts to find the cached key
- Properly unprotects the memory if found
- Throws an exception with a clear error message if memory unprotection fails
This approach effectively reduces the number of Windows Hello prompts by reusing cached credentials.
173-206: Well-implemented cache population logic.This code segment appropriately:
- Falls back to Windows Hello credential retrieval when not found in cache
- Handles different status codes from Windows Hello API
- Creates a protected copy of sensitive data
- Uses a lock guard for thread-safe cache updates
- Cleans up sensitive data by zeroing the buffer after use
The approach aligns well with the PR objective of reducing Windows Hello prompts while maintaining security.
208-215: Good security practice for handling sensitive data.The code properly:
- Creates a signature buffer from the retrieved data
- Derives the encryption key using HKDF
- Clears the sensitive signature data by zeroing the buffer after use
This follows the principle of minimizing the lifetime of sensitive data in memory.
194-198: Memory protection exception handling.Good practice to throw an exception when memory protection fails. This is a critical security operation and failing silently would be inappropriate. The exception with a descriptive message helps with troubleshooting.
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/org/cryptomator/windows/keychain/WindowsHello.java (3)
9-14: Document security considerations for using a fixed challengeUsing a fixed challenge rather than a dynamic one is a deliberate design choice to enable caching Windows Hello authentication results. While this improves usability by reducing authentication prompts, it's worth documenting this security trade-off directly in the code.
Consider adding a comment explaining the purpose and security implications of using a fixed challenge:
private final byte[] fixedChallenge = new byte[] { + // Using a fixed challenge enables caching of Windows Hello authentication results, + // reducing the number of prompts. The actual encryption key is still protected + // in memory using CryptProtectMemory in the native implementation. '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', 'A', 'B', 'C', 'D', 'E', 'F' };
21-22: Document that the challenge parameter is ignoredThe method now uses the fixed challenge instead of the parameter, but the method signature remains unchanged for compatibility. This behavior should be documented to avoid confusion.
Consider adding a comment or updating the method signature:
@Override +// The challenge parameter is ignored and fixedChallenge is used instead to enable caching public byte[] encrypt(byte[] cleartext, byte[] challenge) { return Native.INSTANCE.setEncryptionKey(keyId, cleartext, fixedChallenge); }
26-27: Document that the challenge parameter is ignoredThe decrypt method also now uses the fixed challenge instead of the parameter. As with the encrypt method, this behavior should be documented.
Consider adding a comment similar to the one suggested for encrypt:
@Override +// The challenge parameter is ignored and fixedChallenge is used instead to enable caching public byte[] decrypt(byte[] ciphertext, byte[] challenge) { return Native.INSTANCE.getEncryptionKey(keyId, ciphertext, fixedChallenge); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/windows/keychain/WindowsHello.java(1 hunks)src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp
🧰 Additional context used
🧠 Learnings (1)
src/main/java/org/cryptomator/windows/keychain/WindowsHello.java (1)
Learnt from: infeo
PR: cryptomator/integrations-win#83
File: src/main/java/org/cryptomator/windows/keychain/WinHello.java:29-41
Timestamp: 2024-11-11T15:58:12.072Z
Learning: In `src/main/java/org/cryptomator/windows/keychain/WinHello.java`, the `Native` class is private and only accessible within its enclosing class. Therefore, making `INSTANCE` private and adding a getter method is unnecessary.
🧬 Code Graph Analysis (1)
src/main/java/org/cryptomator/windows/keychain/WindowsHello.java (2)
src/main/java/org/cryptomator/windows/common/WinStrings.java (1)
WinStrings(6-14)src/main/java/org/cryptomator/windows/keychain/WinDataProtection.java (1)
Native(23-33)
🔇 Additional comments (1)
src/main/java/org/cryptomator/windows/keychain/WindowsHello.java (1)
6-48: Implementation aligns well with PR objectives for secure cachingThe changes to use a fixed challenge instead of dynamic ones effectively support the goal of caching Windows Hello authentication results to reduce the number of prompts. This improves usability while maintaining security through secure memory protection.
For future maintenance, consider documenting the purpose of the implementation pattern and potentially refactoring the method signatures to remove unused parameters if backward compatibility becomes less of a concern.
* reduced it to only generate signature * caching done in caller method * return an IBuffer directly
* unify naming for vectors/buffers * skip array_view constructor * remove zeroing public info
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (1)
175-207: 💡 Verification agent🧩 Analysis chain
Verify signature data alignment before caching.
The caching implementation is well-designed with proper thread safety. However, there's a potential issue: Windows Hello signature data may not be aligned to 16 bytes, which is required by
ProtectMemory.Consider adding padding logic before calling
ProtectMemory:- auto copyToProtect = iBufferToVector(signature); + auto copyToProtect = iBufferToVector(signature); + // Ensure 16-byte alignment for CryptProtectMemory + const size_t block = CRYPTPROTECTMEMORY_BLOCK_SIZE; + size_t pad = (block - (copyToProtect.size() % block)) % block; + if (pad) { + copyToProtect.insert(copyToProtect.end(), pad, 0); + }
🏁 Script executed:
#!/bin/bash # Description: Check if Windows Hello signature length is documented or if there are any alignment guarantees # Expected: Find documentation about signature data format and alignment requirements # Search for any existing alignment handling in similar Windows Hello implementations rg -i "signature.*length|signature.*size|cryptprotectmemory.*align" -A 3 -B 3 # Look for any existing padding or alignment logic in the codebase ast-grep --pattern 'CryptProtectMemory($_, $_, $_)'Length of output: 937
Ensure 16-byte alignment before calling ProtectMemory
During verification, there’s no existing padding or alignment logic around
ProtectMemory(which ultimately callsCryptProtectMemoryand requires the buffer length to be a multiple ofCRYPTPROTECTMEMORY_BLOCK_SIZE); Windows Hello signatures are not guaranteed to meet this requirement. Without explicit padding, calls toProtectMemory(copyToProtect)may fail at runtime.Please update the cache-write path in
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp(insidegetOrCreateKey) to pad the vector to a 16-byte boundary before protection:auto copyToProtect = iBufferToVector(signature); + // Ensure buffer length is a multiple of CRYPTPROTECTMEMORY_BLOCK_SIZE (16) + constexpr size_t blockSize = CRYPTPROTECTMEMORY_BLOCK_SIZE; + size_t padding = (blockSize - (copyToProtect.size() % blockSize)) % blockSize; + if (padding) { + copyToProtect.insert(copyToProtect.end(), padding, 0); + } // cache try { ProtectMemory(copyToProtect);
- File: src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp
- Function: getOrCreateKey, cache-path before
ProtectMemorycall
🧹 Nitpick comments (1)
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (1)
322-325: Consider using specific exception types for user cancellation.The current approach uses
std::logic_errorfor user cancellation, which is somewhat hacky as noted in the comment. Consider defining a custom exception type for better clarity.+// Custom exception for user cancellation +class UserCancellationException : public std::exception { +public: + const char* what() const noexcept override { + return "User canceled Windows Hello operation"; + } +};Then update the
getSignaturefunction:- throw std::logic_error("User canceled operation"); + throw UserCancellationException();And the catch blocks:
- catch(const std::logic_error& le) { //tad' hacky: Logic error must only be thrown,if the user cancels the operation. + catch(const UserCancellationException& uce) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/headers/org_cryptomator_windows_keychain_WindowsHello_Native.h(1 hunks)src/main/java/org/cryptomator/windows/keychain/WindowsHello.java(2 hunks)src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/windows/keychain/WindowsHello.java
🧰 Additional context used
🧠 Learnings (1)
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (2)
Learnt from: purejava
PR: cryptomator/integrations-win#83
File: src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp:0-0
Timestamp: 2024-11-16T17:26:11.536Z
Learning: In `src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp`, additional error handling and input validation in the `deriveEncryptionKey` function may not be necessary unless specific issues are identified.
Learnt from: purejava
PR: cryptomator/integrations-win#83
File: src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp:0-0
Timestamp: 2024-11-23T06:39:47.630Z
Learning: In `src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp`, code duplication between the encryption and decryption methods is acceptable to maintain readability, especially in cryptographic code.
🔇 Additional comments (10)
src/main/headers/org_cryptomator_windows_keychain_WindowsHello_Native.h (2)
20-24: LGTM: Method rename improves clarity.The rename from
setEncryptionKeytoencryptbetter reflects the method's actual functionality and aligns with the updated native implementation.
28-32: LGTM: Method rename improves clarity.The rename from
getEncryptionKeytodecryptbetter reflects the method's actual functionality and aligns with the updated native implementation.src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (8)
9-11: LGTM: Appropriate includes for new functionality.The new includes support the caching mechanism (
unordered_map,mutex) and Windows cryptographic functions (wincrypt.h).
27-31: LGTM: Well-designed caching infrastructure.The global mutex and map provide thread-safe caching, and the constants are appropriately defined for the Windows Hello implementation.
33-53: LGTM: Proper implementation of memory protection helpers.The functions correctly implement the validation for
CRYPTPROTECTMEMORY_BLOCK_SIZEalignment as requested in previous reviews. The error handling and memory zeroing on failure are well implemented.
109-147: LGTM: Improved HKDF implementation with secure memory handling.The function now properly zeros sensitive intermediate buffers (
previousBlockandresult) and maintains the same cryptographic correctness while improving security.
151-172: LGTM: Clean signature generation with proper error handling.The function correctly handles Windows Hello credential creation/opening and provides appropriate error handling for both failure and user cancellation scenarios.
210-221: LGTM: Well-documented function with comprehensive documentation.The extensive documentation clearly explains the function's purpose, parameters, return values, and behavior.
268-336: LGTM: Robust encryption implementation with proper security measures.The function implements:
- Proper AES-CBC encryption with PKCS7 padding
- HMAC authentication over IV + ciphertext
- Consistent memory zeroing of sensitive data
- Comprehensive error handling including user cancellation
- Thread-safe key caching
The implementation follows cryptographic best practices.
359-437: LGTM: Secure decryption implementation with integrity verification.The function correctly:
- Verifies HMAC before decryption
- Implements proper AES-CBC decryption
- Handles all error cases appropriately
- Securely zeros computed HMAC data
- Maintains consistency with the encryption function
The implementation is cryptographically sound.
There was a problem hiding this comment.
@purejava I took the time to refactor the cpp code. Most important changes:
- move the fixed challenge to the cpp code as a constant
- use a salt in HKDF, which is now the additional data parameter (instead of the fixed challenge)
- zero memory with WinRT SecureZeroMemory method
- be more exception-like and less return-bool-value
- add docs
Let me know if i missed something/made a mistake in implementation
Thanks for your invest @infeo. I could not spot any flaws in your changes. A test with some vaults with unlocking them in an interchanging order worked as well. LGTM. |
I am glad to request this change to finalize the Windows Hello integration.
Based on an idea from @infeo
I implemented this as natively as possible. Knowing, that caching secure data is a tradeoff to security, this makes Windows Hello usable, as it reduces the prompts to a required minimum, e.g. on changing the password for a vault.
Basically, the Windows Hello prompt comes up once for every vault used.The sensitive information is keep in memory, but nevertheless protected byCryptProtectMemory.So, after all, this is a gain in security and fits well to the new Touch ID feature on Mac.
This PR depends on a small change in Cryptomator, that the Hello keychain path property needs to be added to the
Environment. I'll open a separate PR for this.Edit: depends on cryptomator/cryptomator#3808
Edit 2: The Windows Hello prompt does not come up for every vault used, this was changed by 6bda0f9.