Assure recursive shared indices (possibly malicious) don't crash us

The way this is resolved is similar to what git does by means of comparing the expected and actual hashes of the file at hand. Note that we do not actually protect against recursion but assume that nobody can manufacture a file hash that in fact is recursive. We know that this can actually happen but chose to not consider it worth protecting against just yet, as seemingly neither does git.

Sebastian Thiel committed 3 years ago

Commit ede718c1 parent d6ba3667 files · +136 −74Browse files

Unified · Split

7 files

git-index/src/decode/mod.rs +18 −0
git-index/src/extension/decode.rs +8 −4
git-index/src/extension/link.rs +72 −0
git-index/src/extension/mod.rs +2 −1
git-index/src/file/init.rs +2 −65
git-index/tests/fixtures/make_index/V2_split_index_recursive.sh +19 −0
git-index/tests/index/file/read.rs +15 −4

git-index/src/decode/mod.rs+18−0

         Extension(#[from] extension::decode::Error),
         #[error("Index trailer should have been {expected} bytes long, but was {actual}")]
         UnexpectedTrailerLength { expected: usize, actual: usize },
+        #[error("Shared index checksum was {actual_checksum} but should have been {expected_checksum}")]
+        ChecksumMismatch {
+            actual_checksum: git_hash::ObjectId,
+            expected_checksum: git_hash::ObjectId,
+        },
     }
 }
 pub use error::Error;
     /// The minimum size in bytes to load extensions in their own thread, assuming there is enough `num_threads` available.
     /// If set to 0, for example, extensions will always be read in their own thread if enough threads are available.
     pub min_extension_block_in_bytes_for_threading: usize,
+    /// Set the expected hash of this index if we are read as part of a `link` extension.
+    ///
+    /// We will abort reading this file if it doesn't match.
+    pub expected_checksum: Option<git_hash::ObjectId>,
 }
 impl State {
         Options {
             thread_limit,
             min_extension_block_in_bytes_for_threading,
+            expected_checksum,
         }: Options,
     ) -> Result<(Self, git_hash::ObjectId), Error> {
         let (version, num_entries, post_header_data) = header::decode(data, object_hash)?;
         }
         let checksum = git_hash::ObjectId::from(data);
