Extension Encryption #791
Conversation
b7248da to
df37b63
Compare
…extension-encryption/manager
…tion/manager Collaboration Extension Encryption
| dprintf("[SCHEDULER] scheduler_waitable_thread( 0x%08X ), calling extensionFindDecrypt"); | ||
| if (extensionFindDecrypt(entry->routine)) { | ||
| dprintf("[SCHEDULER] scheduler_waitable_thread ( 0x%08X ), decryption of the extension failed"); | ||
| //break; |
There was a problem hiding this comment.
Yeah we need to think what to do here, make sense to break i guess
| BOOL bInitialized; | ||
| LPVOID lpCryptoContext; | ||
| LPCSTR lpCryptoParams; | ||
| BOOL bNeedsRefresh; | ||
| DWORD (*initialize)(LPVOID* lpCryptoContext, LPVOID lpParams); | ||
| DWORD (*encrypt)(LPVOID lpDataIn, DWORD dwDataInSize, LPVOID lpDataOut, DWORD dwDataOutSize); | ||
| DWORD (*decrypt)(LPVOID lpDataIn, DWORD dwDataInSize, LPVOID lpDataOut, DWORD dwDataOutSize); | ||
| DWORD (*refresh)(LPVOID lpCryptoContext, LPVOID lpParams); |
There was a problem hiding this comment.
Make sense here to keep track of what type of Cryptographic Manager we have created, holding the ENUM should be enough
| ret = ReadProcessMemory(GetCurrentProcess(), (unsigned char*)ExtensionLoc + i, lpTempBufferRead, diff, &ByteCounter); | ||
| if (!ret || ByteCounter != diff) { | ||
| dprintf("[extension_encryption][extension_encryption_decrypt] ReadProcessMemory failed with error 0x%x", GetLastError()); | ||
| bError = TRUE; | ||
| break; | ||
| } | ||
| if (!g_ExtensionEncryptionManager->cryptoManager.decrypt(lpTempBufferRead, diff, lpTempBufferWrite, BUFFER_SIZE)) { | ||
| dprintf("[extension_encryption][extension_encryption_decrypt] CryptographicManager decrypt failed."); | ||
| ret = FALSE; | ||
| bError = TRUE; | ||
| break; | ||
| } | ||
| ret = WriteProcessMemory(GetCurrentProcess(), (unsigned char*)ExtensionLoc + i, lpTempBufferWrite, diff, &ByteCounter); | ||
| if (!ret || ByteCounter != diff) { | ||
| dprintf("[extension_encryption][extension_encryption_decrypt] WriteProcessMemory failed with error 0x%x", GetLastError()); | ||
| bError = TRUE; | ||
| break; | ||
| } |
There was a problem hiding this comment.
I am really curious about running this on a real extension, wondering if ReadProcessMemory and WriteProcessMemory works as expected. With the DEBUG Cryptographic Manager we should just write the same bytes, this seems like a good testing example
There was a problem hiding this comment.
I will test it as soon as I can and let you know. Thanks for your reviews!
…the scheduler and command_process_inline
…actor add function
|
Side note for future me: BEFORE LANDING
|
…tion/manager-2 Fix base address, refactor add function
|
There was a problem hiding this comment.
Pull request overview
Adds an “Extension Encryption Manager” to metsrv and wires it into extension load and dispatch paths so extensions flagged as encryptable can be tracked and (de)crypted around handler execution.
Changes:
- Add
extension_encryption.{c,h}implementing a global extension encryption manager (debug + RC4 managers) and routines to add/find/(de)crypt extensions. - Initialize the manager during server setup; register encryptable, reflectively-loaded extensions during
core_loadlib. - Attempt decryption before running extension handlers in the command dispatcher and scheduler; periodically encrypt unused extensions.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| c/meterpreter/workspace/metsrv/metsrv.vcxproj | Adds new extension encryption source/header to the build. |
| c/meterpreter/workspace/metsrv/metsrv.vcxproj.filters | Adds new files to Visual Studio filters. |
| c/meterpreter/source/metsrv/server_setup.c | Initializes the Extension Encryption Manager during server setup. |
| c/meterpreter/source/metsrv/remote_dispatch.c | Registers encryptable, reflectively-loaded extensions with the manager. |
| c/meterpreter/source/metsrv/base.c | Attempts extension decryption before handler execution; triggers “encrypt unused” after command handling. |
| c/meterpreter/source/metsrv/scheduler.c | Attempts extension decryption before scheduled routine execution. |
| c/meterpreter/source/metsrv/extension_encryption.h | Declares manager/status structs and public APIs. |
| c/meterpreter/source/metsrv/extension_encryption.c | Implements manager initialization, extension tracking, and (de)cryption routines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!extensionFindDecryptVal || extensionFindDecryptVal == EXTENSION_ENCRYPTION_EXTENSION_NOT_ENCRYPTABLE) { | ||
| dprintf("[COMMAND] Decryption successful for command %u", commandId); | ||
| } | ||
| else { | ||
| dprintf("[COMMAND] Decryption failed for command %u", commandId); | ||
| //break; | ||
| } | ||
| serverContinue = command->request.inline_handler(remote, packet, &result) && serverContinue; | ||
| dprintf("[DISPATCH] executed %u, continue %s", commandId, serverContinue ? "yes" : "no"); | ||
| } | ||
| else | ||
| { | ||
| dprintf("[DISPATCH] executing request handler %u", commandId); | ||
| dprintf("[DISPATCH] Calling extensionFindDecrypt for command %u", commandId); | ||
| extensionFindDecryptVal = extensionFindDecrypt(command->request.handler); | ||
| if (!extensionFindDecryptVal || extensionFindDecryptVal == EXTENSION_ENCRYPTION_EXTENSION_NOT_ENCRYPTABLE) { | ||
| dprintf("[COMMAND] Decryption successful for command %u", commandId); | ||
| } | ||
| else { | ||
| dprintf("[COMMAND] Decryption failed for command %u", commandId); | ||
| //break; | ||
| } | ||
| result = command->request.handler(remote, packet); |
There was a problem hiding this comment.
If extensionFindDecrypt() reports a decryption failure, the handler is still invoked. For handlers located inside an encrypted extension, this risks executing encrypted bytes (crash/undefined behavior). Consider short-circuiting the dispatch when decryption fails (except for "not encryptable" / "not found" cases), and avoid logging "Decryption successful" unless the return code indicates success.
| if (!extensionFindDecryptVal || extensionFindDecryptVal == EXTENSION_ENCRYPTION_EXTENSION_NOT_ENCRYPTABLE) { | |
| dprintf("[COMMAND] Decryption successful for command %u", commandId); | |
| } | |
| else { | |
| dprintf("[COMMAND] Decryption failed for command %u", commandId); | |
| //break; | |
| } | |
| serverContinue = command->request.inline_handler(remote, packet, &result) && serverContinue; | |
| dprintf("[DISPATCH] executed %u, continue %s", commandId, serverContinue ? "yes" : "no"); | |
| } | |
| else | |
| { | |
| dprintf("[DISPATCH] executing request handler %u", commandId); | |
| dprintf("[DISPATCH] Calling extensionFindDecrypt for command %u", commandId); | |
| extensionFindDecryptVal = extensionFindDecrypt(command->request.handler); | |
| if (!extensionFindDecryptVal || extensionFindDecryptVal == EXTENSION_ENCRYPTION_EXTENSION_NOT_ENCRYPTABLE) { | |
| dprintf("[COMMAND] Decryption successful for command %u", commandId); | |
| } | |
| else { | |
| dprintf("[COMMAND] Decryption failed for command %u", commandId); | |
| //break; | |
| } | |
| result = command->request.handler(remote, packet); | |
| if (!extensionFindDecryptVal) { | |
| dprintf("[COMMAND] Decryption successful for command %u", commandId); | |
| serverContinue = command->request.inline_handler(remote, packet, &result) && serverContinue; | |
| dprintf("[DISPATCH] executed %u, continue %s", commandId, serverContinue ? "yes" : "no"); | |
| } | |
| else if (extensionFindDecryptVal == EXTENSION_ENCRYPTION_EXTENSION_NOT_ENCRYPTABLE | |
| || extensionFindDecryptVal == EXTENSION_ENCRYPTION_EXTENSION_NOT_FOUND) { | |
| dprintf("[COMMAND] Decryption not required for command %u", commandId); | |
| serverContinue = command->request.inline_handler(remote, packet, &result) && serverContinue; | |
| dprintf("[DISPATCH] executed %u, continue %s", commandId, serverContinue ? "yes" : "no"); | |
| } | |
| else { | |
| dprintf("[COMMAND] Decryption failed for command %u with error %u", commandId, extensionFindDecryptVal); | |
| result = extensionFindDecryptVal; | |
| } | |
| } | |
| else | |
| { | |
| dprintf("[DISPATCH] executing request handler %u", commandId); | |
| dprintf("[DISPATCH] Calling extensionFindDecrypt for command %u", commandId); | |
| extensionFindDecryptVal = extensionFindDecrypt(command->request.handler); | |
| if (!extensionFindDecryptVal) { | |
| dprintf("[COMMAND] Decryption successful for command %u", commandId); | |
| result = command->request.handler(remote, packet); | |
| } | |
| else if (extensionFindDecryptVal == EXTENSION_ENCRYPTION_EXTENSION_NOT_ENCRYPTABLE | |
| || extensionFindDecryptVal == EXTENSION_ENCRYPTION_EXTENSION_NOT_FOUND) { | |
| dprintf("[COMMAND] Decryption not required for command %u", commandId); | |
| result = command->request.handler(remote, packet); | |
| } | |
| else { | |
| dprintf("[COMMAND] Decryption failed for command %u with error %u", commandId, extensionFindDecryptVal); | |
| result = extensionFindDecryptVal; | |
| } |
| manager->lpCryptoParams = (LPCSTR)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, KEY_SIZE_RC4); | ||
| if (manager->lpCryptoParams == NULL) { | ||
| dprintf("[cryptographic_manager_rc4] HeapAlloc failed."); | ||
| return FALSE; | ||
| } | ||
| srand((unsigned int)GetTickCount()); | ||
| for(int i = 0; i < KEY_SIZE_RC4; i++) { | ||
| ((char*)manager->lpCryptoParams)[i] = (char)(rand() % 256); | ||
| } |
There was a problem hiding this comment.
RC4 key generation falls back to srand(GetTickCount()) + rand(), which is predictable and not suitable for generating cryptographic keys. If RC4 mode is intended to provide real confidentiality (not just a debug PoC), use a CSPRNG (e.g., BCryptGenRandom / RtlGenRandom) to fill lpCryptoParams.
| extensionFindDecryptValue = extensionFindDecrypt(entry->routine); | ||
| if (extensionFindDecryptValue && extensionFindDecryptValue != EXTENSION_ENCRYPTION_EXTENSION_NOT_ENCRYPTABLE) { | ||
| dprintf("[SCHEDULER] scheduler_waitable_thread ( 0x%08X ), decryption of the extension failed"); | ||
| //break; |
There was a problem hiding this comment.
When extensionFindDecrypt() indicates a decryption failure, the code still calls entry->routine(...). If the routine lives in an encrypted .text section, this can lead to executing encrypted bytes (crash/undefined behavior). Only invoke the routine when decryption succeeded (or the handler isn't within an encryptable extension), and avoid logging "decrypted successfully" on failure.
| //break; | |
| break; |
There was a problem hiding this comment.
yeah, this'll be uncommented in following PRs.
| EnterCriticalSection(&g_ExtensionEncryptionManager->cs); | ||
| do { | ||
| dprintf("[extension_encryption][extension_encryption_add] Adding extension"); | ||
| if (g_ExtensionEncryptionManager->dwExtensionsCount >= MAX_EXTENSIONS) { | ||
| BREAK_WITH_ERROR("[extension_encryption][extension_encryption_add] Maximum number of extensions reached.", ERROR_NOT_ENOUGH_MEMORY); | ||
| } | ||
|
|
||
| if (hLib == NULL || hLib == INVALID_HANDLE_VALUE) { | ||
| BREAK_WITH_ERROR("[extension_encryption][extension_encryption_add] Invalid parameters.", ERROR_INVALID_PARAMETER); | ||
| } |
There was a problem hiding this comment.
extension_encryption_add() enters the manager critical section but only calls LeaveCriticalSection on the success path. Any BREAK_WITH_ERROR(...) will break out of the loop and return while still holding the lock, causing deadlocks in all future extension-encryption operations. Restructure so the critical section is always released (e.g., a single LeaveCriticalSection after the do { ... } while (0) block).
There was a problem hiding this comment.
this is a good catch!
| if (!bError) { | ||
| for (DWORD i = 0; i != ExtensionSize; i += diff) { | ||
| if ((ExtensionSize - i) < BUFFER_SIZE) { | ||
| diff = ExtensionSize - i; | ||
| } | ||
| ret = ReadProcessMemory(GetCurrentProcess(), (unsigned char*)ExtensionLoc + i, lpTempBufferRead, diff, &ByteCounter); | ||
| if (!ret || ByteCounter != diff) { | ||
| dprintf("[extension_encryption][extension_encryption_encrypt] ReadProcessMemory failed with error 0x%x", GetLastError()); | ||
| break; | ||
| } | ||
| if (!g_ExtensionEncryptionManager->cryptoManager.encrypt(lpTempBufferRead, diff, lpTempBufferWrite, BUFFER_SIZE)) { | ||
| dprintf("[extension_encryption][extension_encryption_encrypt] CryptographicManager encrypt failed."); | ||
| ret = FALSE; | ||
| break; | ||
| } | ||
| ret = WriteProcessMemory(GetCurrentProcess(), (unsigned char*)ExtensionLoc + i, lpTempBufferWrite, diff, &ByteCounter); | ||
| if (!ret || ByteCounter != diff) { | ||
| dprintf("[extension_encryption][extension_encryption_encrypt] WriteProcessMemory failed with error 0x%x", GetLastError()); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| lpExtensionStatus->dwLastUsedTime = GetTickCount(); | ||
| lpExtensionStatus->bEncrypted = TRUE; | ||
| } |
There was a problem hiding this comment.
extension_encryption_encrypt() sets lpExtensionStatus->bEncrypted = TRUE even if the loop breaks early due to Read/Write/Crypto failure (those paths don't set bError). This can mark a partially-encrypted extension as "encrypted" and break subsequent decrypt/execute logic. Only flip bEncrypted (and update dwLastUsedTime) after a verified full success, and treat partial failures as errors.
There was a problem hiding this comment.
this is a good catch as well!
| return EXTENSION_ENCRYPTION_EXTENSION_NOT_ENCRYPTABLE; | ||
| } | ||
|
|
||
| if (!extensionStatus->bEncrypted || !encryptionManager->decrypt(extensionStatus)) { |
There was a problem hiding this comment.
extensionFindDecrypt() currently returns EXTENSION_ENCRYPTION_DECRYPTION_ERROR when the extension is not encrypted because of if (!extensionStatus->bEncrypted || !decrypt(...)). This should be the opposite: if it's already decrypted, return success; only call decrypt() (and fail) when bEncrypted is true and decryption fails.
| if (!extensionStatus->bEncrypted || !encryptionManager->decrypt(extensionStatus)) { | |
| if (!extensionStatus->bEncrypted) { | |
| return ERROR_SUCCESS; | |
| } | |
| if (!encryptionManager->decrypt(extensionStatus)) { |
| } while (0); | ||
|
|
||
| } while (0); | ||
| extension_encryption_encrypt_unused(); |
There was a problem hiding this comment.
command_handle calls extension_encryption_encrypt_unused() unconditionally. If InitExtensionEncryptionManager() failed (or was never called), extension_encryption_encrypt_unused() dereferences g_ExtensionEncryptionManager and will crash. Guard this call (or make extension_encryption_encrypt_unused() a no-op when the manager is NULL).
| extension_encryption_encrypt_unused(); | |
| if (g_ExtensionEncryptionManager != NULL) | |
| { | |
| extension_encryption_encrypt_unused(); | |
| } |
There was a problem hiding this comment.
yeah good catch, we might not need the below suggestion if we do this. I'm not sure, I will take a look at it on VS.
| if(encryptionManager == NULL) { | ||
| dprintf("[SERVER] Extension Encryption Manager Initialization failed! proceeding without extension encryption."); |
There was a problem hiding this comment.
InitExtensionEncryptionManager() failure is logged as "proceeding without extension encryption", but other paths (e.g., command_handle -> extension_encryption_encrypt_unused) assume the manager exists and will dereference a NULL global. Either ensure init cannot fail here, or fully gate all extension-encryption calls on successful initialization.
| if(encryptionManager == NULL) { | |
| dprintf("[SERVER] Extension Encryption Manager Initialization failed! proceeding without extension encryption."); | |
| if (encryptionManager == NULL) | |
| { | |
| dprintf("[SERVER] Extension Encryption Manager initialization failed, aborting server setup."); | |
| break; |
There was a problem hiding this comment.
hmm, this is the tricky part. For now, we dont need it since every extension is not encryptable. But in the future, a similar mechanism can be implemented. What do you think @dledda-r7 ?
There was a problem hiding this comment.
we should probably cover this edge case now
There was a problem hiding this comment.
ok I will fix these on this weekend.
| DWORD cryptographic_manager_rc4_encrypt(LPVOID lpDataIn, DWORD dwDataInSize, LPVOID lpDataOut, DWORD dwDataOutSize) { | ||
| RC4_CTX* ctx = (RC4_CTX*)g_ExtensionEncryptionManager->cryptoManager.lpCryptoContext; | ||
| if (ctx == NULL) { | ||
| dprintf("[cryptographic_manager_rc4_encrypt] lpCryptoContext is NULL."); | ||
| return 0; | ||
| } | ||
| if (dwDataOutSize < dwDataInSize) { | ||
| dprintf("[cryptographic_manager_rc4_encrypt] dwDataOutSize is smaller than dwDataInSize."); | ||
| return 0; | ||
| } | ||
| memcpy(lpDataOut, lpDataIn, dwDataInSize); | ||
| if (!RC4Cipher(ctx, (unsigned char*)lpDataOut, dwDataInSize)) { | ||
| dprintf("[cryptographic_manager_rc4_encrypt] RC4Cipher failed."); | ||
| return 0; | ||
| } | ||
| return dwDataInSize; |
There was a problem hiding this comment.
RC4 encryption/decryption uses a single RC4_CTX that is mutated by RC4Cipher. Because cryptographic_manager_rc4_encrypt() doesn't refresh/reset the context before each encryption, the keystream position depends on previous operations, making later decryptions (which do refresh in extension_encryption_decrypt) incorrect. Refresh/reset the RC4 context before both encrypt and decrypt operations (or use a per-operation/per-extension context) so encrypt/decrypt are reversible.
There was a problem hiding this comment.
that's right, but I'm not sure about this. Imho we were calling the Initialiser before every cipher call, but I'm not sure.
|
|
||
| EnterCriticalSection(&g_ExtensionEncryptionManager->cs); | ||
| ExtensionEncryptionStatus** extension_statuses = g_ExtensionEncryptionManager->extensionStatuses; | ||
| DWORD current_time = GetTickCount(); | ||
| for (DWORD i = 0; i < g_ExtensionEncryptionManager->dwExtensionsCount; i++) { | ||
| ExtensionEncryptionStatus* status = extension_statuses[i]; | ||
| if (status != NULL && status->bEncryptable && !status->bEncrypted) { | ||
| if (current_time - status->dwLastUsedTime > ENCRYPTION_UNUSED_COOLDOWN_MS) { | ||
| g_ExtensionEncryptionManager->encrypt(status); | ||
| } | ||
| } | ||
| } | ||
| LeaveCriticalSection(&g_ExtensionEncryptionManager->cs); |
There was a problem hiding this comment.
extension_encryption_encrypt_unused() assumes g_ExtensionEncryptionManager is non-NULL and immediately enters its critical section. If the manager wasn't initialized (or init failed), this will crash. Add a NULL check at the start and return early when the manager is unavailable.
| EnterCriticalSection(&g_ExtensionEncryptionManager->cs); | |
| ExtensionEncryptionStatus** extension_statuses = g_ExtensionEncryptionManager->extensionStatuses; | |
| DWORD current_time = GetTickCount(); | |
| for (DWORD i = 0; i < g_ExtensionEncryptionManager->dwExtensionsCount; i++) { | |
| ExtensionEncryptionStatus* status = extension_statuses[i]; | |
| if (status != NULL && status->bEncryptable && !status->bEncrypted) { | |
| if (current_time - status->dwLastUsedTime > ENCRYPTION_UNUSED_COOLDOWN_MS) { | |
| g_ExtensionEncryptionManager->encrypt(status); | |
| } | |
| } | |
| } | |
| LeaveCriticalSection(&g_ExtensionEncryptionManager->cs); | |
| ExtensionEncryptionManager* encryptionManager = g_ExtensionEncryptionManager; | |
| if (encryptionManager == NULL) { | |
| return; | |
| } | |
| EnterCriticalSection(&encryptionManager->cs); | |
| ExtensionEncryptionStatus** extension_statuses = encryptionManager->extensionStatuses; | |
| DWORD current_time = GetTickCount(); | |
| for (DWORD i = 0; i < encryptionManager->dwExtensionsCount; i++) { | |
| ExtensionEncryptionStatus* status = extension_statuses[i]; | |
| if (status != NULL && status->bEncryptable && !status->bEncrypted) { | |
| if (current_time - status->dwLastUsedTime > ENCRYPTION_UNUSED_COOLDOWN_MS) { | |
| encryptionManager->encrypt(status); | |
| } | |
| } | |
| } | |
| LeaveCriticalSection(&encryptionManager->cs); |
There was a problem hiding this comment.
nice. Gonna check the remaining ones later. @dledda-r7
… in extension encryption functions
…in extension encryption, sync rc4 stream
…tion/manager-2 some other fixes
Extension Encryption: Encryption Manager
This PR creates the basic encryption manager, and start wiring decryption logic inside metsrv.
Coverage:
ENCRYPTABLEand if it was load by ReflectiveLoaderAt the moment the default CryptographicManager is the a "debug" manager that will just force metsrv to rewrite the same bytes to the extension memory space, this chained with a testing extension loaded with the
ENCRYPTABLEflag set to true will be used as first PoC to ensure writing the memory will not cause any issue for extension loaded with ReflectiveLoader.