[PATCH 0/7] fix segfaults with implicit-bool config

[PATCH 0/7] fix segfaults with implicit-bool config

[Jeff King]

Carlos reported to the security list a case where you can cause Git
to segfault by using an implicit bool like:

  [core]
  someVariable

when the parsing side for core.someVariable does not correctly check a
NULL "value" string. This is mostly harmless, as anybody who can feed
arbitrary config can already execute arbitrary code. There is one case
of this when parsing .gitmodules (which we don't trust), but even there
I don't think the security implications are that interesting. A
malicious repo can get "clone --recurse-submodules" to segfault, but
always with a strict NULL dereference, not any kind of controllable
pointer. See patch 5 for more details.

I audited the whole code base for instances of the problem. It was
fairly manual, so it's possible I missed a spot, but I think this should
cover everything.

The first patch has vanilla cases, and the rest are instances where I
thought it was worth calling out specific details.

  [1/7]: config: handle NULL value when parsing non-bools
  [2/7]: setup: handle NULL value when parsing extensions
  [3/7]: trace2: handle NULL values in tr2_sysenv config callback
  [4/7]: help: handle NULL value for alias.* config
  [5/7]: submodule: handle NULL value when parsing submodule.*.branch
  [6/7]: trailer: handle NULL value when parsing trailer-specific config
  [7/7]: fsck: handle NULL value when parsing message config

 builtin/blame.c        |  2 ++
 builtin/checkout.c     |  2 ++
 builtin/clone.c        |  2 ++
 builtin/log.c          |  5 ++++-
 builtin/pack-objects.c |  6 +++++-
 builtin/receive-pack.c | 11 +++++++----
 compat/mingw.c         |  2 ++
 config.c               |  8 ++++++++
 diff.c                 | 19 ++++++++++++++++---
 fetch-pack.c           | 12 ++++++++----
 fsck.c                 |  8 ++++++--
 help.c                 |  5 ++++-
 mailinfo.c             |  2 ++
 notes-utils.c          |  2 ++
 setup.c                |  2 ++
 submodule-config.c     |  4 +++-
 trace2/tr2_sysenv.c    |  2 ++
 trailer.c              |  8 ++++++++
 18 files changed, 85 insertions(+), 17 deletions(-)

[PATCH 1/7] config: handle NULL value when parsing non-bools

[Jeff King]

When the config parser sees an "implicit" bool like:

  [core]
  someVariable

it passes NULL to the config callback. Any callback code which expects a
string must check for NULL. This usually happens via helpers like
git_config_string(), etc, but some custom code forgets to do so and will
segfault.

These are all fairly vanilla cases where the solution is just the usual
pattern of:

  if (!value)
        return config_error_nonbool(var);

though note that in a few cases we have to split initializers like:

  int some_var = initializer();

into:

  int some_var;
  if (!value)
        return config_error_nonbool(var);
  some_var = initializer();

There are still some broken instances after this patch, which I'll
address on their own in individual patches after this one.

Reported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/blame.c        |  2 ++
 builtin/checkout.c     |  2 ++
 builtin/clone.c        |  2 ++
 builtin/log.c          |  5 ++++-
 builtin/pack-objects.c |  6 +++++-
 compat/mingw.c         |  2 ++
 config.c               |  8 ++++++++
 diff.c                 | 19 ++++++++++++++++---
 mailinfo.c             |  2 ++
 notes-utils.c          |  2 ++
 trailer.c              |  2 ++
 11 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 9c987d6567..2433b7da5c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -748,6 +748,8 @@ static int git_blame_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "blame.coloring")) {
+		if (!value)
+			return config_error_nonbool(var);
 		if (!strcmp(value, "repeatedLines")) {
 			coloring_mode |= OUTPUT_COLOR_LINE;
 		} else if (!strcmp(value, "highlightRecent")) {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f02434bc15..d5c784854f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1202,6 +1202,8 @@ static int git_checkout_config(const char *var, const char *value,
 	struct checkout_opts *opts = cb;
 
 	if (!strcmp(var, "diff.ignoresubmodules")) {
+		if (!value)
+			return config_error_nonbool(var);
 		handle_ignore_submodules_arg(&opts->diff_options, value);
 		return 0;
 	}
diff --git a/builtin/clone.c b/builtin/clone.c
index c6357af949..54d9b9976a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -791,6 +791,8 @@ static int git_clone_config(const char *k, const char *v,
 			    const struct config_context *ctx, void *cb)
 {
 	if (!strcmp(k, "clone.defaultremotename")) {
+		if (!v)
+			return config_error_nonbool(k);
 		free(remote_name);
 		remote_name = xstrdup(v);
 	}
diff --git a/builtin/log.c b/builtin/log.c
index ba775d7b5c..3ce41c4856 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -594,8 +594,11 @@ static int git_log_config(const char *var, const char *value,
 			decoration_style = 0; /* maybe warn? */
 		return 0;
 	}
-	if (!strcmp(var, "log.diffmerges"))
+	if (!strcmp(var, "log.diffmerges")) {
+		if (!value)
+			return config_error_nonbool(var);
 		return diff_merges_config(value);
+	}
 	if (!strcmp(var, "log.showroot")) {
 		default_show_root = git_config_bool(var, value);
 		return 0;
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 89a8b5a976..62c540b4db 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3204,14 +3204,18 @@ static int git_pack_config(const char *k, const char *v,
 		return 0;
 	}
 	if (!strcmp(k, "uploadpack.blobpackfileuri")) {
-		struct configured_exclusion *ex = xmalloc(sizeof(*ex));
+		struct configured_exclusion *ex;
 		const char *oid_end, *pack_end;
 		/*
 		 * Stores the pack hash. This is not a true object ID, but is
 		 * of the same form.
 		 */
 		struct object_id pack_hash;
 
+		if (!v)
+			return config_error_nonbool(k);
+
+		ex = xmalloc(sizeof(*ex));
 		if (parse_oid_hex(v, &ex->e.oid, &oid_end) ||
 		    *oid_end != ' ' ||
 		    parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||
diff --git a/compat/mingw.c b/compat/mingw.c
index ec5280da16..42053c1f65 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -255,6 +255,8 @@ int mingw_core_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "core.unsetenvvars")) {
+		if (!value)
+			return config_error_nonbool(var);
 		free(unset_environment_variables);
 		unset_environment_variables = xstrdup(value);
 		return 0;
diff --git a/config.c b/config.c
index b330c7adb4..18085c7e38 100644
--- a/config.c
+++ b/config.c
@@ -1386,6 +1386,8 @@ static int git_default_core_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "core.checkstat")) {
+		if (!value)
+			return config_error_nonbool(var);
 		if (!strcasecmp(value, "default"))
 			check_stat = 1;
 		else if (!strcasecmp(value, "minimal"))
@@ -1547,11 +1549,15 @@ static int git_default_core_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "core.checkroundtripencoding")) {
+		if (!value)
+			return config_error_nonbool(var);
 		check_roundtrip_encoding = xstrdup(value);
 		return 0;
 	}
 
 	if (!strcmp(var, "core.notesref")) {
+		if (!value)
+			return config_error_nonbool(var);
 		notes_ref_name = xstrdup(value);
 		return 0;
 	}
@@ -1619,6 +1625,8 @@ static int git_default_core_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "core.createobject")) {
+		if (!value)
+			return config_error_nonbool(var);
 		if (!strcmp(value, "rename"))
 			object_creation_mode = OBJECT_CREATION_USES_RENAMES;
 		else if (!strcmp(value, "link"))
diff --git a/diff.c b/diff.c
index 2c602df10a..5b213a4b44 100644
--- a/diff.c
+++ b/diff.c
@@ -372,7 +372,10 @@ int git_diff_ui_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "diff.colormovedws")) {
-		unsigned cm = parse_color_moved_ws(value);
+		unsigned cm;
+		if (!value)
+			return config_error_nonbool(var);
+		cm = parse_color_moved_ws(value);
 		if (cm & COLOR_MOVED_WS_ERROR)
 			return -1;
 		diff_color_moved_ws_default = cm;
@@ -426,10 +429,15 @@ int git_diff_ui_config(const char *var, const char *value,
 	if (!strcmp(var, "diff.orderfile"))
 		return git_config_pathname(&diff_order_file_cfg, var, value);
 
-	if (!strcmp(var, "diff.ignoresubmodules"))
+	if (!strcmp(var, "diff.ignoresubmodules")) {
+		if (!value)
+			return config_error_nonbool(var);
 		handle_ignore_submodules_arg(&default_diff_options, value);
+	}
 
 	if (!strcmp(var, "diff.submodule")) {
+		if (!value)
+			return config_error_nonbool(var);
 		if (parse_submodule_params(&default_diff_options, value))
 			warning(_("Unknown value for 'diff.submodule' config variable: '%s'"),
 				value);
@@ -473,7 +481,10 @@ int git_diff_basic_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "diff.wserrorhighlight")) {
-		int val = parse_ws_error_highlight(value);
+		int val;
+		if (!value)
+			return config_error_nonbool(var);
+		val = parse_ws_error_highlight(value);
 		if (val < 0)
 			return -1;
 		ws_error_highlight_default = val;
@@ -490,6 +501,8 @@ int git_diff_basic_config(const char *var, const char *value,
 
 	if (!strcmp(var, "diff.dirstat")) {
 		struct strbuf errmsg = STRBUF_INIT;
+		if (!value)
+			return config_error_nonbool(var);
 		default_diff_options.dirstat_permille = diff_dirstat_permille_default;
 		if (parse_dirstat_params(&default_diff_options, value, &errmsg))
 			warning(_("Found errors in 'diff.dirstat' config variable:\n%s"),
diff --git a/mailinfo.c b/mailinfo.c
index a07d2da16d..093bed5d8f 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -1253,6 +1253,8 @@ static int git_mailinfo_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "mailinfo.quotedcr")) {
+		if (!value)
+			return config_error_nonbool(var);
 		if (mailinfo_parse_quoted_cr_action(value, &mi->quoted_cr) != 0)
 			return error(_("bad action '%s' for '%s'"), value, var);
 		return 0;
diff --git a/notes-utils.c b/notes-utils.c
index 97c031c26e..01f4f5b424 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -112,6 +112,8 @@ static int notes_rewrite_config(const char *k, const char *v,
 		}
 		return 0;
 	} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
+		if (!v)
+			return config_error_nonbool(k);
 		/* note that a refs/ prefix is implied in the
 		 * underlying for_each_glob_ref */
 		if (starts_with(v, "refs/notes/"))
diff --git a/trailer.c b/trailer.c
index b6de5d9cb2..b0e2ec224a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -507,6 +507,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value,
 				warning(_("unknown value '%s' for key '%s'"),
 					value, conf_key);
 		} else if (!strcmp(trailer_item, "separators")) {
+			if (!value)
+				return config_error_nonbool(conf_key);
 			separators = xstrdup(value);
 		}
 	}
-- 
2.43.0.664.ga12c899002

[PATCH 2/7] setup: handle NULL value when parsing extensions

[Jeff King]

The "partialclone" extension config records a string, and hence it is an
error to have an implicit bool like:

  [extensions]
  partialclone

in your config. We should recognize and reject this, rather than
segfaulting (which is the current behavior). Note that it's OK to use
config_error_nonbool() here, even though the return value is an enum. We
explicitly document EXTENSION_ERROR as -1 for compatibility with
error(), etc.

This is the only extension value that has this problem. Most of the
others are bools that interpret this value naturally. The exception is
extensions.objectformat, which does correctly check for NULL.

Signed-off-by: Jeff King <peff@peff.net>
---
 setup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/setup.c b/setup.c
index fc592dc6dd..50131be7cc 100644
--- a/setup.c
+++ b/setup.c
@@ -559,6 +559,8 @@ static enum extension_result handle_extension_v0(const char *var,
 			data->precious_objects = git_config_bool(var, value);
 			return EXTENSION_OK;
 		} else if (!strcmp(ext, "partialclone")) {
+			if (!value)
+				return config_error_nonbool(var);
 			data->partial_clone = xstrdup(value);
 			return EXTENSION_OK;
 		} else if (!strcmp(ext, "worktreeconfig")) {
-- 
2.43.0.664.ga12c899002

[PATCH 3/7] trace2: handle NULL values in tr2_sysenv config callback

[Jeff King]

If you have config with an implicit bool like:

  [trace2]
  envvars

we'll segfault, as we unconditionally try to xstrdup() the value. We
should instead detect and complain, as a boolean value has no meaning
here. The same is true for every variable in tr2_sysenv_settings (and
this patch covers them all, as we check them in a loop).

Signed-off-by: Jeff King <peff@peff.net>
---
 trace2/tr2_sysenv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
index d3ecac2772..048cdd5438 100644
--- a/trace2/tr2_sysenv.c
+++ b/trace2/tr2_sysenv.c
@@ -68,6 +68,8 @@ static int tr2_sysenv_cb(const char *key, const char *value,
 
 	for (k = 0; k < ARRAY_SIZE(tr2_sysenv_settings); k++) {
 		if (!strcmp(key, tr2_sysenv_settings[k].git_config_name)) {
+			if (!value)
+				return config_error_nonbool(key);
 			free(tr2_sysenv_settings[k].value);
 			tr2_sysenv_settings[k].value = xstrdup(value);
 			return 0;
-- 
2.43.0.664.ga12c899002

[PATCH 4/7] help: handle NULL value for alias.* config

[Jeff King]

When showing all config with "git help --all", we print the list of
defined aliases. But our config callback to do so does not check for a
NULL value, meaning a config block like:

  [alias]
  foo

will cause us to segfault. We should detect and complain about this in
the usual way.

Since this command is purely informational (and we aren't trying to run
the alias), we could perhaps just generate a warning and continue. But
this sort of misconfiguration should be pretty rare, and the error
message we will produce points directly to the line of config that needs
to be fixed. So just generating the usual error should be OK.

Signed-off-by: Jeff King <peff@peff.net>
---
 help.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/help.c b/help.c
index 6d2ebfbd2a..2dbe57b413 100644
--- a/help.c
+++ b/help.c
@@ -464,8 +464,11 @@ static int get_alias(const char *var, const char *value,
 {
 	struct string_list *list = data;
 
-	if (skip_prefix(var, "alias.", &var))
+	if (skip_prefix(var, "alias.", &var)) {
+		if (!value)
+			return config_error_nonbool(var);
 		string_list_append(list, var)->util = xstrdup(value);
+	}
 
 	return 0;
 }
-- 
2.43.0.664.ga12c899002

[PATCH 5/7] submodule: handle NULL value when parsing submodule.*.branch

[Jeff King]

We record the submodule branch config value as a string, so config that
uses an implicit bool like:

  [submodule "foo"]
  branch

will cause us to segfault. Note that unlike most other config-parsing
bugs of this class, this can be triggered by parsing a bogus .gitmodules
file (which we might do after cloning a malicious repository).

I don't think the security implications are important, though. It's
always a strict NULL dereference, not an out-of-bounds read or write. So
we should reliably kill the process. That may be annoying, but the
impact is limited to the attacker preventing the victim from
successfully using "git clone --recurse-submodules", etc, on the
malicious repo.

The "branch" entry is the only one with this problem; other strings like
"path" and "url" already check for NULL.

Signed-off-by: Jeff King <peff@peff.net>
---
 submodule-config.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/submodule-config.c b/submodule-config.c
index 6a48fd12f6..f4dd482abc 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -516,7 +516,9 @@ static int parse_config(const char *var, const char *value,
 			submodule->recommend_shallow =
 				git_config_bool(var, value);
 	} else if (!strcmp(item.buf, "branch")) {
-		if (!me->overwrite && submodule->branch)
+		if (!value)
+			ret = config_error_nonbool(var);
+		else if (!me->overwrite && submodule->branch)
 			warn_multiple_config(me->treeish_name, submodule->name,
 					     "branch");
 		else {
-- 
2.43.0.664.ga12c899002

[PATCH 6/7] trailer: handle NULL value when parsing trailer-specific config

[Jeff King]

When parsing the "key", "command", and "cmd" trailer config, we just
make a copy of the value string.  If we see an implicit bool like:

  [trailer "foo"]
  key

we'll segfault trying to copy a NULL pointer. We can fix this with the
usual config_error_nonbool() check.

I split this out from the other vanilla cases, because at first glance
it looks like a better fix here would be to move the NULL check out of
the switch statement. But it would change the behavior of other keys
like trailer.*.ifExists, where an implicit bool is interpreted as
EXISTS_DEFAULT.

Signed-off-by: Jeff King <peff@peff.net>
---
 trailer.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/trailer.c b/trailer.c
index b0e2ec224a..e4b08ed267 100644
--- a/trailer.c
+++ b/trailer.c
@@ -553,16 +553,22 @@ static int git_trailer_config(const char *conf_key, const char *value,
 	case TRAILER_KEY:
 		if (conf->key)
 			warning(_("more than one %s"), conf_key);
+		if (!value)
+			return config_error_nonbool(conf_key);
 		conf->key = xstrdup(value);
 		break;
 	case TRAILER_COMMAND:
 		if (conf->command)
 			warning(_("more than one %s"), conf_key);
+		if (!value)
+			return config_error_nonbool(conf_key);
 		conf->command = xstrdup(value);
 		break;
 	case TRAILER_CMD:
 		if (conf->cmd)
 			warning(_("more than one %s"), conf_key);
+		if (!value)
+			return config_error_nonbool(conf_key);
 		conf->cmd = xstrdup(value);
 		break;
 	case TRAILER_WHERE:
-- 
2.43.0.664.ga12c899002

[PATCH 7/7] fsck: handle NULL value when parsing message config

[Jeff King]

When parsing fsck.*, receive.fsck.*, or fetch.fsck.*, we don't check for
an implicit bool. So any of:

  [fsck]
  badTree
  [receive "fsck"]
  badTree
  [fetch "fsck"]
  badTree

will cause us to segfault. We can fix it with config_error_nonbool() in
the usual way, but we have to make a few more changes to get good error
messages. The problem is that all three spots do:

  if (skip_prefix(var, "fsck.", &var))

to match and parse the actual message id. But that means that "var" now
just says "badTree" instead of "receive.fsck.badTree", making the
resulting message confusing. We can fix that by storing the parsed
message id in its own separate variable.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/receive-pack.c | 11 +++++++----
 fetch-pack.c           | 12 ++++++++----
 fsck.c                 |  8 ++++++--
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8c4f0cb90a..ccf9738bce 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -142,6 +142,7 @@ static enum deny_action parse_deny_action(const char *var, const char *value)
 static int receive_pack_config(const char *var, const char *value,
 			       const struct config_context *ctx, void *cb)
 {
+	const char *msg_id;
 	int status = parse_hide_refs_config(var, value, "receive", &hidden_refs);
 
 	if (status)
@@ -178,12 +179,14 @@ static int receive_pack_config(const char *var, const char *value,
 		return 0;
 	}
 
-	if (skip_prefix(var, "receive.fsck.", &var)) {
-		if (is_valid_msg_type(var, value))
+	if (skip_prefix(var, "receive.fsck.", &msg_id)) {
+		if (!value)
+			return config_error_nonbool(var);
+		if (is_valid_msg_type(msg_id, value))
 			strbuf_addf(&fsck_msg_types, "%c%s=%s",
-				fsck_msg_types.len ? ',' : '=', var, value);
+				fsck_msg_types.len ? ',' : '=', msg_id, value);
 		else
-			warning("skipping unknown msg id '%s'", var);
+			warning("skipping unknown msg id '%s'", msg_id);
 		return 0;
 	}
 
diff --git a/fetch-pack.c b/fetch-pack.c
index 26999e3b65..31a72d43de 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1862,6 +1862,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 static int fetch_pack_config_cb(const char *var, const char *value,
 				const struct config_context *ctx, void *cb)
 {
+	const char *msg_id;
+
 	if (strcmp(var, "fetch.fsck.skiplist") == 0) {
 		const char *path;
 
@@ -1873,12 +1875,14 @@ static int fetch_pack_config_cb(const char *var, const char *value,
 		return 0;
 	}
 
-	if (skip_prefix(var, "fetch.fsck.", &var)) {
-		if (is_valid_msg_type(var, value))
+	if (skip_prefix(var, "fetch.fsck.", &msg_id)) {
+		if (!value)
+			return config_error_nonbool(var);
+		if (is_valid_msg_type(msg_id, value))
 			strbuf_addf(&fsck_msg_types, "%c%s=%s",
-				fsck_msg_types.len ? ',' : '=', var, value);
+				fsck_msg_types.len ? ',' : '=', msg_id, value);
 		else
-			warning("Skipping unknown msg id '%s'", var);
+			warning("Skipping unknown msg id '%s'", msg_id);
 		return 0;
 	}
 
diff --git a/fsck.c b/fsck.c
index 6a0bbc5087..b624083a13 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1403,6 +1403,8 @@ int git_fsck_config(const char *var, const char *value,
 		    const struct config_context *ctx, void *cb)
 {
 	struct fsck_options *options = cb;
+	const char *msg_id;
+
 	if (strcmp(var, "fsck.skiplist") == 0) {
 		const char *path;
 		struct strbuf sb = STRBUF_INIT;
@@ -1416,8 +1418,10 @@ int git_fsck_config(const char *var, const char *value,
 		return 0;
 	}
 
-	if (skip_prefix(var, "fsck.", &var)) {
-		fsck_set_msg_type(options, var, value);
+	if (skip_prefix(var, "fsck.", &msg_id)) {
+		if (!value)
+			return config_error_nonbool(var);
+		fsck_set_msg_type(options, msg_id, value);
 		return 0;
 	}
 
-- 
2.43.0.664.ga12c899002

Re: [PATCH 0/7] fix segfaults with implicit-bool config

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

On Thu, Dec 07, 2023 at 02:10:30AM -0500, Jeff King wrote:
> Carlos reported to the security list a case where you can cause Git
> to segfault by using an implicit bool like:
> 
>   [core]
>   someVariable
> 
> when the parsing side for core.someVariable does not correctly check a
> NULL "value" string. This is mostly harmless, as anybody who can feed
> arbitrary config can already execute arbitrary code. There is one case
> of this when parsing .gitmodules (which we don't trust), but even there
> I don't think the security implications are that interesting. A
> malicious repo can get "clone --recurse-submodules" to segfault, but
> always with a strict NULL dereference, not any kind of controllable
> pointer. See patch 5 for more details.
> 
> I audited the whole code base for instances of the problem. It was
> fairly manual, so it's possible I missed a spot, but I think this should
> cover everything.
> 
> The first patch has vanilla cases, and the rest are instances where I
> thought it was worth calling out specific details.

Thanks for working on this topic! I've left a couple of comments, most
of which are about whether we should retain previous behaviour where we
generate a warning instead of raising an error for unknown values.

Patrick

>   [1/7]: config: handle NULL value when parsing non-bools
>   [2/7]: setup: handle NULL value when parsing extensions
>   [3/7]: trace2: handle NULL values in tr2_sysenv config callback
>   [4/7]: help: handle NULL value for alias.* config
>   [5/7]: submodule: handle NULL value when parsing submodule.*.branch
>   [6/7]: trailer: handle NULL value when parsing trailer-specific config
>   [7/7]: fsck: handle NULL value when parsing message config
> 
>  builtin/blame.c        |  2 ++
>  builtin/checkout.c     |  2 ++
>  builtin/clone.c        |  2 ++
>  builtin/log.c          |  5 ++++-
>  builtin/pack-objects.c |  6 +++++-
>  builtin/receive-pack.c | 11 +++++++----
>  compat/mingw.c         |  2 ++
>  config.c               |  8 ++++++++
>  diff.c                 | 19 ++++++++++++++++---
>  fetch-pack.c           | 12 ++++++++----
>  fsck.c                 |  8 ++++++--
>  help.c                 |  5 ++++-
>  mailinfo.c             |  2 ++
>  notes-utils.c          |  2 ++
>  setup.c                |  2 ++
>  submodule-config.c     |  4 +++-
>  trace2/tr2_sysenv.c    |  2 ++
>  trailer.c              |  8 ++++++++
>  18 files changed, 85 insertions(+), 17 deletions(-)
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/7] config: handle NULL value when parsing non-bools

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 7461 bytes --]

On Thu, Dec 07, 2023 at 02:11:14AM -0500, Jeff King wrote:
> When the config parser sees an "implicit" bool like:
> 
>   [core]
>   someVariable
> 
> it passes NULL to the config callback. Any callback code which expects a
> string must check for NULL. This usually happens via helpers like
> git_config_string(), etc, but some custom code forgets to do so and will
> segfault.
> 
> These are all fairly vanilla cases where the solution is just the usual
> pattern of:
> 
>   if (!value)
>         return config_error_nonbool(var);
> 
> though note that in a few cases we have to split initializers like:
> 
>   int some_var = initializer();
> 
> into:
> 
>   int some_var;
>   if (!value)
>         return config_error_nonbool(var);
>   some_var = initializer();
> 
> There are still some broken instances after this patch, which I'll
> address on their own in individual patches after this one.
> 
> Reported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/blame.c        |  2 ++
>  builtin/checkout.c     |  2 ++
>  builtin/clone.c        |  2 ++
>  builtin/log.c          |  5 ++++-
>  builtin/pack-objects.c |  6 +++++-
>  compat/mingw.c         |  2 ++
>  config.c               |  8 ++++++++
>  diff.c                 | 19 ++++++++++++++++---
>  mailinfo.c             |  2 ++
>  notes-utils.c          |  2 ++
>  trailer.c              |  2 ++
>  11 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 9c987d6567..2433b7da5c 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -748,6 +748,8 @@ static int git_blame_config(const char *var, const char *value,
>  	}
>  
>  	if (!strcmp(var, "blame.coloring")) {
> +		if (!value)
> +			return config_error_nonbool(var);

In the `else` statement where we fail to parse the value we only
generate a warning and return successfully regardless. Should we do the
same here?

>  		if (!strcmp(value, "repeatedLines")) {
>  			coloring_mode |= OUTPUT_COLOR_LINE;
>  		} else if (!strcmp(value, "highlightRecent")) {
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index f02434bc15..d5c784854f 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1202,6 +1202,8 @@ static int git_checkout_config(const char *var, const char *value,
>  	struct checkout_opts *opts = cb;
>  
>  	if (!strcmp(var, "diff.ignoresubmodules")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		handle_ignore_submodules_arg(&opts->diff_options, value);
>  		return 0;
>  	}

This one is fine, `handle_ignore_submodules_arg()` dies if it gets an
unknown value and `git_config()` will die when the callback function
returns an error. The same is true for many other cases you've
converted.

[snip]
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 89a8b5a976..62c540b4db 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3204,14 +3204,18 @@ static int git_pack_config(const char *k, const char *v,
>  		return 0;
>  	}
>  	if (!strcmp(k, "uploadpack.blobpackfileuri")) {
> -		struct configured_exclusion *ex = xmalloc(sizeof(*ex));
> +		struct configured_exclusion *ex;
>  		const char *oid_end, *pack_end;
>  		/*
>  		 * Stores the pack hash. This is not a true object ID, but is
>  		 * of the same form.
>  		 */
>  		struct object_id pack_hash;
>  
> +		if (!v)
> +			return config_error_nonbool(k);
> +
> +		ex = xmalloc(sizeof(*ex));
>  		if (parse_oid_hex(v, &ex->e.oid, &oid_end) ||
>  		    *oid_end != ' ' ||
>  		    parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||

This isn't part of the diff and not a new issue, but why don't we
`return 0` when parsing this config correctly? We fall through to
`git_default_config()` even if we've successfully parsed the config key,
which seems like a bug to me.

Anyway, this case looks fine.

[snip]
> diff --git a/config.c b/config.c
> index b330c7adb4..18085c7e38 100644
> --- a/config.c
> +++ b/config.c
> @@ -1386,6 +1386,8 @@ static int git_default_core_config(const char *var, const char *value,
>  		return 0;
>  	}
>  	if (!strcmp(var, "core.checkstat")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		if (!strcasecmp(value, "default"))
>  			check_stat = 1;
>  		else if (!strcasecmp(value, "minimal"))

We would ignore `true` here, so should we ignore implicit `true`, as
well?

> @@ -1547,11 +1549,15 @@ static int git_default_core_config(const char *var, const char *value,
>  	}
>  
>  	if (!strcmp(var, "core.checkroundtripencoding")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		check_roundtrip_encoding = xstrdup(value);
>  		return 0;
>  	}
>  
>  	if (!strcmp(var, "core.notesref")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		notes_ref_name = xstrdup(value);
>  		return 0;
>  	}

I wonder the same here. We might as well use `xstrdup_or_null()`, but it
feels like the right thing to do to convert these to actual errors.

> @@ -426,10 +429,15 @@ int git_diff_ui_config(const char *var, const char *value,
>  	if (!strcmp(var, "diff.orderfile"))
>  		return git_config_pathname(&diff_order_file_cfg, var, value);
>  
> -	if (!strcmp(var, "diff.ignoresubmodules"))
> +	if (!strcmp(var, "diff.ignoresubmodules")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		handle_ignore_submodules_arg(&default_diff_options, value);
> +	}
>  
>  	if (!strcmp(var, "diff.submodule")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		if (parse_submodule_params(&default_diff_options, value))
>  			warning(_("Unknown value for 'diff.submodule' config variable: '%s'"),
>  				value);

Should we generate a warning instead according to the preexisting code
for "diff.submodule"?

> @@ -490,6 +501,8 @@ int git_diff_basic_config(const char *var, const char *value,
>  
>  	if (!strcmp(var, "diff.dirstat")) {
>  		struct strbuf errmsg = STRBUF_INIT;
> +		if (!value)
> +			return config_error_nonbool(var);
>  		default_diff_options.dirstat_permille = diff_dirstat_permille_default;
>  		if (parse_dirstat_params(&default_diff_options, value, &errmsg))
>  			warning(_("Found errors in 'diff.dirstat' config variable:\n%s"),

Same here, should we generate a warning instead?

> diff --git a/notes-utils.c b/notes-utils.c
> index 97c031c26e..01f4f5b424 100644
> --- a/notes-utils.c
> +++ b/notes-utils.c
> @@ -112,6 +112,8 @@ static int notes_rewrite_config(const char *k, const char *v,
>  		}
>  		return 0;
>  	} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
> +		if (!v)
> +			return config_error_nonbool(k);
>  		/* note that a refs/ prefix is implied in the
>  		 * underlying for_each_glob_ref */
>  		if (starts_with(v, "refs/notes/"))

Here, as well.

> diff --git a/trailer.c b/trailer.c
> index b6de5d9cb2..b0e2ec224a 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -507,6 +507,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value,
>  				warning(_("unknown value '%s' for key '%s'"),
>  					value, conf_key);
>  		} else if (!strcmp(trailer_item, "separators")) {
> +			if (!value)
> +				return config_error_nonbool(conf_key);
>  			separators = xstrdup(value);
>  		}
>  	}

And here.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/7] submodule: handle NULL value when parsing submodule.*.branch

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

On Thu, Dec 07, 2023 at 02:11:29AM -0500, Jeff King wrote:
> We record the submodule branch config value as a string, so config that
> uses an implicit bool like:
> 
>   [submodule "foo"]
>   branch
> 
> will cause us to segfault. Note that unlike most other config-parsing
> bugs of this class, this can be triggered by parsing a bogus .gitmodules
> file (which we might do after cloning a malicious repository).
> 
> I don't think the security implications are important, though. It's
> always a strict NULL dereference, not an out-of-bounds read or write. So
> we should reliably kill the process. That may be annoying, but the
> impact is limited to the attacker preventing the victim from
> successfully using "git clone --recurse-submodules", etc, on the
> malicious repo.
> 
> The "branch" entry is the only one with this problem; other strings like
> "path" and "url" already check for NULL.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  submodule-config.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/submodule-config.c b/submodule-config.c
> index 6a48fd12f6..f4dd482abc 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -516,7 +516,9 @@ static int parse_config(const char *var, const char *value,
>  			submodule->recommend_shallow =
>  				git_config_bool(var, value);
>  	} else if (!strcmp(item.buf, "branch")) {
> -		if (!me->overwrite && submodule->branch)
> +		if (!value)
> +			ret = config_error_nonbool(var);
> +		else if (!me->overwrite && submodule->branch)
>  			warn_multiple_config(me->treeish_name, submodule->name,
>  					     "branch");
>  		else {

I was about to say that I'd rather expect us to handle this gracefully
so that Git doesn't die when parsing an invalid gitmodules file. But
there are other cases where we already fail in the same way, so this
looks good to me.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/7] trailer: handle NULL value when parsing trailer-specific config

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On Thu, Dec 07, 2023 at 02:11:32AM -0500, Jeff King wrote:
> When parsing the "key", "command", and "cmd" trailer config, we just
> make a copy of the value string.  If we see an implicit bool like:
> 
>   [trailer "foo"]
>   key
> 
> we'll segfault trying to copy a NULL pointer. We can fix this with the
> usual config_error_nonbool() check.
> 
> I split this out from the other vanilla cases, because at first glance
> it looks like a better fix here would be to move the NULL check out of
> the switch statement. But it would change the behavior of other keys
> like trailer.*.ifExists, where an implicit bool is interpreted as
> EXISTS_DEFAULT.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  trailer.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/trailer.c b/trailer.c
> index b0e2ec224a..e4b08ed267 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -553,16 +553,22 @@ static int git_trailer_config(const char *conf_key, const char *value,
>  	case TRAILER_KEY:
>  		if (conf->key)
>  			warning(_("more than one %s"), conf_key);
> +		if (!value)
> +			return config_error_nonbool(conf_key);
>  		conf->key = xstrdup(value);
>  		break;
>  	case TRAILER_COMMAND:
>  		if (conf->command)
>  			warning(_("more than one %s"), conf_key);
> +		if (!value)
> +			return config_error_nonbool(conf_key);
>  		conf->command = xstrdup(value);
>  		break;
>  	case TRAILER_CMD:
>  		if (conf->cmd)
>  			warning(_("more than one %s"), conf_key);
> +		if (!value)
> +			return config_error_nonbool(conf_key);
>  		conf->cmd = xstrdup(value);
>  		break;
>  	case TRAILER_WHERE:

For the other cases we only generate warnings for unknown values, but
return successfully. Should we do the same here?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 7/7] fsck: handle NULL value when parsing message config

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On Thu, Dec 07, 2023 at 02:11:35AM -0500, Jeff King wrote:
> When parsing fsck.*, receive.fsck.*, or fetch.fsck.*, we don't check for
> an implicit bool. So any of:
> 
>   [fsck]
>   badTree
>   [receive "fsck"]
>   badTree
>   [fetch "fsck"]
>   badTree
> 
> will cause us to segfault. We can fix it with config_error_nonbool() in
> the usual way, but we have to make a few more changes to get good error
> messages. The problem is that all three spots do:
> 
>   if (skip_prefix(var, "fsck.", &var))
> 
> to match and parse the actual message id. But that means that "var" now
> just says "badTree" instead of "receive.fsck.badTree", making the
> resulting message confusing. We can fix that by storing the parsed
> message id in its own separate variable.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/receive-pack.c | 11 +++++++----
>  fetch-pack.c           | 12 ++++++++----
>  fsck.c                 |  8 ++++++--
>  3 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 8c4f0cb90a..ccf9738bce 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -142,6 +142,7 @@ static enum deny_action parse_deny_action(const char *var, const char *value)
>  static int receive_pack_config(const char *var, const char *value,
>  			       const struct config_context *ctx, void *cb)
>  {
> +	const char *msg_id;
>  	int status = parse_hide_refs_config(var, value, "receive", &hidden_refs);
>  
>  	if (status)
> @@ -178,12 +179,14 @@ static int receive_pack_config(const char *var, const char *value,
>  		return 0;
>  	}
>  
> -	if (skip_prefix(var, "receive.fsck.", &var)) {
> -		if (is_valid_msg_type(var, value))
> +	if (skip_prefix(var, "receive.fsck.", &msg_id)) {
> +		if (!value)
> +			return config_error_nonbool(var);
> +		if (is_valid_msg_type(msg_id, value))
>  			strbuf_addf(&fsck_msg_types, "%c%s=%s",
> -				fsck_msg_types.len ? ',' : '=', var, value);
> +				fsck_msg_types.len ? ',' : '=', msg_id, value);
>  		else
> -			warning("skipping unknown msg id '%s'", var);
> +			warning("skipping unknown msg id '%s'", msg_id);
>  		return 0;
>  	}

Same remark here. We would only generate warnings for an actual boolean,
so I'd think we might consider doing the same for implicit booleans.

Partick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/7] submodule: handle NULL value when parsing submodule.*.branch

[Jeff King]

On Thu, Dec 07, 2023 at 09:14:48AM +0100, Patrick Steinhardt wrote:

> > diff --git a/submodule-config.c b/submodule-config.c
> > index 6a48fd12f6..f4dd482abc 100644
> > --- a/submodule-config.c
> > +++ b/submodule-config.c
> > @@ -516,7 +516,9 @@ static int parse_config(const char *var, const char *value,
> >  			submodule->recommend_shallow =
> >  				git_config_bool(var, value);
> >  	} else if (!strcmp(item.buf, "branch")) {
> > -		if (!me->overwrite && submodule->branch)
> > +		if (!value)
> > +			ret = config_error_nonbool(var);
> > +		else if (!me->overwrite && submodule->branch)
> >  			warn_multiple_config(me->treeish_name, submodule->name,
> >  					     "branch");
> >  		else {
> 
> I was about to say that I'd rather expect us to handle this gracefully
> so that Git doesn't die when parsing an invalid gitmodules file. But
> there are other cases where we already fail in the same way, so this
> looks good to me.

We're just returning the error here, so it's really up to the caller to
decide what to do. The config API has an "error_action" field in the
options struct. By default for reading from a blob, this will propagate
the error, and I think that's what we use in most of the submodule code.

For the code in fsck which looks at gitmodules, it suppresses the error
text from the config API (in favor of its own fsck-specific message).
Of course it does not suppress the error() from config_error_nonbool,
which writes straight to stderr. So there may be some room for
improvement, but I doubt anybody cares too much in practice if fsck is a
little chatty when it sees breakage.

-Peff

Re: [PATCH 0/7] fix segfaults with implicit-bool config

[Jeff King]

On Thu, Dec 07, 2023 at 09:14:36AM +0100, Patrick Steinhardt wrote:

> Thanks for working on this topic! I've left a couple of comments, most
> of which are about whether we should retain previous behaviour where we
> generate a warning instead of raising an error for unknown values.

Thanks for taking a look. I see what you're saying about the warnings,
but IMHO it's not worth the extra complexity. Returning early means the
existing code can proceed without worrying about NULLs. Though I suppose
we could have a "warn_error_nonbool()" which issues a warning and
returns 0.

Still, the "return config_error_nonbool()" pattern is pretty
well-established and used for most options. I would go so far as to say
the ones that warn for invalid values are the odd ones out, and probably
should be returning errors as well (though doing so now may not be worth
the trouble and risk of annoyance).

And certainly there should be no regressions in this series; every case
is currently a segfault, so returning an error is a strict improvement.
I'd just as soon stay strict there, as it's easier to loosen later if we
choose than to tighten.

-Peff

Re: [PATCH 1/7] config: handle NULL value when parsing non-bools

[Jeff King]

On Thu, Dec 07, 2023 at 09:14:42AM +0100, Patrick Steinhardt wrote:

> >  	if (!strcmp(k, "uploadpack.blobpackfileuri")) {
> [...]
> This isn't part of the diff and not a new issue, but why don't we
> `return 0` when parsing this config correctly? We fall through to
> `git_default_config()` even if we've successfully parsed the config key,
> which seems like a bug to me.

I don't think it's a functional bug, but merely a pessimization. We can
return early if we know we've handled the option, but the rest of the
code would simply fail to match it. So we are just wasting a few strcmp
calls (and an unknown key already wastes the same number).

So I think it is a good practice to return, but not really a bug if we
don't.

> >  	if (!strcmp(var, "core.checkstat")) {
> > +		if (!value)
> > +			return config_error_nonbool(var);
> >  		if (!strcasecmp(value, "default"))
> >  			check_stat = 1;
> >  		else if (!strcasecmp(value, "minimal"))
> 
> We would ignore `true` here, so should we ignore implicit `true`, as
> well?

IMHO the lack of a final "else" in the strcasecmp if-cascade is a bug
(and I sent a fix as part of the "config fixes on top" series). Even if
we want to leave it for historical reasons, I think it's still worth
returning an error for the NULL case (since we know it would have
segfaulted previously).

(I snipped the rest of your mail, as I think my response to the cover
letter covers the general discussion).

-Peff

Re: [PATCH 0/7] fix segfaults with implicit-bool config

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1338 bytes --]

On Mon, Dec 11, 2023 at 07:52:28PM -0500, Jeff King wrote:
> On Thu, Dec 07, 2023 at 09:14:36AM +0100, Patrick Steinhardt wrote:
> 
> > Thanks for working on this topic! I've left a couple of comments, most
> > of which are about whether we should retain previous behaviour where we
> > generate a warning instead of raising an error for unknown values.
> 
> Thanks for taking a look. I see what you're saying about the warnings,
> but IMHO it's not worth the extra complexity. Returning early means the
> existing code can proceed without worrying about NULLs. Though I suppose
> we could have a "warn_error_nonbool()" which issues a warning and
> returns 0.
> 
> Still, the "return config_error_nonbool()" pattern is pretty
> well-established and used for most options. I would go so far as to say
> the ones that warn for invalid values are the odd ones out, and probably
> should be returning errors as well (though doing so now may not be worth
> the trouble and risk of annoyance).
> 
> And certainly there should be no regressions in this series; every case
> is currently a segfault, so returning an error is a strict improvement.
> I'd just as soon stay strict there, as it's easier to loosen later if we
> choose than to tighten.

Fair enough, I'm perfectly fine with this reasoning. Thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]