* [PATCH 1/2] Revert "clone: prevent hooks from running during a clone"
2024-05-14 18:16 [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS brian m. carlson
@ 2024-05-14 18:16 ` brian m. carlson
2024-05-14 18:16 ` [PATCH 2/2] Revert "core.hooksPath: add some protection while cloning" brian m. carlson
2024-05-14 19:07 ` [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS Johannes Schindelin
2 siblings, 0 replies; 14+ messages in thread
From: brian m. carlson @ 2024-05-14 18:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin
From: "brian m. carlson" <bk2204@github.com>
This change breaks Git LFS, which relies on the pre-checkout hook to run
in a clone. This is to allow files which have the lockable attribute,
which are usually large binary assets which cannot be merged, to
be marked read-only when a user doesn't have a lock so as to prevent
conflicts.
Revert this change since it causes functional problems.
This reverts commit 8db1e8743c0f1ed241f6a1b8bf55b6fef07d6751.
Signed-off-by: brian m. carlson <bk2204@github.com>
---
builtin/clone.c | 5 -----
hook.c | 32 ------------------------------
t/t5601-clone.sh | 51 ------------------------------------------------
3 files changed, 88 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 0e7cf198f5..c66a32feb3 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -987,12 +987,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
usage_msg_opt(_("You must specify a repository to clone."),
builtin_clone_usage, builtin_clone_options);
- xsetenv("GIT_CLONE_PROTECTION_ACTIVE", "true", 0 /* allow user override */);
template_dir = get_template_dir(option_template);
- if (*template_dir && !is_absolute_path(template_dir))
- template_dir = template_dir_dup =
- absolute_pathdup(template_dir);
- xsetenv("GIT_CLONE_TEMPLATE_DIR", template_dir, 1);
if (option_depth || option_since || option_not.nr)
deepen = 1;
diff --git a/hook.c b/hook.c
index eebc4d4473..f5f46e844e 100644
--- a/hook.c
+++ b/hook.c
@@ -11,30 +11,6 @@
#include "setup.h"
#include "copy.h"
-static int identical_to_template_hook(const char *name, const char *path)
-{
- const char *env = getenv("GIT_CLONE_TEMPLATE_DIR");
- const char *template_dir = get_template_dir(env && *env ? env : NULL);
- struct strbuf template_path = STRBUF_INIT;
- int found_template_hook, ret;
-
- strbuf_addf(&template_path, "%s/hooks/%s", template_dir, name);
- found_template_hook = access(template_path.buf, X_OK) >= 0;
-#ifdef STRIP_EXTENSION
- if (!found_template_hook) {
- strbuf_addstr(&template_path, STRIP_EXTENSION);
- found_template_hook = access(template_path.buf, X_OK) >= 0;
- }
-#endif
- if (!found_template_hook)
- return 0;
-
- ret = do_files_match(template_path.buf, path);
-
- strbuf_release(&template_path);
- return ret;
-}
-
const char *find_hook(const char *name)
{
static struct strbuf path = STRBUF_INIT;
@@ -70,14 +46,6 @@ const char *find_hook(const char *name)
}
return NULL;
}
- if (!git_hooks_path && git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0) &&
- !identical_to_template_hook(name, path.buf))
- die(_("active `%s` hook found during `git clone`:\n\t%s\n"
- "For security reasons, this is disallowed by default.\n"
- "If this is intentional and the hook should actually "
- "be run, please\nrun the command again with "
- "`GIT_CLONE_PROTECTION_ACTIVE=false`"),
- name, path.buf);
return path.buf;
}
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index deb1c282c7..cc0b953f14 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -788,57 +788,6 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe
git clone --filter=blob:limit=0 "file://$(pwd)/server" client
'
-test_expect_success 'clone with init.templatedir runs hooks' '
- git init tmpl/hooks &&
- write_script tmpl/hooks/post-checkout <<-EOF &&
- echo HOOK-RUN >&2
- echo I was here >hook.run
- EOF
- git -C tmpl/hooks add . &&
- test_tick &&
- git -C tmpl/hooks commit -m post-checkout &&
-
- test_when_finished "git config --global --unset init.templateDir || :" &&
- test_when_finished "git config --unset init.templateDir || :" &&
- (
- sane_unset GIT_TEMPLATE_DIR &&
- NO_SET_GIT_TEMPLATE_DIR=t &&
- export NO_SET_GIT_TEMPLATE_DIR &&
-
- git -c core.hooksPath="$(pwd)/tmpl/hooks" \
- clone tmpl/hooks hook-run-hookspath 2>err &&
- test_grep ! "active .* hook found" err &&
- test_path_is_file hook-run-hookspath/hook.run &&
-
- git -c init.templateDir="$(pwd)/tmpl" \
- clone tmpl/hooks hook-run-config 2>err &&
- test_grep ! "active .* hook found" err &&
- test_path_is_file hook-run-config/hook.run &&
-
- git clone --template=tmpl tmpl/hooks hook-run-option 2>err &&
- test_grep ! "active .* hook found" err &&
- test_path_is_file hook-run-option/hook.run &&
-
- git config --global init.templateDir "$(pwd)/tmpl" &&
- git clone tmpl/hooks hook-run-global-config 2>err &&
- git config --global --unset init.templateDir &&
- test_grep ! "active .* hook found" err &&
- test_path_is_file hook-run-global-config/hook.run &&
-
- # clone ignores local `init.templateDir`; need to create
- # a new repository because we deleted `.git/` in the
- # `setup` test case above
- git init local-clone &&
- cd local-clone &&
-
- git config init.templateDir "$(pwd)/../tmpl" &&
- git clone ../tmpl/hooks hook-run-local-config 2>err &&
- git config --unset init.templateDir &&
- test_grep ! "active .* hook found" err &&
- test_path_is_missing hook-run-local-config/hook.run
- )
-'
-
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 2/2] Revert "core.hooksPath: add some protection while cloning"
2024-05-14 18:16 [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS brian m. carlson
2024-05-14 18:16 ` [PATCH 1/2] Revert "clone: prevent hooks from running during a clone" brian m. carlson
@ 2024-05-14 18:16 ` brian m. carlson
2024-05-14 19:07 ` [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS Johannes Schindelin
2 siblings, 0 replies; 14+ messages in thread
From: brian m. carlson @ 2024-05-14 18:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin
From: "brian m. carlson" <bk2204@github.com>
The original commit breaks Git LFS, which installs hooks when it is
invoked during the smudge process as part of checkout. This is required
to install a post-checkout hook that causes files which are set as
lockable (which are typically large binary assets that cannot be merged)
to be read-only unless they've been locked. In addition, Git LFS
requires the pre-push hook to be installed so that LFS objects can be
pushed as part of the invocation of git push.
Without the ability to install these hooks, the locking functionality
would not work until the user invoked Git LFS again and did a completely
new checkout with all files changed, since Git LFS optimizes for only
changed files. In addition, an invocation of git push might not push
anything LFS files all to the remote, potentially causing data loss.
Note that this affects all clone operations with a repository with Git
LFS files in it, even if they are configured not to smudge data by
default, so it breaks all automated clones (which will see "die" called)
without the relevant environment variable specified.
Revert this change to restore functionality.
This reverts commit 20f3588efc6cbcae5bbaabf65ee12df87b51a9ea.
Signed-off-by: brian m. carlson <bk2204@github.com>
---
config.c | 13 +------------
t/t1800-hook.sh | 15 ---------------
2 files changed, 1 insertion(+), 27 deletions(-)
diff --git a/config.c b/config.c
index 77a0fd2d80..ae3652b08f 100644
--- a/config.c
+++ b/config.c
@@ -1416,19 +1416,8 @@ static int git_default_core_config(const char *var, const char *value,
if (!strcmp(var, "core.attributesfile"))
return git_config_pathname(&git_attributes_file, var, value);
- if (!strcmp(var, "core.hookspath")) {
- if (ctx->kvi && ctx->kvi->scope == CONFIG_SCOPE_LOCAL &&
- git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0))
- die(_("active `core.hooksPath` found in the local "
- "repository config:\n\t%s\nFor security "
- "reasons, this is disallowed by default.\nIf "
- "this is intentional and the hook should "
- "actually be run, please\nrun the command "
- "again with "
- "`GIT_CLONE_PROTECTION_ACTIVE=false`"),
- value);
+ if (!strcmp(var, "core.hookspath"))
return git_config_pathname(&git_hooks_path, var, value);
- }
if (!strcmp(var, "core.bare")) {
is_bare_repository_cfg = git_config_bool(var, value);
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 1894ebeb0e..8b0234cf2d 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -185,19 +185,4 @@ test_expect_success 'stdin to hooks' '
test_cmp expect actual
'
-test_expect_success 'clone protections' '
- test_config core.hooksPath "$(pwd)/my-hooks" &&
- mkdir -p my-hooks &&
- write_script my-hooks/test-hook <<-\EOF &&
- echo Hook ran $1
- EOF
-
- git hook run test-hook 2>err &&
- test_grep "Hook ran" err &&
- test_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \
- git hook run test-hook 2>err &&
- test_grep "active .core.hooksPath" err &&
- test_grep ! "Hook ran" err
-'
-
test_done
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
2024-05-14 18:16 [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS brian m. carlson
2024-05-14 18:16 ` [PATCH 1/2] Revert "clone: prevent hooks from running during a clone" brian m. carlson
2024-05-14 18:16 ` [PATCH 2/2] Revert "core.hooksPath: add some protection while cloning" brian m. carlson
@ 2024-05-14 19:07 ` Johannes Schindelin
2024-05-14 19:41 ` brian m. carlson
2 siblings, 1 reply; 14+ messages in thread
From: Johannes Schindelin @ 2024-05-14 19:07 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Junio C Hamano
brian,
On Tue, 14 May 2024, brian m. carlson wrote:
> The recent defense-in-depth patches to restrict hooks while cloning
> broke Git LFS because it installs necessary hooks when it is invoked by
> Git's smudge filter. This means that currently, anyone with Git LFS
> installed who attempts to clone a repository with at least one LFS file
> will see a message like the following (fictitious example):
>
> ----
> $ git clone https://github.com/octocat/xyzzy.git
> Cloning into 'pull-bug'...
> remote: Enumerating objects: 1275, done.
> remote: Counting objects: 100% (343/343), done.
> remote: Compressing objects: 100% (136/136), done.
> remote: Total 1275 (delta 221), reused 327 (delta 206), pack-reused 932
> Receiving objects: 100% (1275/1275), 290.78 KiB | 2.88 MiB/s, done.
> Resolving deltas: 100% (226/226), done.
> Filtering content: 100% (504/504), 1.86 KiB | 0 bytes/s, done.
> fatal: active `post-checkout` hook found during `git clone`:
> /home/octocat/xyzzy/.git/hooks/post-checkout
> For security reasons, this is disallowed by default.
> If this is intentional and the hook should actually be run, please
> run the command again with `GIT_CLONE_PROTECTION_ACTIVE=false`
> warning: Clone succeeded, but checkout failed.
> You can inspect what was checked out with 'git status'
> and retry with 'git restore --source=HEAD :/'
> ----
>
> This causes most CI systems to be broken in such a case, as well as a
> confusing message for the user.
When using `actions/checkout` in GitHub workflows, nothing is broken
because `actions/checkout` uses a fetch + checkout (to allow for things
like sparse checkout), which obviously lacks the clone protections because
it is not a clone.
> It's not really possible to avoid the need to install the hooks at this
> location because the post-checkout hook must be ready during the
> checkout that's part of the clone in order to properly adjust
> permissions on files. Thus, we'll need to revert the changes to
> restrict hooks while cloning, which this series does.
Dropping protections is in general a bad idea. While previously, hackers
wishing to exploit weaknesses in Git might have been unaware of the
particular attack vector we want to prevent with these defense-in-depth
measurements, we now must assume that they are fully aware. Reverting
those protections can be seen as a very public invitation to search for
ways to exploit the now re-introduced avenues to craft Remote Code
Execution attacks.
I have pointed out several times that there are alternatives while
discussing this under embargo, even sent them to the git-security list
before the embargo was lifted, and have not received any reply. One
proposal was to introduce a way to cross-check the SHA-256 of hooks that
_were_ written during a clone operation against a list of known-good ones.
Another alternative was to special-case Git LFS by matching the hooks'
contents against a regular expression that matches Git LFS' current
hooks'.
Both alternatives demonstrate that we are far from _needing_ to revert the
changes that were designed to prevent future vulnerabilities from
immediately becoming critical Remote Code Executions. It might be an
easier way to address the Git LFS breakage, but "easy" does not equal
"right".
I did not yet get around to sending these patches to the Git mailing list
solely because I am still busy with a lot of follow-up work of the
embargoed release. It was an unwelcome surprise to see this here patch
series in my inbox and still no reply to the patches I had sent to the
git-security list for comments.
I am still busy wrapping up follow-up work and won't be able to
participate in this here mail thread meaningfully for the next hours. I do
want to invite you to think about alternative ways to address the Git LFS
issues, alternatives that do not re-open weaknesses we had hoped to
address for good.
I do want to extend the invitation to work with me on that, for example by
reviewing those patches I sent to the git-security mailing list (or even
to send them to the Git mailing list for public review on my behalf, that
would be helpful).
Ciao,
Johannes
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
2024-05-14 19:07 ` [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS Johannes Schindelin
@ 2024-05-14 19:41 ` brian m. carlson
2024-05-22 9:49 ` Joey Hess
2024-05-24 17:37 ` Joey Hess
0 siblings, 2 replies; 14+ messages in thread
From: brian m. carlson @ 2024-05-14 19:41 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 5710 bytes --]
On 2024-05-14 at 19:07:28, Johannes Schindelin wrote:
> brian,
>
> On Tue, 14 May 2024, brian m. carlson wrote:
> > This causes most CI systems to be broken in such a case, as well as a
> > confusing message for the user.
>
> When using `actions/checkout` in GitHub workflows, nothing is broken
> because `actions/checkout` uses a fetch + checkout (to allow for things
> like sparse checkout), which obviously lacks the clone protections because
> it is not a clone.
I'm not saying all CI systems do this, but we probably have broken quite
a few.
> > It's not really possible to avoid the need to install the hooks at this
> > location because the post-checkout hook must be ready during the
> > checkout that's part of the clone in order to properly adjust
> > permissions on files. Thus, we'll need to revert the changes to
> > restrict hooks while cloning, which this series does.
>
> Dropping protections is in general a bad idea. While previously, hackers
> wishing to exploit weaknesses in Git might have been unaware of the
> particular attack vector we want to prevent with these defense-in-depth
> measurements, we now must assume that they are fully aware. Reverting
> those protections can be seen as a very public invitation to search for
> ways to exploit the now re-introduced avenues to craft Remote Code
> Execution attacks.
If these protections hadn't broken things, I'd agree that we should keep
them. However, they have broken things and they've introduced a
serious regression breaking a major project, and we should revert them.
I'm not opposed to us exploring other alternatives for defense in depth
measures on the list. I definitely think such work is valuable and
important, but I want to make sure the changes we make don't regress
important functionality.
> I have pointed out several times that there are alternatives while
> discussing this under embargo, even sent them to the git-security list
> before the embargo was lifted, and have not received any reply. One
> proposal was to introduce a way to cross-check the SHA-256 of hooks that
> _were_ written during a clone operation against a list of known-good ones.
> Another alternative was to special-case Git LFS by matching the hooks'
> contents against a regular expression that matches Git LFS' current
> hooks'.
I have replied to those on the security list and to the general idea. I
don't think we should special-case Git LFS here. That's antithetical to
the long-standing ethos of the project. While Git LFS is _one_ known
project to have been broken, there may be others, or people may have
forks of Git LFS under other names, and we want that tooling to be able
to take advantage of all of the same features.
I'm also opposed to any kind of pinning of the Git LFS hooks to Git in
general, whatever the approach is. Git LFS runs against multiple
versions of Git in our CI, and if we change the hooks in a way that Git
hasn't pinned, either via SHA-256 hash or regex, then Git LFS (and its
CI) is broken until Git gets an update. We've already had to deal with
small incompatible changes that have broken Git LFS, and I don't think
coupling the projects in this way is a good idea.
Finally, it's very easy to work around the protections by merely placing
a malicious binary called "git-lfs" earlier in the PATH, so we're not
necessarily getting a substantial amount of improved security by
requiring that binary.
> Both alternatives demonstrate that we are far from _needing_ to revert the
> changes that were designed to prevent future vulnerabilities from
> immediately becoming critical Remote Code Executions. It might be an
> easier way to address the Git LFS breakage, but "easy" does not equal
> "right".
>
> I did not yet get around to sending these patches to the Git mailing list
> solely because I am still busy with a lot of follow-up work of the
> embargoed release. It was an unwelcome surprise to see this here patch
> series in my inbox and still no reply to the patches I had sent to the
> git-security list for comments.
As you may know, I don't read the git or git-security lists except on
my personal computer using my personal email, and I have been at work
most of the day putting out fires related to the Git LFS breakage above.
Thus, I haven't seen them until just recently, when I did try to get a
reply out.
> I am still busy wrapping up follow-up work and won't be able to
> participate in this here mail thread meaningfully for the next hours. I do
> want to invite you to think about alternative ways to address the Git LFS
> issues, alternatives that do not re-open weaknesses we had hoped to
> address for good.
I don't want to put undue pressure on you. Please feel free to respond
or not at your leisure.
> I do want to extend the invitation to work with me on that, for example by
> reviewing those patches I sent to the git-security mailing list (or even
> to send them to the Git mailing list for public review on my behalf, that
> would be helpful).
I think the substance of my response was the same one that I gave above.
With all due respect, I don't think your patches are the right way to
address the problem, and I fear that by bringing them to the list, I'd
be giving the list the misleading impression that I endorsed them with
my sign-off. While I respect you as a colleague and a contributor, even
if we may disagree on this or other issues, I don't agree with the
approach in the patches, and I don't think it would be a good idea to
apply my sign-off in sending them.
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
2024-05-14 19:41 ` brian m. carlson
@ 2024-05-22 9:49 ` Joey Hess
2024-05-27 19:35 ` Johannes Schindelin
2024-05-24 17:37 ` Joey Hess
1 sibling, 1 reply; 14+ messages in thread
From: Joey Hess @ 2024-05-22 9:49 UTC (permalink / raw)
To: brian m. carlson, Johannes Schindelin, git, Junio C Hamano
brian m. carlson wrote:
> If these protections hadn't broken things, I'd agree that we should keep
> them. However, they have broken things and they've introduced a
> serious regression breaking a major project, and we should revert them.
More than one major project; they also broke git-annex in the case where
a git-annex repository, which contains symlinks into
.git/annex/objects/, is pushed to a bare repository with
receive.fsckObjects set. (Gitlab is currently affected[1].)
BTW, do I understand correctly that the defence in depth patch set was
developed under embargo and has never been publically reviewed?
Looking at commit a33fea0886cfa016d313d2bd66bdd08615bffbc9, I noticed
that its PATH_MAX check is also dodgy due to that having values ranging
from 260 (Windows) to 1024 (Freebsd) to 4096 (Linux), which means git
repositories containing legitimate, working symlinks can now fail to be
pushed depending on what OS happens to host a reciving bare repository.
+ if (is_ntfs_dotgit(p))
This means that symlinks to eg "git~1" are also warned about,
which seems strange behavior on eg Linux.
+ backslash = memchr(p, '\\', slash - p);
This and other backslash handling code for some reason is also run on
linux, so a symlink to eg "ummmm\\git~1" is also warned about.
+ if (!buf || size > PATH_MAX) {
I suspect, but have not confirmed, that this is allows a symlink
target 1 byte longer than the OS supports, because PATH_MAX includes
a trailing NUL.
All in all, this seems to need more review and a more careful
consideration of breakage now that the security holes are not under
embargo.
--
see shy jo
[1] https://forum.gitlab.com/t/recent-git-v2-45-1-breaks-git-annex-compatibility-because-of-apparent-fsck-symlinkpointstogitdir-error-on-gitlab/104909
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
2024-05-22 9:49 ` Joey Hess
@ 2024-05-27 19:35 ` Johannes Schindelin
2024-05-28 2:13 ` Joey Hess
0 siblings, 1 reply; 14+ messages in thread
From: Johannes Schindelin @ 2024-05-27 19:35 UTC (permalink / raw)
To: Joey Hess; +Cc: brian m. carlson, git, Junio C Hamano
Hi Joey,
On Wed, 22 May 2024, Joey Hess wrote:
> brian m. carlson wrote:
> > If these protections hadn't broken things, I'd agree that we should keep
> > them. However, they have broken things and they've introduced a
> > serious regression breaking a major project, and we should revert them.
>
> More than one major project; they also broke git-annex in the case where
> a git-annex repository, which contains symlinks into
> .git/annex/objects/, is pushed to a bare repository with
> receive.fsckObjects set. (Gitlab is currently affected[1].)
This added fsck functionality was specifically marked as `WARN` instead of
`ERROR`, though. So it should not have failed.
> BTW, do I understand correctly that the defence in depth patch set was
> developed under embargo and has never been publically reviewed?
>
> Looking at commit a33fea0886cfa016d313d2bd66bdd08615bffbc9, I noticed
> that its PATH_MAX check is also dodgy due to that having values ranging
> from 260 (Windows) to 1024 (Freebsd) to 4096 (Linux), which means git
> repositories containing legitimate, working symlinks can now fail to be
> pushed depending on what OS happens to host a reciving bare repository.
Likewise, this fsck functionality was specifically marked as `WARN`
instead of `ERROR`, to prevent exactly the issue you are seeing.
Are you saying that Gitlab is upgrading fsck warnings to errors? If so, I
fear we need to ask Gitlab to stop doing that.
> + if (is_ntfs_dotgit(p))
>
> This means that symlinks to eg "git~1" are also warned about,
> which seems strange behavior on eg Linux.
Only until you realize that there are many cross-platform projects, and
that Windows Subsystem for Linux is a thing.
> + backslash = memchr(p, '\\', slash - p);
>
> This and other backslash handling code for some reason is also run on
> linux, so a symlink to eg "ummmm\\git~1" is also warned about.
Right. As far as I can tell, there are very few Linux-only projects left,
so this is in line with many (most?) projects being cross-platform.
> + if (!buf || size > PATH_MAX) {
>
> I suspect, but have not confirmed, that this is allows a symlink
> target 1 byte longer than the OS supports, because PATH_MAX includes
> a trailing NUL.
True. That condition is basically imitating the `size >
ATTR_MAX_FILE_SIZE` one a couple of lines earlier, but it should be `>=`
here because `PATH_MAX` is supposed to accommodate the trailing NUL.
Ciao,
Johannes
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
2024-05-27 19:35 ` Johannes Schindelin
@ 2024-05-28 2:13 ` Joey Hess
[not found] ` <ZlZSZ1-0F2DEp9yV@tapette.crustytoothpaste.net>
2024-05-29 8:54 ` Jeff King
0 siblings, 2 replies; 14+ messages in thread
From: Joey Hess @ 2024-05-28 2:13 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: brian m. carlson, git, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 2344 bytes --]
Johannes Schindelin wrote:
> > More than one major project; they also broke git-annex in the case where
> > a git-annex repository, which contains symlinks into
> > .git/annex/objects/, is pushed to a bare repository with
> > receive.fsckObjects set. (Gitlab is currently affected[1].)
>
> This added fsck functionality was specifically marked as `WARN` instead of
> `ERROR`, though. So it should not have failed.
A git push into a bare repository with receive.fsckobjects = true fails:
joey@darkstar:~/tmp/bench/bar.git>git config --list |grep fsck
receive.fsckobjects=true
joey@darkstar:~/tmp/bench/bar.git>cd ..
joey@darkstar:~/tmp/bench>cd foo
joey@darkstar:~/tmp/bench/foo>git push ../bar.git master
Enumerating objects: 4, done.
Counting objects: 100% (4/4), done.
Delta compression using up to 12 threads
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir
remote: fatal: fsck error in pack objects
error: remote unpack failed: unpack-objects abnormal exit
To ../bar.git
! [remote rejected] master -> master (unpacker error)
error: failed to push some refs to '../bar.git'
So I guess that the WARN doesn't work like you expected it to in this case of
receive.fsckobjects checking.
> > This means that symlinks to eg "git~1" are also warned about,
> > which seems strange behavior on eg Linux.
>
> Only until you realize that there are many cross-platform projects, and
> that Windows Subsystem for Linux is a thing.
I realize that of course, but I also reserve the right to make git repos that
contain files named eg "CON" if I want to. Git should not demand
filename interoperability with arbitrary OSes.
> > + backslash = memchr(p, '\\', slash - p);
> >
> > This and other backslash handling code for some reason is also run on
> > linux, so a symlink to eg "ummmm\\git~1" is also warned about.
>
> Right. As far as I can tell, there are very few Linux-only projects left,
> so this is in line with many (most?) projects being cross-platform.
We may have very different lived experiences then.
--
see shy jo
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread[parent not found: <ZlZSZ1-0F2DEp9yV@tapette.crustytoothpaste.net>]
* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
[not found] ` <ZlZSZ1-0F2DEp9yV@tapette.crustytoothpaste.net>
@ 2024-05-28 23:46 ` Junio C Hamano
0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2024-05-28 23:46 UTC (permalink / raw)
To: brian m. carlson; +Cc: Joey Hess, Johannes Schindelin, git
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> On 2024-05-28 at 02:13:55, Joey Hess wrote:
>> Johannes Schindelin wrote:
>> Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done.
>> Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
>> remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir
>> remote: fatal: fsck error in pack objects
>> error: remote unpack failed: unpack-objects abnormal exit
>> To ../bar.git
>> ! [remote rejected] master -> master (unpacker error)
>> error: failed to push some refs to '../bar.git'
>>
>> So I guess that the WARN doesn't work like you expected it to in this case of
>> receive.fsckobjects checking.
>
> Then my guess is that this will affect most forges.
FWIW, it is detected as "error" according to the above.
In any case, a33fea08 (fsck: warn about symlink pointing inside a
gitdir, 2024-04-10) adds two ERRORs, in addition to two WARNs:
+ FUNC(SYMLINK_TARGET_MISSING, ERROR) \
+ FUNC(SYMLINK_TARGET_BLOB, ERROR) \
+ FUNC(SYMLINK_TARGET_LENGTH, WARN) \
+ FUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \
so "they are only warnings and won't break" is not quite what I see
in the change, but what is causing the above error does look like
the one that is marked as WARN in the patch.
Thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
2024-05-28 2:13 ` Joey Hess
[not found] ` <ZlZSZ1-0F2DEp9yV@tapette.crustytoothpaste.net>
@ 2024-05-29 8:54 ` Jeff King
2024-05-29 12:17 ` Johannes Schindelin
1 sibling, 1 reply; 14+ messages in thread
From: Jeff King @ 2024-05-29 8:54 UTC (permalink / raw)
To: Joey Hess; +Cc: Johannes Schindelin, brian m. carlson, git, Junio C Hamano
On Mon, May 27, 2024 at 10:13:55PM -0400, Joey Hess wrote:
> Johannes Schindelin wrote:
> > > More than one major project; they also broke git-annex in the case where
> > > a git-annex repository, which contains symlinks into
> > > .git/annex/objects/, is pushed to a bare repository with
> > > receive.fsckObjects set. (Gitlab is currently affected[1].)
> >
> > This added fsck functionality was specifically marked as `WARN` instead of
> > `ERROR`, though. So it should not have failed.
>
> A git push into a bare repository with receive.fsckobjects = true fails:
>
> joey@darkstar:~/tmp/bench/bar.git>git config --list |grep fsck
> receive.fsckobjects=true
> joey@darkstar:~/tmp/bench/bar.git>cd ..
> joey@darkstar:~/tmp/bench>cd foo
> joey@darkstar:~/tmp/bench/foo>git push ../bar.git master
> Enumerating objects: 4, done.
> Counting objects: 100% (4/4), done.
> Delta compression using up to 12 threads
> Compressing objects: 100% (2/2), done.
> Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done.
> Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
> remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir
> remote: fatal: fsck error in pack objects
> error: remote unpack failed: unpack-objects abnormal exit
> To ../bar.git
> ! [remote rejected] master -> master (unpacker error)
> error: failed to push some refs to '../bar.git'
>
> So I guess that the WARN doesn't work like you expected it to in this case of
> receive.fsckobjects checking.
This is a long-standing weirdness with the fsck severities. The
fsck_options struct used for fetches/pushes has the "strict" flag set,
which upgrades warnings to errors. But if you manually configure a
severity to "warn", then we respect that.
For example, try:
git init
git commit --allow-empty -m 'message with NUL'
commit=$(git cat-file commit HEAD |
perl -pe 's/NUL/\0/' |
git hash-object -w --stdin -t commit --literally)
git update-ref HEAD $commit
which is defined as WARN in fsck.h. And hence:
$ git fsck; echo $?
warning in commit 09b2d5bda87ffda7a0f36ea80c4b542edf9b9374: nulInCommit: NUL byte in the commit object body
Checking object directories: 100% (256/256), done.
0
But that's upgraded to ERROR for transfers:
$ git init --bare dst.git
$ git -C dst.git config transfer.fsckObjects true
$ git push dst.git
...
remote: error: object e6db180f21250e03b633a3684f593ceb7b9cd844: nulInCommit: NUL byte in the commit object body
remote: fatal: fsck error in packed object
error: remote unpack failed: unpack-objects abnormal exit
To dst.git
! [remote rejected] main -> main (unpacker error)
error: failed to push some refs to 'dst.git'
Unless we override it:
$ git -C dst.git config receive.fsck.nulInCommit warn
$ git push dst.git
remote: warning: object 09b2d5bda87ffda7a0f36ea80c4b542edf9b9374: nulInCommit: NUL byte in the commit object body
To dst.git
* [new branch] main -> main
But of course most sites just use the defaults, so all warnings are
effectively errors.
I think it's been this way at least since c99ba492f1 (fsck: introduce
identifiers for fsck messages, 2015-06-22). We've discussed it once or
twice on the list. It mostly seemed like a cosmetic issue to me, but in
this case it looks like it caused functional confusion.
I don't think just turning off the "strict" flag is a good idea, though.
The current severities are all over the place. A missing space in an
ident line is an error, but a tree with a ".git" directory is just a
warning!
So I think we'd first want to straighten out the severities, and then
think about letting warnings bypass transfer fscks. Though it's not
clear to me what hosters would want; pushing to a public site is a great
time to let people know their objects are broken _before_ everyone else
sees them, even if it's "just" a warning. But when you do have old
history with broken objects, the control and incentives are in the wrong
place; every person who wants to interact with the repo has to loosen
their fsck config. So it's not clear to me how aggressive transfer-level
fsck-ing should be.
In the meantime, we also have an "INFO" severity which gets reported but
not upgraded via strict. It sounds like that's what was intended here.
It should be available in all backport versions if we want it; it was
introduced in f27d05b170 (fsck: allow upgrading fsck warnings to errors,
2015-06-22).
-Peff
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
2024-05-29 8:54 ` Jeff King
@ 2024-05-29 12:17 ` Johannes Schindelin
2024-05-29 16:17 ` Junio C Hamano
2024-05-30 8:17 ` Jeff King
0 siblings, 2 replies; 14+ messages in thread
From: Johannes Schindelin @ 2024-05-29 12:17 UTC (permalink / raw)
To: Jeff King; +Cc: Joey Hess, brian m. carlson, git, Junio C Hamano
Hi Jeff,
On Wed, 29 May 2024, Jeff King wrote:
> [...] But of course most sites just use the defaults, so all warnings
> are effectively errors.
I wish that had been pointed out on the git-security mailing list when I
offered this patch up for review.
> In the meantime, we also have an "INFO" severity which gets reported but
> not upgraded via strict. It sounds like that's what was intended here.
Precisely.
So this is what the fix-up patch would look like to make the code match my
intention:
-- snipsnap --
Subject: [PATCH] fsck: demote the newly-introduced symlink issues from WARN -> IGNORE
The idea of the symlink check to prevent overly-long symlink targets and
targets inside the `.git/` directory was to _warn_, but not to prevent
any operation.
However, that's not how Git works, I was confused by the label `WARN`.
What we need instead is the `IGNORE` label, which still warns
(confusingly so ;-)), but does not prevent any operations from
continuing.
Adjust t1450 accordingly, documenting that `git fsck` unfortunately no
longer warns about these issues by default.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/fsck-msgids.txt | 4 ++--
fsck.h | 4 ++--
t/t1450-fsck.sh | 13 ++++++++++++-
3 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt
index b06ec385aff..f5016ecda6a 100644
--- a/Documentation/fsck-msgids.txt
+++ b/Documentation/fsck-msgids.txt
@@ -158,13 +158,13 @@
(WARN) Tree contains entries pointing to a null sha1.
`symlinkPointsToGitDir`::
- (WARN) Symbolic link points inside a gitdir.
+ (INFO) Symbolic link points inside a gitdir.
`symlinkTargetBlob`::
(ERROR) A non-blob found instead of a symbolic link's target.
`symlinkTargetLength`::
- (WARN) Symbolic link target longer than maximum path length.
+ (INFO) Symbolic link target longer than maximum path length.
`symlinkTargetMissing`::
(ERROR) Unable to read symbolic link target's blob.
diff --git a/fsck.h b/fsck.h
index 130fa8d8f91..d41ec98064b 100644
--- a/fsck.h
+++ b/fsck.h
@@ -74,8 +74,6 @@ enum fsck_msg_type {
FUNC(NULL_SHA1, WARN) \
FUNC(ZERO_PADDED_FILEMODE, WARN) \
FUNC(NUL_IN_COMMIT, WARN) \
- FUNC(SYMLINK_TARGET_LENGTH, WARN) \
- FUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \
/* infos (reported as warnings, but ignored by default) */ \
FUNC(BAD_FILEMODE, INFO) \
FUNC(GITMODULES_PARSE, INFO) \
@@ -84,6 +82,8 @@ enum fsck_msg_type {
FUNC(MAILMAP_SYMLINK, INFO) \
FUNC(BAD_TAG_NAME, INFO) \
FUNC(MISSING_TAGGER_ENTRY, INFO) \
+ FUNC(SYMLINK_TARGET_LENGTH, INFO) \
+ FUNC(SYMLINK_POINTS_TO_GIT_DIR, INFO) \
/* ignored (elevated when requested) */ \
FUNC(EXTRA_HEADER_ENTRY, IGNORE)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 5669872bc80..8339e60efb2 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -1032,7 +1032,18 @@ test_expect_success 'fsck warning on symlink target with excessive length' '
warning in blob $symlink_target: symlinkTargetLength: symlink target too long
EOF
git fsck --no-dangling >actual 2>&1 &&
- test_cmp expected actual
+ test_cmp expected actual &&
+
+ test_when_finished "git tag -d symlink-target-length" &&
+ git tag symlink-target-length $tree &&
+ test_when_finished "rm -rf throwaway.git" &&
+ git init --bare throwaway.git &&
+ git --git-dir=throwaway.git config receive.fsckObjects true &&
+ git --git-dir=throwaway.git config receive.fsck.symlinkTargetLength error &&
+ test_must_fail git push throwaway.git symlink-target-length &&
+ git --git-dir=throwaway.git config --unset receive.fsck.symlinkTargetLength &&
+ git push throwaway.git symlink-target-length 2>err &&
+ grep "warning.*symlinkTargetLength" err
'
test_expect_success 'fsck warning on symlink target pointing inside git dir' '
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
2024-05-29 12:17 ` Johannes Schindelin
@ 2024-05-29 16:17 ` Junio C Hamano
2024-05-30 8:17 ` Jeff King
1 sibling, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2024-05-29 16:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, Joey Hess, brian m. carlson, git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Wed, 29 May 2024, Jeff King wrote:
>
>> [...] But of course most sites just use the defaults, so all warnings
>> are effectively errors.
>
> I wish that had been pointed out on the git-security mailing list when I
> offered this patch up for review.
I sympathize with the sentiment, but there are things that becomes
much clearer once you know what to look for by getting specific
complaints, and I am sure that you would have come to "ah, there is
this strict thing in addition to the msg_type" yourself, without
anybody pointing it out to you, once you looked, if we had Joey's
report while working on the patch. I would have noticed it with a
breakage example back when the patch was first floated on the
security list, but of course I didn't, because the patch was only on
the security list without wider testers.
The take home lesson from this episode should not be "people should
speak up more in the security list". It instead is "let's try to
limit the work under embargo to absolute minimum, and work in the
open for anything on top".
"We saw an issue that we followed a symlink when we shouldn't, which
we are going to fix here, but it became high severity because of
where that symlink pointed at" may be a valid sentiment to have, but
we should stop at "fixing" it under embargo, and addressing the "but
... because" issue on top is better done in the open. Even if we
propose "let's not allow symlink at all---that way even if we wrote
through symlinks by mistake, we won't damage anything", there will
be more people to correct us when we worked in the open.
In any case, let's clean up the mess we created in 2.45.1 and
friends quickly to prepare a solid foundation to allow us do
additional work on top. The reverts are in 'next' and I plan to
merge it down to 'master', which hopefully allows us to do the
follow up releases soonish.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
2024-05-29 12:17 ` Johannes Schindelin
2024-05-29 16:17 ` Junio C Hamano
@ 2024-05-30 8:17 ` Jeff King
1 sibling, 0 replies; 14+ messages in thread
From: Jeff King @ 2024-05-30 8:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Joey Hess, brian m. carlson, git, Junio C Hamano
On Wed, May 29, 2024 at 02:17:41PM +0200, Johannes Schindelin wrote:
> On Wed, 29 May 2024, Jeff King wrote:
>
> > [...] But of course most sites just use the defaults, so all warnings
> > are effectively errors.
>
> I wish that had been pointed out on the git-security mailing list when I
> offered this patch up for review.
Me too. But I agree with everything Junio already responded here.
> So this is what the fix-up patch would look like to make the code match my
> intention:
>
> -- snipsnap --
> Subject: [PATCH] fsck: demote the newly-introduced symlink issues from WARN -> IGNORE
I think you mean s/IGNORE/INFO/ here and elsewhere in the commit
message? The actual code change looks correct.
-Peff
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
2024-05-14 19:41 ` brian m. carlson
2024-05-22 9:49 ` Joey Hess
@ 2024-05-24 17:37 ` Joey Hess
1 sibling, 0 replies; 14+ messages in thread
From: Joey Hess @ 2024-05-24 17:37 UTC (permalink / raw)
To: brian m. carlson, Johannes Schindelin, git, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 898 bytes --]
brian m. carlson wrote:
> > proposal was to introduce a way to cross-check the SHA-256 of hooks that
> > _were_ written during a clone operation against a list of known-good ones.
> > Another alternative was to special-case Git LFS by matching the hooks'
> > contents against a regular expression that matches Git LFS' current
> > hooks'.
>
> I have replied to those on the security list and to the general idea. I
> don't think we should special-case Git LFS here. That's antithetical to
> the long-standing ethos of the project.
I was surprised today to find that git-annex also triggers the hook
problem. In particular, a git clone that uses git-remote-annex can
cause several hooks to get created.
I think the hook check is already scheduled for reversion, but in case
not, here's another data point against hard-coding known-good hooks as a
solution.
--
see shy jo
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread