fast-import: disallow more path components

PATCH Superseded Elijah Newren via GitGitGadget 4 participants 11 messages Nov 27, 2024 – Dec 4, 2024

lore

Share & tools

lore

Elijah Newren via GitGitGadget

author

2024-11-27 20:47 UTC #0 lore raw

PATCH 1/1 fast-import: disallow more path components

From: Elijah Newren <[email protected]>

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 <[email protected]>

---
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/[email protected]/
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

 builtin/fast-import.c  |  5 ++-
 t/t9300-fast-import.sh | 88 ++++++++++++++++++++++++++++++++++++++++--
 t/t9350-fast-export.sh |  2 +-
 3 files changed, 88 insertions(+), 7 deletions(-)

builtin/fast-import.c +3 -2

index 3e7ec1f1198..bb4b769c7c3 100644
@@ -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;

t/t9300-fast-import.sh +84 -4

118 lines not on this page · raw message

t/t9350-fast-export.sh +1 -1

index 1eb035ee4ce..bb83e5accd9 100755
@@ -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
 	)

base-commit: 4a2790a257b314ab59f6f2e25f3d7ca120219922

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-11-28 00:22 UTC #1 lore raw

Quoted 22 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>
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 <[email protected]>
---
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/[email protected]/
for discussion of that series.)

Quoted 8 lines

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.

Jeff King

2024-11-28 16:12 UTC #2 lore raw

In reply to Elijah Newren via GitGitGadget

Quoted 10 lines · Elijah Newren via GitGitGadget

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...

Review static int tree_content_set( · line 1418

Quoted 6 lines

(quoted diff) +2 -0

@@ -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

---

builtin/fast-import.c +5 -2

index bb4b769c7c..265d1b7d52 100644
@@ -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,

Elijah Newren via GitGitGadget

author

2024-11-30 01:09 UTC #3 lore raw

PATCH v2 1/1 fast-import: disallow more path components

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Helped-by Jeff King <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---
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/[email protected]/
for discussion of that series.)

Changes since v1:

* Moved the check to a higher level, as suggested by Peff.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1832%2Fnewren%2Fdisallow-verify-path-fast-import-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1832/newren/disallow-verify-path-fast-import-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1832

Range-diff vs v1:

1: 2c23d601112 ! 1: 787e9d71ae7 fast-import: disallow more path components
@@ Commit message
or backslashes within the filename, turn off core.protectNTFS for those tests to ensure they keep passing.

+ Helped-by: Jeff King <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

## builtin/fast-import.c ##
@@ builtin/fast-import.c
#include "refs.h"
#include "csum-file.h"
#include "quote.h"
-@@ builtin/fast-import.c: 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);
@@ builtin/fast-import.c: static int tree_content_set(
root->tree = t = grow_tree_content(t, t->entry_count);
e = new_tree_entry();
@@ builtin/fast-import.c: static int tree_content_set(
e->versions[0].mode = 0;
oidclr(&e->versions[0].oid, the_repository->hash_algo); t->entries[t->entry_count++] = e;
+@@ builtin/fast-import.c: 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);
+ }
+
+@@ builtin/fast-import.c: 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,

## t/t9300-fast-import.sh ##
@@ t/t9300-fast-import.sh: test_expect_success 'B: fail on invalid committer (5)' '

 builtin/fast-import.c  |  8 +++-
 t/t9300-fast-import.sh | 88 ++++++++++++++++++++++++++++++++++++++++--
 t/t9350-fast-export.sh |  2 +-
 3 files changed, 91 insertions(+), 7 deletions(-)

builtin/fast-import.c +6 -2

index 3e7ec1f1198..265d1b7d52b 100644
@@ -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"

@@ -1468,8 +1469,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;

@@ -2416,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);
 }

@@ -2453,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,

t/t9300-fast-import.sh +84 -4

118 lines not on this page · raw message

t/t9350-fast-export.sh +1 -1

index 1eb035ee4ce..bb83e5accd9 100755
@@ -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
 	)

base-commit: 4a2790a257b314ab59f6f2e25f3d7ca120219922

