[PATCH] archive: fix segfault from too long --format parameter

[PATCH] archive: fix segfault from too long --format parameter

[Jonathan Nieder]

‘git archive --format=<string of 25 characters or more>’ overflows a
local buffer, producing a segfault here.

The context: in commit 0f4b377 (git-archive: infer output format from
filename when unspecified, 2009-09-14), the cmd_archive wrapper
learned to produce a format argument for the local or remote archive
machinery in a local buffer, but that code was missing a bounds check.

So add the missing check.  As a belt-and-suspenders measure, also use
snprintf to make sure the copy afterwards does not overflow.

Cc: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
Cc: Dmitry Potapov <dpotapov@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I noticed this while reading over the archive code.  Thoughts?

 builtin-archive.c   |    4 +++-
 t/t5000-tar-tree.sh |    6 ++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/builtin-archive.c b/builtin-archive.c
index 3fb4136..94db00d 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -107,7 +107,9 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 	}
 
 	if (format) {
-		sprintf(fmt_opt, "--format=%s", format);
+		if (strlen(format) > sizeof(fmt_opt) - sizeof("--format="))
+			die("git archive: format is too long: %.50s", format);
+		snprintf(fmt_opt, sizeof(fmt_opt), "--format=%s", format);
 		/*
 		 * We have enough room in argv[] to muck it in place,
 		 * because either --format and/or --output must have
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 0037f63..cf114b2 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -174,6 +174,12 @@ test_expect_success \
 '
 
 test_expect_success \
+    'git archive --format=<long nonsense string>' \
+    'format=abacadabra &&
+    format="${format}${format}${format}zip" &&
+    test_must_fail git archive "--format=$format" HEAD'
+
+test_expect_success \
     'git archive --format=zip' \
     'git archive --format=zip HEAD >d.zip'
 
-- 
1.7.0.rc1

Re: [PATCH] archive: fix segfault from too long --format parameter

[René Scharfe]

Am 07.02.2010 08:10, schrieb Jonathan Nieder:
> ‘git archive --format=<string of 25 characters or more>’ overflows a
> local buffer, producing a segfault here.
> 
> The context: in commit 0f4b377 (git-archive: infer output format from
> filename when unspecified, 2009-09-14), the cmd_archive wrapper
> learned to produce a format argument for the local or remote archive
> machinery in a local buffer, but that code was missing a bounds check.
> 
> So add the missing check.  As a belt-and-suspenders measure, also use
> snprintf to make sure the copy afterwards does not overflow.
> 
> Cc: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> Cc: Dmitry Potapov <dpotapov@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> I noticed this while reading over the archive code.  Thoughts?
> 
>  builtin-archive.c   |    4 +++-
>  t/t5000-tar-tree.sh |    6 ++++++
>  2 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin-archive.c b/builtin-archive.c
> index 3fb4136..94db00d 100644
> --- a/builtin-archive.c
> +++ b/builtin-archive.c
> @@ -107,7 +107,9 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (format) {
> -		sprintf(fmt_opt, "--format=%s", format);
> +		if (strlen(format) > sizeof(fmt_opt) - sizeof("--format="))
> +			die("git archive: format is too long: %.50s", format);
> +		snprintf(fmt_opt, sizeof(fmt_opt), "--format=%s", format);
>  		/*
>  		 * We have enough room in argv[] to muck it in place,
>  		 * because either --format and/or --output must have

Thanks.  I think this should go into 1.7.0.  I'd use the same format
string for the error message as archive.c, i.e. "Unknown archive format
'%s'".  Later I'd rather do this:


 builtin-archive.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/builtin-archive.c b/builtin-archive.c
index 3fb4136..ffe4f4a 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -70,7 +70,7 @@ static const char *format_from_name(const char *filename)
 		return NULL;
 	ext++;
 	if (!strcasecmp(ext, "zip"))
-		return "zip";
+		return "--format=zip";
 	return NULL;
 }
 
@@ -92,33 +92,32 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 			"retrieve the archive from remote repository <repo>"),
 		OPT_STRING(0, "exec", &exec, "cmd",
 			"path to the remote git-upload-archive command"),
-		OPT_STRING(0, "format", &format, "fmt", "archive format"),
 		OPT_END()
 	};
-	char fmt_opt[32];
 
 	argc = parse_options(argc, argv, prefix, local_opts, NULL,
 			     PARSE_OPT_KEEP_ALL);
 
 	if (output) {
 		create_output_file(output);
-		if (!format)
-			format = format_from_name(output);
+		format = format_from_name(output);
 	}
 
 	if (format) {
-		sprintf(fmt_opt, "--format=%s", format);
 		/*
 		 * We have enough room in argv[] to muck it in place,
-		 * because either --format and/or --output must have
-		 * been given on the original command line if we get
-		 * to this point, and parse_options() must have eaten
-		 * it, i.e. we can add back one element to the array.
-		 * But argv[] may contain "--"; we should make it the
-		 * first option.
+		 * because --output must have been given on the
+		 * original command line if we get to this point, and
+		 * parse_options() must have eaten it, i.e. we can add
+		 * back one element to the array.  We add a fake
+		 * --format option at the beginning with the hint
+		 * derived from our output filename.  This way explicit
+		 * --format options can override it, and the fake
+		 * option is inserted before any "--" that might have
+		 * been given.
 		 */
 		memmove(argv + 2, argv + 1, sizeof(*argv) * argc);
-		argv[1] = fmt_opt;
+		argv[1] = format;
 		argv[++argc] = NULL;
 	}
 

