From ba176db511b3438738a4aeb98e574310e697ff5f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Dec 2023 02:11:14 -0500 Subject: config: handle NULL value when parsing non-bools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- 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,7 +3204,7 @@ 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 @@ -3212,6 +3212,10 @@ static int git_pack_config(const char *k, const char *v, */ 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); } } -- cgit 1.2.3-korg From a62712696e76eddebf9b5e0ecf2c9040558584bb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Dec 2023 02:11:21 -0500 Subject: setup: handle NULL value when parsing extensions 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 Signed-off-by: Junio C Hamano --- 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")) { -- cgit 1.2.3-korg From 24942ef31636f5e1f8076454200be76db8740848 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Dec 2023 02:11:24 -0500 Subject: trace2: handle NULL values in tr2_sysenv config callback 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 Signed-off-by: Junio C Hamano --- 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; -- cgit 1.2.3-korg From 89086c9466624ca0b53f04374e293a7afcec592b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Dec 2023 02:11:27 -0500 Subject: help: handle NULL value for alias.* config 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 Signed-off-by: Junio C Hamano --- 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; } -- cgit 1.2.3-korg From 34b1a0d320e3a1531997d6681bacffbe9da7a046 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Dec 2023 02:11:29 -0500 Subject: submodule: handle NULL value when parsing submodule.*.branch 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 Signed-off-by: Junio C Hamano --- 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 { -- cgit 1.2.3-korg From 1b274c98341ef17f3bbfe80f603f629e7c950668 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Dec 2023 02:11:32 -0500 Subject: trailer: handle NULL value when parsing trailer-specific config 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 Signed-off-by: Junio C Hamano --- 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: -- cgit 1.2.3-korg From d49cb162fa752d62cf20548ae057471d348e42ae Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Dec 2023 02:11:35 -0500 Subject: fsck: handle NULL value when parsing message config 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 Signed-off-by: Junio C Hamano --- 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; } -- cgit 1.2.3-korg