From 6f054f9fb3a501c35b55c65e547a244f14c38d56 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 28 Jul 2022 17:35:17 -0400 Subject: builtin/clone.c: disallow `--local` clones with symlinks When cloning a repository with `--local`, Git relies on either making a hardlink or copy to every file in the "objects" directory of the source repository. This is done through the callpath `cmd_clone()` -> `clone_local()` -> `copy_or_link_directory()`. The way this optimization works is by enumerating every file and directory recursively in the source repository's `$GIT_DIR/objects` directory, and then either making a copy or hardlink of each file. The only exception to this rule is when copying the "alternates" file, in which case paths are rewritten to be absolute before writing a new "alternates" file in the destination repo. One quirk of this implementation is that it dereferences symlinks when cloning. This behavior was most recently modified in 36596fd2df (clone: better handle symlinked files at .git/objects/, 2019-07-10), which attempted to support `--local` clones of repositories with symlinks in their objects directory in a platform-independent way. Unfortunately, this behavior of dereferencing symlinks (that is, creating a hardlink or copy of the source's link target in the destination repository) can be used as a component in attacking a victim by inadvertently exposing the contents of file stored outside of the repository. Take, for example, a repository that stores a Dockerfile and is used to build Docker images. When building an image, Docker copies the directory contents into the VM, and then instructs the VM to execute the Dockerfile at the root of the copied directory. This protects against directory traversal attacks by copying symbolic links as-is without dereferencing them. That is, if a user has a symlink pointing at their private key material (where the symlink is present in the same directory as the Dockerfile, but the key itself is present outside of that directory), the key is unreadable to a Docker image, since the link will appear broken from the container's point of view. This behavior enables an attack whereby a victim is convinced to clone a repository containing an embedded submodule (with a URL like "file:///proc/self/cwd/path/to/submodule") which has a symlink pointing at a path containing sensitive information on the victim's machine. If a user is tricked into doing this, the contents at the destination of those symbolic links are exposed to the Docker image at runtime. One approach to preventing this behavior is to recreate symlinks in the destination repository. But this is problematic, since symlinking the objects directory are not well-supported. (One potential problem is that when sharing, e.g. a "pack" directory via symlinks, different writers performing garbage collection may consider different sets of objects to be reachable, enabling a situation whereby garbage collecting one repository may remove reachable objects in another repository). Instead, prohibit the local clone optimization when any symlinks are present in the `$GIT_DIR/objects` directory of the source repository. Users may clone the repository again by prepending the "file://" scheme to their clone URL, or by adding the `--no-local` option to their `git clone` invocation. The directory iterator used by `copy_or_link_directory()` must no longer dereference symlinks (i.e., it *must* call `lstat()` instead of `stat()` in order to discover whether or not there are symlinks present). This has no bearing on the overall behavior, since we will immediately `die()` on encounter a symlink. Note that t5604.33 suggests that we do support local clones with symbolic links in the source repository's objects directory, but this was likely unintentional, or at least did not take into consideration the problem with sharing parts of the objects directory with symbolic links at the time. Update this test to reflect which options are and aren't supported. Helped-by: Johannes Schindelin Signed-off-by: Taylor Blau diff --git a/builtin/clone.c b/builtin/clone.c index e335734..e626073 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -420,13 +420,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, int src_len, dest_len; struct dir_iterator *iter; int iter_status; - unsigned int flags; struct strbuf realpath = STRBUF_INIT; mkdir_if_missing(dest->buf, 0777); - flags = DIR_ITERATOR_PEDANTIC | DIR_ITERATOR_FOLLOW_SYMLINKS; - iter = dir_iterator_begin(src->buf, flags); + iter = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC); if (!iter) die_errno(_("failed to start iterator over '%s'"), src->buf); @@ -442,6 +440,10 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, strbuf_setlen(dest, dest_len); strbuf_addstr(dest, iter->relative_path); + if (S_ISLNK(iter->st.st_mode)) + die(_("symlink '%s' exists, refusing to clone with --local"), + iter->relative_path); + if (S_ISDIR(iter->st.st_mode)) { mkdir_if_missing(dest->buf, 0777); continue; diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh index 2f7be23..9d32f1c 100755 --- a/t/t5604-clone-reference.sh +++ b/t/t5604-clone-reference.sh @@ -300,8 +300,6 @@ test_expect_success SYMLINKS 'setup repo with manually symlinked or unknown file ln -s ../an-object $obj && cd ../ && - find . -type f | sort >../../../T.objects-files.raw && - find . -type l | sort >../../../T.objects-symlinks.raw && echo unknown_content >unknown_file ) && git -C T fsck && @@ -310,19 +308,27 @@ test_expect_success SYMLINKS 'setup repo with manually symlinked or unknown file test_expect_success SYMLINKS 'clone repo with symlinked or unknown files at objects/' ' - for option in --local --no-hardlinks --shared --dissociate + # None of these options work when cloning locally, since T has + # symlinks in its `$GIT_DIR/objects` directory + for option in --local --no-hardlinks --dissociate do - git clone $option T T$option || return 1 && - git -C T$option fsck || return 1 && - git -C T$option rev-list --all --objects >T$option.objects && - test_cmp T.objects T$option.objects && - ( - cd T$option/.git/objects && - find . -type f | sort >../../../T$option.objects-files.raw && - find . -type l | sort >../../../T$option.objects-symlinks.raw - ) + test_must_fail git clone $option T T$option 2>err || return 1 && + test_i18ngrep "symlink.*exists" err || return 1 done && + # But `--shared` clones should still work, even when specifying + # a local path *and* that repository has symlinks present in its + # `$GIT_DIR/objects` directory. + git clone --shared T T--shared && + git -C T--shared fsck && + git -C T--shared rev-list --all --objects >T--shared.objects && + test_cmp T.objects T--shared.objects && + ( + cd T--shared/.git/objects && + find . -type f | sort >../../../T--shared.objects-files.raw && + find . -type l | sort >../../../T--shared.objects-symlinks.raw + ) && + for raw in $(ls T*.raw) do sed -e "s!/../!/Y/!; s![0-9a-f]\{38,\}!Z!" -e "/commit-graph/d" \ @@ -330,26 +336,6 @@ test_expect_success SYMLINKS 'clone repo with symlinked or unknown files at obje sort $raw.de-sha-1 >$raw.de-sha || return 1 done && - cat >expected-files <<-EOF && - ./Y/Z - ./Y/Z - ./Y/Z - ./a-loose-dir/Z - ./an-object - ./info/packs - ./pack/pack-Z.idx - ./pack/pack-Z.pack - ./packs/pack-Z.idx - ./packs/pack-Z.pack - ./unknown_file - EOF - - for option in --local --no-hardlinks --dissociate - do - test_cmp expected-files T$option.objects-files.raw.de-sha || return 1 && - test_must_be_empty T$option.objects-symlinks.raw.de-sha || return 1 - done && - echo ./info/alternates >expected-files && test_cmp expected-files T--shared.objects-files.raw && test_must_be_empty T--shared.objects-symlinks.raw -- cgit v0.10.2-6-g49f6 From 7de0c306f7b758d3fb537c18c2751f6250cea7a0 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 29 Jul 2022 15:13:58 -0400 Subject: t/lib-submodule-update.sh: allow local submodules To prepare for changing the default value of `protocol.file.allow` to "user", update the `prolog()` function in lib-submodule-update to allow submodules to be cloned over the file protocol. This is used by a handful of submodule-related test scripts, which themselves will have to tweak the value of `protocol.file.allow` in certain locations. Those will be done in subsequent commits. Signed-off-by: Taylor Blau diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 4b714e9..cc5b58b 100644 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -196,6 +196,7 @@ test_git_directory_exists () { # the submodule repo if it doesn't exist and configures the most problematic # settings for diff.ignoreSubmodules. prolog () { + test_config_global protocol.file.allow always && (test -d submodule_update_repo || create_lib_submodule_repo) && test_config_global diff.ignoreSubmodules all && test_config diff.ignoreSubmodules all -- cgit v0.10.2-6-g49f6 From 8a96dbcb339d25ba1813632319ea4052bc586ddf Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 29 Jul 2022 15:16:10 -0400 Subject: t/t1NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Signed-off-by: Taylor Blau diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 84acfc4..749c8f1 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -449,7 +449,8 @@ test_expect_success 'interaction with submodules' ' ( cd super && mkdir modules && - git submodule add ../repo modules/child && + git -c protocol.file.allow=always \ + submodule add ../repo modules/child && git add . && git commit -m "add submodule" && git sparse-checkout init --cone && diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh index 408b97d..acef9fd 100755 --- a/t/t1500-rev-parse.sh +++ b/t/t1500-rev-parse.sh @@ -163,7 +163,8 @@ test_expect_success 'showing the superproject correctly' ' test_commit -C super test_commit && test_create_repo sub && test_commit -C sub test_commit && - git -C super submodule add ../sub dir/sub && + git -c protocol.file.allow=always \ + -C super submodule add ../sub dir/sub && echo $(pwd)/super >expect && git -C super/dir/sub rev-parse --show-superproject-working-tree >out && test_cmp expect out && -- cgit v0.10.2-6-g49f6 From 99f4abb8dae4c9c604e5d5cf255958bbe537b928 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 29 Jul 2022 15:20:03 -0400 Subject: t/2NNNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Test scripts that rely on submodules throughout use a `git config --global` during a setup test towards the beginning of the script. Signed-off-by: Taylor Blau diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index 5a74954..cd02f78 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -597,6 +597,7 @@ test_expect_success '"add" should not fail because of another bad worktree' ' ' test_expect_success '"add" with uninitialized submodule, with submodule.recurse unset' ' + test_config_global protocol.file.allow always && test_create_repo submodule && test_commit -C submodule first && test_create_repo project && @@ -612,6 +613,7 @@ test_expect_success '"add" with uninitialized submodule, with submodule.recurse ' test_expect_success '"add" with initialized submodule, with submodule.recurse unset' ' + test_config_global protocol.file.allow always && git -C project-clone submodule update --init && git -C project-clone worktree add ../project-4 ' diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh index a4e1a17..e8246ee 100755 --- a/t/t2403-worktree-move.sh +++ b/t/t2403-worktree-move.sh @@ -138,7 +138,8 @@ test_expect_success 'move a repo with uninitialized submodule' ' ( cd withsub && test_commit initial && - git submodule add "$PWD"/.git sub && + git -c protocol.file.allow=always \ + submodule add "$PWD"/.git sub && git commit -m withsub && git worktree add second HEAD && git worktree move second third @@ -148,7 +149,7 @@ test_expect_success 'move a repo with uninitialized submodule' ' test_expect_success 'not move a repo with initialized submodule' ' ( cd withsub && - git -C third submodule update && + git -c protocol.file.allow=always -C third submodule update && test_must_fail git worktree move third forth ) ' @@ -227,6 +228,7 @@ test_expect_success 'remove cleans up .git/worktrees when empty' ' ' test_expect_success 'remove a repo with uninitialized submodule' ' + test_config_global protocol.file.allow always && ( cd withsub && git worktree add to-remove HEAD && @@ -235,6 +237,7 @@ test_expect_success 'remove a repo with uninitialized submodule' ' ' test_expect_success 'not remove a repo with initialized submodule' ' + test_config_global protocol.file.allow always && ( cd withsub && git worktree add to-remove HEAD && diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh index e1b2bfd..51120d5 100755 --- a/t/t2405-worktree-submodule.sh +++ b/t/t2405-worktree-submodule.sh @@ -7,6 +7,7 @@ test_description='Combination of submodules and multiple worktrees' base_path=$(pwd -P) test_expect_success 'setup: create origin repos' ' + git config --global protocol.file.allow always && git init origin/sub && test_commit -C origin/sub file1 && git init origin/main && -- cgit v0.10.2-6-g49f6 From f8d510ed0b357787c8d035d64f240bd82b424dc4 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 29 Jul 2022 15:20:28 -0400 Subject: t/t3NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Test scripts that rely on submodules throughout use a `git config --global` during a setup test towards the beginning of the script. Signed-off-by: Taylor Blau diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 3ec3e1d..631a0b5 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -279,6 +279,7 @@ test_expect_success 'deleting checked-out branch from repo that is a submodule' git init repo1 && git init repo1/sub && test_commit -C repo1/sub x && + test_config_global protocol.file.allow always && git -C repo1 submodule add ./sub && git -C repo1 commit -m "adding sub" && diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh index ca33173..80df13a 100755 --- a/t/t3420-rebase-autostash.sh +++ b/t/t3420-rebase-autostash.sh @@ -307,7 +307,7 @@ test_expect_success 'autostash is saved on editor failure with conflict' ' test_expect_success 'autostash with dirty submodules' ' test_when_finished "git reset --hard && git checkout master" && git checkout -b with-submodule && - git submodule add ./ sub && + git -c protocol.file.allow=always submodule add ./ sub && test_tick && git commit -m add-submodule && echo changed >sub/file0 && diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh index 0ad3a07..fb21f67 100755 --- a/t/t3426-rebase-submodule.sh +++ b/t/t3426-rebase-submodule.sh @@ -47,7 +47,8 @@ test_expect_success 'rebase interactive ignores modified submodules' ' git init sub && git -C sub commit --allow-empty -m "Initial commit" && git init super && - git -C super submodule add ../sub && + git -c protocol.file.allow=always \ + -C super submodule add ../sub && git -C super config submodule.sub.ignore dirty && >super/foo && git -C super add foo && diff --git a/t/t3512-cherry-pick-submodule.sh b/t/t3512-cherry-pick-submodule.sh index 6ece1d8..697bc68 100755 --- a/t/t3512-cherry-pick-submodule.sh +++ b/t/t3512-cherry-pick-submodule.sh @@ -10,6 +10,8 @@ KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1 test_submodule_switch "cherry-pick" test_expect_success 'unrelated submodule/file conflict is ignored' ' + test_config_global protocol.file.allow always && + test_create_repo sub && touch sub/file && diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index efec8d1..99dab76 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -321,7 +321,7 @@ test_expect_success 'rm removes empty submodules from work tree' ' test_expect_success 'rm removes removed submodule from index and .gitmodules' ' git reset --hard && - git submodule update && + git -c protocol.file.allow=always submodule update && rm -rf submod && git rm submod && git status -s -uno --ignore-submodules=none >actual && @@ -627,6 +627,7 @@ cat >expect.deepmodified < Date: Fri, 29 Jul 2022 15:20:43 -0400 Subject: t/t4NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Test scripts that rely on submodules throughout use a `git config --global` during a setup test towards the beginning of the script. Signed-off-by: Taylor Blau diff --git a/t/t4059-diff-submodule-not-initialized.sh b/t/t4059-diff-submodule-not-initialized.sh index 49bca7b..d489230 100755 --- a/t/t4059-diff-submodule-not-initialized.sh +++ b/t/t4059-diff-submodule-not-initialized.sh @@ -49,7 +49,7 @@ test_expect_success 'setup - submodules' ' ' test_expect_success 'setup - git submodule add' ' - git submodule add ./sm2 sm1 && + git -c protocol.file.allow=always submodule add ./sm2 sm1 && commit_file sm1 .gitmodules && git diff-tree -p --no-commit-id --submodule=log HEAD -- sm1 >actual && cat >expected <<-EOF && diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh index fc8229c..57b1912 100755 --- a/t/t4060-diff-submodule-option-diff-format.sh +++ b/t/t4060-diff-submodule-option-diff-format.sh @@ -759,9 +759,9 @@ test_expect_success 'diff --submodule=diff with .git file' ' ' test_expect_success 'setup nested submodule' ' - git submodule add -f ./sm2 && + git -c protocol.file.allow=always submodule add -f ./sm2 && git commit -a -m "add sm2" && - git -C sm2 submodule add ../sm2 nested && + git -c protocol.file.allow=always -C sm2 submodule add ../sm2 nested && git -C sm2 commit -a -m "nested sub" && head10=$(git -C sm2 rev-parse --short --verify HEAD) ' diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh index 804f2a8..28f42a4 100755 --- a/t/t4067-diff-partial-clone.sh +++ b/t/t4067-diff-partial-clone.sh @@ -77,6 +77,7 @@ test_expect_success 'diff skips same-OID blobs' ' test_expect_success 'when fetching missing objects, diff skips GITLINKs' ' test_when_finished "rm -rf sub server client trace" && + test_config_global protocol.file.allow always && test_create_repo sub && test_commit -C sub first && diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index 6cdbe47..aeaf0d5 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -126,6 +126,7 @@ test_expect_success 'command line pathspec parsing for "git log"' ' test_expect_success 'tree_entry_interesting does not match past submodule boundaries' ' test_when_finished "rm -rf repo submodule" && + test_config_global protocol.file.allow always && git init submodule && test_commit -C submodule initial && git init repo && -- cgit v0.10.2-6-g49f6 From 225d2d50ccef4baae410a96b9dc9e3978d164826 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 29 Jul 2022 15:21:06 -0400 Subject: t/t5NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Test scripts that rely on submodules throughout use a `git config --global` during a setup test towards the beginning of the script. Signed-off-by: Taylor Blau diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 2013051..b60ba0f 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -627,6 +627,7 @@ test_expect_success 'fetch.writeCommitGraph' ' ' test_expect_success 'fetch.writeCommitGraph with submodules' ' + test_config_global protocol.file.allow always && git clone dups super && ( cd super && diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index 53d7b8e..aa04eac 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -35,6 +35,7 @@ add_upstream_commit() { } test_expect_success setup ' + git config --global protocol.file.allow always && mkdir deepsubmodule && ( cd deepsubmodule && diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh index 38e6f73..77ce917 100755 --- a/t/t5545-push-options.sh +++ b/t/t5545-push-options.sh @@ -113,6 +113,7 @@ test_expect_success 'push options and submodules' ' test_commit -C parent one && git -C parent push --mirror up && + test_config_global protocol.file.allow always && git -C parent submodule add ../upstream workbench && git -C parent/workbench remote add up ../../upstream && git -C parent commit -m "add submodule" && diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh index 37fd06b..9b3b4af 100755 --- a/t/t5572-pull-submodule.sh +++ b/t/t5572-pull-submodule.sh @@ -46,6 +46,10 @@ KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1 test_submodule_switch_func "git_pull_noff" +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' + test_expect_success 'pull --recurse-submodule setup' ' test_create_repo child && test_commit -C child bar && diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 7df3c53..50d4821 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -738,6 +738,7 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe echo aa >server/a && echo bb >server/b && # Also add a gitlink pointing to an arbitrary repository + test_config_global protocol.file.allow always && git -C server submodule add "$(pwd)/repo_for_submodule" c && git -C server add a b c && git -C server commit -m x && diff --git a/t/t5614-clone-submodules-shallow.sh b/t/t5614-clone-submodules-shallow.sh index e4e6ea4..d361738 100755 --- a/t/t5614-clone-submodules-shallow.sh +++ b/t/t5614-clone-submodules-shallow.sh @@ -24,6 +24,7 @@ test_expect_success 'setup' ' test_expect_success 'nonshallow clone implies nonshallow submodule' ' test_when_finished "rm -rf super_clone" && + test_config_global protocol.file.allow always && git clone --recurse-submodules "file://$pwd/." super_clone && git -C super_clone log --oneline >lines && test_line_count = 3 lines && @@ -33,6 +34,7 @@ test_expect_success 'nonshallow clone implies nonshallow submodule' ' test_expect_success 'shallow clone with shallow submodule' ' test_when_finished "rm -rf super_clone" && + test_config_global protocol.file.allow always && git clone --recurse-submodules --depth 2 --shallow-submodules "file://$pwd/." super_clone && git -C super_clone log --oneline >lines && test_line_count = 2 lines && @@ -42,6 +44,7 @@ test_expect_success 'shallow clone with shallow submodule' ' test_expect_success 'shallow clone does not imply shallow submodule' ' test_when_finished "rm -rf super_clone" && + test_config_global protocol.file.allow always && git clone --recurse-submodules --depth 2 "file://$pwd/." super_clone && git -C super_clone log --oneline >lines && test_line_count = 2 lines && @@ -51,6 +54,7 @@ test_expect_success 'shallow clone does not imply shallow submodule' ' test_expect_success 'shallow clone with non shallow submodule' ' test_when_finished "rm -rf super_clone" && + test_config_global protocol.file.allow always && git clone --recurse-submodules --depth 2 --no-shallow-submodules "file://$pwd/." super_clone && git -C super_clone log --oneline >lines && test_line_count = 2 lines && @@ -60,6 +64,7 @@ test_expect_success 'shallow clone with non shallow submodule' ' test_expect_success 'non shallow clone with shallow submodule' ' test_when_finished "rm -rf super_clone" && + test_config_global protocol.file.allow always && git clone --recurse-submodules --no-local --shallow-submodules "file://$pwd/." super_clone && git -C super_clone log --oneline >lines && test_line_count = 3 lines && @@ -69,6 +74,7 @@ test_expect_success 'non shallow clone with shallow submodule' ' test_expect_success 'clone follows shallow recommendation' ' test_when_finished "rm -rf super_clone" && + test_config_global protocol.file.allow always && git config -f .gitmodules submodule.sub.shallow true && git add .gitmodules && git commit -m "recommend shallow for sub" && @@ -87,6 +93,7 @@ test_expect_success 'clone follows shallow recommendation' ' test_expect_success 'get unshallow recommended shallow submodule' ' test_when_finished "rm -rf super_clone" && + test_config_global protocol.file.allow always && git clone --no-local "file://$pwd/." super_clone && ( cd super_clone && @@ -103,6 +110,7 @@ test_expect_success 'get unshallow recommended shallow submodule' ' test_expect_success 'clone follows non shallow recommendation' ' test_when_finished "rm -rf super_clone" && + test_config_global protocol.file.allow always && git config -f .gitmodules submodule.sub.shallow false && git add .gitmodules && git commit -m "recommend non shallow for sub" && diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index d98c550..a6138e8 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -171,6 +171,8 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 works with submod test_config -C src_with_sub uploadpack.allowfilter 1 && test_config -C src_with_sub uploadpack.allowanysha1inwant 1 && + test_config_global protocol.file.allow always && + git -C src_with_sub submodule add "file://$(pwd)/submodule" mysub && git -C src_with_sub commit -m "commit with submodule" && diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh index 1a041df..0152dc0 100755 --- a/t/t5617-clone-submodules-remote.sh +++ b/t/t5617-clone-submodules-remote.sh @@ -7,6 +7,7 @@ test_description='Test cloning repos with submodules using remote-tracking branc pwd=$(pwd) test_expect_success 'setup' ' + git config --global protocol.file.allow always && git checkout -b master && test_commit commit1 && mkdir sub && -- cgit v0.10.2-6-g49f6 From 0f21b8f468566b991eea60bb7bdf2fce9265e367 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 29 Jul 2022 15:21:18 -0400 Subject: t/t6NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Signed-off-by: Taylor Blau diff --git a/t/t6008-rev-list-submodule.sh b/t/t6008-rev-list-submodule.sh index c4af9ca..a65e5f2 100755 --- a/t/t6008-rev-list-submodule.sh +++ b/t/t6008-rev-list-submodule.sh @@ -23,7 +23,7 @@ test_expect_success 'setup' ' : > super-file && git add super-file && - git submodule add "$(pwd)" sub && + git -c protocol.file.allow=always submodule add "$(pwd)" sub && git symbolic-ref HEAD refs/heads/super && test_tick && git commit -m super-initial && diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh index c670668..2fde65b 100755 --- a/t/t6134-pathspec-in-submodule.sh +++ b/t/t6134-pathspec-in-submodule.sh @@ -9,7 +9,7 @@ test_expect_success 'setup a submodule' ' : >pretzel/a && git -C pretzel add a && git -C pretzel commit -m "add a file" -- a && - git submodule add ./pretzel sub && + git -c protocol.file.allow=always submodule add ./pretzel sub && git commit -a -m "add submodule" && git submodule deinit --all ' -- cgit v0.10.2-6-g49f6 From 0d3beb71dad7906f576b0de9cea32164549163fe Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 29 Jul 2022 15:21:40 -0400 Subject: t/t7NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Test scripts that rely on submodules throughout use a `git config --global` during a setup test towards the beginning of the script. Signed-off-by: Taylor Blau diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 63d5f41..6156eeb 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -307,6 +307,7 @@ test_expect_success SYMLINKS 'check moved symlink' ' rm -f moved symlink test_expect_success 'setup submodule' ' + test_config_global protocol.file.allow always && git commit -m initial && git reset --hard && git submodule add ./. sub && @@ -513,6 +514,7 @@ test_expect_success 'moving a submodule in nested directories' ' ' test_expect_success 'moving nested submodules' ' + test_config_global protocol.file.allow always && git commit -am "cleanup commit" && mkdir sub_nested_nested && (cd sub_nested_nested && diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh index 601b478..2fdb6ec 100755 --- a/t/t7064-wtstatus-pv2.sh +++ b/t/t7064-wtstatus-pv2.sh @@ -462,6 +462,7 @@ test_expect_success 'create and add submodule, submodule appears clean (A. S...) git checkout initial-branch && git clone . sub_repo && git clone . super_repo && + test_config_global protocol.file.allow always && ( cd super_repo && git submodule add ../sub_repo sub1 && diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh index cb5e34d..713f113 100755 --- a/t/t7300-clean.sh +++ b/t/t7300-clean.sh @@ -480,6 +480,7 @@ test_expect_success 'should not clean submodules' ' git init && test_commit msg hello.world ) && + test_config_global protocol.file.allow always && git submodule add ./repo/.git sub1 && git commit -m "sub1" && git branch before_sub2 && diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index fec7e02..bf1a4df 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -11,6 +11,10 @@ subcommands of git submodule. . ./test-lib.sh +test_expect_success 'setup - enable local submodules' ' + git config --global protocol.file.allow always +' + test_expect_success 'submodule deinit works on empty repository' ' git submodule deinit --all ' diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh index 0726799..3bc904b 100755 --- a/t/t7403-submodule-sync.sh +++ b/t/t7403-submodule-sync.sh @@ -11,6 +11,8 @@ These tests exercise the "git submodule sync" subcommand. . ./test-lib.sh test_expect_success setup ' + git config --global protocol.file.allow always && + echo file >file && git add file && test_tick && diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index acb8766..328b9b7 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -22,6 +22,7 @@ compare_head() test_expect_success 'setup a submodule tree' ' + git config --global protocol.file.allow always && echo file > file && git add file && test_tick && diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh index 6b2aa91..a3404ac 100755 --- a/t/t7407-submodule-foreach.sh +++ b/t/t7407-submodule-foreach.sh @@ -13,6 +13,7 @@ that are currently checked out. test_expect_success 'setup a submodule tree' ' + git config --global protocol.file.allow always && echo file > file && git add file && test_tick && diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh index a3892f4..02feb85 100755 --- a/t/t7408-submodule-reference.sh +++ b/t/t7408-submodule-reference.sh @@ -17,6 +17,10 @@ test_alternate_is_used () { test_cmp expect actual } +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' + test_expect_success 'preparing first repository' ' test_create_repo A && ( diff --git a/t/t7409-submodule-detached-work-tree.sh b/t/t7409-submodule-detached-work-tree.sh index fc018e3..e12eed5 100755 --- a/t/t7409-submodule-detached-work-tree.sh +++ b/t/t7409-submodule-detached-work-tree.sh @@ -12,6 +12,10 @@ on detached working trees TEST_NO_CREATE_REPO=1 . ./test-lib.sh +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' + test_expect_success 'submodule on detached working tree' ' git init --bare remote && test_create_repo bundle1 && diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index ad28e93..c583c4e 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -12,6 +12,9 @@ from the database and from the worktree works. TEST_NO_CREATE_REPO=1 . ./test-lib.sh +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' test_expect_success 'submodule config cache setup' ' mkdir submodule && (cd submodule && diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh index c8e7e98..c8b5ac2 100755 --- a/t/t7413-submodule-is-active.sh +++ b/t/t7413-submodule-is-active.sh @@ -9,6 +9,7 @@ submodules which are "active" and interesting to the user. . ./test-lib.sh test_expect_success 'setup' ' + git config --global protocol.file.allow always && git init sub && test_commit -C sub initial && git init super && diff --git a/t/t7414-submodule-mistakes.sh b/t/t7414-submodule-mistakes.sh index f2e7df5..cf95603 100755 --- a/t/t7414-submodule-mistakes.sh +++ b/t/t7414-submodule-mistakes.sh @@ -30,7 +30,8 @@ test_expect_success 'no warning when updating entry' ' test_expect_success 'submodule add does not warn' ' test_when_finished "git rm -rf submodule .gitmodules" && - git submodule add ./embed submodule 2>stderr && + git -c protocol.file.allow=always \ + submodule add ./embed submodule 2>stderr && test_i18ngrep ! warning stderr ' diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh index f70368b..f37456f 100755 --- a/t/t7415-submodule-names.sh +++ b/t/t7415-submodule-names.sh @@ -8,6 +8,10 @@ real-world setup that confirms we catch this in practice. . ./test-lib.sh . "$TEST_DIRECTORY"/lib-pack.sh +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' + test_expect_success 'check names' ' cat >expect <<-\EOF && valid diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh index d21dc8b..3ebd985 100755 --- a/t/t7416-submodule-dash-url.sh +++ b/t/t7416-submodule-dash-url.sh @@ -3,6 +3,10 @@ test_description='check handling of disallowed .gitmodule urls' . ./test-lib.sh +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' + test_expect_success 'create submodule with protected dash in url' ' git init upstream && git -C upstream commit --allow-empty -m base && diff --git a/t/t7417-submodule-path-url.sh b/t/t7417-submodule-path-url.sh index f7e7e94..b17d180 100755 --- a/t/t7417-submodule-path-url.sh +++ b/t/t7417-submodule-path-url.sh @@ -3,6 +3,10 @@ test_description='check handling of .gitmodule path with dash' . ./test-lib.sh +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' + test_expect_success 'create submodule with dash in path' ' git init upstream && git -C upstream commit --allow-empty -m base && diff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh index 3f7f2718..16331c3 100755 --- a/t/t7418-submodule-sparse-gitmodules.sh +++ b/t/t7418-submodule-sparse-gitmodules.sh @@ -14,6 +14,10 @@ also by committing .gitmodules and then just removing it from the filesystem. . ./test-lib.sh +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' + test_expect_success 'sparse checkout setup which hides .gitmodules' ' git init upstream && git init submodule && diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh index 3b925c3..5357093 100755 --- a/t/t7419-submodule-set-branch.sh +++ b/t/t7419-submodule-set-branch.sh @@ -12,6 +12,10 @@ as expected. TEST_NO_CREATE_REPO=1 . ./test-lib.sh +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' + test_expect_success 'submodule config cache setup' ' mkdir submodule && (cd submodule && diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh index ef0cb6e..d6bf62b 100755 --- a/t/t7420-submodule-set-url.sh +++ b/t/t7420-submodule-set-url.sh @@ -12,6 +12,10 @@ as expected. TEST_NO_CREATE_REPO=1 . ./test-lib.sh +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' + test_expect_success 'submodule config cache setup' ' mkdir submodule && ( diff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh index b070f13..ce64d8b 100755 --- a/t/t7421-submodule-summary-add.sh +++ b/t/t7421-submodule-summary-add.sh @@ -12,6 +12,10 @@ while making sure to add submodules using `git submodule add` instead of . ./test-lib.sh +test_expect_success 'setup' ' + git config --global protocol.file.allow always +' + test_expect_success 'summary test environment setup' ' git init sm && test_commit -C sm "add file" file file-content file-tag && diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh index 3fcb447..459300c 100755 --- a/t/t7506-status-submodule.sh +++ b/t/t7506-status-submodule.sh @@ -251,6 +251,7 @@ test_expect_success 'status with merge conflict in .gitmodules' ' test_create_repo_with_commit sub1 && test_tick && test_create_repo_with_commit sub2 && + test_config_global protocol.file.allow always && ( cd super && prev=$(git rev-parse HEAD) && @@ -326,6 +327,7 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' ' # sub2 will have an untracked file # sub3 will have an untracked repository test_expect_success 'setup superproject with untracked file in nested submodule' ' + test_config_global protocol.file.allow always && ( cd super && git clean -dfx && diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index ed2653d..bd0ae4b 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -74,6 +74,7 @@ test_expect_success 'diff in message is retained with -v' ' test_expect_success 'submodule log is stripped out too with -v' ' git config diff.submodule log && + test_config_global protocol.file.allow always && git submodule add ./. sub && git commit -m "sub added" && ( diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index a578b35..cca5069 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -626,6 +626,7 @@ test_expect_success 'difftool --no-symlinks detects conflict ' ' test_expect_success 'difftool properly honors gitlink and core.worktree' ' test_when_finished rm -rf submod/ule && + test_config_global protocol.file.allow always && git submodule add ./. submod/ule && test_config -C submod/ule diff.tool checktrees && test_config -C submod/ule difftool.checktrees.cmd '\'' diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh index 828cb3b..f465c0d 100755 --- a/t/t7814-grep-recurse-submodules.sh +++ b/t/t7814-grep-recurse-submodules.sh @@ -193,6 +193,7 @@ test_expect_success !MINGW 'grep recurse submodule colon in name' ' git -C "su:b" commit -m "add fi:le" && test_tick && + test_config_global protocol.file.allow always && git -C parent submodule add "../su:b" "su:b" && git -C parent commit -m "add submodule" && test_tick && @@ -227,6 +228,7 @@ test_expect_success 'grep history with moved submoules' ' git -C sub commit -m "add file" && test_tick && + test_config_global protocol.file.allow always && git -C parent submodule add ../sub dir/sub && git -C parent commit -m "add submodule" && test_tick && @@ -271,6 +273,7 @@ test_expect_success 'grep using relative path' ' mkdir parent/src && echo "(1|2)d(3|4)" >parent/src/file2 && git -C parent add src/file2 && + test_config_global protocol.file.allow always && git -C parent submodule add ../sub && git -C parent commit -m "add files and submodule" && test_tick && @@ -313,6 +316,7 @@ test_expect_success 'grep from a subdir' ' mkdir parent/src && echo "(1|2)d(3|4)" >parent/src/file && git -C parent add src/file && + test_config_global protocol.file.allow always && git -C parent submodule add ../sub src/sub && git -C parent submodule add ../sub sub && git -C parent commit -m "add files and submodules" && -- cgit v0.10.2-6-g49f6 From f4a32a550f9d40471fb42ed1e5c8612dfe4a83b1 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 29 Jul 2022 15:21:53 -0400 Subject: t/t9NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that interact with submodules a handful of times use `test_config_global`. Signed-off-by: Taylor Blau diff --git a/t/t9304-fast-import-marks.sh b/t/t9304-fast-import-marks.sh index d4359db..73f3ca2 100755 --- a/t/t9304-fast-import-marks.sh +++ b/t/t9304-fast-import-marks.sh @@ -25,6 +25,7 @@ test_expect_success 'import with large marks file' ' ' test_expect_success 'setup dump with submodule' ' + test_config_global protocol.file.allow always && git submodule add "$PWD" sub && git commit -m "add submodule" && git fast-export HEAD >dump diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 1372842..703428f 100755 --- a/t/t9350-fast-export.sh +++ b/t/t9350-fast-export.sh @@ -265,6 +265,7 @@ test_expect_success 'signed-tags=warn-strip' ' test_expect_success 'setup submodule' ' + test_config_global protocol.file.allow always && git checkout -f master && mkdir sub && ( @@ -290,6 +291,7 @@ test_expect_success 'setup submodule' ' test_expect_success 'submodule fast-export | fast-import' ' + test_config_global protocol.file.allow always && SUBENT1=$(git ls-tree master^ sub) && SUBENT2=$(git ls-tree master sub) && rm -rf new && -- cgit v0.10.2-6-g49f6 From a1d4f67c12ac172f835e6d5e4e0a197075e2146b Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 29 Jul 2022 15:22:13 -0400 Subject: transport: make `protocol.file.allow` be "user" by default An earlier patch discussed and fixed a scenario where Git could be used as a vector to exfiltrate sensitive data through a Docker container when a potential victim clones a suspicious repository with local submodules that contain symlinks. That security hole has since been plugged, but a similar one still exists. Instead of convincing a would-be victim to clone an embedded submodule via the "file" protocol, an attacker could convince an individual to clone a repository that has a submodule pointing to a valid path on the victim's filesystem. For example, if an individual (with username "foo") has their home directory ("/home/foo") stored as a Git repository, then an attacker could exfiltrate data by convincing a victim to clone a malicious repository containing a submodule pointing at "/home/foo/.git" with `--recurse-submodules`. Doing so would expose any sensitive contents in stored in "/home/foo" tracked in Git. For systems (such as Docker) that consider everything outside of the immediate top-level working directory containing a Dockerfile as inaccessible to the container (with the exception of volume mounts, and so on), this is a violation of trust by exposing unexpected contents in the working copy. To mitigate the likelihood of this kind of attack, adjust the "file://" protocol's default policy to be "user" to prevent commands that execute without user input (including recursive submodule initialization) from taking place by default. Suggested-by: Jeff King Signed-off-by: Taylor Blau diff --git a/Documentation/config/protocol.txt b/Documentation/config/protocol.txt index 756591d..7993891 100644 --- a/Documentation/config/protocol.txt +++ b/Documentation/config/protocol.txt @@ -1,10 +1,10 @@ protocol.allow:: If set, provide a user defined default policy for all protocols which don't explicitly have a policy (`protocol..allow`). By default, - if unset, known-safe protocols (http, https, git, ssh, file) have a + if unset, known-safe protocols (http, https, git, ssh) have a default policy of `always`, known-dangerous protocols (ext) have a - default policy of `never`, and all other protocols have a default - policy of `user`. Supported policies: + default policy of `never`, and all other protocols (including file) + have a default policy of `user`. Supported policies: + -- diff --git a/transport.c b/transport.c index 679a35e..d2e3a90 100644 --- a/transport.c +++ b/transport.c @@ -964,8 +964,7 @@ static enum protocol_allow_config get_protocol_config(const char *type) if (!strcmp(type, "http") || !strcmp(type, "https") || !strcmp(type, "git") || - !strcmp(type, "ssh") || - !strcmp(type, "file")) + !strcmp(type, "ssh")) return PROTOCOL_ALLOW_ALWAYS; /* known scary; err on the side of caution */ -- cgit v0.10.2-6-g49f6 From 32696a4cbe90929ae79ea442f5102c513ce3dfaa Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 28 Sep 2022 18:50:36 -0400 Subject: shell: add basic tests We have no tests of even basic functionality of git-shell. Let's add a couple of obvious ones. This will serve as a framework for adding tests for new things we fix, as well as making sure we don't screw anything up too badly while doing so. Signed-off-by: Jeff King Signed-off-by: Taylor Blau diff --git a/t/t9850-shell.sh b/t/t9850-shell.sh new file mode 100755 index 0000000..2af476c --- /dev/null +++ b/t/t9850-shell.sh @@ -0,0 +1,31 @@ +#!/bin/sh + +test_description='git shell tests' +. ./test-lib.sh + +test_expect_success 'shell allows upload-pack' ' + printf 0000 >input && + git upload-pack . expect && + git shell -c "git-upload-pack $SQ.$SQ" actual && + test_cmp expect actual +' + +test_expect_success 'shell forbids other commands' ' + test_must_fail git shell -c "git config foo.bar baz" +' + +test_expect_success 'shell forbids interactive use by default' ' + test_must_fail git shell +' + +test_expect_success 'shell allows interactive command' ' + mkdir git-shell-commands && + write_script git-shell-commands/ping <<-\EOF && + echo pong + EOF + echo pong >expect && + echo ping | git shell >actual && + test_cmp expect actual +' + +test_done -- cgit v0.10.2-6-g49f6 From 71ad7fe1bcec2a115bd0ab187240348358aa7f21 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 28 Sep 2022 18:52:48 -0400 Subject: shell: limit size of interactive commands When git-shell is run in interactive mode (which must be enabled by creating $HOME/git-shell-commands), it reads commands from stdin, one per line, and executes them. We read the commands with git_read_line_interactively(), which uses a strbuf under the hood. That means we'll accept an input of arbitrary size (limited only by how much heap we can allocate). That creates two problems: - the rest of the code is not prepared to handle large inputs. The most serious issue here is that split_cmdline() uses "int" for most of its types, which can lead to integer overflow and out-of-bounds array reads and writes. But even with that fixed, we assume that we can feed the command name to snprintf() (via xstrfmt()), which is stuck for historical reasons using "int", and causes it to fail (and even trigger a BUG() call). - since the point of git-shell is to take input from untrusted or semi-trusted clients, it's a mild denial-of-service. We'll allocate as many bytes as the client sends us (actually twice as many, since we immediately duplicate the buffer). We can fix both by just limiting the amount of per-command input we're willing to receive. We should also fix split_cmdline(), of course, which is an accident waiting to happen, but that can come on top. Most calls to split_cmdline(), including the other one in git-shell, are OK because they are reading from an OS-provided argv, which is limited in practice. This patch should eliminate the immediate vulnerabilities. I picked 4MB as an arbitrary limit. It's big enough that nobody should ever run into it in practice (since the point is to run the commands via exec, we're subject to OS limits which are typically much lower). But it's small enough that allocating it isn't that big a deal. The code is mostly just swapping out fgets() for the strbuf call, but we have to add a few niceties like flushing and trimming line endings. We could simplify things further by putting the buffer on the stack, but 4MB is probably a bit much there. Note that we'll _always_ allocate 4MB, which for normal, non-malicious requests is more than we would before this patch. But on the other hand, other git programs are happy to use 96MB for a delta cache. And since we'd never touch most of those pages, on a lazy-allocating OS like Linux they won't even get allocated to actual RAM. The ideal would be a version of strbuf_getline() that accepted a maximum value. But for a minimal vulnerability fix, let's keep things localized and simple. We can always refactor further on top. The included test fails in an obvious way with ASan or UBSan (which notice the integer overflow and out-of-bounds reads). Without them, it fails in a less obvious way: we may segfault, or we may try to xstrfmt() a long string, leading to a BUG(). Either way, it fails reliably before this patch, and passes with it. Note that we don't need an EXPENSIVE prereq on it. It does take 10-15s to fail before this patch, but with the new limit, we fail almost immediately (and the perl process generating 2GB of data exits via SIGPIPE). Signed-off-by: Jeff King Signed-off-by: Taylor Blau diff --git a/shell.c b/shell.c index cef7ffd..02cfd96 100644 --- a/shell.c +++ b/shell.c @@ -47,6 +47,8 @@ static void cd_to_homedir(void) die("could not chdir to user's home directory"); } +#define MAX_INTERACTIVE_COMMAND (4*1024*1024) + static void run_shell(void) { int done = 0; @@ -67,22 +69,46 @@ static void run_shell(void) run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE); do { - struct strbuf line = STRBUF_INIT; const char *prog; char *full_cmd; char *rawargs; + size_t len; char *split_args; const char **argv; int code; int count; fprintf(stderr, "git> "); - if (git_read_line_interactively(&line) == EOF) { + + /* + * Avoid using a strbuf or git_read_line_interactively() here. + * We don't want to allocate arbitrary amounts of memory on + * behalf of a possibly untrusted client, and we're subject to + * OS limits on command length anyway. + */ + fflush(stdout); + rawargs = xmalloc(MAX_INTERACTIVE_COMMAND); + if (!fgets(rawargs, MAX_INTERACTIVE_COMMAND, stdin)) { fprintf(stderr, "\n"); - strbuf_release(&line); + free(rawargs); break; } - rawargs = strbuf_detach(&line, NULL); + len = strlen(rawargs); + + /* + * If we truncated due to our input buffer size, reject the + * command. That's better than running bogus input, and + * there's a good chance it's just malicious garbage anyway. + */ + if (len >= MAX_INTERACTIVE_COMMAND - 1) + die("invalid command format: input too long"); + + if (len > 0 && rawargs[len - 1] == '\n') { + if (--len > 0 && rawargs[len - 1] == '\r') + --len; + rawargs[len] = '\0'; + } + split_args = xstrdup(rawargs); count = split_cmdline(split_args, &argv); if (count < 0) { diff --git a/t/t9850-shell.sh b/t/t9850-shell.sh index 2af476c..cfc71c3 100755 --- a/t/t9850-shell.sh +++ b/t/t9850-shell.sh @@ -28,4 +28,10 @@ test_expect_success 'shell allows interactive command' ' test_cmp expect actual ' +test_expect_success 'shell complains of overlong commands' ' + perl -e "print \"a\" x 2**12 for (0..2**19)" | + test_must_fail git shell 2>err && + grep "too long" err +' + test_done -- cgit v0.10.2-6-g49f6 From 0ca6ead81edd4fb1984b69aae87c1189e3025530 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Wed, 28 Sep 2022 18:53:32 -0400 Subject: alias.c: reject too-long cmdline strings in split_cmdline() This function improperly uses an int to represent the number of entries in the resulting argument array. This allows a malicious actor to intentionally overflow the return value, leading to arbitrary heap writes. Because the resulting argv array is typically passed to execv(), it may be possible to leverage this attack to gain remote code execution on a victim machine. This was almost certainly the case for certain configurations of git-shell until the previous commit limited the size of input it would accept. Other calls to split_cmdline() are typically limited by the size of argv the OS is willing to hand us, so are similarly protected. So this is not strictly fixing a known vulnerability, but is a hardening of the function that is worth doing to protect against possible unknown vulnerabilities. One approach to fixing this would be modifying the signature of `split_cmdline()` to look something like: int split_cmdline(char *cmdline, const char ***argv, size_t *argc); Where the return value of `split_cmdline()` is negative for errors, and zero otherwise. If non-NULL, the `*argc` pointer is modified to contain the size of the `**argv` array. But this implies an absurdly large `argv` array, which more than likely larger than the system's argument limit. So even if split_cmdline() allowed this, it would fail immediately afterwards when we called execv(). So instead of converting all of `split_cmdline()`'s callers to work with `size_t` types in this patch, instead pursue the minimal fix here to prevent ever returning an array with more than INT_MAX entries in it. Signed-off-by: Kevin Backhouse Signed-off-by: Taylor Blau Signed-off-by: Jeff King Signed-off-by: Taylor Blau diff --git a/alias.c b/alias.c index c471538..00abde0 100644 --- a/alias.c +++ b/alias.c @@ -46,14 +46,16 @@ void list_aliases(struct string_list *list) #define SPLIT_CMDLINE_BAD_ENDING 1 #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 +#define SPLIT_CMDLINE_ARGC_OVERFLOW 3 static const char *split_cmdline_errors[] = { N_("cmdline ends with \\"), - N_("unclosed quote") + N_("unclosed quote"), + N_("too many arguments"), }; int split_cmdline(char *cmdline, const char ***argv) { - int src, dst, count = 0, size = 16; + size_t src, dst, count = 0, size = 16; char quoted = 0; ALLOC_ARRAY(*argv, size); @@ -96,6 +98,11 @@ int split_cmdline(char *cmdline, const char ***argv) return -SPLIT_CMDLINE_UNCLOSED_QUOTE; } + if (count >= INT_MAX) { + FREE_AND_NULL(*argv); + return -SPLIT_CMDLINE_ARGC_OVERFLOW; + } + ALLOC_GROW(*argv, count + 1, size); (*argv)[count] = NULL; -- cgit v0.10.2-6-g49f6 From abd4d67ab0f84fff703fa14d9eebfd287b42daeb Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 30 Sep 2022 16:32:10 -0400 Subject: Git 2.30.6 Signed-off-by: Taylor Blau diff --git a/Documentation/RelNotes/2.30.6.txt b/Documentation/RelNotes/2.30.6.txt new file mode 100644 index 0000000..d649071 --- /dev/null +++ b/Documentation/RelNotes/2.30.6.txt @@ -0,0 +1,60 @@ +Git v2.30.6 Release Notes +========================= + +This release addresses the security issues CVE-2022-39253 and +CVE-2022-39260. + +Fixes since v2.30.5 +------------------- + + * CVE-2022-39253: + When relying on the `--local` clone optimization, Git dereferences + symbolic links in the source repository before creating hardlinks + (or copies) of the dereferenced link in the destination repository. + This can lead to surprising behavior where arbitrary files are + present in a repository's `$GIT_DIR` when cloning from a malicious + repository. + + Git will no longer dereference symbolic links via the `--local` + clone mechanism, and will instead refuse to clone repositories that + have symbolic links present in the `$GIT_DIR/objects` directory. + + Additionally, the value of `protocol.file.allow` is changed to be + "user" by default. + + * CVE-2022-39260: + An overly-long command string given to `git shell` can result in + overflow in `split_cmdline()`, leading to arbitrary heap writes and + remote code execution when `git shell` is exposed and the directory + `$HOME/git-shell-commands` exists. + + `git shell` is taught to refuse interactive commands that are + longer than 4MiB in size. `split_cmdline()` is hardened to reject + inputs larger than 2GiB. + +Credit for finding CVE-2022-39253 goes to Cory Snider of Mirantis. The +fix was authored by Taylor Blau, with help from Johannes Schindelin. + +Credit for finding CVE-2022-39260 goes to Kevin Backhouse of GitHub. +The fix was authored by Kevin Backhouse, Jeff King, and Taylor Blau. + + +Jeff King (2): + shell: add basic tests + shell: limit size of interactive commands + +Kevin Backhouse (1): + alias.c: reject too-long cmdline strings in split_cmdline() + +Taylor Blau (11): + builtin/clone.c: disallow `--local` clones with symlinks + t/lib-submodule-update.sh: allow local submodules + t/t1NNN: allow local submodules + t/2NNNN: allow local submodules + t/t3NNN: allow local submodules + t/t4NNN: allow local submodules + t/t5NNN: allow local submodules + t/t6NNN: allow local submodules + t/t7NNN: allow local submodules + t/t9NNN: allow local submodules + transport: make `protocol.file.allow` be "user" by default diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 39d0c99..40fa0b5 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.30.5 +DEF_VER=v2.30.6 LF=' ' diff --git a/RelNotes b/RelNotes index 406d238..018a28e 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.30.5.txt \ No newline at end of file +Documentation/RelNotes/2.30.6.txt \ No newline at end of file -- cgit v0.10.2-6-g49f6