From a5bb10fd5e74101e7c07da93e7c32bbe60f6173a Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 6 Apr 2023 14:07:58 -0400 Subject: config: avoid fixed-sized buffer when renaming/deleting a section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When renaming (or deleting) a section of configuration, Git uses the function `git_config_copy_or_rename_section_in_file()` to rewrite the configuration file after applying the rename or deletion to the given section. To do this, Git repeatedly calls `fgets()` to read the existing configuration data into a fixed size buffer. When the configuration value under `old_name` exceeds the size of the buffer, we will call `fgets()` an additional time even if there is no newline in the configuration file, since our read length is capped at `sizeof(buf)`. If the first character of the buffer (after zero or more characters satisfying `isspace()`) is a '[', Git will incorrectly treat it as beginning a new section when the original section is being removed. In other words, a configuration value satisfying this criteria can incorrectly be considered as a new secftion instead of a variable in the original section. Avoid this issue by using a variable-width buffer in the form of a strbuf rather than a fixed-with region on the stack. A couple of small points worth noting: - Using a strbuf will cause us to allocate arbitrary sizes to match the length of each line. In practice, we don't expect any reasonable configuration files to have lines that long, and a bandaid will be introduced in a later patch to ensure that this is the case. - We are using strbuf_getwholeline() here instead of strbuf_getline() in order to match `fgets()`'s behavior of leaving the trailing LF character on the buffer (as well as a trailing NUL). This could be changed later, but using strbuf_getwholeline() changes the least about this function's implementation, so it is picked as the safest path. - It is temping to want to replace the loop to skip over characters matching isspace() at the beginning of the buffer with a convenience function like `strbuf_ltrim()`. But this is the wrong approach for a couple of reasons: First, it involves a potentially large and expensive `memmove()` which we would like to avoid. Second, and more importantly, we also *do* want to preserve those spaces to avoid changing the output of other sections. In all, this patch is a minimal replacement of the fixed-width buffer in `git_config_copy_or_rename_section_in_file()` to instead use a `struct strbuf`. Reported-by: André Baptista Reported-by: Vítor Pinho Helped-by: Patrick Steinhardt Co-authored-by: Johannes Schindelin Signed-off-by: Johannes Schindelin Signed-off-by: Taylor Blau --- config.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'config.c') diff --git a/config.c b/config.c index 1137bd73af..524347676d 100644 --- a/config.c +++ b/config.c @@ -3091,7 +3091,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename char *filename_buf = NULL; struct lock_file lock = LOCK_INIT; int out_fd; - char buf[1024]; + struct strbuf buf = STRBUF_INIT; FILE *config_file = NULL; struct stat st; struct strbuf copystr = STRBUF_INIT; @@ -3132,14 +3132,14 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename goto out; } - while (fgets(buf, sizeof(buf), config_file)) { + while (!strbuf_getwholeline(&buf, config_file, '\n')) { unsigned i; int length; int is_section = 0; - char *output = buf; - for (i = 0; buf[i] && isspace(buf[i]); i++) + char *output = buf.buf; + for (i = 0; buf.buf[i] && isspace(buf.buf[i]); i++) ; /* do nothing */ - if (buf[i] == '[') { + if (buf.buf[i] == '[') { /* it's a section */ int offset; is_section = 1; @@ -3158,7 +3158,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename strbuf_reset(©str); } - offset = section_name_match(&buf[i], old_name); + offset = section_name_match(&buf.buf[i], old_name); if (offset > 0) { ret++; if (new_name == NULL) { @@ -3233,6 +3233,7 @@ out: out_no_rollback: free(filename_buf); config_store_data_clear(&store); + strbuf_release(&buf); return ret; } -- cgit v1.2.3 From e91cfe6085c4a61372d1f800b473b73b8d225d0d Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 6 Apr 2023 14:28:53 -0400 Subject: config.c: avoid integer truncation in `copy_or_rename_section_in_file()` There are a couple of spots within `copy_or_rename_section_in_file()` that incorrectly use an `int` to track an offset within a string, which may truncate or wrap around to a negative value. Historically it was impossible to have a line longer than 1024 bytes anyway, since we used fgets() with a fixed-size buffer of exactly that length. But the recent change to use a strbuf permits us to read lines of arbitrary length, so it's possible for a malicious input to cause us to overflow past INT_MAX and do an out-of-bounds array read. Practically speaking, however, this should never happen, since it requires 2GB section names or values, which are unrealistic in non-malicious circumstances. Co-authored-by: Jeff King Signed-off-by: Jeff King Signed-off-by: Taylor Blau --- config.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'config.c') diff --git a/config.c b/config.c index 524347676d..e4189aa2d7 100644 --- a/config.c +++ b/config.c @@ -3027,9 +3027,10 @@ void git_config_set_multivar(const char *key, const char *value, flags); } -static int section_name_match (const char *buf, const char *name) +static size_t section_name_match (const char *buf, const char *name) { - int i = 0, j = 0, dot = 0; + size_t i = 0, j = 0; + int dot = 0; if (buf[i] != '[') return 0; for (i = 1; buf[i] && buf[i] != ']'; i++) { @@ -3133,15 +3134,14 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename } while (!strbuf_getwholeline(&buf, config_file, '\n')) { - unsigned i; - int length; + size_t i, length; int is_section = 0; char *output = buf.buf; for (i = 0; buf.buf[i] && isspace(buf.buf[i]); i++) ; /* do nothing */ if (buf.buf[i] == '[') { /* it's a section */ - int offset; + size_t offset; is_section = 1; /* -- cgit v1.2.3 From 3bb3d6bac5f2b496dfa2862dc1a84cbfa9b4449a Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 12 Apr 2023 19:18:28 -0400 Subject: config.c: disallow overly-long lines in `copy_or_rename_section_in_file()` As a defense-in-depth measure to guard against any potentially-unknown buffer overflows in `copy_or_rename_section_in_file()`, refuse to work with overly-long lines in a gitconfig. Signed-off-by: Taylor Blau Signed-off-by: Johannes Schindelin --- config.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'config.c') diff --git a/config.c b/config.c index e4189aa2d7..b8194dfd8a 100644 --- a/config.c +++ b/config.c @@ -3083,6 +3083,8 @@ static int section_name_is_ok(const char *name) return 1; } +#define GIT_CONFIG_MAX_LINE_LEN (512 * 1024) + /* if new_name == NULL, the section is removed instead */ static int git_config_copy_or_rename_section_in_file(const char *config_filename, const char *old_name, @@ -3097,6 +3099,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename struct stat st; struct strbuf copystr = STRBUF_INIT; struct config_store_data store; + uint32_t line_nr = 0; memset(&store, 0, sizeof(store)); @@ -3137,6 +3140,16 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename size_t i, length; int is_section = 0; char *output = buf.buf; + + line_nr++; + + if (buf.len >= GIT_CONFIG_MAX_LINE_LEN) { + ret = error(_("refusing to work with overly long line " + "in '%s' on line %"PRIuMAX), + config_filename, (uintmax_t)line_nr); + goto out; + } + for (i = 0; buf.buf[i] && isspace(buf.buf[i]); i++) ; /* do nothing */ if (buf.buf[i] == '[') { -- cgit v1.2.3