From: Jeff King <peff@peff.net> To: git@vger.kernel.org Subject: [PATCH 6/6] config: reject parsing of files over INT_MAX Date: Fri, 10 Apr 2020 15:50:07 -0400 [thread overview] Message-ID: <20200410195007.GF1363756@coredump.intra.peff.net> (raw) In-Reply-To: <20200410194211.GA1363484@coredump.intra.peff.net> While the last few commits have made it possible for the config parser to handle config files up to the limits of size_t, the rest of the code isn't really ready for this. In particular, we often feed the keys as strings into printf "%s" format specifiers. And because the printf family of functions must return an int to specify the result, they complain. Here are two concrete examples (using glibc; we're in uncharted territory here so results may vary): Generate a gigantic .gitmodules file like this: git submodule add /some/other/repo foo { printf '[submodule "' perl -e 'print "a" x 2**31' echo '"]path = foo' } >.gitmodules git commit -m 'huge gitmodule' then try this: $ git show BUG: strbuf.c:397: your vsnprintf is broken (returned -1) The problem is that we end up calling: strbuf_addf(&sb, "submodule.%s.ignore", submodule_name); which relies on vsnprintf(), and that function has no way to report back a size larger than INT_MAX. Taking that same file, try this: git config --file=.gitmodules --list --name-only On my system it produces an output with exactly 4GB of spaces. I confirmed in a debugger that we reach the config callback with the key intact: it's 2147483663 bytes and full of a's. But when we print it with this call: printf("%s%c", key_, term); we just get the spaces. So given the fact that these are insane cases which we have no need to support, the weird behavior from feeding the results to printf even if the code is careful, and the possibility of uncareful code introducing its own integer truncation issues, let's just declare INT_MAX as a limit for parsing config files. We'll enforce the limit in get_next_char(), which generalizes over all sources (blobs, files, etc) and covers any element we're parsing (whether section, key, value, etc). For simplicity, the limit is over the length of the _whole_ file, so you couldn't have two 1GB values in the same file. This should be perfectly fine, as the expected size for config files is generally kilobytes at most. With this patch both cases above will yield: fatal: bad config line 1 in file .gitmodules That's not an amazing error message, but the parser isn't set up to provide specific messages (it just breaks out of the parsing loop and gives that generic error even if see a syntactic issue). And we really wouldn't expect to see this case outside of somebody maliciously probing the limits of the config system. Signed-off-by: Jeff King <peff@peff.net> --- config.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/config.c b/config.c index 1c25c94863..8db9c77098 100644 --- a/config.c +++ b/config.c @@ -37,6 +37,7 @@ struct config_source { enum config_error_action default_error_action; int linenr; int eof; + size_t total_len; struct strbuf value; struct strbuf var; unsigned subsection_case_sensitive : 1; @@ -524,6 +525,19 @@ static int get_next_char(void) c = '\r'; } } + + if (c != EOF && ++cf->total_len > INT_MAX) { + /* + * This is an absurdly long config file; refuse to parse + * further in order to protect downstream code from integer + * overflows. Note that we can't return an error specifically, + * but we can mark EOF and put trash in the return value, + * which will trigger a parse error. + */ + cf->eof = 1; + return 0; + } + if (c == '\n') cf->linenr++; if (c == EOF) { @@ -1540,6 +1554,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn, void *data, top->prev = cf; top->linenr = 1; top->eof = 0; + top->total_len = 0; strbuf_init(&top->value, 1024); strbuf_init(&top->var, 1024); cf = top; -- 2.26.0.414.ge3a6455e3d
next prev parent reply other threads:[~2020-04-10 19:50 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-10 19:42 [PATCH 0/6] better handling of gigantic config files Jeff King 2020-04-10 19:43 ` [PATCH 1/6] remote: drop auto-strlen behavior of make_branch() and make_rewrite() Jeff King 2020-04-10 21:44 ` Junio C Hamano 2020-04-10 19:44 ` [PATCH 2/6] parse_config_key(): return subsection len as size_t Jeff King 2020-04-10 19:44 ` [PATCH 3/6] config: drop useless length variable in write_pair() Jeff King 2020-04-10 21:51 ` Junio C Hamano 2020-04-10 19:46 ` [PATCH 4/6] git_config_parse_key(): return baselen as size_t Jeff King 2020-04-10 19:47 ` [PATCH 5/6] config: use size_t to store parsed variable baselen Jeff King 2020-04-10 19:50 ` Jeff King [this message] 2020-04-10 22:04 ` [PATCH 6/6] config: reject parsing of files over INT_MAX Junio C Hamano 2020-04-10 22:15 ` Jeff King 2020-04-13 0:47 ` Taylor Blau 2020-04-13 22:14 ` Junio C Hamano 2020-04-13 0:49 ` [PATCH 0/6] better handling of gigantic config files Taylor Blau 2020-04-13 17:20 ` Jeff King 2020-04-13 17:23 ` Taylor Blau
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200410195007.GF1363756@coredump.intra.peff.net \ --to=peff@peff.net \ --cc=git@vger.kernel.org \ --subject='Re: [PATCH 6/6] config: reject parsing of files over INT_MAX' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).