Signature · 1 line

gitgitgadget

Jeff King

2024-12-01 21:40 UTC #4 lore raw

Quoted 3 lines · Elijah Newren via GitGitGadget

Changes since v1:
* Moved the check to a higher level, as suggested by Peff.

Thanks, the code change looks good. Is it worth tweaking one of the tests to do "R innocent-path .git/evil"? Otherwise I don't think there's any coverage of the file_change_cr() call at all.

-Peff

Junio C Hamano

2024-12-02 01:04 UTC #10 lore raw

In reply to Elijah Newren via GitGitGadget

Quoted 15 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>
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.
Helped-by Jeff King <[email protected]>
Signed-off-by Elijah Newren <[email protected]>

Quoted 7 lines

---
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/[email protected]/
for discussion of that series.)

Ah, sorry and thanks.

Will queue.

Elijah Newren

2024-12-03 08:02 UTC #5 lore raw

In reply to Jeff King

Quoted 5 lines · Jeff King

Quoted 2 lines · Elijah Newren via GitGitGadget
Changes since v1:

Quoted 7 lines

Quoted 1 line
* Moved the check to a higher level, as suggested by Peff.
Thanks, the code change looks good. Is it worth tweaking one of the tests to do "R innocent-path .git/evil"? Otherwise I don't think there's any coverage of the file_change_cr() call at all.
-Peff

I would say yes, but since this patch too has made it to next and is marked for master, I'm kinda tempted to just leave it as-is...

Jeff King

2024-12-03 21:07 UTC #6 lore raw

[PATCH 2/1] t9300: test verification of renamed paths

Quoted 4 lines · Elijah Newren

Quoted 4 lines · Elijah Newren via GitGitGadget
Quoted 3 lines
Changes since v1:

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
* Moved the check to a higher level, as suggested by Peff.
Thanks, the code change looks good. Is it worth tweaking one of the tests to do "R innocent-path .git/evil"? Otherwise I don't think there's any coverage of the file_change_cr() call at all.
I would say yes, but since this patch too has made it to next and is marked for master, I'm kinda tempted to just leave it as-is...

Is is tempting. :) I wrote this up, though, which can just go on top (of en/fast-import-verify-path).

-Peff

-- >8 --
Subject: [PATCH] t9300: test verification of renamed paths

Commit da91a90c2f (fast-import: disallow more path components, 2024-11-30) added two separate verify_path() calls (one for added/modified files, and one for renames/copies). But our tests only exercise the first one. Let's protect ourselves against regressions by tweaking one of the tests to rename into the bad path. There are adjacent tests that will stay as additions, so now both calls are covered.

Signed-off-by Jeff King <[email protected]>

---
 t/t9300-fast-import.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

t/t9300-fast-import.sh +9 -2

index e2b1db6bc2..fd01a2353c 100755
@@ -553,9 +553,16 @@ test_expect_success 'B: fail on invalid file path of .' '
 	commit refs/heads/badpath
 	committer Name <email> $GIT_COMMITTER_DATE
 	data <<COMMIT
-	Commit Message
+	Good path
+	COMMIT
+	M 100644 :1 ok-path
+
+	commit refs/heads/badpath
+	committer Name <email> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	Bad path
 	COMMIT
-	M 100644 :1 ./invalid-path
+	R ok-path ./invalid-path
 	INPUT_END
 
 	test_when_finished "git update-ref -d refs/heads/badpath" &&

Signature · 1 line

2.47.1.707.g92f6f18526

Junio C Hamano

2024-12-03 22:17 UTC #9 lore raw

In reply to Elijah Newren

Quoted 8 lines · Elijah Newren

Quoted 7 lines · Jeff King
Quoted 3 lines · Elijah Newren via GitGitGadget
Changes since v1:
* Moved the check to a higher level, as suggested by Peff.

Quoted 8 lines

Quoted 5 lines
Thanks, the code change looks good. Is it worth tweaking one of the tests to do "R innocent-path .git/evil"? Otherwise I don't think there's any coverage of the file_change_cr() call at all.
-Peff
I would say yes, but since this patch too has made it to next and is marked for master, I'm kinda tempted to just leave it as-is...

