git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Dec 7, 2023, 07:11 UTC
Message-ID
<20231207071129.GE1276005@coredump.intra.peff.net>
In-Reply-To
<20231207071030.GA1275835@coredump.intra.peff.net>

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
Previous: Jeff KingNext: Patrick Steinhardt
Message 8 of 17 in “fix segfaults with implicit-bool config”
  1. 0/7 fix segfaults with implicit-bool configJeff King, Dec 7, 2023
  2. 1/7 config: handle NULL value when parsing non-boolsJeff King, Dec 7, 2023
  3. Patrick SteinhardtDec 7, 2023
  4. Jeff KingDec 12, 2023
  5. 2/7 setup: handle NULL value when parsing extensionsJeff King, Dec 7, 2023
  6. 3/7 trace2: handle NULL values in tr2_sysenv config callbackJeff King, Dec 7, 2023
  7. 4/7 help: handle NULL value for alias.* configJeff King, Dec 7, 2023
  8. 5/7 submodule: handle NULL value when parsing submodule.*.branchJeff King, Dec 7, 2023
  9. Patrick SteinhardtDec 7, 2023
  10. Jeff KingDec 12, 2023
  11. 6/7 trailer: handle NULL value when parsing trailer-specific configJeff King, Dec 7, 2023
  12. Patrick SteinhardtDec 7, 2023
  13. 7/7 fsck: handle NULL value when parsing message configJeff King, Dec 7, 2023
  14. Patrick SteinhardtDec 7, 2023
  15. Patrick SteinhardtDec 7, 2023
  16. Jeff KingDec 12, 2023
  17. Patrick SteinhardtDec 12, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.