@matheusbsbs / matheus-bagliano / commits / 68061e3470

Fork 0

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>

Jeff King committed Aug 29, 2019 at 14:37 UTC 68061e3470210703cb15594194718d35094afdc0

4 files changed +55 -8

Documentation/git-fast-import.txt

+14

@@ -50,6 +50,20 @@ OPTIONS

50 memory used by fast-import during this run. Showing this output

51 is currently the default, but can be disabled with --quiet.

53 +--allow-unsafe-features::

54 + Many command-line options can be provided as part of the

55 + fast-import stream itself by using the `feature` or `option`

56 + commands. However, some of these options are unsafe (e.g.,

57 + allowing fast-import to access the filesystem outside of the

58 + repository). These options are disabled by default, but can be

59 + allowed by providing this option on the command line. This

60 + currently impacts only the `feature export-marks` command.

61 ++

62 + Only enable this option if you trust the program generating the

63 + fast-import stream! This option is enabled automatically for

64 + remote-helpers that use the `import` capability, as they are

65 + already trusted to run their own code.

66 +

67 Options for Frontends

68 ~~~~~~~~~~~~~~~~~~~~~

fast-import.c

+25

@@ -366,6 +366,7 @@ static uintmax_t next_mark;

366 static struct strbuf new_data = STRBUF_INIT;

367 static int seen_data_command;

368 static int require_explicit_termination;

369 +static int allow_unsafe_features;

370

371 /* Signal handling */

372 static volatile sig_atomic_t checkpoint_requested;

@@ -3320,6 +3321,8 @@ static int parse_one_option(const char *option)

3321 show_stats = 0;

3322 } else if (!strcmp(option, "stats")) {

3323 show_stats = 1;

3324 + } else if (!strcmp(option, "allow-unsafe-features")) {

3325 + ; /* already handled during early option parsing */

3326 } else {

3327 return 0;

3328 }

@@ -3327,6 +3330,13 @@ static int parse_one_option(const char *option)

3330 return 1;

3331 }

3332

3333 +static void check_unsafe_feature(const char *feature, int from_stream)

3334 +{

3335 + if (from_stream && !allow_unsafe_features)

3336 + die(_("feature '%s' forbidden in input without --allow-unsafe-features"),

3337 + feature);

3338 +}

3339 +

3340 static int parse_one_feature(const char *feature, int from_stream)