It is perfectly OK to have a follow-up patch that adds a test or two ;-)

Elijah Newren

2024-12-03 22:23 UTC #7 lore raw

Re: [PATCH 2/1] t9300: test verification of renamed paths

In reply to Jeff King

Quoted 13 lines · Jeff King

Quoted 10 lines · Elijah Newren
Quoted 9 lines · Elijah Newren via GitGitGadget
Quoted 4 lines
Changes since v1:
* Moved the check to a higher level, as suggested by Peff.
Thanks, the code change looks good. Is it worth tweaking one of the tests to do "R innocent-path .git/evil"? Otherwise I don't think there's any coverage of the file_change_cr() call at all.

Quoted 5 lines

Quoted 2 lines
I would say yes, but since this patch too has made it to next and is marked for master, I'm kinda tempted to just leave it as-is...
Is is tempting. :) I wrote this up, though, which can just go on top (of en/fast-import-verify-path).

Thanks!

Quoted 42 lines

-Peff
-- >8 --
Subject: [PATCH] t9300: test verification of renamed paths
Commit da91a90c2f (fast-import: disallow more path components, 2024-11-30) added two separate verify_path() calls (one for added/modified files, and one for renames/copies). But our tests only exercise the first one. Let's protect ourselves against regressions by tweaking one of the tests to rename into the bad path. There are adjacent tests that will stay as additions, so now both calls are covered.
Signed-off-by Jeff King <[email protected]>
---
 t/t9300-fast-import.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
t/t9300-fast-import.sh +9 -3
index e2b1db6bc2..fd01a2353c 100755
@@ -553,9 +553,16 @@ test_expect_success 'B: fail on invalid file path of .' '
        commit refs/heads/badpath
        committer Name <email> $GIT_COMMITTER_DATE
        data <<COMMIT
-       Commit Message
+       Good path
+       COMMIT
+       M 100644 :1 ok-path
+
+       commit refs/heads/badpath
+       committer Name <email> $GIT_COMMITTER_DATE
+       data <<COMMIT
+       Bad path
        COMMIT
-       M 100644 :1 ./invalid-path
+       R ok-path ./invalid-path
        INPUT_END
 
        test_when_finished "git update-ref -d refs/heads/badpath" &&
--

Quoted 1 line

2.47.1.707.g92f6f18526

Change looks good to me.

Junio C Hamano

2024-12-04 00:15 UTC #8 lore raw

Re: [PATCH 2/1] t9300: test verification of renamed paths

In reply to Jeff King

Quoted 5 lines · Jeff King

Quoted 2 lines
I would say yes, but since this patch too has made it to next and is marked for master, I'm kinda tempted to just leave it as-is...
Is is tempting. :) I wrote this up, though, which can just go on top (of en/fast-import-verify-path).

Thanks, queued.

Quoted 39 lines

-- >8 --
Subject: [PATCH] t9300: test verification of renamed paths
Commit da91a90c2f (fast-import: disallow more path components, 2024-11-30) added two separate verify_path() calls (one for added/modified files, and one for renames/copies). But our tests only exercise the first one. Let's protect ourselves against regressions by tweaking one of the tests to rename into the bad path. There are adjacent tests that will stay as additions, so now both calls are covered.
Signed-off-by Jeff King <[email protected]>
---
 t/t9300-fast-import.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
t/t9300-fast-import.sh +9 -2
index e2b1db6bc2..fd01a2353c 100755
@@ -553,9 +553,16 @@ test_expect_success 'B: fail on invalid file path of .' '
 	commit refs/heads/badpath
 	committer Name <email> $GIT_COMMITTER_DATE
 	data <<COMMIT
-	Commit Message
+	Good path
+	COMMIT
+	M 100644 :1 ok-path
+
+	commit refs/heads/badpath
+	committer Name <email> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	Bad path
 	COMMIT
-	M 100644 :1 ./invalid-path
+	R ok-path ./invalid-path
 	INPUT_END
 
 	test_when_finished "git update-ref -d refs/heads/badpath" &&