| Message ID | pull.1832.git.1732740464398.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fast-import: disallow more path components | expand |
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > Instead of just disallowing '.' and '..', make use of verify_path() to > ensure that fast-import will disallow anything we wouldn't allow into > the index, such as anything under .git/, .gitmodules as a symlink, or > a dos drive prefix on Windows. > > Since a few fast-export and fast-import tests that tried to stress-test > the correct handling of quoting relied on filenames that fail > is_valid_win32_path(), such as spaces or periods at the end of filenames > or backslashes within the filename, turn off core.protectNTFS for those > tests to ensure they keep passing. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > Disallow verify_path() failures from fast-import > > Since en/fast-import-path-sanitize has already made it to next, this > commit is based on that. (See > https://lore.kernel.org/git/pull.1831.v2.git.1732561248717.gitgitgadget@gmail.com/ > for discussion of that series.) > > Changes relative to that commit: this fixes up the error message as > suggested by Kristoffer, and makes the checks more encompassing as > suggested by Patrick and Peff -- in particular, using verify_path() as > suggested by Peff. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1832%2Fnewren%2Fdisallow-verify-path-fast-import-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1832/newren/disallow-verify-path-fast-import-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1832 Thanks, all. Looking good to me. Will queue.
On Wed, Nov 27, 2024 at 08:47:44PM +0000, Elijah Newren via GitGitGadget wrote: > Instead of just disallowing '.' and '..', make use of verify_path() to > ensure that fast-import will disallow anything we wouldn't allow into > the index, such as anything under .git/, .gitmodules as a symlink, or > a dos drive prefix on Windows. > > Since a few fast-export and fast-import tests that tried to stress-test > the correct handling of quoting relied on filenames that fail > is_valid_win32_path(), such as spaces or periods at the end of filenames > or backslashes within the filename, turn off core.protectNTFS for those > tests to ensure they keep passing. Great, thanks for following up on this. I think it will work as advertised, though... > @@ -1413,6 +1414,8 @@ static int tree_content_set( > die("Empty path component found in input"); > if (!*slash1 && !S_ISDIR(mode) && subtree) > die("Non-directories cannot have subtrees"); > + if (!verify_path(p, mode)) > + die("invalid path '%s'", p); This spot is over-verifying, I think, because it's recursive. Given the path foo/bar/baz, it will see "foo/bar/baz", then "bar/baz", then "baz". But just the first one should be sufficient to feed to verify_path(). I'd have expected the check when we parse the path in file_change_m(). That would also require touching other callers, too. One option would be to put a non-recursive wrapper around tree_content_set(), and add the check there. But looking at those other callers, I think it's really just file_change_cr() that maters. The other call in do_change_note_fanout() is using a notes path that we've constructed ourselves (so it's not wrong to check, but probably pointless). The patch below passes your tests (though perhaps it would be worth adding an explicit check of a copy/rename). Is it worth worrying about the efficiency of the extra calls? I'm not sure, and I didn't measure. But this just seems less surprising to me overall. (Both your patch and what I've shown below do not verify deletions from the tree, but I think that's fine; such a name would not exist in the tree in the first place). -Peff --- diff --git a/builtin/fast-import.c b/builtin/fast-import.c index bb4b769c7c..265d1b7d52 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -1414,8 +1414,6 @@ static int tree_content_set( die("Empty path component found in input"); if (!*slash1 && !S_ISDIR(mode) && subtree) die("Non-directories cannot have subtrees"); - if (!verify_path(p, mode)) - die("invalid path '%s'", p); if (!root->tree) load_tree(root); @@ -2417,6 +2415,9 @@ static void file_change_m(const char *p, struct branch *b) tree_content_replace(&b->branch_tree, &oid, mode, NULL); return; } + + if (!verify_path(path.buf, mode)) + die("invalid path '%s'", path.buf); tree_content_set(&b->branch_tree, path.buf, &oid, mode, NULL); } @@ -2454,6 +2455,8 @@ static void file_change_cr(const char *p, struct branch *b, int rename) leaf.tree); return; } + if (!verify_path(dest.buf, leaf.versions[1].mode)) + die("invalid path '%s'", dest.buf); tree_content_set(&b->branch_tree, dest.buf, &leaf.versions[1].oid, leaf.versions[1].mode,
diff --git a/builtin/fast-import.c b/builtin/fast-import.c index 3e7ec1f1198..bb4b769c7c3 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -13,6 +13,7 @@ #include "delta.h" #include "pack.h" #include "path.h" +#include "read-cache-ll.h" #include "refs.h" #include "csum-file.h" #include "quote.h" @@ -1413,6 +1414,8 @@ static int tree_content_set( die("Empty path component found in input"); if (!*slash1 && !S_ISDIR(mode) && subtree) die("Non-directories cannot have subtrees"); + if (!verify_path(p, mode)) + die("invalid path '%s'", p); if (!root->tree) load_tree(root); @@ -1468,8 +1471,6 @@ static int tree_content_set( root->tree = t = grow_tree_content(t, t->entry_count); e = new_tree_entry(); e->name = to_atom(p, n); - if (is_dot_or_dotdot(e->name->str_dat)) - die("path %s contains invalid component", p); e->versions[0].mode = 0; oidclr(&e->versions[0].oid, the_repository->hash_algo); t->entries[t->entry_count++] = e; diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 5a5127fffa7..e2b1db6bc2f 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -522,7 +522,7 @@ test_expect_success 'B: fail on invalid committer (5)' ' test_must_fail git fast-import <input ' -test_expect_success 'B: fail on invalid file path' ' +test_expect_success 'B: fail on invalid file path of ..' ' cat >input <<-INPUT_END && blob mark :1 @@ -542,6 +542,86 @@ test_expect_success 'B: fail on invalid file path' ' test_must_fail git fast-import <input ' +test_expect_success 'B: fail on invalid file path of .' ' + cat >input <<-INPUT_END && + blob + mark :1 + data <<EOF + File contents + EOF + + commit refs/heads/badpath + committer Name <email> $GIT_COMMITTER_DATE + data <<COMMIT + Commit Message + COMMIT + M 100644 :1 ./invalid-path + INPUT_END + + test_when_finished "git update-ref -d refs/heads/badpath" && + test_must_fail git fast-import <input +' + +test_expect_success WINDOWS 'B: fail on invalid file path of C:' ' + cat >input <<-INPUT_END && + blob + mark :1 + data <<EOF + File contents + EOF + + commit refs/heads/badpath + committer Name <email> $GIT_COMMITTER_DATE + data <<COMMIT + Commit Message + COMMIT + M 100644 :1 C:/invalid-path + INPUT_END + + test_when_finished "git update-ref -d refs/heads/badpath" && + test_must_fail git fast-import <input +' + +test_expect_success 'B: fail on invalid file path of .git' ' + cat >input <<-INPUT_END && + blob + mark :1 + data <<EOF + File contents + EOF + + commit refs/heads/badpath + committer Name <email> $GIT_COMMITTER_DATE + data <<COMMIT + Commit Message + COMMIT + M 100644 :1 .git/invalid-path + INPUT_END + + test_when_finished "git update-ref -d refs/heads/badpath" && + test_must_fail git fast-import <input +' + +test_expect_success 'B: fail on invalid file path of .gitmodules' ' + cat >input <<-INPUT_END && + blob + mark :1 + data <<EOF + File contents + EOF + + commit refs/heads/badpath + committer Name <email> $GIT_COMMITTER_DATE + data <<COMMIT + Commit Message + COMMIT + M 120000 :1 .gitmodules + INPUT_END + + test_when_finished "git update-ref -d refs/heads/badpath" && + test_must_fail git fast-import <input +' + ### ### series C ### @@ -966,7 +1046,7 @@ test_expect_success 'L: verify internal tree sorting' ' :100644 100644 M ba EXPECT_END - git fast-import <input && + git -c core.protectNTFS=false fast-import <input && GIT_PRINT_SHA1_ELLIPSIS="yes" git diff-tree --abbrev --raw L^ L >output && cut -d" " -f1,2,5 output >actual && test_cmp expect actual @@ -3117,7 +3197,7 @@ test_path_eol_success () { test_expect_success "S: paths at EOL with $test must work" ' test_when_finished "git branch -D S-path-eol" && - git fast-import --export-marks=marks.out <<-EOF >out 2>err && + git -c core.protectNTFS=false fast-import --export-marks=marks.out <<-EOF >out 2>err && blob mark :401 data <<BLOB @@ -3226,7 +3306,7 @@ test_path_space_success () { test_expect_success "S: paths before space with $test must work" ' test_when_finished "git branch -D S-path-space" && - git fast-import --export-marks=marks.out <<-EOF 2>err && + git -c core.protectNTFS=false fast-import --export-marks=marks.out <<-EOF 2>err && blob mark :401 data <<BLOB diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 1eb035ee4ce..bb83e5accd9 100755 --- a/t/t9350-fast-export.sh +++ b/t/t9350-fast-export.sh @@ -631,7 +631,7 @@ test_expect_success 'fast-export quotes pathnames' ' git rev-list HEAD >expect && git init result && cd result && - git fast-import <../export.out && + git -c core.protectNTFS=false fast-import <../export.out && git rev-list HEAD >actual && test_cmp ../expect actual )