Merge pull request #3633 from ethomson/safe_creation

Stricter object dependency checking during creation

7 files changed, 128 insertions, 24 deletions

diff --git a/src/commit.c b/src/commit.c
index 5a0509803..685c642aa 100644
--- a/src/commit.c
+++ b/src/commit.c
@@ -17,6 +17,7 @@
 #include "signature.h"
 #include "message.h"
 #include "refs.h"
+#include "object.h"
 
 void git_commit__free(void *_commit)
 {
@@ -36,7 +37,7 @@ void git_commit__free(void *_commit)
 	git__free(commit);
 }
 
-int git_commit_create_from_callback(
+static int git_commit__create_internal(
 	git_oid *id,
 	git_repository *repo,
 	const char *update_ref,
@@ -46,7 +47,8 @@ int git_commit_create_from_callback(
 	const char *message,
 	const git_oid *tree,
 	git_commit_parent_callback parent_cb,
-	void *parent_payload)
+	void *parent_payload,
+	bool validate)
 {
 	git_reference *ref = NULL;
 	int error = 0, matched_parent = 0;
@@ -58,6 +60,9 @@ int git_commit_create_from_callback(
 
 	assert(id && repo && tree && parent_cb);
 
+	if (validate && !git_object__is_valid(repo, tree, GIT_OBJ_TREE))
+		return -1;
+
 	if (update_ref) {
 		error = git_reference_lookup_resolved(&ref, repo, update_ref, 10);
 		if (error < 0 && error != GIT_ENOTFOUND)
@@ -71,6 +76,11 @@ int git_commit_create_from_callback(
 	git_oid__writebuf(&commit, "tree ", tree);
 
 	while ((parent = parent_cb(i, parent_payload)) != NULL) {
+		if (validate && !git_object__is_valid(repo, parent, GIT_OBJ_COMMIT)) {
+			error = -1;
+			goto on_error;
+		}
+
 		git_oid__writebuf(&commit, "parent ", parent);
 		if (i == 0 && current_id && git_oid_equal(current_id, parent))
 			matched_parent = 1;
@@ -114,10 +124,26 @@ int git_commit_create_from_callback(
 
 on_error:
 	git_buf_free(&commit);
-	giterr_set(GITERR_OBJECT, "Failed to create commit.");
 	return -1;
 }
 
+int git_commit_create_from_callback(
+	git_oid *id,
+	git_repository *repo,
+	const char *update_ref,
+	const git_signature *author,
+	const git_signature *committer,
+	const char *message_encoding,
+	const char *message,
+	const git_oid *tree,
+	git_commit_parent_callback parent_cb,
+	void *parent_payload)
+{
+	return git_commit__create_internal(
+		id, repo, update_ref, author, committer, message_encoding, message,
+		tree, parent_cb, parent_payload, true);
+}
+
 typedef struct {
 	size_t total;
 	va_list args;
@@ -153,10 +179,10 @@ int git_commit_create_v(
 	data.total = parent_count;
 	va_start(data.args, parent_count);
 
-	error = git_commit_create_from_callback(
+	error = git_commit__create_internal(
 		id, repo, update_ref, author, committer,
 		message_encoding, message, git_tree_id(tree),
-		commit_parent_from_varargs, &data);
+		commit_parent_from_varargs, &data, false);
 
 	va_end(data.args);
 	return error;
@@ -187,10 +213,10 @@ int git_commit_create_from_ids(
 {
 	commit_parent_oids data = { parent_count, parents };
 
-	return git_commit_create_from_callback(
+	return git_commit__create_internal(
 		id, repo, update_ref, author, committer,
 		message_encoding, message, tree,
-		commit_parent_from_ids, &data);
+		commit_parent_from_ids, &data, true);
 }
 
 typedef struct {
@@ -227,10 +253,10 @@ int git_commit_create(
 
 	assert(tree && git_tree_owner(tree) == repo);
 
-	return git_commit_create_from_callback(
+	return git_commit__create_internal(
 		id, repo, update_ref, author, committer,
 		message_encoding, message, git_tree_id(tree),
-		commit_parent_from_array, &data);
+		commit_parent_from_array, &data, false);
 }
 
 static const git_oid *commit_parent_for_amend(size_t curr, void *payload)
@@ -290,9 +316,9 @@ int git_commit_amend(
 		}
 	}
 
-	error = git_commit_create_from_callback(
+	error = git_commit__create_internal(
 		id, repo, NULL, author, committer, message_encoding, message,
-		&tree_id, commit_parent_for_amend, (void *)commit_to_amend);
+		&tree_id, commit_parent_for_amend, (void *)commit_to_amend, false);
 
 	if (!error && update_ref) {
 		error = git_reference__update_for_commit(
diff --git a/src/index.c b/src/index.c
index 5704432ae..b97f8091d 100644
--- a/src/index.c
+++ b/src/index.c
@@ -1245,17 +1245,22 @@ static void index_existing_and_best(
  * it, then it will return an error **and also free the entry**.  When
  * it replaces an existing entry, it will update the entry_ptr with the
  * actual entry in the index (and free the passed in one).
+ *
  * trust_path is whether we use the given path, or whether (on case
  * insensitive systems only) we try to canonicalize the given path to
  * be within an existing directory.
+ *
  * trust_mode is whether we trust the mode in entry_ptr.
+ *
+ * trust_id is whether we trust the id or it should be validated.
  */
 static int index_insert(
 	git_index *index,
 	git_index_entry **entry_ptr,
 	int replace,
 	bool trust_path,
-	bool trust_mode)
+	bool trust_mode,
+	bool trust_id)
 {
 	int error = 0;
 	size_t path_length, position;
@@ -1288,6 +1293,15 @@ static int index_insert(
 	if (!trust_path)
 		error = canonicalize_directory_path(index, entry, best);
 
+	/* ensure that the given id exists (unless it's a submodule) */
+	if (!error && !trust_id && INDEX_OWNER(index) &&
+		(entry->mode & GIT_FILEMODE_COMMIT) != GIT_FILEMODE_COMMIT) {
+
+		if (!git_object__is_valid(INDEX_OWNER(index), &entry->id,
+			git_object__type_from_filemode(entry->mode)))
+			error = -1;
+	}
+
 	/* look for tree / blob name collisions, removing conflicts if requested */
 	if (!error)
 		error = check_file_directory_collision(index, entry, position, replace);
@@ -1395,7 +1409,7 @@ int git_index_add_frombuffer(
 	git_oid_cpy(&entry->id, &id);
 	entry->file_size = len;
 
-	if ((error = index_insert(index, &entry, 1, true, true)) < 0)
+	if ((error = index_insert(index, &entry, 1, true, true, true)) < 0)
 		return error;
 
 	/* Adding implies conflict was resolved, move conflict entries to REUC */
@@ -1454,7 +1468,7 @@ int git_index_add_bypath(git_index *index, const char *path)
 	assert(index && path);
 
 	if ((ret = index_entry_init(&entry, index, path)) == 0)
-		ret = index_insert(index, &entry, 1, false, false);
+		ret = index_insert(index, &entry, 1, false, false, true);
 
 	/* If we were given a directory, let's see if it's a submodule */
 	if (ret < 0 && ret != GIT_EDIRECTORY)
@@ -1480,7 +1494,7 @@ int git_index_add_bypath(git_index *index, const char *path)
 			if ((ret = add_repo_as_submodule(&entry, index, path)) < 0)
 				return ret;
 
-			if ((ret = index_insert(index, &entry, 1, false, false)) < 0)
+			if ((ret = index_insert(index, &entry, 1, false, false, true)) < 0)
 				return ret;
 		} else if (ret < 0) {
 			return ret;
@@ -1569,7 +1583,7 @@ int git_index_add(git_index *index, const git_index_entry *source_entry)
 	}
 
 	if ((ret = index_entry_dup(&entry, index, source_entry)) < 0 ||
-		(ret = index_insert(index, &entry, 1, true, true)) < 0)
+		(ret = index_insert(index, &entry, 1, true, true, false)) < 0)
 		return ret;
 
 	git_tree_cache_invalidate_path(index->tree, entry->path);
@@ -1731,7 +1745,7 @@ int git_index_conflict_add(git_index *index,
 		/* Make sure stage is correct */
 		GIT_IDXENTRY_STAGE_SET(entries[i], i + 1);
 
-		if ((ret = index_insert(index, &entries[i], 1, true, true)) < 0)
+		if ((ret = index_insert(index, &entries[i], 1, true, true, false)) < 0)
 			goto on_error;
 
 		entries[i] = NULL; /* don't free if later entry fails */
diff --git a/src/object.c b/src/object.c
index b0a8199bc..ebf77fb47 100644
--- a/src/object.c
+++ b/src/object.c
@@ -14,6 +14,8 @@
 #include "blob.h"
 #include "tag.h"
 
+bool git_object__strict_input_validation = true;
+
 typedef struct {
 	const char	*str;	/* type name string */
 	size_t		size;	/* size in bytes of the object structure */
@@ -465,3 +467,27 @@ int git_object_short_id(git_buf *out, const git_object *obj)
 	return error;
 }
 
+bool git_object__is_valid(
+	git_repository *repo, const git_oid *id, git_otype expected_type)
+{
+	git_odb *odb;
+	git_otype actual_type;
+	size_t len;
+	int error;
+
+	if (!git_object__strict_input_validation)
+		return true;
+
+	if ((error = git_repository_odb__weakptr(&odb, repo)) < 0 ||
+		(error = git_odb_read_header(&len, &actual_type, odb, id)) < 0)
+		return false;
+
+	if (expected_type != GIT_OBJ_ANY && expected_type != actual_type) {
+		giterr_set(GITERR_INVALID,
+			"the requested type does not match the type in the ODB");
+		return false;
+	}
+
+	return true;
+}
+
diff --git a/src/object.h b/src/object.h
index d187c55b7..dd227d16d 100644
--- a/src/object.h
+++ b/src/object.h
@@ -7,6 +7,10 @@
 #ifndef INCLUDE_object_h__
 #define INCLUDE_object_h__
 
+#include "repository.h"
+
+extern bool git_object__strict_input_validation;
+
 /** Base git object for inheritance */
 struct git_object {
 	git_cached_obj cached;
@@ -28,4 +32,23 @@ int git_oid__parse(git_oid *oid, const char **buffer_out, const char *buffer_end
 
 void git_oid__writebuf(git_buf *buf, const char *header, const git_oid *oid);
 
+bool git_object__is_valid(
+	git_repository *repo, const git_oid *id, git_otype expected_type);
+
+GIT_INLINE(git_otype) git_object__type_from_filemode(git_filemode_t mode)
+{
+	switch (mode) {
+	case GIT_FILEMODE_TREE:
+		return GIT_OBJ_TREE;
+	case GIT_FILEMODE_COMMIT:
+		return GIT_OBJ_COMMIT;
+	case GIT_FILEMODE_BLOB:
+	case GIT_FILEMODE_BLOB_EXECUTABLE:
+	case GIT_FILEMODE_LINK:
+		return GIT_OBJ_BLOB;
+	default:
+		return GIT_OBJ_BAD;
+	}
+}
+
 #endif
diff --git a/src/refs.c b/src/refs.c
index 7b538659d..a15e31b53 100644
--- a/src/refs.c
+++ b/src/refs.c
@@ -377,15 +377,9 @@ static int reference__create(
 		return error;
 
 	if (oid != NULL) {
-		git_odb *odb;
-
 		assert(symbolic == NULL);
 
-		/* Sanity check the reference being created - target must exist. */
-		if ((error = git_repository_odb__weakptr(&odb, repo)) < 0)
-			return error;
-
-		if (!git_odb_exists(odb, oid)) {
+		if (!git_object__is_valid(repo, oid, GIT_OBJ_ANY)) {
 			giterr_set(GITERR_REFERENCE,
 				"Target OID for the reference doesn't exist on the repository");
 			return -1;
diff --git a/src/settings.c b/src/settings.c
index d7341abe8..88602bad0 100644
--- a/src/settings.c
+++ b/src/settings.c
@@ -14,6 +14,7 @@
 #include "sysdir.h"
 #include "cache.h"
 #include "global.h"
+#include "object.h"
 
 void git_libgit2_version(int *major, int *minor, int *rev)
 {
@@ -181,6 +182,11 @@ int git_libgit2_opts(int key, ...)
 		}
 
 		break;
+
+	case GIT_OPT_ENABLE_STRICT_OBJECT_CREATION:
+		git_object__strict_input_validation = (va_arg(ap, int) != 0);
+		break;
+
 	default:
 		giterr_set(GITERR_INVALID, "invalid option key");
 		error = -1;
diff --git a/src/tree.c b/src/tree.c
index cfceb3f33..2c3151546 100644
--- a/src/tree.c
+++ b/src/tree.c
@@ -726,6 +726,18 @@ on_error:
 	return -1;
 }
 
+static git_otype otype_from_mode(git_filemode_t filemode)
+{
+	switch (filemode) {
+	case GIT_FILEMODE_TREE:
+		return GIT_OBJ_TREE;
+	case GIT_FILEMODE_COMMIT:
+		return GIT_OBJ_COMMIT;
+	default:
+		return GIT_OBJ_BLOB;
+	}
+}
+
 int git_treebuilder_insert(
 	const git_tree_entry **entry_out,
 	git_treebuilder *bld,
@@ -745,6 +757,9 @@ int git_treebuilder_insert(
 	if (!valid_entry_name(bld->repo, filename))
 		return tree_error("Failed to insert entry. Invalid name for a tree entry", filename);
 
+	if (!git_object__is_valid(bld->repo, id, otype_from_mode(filemode)))
+		return tree_error("Failed to insert entry; invalid object specified", filename);
+
 	pos = git_strmap_lookup_index(bld->map, filename);
 	if (git_strmap_valid_index(bld->map, pos)) {
 		entry = git_strmap_value_at(bld->map, pos);