[PATCH] archive: simplify archive format guessing

[René Scharfe]

Here's my patch again, with a reformatted comment, a renamed variable and
two simple tests.
-- >8 --
The code to guess an output archive's format consumed any --format
options and built a new one.  Jonathan noticed that it does so in an
unsafe way, risking to overflow the static buffer fmt_opt.

Change the code to keep the existing --format options intact and to only
add a new one if a format could be guessed based on the output file name.
The new option is added as the first one, allowing the existing ones to
overrule it, i.e. explicit --format options given on the command line win
over format guesses, as before.

To simplify the code further, format_from_name() is changed to return the
full --format option, thus no potentially dangerous sprintf() calls are
needed any more.

Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin-archive.c   |   34 ++++++++++++++++------------------
 t/t5000-tar-tree.sh |   10 ++++++++++
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/builtin-archive.c b/builtin-archive.c
index 3fb4136..6a887f5 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -70,7 +70,7 @@ static const char *format_from_name(const char *filename)
 		return NULL;
 	ext++;
 	if (!strcasecmp(ext, "zip"))
-		return "zip";
+		return "--format=zip";
 	return NULL;
 }
 
@@ -84,7 +84,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 	const char *exec = "git-upload-archive";
 	const char *output = NULL;
 	const char *remote = NULL;
-	const char *format = NULL;
+	const char *format_option = NULL;
 	struct option local_opts[] = {
 		OPT_STRING('o', "output", &output, "file",
 			"write the archive to this file"),
@@ -92,33 +92,31 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 			"retrieve the archive from remote repository <repo>"),
 		OPT_STRING(0, "exec", &exec, "cmd",
 			"path to the remote git-upload-archive command"),
-		OPT_STRING(0, "format", &format, "fmt", "archive format"),
 		OPT_END()
 	};
-	char fmt_opt[32];
 
 	argc = parse_options(argc, argv, prefix, local_opts, NULL,
 			     PARSE_OPT_KEEP_ALL);
 
 	if (output) {
 		create_output_file(output);
-		if (!format)
-			format = format_from_name(output);
+		format_option = format_from_name(output);
 	}
 
-	if (format) {
-		sprintf(fmt_opt, "--format=%s", format);
-		/*
-		 * We have enough room in argv[] to muck it in place,
-		 * because either --format and/or --output must have
-		 * been given on the original command line if we get
-		 * to this point, and parse_options() must have eaten
-		 * it, i.e. we can add back one element to the array.
-		 * But argv[] may contain "--"; we should make it the
-		 * first option.
-		 */
+	/*
+	 * We have enough room in argv[] to muck it in place, because
+	 * --output must have been given on the original command line
+	 * if we get to this point, and parse_options() must have eaten
+	 * it, i.e. we can add back one element to the array.
+	 *
+	 * We add a fake --format option at the beginning, with the
+	 * format inferred from our output filename.  This way explicit
+	 * --format options can override it, and the fake option is
+	 * inserted before any "--" that might have been given.
+	 */
+	if (format_option) {
 		memmove(argv + 2, argv + 1, sizeof(*argv) * argc);
-		argv[1] = fmt_opt;
+		argv[1] = format_option;
 		argv[++argc] = NULL;
 	}
 
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 0037f63..27bfba5 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -189,6 +189,16 @@ test_expect_success 'git archive --format=zip with --output' \
     'git archive --format=zip --output=d2.zip HEAD &&
     test_cmp d.zip d2.zip'
 
+test_expect_success 'git archive with --output, inferring format' '
+	git archive --output=d3.zip HEAD &&
+	test_cmp d.zip d3.zip
+'
+
+test_expect_success 'git archive with --output, override inferred format' '
+	git archive --format=tar --output=d4.zip HEAD &&
+	test_cmp b.tar d4.zip
+'
+
 $UNZIP -v >/dev/null 2>&1
 if [ $? -eq 127 ]; then
 	say "Skipping ZIP tests, because unzip was not found"
-- 
1.7.0.rc1

Re: [PATCH] archive: simplify archive format guessing

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Here's my patch again, with a reformatted comment, a renamed variable and
> two simple tests.

Thanks; will apply to maint and merge upwards.