object_store: Verify object id matches contents on retrieval (#2233)

Fixes #2223

Author: jelmer

NEWS

@@ -1,5 +1,8 @@
 1.2.7	UNRELEASED
 
+ * Verify that an object retrieved by id actually hashes to the requested
+   id, raising ``ChecksumMismatch`` otherwise. (Jelmer Vernooij, #2223)
+
  * Reject pack names containing path separators in the dumb HTTP transport,
    so a malicious server can no longer escape the temporary directory.
    (Jelmer Vernooij, #2213)

contrib/test_release_robot.py

@@ -89,11 +89,12 @@ class GetRecentTagsTest(unittest.TestCase):
     test_repo = os.path.join(BASEDIR, "dulwich_test_repo.zip")
     committer = b"Mark Mikofski <mark.mikofski@sunpowercorp.com>"
     test_tags: ClassVar[list[bytes]] = [b"v0.1a", b"v0.1"]
-    tag_test_data: ClassVar[
-        dict[bytes, tuple[int, bytes, tuple[int, bytes] | None]]
-    ] = {
-        test_tags[0]: (1484788003, b"3" * 40, None),
-        test_tags[1]: (1484788314, b"1" * 40, (1484788401, b"2" * 40)),
+    # Commit times and tag times per tag. Object ids are filled in by
+    # setUpClass once the objects have been created, since the contents (and
+    # thus the ids) are computed from the commit/tag data.
+    tag_test_data: ClassVar[dict[bytes, list]] = {
+        test_tags[0]: [1484788003, None, None],
+        test_tags[1]: [1484788314, None, [1484788401, None]],
     }
 
     # Class attributes set in setUpClass
@@ -111,35 +112,35 @@ def setUpClass(cls) -> None:
         obj_store = cls.repo.object_store  # test repo object store
         # commit 1 ('2017-01-19T01:06:43')
         cls.c1 = make_commit(
-            id=cls.tag_test_data[cls.test_tags[0]][1],
             commit_time=cls.tag_test_data[cls.test_tags[0]][0],
             message=b"unannotated tag",
             author=cls.committer,
         )
         obj_store.add_object(cls.c1)
+        cls.tag_test_data[cls.test_tags[0]][1] = cls.c1.id
         # tag 1: unannotated
         cls.t1 = cls.test_tags[0]
         cls.repo[b"refs/tags/" + cls.t1] = cls.c1.id  # add unannotated tag
         # commit 2 ('2017-01-19T01:11:54')
         cls.c2 = make_commit(
-            id=cls.tag_test_data[cls.test_tags[1]][1],
             commit_time=cls.tag_test_data[cls.test_tags[1]][0],
             message=b"annotated tag",
             parents=[cls.c1.id],
             author=cls.committer,
         )
         obj_store.add_object(cls.c2)
+        cls.tag_test_data[cls.test_tags[1]][1] = cls.c2.id
         # tag 2: annotated ('2017-01-19T01:13:21')
         tag_data = cls.tag_test_data[cls.test_tags[1]][2]
         if tag_data is None:
             raise AssertionError("test_tags[1] should have annotated tag data")
         cls.t2 = make_tag(
             cls.c2,
-            id=tag_data[1],
             name=cls.test_tags[1],
             tag_time=tag_data[0],
         )
         obj_store.add_object(cls.t2)
+        tag_data[1] = cls.t2.id
         cls.repo[b"refs/heads/master"] = cls.c2.id
         cls.repo[b"refs/tags/" + cls.t2.name] = cls.t2.id  # add annotated tag
 

dulwich/object_store.py

@@ -461,10 +461,19 @@ def get_raw(self, name: RawObjectID | ObjectID) -> tuple[int, bytes]:
         raise NotImplementedError(self.get_raw)
 
     def __getitem__(self, sha1: ObjectID | RawObjectID) -> ShaFile:
-        """Obtain an object by SHA1."""
+        """Obtain an object by SHA1.
+
+        Raises:
+          ChecksumMismatch: if the stored contents do not hash to the
+            requested object id.
+        """
+        if len(sha1) == self.object_format.oid_length:
+            hexsha = sha_to_hex(RawObjectID(sha1))
+        else:
+            hexsha = ObjectID(sha1)
         type_num, uncomp = self.get_raw(sha1)
         return ShaFile.from_raw_string(
-            type_num, uncomp, sha=sha1, object_format=self.object_format
+            type_num, uncomp, verify_sha=hexsha, object_format=self.object_format
         )
 
     def __iter__(self) -> Iterator[ObjectID]:

dulwich/objects.py

@@ -557,21 +557,38 @@ def as_pretty_string(self) -> str:
         return self.as_raw_string().decode("utf-8", "replace")
 
     def set_raw_string(
-        self, text: bytes, sha: ObjectID | RawObjectID | None = None
+        self,
+        text: bytes,
+        sha: ObjectID | RawObjectID | None = None,
+        *,
+        verify_sha: ObjectID | RawObjectID | None = None,
     ) -> None:
         """Set the contents of this object from a serialized string."""
         if not isinstance(text, bytes):
             raise TypeError(f"Expected bytes for text, got {text!r}")
-        self.set_raw_chunks([text], sha)
+        self.set_raw_chunks([text], sha, verify_sha=verify_sha)
 
     def set_raw_chunks(
         self,
         chunks: list[bytes],
         sha: ObjectID | RawObjectID | None = None,
         *,
         object_format: ObjectFormat | None = None,
+        verify_sha: ObjectID | RawObjectID | None = None,
     ) -> None:
-        """Set the contents of this object from a list of chunks."""
+        """Set the contents of this object from a list of chunks.
+
+        Args:
+          chunks: The raw uncompressed contents.
+          sha: Optional known, trusted sha for the object. Cached without
+            being checked against the contents.
+          object_format: Optional hash algorithm for the object.
+          verify_sha: Optional sha that the contents are checked against;
+            raises ChecksumMismatch if the object does not hash to it. On
+            success it is cached like ``sha``. Mutually exclusive with ``sha``.
+        """
+        if sha is not None and verify_sha is not None:
+            raise ValueError("sha and verify_sha are mutually exclusive")
         self._chunked_text = chunks
         # Set hash algorithm if provided
         if object_format is not None:
@@ -583,6 +600,11 @@ def set_raw_chunks(
             self._sha = FixedSha(sha)
         self._deserialize(chunks)
         self._needs_serialization = False
+        if verify_sha is not None:
+            got = self.get_id(self.object_format)
+            if got != verify_sha:
+                raise ChecksumMismatch(verify_sha, got)
+            self._sha = FixedSha(verify_sha)
 
     @staticmethod
     def _parse_object_header(
@@ -690,22 +712,25 @@ def from_raw_string(
         sha: ObjectID | RawObjectID | None = None,
         *,
         object_format: ObjectFormat | None = None,
+        verify_sha: ObjectID | RawObjectID | None = None,
     ) -> "ShaFile":
         """Creates an object of the indicated type from the raw string given.
 
         Args:
           type_num: The numeric type of the object.
           string: The raw uncompressed contents.
-          sha: Optional known sha for the object
+          sha: Optional known, trusted sha for the object.
           object_format: Optional hash algorithm for the object
+          verify_sha: Optional sha to check the contents against; raises
+            ChecksumMismatch on mismatch. Mutually exclusive with ``sha``.
         """
         cls = object_class(type_num)
         if cls is None:
             raise AssertionError(f"unsupported class type num: {type_num}")
         obj = cls()
         if object_format is not None:
             obj.object_format = object_format
-        obj.set_raw_string(string, sha)
+        obj.set_raw_string(string, sha, verify_sha=verify_sha)
         return obj
 
     @staticmethod

tests/cli/test_cli.py

@@ -3066,7 +3066,7 @@ def test_mktag_create(self):
         # Verify the tag object exists and has correct content
         from dulwich.objects import Tag
 
-        tag_obj = self.repo.object_store[tag_sha]
+        tag_obj = self.repo.object_store[tag_sha.encode("ascii")]
         self.assertIsInstance(tag_obj, Tag)
         self.assertEqual(tag_obj.object[1], commit_sha)
         self.assertEqual(tag_obj.name, b"v1.0")

tests/porcelain/test_bisect.py

@@ -45,15 +45,15 @@ def setUp(self) -> None:
         self.repo.object_store.add_object(tree)
 
         # Create some commits with proper trees
-        self.c1 = make_commit(id=b"1" * 40, message=b"initial commit", tree=tree.id)
+        self.c1 = make_commit(message=b"initial commit", tree=tree.id)
         self.c2 = make_commit(
-            id=b"2" * 40, message=b"second commit", parents=[b"1" * 40], tree=tree.id
+            message=b"second commit", parents=[self.c1.id], tree=tree.id
         )
         self.c3 = make_commit(
-            id=b"3" * 40, message=b"third commit", parents=[b"2" * 40], tree=tree.id
+            message=b"third commit", parents=[self.c2.id], tree=tree.id
         )
         self.c4 = make_commit(
-            id=b"4" * 40, message=b"fourth commit", parents=[b"3" * 40], tree=tree.id
+            message=b"fourth commit", parents=[self.c3.id], tree=tree.id
         )
 
         # Add commits to object store

tests/test_bisect.py

@@ -118,10 +118,10 @@ def test_reset_no_session(self) -> None:
     def test_bisect_workflow(self) -> None:
         """Test a complete bisect workflow."""
         # Create some commits
-        c1 = make_commit(id=b"1" * 40, message=b"good commit 1")
-        c2 = make_commit(id=b"2" * 40, message=b"good commit 2", parents=[b"1" * 40])
-        c3 = make_commit(id=b"3" * 40, message=b"bad commit", parents=[b"2" * 40])
-        c4 = make_commit(id=b"4" * 40, message=b"bad commit 2", parents=[b"3" * 40])
+        c1 = make_commit(message=b"good commit 1")
+        c2 = make_commit(message=b"good commit 2", parents=[c1.id])
+        c3 = make_commit(message=b"bad commit", parents=[c2.id])
+        c4 = make_commit(message=b"bad commit 2", parents=[c3.id])
 
         # Add commits to object store
         for commit in [c1, c2, c3, c4]:

tests/test_object_store.py

@@ -271,6 +271,19 @@ def test_corrupted_object_raise_exception(self) -> None:
         # this does not change iteration on loose objects though
         self.assertEqual([testobject.id], list(self.store._iter_loose_objects()))
 
+    def test_getitem_verifies_sha(self) -> None:
+        """Retrieving a loose object stored under a wrong sha is rejected."""
+        from dulwich.errors import ChecksumMismatch
+        from dulwich.file import GitFile
+
+        self.store.add_object(testobject)
+        wrong_sha = b"1" * 40
+        path = self.store._get_shafile_path(wrong_sha)
+        os.makedirs(os.path.dirname(path), exist_ok=True)
+        with GitFile(path, "wb") as f:
+            f.write(testobject.as_legacy_object())
+        self.assertRaises(ChecksumMismatch, self.store.__getitem__, wrong_sha)
+
     def test_tempfile_in_loose_store(self) -> None:
         self.store.add_object(testobject)
         self.assertEqual([testobject.id], list(self.store._iter_loose_objects()))

tests/test_server.py

@@ -241,17 +241,18 @@ def test_get_tagged(self) -> None:
     def test_nothing_to_do_but_wants(self) -> None:
         # Just the fact that the client claims to want an object is enough
         # for sending a pack. Even if there turns out to be nothing.
-        refs = {b"refs/tags/tag1": ONE}
         tree = Tree()
         self._repo.object_store.add_object(tree)
-        self._repo.object_store.add_object(make_commit(id=ONE, tree=tree))
+        commit = make_commit(tree=tree.id)
+        self._repo.object_store.add_object(commit)
+        refs = {b"refs/tags/tag1": commit.id}
         for name, sha in refs.items():
             self._repo.refs[name] = sha
         self._handler.proto.set_output(
             [
-                b"want " + ONE + b" side-band-64k thin-pack ofs-delta",
+                b"want " + commit.id + b" side-band-64k thin-pack ofs-delta",
                 None,
-                b"have " + ONE,
+                b"have " + commit.id,
                 b"done",
                 None,
             ]
@@ -262,10 +263,11 @@ def test_nothing_to_do_but_wants(self) -> None:
 
     def test_nothing_to_do_no_wants(self) -> None:
         # Don't send a pack if the client didn't ask for anything.
-        refs = {b"refs/tags/tag1": ONE}
         tree = Tree()
         self._repo.object_store.add_object(tree)
-        self._repo.object_store.add_object(make_commit(id=ONE, tree=tree))
+        commit = make_commit(tree=tree.id)
+        self._repo.object_store.add_object(commit)
+        refs = {b"refs/tags/tag1": commit.id}
         for ref, sha in refs.items():
             self._repo.refs[ref] = sha
         self._handler.proto.set_output([None])