fsck: warn about symlink pointing inside a gitdir

In the wake of fixing a vulnerability where `git clone` mistakenly
followed a symbolic link that it had just written while checking out
files, writing into a gitdir, let's add some defense-in-depth by
teaching `git fsck` to report symbolic links stored in its trees that
point inside `.git/`.

Even though the Git project never made any promises about the exact
shape of the `.git/` directory's contents, there are likely repositories
out there containing symbolic links that point inside the gitdir. For
that reason, let's only report these as warnings, not as errors.
Security-conscious users are encouraged to configure
`fsck.symlinkPointsToGitDir = error`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt
index 12eae8a2225a3c4d216fd05c874c1ed2930b5545..b06ec385aff6dc0aa64c9f3f26de18e84fe357b6 100644 (file)
--- a/Documentation/fsck-msgids.txt
+++ b/Documentation/fsck-msgids.txt
@@ -157,6 +157,18 @@
 `nullSha1`::
        (WARN) Tree contains entries pointing to a null sha1.
 
+`symlinkPointsToGitDir`::
+       (WARN) 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.
+
+`symlinkTargetMissing`::
+       (ERROR) Unable to read symbolic link target's blob.
+
 `treeNotSorted`::
        (ERROR) A tree is not properly sorted.
 

diff --git a/fsck.c b/fsck.c
index 47eaeedd7076ba60a621e072abb405127b3c33fe..b85868e122504cc833051f7649896de1eb8fd02e 100644 (file)
--- a/fsck.c
+++ b/fsck.c
@@ -636,6 +636,8 @@ static int fsck_tree(const struct object_id *tree_oid,
                                retval += report(options, tree_oid, OBJ_TREE,
                                                 FSCK_MSG_MAILMAP_SYMLINK,
                                                 ".mailmap is a symlink");
+                       oidset_insert(&options->symlink_targets_found,
+                                     entry_oid);
                }
 
                if ((backslash = strchr(name, '\\'))) {
@@ -1228,6 +1230,56 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
                }
        }
 
+       if (oidset_contains(&options->symlink_targets_found, oid)) {
+               const char *ptr = buf;
+               const struct object_id *reported = NULL;
+
+               oidset_insert(&options->symlink_targets_done, oid);
+
+               if (!buf || size > PATH_MAX) {
+                       /*
+                        * A missing buffer here is a sign that the caller found the
+                        * blob too gigantic to load into memory. Let's just consider
+                        * that an error.
+                        */
+                       return report(options, oid, OBJ_BLOB,
+                                       FSCK_MSG_SYMLINK_TARGET_LENGTH,
+                                       "symlink target too long");
+               }
+
+               while (!reported && ptr) {
+                       const char *p = ptr;
+                       char c, *slash = strchrnul(ptr, '/');
+                       char *backslash = memchr(ptr, '\\', slash - ptr);
+
+                       c = *slash;
+                       *slash = '\0';
+
+                       while (!reported && backslash) {
+                               *backslash = '\0';
+                               if (is_ntfs_dotgit(p))
+                                       ret |= report(options, reported = oid, OBJ_BLOB,
+                                                     FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR,
+                                                     "symlink target points to git dir");
+                               *backslash = '\\';
+                               p = backslash + 1;
+                               backslash = memchr(p, '\\', slash - p);
+                       }
+                       if (!reported && is_ntfs_dotgit(p))
+                               ret |= report(options, reported = oid, OBJ_BLOB,
+                                             FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR,
+                                             "symlink target points to git dir");
+
+                       if (!reported && is_hfs_dotgit(ptr))
+                               ret |= report(options, reported = oid, OBJ_BLOB,
+                                             FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR,
+                                             "symlink target points to git dir");
+
+                       *slash = c;
+                       ptr = c ? slash + 1 : NULL;
+               }
+       }
+
        return ret;
 }
 
@@ -1319,6 +1371,10 @@ int fsck_finish(struct fsck_options *options)
                          FSCK_MSG_GITATTRIBUTES_MISSING, FSCK_MSG_GITATTRIBUTES_BLOB,
                          options, ".gitattributes");
 
+       ret |= fsck_blobs(&options->symlink_targets_found, &options->symlink_targets_done,
+                         FSCK_MSG_SYMLINK_TARGET_MISSING, FSCK_MSG_SYMLINK_TARGET_BLOB,
+                         options, "<symlink-target>");
+
        return ret;
 }
 

