Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

encryption: encryption may corrupt data when partial write #15080

Closed
YuJuncen opened this issue Jul 6, 2023 · 1 comment · Fixed by #15092
Closed

encryption: encryption may corrupt data when partial write #15080

YuJuncen opened this issue Jul 6, 2023 · 1 comment · Fixed by #15092

Comments

@YuJuncen
Copy link
Contributor

YuJuncen commented Jul 6, 2023

Bug Report

What version of TiKV are you using?

Current Master.

What operating system and CPU are you using?

Linux, x86_64.

Steps to reproduce

Apply the following patch (based on a5f1a26d3bb5121a845f644b5eebff3d6c041278):

Patch Content
+++ b/components/encryption/src/io.rs
@@ -525,6 +525,18 @@ mod tests {
         key
     }
 
+    struct Throttle<W>(W);
+
+    impl<W: Write> Write for Throttle<W> {
+        fn write(&mut self, buf: &[u8]) -> IoResult<usize> {
+            self.0.write(&buf[..2.min(buf.len())])
+        }
+
+        fn flush(&mut self) -> IoResult<()> {
+            Ok(())
+        }
+    }
+
     #[test]
     fn test_decrypt_encrypted_text() {
         let methods = [
@@ -557,10 +569,10 @@ mod tests {
                 let mut plaintext = vec![0; 1024];
                 OsRng.fill_bytes(&mut plaintext);
                 let buf = Vec::with_capacity(1024);
-                let mut encrypter = EncrypterWriter::new(buf, method, &key, iv).unwrap();
+                let mut encrypter = EncrypterWriter::new(Throttle(buf), method, &key, iv).unwrap();
                 encrypter.write_all(&plaintext).unwrap();
 
-                let buf = encrypter.finalize().unwrap();
+                let buf = encrypter.finalize().unwrap().0;
                 // Make sure it's properly encrypted.
                 if method != EncryptionMethod::Plaintext {
                     assert_ne!(buf, plaintext);

Then, run the test case test_decrypt_encrypted_text.

What did you expect?

The case should pass.

What did happened?

---- io::tests::test_decrypt_encrypted_text stdout ----
thread 'io::tests::test_decrypt_encrypted_text' panicked at 'assertion failed: `(left == right)`
  left: `[122, 11, 30, 119, 4]`,
 right: `[122, 11, 66, 84, 249]`', components/encryption/src/io.rs:589:21
@YuJuncen
Copy link
Contributor Author

YuJuncen commented Jul 6, 2023

This is probably because some of the block encryption method(includes AES256, if I have rightly understood it.) requires the integrity of the encrypted block. But the current Write implementation doesn't make sure the integrity of data written.

fn write(&mut self, buf: &[u8]) -> IoResult<usize> {
if let Some(crypter) = self.crypter.as_mut() {
let crypted = crypter.do_crypter(buf)?;
debug_assert!(crypted.len() == buf.len());
self.writer.write(crypted)
} else {

When L337 writes partial of the content, the encrypted block will be partial. The counter of CTR mode may contribute to this problem too. (What if some blocks are skipped? they will be lost in the sequence generated by the counter.)

ti-chi-bot bot pushed a commit that referenced this issue Jul 25, 2023
close #15080

Fix offset inconsistency between crypter and file that could cause data corruption when file I/O is interrupted.

Signed-off-by: tabokie <xy.tao@outlook.com>

Co-authored-by: tonyxuqqi <tonyxuqi@outlook.com>
ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this issue Jul 25, 2023
close tikv#15080

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot bot added a commit that referenced this issue Aug 2, 2023
#15206)

close #15080

Fix offset inconsistency between crypter and file that could cause data corruption when file I/O is interrupted.

Signed-off-by: tabokie <xy.tao@outlook.com>

Co-authored-by: tabokie <xy.tao@outlook.com>
Co-authored-by: tonyxuqqi <tonyxuqi@outlook.com>
Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
ti-chi-bot bot pushed a commit that referenced this issue Aug 15, 2023
#15205)

close #15080

Fix offset inconsistency between crypter and file that could cause data corruption when file I/O is interrupted.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
Signed-off-by: Xinye Tao <xy.tao@outlook.com>

Co-authored-by: Xinye Tao <xy.tao@outlook.com>
ti-chi-bot bot pushed a commit that referenced this issue Aug 15, 2023
#15207)

close #15080

Fix offset inconsistency between crypter and file that could cause data corruption when file I/O is interrupted.

Signed-off-by: tabokie <xy.tao@outlook.com>

Co-authored-by: tabokie <xy.tao@outlook.com>
Co-authored-by: tonyxuqqi <tonyxuqi@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants