Conversation
| ) | ||
| })?; | ||
|
|
||
| k_intermediate.zeroize(); |
There was a problem hiding this comment.
This won't be called if we error out earlier, right? Consider putting k_intermediate into a Zeroizing wrapper so it zeroes on drop
There was a problem hiding this comment.
Also under the hood of execute_batch the pragma statement below which contains the key hex repr will be cloned into a CString https://github.com/worldcoin/walletkit/blob/sqlite-refactor/walletkit-core/src/storage/db/ffi.rs#L108-L108 which is not zeroed on drop
There was a problem hiding this comment.
Maybe https://github.com/worldcoin/walletkit/blob/sqlite-refactor/walletkit-core/src/storage/keys.rs#L61-L65 should return Zeroizing<[u8; 32]>?
|
|
||
| /// Opens an in-memory database (useful for tests). | ||
| #[allow(dead_code)] | ||
| pub fn open_in_memory() -> DbResult<Self> { |
There was a problem hiding this comment.
Test only? Can we move it into its own impl in #[cfg(test)]?
| @@ -8,7 +8,7 @@ unknown-registry = "deny" | |||
| [bans] | |||
| deny = [ | |||
| # openssl-sys is allowed to bundle it in Android apps which require it for sqlcipher | |||
There was a problem hiding this comment.
this comment seems to be outdated
| use std::path::PathBuf; | ||
| let path = PathBuf::from(format!( | ||
| "{}/walletkit-db-cipher-test-{}.sqlite", | ||
| std::env::temp_dir().display(), | ||
| uuid::Uuid::new_v4() | ||
| )); |
There was a problem hiding this comment.
Consider using https://crates.io/crates/tempfile
| k_intermediate: [u8; 32], | ||
| read_only: bool, | ||
| ) -> DbResult<Connection> { | ||
| let conn = Connection::open(path, read_only)?; |
There was a problem hiding this comment.
Consider adding PRAGMA SECURE_DELETE: https://www.sqlite.org/pragma.html#pragma_secure_delete
Doesn't seem like we'd make use of it now but could be relevant in the future if we ever start deleting data
| pub(super) struct RawStmt { | ||
| ptr: *mut c_void, | ||
| /// Kept to extract error messages via `sqlite3_errmsg`. | ||
| db: *mut c_void, |
There was a problem hiding this comment.
Consider changing this to &'db RawDb to ensure statements don't outlive the db
| SqlcipherError::CipherUnavailable => StorageError::CacheDb(err.to_string()), | ||
| } | ||
| /// Maps an owned database error into a cache storage error. | ||
| pub(super) fn map_db_err_owned(err: &DbError) -> StorageError { |
There was a problem hiding this comment.
Same as map_db_err? Consider removing this one?
| @@ -0,0 +1,33 @@ | |||
| //! Minimal safe `SQLite` wrapper backed by `sqlite3mc`. | |||
There was a problem hiding this comment.
This should be an internal crate and not a module IMO
|
|
||
| /// Convenience macro for building parameter lists. | ||
| /// | ||
| /// Usage: `params![1_i64, blob.as_slice(), "text"]` |
There was a problem hiding this comment.
I noticed that params! is often used with explicit variants, e.g.:
|
|
||
| impl From<&str> for Value { | ||
| fn from(v: &str) -> Self { | ||
| Self::Text(v.to_string()) |
There was a problem hiding this comment.
We allocate and copy string here only for it to be immediately taken as a reference and sent to sqlite copying it again
I think we should consider adding a Cow and track lifetimes to avoid copying/allocation
There was a problem hiding this comment.
Actually on second check this From is never utilizied. In fact it looks like Value::Text is never constructed.
There was a problem hiding this comment.
I guess let's leave it as is
| } | ||
|
|
||
| pub fn column_blob(&self, col: i32) -> Vec<u8> { | ||
| // Safety: blob pointer is valid until the next step/reset/finalize. |
There was a problem hiding this comment.
So this isn't actually enforced by the interface - so if we're being pedantic this means that column_blob should be unsafe.
I'm wondering if it's worth it to modify Statement::step to return some sort of exclusive guard. This would allow us to:
- Ensure that
column_xxxmethods are only called afterstepreturnsSQLITE_ROW - Enforce this safety constraint
| } | ||
|
|
||
| fn errmsg_from_ptr(db: *mut c_void) -> String { | ||
| // Safety: db is a valid sqlite3 handle (or null, which we check). |
There was a problem hiding this comment.
This is actually not true, because RawStmt can outlive RawDb at which point the db pointer will be invalid, related to #197 (comment)
Co-authored-by: Jakub Trąd <jakubtrad@gmail.com>
Co-authored-by: Jakub Trąd <jakubtrad@gmail.com>
Resolve conflicts keeping walletkit-db in place of rusqlite: - Cargo.toml: keep walletkit-db workspace member - walletkit-core/Cargo.toml: walletkit-db dep instead of rusqlite, restore rand as optional with dev-dep - vault/mod.rs: walletkit-db imports, rewrite fetch_credential_and_blinding_factor to use prepare/bind/step API Co-authored-by: Cursor <cursoragent@cursor.com>
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
@SocketSecurity ignore cargo/unicode-ident@1.0.22 |
| )?; | ||
| let vault = | ||
| VaultDb::new(&self.paths.vault_db_path(), keys.intermediate_key(), &guard)?; | ||
| VaultDb::new(&self.paths.vault_db_path(), *keys.intermediate_key(), &guard)?; |
There was a problem hiding this comment.
keys.intermediate_key() returns Zeroizing<[u8; 32]>`
VaultDb::new(...) takes in [u8; 32] and then passes that to cipher::open_encrypted which https://github.com/worldcoin/walletkit/blob/sqlite-refactor/walletkit-db/src/cipher.rs#L53-L53
immediately puts the [u8; 32] into a Zeroizing<[u8; 32]>
could we just pass a &Zeroizing<[u8; 32]> reference to VaultDb::new and then transitively to open_encrypted? Same for CacheDb::new.
* Harden sqlite unsafe boundaries and statement state API * Drop defensive sqlite open null-pointer success check
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| /// See [`Connection::prepare`]. | ||
| pub fn prepare(&self, sql: &str) -> DbResult<Statement<'_>> { | ||
| self.conn.prepare(sql) | ||
| } |
There was a problem hiding this comment.
Transaction missing query_row_optional forces code duplication
Low Severity
Transaction delegates execute_batch, execute, query_row, and prepare to Connection but omits query_row_optional. This forces get_cache_entry_tx in cache/util.rs to manually reimplement the prepare → bind → step → match pattern that Connection::query_row_optional already encapsulates. Adding a one-line delegation (self.conn.query_row_optional(sql, params, mapper)) to Transaction would eliminate ~30 lines of duplicated logic and reduce divergence risk.


rusqlite doesn't compile for wasm32-unknown-unknown, and SQLCipher requires OpenSSL which added build complexity. sqlite3mc solves both: it compiles to WASM via sqlite-wasm-rs, and its crypto is fully built-in.
sqlite3mc
sqlite3mc is a modified SQLite amalgamation that adds transparent page-level encryption. It ships as a single C file (sqlite3mc_amalgamation.c) that replaces the standard sqlite3.c -- no external libraries needed.
Encryption is handled at the pager layer. When a database is keyed with PRAGMA key, sqlite3mc derives a page encryption key from the provided material using PBKDF2-SHA256, then encrypts every database page on write and decrypts on read using ChaCha20-Poly1305 (the default cipher). Each page gets its own one-time key derived from the master key, the page number, and a 16-byte nonce, with a Poly1305 authentication tag appended for tamper detection. This is fully transparent to the SQL layer -- queries, transactions, and WAL journaling work identically to an unencrypted database.
sqlite3mc supports multiple cipher schemes (AES-128/256-CBC, SQLCipher-compatible AES-256, RC4, Ascon-128) selectable at runtime via PRAGMA cipher. We default to ChaCha20-Poly1305 via the CODEC_TYPE=CODEC_TYPE_CHACHA20 compile flag. All crypto primitives are compiled directly into the amalgamation.
Closes #184
Note
High Risk
Large refactor of encrypted on-disk storage and transaction/query code, plus new build-time SQLite FFI/crypto plumbing; regressions could impact data integrity, keying correctness, or runtime portability across targets.
Overview
Migrates credential storage (vault + cache) off
rusqlite/SQLCipher onto a new internalwalletkit-dbcrate that provides a minimal safe SQLite wrapper backed bysqlite3mc, enabling encrypted storage without OpenSSL and making the storage stack compile onwasm32.Storage open/key/config/integrity-check logic is centralized in
walletkit_db::cipher(WAL +synchronous=FULL+secure_delete=ON), and all vault/cache SQL execution paths are updated to the new statement/transaction APIs (including explicit transactional helpers for replay-guard idempotency).Adds WASM-specific behavior: UniFFI exports/derives are
cfg_attr-gated, and the cross-process storage lock becomes a native file lock with a WASM no-op implementation; intermediate keys are now passed asZeroizing<[u8;32]>references to reduce key material lifetime/copies.Written by Cursor Bugbot for commit 71bba79. This will update automatically on new commits. Configure here.