From f94804c1f2626831c6bdf8cc269a571324e3f2f2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 29 Aug 2019 11:19:18 -0400 Subject: t9300: drop some useless uses of cat These waste a process, and make the line longer than it needs to be. Signed-off-by: Jeff King --- t/t9300-fast-import.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 't/t9300-fast-import.sh') diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index d47560b634..1d2a7516fd 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2125,12 +2125,12 @@ test_expect_success 'R: export-marks feature results in a marks file being creat EOF - cat input | git fast-import && + git fast-import output && + git fast-import 2>output Date: Thu, 29 Aug 2019 13:43:23 -0400 Subject: t9300: create marks files for double-import-marks test Our tests confirm that providing two "import-marks" options in a fast-import stream is an error. However, the invoked command would fail even without covering this case, because the marks files themselves do not actually exist. Let's create the files to make sure we fail for the right reason (we actually do, because the option parsing happens before we open anything, but this future-proofs our test). Signed-off-by: Jeff King --- t/t9300-fast-import.sh | 2 ++ 1 file changed, 2 insertions(+) (limited to 't/t9300-fast-import.sh') diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 1d2a7516fd..c0d04ec3ee 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2107,6 +2107,8 @@ test_expect_success 'R: abort on receiving feature after data command' ' ' 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 -- cgit v1.2.3 From 019683025f1b14d7cb671312ab01f7330e9b33e7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 29 Aug 2019 13:33:48 -0400 Subject: fast-import: delay creating leading directories for export-marks When we parse the --export-marks option, we don't immediately open the file, but we do create any leading directories. This can be especially confusing when a command-line option overrides an in-stream one, in which case we'd create the leading directory for the in-stream file, even though we never actually write the file. Let's instead create the directories just before opening the file, which means we'll create only useful directories. Note that this could change the handling of relative paths if we chdir() in between, but we don't actually do so; the only permanent chdir is from setup_git_directory() which runs before either code path (potentially we should take the pre-setup dir into account to avoid surprising the user, but that's an orthogonal change). The test just adapts the existing "override" test to use paths with leading directories. This checks both that the correct directory is created (which worked before but was not tested), and that the overridden one is not (our new fix here). While we're here, let's also check the error result of safe_create_leading_directories(). We'd presumably notice any failure immediately after when we try to open the file itself, but we can give a more specific error message in this case. Signed-off-by: Jeff King --- t/t9300-fast-import.sh | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) (limited to 't/t9300-fast-import.sh') diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index c0d04ec3ee..1ba20c1f1a 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2132,8 +2132,17 @@ test_expect_success 'R: export-marks feature results in a marks file being creat ' test_expect_success 'R: export-marks options can be overridden by commandline options' ' - git fast-import --export-marks=other.marks input <<-\EOF && + feature export-marks=feature-sub/git.marks + blob + mark :1 + data 3 + hi + + EOF + git fast-import --export-marks=cmdline-sub/other.marks Date: Thu, 29 Aug 2019 14:37:26 -0400 Subject: fast-import: disallow "feature export-marks" by default The fast-import stream command "feature export-marks=" 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 --- t/t9300-fast-import.sh | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) (limited to 't/t9300-fast-import.sh') diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 1ba20c1f1a..ba5a35c32c 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2117,6 +2117,11 @@ test_expect_success 'R: only one import-marks feature allowed per stream' ' test_must_fail git fast-import input && + test_must_fail git fast-import input <<-EOF && feature export-marks=git.marks @@ -2127,7 +2132,7 @@ test_expect_success 'R: export-marks feature results in a marks file being creat EOF - git fast-import one.marks && tail -n +3 marks.out > two.marks && - git fast-import --import-marks=one.marks --import-marks=two.marks Date: Thu, 29 Aug 2019 15:08:42 -0400 Subject: fast-import: disallow "feature import-marks" by default As with export-marks in the previous commit, import-marks can access the filesystem. This is significantly less dangerous than export-marks because it only involves reading from arbitrary paths, rather than writing them. However, it could still be surprising and have security implications (e.g., exfiltrating data from a service that accepts fast-import streams). Let's lump it (and its "if-exists" counterpart) in with export-marks, and enable the in-stream version only if --allow-unsafe-features is set. Signed-off-by: Jeff King --- t/t9300-fast-import.sh | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) (limited to 't/t9300-fast-import.sh') diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index ba5a35c32c..77104f9daa 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2106,6 +2106,14 @@ test_expect_success 'R: abort on receiving feature after data command' ' test_must_fail git fast-import git.marks && + echo "feature import-marks=git.marks" >input && + test_must_fail git fast-import input && + test_must_fail git fast-import git.marks && >git2.marks && @@ -2114,7 +2122,7 @@ test_expect_success 'R: only one import-marks feature allowed per stream' ' feature import-marks=git2.marks EOF - test_must_fail git fast-import 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 && @@ -2221,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 @@ -2234,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 @@ -2247,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 -- cgit v1.2.3