Merge branch 'jk/fast-import-unsafe'

The `--export-marks` option of `git fast-import` is exposed also via the
in-stream command `feature export-marks=...` and it allows overwriting
arbitrary paths.

This topic branch prevents the in-stream version, to prevent arbitrary
file accesses by `git fast-import` streams coming from untrusted sources
(e.g. in remote helpers that are based on `git fast-import`).

This fixes CVE-2019-1348.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

4 files changed, 95 insertions, 18 deletions

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 3d3d219e58..ff71fc2962 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -50,6 +50,21 @@ OPTIONS
 	memory used by fast-import during this run.  Showing this output
 	is currently the default, but can be disabled with --quiet.
 
+--allow-unsafe-features::
+	Many command-line options can be provided as part of the
+	fast-import stream itself by using the `feature` or `option`
+	commands. However, some of these options are unsafe (e.g.,
+	allowing fast-import to access the filesystem outside of the
+	repository). These options are disabled by default, but can be
+	allowed by providing this option on the command line.  This
+	currently impacts only the `export-marks`, `import-marks`, and
+	`import-marks-if-exists` feature commands.
++
+	Only enable this option if you trust the program generating the
+	fast-import stream! This option is enabled automatically for
+	remote-helpers that use the `import` capability, as they are
+	already trusted to run their own code.
+
 Options for Frontends
 ~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/fast-import.c b/fast-import.c
index 32ac5323f6..93c3838254 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -366,6 +366,7 @@ static uintmax_t next_mark;
 static struct strbuf new_data = STRBUF_INIT;
 static int seen_data_command;
 static int require_explicit_termination;
+static int allow_unsafe_features;
 
 /* Signal handling */
 static volatile sig_atomic_t checkpoint_requested;
@@ -1861,6 +1862,12 @@ static void dump_marks(void)
 	if (!export_marks_file || (import_marks_file && !import_marks_file_done))
 		return;
 
+	if (safe_create_leading_directories_const(export_marks_file)) {
+		failure |= error_errno("unable to create leading directories of %s",
+				       export_marks_file);
+		return;
+	}
+
 	if (hold_lock_file_for_update(&mark_lock, export_marks_file, 0) < 0) {
 		failure |= error_errno("Unable to write marks file %s",
 				       export_marks_file);
@@ -3228,7 +3235,6 @@ static void option_import_marks(const char *marks,
 	}
 
 	import_marks_file = make_fast_import_path(marks);
-	safe_create_leading_directories_const(import_marks_file);
 	import_marks_file_from_stream = from_stream;
 	import_marks_file_ignore_missing = ignore_missing;
 }
@@ -3269,7 +3275,6 @@ static void option_active_branches(const char *branches)
 static void option_export_marks(const char *marks)
 {
 	export_marks_file = make_fast_import_path(marks);
-	safe_create_leading_directories_const(export_marks_file);
 }
 
 static void option_cat_blob_fd(const char *fd)
@@ -3312,10 +3317,12 @@ static int parse_one_option(const char *option)
 		option_active_branches(option);
 	} else if (skip_prefix(option, "export-pack-edges=", &option)) {
 		option_export_pack_edges(option);
-	} else if (starts_with(option, "quiet")) {
+	} else if (!strcmp(option, "quiet")) {
 		show_stats = 0;
-	} else if (starts_with(option, "stats")) {
+	} else if (!strcmp(option, "stats")) {
 		show_stats = 1;
+	} else if (!strcmp(option, "allow-unsafe-features")) {
+		; /* already handled during early option parsing */
 	} else {
 		return 0;
 	}
@@ -3323,6 +3330,13 @@ static int parse_one_option(const char *option)
 	return 1;
 }
 
