change!: make hashing fallible to prepare for collision detection

This seems better than killing the process as Git does, since
we’re in a library context and it would be bad if you could
perform denial‐of‐service attacks on a server by sending it hash
collisions. (Although there are probably cheaper ways to mount a
denial‐of‐service attack.)

It does mean a lot of churn across the tree, but the underlying
breaking change is to a low‐level API, and the change is usually
just an adjustment to variants of an existing error type, so I expect
that most downstream users will require little to no adaption for
this change.

Author: emilazy

gix-commitgraph/src/file/verify.rs

@@ -45,6 +45,8 @@ pub mod checksum {
     #[derive(thiserror::Error, Debug)]
     #[allow(missing_docs)]
     pub enum Error {
+        #[error("failed to hash commit graph file")]
+        Hasher(#[from] gix_hash::hasher::Error),
         #[error(transparent)]
         Verify(#[from] gix_hash::verify::Error),
     }
@@ -153,7 +155,7 @@ impl File {
         let data_len_without_trailer = self.data.len() - self.hash_len;
         let mut hasher = gix_hash::hasher(self.object_hash());
         hasher.update(&self.data[..data_len_without_trailer]);
-        let actual = hasher.digest();
+        let actual = hasher.try_finalize()?;
         actual.verify(self.checksum())?;
         Ok(actual)
     }

gix-diff/tests/diff/blob/pipeline.rs

@@ -72,7 +72,7 @@ pub(crate) mod convert_to_diffable {
 
             let mut db = ObjectDb::default();
             let b_content = "b-content";
-            let id = db.insert(b_content);
+            let id = db.insert(b_content)?;
 
             let out = filter.convert_to_diffable(
                 &id,
@@ -129,7 +129,7 @@ pub(crate) mod convert_to_diffable {
         assert_eq!(buf.len(), 0, "it should avoid querying that data in the first place");
 
         let mut db = ObjectDb::default();
-        let id = db.insert(large_content);
+        let id = db.insert(large_content)?;
         let out = filter.convert_to_diffable(
             &id,
             EntryKind::Blob,
@@ -211,7 +211,7 @@ pub(crate) mod convert_to_diffable {
         drop(tmp);
 
         let mut db = ObjectDb::default();
-        let id = db.insert(large_content);
+        let id = db.insert(large_content)?;
 
         let out = filter.convert_to_diffable(
             &id,
@@ -397,7 +397,7 @@ pub(crate) mod convert_to_diffable {
 
         let mut db = ObjectDb::default();
         let b_content = "b-content\n";
-        let id = db.insert(b_content);
+        let id = db.insert(b_content)?;
 
         let out = filter.convert_to_diffable(
             &id,
@@ -416,7 +416,7 @@ pub(crate) mod convert_to_diffable {
 
         let mut db = ObjectDb::default();
         let b_content = "b\n";
-        let id = db.insert(b_content);
+        let id = db.insert(b_content)?;
         let out = filter.convert_to_diffable(
             &id,
             EntryKind::Blob,
@@ -490,7 +490,7 @@ pub(crate) mod convert_to_diffable {
 
         let mut db = ObjectDb::default();
         let b_content = "b-co\0ntent\n";
-        let id = db.insert(b_content);
+        let id = db.insert(b_content)?;
 
         let out = filter.convert_to_diffable(
             &id,
@@ -509,7 +509,7 @@ pub(crate) mod convert_to_diffable {
 
         let platform = attributes.at_entry("c", None, &gix_object::find::Never)?;
 
-        let id = db.insert("b");
+        let id = db.insert("b")?;
         let out = filter.convert_to_diffable(
             &id,
             EntryKind::Blob,
@@ -638,7 +638,7 @@ pub(crate) mod convert_to_diffable {
         assert_eq!(out.data, Some(pipeline::Data::Buffer { is_derived: false }));
         assert_eq!(buf.as_bstr(), "a\n", "unconditionally use git according to mode");
 
-        let id = db.insert("a\n");
+        let id = db.insert("a\n")?;
         for mode in worktree_modes {
             let out = filter.convert_to_diffable(
                 &id,
@@ -714,7 +714,7 @@ pub(crate) mod convert_to_diffable {
             assert_eq!(buf.len(), 0, "always cleared");
 
             buf.push(1);
-            let id = db.insert("link-target");
+            let id = db.insert("link-target")?;
             let out = filter.convert_to_diffable(
                 &id,
                 EntryKind::Link,
@@ -761,7 +761,7 @@ pub(crate) mod convert_to_diffable {
             assert_eq!(buf.len(), 0, "it's always cleared before any potential use");
         }
 
-        let id = db.insert("b\n");
+        let id = db.insert("b\n")?;
         for mode in all_modes {
             buf.push(1);
             let out = filter.convert_to_diffable(
@@ -814,7 +814,7 @@ pub(crate) mod convert_to_diffable {
             );
         }
 
-        let id = db.insert("c\n");
+        let id = db.insert("c\n")?;
         for mode in worktree_modes {
             let out = filter.convert_to_diffable(
                 &id,
@@ -863,7 +863,7 @@ pub(crate) mod convert_to_diffable {
             assert_eq!(buf.len(), 0);
         }
 
-        let id = db.insert("unset\n");
+        let id = db.insert("unset\n")?;
         for mode in all_modes {
             let out = filter.convert_to_diffable(
                 &id,
@@ -890,7 +890,7 @@ pub(crate) mod convert_to_diffable {
         }
 
         let platform = attributes.at_entry("d", None, &gix_object::find::Never)?;
-        let id = db.insert("d-in-db");
+        let id = db.insert("d-in-db")?;
         for mode in worktree_modes {
             let out = filter.convert_to_diffable(
                 &null,
@@ -959,7 +959,7 @@ pub(crate) mod convert_to_diffable {
             "no text filter, so git conversion was applied for worktree source"
         );
 
-        let id = db.insert("e-in-db");
+        let id = db.insert("e-in-db")?;
         let out = filter.convert_to_diffable(
             &id,
             EntryKind::Blob,

gix-diff/tests/diff/blob/platform.rs

@@ -30,7 +30,7 @@ fn resources_of_worktree_and_odb_and_check_link() -> crate::Result {
 
     let mut db = ObjectDb::default();
     let a_content = "a-content";
-    let id = db.insert(a_content);
+    let id = db.insert(a_content)?;
     platform.set_resource(
         id,
         EntryKind::BlobExecutable,
@@ -194,7 +194,7 @@ fn diff_binary() -> crate::Result {
 
     let mut db = ObjectDb::default();
     let a_content = "b";
-    let id = db.insert(a_content);
+    let id = db.insert(a_content)?;
     platform.set_resource(id, EntryKind::Blob, "b".into(), ResourceKind::NewOrDestination, &db)?;
 
     let out = platform.prepare_diff()?;
@@ -235,7 +235,7 @@ fn diff_performed_despite_external_command() -> crate::Result {
 
     let mut db = ObjectDb::default();
     let a_content = "b";
-    let id = db.insert(a_content);
+    let id = db.insert(a_content)?;
     platform.set_resource(id, EntryKind::Blob, "b".into(), ResourceKind::NewOrDestination, &db)?;
 
     let out = platform.prepare_diff()?;
@@ -277,7 +277,7 @@ fn diff_skipped_due_to_external_command_and_enabled_option() -> crate::Result {
 
     let mut db = ObjectDb::default();
     let a_content = "b";
-    let id = db.insert(a_content);
+    let id = db.insert(a_content)?;
     platform.set_resource(id, EntryKind::Blob, "b".into(), ResourceKind::NewOrDestination, &db)?;
 
     let out = platform.prepare_diff()?;

gix-diff/tests/diff/main.rs

@@ -51,10 +51,10 @@ mod util {
 
     impl ObjectDb {
         /// Insert `data` and return its hash. That can be used to find it again.
-        pub fn insert(&mut self, data: &str) -> gix_hash::ObjectId {
-            let id = gix_object::compute_hash(gix_hash::Kind::Sha1, gix_object::Kind::Blob, data.as_bytes());
+        pub fn insert(&mut self, data: &str) -> Result<gix_hash::ObjectId, Error> {
+            let id = gix_object::compute_hash(gix_hash::Kind::Sha1, gix_object::Kind::Blob, data.as_bytes())?;
             self.data_by_id.insert(id, data.into());
-            id
+            Ok(id)
         }
     }
 }

gix-diff/tests/diff/rewrites/tracker.rs

@@ -92,7 +92,7 @@ fn copy_by_similarity_reports_limit_if_encountered() -> crate::Result {
             (Change::addition(), "a-cpy-2", "a"),
             (Change::modification(), "d", "ab"),
         ],
-    );
+    )?;
 
     let mut calls = 0;
     let out = util::assert_emit_with_objects(
@@ -145,7 +145,7 @@ fn copy_by_id() -> crate::Result {
                 (Change::addition(), "a-cpy-2", "a"),
                 (Change::modification(), "d", "a"),
             ],
-        );
+        )?;
 
         let mut calls = 0;
         let out = util::assert_emit_with_objects(
@@ -218,7 +218,7 @@ fn copy_by_id_search_in_all_sources() -> crate::Result {
                 (Change::addition(), "a-cpy-1", "a"),
                 (Change::addition(), "a-cpy-2", "a"),
             ],
-        );
+        )?;
 
         let mut calls = 0;
         let content_id = hex_to_id("2e65efe2a145dda7ee51d1741299f848e5bf752e");
@@ -299,7 +299,7 @@ fn copy_by_50_percent_similarity() -> crate::Result {
             (Change::addition(), "a-cpy-2", "a\nc"),
             (Change::modification(), "d", "a"),
         ],
-    );
+    )?;
 
     let mut calls = 0;
     let out = util::assert_emit_with_objects(
@@ -377,7 +377,7 @@ fn copy_by_id_in_additions_only() -> crate::Result {
             (Change::modification(), "a", "a"),
             (Change::modification(), "a-cpy-1", "a"),
         ],
-    );
+    )?;
 
     let mut calls = 0;
     let out = util::assert_emit_with_objects(
@@ -429,7 +429,7 @@ fn rename_by_similarity_reports_limit_if_encountered() -> crate::Result {
             (Change::addition(), "b", "firt\nsecond\n"),
             (Change::addition(), "c", "second\nunrelated\n"),
         ],
-    );
+    )?;
 
     let mut calls = 0;
     let out = util::assert_emit_with_objects(
@@ -475,7 +475,7 @@ fn rename_by_50_percent_similarity() -> crate::Result {
             (Change::addition(), "b", "firt\nsecond\n"),
             (Change::addition(), "c", "second\nunrelated\n"),
         ],
-    );
+    )?;
 
     let mut calls = 0;
     let out = util::assert_emit_with_objects(
@@ -596,7 +596,7 @@ fn directory_renames_by_id_can_fail_gracefully() -> crate::Result {
             (Change::deletion(), "a", "first\nsecond\n"),
             (Change::addition(), "b", "firt\nsecond\n"),
         ],
-    );
+    )?;
 
     let mut calls = 0;
     let out = util::assert_emit_with_objects(
@@ -803,16 +803,16 @@ mod util {
     pub fn add_retained_blobs<'a>(
         tracker: &mut rewrites::Tracker<Change>,
         blobs: impl IntoIterator<Item = (Change, &'a str, &'a str)>,
-    ) -> ObjectDb {
+    ) -> crate::Result<ObjectDb> {
         let mut db = ObjectDb::default();
         for (mut change, location, data) in blobs {
-            change.id = db.insert(data);
+            change.id = db.insert(data)?;
             assert!(
                 tracker.try_push_change(change, location.into()).is_none(),
                 "input changes must be tracked"
             );
         }
-        db
+        Ok(db)
     }
 
     pub fn assert_emit(

gix-filter/src/ident.rs

@@ -48,6 +48,8 @@ pub mod apply {
     pub enum Error {
         #[error("Could not allocate buffer")]
         OutOfMemory(#[from] std::collections::TryReserveError),
+        #[error("Could not hash blob")]
+        Hasher(#[from] gix_hash::hasher::Error),
     }
 }
 
@@ -66,7 +68,7 @@ pub fn apply(src: &[u8], object_hash: gix_hash::Kind, buf: &mut Vec<u8>) -> Resu
     while let Some(pos) = src[ofs..].find(b"$Id$") {
         let id = match id {
             None => {
-                let new_id = gix_object::compute_hash(object_hash, gix_object::Kind::Blob, src);
+                let new_id = gix_object::compute_hash(object_hash, gix_object::Kind::Blob, src)?;
                 id = new_id.into();
                 clear_and_set_capacity(buf, src.len() + HASH_LEN)?; // pre-allocate for one ID
                 new_id

gix-hash/src/hasher/io.rs

@@ -6,6 +6,8 @@ use crate::{hasher, Hasher};
 pub enum Error {
     #[error(transparent)]
     Io(#[from] std::io::Error),
+    #[error("Failed to hash data")]
+    Hasher(#[from] hasher::Error),
 }
 
 /// Compute the hash of `kind` for the bytes in the file at `path`, hashing only the first `num_bytes_from_start`
@@ -74,7 +76,7 @@ pub fn bytes_with_hasher(
         }
     }
 
-    let id = hasher.digest();
+    let id = hasher.try_finalize()?;
     progress.show_throughput(start);
     Ok(id)
 }

gix-hash/src/hasher/mod.rs

@@ -1,3 +1,8 @@
+/// The error returned by [`Hasher::try_finalize()`].
+#[derive(Debug, thiserror::Error)]
+#[allow(missing_docs)]
+pub enum Error {}
+
 /// A implementation of the Sha1 hash, which can be used once.
 #[derive(Default, Clone)]
 pub struct Hasher(gix_features::hash::Hasher);
@@ -8,8 +13,8 @@ impl Hasher {
         self.0.update(bytes);
     }
     /// Finalize the hash and produce an object ID.
-    pub fn digest(self) -> crate::ObjectId {
-        self.0.digest().into()
+    pub fn try_finalize(self) -> Result<crate::ObjectId, Error> {
+        Ok(self.0.digest().into())
     }
 }
 

gix-hash/tests/object_id/mod.rs

@@ -47,20 +47,26 @@ mod sha1 {
 
     use gix_hash::{hasher, Kind, ObjectId};
 
-    fn hash_contents(s: &[u8]) -> ObjectId {
+    fn hash_contents(s: &[u8]) -> Result<ObjectId, hasher::Error> {
         let mut hasher = hasher(Kind::Sha1);
         hasher.update(s);
-        hasher.digest()
+        hasher.try_finalize()
     }
 
     #[test]
     fn empty_blob() {
-        assert_eq!(ObjectId::empty_blob(Kind::Sha1), hash_contents(b"blob 0\0"));
+        assert_eq!(
+            ObjectId::empty_blob(Kind::Sha1),
+            hash_contents(b"blob 0\0").expect("empty blob to not collide"),
+        );
     }
 
     #[test]
     fn empty_tree() {
-        assert_eq!(ObjectId::empty_tree(Kind::Sha1), hash_contents(b"tree 0\0"));
+        assert_eq!(
+            ObjectId::empty_tree(Kind::Sha1),
+            hash_contents(b"tree 0\0").expect("empty tree to not collide"),
+        );
     }
 
     /// Check the test vectors from RFC 3174.
@@ -83,7 +89,7 @@ mod sha1 {
         ];
         for (input, output) in fixtures {
             assert_eq!(
-                hash_contents(input),
+                hash_contents(input).expect("RFC inputs to not collide"),
                 ObjectId::from_str(&output.to_lowercase().replace(' ', "")).expect("RFC digests to be valid"),
             );
         }
@@ -103,7 +109,13 @@ mod sha1 {
         // BUG: These should be detected as a collision attack.
         let expected =
             ObjectId::from_str("8ac60ba76f1999a1ab70223f225aefdc78d4ddc0").expect("Shambles digest to be valid");
-        assert_eq!(hash_contents(message_a), expected);
-        assert_eq!(hash_contents(message_b), expected);
+        assert_eq!(
+            hash_contents(message_a).expect("collision attacks to not be detected"),
+            expected,
+        );
+        assert_eq!(
+            hash_contents(message_b).expect("collision attacks to not be detected"),
+            expected,
+        );
     }
 }