File CVE-2023-29007-1.patch of Package git

Overview Repositories Revisions Requests Users Attributes Meta

File CVE-2023-29007-1.patch of Package git

commit a5bb10fd5e74101e7c07da93e7c32bbe60f6173a
Author: Taylor Blau <me@ttaylorr.com>
Date:   Thu Apr 6 14:07:58 2023 -0400

config: avoid fixed-sized buffer when renaming/deleting a section
    
    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 <andre@ethiack.com>
    Reported-by: Vítor Pinho <vitor@ethiack.com>
    Helped-by: Patrick Steinhardt <ps@pks.im>
    Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
    Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
    Signed-off-by: Taylor Blau <me@ttaylorr.com>

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(&copystr);
 			}
 
-			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 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
 out_no_rollback:
 	free(filename_buf);
 	config_store_data_clear(&store);
+	strbuf_release(&buf);
 	return ret;
 }
 
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index cd8f744160..24c13b91db 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -613,7 +613,7 @@ test_expect_success 'renaming to bogus section is rejected' '
 	test_must_fail git config --rename-section branch.zwei "bogus name"
 '
 
-test_expect_failure 'renaming a section with a long line' '
+test_expect_success 'renaming a section with a long line' '
 	{
 		printf "[b]\\n" &&
 		printf "  c = d %1024s [a] e = f\\n" " " &&
@@ -623,7 +623,7 @@ test_expect_failure 'renaming a section with a long line' '
 	test_must_fail git config -f y b.e
 '
 
-test_expect_failure 'renaming an embedded section with a long line' '
+test_expect_success 'renaming an embedded section with a long line' '
 	{
 		printf "[b]\\n" &&
 		printf "  c = d %1024s [a] [foo] e = f\\n" " " &&

Places

File CVE-2023-29007-1.patch of Package git

Places