3341 {

3342 const char *arg;

@@ -3338,6 +3348,7 @@ static int parse_one_feature(const char *feature, int from_stream)

3348 } else if (skip_prefix(feature, "import-marks-if-exists=", &arg)) {

3349 option_import_marks(arg, from_stream, 1);

3350 } else if (skip_prefix(feature, "export-marks=", &arg)) {

3351 + check_unsafe_feature(feature, from_stream);

3352 option_export_marks(arg);

3353 } else if (!strcmp(feature, "get-mark")) {

3354 ; /* Don't die - this feature is supported */

@@ -3464,6 +3475,20 @@ int cmd_main(int argc, const char **argv)

3475 avail_tree_table = xcalloc(avail_tree_table_sz, sizeof(struct avail_tree_content*));

3476 marks = pool_calloc(1, sizeof(struct mark_set));

3477

3478 + /*

3479 + * We don't parse most options until after we've seen the set of

3480 + * "feature" lines at the start of the stream (which allows the command

3481 + * line to override stream data). But we must do an early parse of any

3482 + * command-line options that impact how we interpret the feature lines.

3483 + */

3484 + for (i = 1; i < argc; i++) {

3485 + const char *arg = argv[i];

3486 + if (*arg != '-' || !strcmp(arg, "--"))

3487 + break;

3488 + if (!strcmp(arg, "--allow-unsafe-features"))

3489 + allow_unsafe_features = 1;

3490 + }

3491 +

3492 global_argc = argc;

3493 global_argv = argv;

3494

t/t9300-fast-import.sh

+15 -8

@@ -2117,6 +2117,11 @@ test_expect_success 'R: only one import-marks feature allowed per stream' '

2117 test_must_fail git fast-import <input

2118 '

2119

2120 +test_expect_success 'R: export-marks feature forbidden by default' '

2121 + echo "feature export-marks=git.marks" >input &&

2122 + test_must_fail git fast-import <input

2123 +'

2124 +

2125 test_expect_success 'R: export-marks feature results in a marks file being created' '

2126 cat >input <<-EOF &&

2127 feature export-marks=git.marks

@@ -2127,7 +2132,7 @@ test_expect_success 'R: export-marks feature results in a marks file being creat

2132

2133 EOF

2134

2130 - git fast-import <input &&

2135 + git fast-import --allow-unsafe-features <input &&

2136 grep :1 git.marks

2137 '

2138

@@ -2140,7 +2145,8 @@ test_expect_success 'R: export-marks options can be overridden by commandline op

2145 hi

2146

2147 EOF

2143 - git fast-import --export-marks=cmdline-sub/other.marks <input &&

2148 + git fast-import --allow-unsafe-features \

2149 + --export-marks=cmdline-sub/other.marks <input &&

2150 grep :1 cmdline-sub/other.marks &&

2151 test_path_is_missing feature-sub

2152 '

@@ -2148,7 +2154,7 @@ test_expect_success 'R: export-marks options can be overridden by commandline op

2154 test_expect_success 'R: catch typo in marks file name' '

2155 test_must_fail git fast-import --import-marks=nonexistent.marks </dev/null &&

2156 echo "feature import-marks=nonexistent.marks" |

2151 - test_must_fail git fast-import

2157 + test_must_fail git fast-import --allow-unsafe-features

2158 '

2159

2160 test_expect_success 'R: import and output marks can be the same file' '

@@ -2253,7 +2259,7 @@ test_expect_success 'R: import to output marks works without any content' '

2259 feature export-marks=marks.new

2260 EOF

2261

2256 - git fast-import <input &&

2262 + git fast-import --allow-unsafe-features <input &&

2263 test_cmp marks.out marks.new

2264 '

2265

@@ -2263,7 +2269,7 @@ test_expect_success 'R: import marks prefers commandline marks file over the str

2269 feature export-marks=marks.new

2270 EOF

2271

2266 - git fast-import --import-marks=marks.out <input &&

2272 + git fast-import --import-marks=marks.out --allow-unsafe-features <input &&

2273 test_cmp marks.out marks.new

2274 '

2275

@@ -2276,7 +2282,8 @@ test_expect_success 'R: multiple --import-marks= should be honoured' '

2282

2283 head -n2 marks.out > one.marks &&

2284 tail -n +3 marks.out > two.marks &&

2279 - git fast-import --import-marks=one.marks --import-marks=two.marks <input &&

2285 + git fast-import --import-marks=one.marks --import-marks=two.marks \

2286 + --allow-unsafe-features <input &&

2287 test_cmp marks.out combined.marks

2288 '

2289

@@ -2289,7 +2296,7 @@ test_expect_success 'R: feature relative-marks should be honoured' '

2296

2297 mkdir -p .git/info/fast-import/ &&

2298 cp marks.new .git/info/fast-import/relative.in &&

2292 - git fast-import <input &&

2299 + git fast-import --allow-unsafe-features <input &&

2300 test_cmp marks.new .git/info/fast-import/relative.out

2301 '

2302

@@ -2301,7 +2308,7 @@ test_expect_success 'R: feature no-relative-marks should be honoured' '

2308 feature export-marks=non-relative.out

2309 EOF

2310

2304 - git fast-import <input &&

2311 + git fast-import --allow-unsafe-features <input &&

2312 test_cmp marks.new non-relative.out

2313 '

2314

transport-helper.c

@@ -431,6 +431,7 @@ static int get_importer(struct transport *transport, struct child_process *fasti

431 child_process_init(fastimport);

432 fastimport->in = helper->out;

433 argv_array_push(&fastimport->args, "fast-import");

434 + argv_array_push(&fastimport->args, "--allow-unsafe-features");

435 argv_array_push(&fastimport->args, debug ? "--stats" : "--quiet");

436

437 if (data->bidi_import) {