Merge pull request #212 from dint-dev/fix/overwriting

Improve overwriting of bytes

Author: terrier989

cryptography/lib/src/cryptography/cipher.dart

@@ -19,6 +19,7 @@ import 'dart:typed_data';
 
 import 'package:cryptography/cryptography.dart';
 import 'package:cryptography/helpers.dart';
+import 'package:cryptography/src/helpers/erase_bytes.dart';
 import 'package:meta/meta.dart';
 
 import '../../dart.dart';
@@ -267,7 +268,7 @@ abstract class Cipher {
       return utf8.decode(clearText);
     } finally {
       // Don't leave possibly sensitive data in the heap.
-      clearText.fillRange(0, clearText.length, 0);
+      tryEraseBytes(clearText);
     }
   }
 
@@ -407,11 +408,7 @@ abstract class Cipher {
     );
 
     // Overwrite `bytes` if it was not overwritten by the cipher.
-    final cipherText = secretBox.cipherText;
-    if (cipherText is! Uint8List ||
-        !identical(bytes.buffer, cipherText.buffer)) {
-      bytes.fillRange(0, bytes.length, 0);
-    }
+    tryEraseBytes(bytes, unlessUsedIn: secretBox.cipherText);
 
     return secretBox;
   }

cryptography/lib/src/cryptography/cipher_wand.dart

@@ -15,6 +15,8 @@
 import 'dart:convert';
 import 'dart:typed_data';
 
+import 'package:cryptography/src/helpers/erase_bytes.dart';
+
 import '../../cryptography.dart';
 
 /// An opaque object that possesses some non-extractable secret key.
@@ -109,11 +111,9 @@ abstract class CipherWand extends Wand {
     try {
       return utf8.decode(clearText);
     } finally {
-      try {
-        // Cut the amount of possibly sensitive data in the heap.
-        // This should be a cheap operation relative to decryption.
-        clearText.fillRange(0, clearText.length, 0);
-      } catch (_) {}
+      // Cut the amount of possibly sensitive data in the heap.
+      // This should be a cheap operation relative to decryption.
+      tryEraseBytes(clearText);
     }
   }
 
@@ -185,11 +185,7 @@ abstract class CipherWand extends Wand {
 
     // Cut the amount of possibly sensitive data in the heap.
     // This should be a cheap operation relative to encryption.
-    final cipherText = secretBox.cipherText;
-    if (cipherText is! Uint8List ||
-        !identical(bytes.buffer, cipherText.buffer)) {
-      bytes.fillRange(0, bytes.length, 0);
-    }
+    tryEraseBytes(bytes, unlessUsedIn: secretBox.cipherText);
 
     return secretBox;
   }

cryptography/lib/src/cryptography/sensitive_bytes.dart

@@ -16,6 +16,7 @@ import 'dart:collection';
 import 'dart:typed_data';
 
 import 'package:cryptography/cryptography.dart';
+import 'package:cryptography/src/helpers/erase_bytes.dart';
 
 /// List of security-sensitive bytes that can be destroyed with [destroy].
 ///
@@ -74,15 +75,7 @@ class SensitiveBytes extends ListBase<int> {
     if (bytes != null) {
       _bytes = null;
       if (overwriteWhenDestroyed) {
-        try {
-          for (var i = 0; i < bytes.length; i++) {
-            bytes[i] = 0;
-          }
-        } on UnsupportedError {
-          // Ignore error
-        } on StateError {
-          // Ignore error
-        }
+        tryEraseBytes(bytes);
       }
     }
   }

cryptography/lib/src/dart/hmac.dart

@@ -16,6 +16,7 @@ import 'dart:typed_data';
 
 import 'package:cryptography/cryptography.dart';
 import 'package:cryptography/dart.dart';
+import 'package:cryptography/src/helpers/erase_bytes.dart';
 
 /// An implementation of [Hmac] in pure Dart.
 ///