+        if let Some(expected_checksum) = expected_checksum {
+            if checksum != expected_checksum {
+                return Err(Error::ChecksumMismatch {
+                    actual_checksum: checksum,
+                    expected_checksum,
+                });
+            }
+        }
         let EntriesOutcome {
             entries,
             path_backing,

git-index/src/extension/decode.rs+8−4

 use crate::{extension, extension::Signature, util::from_be_u32};
-pub fn header(data: &[u8]) -> (Signature, u32, &[u8]) {
+pub(crate) fn header(data: &[u8]) -> (Signature, u32, &[u8]) {
     let (signature, data) = data.split_at(4);
     let (size, data) = data.split_at(4);
     (signature.try_into().unwrap(), from_be_u32(size), data)
 mod error {
     use crate::extension;
-    /// The error returned by [extension::decode::all()][extension::decode::all()].
+    /// The error returned when decoding extensions.
     #[derive(Debug, thiserror::Error)]
+    #[allow(missing_docs)]
     pub enum Error {
         #[error(
             "Encountered mandatory extension '{}' which isn't implemented yet",
 }
 pub use error::Error;
-pub fn all(maybe_beginning_of_extensions: &[u8], object_hash: git_hash::Kind) -> Result<(Outcome, &[u8]), Error> {
+pub(crate) fn all(
+    maybe_beginning_of_extensions: &[u8],
+    object_hash: git_hash::Kind,
+) -> Result<(Outcome, &[u8]), Error> {
     let mut ext_iter = match extension::Iter::new_without_checksum(maybe_beginning_of_extensions, object_hash) {
         Some(iter) => iter,
         None => return Ok((Outcome::default(), maybe_beginning_of_extensions)),
 }
 #[derive(Default)]
-pub struct Outcome {
+pub(crate) struct Outcome {
     pub tree: Option<extension::Tree>,
     pub link: Option<extension::Link>,
     pub resolve_undo: Option<extension::resolve_undo::Paths>,

git-index/src/extension/link.rs+72−0

@@ -60,3 +60,75 @@ pub(crate) fn decode(data: &[u8], object_hash: git_hash::Kind) -> Result<Link, d · 59 unmodified lines
60	bitmaps: Some(Bitmaps { delete, replace }),	60	bitmaps: Some(Bitmaps { delete, replace }),
61	})	61	})
62	}	62	}
		63
		64	impl Link {
		65	pub(crate) fn dissolve_into(
		66	self,
		67	split_index: &mut crate::File,
		68	object_hash: git_hash::Kind,
		69	options: crate::decode::Options,
		70	) -> Result<(), crate::file::init::Error> {
		71	let shared_index_path = split_index
		72	.path
		73	.parent()
		74	.expect("split index file in .git folder")
		75	.join(format!("sharedindex.{}", self.shared_index_checksum));
		76	let mut shared_index = crate::File::at(
		77	&shared_index_path,
		78	object_hash,
		79	crate::decode::Options {
		80	expected_checksum: self.shared_index_checksum.into(),
		81	..options
		82	},
		83	)?;
		84
		85	if let Some(bitmaps) = &self.bitmaps {
		86	let mut counter = 0;
		87	let shared_entries = shared_index.entries_mut();
		88	let split_entries = split_index.entries();
		89
		90	bitmaps.replace.for_each_set_bit(\|index\| {
		91	match (shared_entries.get_mut(index), split_entries.get(counter)) {
		92	(Some(shared_entry), Some(split_entry)) => {
		93	shared_entry.stat = split_entry.stat;
		94	shared_entry.id = split_entry.id;
		95	shared_entry.flags = split_entry.flags;
		96	shared_entry.mode = split_entry.mode;
		97	}
		98	_ => unreachable!(),
		99	}
		100
		101	counter += 1;
		102	Some(())
		103	});
		104
		105	if split_entries.len() > counter {
		106	split_entries[counter..].iter().for_each(\|split_entry\| {
		107	let mut e = split_entry.clone();
		108	let start = shared_index.path_backing.len();
		109	e.path = start..start + split_entry.path.len();
		110	shared_index.entries.push(e);
		111
		112	shared_index
		113	.path_backing
		114	.extend_from_slice(&split_index.path_backing[split_entry.path.clone()]);
		115	});
		116	}
		117
		118	let mut removed_count = 0;
		119	bitmaps.delete.for_each_set_bit(\|index\| {
		120	shared_index.entries.remove(index - removed_count);
		121	removed_count += 1;
		122	Some(())
		123	});
		124
		125	let mut shared_entries = std::mem::take(&mut shared_index.entries);
		126	shared_entries.sort_by(\|a, b\| a.cmp(b, &shared_index.state));
		127
		128	std::mem::swap(&mut split_index.entries, &mut shared_entries);
		129	std::mem::swap(&mut split_index.path_backing, &mut shared_index.path_backing);
		130	}
		131
		132	Ok(())
		133	}
		134	}

git-index/src/extension/mod.rs+2−1

@@ -73,7 +73,8 @@ mod iter; · 72 unmodified lines
73		73
74	pub(crate) mod fs_monitor;	74	pub(crate) mod fs_monitor;
75		75
76	pub(crate) mod decode;	76	///
		77	pub mod decode;
77		78
78	///	79	///
79	pub mod tree;	80	pub mod tree;

git-index/src/file/init.rs+2−65

@@ -40,8 +40,8 @@ impl File { · 39 unmodified lines
40	path,	40	path,
41	checksum: Some(checksum),	41	checksum: Some(checksum),
42	};	42	};
43	if let Some(link) = file.link.take() {	43	if let Some(mut link) = file.link.take() {
44	dissolve_link_extension(&mut file, link, object_hash, options)?;	44	link.dissolve_into(&mut file, object_hash, options)?;
45	}	45	}
46		46
47	Ok(file)	47	Ok(file)
@@ -58,66 +58,3 @@ impl File { · 10 unmodified lines
58	}	58	}
59	}	59	}
60	}	60	}
61
62	fn dissolve_link_extension(
63	split_index: &mut File,
64	link: extension::Link,
65	object_hash: git_hash::Kind,
66	options: decode::Options,
67	) -> Result<(), Error> {
68	let shared_index_path = split_index
69	.path
70	.parent()
71	.expect("split index file in .git folder")
72	.join(format!("sharedindex.{}", link.shared_index_checksum));
73	let mut shared_index = File::at(&shared_index_path, object_hash, options)?;
74
75	if let Some(bitmaps) = &link.bitmaps {
76	let mut counter = 0;
77	let mut shared_entries = shared_index.entries_mut();
78	let split_entries = split_index.entries();
79
80	bitmaps.replace.for_each_set_bit(\|index\| {
81	match (shared_entries.get_mut(index), split_entries.get(counter)) {
82	(Some(shared_entry), Some(split_entry)) => {
83	shared_entry.stat = split_entry.stat;
84	shared_entry.id = split_entry.id;
85	shared_entry.flags = split_entry.flags;
86	shared_entry.mode = split_entry.mode;
87	}
88	_ => unreachable!(),
89	}
90
91	counter += 1;
92	Some(())
93	});
94
95	if split_entries.len() > counter {
96	split_entries[counter..].iter().for_each(\|split_entry\| {
97	let mut e = split_entry.clone();
98	let start = shared_index.path_backing.len();
99	e.path = start..start + split_entry.path.len();
100	shared_index.entries.push(e);
101
102	shared_index
103	.path_backing
104	.extend_from_slice(&split_index.path_backing[split_entry.path.clone()]);
105	});
106	}
107
108	let mut removed_count = 0;
109	bitmaps.delete.for_each_set_bit(\|index\| {
110	shared_index.entries.remove(index - removed_count);
111	removed_count += 1;
112	Some(())
113	});
114
115	let mut shared_entries = std::mem::take(&mut shared_index.entries);
116	shared_entries.sort_by(\|a, b\| a.cmp(b, &shared_index.state));
117
118	std::mem::swap(&mut split_index.entries, &mut shared_entries);
119	std::mem::swap(&mut split_index.path_backing, &mut shared_index.path_backing);
120	}
121
122	Ok(())
123	}

