sqlite3mcSecureZeroMemory called with wrong argument count in mcMemoryFree; memset used instead of secure wipe in all Free*Cipher functions

[rajsin31415]

Bug 1: 3-argument call to 2-parameter function
In sqlite3mc_amalgamation.c line 335796:

sqlite3mcSecureZeroMemory(pPrior, 0, nSize);
sqlite3mcSecureZeroMemory takes (void*, size_t) — two parameters. The 0 is interpreted as the size, making this a zero-byte wipe (no-op). nSize is silently ignored. This means the entire SQLITE3MC_SECURE_MEMORY free path wipes nothing.

Fix: sqlite3mcSecureZeroMemory(pPrior, nSize);

Bug 2: Plain memset on key material before sqlite3_free
All Free*Cipher functions (FreeAES128Cipher, FreeAES256Cipher, FreeChaCha20Cipher, FreeSQLCipherCipher, FreeRC4Cipher, FreeAscon128Cipher, FreeAegisCipher) and sqlite3mcCodecTerm use memset() to clear key material (m_key, m_hmacKey, m_salt, Rijndael state) before freeing. Compilers are permitted to elide memset on dead stores (memory freed immediately after), so key material may persist in freed heap memory.

These should use sqlite3mcSecureZeroMemory instead.

Affected lines (approximate, in amalgamation): 329041, 329043, 329334, 329336, 329599, 330043, 330045, 330486, 331894, 332369, 332909, 332916.

[utelle]

Thanks for reporting the issue.

Bug 1: 3-argument call to 2-parameter function In sqlite3mc_amalgamation.c line 335796:

sqlite3mcSecureZeroMemory(pPrior, 0, nSize); sqlite3mcSecureZeroMemory takes (void*, size_t) — two parameters. The 0 is interpreted as the size, making this a zero-byte wipe (no-op). nSize is silently ignored. This means the entire SQLITE3MC_SECURE_MEMORY free path wipes nothing.

In theory, you are right. However, this code path is only used when the symbol SQLITE3MC_SECURE_MEMORY=1 is defined. By default it is not defined. And if it is defined then every good compiler will throw an error telling you something similar as Visual C++:

memory_secure.c(71,42): error C2197: 'void sqlite3mcSecureZeroMemory(void *,size_t)': too many arguments for call

That this bug went undetected for quite a long time shows that this feature isn't used frequently. And actually, IMHO this feature gives a false sense of security. The SQLite encryption feature is primarily meant for protecting SQLite databases at rest. If an attacker has access to the memory of a compromised system, they have far more ways to obtain critical information than simply inspecting freed heap space. For example, database pages are kept unencrypted in memory by SQLite itself - that's how the encryption feature was designed by SQLite.

Fix: sqlite3mcSecureZeroMemory(pPrior, nSize);

Of course, I will fix this for the next release.

Bug 2: Plain memset on key material before sqlite3_free All Free*Cipher functions (FreeAES128Cipher, FreeAES256Cipher, FreeChaCha20Cipher, FreeSQLCipherCipher, FreeRC4Cipher, FreeAscon128Cipher, FreeAegisCipher) and sqlite3mcCodecTerm use memset() to clear key material (m_key, m_hmacKey, m_salt, Rijndael state) before freeing. Compilers are permitted to elide memset on dead stores (memory freed immediately after), so key material may persist in freed heap memory.

These should use sqlite3mcSecureZeroMemory instead.

The code uses function sqlite3_free() to free memory, and if the symbol SQLITE3MC_SECURE_MEMORY=1 is defined then sqlite3_free() will use sqlite3mcSecureZeroMemory internally. So, calling memset() in the Free*Cipher methods is somewhat redundant. However, I will consider to use sqlite3mcSecureZeroMemory() independent of SQLITE3MC_SECURE_MEMORY feature.