@@ -153,7 +154,7 @@ class _DartHmacSink extends MacSink with DartMacSinkMixin {
     // (safer to not leave the data in memory)
     tmp.fillRange(0, tmp.length, 0);
     if (eraseKey) {
-      hmacKey.fillRange(0, hmacKey.length, 0);
+      tryEraseBytes(hmacKey);
     }
   }
 

cryptography/lib/src/helpers/erase_bytes.dart

@@ -0,0 +1,21 @@
+import 'dart:typed_data';
+
+final _typeOfModifiableUint8List = Uint8List.fromList(const []).runtimeType;
+
+void tryEraseBytes(List<int> bytes, {List<int>? unlessUsedIn}) {
+  if (unlessUsedIn != null) {
+    if (identical(bytes, unlessUsedIn) ||
+        bytes is Uint8List &&
+            unlessUsedIn is Uint8List &&
+            identical(bytes.buffer, unlessUsedIn.buffer)) {
+      return;
+    }
+  }
+  try {
+    if (identical(bytes.runtimeType, _typeOfModifiableUint8List)) {
+      bytes.fillRange(0, bytes.length, 0);
+    }
+  } catch (error) {
+    assert(false, '$error');
+  }
+}

cryptography/test/secret_key_test.dart

@@ -87,33 +87,32 @@ void main() {
       }
     });
 
-    test('SecretKeyData([...], overwriteWhenDestroyed: true)', () {
-      final inputs = [
-        [42, 43, 44],
-        Uint8List.fromList([42, 43, 44])
-      ];
-      for (var input in inputs) {
-        final a = SecretKeyData(input, overwriteWhenDestroyed: true);
-        final capturedBytes = a.bytes;
-        expect(capturedBytes, [42, 43, 44]);
-
-        a.destroy();
-        expect(input, [0, 0, 0]);
-        expect(() => a.bytes, throwsStateError);
-        expect(() => capturedBytes[0], throwsStateError);
-      }
+    test('SecretKeyData(Uint8List(...), overwriteWhenDestroyed: true)', () {
+      final bytes = Uint8List.fromList([42, 43, 44]);
+      final secretKey = SecretKeyData(bytes, overwriteWhenDestroyed: true);
+      expect(secretKey.bytes, [42, 43, 44]);
+
+      secretKey.destroy();
+      expect(bytes, [0, 0, 0]);
+      expect(() => secretKey.bytes, throwsStateError);
     });
 
-    test('SecretKeyData(const [...], overwriteWhenDestroyed: true)', () {
-      const data = [42, 43, 44];
-      final a = SecretKeyData(data, overwriteWhenDestroyed: true);
-      final capturedBytes = a.bytes;
-      expect(capturedBytes, [42, 43, 44]);
+    test('SecretKeyData([...], overwriteWhenDestroyed: true)', () {
+      final bytes = [42, 43, 44];
+      final secretKey = SecretKeyData(bytes, overwriteWhenDestroyed: true);
+      expect(secretKey.bytes, [42, 43, 44]);
+      secretKey.destroy();
+      expect(bytes, [42, 43, 44]);
+      expect(() => secretKey.bytes, throwsStateError);
+    });
 
-      a.destroy();
-      expect(data, [42, 43, 44]);
-      expect(() => a.bytes, throwsStateError);
-      expect(() => capturedBytes[0], throwsStateError);
+    test('SecretKeyData(Uint8List(...), overwriteWhenDestroyed: false)', () {
+      final bytes = Uint8List.fromList([42, 43, 44]);
+      final secretKey = SecretKeyData(bytes, overwriteWhenDestroyed: false);
+      expect(secretKey.bytes, [42, 43, 44]);
+      secretKey.destroy();
+      expect(bytes, [42, 43, 44]);
+      expect(() => secretKey.bytes, throwsStateError);
     });
 
     test('SecretKeyData.random()', () {