+static void check_unsafe_feature(const char *feature, int from_stream)
+{
+	if (from_stream && !allow_unsafe_features)
+		die(_("feature '%s' forbidden in input without --allow-unsafe-features"),
+		    feature);
+}
+
 static int parse_one_feature(const char *feature, int from_stream)
 {
 	const char *arg;
@@ -3330,10 +3344,13 @@ static int parse_one_feature(const char *feature, int from_stream)
 	if (skip_prefix(feature, "date-format=", &arg)) {
 		option_date_format(arg);
 	} else if (skip_prefix(feature, "import-marks=", &arg)) {
+		check_unsafe_feature("import-marks", from_stream);
 		option_import_marks(arg, from_stream, 0);
 	} else if (skip_prefix(feature, "import-marks-if-exists=", &arg)) {
+		check_unsafe_feature("import-marks-if-exists", from_stream);
 		option_import_marks(arg, from_stream, 1);
 	} else if (skip_prefix(feature, "export-marks=", &arg)) {
+		check_unsafe_feature(feature, from_stream);
 		option_export_marks(arg);
 	} else if (!strcmp(feature, "get-mark")) {
 		; /* Don't die - this feature is supported */
@@ -3460,6 +3477,20 @@ int cmd_main(int argc, const char **argv)
 	avail_tree_table = xcalloc(avail_tree_table_sz, sizeof(struct avail_tree_content*));
 	marks = pool_calloc(1, sizeof(struct mark_set));
 
+	/*
+	 * We don't parse most options until after we've seen the set of
+	 * "feature" lines at the start of the stream (which allows the command
+	 * line to override stream data). But we must do an early parse of any
+	 * command-line options that impact how we interpret the feature lines.
+	 */
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (*arg != '-' || !strcmp(arg, "--"))
+			break;
+		if (!strcmp(arg, "--allow-unsafe-features"))
+			allow_unsafe_features = 1;
+	}
+
 	global_argc = argc;
 	global_argv = argv;
 
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index d47560b634..77104f9daa 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2106,12 +2106,27 @@ test_expect_success 'R: abort on receiving feature after data command' '
 	test_must_fail git fast-import <input
 '
 
+test_expect_success 'R: import-marks features forbidden by default' '
+	>git.marks &&
+	echo "feature import-marks=git.marks" >input &&
+	test_must_fail git fast-import <input &&
+	echo "feature import-marks-if-exists=git.marks" >input &&
+	test_must_fail git fast-import <input
+'
+
 test_expect_success 'R: only one import-marks feature allowed per stream' '
+	>git.marks &&
+	>git2.marks &&
 	cat >input <<-EOF &&
 	feature import-marks=git.marks
 	feature import-marks=git2.marks
 	EOF
 
+	test_must_fail git fast-import --allow-unsafe-features <input
+'
+
+test_expect_success 'R: export-marks feature forbidden by default' '
+	echo "feature export-marks=git.marks" >input &&
 	test_must_fail git fast-import <input
 '
 
@@ -2125,19 +2140,29 @@ test_expect_success 'R: export-marks feature results in a marks file being creat
 
 	EOF
 
-	cat input | git fast-import &&
+	git fast-import --allow-unsafe-features <input &&
 	grep :1 git.marks
 '
 
 test_expect_success 'R: export-marks options can be overridden by commandline options' '
-	cat input | git fast-import --export-marks=other.marks &&
-	grep :1 other.marks
+	cat >input <<-\EOF &&
+	feature export-marks=feature-sub/git.marks
+	blob
+	mark :1
+	data 3
+	hi
+
+	EOF
+	git fast-import --allow-unsafe-features \
+			--export-marks=cmdline-sub/other.marks <input &&
+	grep :1 cmdline-sub/other.marks &&
+	test_path_is_missing feature-sub
 '
 
 test_expect_success 'R: catch typo in marks file name' '
 	test_must_fail git fast-import --import-marks=nonexistent.marks </dev/null &&
 	echo "feature import-marks=nonexistent.marks" |
-	test_must_fail git fast-import
+	test_must_fail git fast-import --allow-unsafe-features
 '
 
 test_expect_success 'R: import and output marks can be the same file' '
@@ -2193,7 +2218,8 @@ test_expect_success 'R: feature import-marks-if-exists' '
 	rm -f io.marks &&
 	>expect &&
 
-	git fast-import --export-marks=io.marks <<-\EOF &&
+	git fast-import --export-marks=io.marks \
+			--allow-unsafe-features <<-\EOF &&
 	feature import-marks-if-exists=not_io.marks
 	EOF
 	test_cmp expect io.marks &&
@@ -2204,7 +2230,8 @@ test_expect_success 'R: feature import-marks-if-exists' '
 	echo ":1 $blob" >expect &&
 	echo ":2 $blob" >>expect &&
 
-	git fast-import --export-marks=io.marks <<-\EOF &&
+	git fast-import --export-marks=io.marks \
+			--allow-unsafe-features <<-\EOF &&
 	feature import-marks-if-exists=io.marks
 	blob
 	mark :2
@@ -2217,7 +2244,8 @@ test_expect_success 'R: feature import-marks-if-exists' '
 	echo ":3 $blob" >>expect &&
 
 	git fast-import --import-marks=io.marks \
-			--export-marks=io.marks <<-\EOF &&
+			--export-marks=io.marks \
+			--allow-unsafe-features <<-\EOF &&
 	feature import-marks-if-exists=not_io.marks
 	blob
 	mark :3
@@ -2230,7 +2258,8 @@ test_expect_success 'R: feature import-marks-if-exists' '
 	>expect &&
 
 	git fast-import --import-marks-if-exists=not_io.marks \
-			--export-marks=io.marks <<-\EOF &&
+			--export-marks=io.marks \
+			--allow-unsafe-features <<-\EOF &&
 	feature import-marks-if-exists=io.marks
 	EOF
 	test_cmp expect io.marks
@@ -2242,7 +2271,7 @@ test_expect_success 'R: import to output marks works without any content' '
 	feature export-marks=marks.new
 	EOF
 
-	cat input | git fast-import &&
+	git fast-import --allow-unsafe-features <input &&
 	test_cmp marks.out marks.new
 '
 
@@ -2252,7 +2281,7 @@ test_expect_success 'R: import marks prefers commandline marks file over the str
 	feature export-marks=marks.new
 	EOF
 
-	cat input | git fast-import --import-marks=marks.out &&
+	git fast-import --import-marks=marks.out --allow-unsafe-features <input &&
 	test_cmp marks.out marks.new
 '
 
@@ -2265,7 +2294,8 @@ test_expect_success 'R: multiple --import-marks= should be honoured' '
 
 	head -n2 marks.out > one.marks &&
 	tail -n +3 marks.out > two.marks &&
-	git fast-import --import-marks=one.marks --import-marks=two.marks <input &&
+	git fast-import --import-marks=one.marks --import-marks=two.marks \
+		--allow-unsafe-features <input &&
 	test_cmp marks.out combined.marks
 '
 
@@ -2278,7 +2308,7 @@ test_expect_success 'R: feature relative-marks should be honoured' '
 
 	mkdir -p .git/info/fast-import/ &&
 	cp marks.new .git/info/fast-import/relative.in &&
-	git fast-import <input &&
+	git fast-import --allow-unsafe-features <input &&
 	test_cmp marks.new .git/info/fast-import/relative.out
 '
 
@@ -2290,7 +2320,7 @@ test_expect_success 'R: feature no-relative-marks should be honoured' '
 	feature export-marks=non-relative.out
 	EOF
 
-	git fast-import <input &&
+	git fast-import --allow-unsafe-features <input &&
 	test_cmp marks.new non-relative.out
 '
 
@@ -2560,7 +2590,7 @@ test_expect_success 'R: quiet option results in no stats being output' '
 
 	EOF
 
-	cat input | git fast-import 2> output &&
+	git fast-import 2>output <input &&
 	test_must_be_empty output
 '
 
diff --git a/transport-helper.c b/transport-helper.c
index a72ed18efb..af519c1ff7 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -431,6 +431,7 @@ static int get_importer(struct transport *transport, struct child_process *fasti
 	child_process_init(fastimport);
 	fastimport->in = helper->out;
 	argv_array_push(&fastimport->args, "fast-import");
+	argv_array_push(&fastimport->args, "--allow-unsafe-features");
 	argv_array_push(&fastimport->args, debug ? "--stats" : "--quiet");
 
 	if (data->bidi_import) {