diff --git a/fsck.h b/fsck.h
index fcecf4101cfc9cbb60f9467d629403fe4221f782..130fa8d8f916e700599f7fecea32858e0143a8f4 100644 (file)
--- a/fsck.h
+++ b/fsck.h
@@ -63,6 +63,8 @@ enum fsck_msg_type {
        FUNC(GITATTRIBUTES_LARGE, ERROR) \
        FUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \
        FUNC(GITATTRIBUTES_BLOB, ERROR) \
+       FUNC(SYMLINK_TARGET_MISSING, ERROR) \
+       FUNC(SYMLINK_TARGET_BLOB, ERROR) \
        /* warnings */ \
        FUNC(EMPTY_NAME, WARN) \
        FUNC(FULL_PATHNAME, WARN) \
@@ -72,6 +74,8 @@ 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) \
@@ -139,6 +143,8 @@ struct fsck_options {
        struct oidset gitmodules_done;
        struct oidset gitattributes_found;
        struct oidset gitattributes_done;
+       struct oidset symlink_targets_found;
+       struct oidset symlink_targets_done;
        kh_oid_map_t *object_names;
 };
 
@@ -148,6 +154,8 @@ struct fsck_options {
        .gitmodules_done = OIDSET_INIT, \
        .gitattributes_found = OIDSET_INIT, \
        .gitattributes_done = OIDSET_INIT, \
+       .symlink_targets_found = OIDSET_INIT, \
+       .symlink_targets_done = OIDSET_INIT, \
        .error_func = fsck_error_function \
 }
 #define FSCK_OPTIONS_STRICT { \
@@ -156,6 +164,8 @@ struct fsck_options {
        .gitmodules_done = OIDSET_INIT, \
        .gitattributes_found = OIDSET_INIT, \
        .gitattributes_done = OIDSET_INIT, \
+       .symlink_targets_found = OIDSET_INIT, \
+       .symlink_targets_done = OIDSET_INIT, \
        .error_func = fsck_error_function, \
 }
 #define FSCK_OPTIONS_MISSING_GITMODULES { \
@@ -164,6 +174,8 @@ struct fsck_options {
        .gitmodules_done = OIDSET_INIT, \
        .gitattributes_found = OIDSET_INIT, \
        .gitattributes_done = OIDSET_INIT, \
+       .symlink_targets_found = OIDSET_INIT, \
+       .symlink_targets_done = OIDSET_INIT, \
        .error_func = fsck_error_cb_print_missing_gitmodules, \
 }
 

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index de0f6d5e7fd49ca5276696833d8f9dd034a15629..5669872bc8057d7d5723ddba9f1153fa9ec5f164 100755 (executable)
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -1023,4 +1023,41 @@ test_expect_success 'fsck error on gitattributes with excessive size' '
        test_cmp expected actual
 '
 
+test_expect_success 'fsck warning on symlink target with excessive length' '
+       symlink_target=$(printf "pattern %032769d" 1 | git hash-object -w --stdin) &&
+       test_when_finished "remove_object $symlink_target" &&
+       tree=$(printf "120000 blob %s\t%s\n" $symlink_target symlink | git mktree) &&
+       test_when_finished "remove_object $tree" &&
+       cat >expected <<-EOF &&
+       warning in blob $symlink_target: symlinkTargetLength: symlink target too long
+       EOF
+       git fsck --no-dangling >actual 2>&1 &&
+       test_cmp expected actual
+'
+
+test_expect_success 'fsck warning on symlink target pointing inside git dir' '
+       gitdir=$(printf ".git" | git hash-object -w --stdin) &&
+       ntfs_gitdir=$(printf "GIT~1" | git hash-object -w --stdin) &&
+       hfs_gitdir=$(printf ".${u200c}git" | git hash-object -w --stdin) &&
+       inside_gitdir=$(printf "nested/.git/config" | git hash-object -w --stdin) &&
+       benign_target=$(printf "legit/config" | git hash-object -w --stdin) &&
+       tree=$(printf "120000 blob %s\t%s\n" \
+               $benign_target benign_target \
+               $gitdir gitdir \
+               $hfs_gitdir hfs_gitdir \
+               $inside_gitdir inside_gitdir \
+               $ntfs_gitdir ntfs_gitdir |
+               git mktree) &&
+       for o in $gitdir $ntfs_gitdir $hfs_gitdir $inside_gitdir $benign_target $tree
+       do
+               test_when_finished "remove_object $o" || return 1
+       done &&
+       printf "warning in blob %s: symlinkPointsToGitDir: symlink target points to git dir\n" \
+               $gitdir $hfs_gitdir $inside_gitdir $ntfs_gitdir |
+       sort >expected &&
+       git fsck --no-dangling >actual 2>&1 &&
+       sort actual >actual.sorted &&
+       test_cmp expected actual.sorted
+'
+
 test_done