From 4066ff183234a03d19d1c23330776db700834e53 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 31 Jul 2026 11:03:27 -0500 Subject: [PATCH] fix(store): fail closed on decrypt failure in decrypt_text_at_rest decrypt_text_at_rest previously swallowed any hex-decode, AEAD-decrypt, or UTF-8 failure with a fallback to an empty string, so a corrupted or tampered body column read back as an empty body instead of surfacing an error -- defeating the AEAD's integrity guarantee exactly where it matters: detecting tampering on read. Change its signature to return Result and propagate failures through every read call site in db.rs as the new DatabaseError::Decryption variant. Refs #237 #162 --- crates/nuncio-store/src/cipher.rs | 68 ++++++++++--- crates/nuncio-store/src/db.rs | 162 +++++++++++++++++++++++------- 2 files changed, 179 insertions(+), 51 deletions(-) diff --git a/crates/nuncio-store/src/cipher.rs b/crates/nuncio-store/src/cipher.rs index fe5a508..7a1943d 100644 --- a/crates/nuncio-store/src/cipher.rs +++ b/crates/nuncio-store/src/cipher.rs @@ -74,18 +74,20 @@ impl PayloadCipher { /// Decrypt text payload from database column storage at rest using a caller-supplied /// AES-256-GCM key. See [`PayloadCipher::encrypt_text_at_rest`]. - pub fn decrypt_text_at_rest(key: &[u8; 32], stored: &str) -> String { + /// + /// Fails closed: a hex-decode error, an AEAD authentication failure (tampered or + /// corrupted ciphertext), or a non-UTF-8 decrypted payload is returned as an error + /// rather than silently read back as an empty body -- an empty stored ciphertext is + /// the only case that legitimately yields `Ok(String::new())`. + pub fn decrypt_text_at_rest(key: &[u8; 32], stored: &str) -> Result { if stored.is_empty() { - return String::new(); + return Ok(String::new()); } - if let Ok(bytes) = hex::decode(stored) { - if let Ok(decrypted) = Self::decrypt_bytes(key, &bytes) { - if let Ok(s) = String::from_utf8(decrypted) { - return s; - } - } - } - String::new() + let bytes = hex::decode(stored) + .map_err(|e| CipherError::DecryptionFailed(format!("invalid hex encoding: {e}")))?; + let decrypted = Self::decrypt_bytes(key, &bytes)?; + String::from_utf8(decrypted) + .map_err(|e| CipherError::DecryptionFailed(format!("invalid utf-8 payload: {e}"))) } /// Encrypt binary attachment payload using `age` passphrase encryption. @@ -240,28 +242,62 @@ mod tests { PayloadCipher::encrypt_text_at_rest(&key, text).expect("encryption succeeds"); assert_ne!(text, encrypted); - let decrypted = PayloadCipher::decrypt_text_at_rest(&key, &encrypted); + let decrypted = + PayloadCipher::decrypt_text_at_rest(&key, &encrypted).expect("decryption succeeds"); assert_eq!(text, decrypted); assert_eq!( PayloadCipher::encrypt_text_at_rest(&key, "").expect("empty text is not encrypted"), "" ); - assert_eq!(PayloadCipher::decrypt_text_at_rest(&key, ""), ""); + assert_eq!( + PayloadCipher::decrypt_text_at_rest(&key, "").expect("empty ciphertext decrypts"), + "" + ); } #[test] - fn text_at_rest_wrong_key_fails_to_decrypt() { + fn text_at_rest_wrong_key_fails_closed() { // Proves there is no compiled-in default key: ciphertext produced under one - // caller-supplied key cannot be recovered using a different key. + // caller-supplied key cannot be recovered using a different key, and the failure + // is surfaced as an error rather than a silently-empty body. let key_a = [1u8; 32]; let key_b = [2u8; 32]; let text = "Confidential ledger payload"; let encrypted = PayloadCipher::encrypt_text_at_rest(&key_a, text).expect("encryption succeeds"); - let decrypted_with_wrong_key = PayloadCipher::decrypt_text_at_rest(&key_b, &encrypted); + let err = PayloadCipher::decrypt_text_at_rest(&key_b, &encrypted) + .expect_err("wrong key must fail closed"); - assert_eq!(decrypted_with_wrong_key, ""); + assert!(matches!(err, CipherError::DecryptionFailed(_))); + } + + #[test] + fn text_at_rest_tampered_ciphertext_fails_closed() { + // A corrupted-but-valid-hex ciphertext column must surface as an error, not read + // back as an empty body -- this is the read-path twin of the AEAD integrity + // guarantee: tampering must be detectable, not silently masked. + let key = [9u8; 32]; + let text = "Do not silently drop this on tamper"; + let encrypted = + PayloadCipher::encrypt_text_at_rest(&key, text).expect("encryption succeeds"); + + let mut bytes = hex::decode(&encrypted).expect("valid hex"); + let last_idx = bytes.len() - 1; + bytes[last_idx] ^= 0xFF; + let tampered = hex::encode(bytes); + + let err = PayloadCipher::decrypt_text_at_rest(&key, &tampered) + .expect_err("tampered ciphertext must fail closed"); + assert!(matches!(err, CipherError::DecryptionFailed(_))); + } + + #[test] + fn text_at_rest_invalid_hex_fails_closed() { + let key = [3u8; 32]; + let err = PayloadCipher::decrypt_text_at_rest(&key, "not-valid-hex") + .expect_err("invalid hex must fail closed"); + assert!(matches!(err, CipherError::DecryptionFailed(_))); } } diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index 28f66e0..821adb9 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -43,6 +43,13 @@ pub enum DatabaseError { /// genuinely empty body on read-back. #[error("payload encryption failed: {0}")] Encryption(String), + /// A body payload failed AEAD decryption (or UTF-8/hex decoding) on read. + /// This is a fail-closed error: it is returned instead of ever reading back a + /// corrupted or tampered ciphertext column as a silently-empty body, which would + /// defeat the AEAD's integrity guarantee exactly where it matters -- detecting + /// tampering on read. + #[error("payload decryption failed: {0}")] + Decryption(String), } impl From for DatabaseError { @@ -811,6 +818,8 @@ impl DatabaseEngine { for (id, subject, sender, body_plain) in rows { let dec_plain = body_plain .map(|p| crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &p)) + .transpose() + .map_err(|e| DatabaseError::Decryption(e.to_string()))? .unwrap_or_default(); sqlx::query( @@ -1112,8 +1121,7 @@ impl DatabaseEngine { .await .map_err(DatabaseError::Query)?; - Ok(rows - .into_iter() + rows.into_iter() .map( |( id, @@ -1127,13 +1135,25 @@ impl DatabaseEngine { body_plain, body_html, )| { - let dec_plain = body_plain.map(|p| { - crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &p) - }); - let dec_html = body_html.map(|h| { - crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &h) - }); - nuncio_core::model::Email { + let dec_plain = body_plain + .map(|p| { + crate::cipher::PayloadCipher::decrypt_text_at_rest( + &self.storage_key, + &p, + ) + }) + .transpose() + .map_err(|e| DatabaseError::Decryption(e.to_string()))?; + let dec_html = body_html + .map(|h| { + crate::cipher::PayloadCipher::decrypt_text_at_rest( + &self.storage_key, + &h, + ) + }) + .transpose() + .map_err(|e| DatabaseError::Decryption(e.to_string()))?; + Ok(nuncio_core::model::Email { id, account_id, folder_id, @@ -1145,10 +1165,10 @@ impl DatabaseEngine { body_plain: dec_plain, body_html: dec_html, attachments: Vec::new(), - } + }) }, ) - .collect()) + .collect::, DatabaseError>>() } /// Retrieve a single message by ID. @@ -1182,10 +1202,14 @@ impl DatabaseEngine { let dec_plain = row .8 - .map(|p| crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &p)); + .map(|p| crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &p)) + .transpose() + .map_err(|e| DatabaseError::Decryption(e.to_string()))?; let dec_html = row .9 - .map(|h| crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &h)); + .map(|h| crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &h)) + .transpose() + .map_err(|e| DatabaseError::Decryption(e.to_string()))?; Ok(nuncio_core::model::Email { id: row.0, @@ -1839,16 +1863,21 @@ impl DatabaseEngine { .await .map_err(DatabaseError::Query)?; - Ok(rows - .into_iter() + rows.into_iter() .map(|r| { - let dec_plain = r.8.map(|p| { - crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &p) - }); - let dec_html = r.9.map(|h| { - crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &h) - }); - nuncio_core::model::Email { + let dec_plain = + r.8.map(|p| { + crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &p) + }) + .transpose() + .map_err(|e| DatabaseError::Decryption(e.to_string()))?; + let dec_html = + r.9.map(|h| { + crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &h) + }) + .transpose() + .map_err(|e| DatabaseError::Decryption(e.to_string()))?; + Ok(nuncio_core::model::Email { id: r.0, account_id: r.1, folder_id: r.2, @@ -1860,9 +1889,9 @@ impl DatabaseEngine { body_plain: dec_plain, body_html: dec_html, attachments: Vec::new(), - } + }) }) - .collect()) + .collect::, DatabaseError>>() } /// Query messages across the WHOLE store for export purposes, @@ -1914,16 +1943,21 @@ impl DatabaseEngine { .await .map_err(DatabaseError::Query)?; - Ok(rows - .into_iter() + rows.into_iter() .map(|r| { - let dec_plain = r.8.map(|p| { - crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &p) - }); - let dec_html = r.9.map(|h| { - crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &h) - }); - nuncio_core::model::Email { + let dec_plain = + r.8.map(|p| { + crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &p) + }) + .transpose() + .map_err(|e| DatabaseError::Decryption(e.to_string()))?; + let dec_html = + r.9.map(|h| { + crate::cipher::PayloadCipher::decrypt_text_at_rest(&self.storage_key, &h) + }) + .transpose() + .map_err(|e| DatabaseError::Decryption(e.to_string()))?; + Ok(nuncio_core::model::Email { id: r.0, account_id: r.1, folder_id: r.2, @@ -1935,9 +1969,9 @@ impl DatabaseEngine { body_plain: dec_plain, body_html: dec_html, attachments: Vec::new(), - } + }) }) - .collect()) + .collect::, DatabaseError>>() } /// Append a new immutable WORM audit record to the log ledger, signed with the WORM @@ -2402,6 +2436,64 @@ mod tests { assert_eq!(folders[0].unread_messages, 1); } + /// A `body_plain` column corrupted after a legitimate write (simulating tampering or + /// bit-rot) must make `get_message` and `list_messages` return + /// `Err(DatabaseError::Decryption)`, never a silently-empty body -- that would defeat + /// the AEAD's integrity guarantee exactly where it matters: detecting tampering on read. + #[tokio::test] + async fn corrupted_body_ciphertext_fails_closed_on_read() { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + let email = nuncio_core::model::Email { + id: "msg-db-corrupt".to_string(), + account_id: "acct-1".to_string(), + folder_id: "INBOX".to_string(), + subject: "Corrupted At Rest".to_string(), + sender: "alice@nuncio.mx".to_string(), + recipient: "bob@nuncio.mx".to_string(), + received_at: 1700000000, + read: false, + body_plain: Some("Sensitive plaintext body".to_string()), + body_html: None, + attachments: Vec::new(), + }; + engine + .save_email(&email) + .await + .expect("save email succeeds"); + + // Flip a byte in the stored (still valid-hex) ciphertext to simulate tampering or + // storage bit-rot. + let (stored,): (String,) = sqlx::query_as("SELECT body_plain FROM messages WHERE id = ?") + .bind("msg-db-corrupt") + .fetch_one(&engine.pool) + .await + .expect("fetch stored ciphertext"); + let mut bytes = hex::decode(&stored).expect("stored ciphertext is valid hex"); + let last_idx = bytes.len() - 1; + bytes[last_idx] ^= 0xFF; + let tampered = hex::encode(bytes); + + sqlx::query("UPDATE messages SET body_plain = ? WHERE id = ?") + .bind(&tampered) + .bind("msg-db-corrupt") + .execute(&engine.pool) + .await + .expect("corrupt stored ciphertext"); + + let err = engine + .get_message("msg-db-corrupt") + .await + .expect_err("get_message must fail closed on corrupted ciphertext"); + assert!(matches!(err, DatabaseError::Decryption(_))); + + let err = engine + .list_messages("INBOX", 10) + .await + .expect_err("list_messages must fail closed on corrupted ciphertext"); + assert!(matches!(err, DatabaseError::Decryption(_))); + } + fn sample_calendar_event(id: &str, account_id: &str) -> nuncio_core::model::CalendarEvent { nuncio_core::model::CalendarEvent { id: id.to_string(), -- 2.54.0