packages/git
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
git/CVE-2019-1348.patch

260 lines
9.9 KiB
Diff
Raw Blame History

From 68061e3470210703cb15594194718d35094afdc0 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Thu, 29 Aug 2019 14:37:26 -0400
Subject: [PATCH] fast-import: disallow "feature export-marks" by default
The fast-import stream command "feature export-marks=<path>" lets the
stream write marks to an arbitrary path. This may be surprising if you
are running fast-import against an untrusted input (which otherwise
cannot do anything except update Git objects and refs).
Let's disallow the use of this feature by default, and provide a
command-line option to re-enable it (you can always just use the
command-line --export-marks as well, but the in-stream version provides
an easy way for exporters to control the process).
This is a backwards-incompatible change, since the default is flipping
to the new, safer behavior. However, since the main users of the
in-stream versions would be import/export-based remote helpers, and
since we trust remote helpers already (which are already running
arbitrary code), we'll pass the new option by default when reading a
remote helper's stream. This should minimize the impact.
Note that the implementation isn't totally simple, as we have to work
around the fact that fast-import doesn't parse its command-line options
until after it has read any "feature" lines from the stream. This is how
it lets command-line options override in-stream. But in our case, it's
important to parse the new --allow-unsafe-features first.
There are three options for resolving this:
1. Do a separate "early" pass over the options. This is easy for us to
do because there are no command-line options that allow the
"unstuck" form (so there's no chance of us mistaking an argument
for an option), though it does introduce a risk of incorrect
parsing later (e.g,. if we convert to parse-options).
2. Move the option parsing phase back to the start of the program, but
teach the stream-reading code never to override an existing value.
This is tricky, because stream "feature" lines override each other
(meaning we'd have to start tracking the source for every option).
3. Accept that we might parse a "feature export-marks" line that is
forbidden, as long we don't _act_ on it until after we've parsed
the command line options.
This would, in fact, work with the current code, but only because
the previous patch fixed the export-marks parser to avoid touching
the filesystem.
So while it works, it does carry risk of somebody getting it wrong
in the future in a rather subtle and unsafe way.
I've gone with option (1) here as simple, safe, and unlikely to cause
regressions.
This fixes CVE-2019-1348.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-fast-import.txt | 14 ++++++++++++++
fast-import.c | 25 +++++++++++++++++++++++++
t/t9300-fast-import.sh | 23 +++++++++++++++--------
transport-helper.c | 1 +
4 files changed, 55 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index fad327a..76747ef 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -51,6 +51,20 @@ 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 `feature export-marks` command.
++
+ 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 71312d4..b001abc 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -217,6 +217,7 @@ struct recent_command {
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;
@@ -3171,6 +3172,8 @@ static int parse_one_option(const char *option)
show_stats = 0;
} else if (!strcmp(option, "stats")) {
show_stats = 1;
+ } else if (!strcmp(option, "allow-unsafe-features")) {
+ ; /* already handled during early option parsing */
} else {
return 0;
}
@@ -3178,6 +3181,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;
@@ -3189,6 +3199,7 @@ static int parse_one_feature(const char *feature, int from_stream)
} else if (skip_prefix(feature, "import-marks-if-exists=", &arg)) {
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 */
@@ -3315,6 +3326,20 @@ int cmd_main(int argc, const char **argv)
avail_tree_table = xcalloc(avail_tree_table_sz, sizeof(struct avail_tree_content*));
marks = mem_pool_calloc(&fi_mem_pool, 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 913ff89..8ca58f5 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2115,6 +2115,11 @@ test_expect_success 'R: only one import-marks feature allowed per stream' '
test_must_fail git fast-import <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
+'
+
test_expect_success 'R: export-marks feature results in a marks file being created' '
cat >input <<-EOF &&
feature export-marks=git.marks
@@ -2125,7 +2130,7 @@ test_expect_success 'R: export-marks feature results in a marks file being creat
EOF
- git fast-import <input &&
+ git fast-import --allow-unsafe-features <input &&
grep :1 git.marks
'
@@ -2138,7 +2143,8 @@ test_expect_success 'R: export-marks options can be overridden by commandline op
hi
EOF
- git fast-import --export-marks=cmdline-sub/other.marks <input &&
+ 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
'
@@ -2146,7 +2152,7 @@ test_expect_success 'R: export-marks options can be overridden by commandline op
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' '
@@ -2248,7 +2254,7 @@ test_expect_success 'R: import to output marks works without any content' '
feature export-marks=marks.new
EOF
- git fast-import <input &&
+ git fast-import --allow-unsafe-features <input &&
test_cmp marks.out marks.new
'
@@ -2258,7 +2264,7 @@ test_expect_success 'R: import marks prefers commandline marks file over the str
feature export-marks=marks.new
EOF
- git fast-import --import-marks=marks.out <input &&
+ git fast-import --import-marks=marks.out --allow-unsafe-features <input &&
test_cmp marks.out marks.new
'
@@ -2271,7 +2277,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
'
@@ -2284,7 +2291,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
'
@@ -2296,7 +2303,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
'
diff --git a/transport-helper.c b/transport-helper.c
index 6b05a88..f828a6d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -425,6 +425,7 @@ static int get_importer(struct transport *transport, struct child_process *fasti
child_process_init(fastimport);
fastimport->in = xdup(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) {
--
1.8.3.1
Reference in New Issue View Git Blame Copy Permalink