Merge pull request #1296 from arrbee/stricter-config-name-checks

Stricter config entry name validation

5 files changed, 170 insertions, 62 deletions

diff --git a/src/config.c b/src/config.c
index 84f3ba0f9..ce105089e 100644
--- a/src/config.c
+++ b/src/config.c
@@ -11,6 +11,7 @@
 #include "git2/config.h"
 #include "vector.h"
 #include "buf_text.h"
+#include "config_file.h"
 #if GIT_WIN32
 # include <windows.h>
 #endif
@@ -758,42 +759,36 @@ fail_parse:
 	return -1;
 }
 
-struct rename_data
-{
+struct rename_data {
 	git_config *config;
-	const char *old_name;
-	const char *new_name;
+	git_buf *name;
+	size_t old_len;
+	int actual_error;
 };
 
 static int rename_config_entries_cb(
 	const git_config_entry *entry,
 	void *payload)
 {
+	int error = 0;
 	struct rename_data *data = (struct rename_data *)payload;
+	size_t base_len = git_buf_len(data->name);
 
-	if (data->new_name != NULL) {
-		git_buf name = GIT_BUF_INIT;
-		int error;
-
-		if (git_buf_printf(
-			&name,
-			"%s.%s",
-			data->new_name,
-			entry->name + strlen(data->old_name) + 1) < 0)
-				return -1;
-
+	if (base_len > 0 &&
+		!(error = git_buf_puts(data->name, entry->name + data->old_len)))
+	{
 		error = git_config_set_string(
-			data->config,
-			git_buf_cstr(&name),
-			entry->value);
+			data->config, git_buf_cstr(data->name), entry->value);
 
-		git_buf_free(&name);
-
-		if (error)
-			return error;
+		git_buf_truncate(data->name, base_len);
 	}
 
-	return git_config_delete_entry(data->config, entry->name);
+	if (!error)
+		error = git_config_delete_entry(data->config, entry->name);
+
+	data->actual_error = error; /* preserve actual error code */
+
+	return error;
 }
 
 int git_config_rename_section(
@@ -802,36 +797,44 @@ int git_config_rename_section(
 	const char *new_section_name)
 {
 	git_config *config;
-	git_buf pattern = GIT_BUF_INIT;
-	int error = -1;
+	git_buf pattern = GIT_BUF_INIT, replace = GIT_BUF_INIT;
+	int error = 0;
 	struct rename_data data;
 
-	git_buf_text_puts_escape_regex(&pattern,  old_section_name);
-	git_buf_puts(&pattern, "\\..+");
-	if (git_buf_oom(&pattern))
+	git_buf_text_puts_escape_regex(&pattern, old_section_name);
+
+	if ((error = git_buf_puts(&pattern, "\\..+")) < 0)
+		goto cleanup;
+
+	if ((error = git_repository_config__weakptr(&config, repo)) < 0)
 		goto cleanup;
 
-	if (git_repository_config__weakptr(&config, repo) < 0)
+	data.config  = config;
+	data.name    = &replace;
+	data.old_len = strlen(old_section_name) + 1;
+	data.actual_error = 0;
+
+	if ((error = git_buf_join(&replace, '.', new_section_name, "")) < 0)
 		goto cleanup;
 
-	data.config = config;
-	data.old_name = old_section_name;
-	data.new_name = new_section_name;
-
-	if ((error = git_config_foreach_match(
-			config,
-			git_buf_cstr(&pattern),
-			rename_config_entries_cb, &data)) < 0) {
-				giterr_set(GITERR_CONFIG,
-					"Cannot rename config section '%s' to '%s'",
-					old_section_name,
-					new_section_name);
-				goto cleanup;
+	if (new_section_name != NULL &&
+		(error = git_config_file_normalize_section(
+			replace.ptr, strchr(replace.ptr, '.'))) < 0)
+	{
+		giterr_set(
+			GITERR_CONFIG, "Invalid config section '%s'", new_section_name);
+		goto cleanup;
 	}
 
-	error = 0;
+	error = git_config_foreach_match(
+		config, git_buf_cstr(&pattern), rename_config_entries_cb, &data);
+
+	if (error == GIT_EUSER)
+		error = data.actual_error;
 
 cleanup:
 	git_buf_free(&pattern);
+	git_buf_free(&replace);
+
 	return error;
 }
diff --git a/src/config_file.c b/src/config_file.c
index 2b1be05bf..8b51ab21b 100644
--- a/src/config_file.c
+++ b/src/config_file.c
@@ -105,6 +105,29 @@ static void cvar_free(cvar_t *var)
 	git__free(var);
 }
 
+int git_config_file_normalize_section(char *start, char *end)
+{
+	char *scan;
+
+	if (start == end)
+		return GIT_EINVALIDSPEC;
+
+	/* Validate and downcase range */
+	for (scan = start; *scan; ++scan) {
+		if (end && scan >= end)
+			break;
+		if (isalnum(*scan))
+			*scan = tolower(*scan);
+		else if (*scan != '-' || scan == start)
+			return GIT_EINVALIDSPEC;
+	}
+
+	if (scan == start)
+		return GIT_EINVALIDSPEC;
+
+	return 0;
+}
+
 /* Take something the user gave us and make it nice for our hash function */
 static int normalize_name(const char *in, char **out)
 {
@@ -118,19 +141,26 @@ static int normalize_name(const char *in, char **out)
 	fdot = strchr(name, '.');
 	ldot = strrchr(name, '.');
 
-	if (fdot == NULL || ldot == NULL) {
-		git__free(name);
-		giterr_set(GITERR_CONFIG,
-			"Invalid variable name: '%s'", in);
-		return -1;
-	}
+	if (fdot == NULL || fdot == name || ldot == NULL || !ldot[1])
+		goto invalid;
+
+	/* Validate and downcase up to first dot and after last dot */
+	if (git_config_file_normalize_section(name, fdot) < 0 ||
+		git_config_file_normalize_section(ldot + 1, NULL) < 0)
+		goto invalid;
 
-	/* Downcase up to the first dot and after the last one */
-	git__strntolower(name, fdot - name);
-	git__strtolower(ldot);
+	/* If there is a middle range, make sure it doesn't have newlines */
+	while (fdot < ldot)
+		if (*fdot++ == '\n')
+			goto invalid;
 
 	*out = name;
 	return 0;
+
+invalid:
+	git__free(name);
+	giterr_set(GITERR_CONFIG, "Invalid config item name '%s'", in);
+	return GIT_EINVALIDSPEC;
 }
 
 static void free_vars(git_strmap *values)
@@ -271,8 +301,8 @@ static int config_set(git_config_backend *cfg, const char *name, const char *val
 	khiter_t pos;
 	int rval, ret;
 
-	if (normalize_name(name, &key) < 0)
-		return -1;
+	if ((rval = normalize_name(name, &key)) < 0)
+		return rval;
 
 	/*
 	 * Try to find it in the existing values and update it if it
@@ -352,9 +382,10 @@ static int config_get(const git_config_backend *cfg, const char *name, const git
 	diskfile_backend *b = (diskfile_backend *)cfg;
 	char *key;
 	khiter_t pos;
+	int error;
 
-	if (normalize_name(name, &key) < 0)
-		return -1;
+	if ((error = normalize_name(name, &key)) < 0)
+		return error;
 
 	pos = git_strmap_lookup_index(b->values, key);
 	git__free(key);
@@ -379,9 +410,10 @@ static int config_get_multivar(
 	diskfile_backend *b = (diskfile_backend *)cfg;
 	char *key;
 	khiter_t pos;
+	int error;
 
-	if (normalize_name(name, &key) < 0)
-		return -1;
+	if ((error = normalize_name(name, &key)) < 0)
+		return error;
 
 	pos = git_strmap_lookup_index(b->values, key);
 	git__free(key);
@@ -444,8 +476,8 @@ static int config_set_multivar(
 
 	assert(regexp);
 
-	if (normalize_name(name, &key) < 0)
-		return -1;
+	if ((result = normalize_name(name, &key)) < 0)
+		return result;
 
 	pos = git_strmap_lookup_index(b->values, key);
 	if (!git_strmap_valid_index(b->values, pos)) {
@@ -515,8 +547,8 @@ static int config_delete(git_config_backend *cfg, const char *name)
 	int result;
 	khiter_t pos;
 
-	if (normalize_name(name, &key) < 0)
-		return -1;
+	if ((result = normalize_name(name, &key)) < 0)
+		return result;
 
 	pos = git_strmap_lookup_index(b->values, key);
 	git__free(key);
diff --git a/src/config_file.h b/src/config_file.h
index cf1a59036..7445859c4 100644
--- a/src/config_file.h
+++ b/src/config_file.h
@@ -54,5 +54,7 @@ GIT_INLINE(int) git_config_file_foreach_match(
 	return cfg->foreach(cfg, regexp, fn, data);
 }
 
+extern int git_config_file_normalize_section(char *start, char *end);
+
 #endif
 
diff --git a/tests-clar/config/validkeyname.c b/tests-clar/config/validkeyname.c
new file mode 100644
index 000000000..03c13d723
--- /dev/null
+++ b/tests-clar/config/validkeyname.c
@@ -0,0 +1,68 @@
+#include "clar_libgit2.h"
+
+#include "config.h"
+
+static git_config *cfg;
+static const char *value;
+
+void test_config_validkeyname__initialize(void)
+{
+	cl_fixture_sandbox("config/config10");
+
+	cl_git_pass(git_config_open_ondisk(&cfg, "config10"));
+}
+
+void test_config_validkeyname__cleanup(void)
+{
+	git_config_free(cfg);
+	cfg = NULL;
+
+	cl_fixture_cleanup("config10");
+}
+
+static void assert_invalid_config_key_name(const char *name)
+{
+	cl_git_fail_with(git_config_get_string(&value, cfg, name),
+		GIT_EINVALIDSPEC);
+	cl_git_fail_with(git_config_set_string(cfg, name, "42"),
+		GIT_EINVALIDSPEC);
+	cl_git_fail_with(git_config_delete_entry(cfg, name),
+		GIT_EINVALIDSPEC);
+	cl_git_fail_with(git_config_get_multivar(cfg, name, "*", NULL, NULL),
+		GIT_EINVALIDSPEC);
+	cl_git_fail_with(git_config_set_multivar(cfg, name, "*", "42"),
+		GIT_EINVALIDSPEC);
+}
+
+void test_config_validkeyname__accessing_requires_a_valid_name(void)
+{
+	assert_invalid_config_key_name("");
+	assert_invalid_config_key_name(".");
+	assert_invalid_config_key_name("..");
+	assert_invalid_config_key_name("core.");
+	assert_invalid_config_key_name("d#ff.dirstat.lines");
+	assert_invalid_config_key_name("diff.dirstat.lines#");
+	assert_invalid_config_key_name("dif\nf.dirstat.lines");
+	assert_invalid_config_key_name("dif.dir\nstat.lines");
+	assert_invalid_config_key_name("dif.dirstat.li\nes");
+}
+
+static void assert_invalid_config_section_name(git_repository *repo, const char *name)
+{
+	cl_git_fail_with(git_config_rename_section(repo, "branch.remoteless", name), GIT_EINVALIDSPEC);
+}
+
+void test_config_validkeyname__renaming_a_section_requires_a_valid_name(void)
+{
+	git_repository *repo;
+
+	cl_git_pass(git_repository_open(&repo, cl_fixture("testrepo.git")));
+
+	assert_invalid_config_section_name(repo, "");
+	assert_invalid_config_section_name(repo, "bra\nch");
+	assert_invalid_config_section_name(repo, "branc#");
+	assert_invalid_config_section_name(repo, "bra\nch.duh");
+	assert_invalid_config_section_name(repo, "branc#.duh");
+
+	git_repository_free(repo);
+}
diff --git a/tests-clar/core/buffer.c b/tests-clar/core/buffer.c
index 5d9b7850c..49ab41f71 100644
--- a/tests-clar/core/buffer.c
+++ b/tests-clar/core/buffer.c
@@ -457,6 +457,9 @@ void test_core_buffer__8(void)
 
 	git_buf_free(&a);
 
+	check_joinbuf_2(NULL, "", "");
+	check_joinbuf_2(NULL, "a", "a");
+	check_joinbuf_2(NULL, "/a", "/a");
 	check_joinbuf_2("", "", "");
 	check_joinbuf_2("", "a", "a");
 	check_joinbuf_2("", "/a", "/a");