git-index/tests/fixtures/make_index/V2_split_index_recursive.sh+19−0

@@ -0,0 +1,19 @@
	1	#!/bin/bash
	2	set -eu -o pipefail
	3
	4	export GIT_INDEX_VERSION=2
	5	git init -q
	6	git config index.threads 1
	7
	8	touch a
	9	git add a
	10	git commit -m "empty"
	11
	12	git update-index --split-index
	13
	14	shared_index=".git/sharedindex.*"
	15	test -f $shared_index \|\| \
	16	{ echo "shared index must be present in 'split' repository" && exit 42; }
	17
	18	cp .git/index $shared_index
	19

git-index/tests/index/file/read.rs+15−4

     let file = git_index::File::at(path, git_hash::Kind::Sha1, Default::default()).unwrap();
     verify(file)
 }
-pub(crate) fn file(name: &str) -> git_index::File {
+pub(crate) fn try_file(name: &str) -> Result<git_index::File, git_index::file::init::Error> {
     let file = git_index::File::at(
         crate::fixture_index_path(name),
         git_hash::Kind::Sha1,
         Default::default(),
-    )
+    )?;
-    .unwrap();
+    Ok(verify(file))
-    verify(file)
+}
+pub(crate) fn file(name: &str) -> git_index::File {
+    try_file(name).unwrap()
 }
 fn file_opt(name: &str, opts: git_index::decode::Options) -> git_index::File {
     let file = git_index::File::at(crate::fixture_index_path(name), git_hash::Kind::Sha1, opts).unwrap();
     assert_eq!(file.version(), Version::V2);
 }
+#[test]
+fn v2_split_index_recursion_is_handled_gracefully() {
+    let err = try_file("v2_split_index_recursive").expect_err("recursion fails gracefully");
+    assert!(matches!(
+        err,
+        git_index::file::init::Error::Decode(git_index::decode::Error::ChecksumMismatch { .. })
+    ));
+}
 #[test]
 fn split_index_and_regular_index_of_same_content_are_indeed_the_same() {
     let base =

Assure recursive shared indices (possibly malicious) don't crash us

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code