commit
236442372f
@ -1,259 +0,0 @@
|
||||
From 68061e3470210703cb15594194718d35094afdc0 Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Thu, 29 Aug 2019 14:37:26 -0400
|
||||
Subject: [PATCH] fast-import: disallow "feature export-marks" by default
|
||||
|
||||
The fast-import stream command "feature export-marks=<path>" lets the
|
||||
stream write marks to an arbitrary path. This may be surprising if you
|
||||
are running fast-import against an untrusted input (which otherwise
|
||||
cannot do anything except update Git objects and refs).
|
||||
|
||||
Let's disallow the use of this feature by default, and provide a
|
||||
command-line option to re-enable it (you can always just use the
|
||||
command-line --export-marks as well, but the in-stream version provides
|
||||
an easy way for exporters to control the process).
|
||||
|
||||
This is a backwards-incompatible change, since the default is flipping
|
||||
to the new, safer behavior. However, since the main users of the
|
||||
in-stream versions would be import/export-based remote helpers, and
|
||||
since we trust remote helpers already (which are already running
|
||||
arbitrary code), we'll pass the new option by default when reading a
|
||||
remote helper's stream. This should minimize the impact.
|
||||
|
||||
Note that the implementation isn't totally simple, as we have to work
|
||||
around the fact that fast-import doesn't parse its command-line options
|
||||
until after it has read any "feature" lines from the stream. This is how
|
||||
it lets command-line options override in-stream. But in our case, it's
|
||||
important to parse the new --allow-unsafe-features first.
|
||||
|
||||
There are three options for resolving this:
|
||||
|
||||
1. Do a separate "early" pass over the options. This is easy for us to
|
||||
do because there are no command-line options that allow the
|
||||
"unstuck" form (so there's no chance of us mistaking an argument
|
||||
for an option), though it does introduce a risk of incorrect
|
||||
parsing later (e.g,. if we convert to parse-options).
|
||||
|
||||
2. Move the option parsing phase back to the start of the program, but
|
||||
teach the stream-reading code never to override an existing value.
|
||||
This is tricky, because stream "feature" lines override each other
|
||||
(meaning we'd have to start tracking the source for every option).
|
||||
|
||||
3. Accept that we might parse a "feature export-marks" line that is
|
||||
forbidden, as long we don't _act_ on it until after we've parsed
|
||||
the command line options.
|
||||
|
||||
This would, in fact, work with the current code, but only because
|
||||
the previous patch fixed the export-marks parser to avoid touching
|
||||
the filesystem.
|
||||
|
||||
So while it works, it does carry risk of somebody getting it wrong
|
||||
in the future in a rather subtle and unsafe way.
|
||||
|
||||
I've gone with option (1) here as simple, safe, and unlikely to cause
|
||||
regressions.
|
||||
|
||||
This fixes CVE-2019-1348.
|
||||
|
||||
Signed-off-by: Jeff King <peff@peff.net>
|
||||
---
|
||||
Documentation/git-fast-import.txt | 14 ++++++++++++++
|
||||
fast-import.c | 25 +++++++++++++++++++++++++
|
||||
t/t9300-fast-import.sh | 23 +++++++++++++++--------
|
||||
transport-helper.c | 1 +
|
||||
4 files changed, 55 insertions(+), 8 deletions(-)
|
||||
|
||||
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
|
||||
index fad327a..76747ef 100644
|
||||
--- a/Documentation/git-fast-import.txt
|
||||
+++ b/Documentation/git-fast-import.txt
|
||||
@@ -51,6 +51,20 @@ OPTIONS
|
||||
memory used by fast-import during this run. Showing this output
|
||||
is currently the default, but can be disabled with --quiet.
|
||||
|
||||
+--allow-unsafe-features::
|
||||
+ Many command-line options can be provided as part of the
|
||||
+ fast-import stream itself by using the `feature` or `option`
|
||||
+ commands. However, some of these options are unsafe (e.g.,
|
||||
+ allowing fast-import to access the filesystem outside of the
|
||||
+ repository). These options are disabled by default, but can be
|
||||
+ allowed by providing this option on the command line. This
|
||||
+ currently impacts only the `feature export-marks` command.
|
||||
++
|
||||
+ Only enable this option if you trust the program generating the
|
||||
+ fast-import stream! This option is enabled automatically for
|
||||
+ remote-helpers that use the `import` capability, as they are
|
||||
+ already trusted to run their own code.
|
||||
+
|
||||
Options for Frontends
|
||||
~~~~~~~~~~~~~~~~~~~~~
|
||||
|
||||
diff --git a/fast-import.c b/fast-import.c
|
||||
index 71312d4..b001abc 100644
|
||||
--- a/fast-import.c
|
||||
+++ b/fast-import.c
|
||||
@@ -217,6 +217,7 @@ struct recent_command {
|
||||
static struct strbuf new_data = STRBUF_INIT;
|
||||
static int seen_data_command;
|
||||
static int require_explicit_termination;
|
||||
+static int allow_unsafe_features;
|
||||
|
||||
/* Signal handling */
|
||||
static volatile sig_atomic_t checkpoint_requested;
|
||||
@@ -3171,6 +3172,8 @@ static int parse_one_option(const char *option)
|
||||
show_stats = 0;
|
||||
} else if (!strcmp(option, "stats")) {
|
||||
show_stats = 1;
|
||||
+ } else if (!strcmp(option, "allow-unsafe-features")) {
|
||||
+ ; /* already handled during early option parsing */
|
||||
} else {
|
||||
return 0;
|
||||
}
|
||||
@@ -3178,6 +3181,13 @@ static int parse_one_option(const char *option)
|
||||
return 1;
|
||||
}
|
||||
|
||||
+static void check_unsafe_feature(const char *feature, int from_stream)
|
||||
+{
|
||||
+ if (from_stream && !allow_unsafe_features)
|
||||
+ die(_("feature '%s' forbidden in input without --allow-unsafe-features"),
|
||||
+ feature);
|
||||
+}
|
||||
+
|
||||
static int parse_one_feature(const char *feature, int from_stream)
|
||||
{
|
||||
const char *arg;
|
||||
@@ -3189,6 +3199,7 @@ static int parse_one_feature(const char *feature, int from_stream)
|
||||
} else if (skip_prefix(feature, "import-marks-if-exists=", &arg)) {
|
||||
option_import_marks(arg, from_stream, 1);
|
||||
} else if (skip_prefix(feature, "export-marks=", &arg)) {
|
||||
+ check_unsafe_feature(feature, from_stream);
|
||||
option_export_marks(arg);
|
||||
} else if (!strcmp(feature, "get-mark")) {
|
||||
; /* Don't die - this feature is supported */
|
||||
@@ -3315,6 +3326,20 @@ int cmd_main(int argc, const char **argv)
|
||||
avail_tree_table = xcalloc(avail_tree_table_sz, sizeof(struct avail_tree_content*));
|
||||
marks = mem_pool_calloc(&fi_mem_pool, 1, sizeof(struct mark_set));
|
||||
|
||||
+ /*
|
||||
+ * We don't parse most options until after we've seen the set of
|
||||
+ * "feature" lines at the start of the stream (which allows the command
|
||||
+ * line to override stream data). But we must do an early parse of any
|
||||
+ * command-line options that impact how we interpret the feature lines.
|
||||
+ */
|
||||
+ for (i = 1; i < argc; i++) {
|
||||
+ const char *arg = argv[i];
|
||||
+ if (*arg != '-' || !strcmp(arg, "--"))
|
||||
+ break;
|
||||
+ if (!strcmp(arg, "--allow-unsafe-features"))
|
||||
+ allow_unsafe_features = 1;
|
||||
+ }
|
||||
+
|
||||
global_argc = argc;
|
||||
global_argv = argv;
|
||||
|
||||
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
|
||||
index 913ff89..8ca58f5 100755
|
||||
--- a/t/t9300-fast-import.sh
|
||||
+++ b/t/t9300-fast-import.sh
|
||||
@@ -2115,6 +2115,11 @@ test_expect_success 'R: only one import-marks feature allowed per stream' '
|
||||
test_must_fail git fast-import <input
|
||||
'
|
||||
|
||||
+test_expect_success 'R: export-marks feature forbidden by default' '
|
||||
+ echo "feature export-marks=git.marks" >input &&
|
||||
+ test_must_fail git fast-import <input
|
||||
+'
|
||||
+
|
||||
test_expect_success 'R: export-marks feature results in a marks file being created' '
|
||||
cat >input <<-EOF &&
|
||||
feature export-marks=git.marks
|
||||
@@ -2125,7 +2130,7 @@ test_expect_success 'R: export-marks feature results in a marks file being creat
|
||||
|
||||
EOF
|
||||
|
||||
- git fast-import <input &&
|
||||
+ git fast-import --allow-unsafe-features <input &&
|
||||
grep :1 git.marks
|
||||
'
|
||||
|
||||
@@ -2138,7 +2143,8 @@ test_expect_success 'R: export-marks options can be overridden by commandline op
|
||||
hi
|
||||
|
||||
EOF
|
||||
- git fast-import --export-marks=cmdline-sub/other.marks <input &&
|
||||
+ git fast-import --allow-unsafe-features \
|
||||
+ --export-marks=cmdline-sub/other.marks <input &&
|
||||
grep :1 cmdline-sub/other.marks &&
|
||||
test_path_is_missing feature-sub
|
||||
'
|
||||
@@ -2146,7 +2152,7 @@ test_expect_success 'R: export-marks options can be overridden by commandline op
|
||||
test_expect_success 'R: catch typo in marks file name' '
|
||||
test_must_fail git fast-import --import-marks=nonexistent.marks </dev/null &&
|
||||
echo "feature import-marks=nonexistent.marks" |
|
||||
- test_must_fail git fast-import
|
||||
+ test_must_fail git fast-import --allow-unsafe-features
|
||||
'
|
||||
|
||||
test_expect_success 'R: import and output marks can be the same file' '
|
||||
@@ -2248,7 +2254,7 @@ test_expect_success 'R: import to output marks works without any content' '
|
||||
feature export-marks=marks.new
|
||||
EOF
|
||||
|
||||
- git fast-import <input &&
|
||||
+ git fast-import --allow-unsafe-features <input &&
|
||||
test_cmp marks.out marks.new
|
||||
'
|
||||
|
||||
@@ -2258,7 +2264,7 @@ test_expect_success 'R: import marks prefers commandline marks file over the str
|
||||
feature export-marks=marks.new
|
||||
EOF
|
||||
|
||||
- git fast-import --import-marks=marks.out <input &&
|
||||
+ git fast-import --import-marks=marks.out --allow-unsafe-features <input &&
|
||||
test_cmp marks.out marks.new
|
||||
'
|
||||
|
||||
@@ -2271,7 +2277,8 @@ test_expect_success 'R: multiple --import-marks= should be honoured' '
|
||||
|
||||
head -n2 marks.out > one.marks &&
|
||||
tail -n +3 marks.out > two.marks &&
|
||||
- git fast-import --import-marks=one.marks --import-marks=two.marks <input &&
|
||||
+ git fast-import --import-marks=one.marks --import-marks=two.marks \
|
||||
+ --allow-unsafe-features <input &&
|
||||
test_cmp marks.out combined.marks
|
||||
'
|
||||
|
||||
@@ -2284,7 +2291,7 @@ test_expect_success 'R: feature relative-marks should be honoured' '
|
||||
|
||||
mkdir -p .git/info/fast-import/ &&
|
||||
cp marks.new .git/info/fast-import/relative.in &&
|
||||
- git fast-import <input &&
|
||||
+ git fast-import --allow-unsafe-features <input &&
|
||||
test_cmp marks.new .git/info/fast-import/relative.out
|
||||
'
|
||||
|
||||
@@ -2296,7 +2303,7 @@ test_expect_success 'R: feature no-relative-marks should be honoured' '
|
||||
feature export-marks=non-relative.out
|
||||
EOF
|
||||
|
||||
- git fast-import <input &&
|
||||
+ git fast-import --allow-unsafe-features <input &&
|
||||
test_cmp marks.new non-relative.out
|
||||
'
|
||||
|
||||
diff --git a/transport-helper.c b/transport-helper.c
|
||||
index 6b05a88..f828a6d 100644
|
||||
--- a/transport-helper.c
|
||||
+++ b/transport-helper.c
|
||||
@@ -425,6 +425,7 @@ static int get_importer(struct transport *transport, struct child_process *fasti
|
||||
child_process_init(fastimport);
|
||||
fastimport->in = xdup(helper->out);
|
||||
argv_array_push(&fastimport->args, "fast-import");
|
||||
+ argv_array_push(&fastimport->args, "--allow-unsafe-features");
|
||||
argv_array_push(&fastimport->args, debug ? "--stats" : "--quiet");
|
||||
|
||||
if (data->bidi_import) {
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,237 +0,0 @@
|
||||
From 629226a8902640470bb8cf9521f058340e7c0248 Mon Sep 17 00:00:00 2001
|
||||
From: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
Date: Thu, 12 Sep 2019 14:20:39 +0200
|
||||
Subject: [PATCH 07/30] clone --recurse-submodules: prevent name squatting on
|
||||
Windows
|
||||
|
||||
In addition to preventing `.git` from being tracked by Git, on Windows
|
||||
we also have to prevent `git~1` from being tracked, as the default NTFS
|
||||
short name (also known as the "8.3 filename") for the file name `.git`
|
||||
is `git~1`, otherwise it would be possible for malicious repositories to
|
||||
write directly into the `.git/` directory, e.g. a `post-checkout` hook
|
||||
that would then be executed _during_ a recursive clone.
|
||||
|
||||
When we implemented appropriate protections in 2b4c6efc821 (read-cache:
|
||||
optionally disallow NTFS .git variants, 2014-12-16), we had analyzed
|
||||
carefully that the `.git` directory or file would be guaranteed to be
|
||||
the first directory entry to be written. Otherwise it would be possible
|
||||
e.g. for a file named `..git` to be assigned the short name `git~1` and
|
||||
subsequently, the short name generated for `.git` would be `git~2`. Or
|
||||
`git~3`. Or even `~9999999` (for a detailed explanation of the lengths
|
||||
we have to go to protect `.gitmodules`, see the commit message of
|
||||
e7cb0b4455c (is_ntfs_dotgit: match other .git files, 2018-05-11)).
|
||||
|
||||
However, by exploiting two issues (that will be addressed in a related
|
||||
patch series close by), it is currently possible to clone a submodule
|
||||
into a non-empty directory:
|
||||
|
||||
- On Windows, file names cannot end in a space or a period (for
|
||||
historical reasons: the period separating the base name from the file
|
||||
extension was not actually written to disk, and the base name/file
|
||||
extension was space-padded to the full 8/3 characters, respectively).
|
||||
Helpfully, when creating a directory under the name, say, `sub.`, that
|
||||
trailing period is trimmed automatically and the actual name on disk
|
||||
is `sub`.
|
||||
|
||||
This means that while Git thinks that the submodule names `sub` and
|
||||
`sub.` are different, they both access `.git/modules/sub/`.
|
||||
|
||||
- While the backslash character is a valid file name character on Linux,
|
||||
it is not so on Windows. As Git tries to be cross-platform, it
|
||||
therefore allows backslash characters in the file names stored in tree
|
||||
objects.
|
||||
|
||||
Which means that it is totally possible that a submodule `c` sits next
|
||||
to a file `c\..git`, and on Windows, during recursive clone a file
|
||||
called `..git` will be written into `c/`, of course _before_ the
|
||||
submodule is cloned.
|
||||
|
||||
Note that the actual exploit is not quite as simple as having a
|
||||
submodule `c` next to a file `c\..git`, as we have to make sure that the
|
||||
directory `.git/modules/b` already exists when the submodule is checked
|
||||
out, otherwise a different code path is taken in `module_clone()` that
|
||||
does _not_ allow a non-empty submodule directory to exist already.
|
||||
|
||||
Even if we will address both issues nearby (the next commit will
|
||||
disallow backslash characters in tree entries' file names on Windows,
|
||||
and another patch will disallow creating directories/files with trailing
|
||||
spaces or periods), it is a wise idea to defend in depth against this
|
||||
sort of attack vector: when submodules are cloned recursively, we now
|
||||
_require_ the directory to be empty, addressing CVE-2019-1349.
|
||||
|
||||
Note: the code path we patch is shared with the code path of `git
|
||||
submodule update --init`, which must not expect, in general, that the
|
||||
directory is empty. Hence we have to introduce the new option
|
||||
`--force-init` and hand it all the way down from `git submodule` to the
|
||||
actual `git submodule--helper` process that performs the initial clone.
|
||||
|
||||
Reported-by: Nicolas Joly <Nicolas.Joly@microsoft.com>
|
||||
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
---
|
||||
builtin/clone.c | 2 +-
|
||||
builtin/submodule--helper.c | 14 ++++++++++++--
|
||||
git-submodule.sh | 6 ++++++
|
||||
t/t7415-submodule-names.sh | 31 +++++++++++++++++++++++++++++++
|
||||
4 files changed, 50 insertions(+), 3 deletions(-)
|
||||
|
||||
diff --git a/builtin/clone.c b/builtin/clone.c
|
||||
index f665b28..e48476d 100644
|
||||
--- a/builtin/clone.c
|
||||
+++ b/builtin/clone.c
|
||||
@@ -790,7 +790,7 @@ static int checkout(int submodule_progress)
|
||||
|
||||
if (!err && (option_recurse_submodules.nr > 0)) {
|
||||
struct argv_array args = ARGV_ARRAY_INIT;
|
||||
- argv_array_pushl(&args, "submodule", "update", "--init", "--recursive", NULL);
|
||||
+ argv_array_pushl(&args, "submodule", "update", "--require-init", "--recursive", NULL);
|
||||
|
||||
if (option_shallow_submodules == 1)
|
||||
argv_array_push(&args, "--depth=1");
|
||||
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
|
||||
index 909e77e..f3ba6aa 100644
|
||||
--- a/builtin/submodule--helper.c
|
||||
+++ b/builtin/submodule--helper.c
|
||||
@@ -19,6 +19,7 @@
|
||||
#include "diffcore.h"
|
||||
#include "diff.h"
|
||||
#include "object-store.h"
|
||||
+#include "dir.h"
|
||||
|
||||
#define OPT_QUIET (1 << 0)
|
||||
#define OPT_CACHED (1 << 1)
|
||||
@@ -1359,7 +1360,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
|
||||
char *p, *path = NULL, *sm_gitdir;
|
||||
struct strbuf sb = STRBUF_INIT;
|
||||
struct string_list reference = STRING_LIST_INIT_NODUP;
|
||||
- int dissociate = 0;
|
||||
+ int dissociate = 0, require_init = 0;
|
||||
char *sm_alternate = NULL, *error_strategy = NULL;
|
||||
|
||||
struct option module_clone_options[] = {
|
||||
@@ -1386,6 +1387,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
|
||||
OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
|
||||
OPT_BOOL(0, "progress", &progress,
|
||||
N_("force cloning progress")),
|
||||
+ OPT_BOOL(0, "require-init", &require_init,
|
||||
+ N_("disallow cloning into non-empty directory")),
|
||||
OPT_END()
|
||||
};
|
||||
|
||||
@@ -1424,6 +1427,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
|
||||
die(_("clone of '%s' into submodule path '%s' failed"),
|
||||
url, path);
|
||||
} else {
|
||||
+ if (require_init && !access(path, X_OK) && !is_empty_dir(path))
|
||||
+ die(_("directory not empty: '%s'"), path);
|
||||
if (safe_create_leading_directories_const(path) < 0)
|
||||
die(_("could not create directory '%s'"), path);
|
||||
strbuf_addf(&sb, "%s/index", sm_gitdir);
|
||||
@@ -1536,6 +1541,7 @@ struct submodule_update_clone {
|
||||
int recommend_shallow;
|
||||
struct string_list references;
|
||||
int dissociate;
|
||||
+ unsigned require_init;
|
||||
const char *depth;
|
||||
const char *recursive_prefix;
|
||||
const char *prefix;
|
||||
@@ -1554,7 +1560,7 @@ struct submodule_update_clone {
|
||||
int max_jobs;
|
||||
};
|
||||
#define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
|
||||
- SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \
|
||||
+ SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, 0, \
|
||||
NULL, NULL, NULL, \
|
||||
NULL, 0, 0, 0, NULL, 0, 0, 1}
|
||||
|
||||
@@ -1681,6 +1687,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
|
||||
argv_array_pushl(&child->args, "--prefix", suc->prefix, NULL);
|
||||
if (suc->recommend_shallow && sub->recommend_shallow == 1)
|
||||
argv_array_push(&child->args, "--depth=1");
|
||||
+ if (suc->require_init)
|
||||
+ argv_array_push(&child->args, "--require-init");
|
||||
argv_array_pushl(&child->args, "--path", sub->path, NULL);
|
||||
argv_array_pushl(&child->args, "--name", sub->name, NULL);
|
||||
argv_array_pushl(&child->args, "--url", url, NULL);
|
||||
@@ -1870,6 +1878,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
|
||||
OPT__QUIET(&suc.quiet, N_("don't print cloning progress")),
|
||||
OPT_BOOL(0, "progress", &suc.progress,
|
||||
N_("force cloning progress")),
|
||||
+ OPT_BOOL(0, "require-init", &suc.require_init,
|
||||
+ N_("disallow cloning into non-empty directory")),
|
||||
OPT_END()
|
||||
};
|
||||
|
||||
diff --git a/git-submodule.sh b/git-submodule.sh
|
||||
index c7f58c5..58713c5 100755
|
||||
--- a/git-submodule.sh
|
||||
+++ b/git-submodule.sh
|
||||
@@ -36,6 +36,7 @@ reference=
|
||||
cached=
|
||||
recursive=
|
||||
init=
|
||||
+require_init=
|
||||
files=
|
||||
remote=
|
||||
nofetch=
|
||||
@@ -466,6 +467,10 @@ cmd_update()
|
||||
-i|--init)
|
||||
init=1
|
||||
;;
|
||||
+ --require-init)
|
||||
+ init=1
|
||||
+ require_init=1
|
||||
+ ;;
|
||||
--remote)
|
||||
remote=1
|
||||
;;
|
||||
@@ -548,6 +553,7 @@ cmd_update()
|
||||
${reference:+"$reference"} \
|
||||
${dissociate:+"--dissociate"} \
|
||||
${depth:+--depth "$depth"} \
|
||||
+ ${require_init:+--require-init} \
|
||||
$recommend_shallow \
|
||||
$jobs \
|
||||
-- \
|
||||
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
|
||||
index 49a37ef..75dcdff 100755
|
||||
--- a/t/t7415-submodule-names.sh
|
||||
+++ b/t/t7415-submodule-names.sh
|
||||
@@ -191,4 +191,35 @@ test_expect_success 'fsck detects corrupt .gitmodules' '
|
||||
)
|
||||
'
|
||||
|
||||
+test_expect_success MINGW 'prevent git~1 squatting on Windows' '
|
||||
+ git init squatting &&
|
||||
+ (
|
||||
+ cd squatting &&
|
||||
+ mkdir a &&
|
||||
+ touch a/..git &&
|
||||
+ git add a/..git &&
|
||||
+ test_tick &&
|
||||
+ git commit -m initial &&
|
||||
+
|
||||
+ modules="$(test_write_lines \
|
||||
+ "[submodule \"b.\"]" "url = ." "path = c" \
|
||||
+ "[submodule \"b\"]" "url = ." "path = d\\\\a" |
|
||||
+ git hash-object -w --stdin)" &&
|
||||
+ rev="$(git rev-parse --verify HEAD)" &&
|
||||
+ hash="$(echo x | git hash-object -w --stdin)" &&
|
||||
+ git update-index --add \
|
||||
+ --cacheinfo 100644,$modules,.gitmodules \
|
||||
+ --cacheinfo 160000,$rev,c \
|
||||
+ --cacheinfo 160000,$rev,d\\a \
|
||||
+ --cacheinfo 100644,$hash,d./a/x \
|
||||
+ --cacheinfo 100644,$hash,d./a/..git &&
|
||||
+ test_tick &&
|
||||
+ git commit -m "module"
|
||||
+ ) &&
|
||||
+ test_must_fail git \
|
||||
+ clone --recurse-submodules squatting squatting-clone 2>err &&
|
||||
+ test_i18ngrep "directory not empty" err &&
|
||||
+ ! grep gitdir squatting-clone/d/a/git~2
|
||||
+'
|
||||
+
|
||||
test_done
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,99 +0,0 @@
|
||||
From 6d8684161ee9c03bed5cb69ae76dfdddb85a0003 Mon Sep 17 00:00:00 2001
|
||||
From: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
Date: Fri, 13 Sep 2019 16:32:43 +0200
|
||||
Subject: [PATCH] mingw: fix quoting of arguments
|
||||
|
||||
We need to be careful to follow proper quoting rules. For example, if an
|
||||
argument contains spaces, we have to quote them. Double-quotes need to
|
||||
be escaped. Backslashes need to be escaped, but only if they are
|
||||
followed by a double-quote character.
|
||||
|
||||
We need to be _extra_ careful to consider the case where an argument
|
||||
ends in a backslash _and_ needs to be quoted: in this case, we append a
|
||||
double-quote character, i.e. the backslash now has to be escaped!
|
||||
|
||||
The current code, however, fails to recognize that, and therefore can
|
||||
turn an argument that ends in a single backslash into a quoted argument
|
||||
that now ends in an escaped double-quote character. This allows
|
||||
subsequent command-line parameters to be split and part of them being
|
||||
mistaken for command-line options, e.g. through a maliciously-crafted
|
||||
submodule URL during a recursive clone.
|
||||
|
||||
|
||||
Technically, we would not need to quote _all_ arguments which end in a
|
||||
backslash _unless_ the argument needs to be quoted anyway. For example,
|
||||
`test\` would not need to be quoted, while `test \` would need to be.
|
||||
|
||||
To keep the code simple, however, and therefore easier to reason about
|
||||
and ensure its correctness, we now _always_ quote an argument that ends
|
||||
in a backslash.
|
||||
|
||||
This addresses CVE-2019-1350.
|
||||
|
||||
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
---
|
||||
compat/mingw.c | 9 ++++++---
|
||||
t/t7416-submodule-dash-url.sh | 14 ++++++++++++++
|
||||
2 files changed, 20 insertions(+), 3 deletions(-)
|
||||
|
||||
diff --git a/compat/mingw.c b/compat/mingw.c
|
||||
index 738f0a8..48cdf83 100644
|
||||
--- a/compat/mingw.c
|
||||
+++ b/compat/mingw.c
|
||||
@@ -1052,7 +1052,7 @@ static const char *quote_arg_msvc(const char *arg)
|
||||
p++;
|
||||
len++;
|
||||
}
|
||||
- if (*p == '"')
|
||||
+ if (*p == '"' || !*p)
|
||||
n += count*2 + 1;
|
||||
continue;
|
||||
}
|
||||
@@ -1074,16 +1074,19 @@ static const char *quote_arg_msvc(const char *arg)
|
||||
count++;
|
||||
*d++ = *arg++;
|
||||
}
|
||||
- if (*arg == '"') {
|
||||
+ if (*arg == '"' || !*arg) {
|
||||
while (count-- > 0)
|
||||
*d++ = '\\';
|
||||
+ /* don't escape the surrounding end quote */
|
||||
+ if (!*arg)
|
||||
+ break;
|
||||
*d++ = '\\';
|
||||
}
|
||||
}
|
||||
*d++ = *arg++;
|
||||
}
|
||||
*d++ = '"';
|
||||
- *d++ = 0;
|
||||
+ *d++ = '\0';
|
||||
return q;
|
||||
}
|
||||
|
||||
diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh
|
||||
index 1cd2c1c..5ba041f 100755
|
||||
--- a/t/t7416-submodule-dash-url.sh
|
||||
+++ b/t/t7416-submodule-dash-url.sh
|
||||
@@ -46,4 +46,18 @@ test_expect_success 'fsck rejects unprotected dash' '
|
||||
grep gitmodulesUrl err
|
||||
'
|
||||
|
||||
+test_expect_success 'trailing backslash is handled correctly' '
|
||||
+ git init testmodule &&
|
||||
+ test_commit -C testmodule c &&
|
||||
+ git submodule add ./testmodule &&
|
||||
+ : ensure that the name ends in a double backslash &&
|
||||
+ sed -e "s|\\(submodule \"testmodule\\)\"|\\1\\\\\\\\\"|" \
|
||||
+ -e "s|url = .*|url = \" --should-not-be-an-option\"|" \
|
||||
+ <.gitmodules >.new &&
|
||||
+ mv .new .gitmodules &&
|
||||
+ git commit -am "Add testmodule" &&
|
||||
+ test_must_fail git clone --verbose --recurse-submodules . dolly 2>err &&
|
||||
+ test_i18ngrep ! "unknown option" err
|
||||
+'
|
||||
+
|
||||
test_done
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,135 +0,0 @@
|
||||
From a94971b023f4583ea4b2c9c9f7a71d53f9781d58 Mon Sep 17 00:00:00 2001
|
||||
From: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
Date: Fri, 6 Sep 2019 00:09:10 +0200
|
||||
Subject: [PATCH] mingw: handle `subst`-ed "DOS drives"
|
||||
MIME-Version: 1.0
|
||||
Content-Type: text/plain; charset=UTF-8
|
||||
Content-Transfer-Encoding: 8bit
|
||||
|
||||
Over a decade ago, in 25fe217b86c (Windows: Treat Windows style path
|
||||
names., 2008-03-05), Git was taught to handle absolute Windows paths,
|
||||
i.e. paths that start with a drive letter and a colon.
|
||||
|
||||
Unbeknownst to us, while drive letters of physical drives are limited to
|
||||
letters of the English alphabet, there is a way to assign virtual drive
|
||||
letters to arbitrary directories, via the `subst` command, which is
|
||||
_not_ limited to English letters.
|
||||
|
||||
It is therefore possible to have absolute Windows paths of the form
|
||||
`1:\what\the\hex.txt`. Even "better": pretty much arbitrary Unicode
|
||||
letters can also be used, e.g. `ä:\tschibät.sch`.
|
||||
|
||||
While it can be sensibly argued that users who set up such funny drive
|
||||
letters really seek adverse consequences, the Windows Operating System
|
||||
is known to be a platform where many users are at the mercy of
|
||||
administrators who have their very own idea of what constitutes a
|
||||
reasonable setup.
|
||||
|
||||
Therefore, let's just make sure that such funny paths are still
|
||||
considered absolute paths by Git, on Windows.
|
||||
|
||||
In addition to Unicode characters, pretty much any character is a valid
|
||||
drive letter, as far as `subst` is concerned, even `:` and `"` or even a
|
||||
space character. While it is probably the opposite of smart to use them,
|
||||
let's safeguard `is_dos_drive_prefix()` against all of them.
|
||||
|
||||
Note: `[::1]:repo` is a valid URL, but not a valid path on Windows.
|
||||
As `[` is now considered a valid drive letter, we need to be very
|
||||
careful to avoid misinterpreting such a string as valid local path in
|
||||
`url_is_local_not_ssh()`. To do that, we use the just-introduced
|
||||
function `is_valid_path()` (which will label the string as invalid file
|
||||
name because of the colon characters).
|
||||
|
||||
This fixes CVE-2019-1351.
|
||||
|
||||
Reported-by: Nicolas Joly <Nicolas.Joly@microsoft.com>
|
||||
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
---
|
||||
compat/win32/path-utils.c | 24 ++++++++++++++++++++++++
|
||||
compat/win32/path-utils.h | 4 ++--
|
||||
connect.c | 2 +-
|
||||
t/t0060-path-utils.sh | 9 +++++++++
|
||||
4 files changed, 36 insertions(+), 3 deletions(-)
|
||||
|
||||
diff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c
|
||||
index d9d3641de8..70352af283 100644
|
||||
--- a/compat/win32/path-utils.c
|
||||
+++ b/compat/win32/path-utils.c
|
||||
@@ -1,5 +1,29 @@
|
||||
#include "../../git-compat-util.h"
|
||||
|
||||
+int mingw_has_dos_drive_prefix(const char *path)
|
||||
+{
|
||||
+ int i;
|
||||
+
|
||||
+ /*
|
||||
+ * Does it start with an ASCII letter (i.e. highest bit not set),
|
||||
+ * followed by a colon?
|
||||
+ */
|
||||
+ if (!(0x80 & (unsigned char)*path))
|
||||
+ return *path && path[1] == ':' ? 2 : 0;
|
||||
+
|
||||
+ /*
|
||||
+ * While drive letters must be letters of the English alphabet, it is
|
||||
+ * possible to assign virtually _any_ Unicode character via `subst` as
|
||||
+ * a drive letter to "virtual drives". Even `1`, or `ä`. Or fun stuff
|
||||
+ * like this:
|
||||
+ *
|
||||
+ * subst ֍: %USERPROFILE%\Desktop
|
||||
+ */
|
||||
+ for (i = 1; i < 4 && (0x80 & (unsigned char)path[i]); i++)
|
||||
+ ; /* skip first UTF-8 character */
|
||||
+ return path[i] == ':' ? i + 1 : 0;
|
||||
+}
|
||||
+
|
||||
int win32_skip_dos_drive_prefix(char **path)
|
||||
{
|
||||
int ret = has_dos_drive_prefix(*path);
|
||||
diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h
|
||||
index 0f70d43920..22b0c63349 100644
|
||||
--- a/compat/win32/path-utils.h
|
||||
+++ b/compat/win32/path-utils.h
|
||||
@@ -1,5 +1,5 @@
|
||||
-#define has_dos_drive_prefix(path) \
|
||||
- (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
|
||||
+int mingw_has_dos_drive_prefix(const char *path);
|
||||
+#define has_dos_drive_prefix mingw_has_dos_drive_prefix
|
||||
int win32_skip_dos_drive_prefix(char **path);
|
||||
#define skip_dos_drive_prefix win32_skip_dos_drive_prefix
|
||||
static inline int win32_is_dir_sep(int c)
|
||||
diff --git a/connect.c b/connect.c
|
||||
index 2778481264..4050a797bf 100644
|
||||
--- a/connect.c
|
||||
+++ b/connect.c
|
||||
@@ -511,7 +511,7 @@ int url_is_local_not_ssh(const char *url)
|
||||
const char *colon = strchr(url, ':');
|
||||
const char *slash = strchr(url, '/');
|
||||
return !colon || (slash && slash < colon) ||
|
||||
- has_dos_drive_prefix(url);
|
||||
+ (has_dos_drive_prefix(url) && is_valid_path(url));
|
||||
}
|
||||
|
||||
static const char *prot_name(enum protocol protocol)
|
||||
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
|
||||
index c7b53e494b..e9b61efdff 100755
|
||||
--- a/t/t0060-path-utils.sh
|
||||
+++ b/t/t0060-path-utils.sh
|
||||
@@ -165,6 +165,15 @@ test_expect_success 'absolute path rejects the empty string' '
|
||||
test_must_fail test-tool path-utils absolute_path ""
|
||||
'
|
||||
|
||||
+test_expect_success MINGW '<drive-letter>:\\abc is an absolute path' '
|
||||
+ for letter in : \" C Z 1 ä
|
||||
+ do
|
||||
+ path=$letter:\\abc &&
|
||||
+ absolute="$(test-path-utils absolute_path "$path")" &&
|
||||
+ test "$path" = "$absolute" || return 1
|
||||
+ done
|
||||
+'
|
||||
+
|
||||
test_expect_success 'real path rejects the empty string' '
|
||||
test_must_fail test-tool path-utils real_path ""
|
||||
'
|
||||
--
|
||||
2.23.0
|
||||
|
||||
@ -1,94 +0,0 @@
|
||||
From 7c3745fc6185495d5765628b4dfe1bd2c25a2981 Mon Sep 17 00:00:00 2001
|
||||
From: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
Date: Wed, 28 Aug 2019 12:22:17 +0200
|
||||
Subject: [PATCH] path: safeguard `.git` against NTFS Alternate Streams
|
||||
Accesses
|
||||
|
||||
Probably inspired by HFS' resource streams, NTFS supports "Alternate
|
||||
Data Streams": by appending `:<stream-name>` to the file name,
|
||||
information in addition to the file contents can be written and read,
|
||||
information that is copied together with the file (unless copied to a
|
||||
non-NTFS location).
|
||||
|
||||
These Alternate Data Streams are typically used for things like marking
|
||||
an executable as having just been downloaded from the internet (and
|
||||
hence not necessarily being trustworthy).
|
||||
|
||||
In addition to a stream name, a stream type can be appended, like so:
|
||||
`:<stream-name>:<stream-type>`. Unless specified, the default stream
|
||||
type is `$DATA` for files and `$INDEX_ALLOCATION` for directories. In
|
||||
other words, `.git::$INDEX_ALLOCATION` is a valid way to reference the
|
||||
`.git` directory!
|
||||
|
||||
In our work in Git v2.2.1 to protect Git on NTFS drives under
|
||||
`core.protectNTFS`, we focused exclusively on NTFS short names, unaware
|
||||
of the fact that NTFS Alternate Data Streams offer a similar attack
|
||||
vector.
|
||||
|
||||
Let's fix this.
|
||||
|
||||
Seeing as it is better to be safe than sorry, we simply disallow paths
|
||||
referring to *any* NTFS Alternate Data Stream of `.git`, not just
|
||||
`::$INDEX_ALLOCATION`. This also simplifies the implementation.
|
||||
|
||||
This closes CVE-2019-1352.
|
||||
|
||||
Further reading about NTFS Alternate Data Streams:
|
||||
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/c54dec26-1551-4d3a-a0ea-4fa40f848eb3
|
||||
|
||||
Reported-by: Nicolas Joly <Nicolas.Joly@microsoft.com>
|
||||
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
---
|
||||
path.c | 12 +++++++++++-
|
||||
t/t1014-read-tree-confusing.sh | 1 +
|
||||
2 files changed, 12 insertions(+), 1 deletion(-)
|
||||
|
||||
diff --git a/path.c b/path.c
|
||||
index f62a37d..e39ecf4 100644
|
||||
--- a/path.c
|
||||
+++ b/path.c
|
||||
@@ -1321,10 +1321,19 @@ static int only_spaces_and_periods(const char *path, size_t len, size_t skip)
|
||||
* `.git` is the first item in a directory, therefore it will be associated
|
||||
* with the short name `git~1` (unless short names are disabled).
|
||||
*
|
||||
+ * - For yet other historical reasons, NTFS supports so-called "Alternate Data
|
||||
+ * Streams", i.e. metadata associated with a given file, referred to via
|
||||
+ * `<filename>:<stream-name>:<stream-type>`. There exists a default stream
|
||||
+ * type for directories, allowing `.git/` to be accessed via
|
||||
+ * `.git::$INDEX_ALLOCATION/`.
|
||||
+ *
|
||||
* When this function returns 1, it indicates that the specified file/directory
|
||||
* name refers to a `.git` file or directory, or to any of these synonyms, and
|
||||
* Git should therefore not track it.
|
||||
*
|
||||
+ * For performance reasons, _all_ Alternate Data Streams of `.git/` are
|
||||
+ * forbidden, not just `::$INDEX_ALLOCATION`.
|
||||
+ *
|
||||
* This function is intended to be used by `git fsck` even on platforms where
|
||||
* the backslash is a regular filename character, therefore it needs to handle
|
||||
* backlash characters in the provided `name` specially: they are interpreted
|
||||
@@ -1335,7 +1344,8 @@ int is_ntfs_dotgit(const char *name)
|
||||
size_t len;
|
||||
|
||||
for (len = 0; ; len++)
|
||||
- if (!name[len] || name[len] == '\\' || is_dir_sep(name[len])) {
|
||||
+ if (!name[len] || name[len] == '\\' || is_dir_sep(name[len]) ||
|
||||
+ name[len] == ':') {
|
||||
if (only_spaces_and_periods(name, len, 4) &&
|
||||
!strncasecmp(name, ".git", 4))
|
||||
return 1;
|
||||
diff --git a/t/t1014-read-tree-confusing.sh b/t/t1014-read-tree-confusing.sh
|
||||
index 2f5a25d..da3376b 100755
|
||||
--- a/t/t1014-read-tree-confusing.sh
|
||||
+++ b/t/t1014-read-tree-confusing.sh
|
||||
@@ -49,6 +49,7 @@ git~1
|
||||
.git.SPACE .git.{space}
|
||||
.\\\\.GIT\\\\foobar backslashes
|
||||
.git\\\\foobar backslashes2
|
||||
+.git...:alternate-stream
|
||||
EOF
|
||||
|
||||
test_expect_success 'utf-8 paths allowed with core.protectHFS off' '
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,142 +0,0 @@
|
||||
From 9102f958ee5254b10c0be72672aa3305bf4f4704 Mon Sep 17 00:00:00 2001
|
||||
From: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
Date: Mon, 9 Sep 2019 21:04:41 +0200
|
||||
Subject: [PATCH] protect_ntfs: turn on NTFS protection by default
|
||||
|
||||
Back in the DOS days, in the FAT file system, file names always
|
||||
consisted of a base name of length 8 plus a file extension of length 3.
|
||||
Shorter file names were simply padded with spaces to the full 8.3
|
||||
format.
|
||||
|
||||
Later, the FAT file system was taught to support _also_ longer names,
|
||||
with an 8.3 "short name" as primary file name. While at it, the same
|
||||
facility allowed formerly illegal file names, such as `.git` (empty base
|
||||
names were not allowed), which would have the "short name" `git~1`
|
||||
associated with it.
|
||||
|
||||
For backwards-compatibility, NTFS supports alternative 8.3 short
|
||||
filenames, too, even if starting with Windows Vista, they are only
|
||||
generated on the system drive by default.
|
||||
|
||||
We addressed the problem that the `.git/` directory can _also_ be
|
||||
accessed via `git~1/` (when short names are enabled) in 2b4c6efc821
|
||||
(read-cache: optionally disallow NTFS .git variants, 2014-12-16), i.e.
|
||||
since Git v1.9.5, by introducing the config setting `core.protectNTFS`
|
||||
and enabling it by default on Windows.
|
||||
|
||||
In the meantime, Windows 10 introduced the "Windows Subsystem for Linux"
|
||||
(short: WSL), i.e. a way to run Linux applications/distributions in a
|
||||
thinly-isolated subsystem on Windows (giving rise to many a "2016 is the
|
||||
Year of Linux on the Desktop" jokes). WSL is getting increasingly
|
||||
popular, also due to the painless way Linux application can operate
|
||||
directly ("natively") on files on Windows' file system: the Windows
|
||||
drives are mounted automatically (e.g. `C:` as `/mnt/c/`).
|
||||
|
||||
Taken together, this means that we now have to enable the safe-guards of
|
||||
Git v1.9.5 also in WSL: it is possible to access a `.git` directory
|
||||
inside `/mnt/c/` via the 8.3 name `git~1` (unless short name generation
|
||||
was disabled manually). Since regular Linux distributions run in WSL,
|
||||
this means we have to enable `core.protectNTFS` at least on Linux, too.
|
||||
|
||||
To enable Services for Macintosh in Windows NT to store so-called
|
||||
resource forks, NTFS introduced "Alternate Data Streams". Essentially,
|
||||
these constitute additional metadata that are connected to (and copied
|
||||
with) their associated files, and they are accessed via pseudo file
|
||||
names of the form `filename:<stream-name>:<stream-type>`.
|
||||
|
||||
In a recent patch, we extended `core.protectNTFS` to also protect
|
||||
against accesses via NTFS Alternate Data Streams, e.g. to prevent
|
||||
contents of the `.git/` directory to be "tracked" via yet another
|
||||
alternative file name.
|
||||
|
||||
While it is not possible (at least by default) to access files via NTFS
|
||||
Alternate Data Streams from within WSL, the defaults on macOS when
|
||||
mounting network shares via SMB _do_ allow accessing files and
|
||||
directories in that way. Therefore, we need to enable `core.protectNTFS`
|
||||
on macOS by default, too, and really, on any Operating System that can
|
||||
mount network shares via SMB/CIFS.
|
||||
|
||||
A couple of approaches were considered for fixing this:
|
||||
|
||||
1. We could perform a dynamic NTFS check similar to the `core.symlinks`
|
||||
check in `init`/`clone`: instead of trying to create a symbolic link
|
||||
in the `.git/` directory, we could create a test file and try to
|
||||
access `.git/config` via 8.3 name and/or Alternate Data Stream.
|
||||
|
||||
2. We could simply "flip the switch" on `core.protectNTFS`, to make it
|
||||
"on by default".
|
||||
|
||||
The obvious downside of 1. is that it won't protect worktrees that were
|
||||
clone with a vulnerable Git version already. We considered patching code
|
||||
paths that check out files to check whether we're running on an NTFS
|
||||
system dynamically and persist the result in the repository-local config
|
||||
setting `core.protectNTFS`, but in the end decided that this solution
|
||||
would be too fragile, and too involved.
|
||||
|
||||
The obvious downside of 2. is that everybody will have to "suffer" the
|
||||
performance penalty incurred from calling `is_ntfs_dotgit()` on every
|
||||
path, even in setups where.
|
||||
|
||||
After the recent work to accelerate `is_ntfs_dotgit()` in most cases,
|
||||
it looks as if the time spent on validating ten million random
|
||||
file names increases only negligibly (less than 20ms, well within the
|
||||
standard deviation of ~50ms). Therefore the benefits outweigh the cost.
|
||||
|
||||
Another downside of this is that paths that might have been acceptable
|
||||
previously now will be forbidden. Realistically, though, this is an
|
||||
improvement because public Git hosters already would reject any `git
|
||||
push` that contains such file names.
|
||||
|
||||
Note: There might be a similar problem mounting HFS+ on Linux. However,
|
||||
this scenario has been considered unlikely and in light of the cost (in
|
||||
the aforementioned benchmark, `core.protectHFS = true` increased the
|
||||
time from ~440ms to ~610ms), it was decided _not_ to touch the default
|
||||
of `core.protectHFS`.
|
||||
|
||||
This change addresses CVE-2019-1353.
|
||||
|
||||
Reported-by: Nicolas Joly <Nicolas.Joly@microsoft.com>
|
||||
Helped-by: Garima Singh <garima.singh@microsoft.com>
|
||||
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
---
|
||||
config.mak.uname | 3 +--
|
||||
environment.c | 2 +-
|
||||
2 files changed, 2 insertions(+), 3 deletions(-)
|
||||
|
||||
diff --git a/config.mak.uname b/config.mak.uname
|
||||
index db7f06b..3e46a8e 100644
|
||||
--- a/config.mak.uname
|
||||
+++ b/config.mak.uname
|
||||
@@ -454,7 +454,6 @@ ifneq ($(USE_MSVC_CRTDBG),)
|
||||
# Optionally enable memory leak reporting.
|
||||
BASIC_CFLAGS += -DUSE_MSVC_CRTDBG
|
||||
endif
|
||||
- BASIC_CFLAGS += -DPROTECT_NTFS_DEFAULT=1
|
||||
# Always give "-Zi" to the compiler and "-debug" to linker (even in
|
||||
# release mode) to force a PDB to be generated (like RelWithDebInfo).
|
||||
BASIC_CFLAGS += -Zi
|
||||
@@ -616,7 +615,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
|
||||
compat/win32/path-utils.o \
|
||||
compat/win32/pthread.o compat/win32/syslog.o \
|
||||
compat/win32/dirent.o
|
||||
- BASIC_CFLAGS += -DWIN32 -DPROTECT_NTFS_DEFAULT=1
|
||||
+ BASIC_CFLAGS += -DWIN32
|
||||
EXTLIBS += -lws2_32
|
||||
GITLIBS += git.res
|
||||
PTHREAD_LIBS =
|
||||
diff --git a/environment.c b/environment.c
|
||||
index 89af47c..9f5f381 100644
|
||||
--- a/environment.c
|
||||
+++ b/environment.c
|
||||
@@ -80,7 +80,7 @@
|
||||
int protect_hfs = PROTECT_HFS_DEFAULT;
|
||||
|
||||
#ifndef PROTECT_NTFS_DEFAULT
|
||||
-#define PROTECT_NTFS_DEFAULT 0
|
||||
+#define PROTECT_NTFS_DEFAULT 1
|
||||
#endif
|
||||
int protect_ntfs = PROTECT_NTFS_DEFAULT;
|
||||
const char *core_fsmonitor;
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,105 +0,0 @@
|
||||
From e1d911dd4c7b76a5a8cec0f5c8de15981e34da83 Mon Sep 17 00:00:00 2001
|
||||
From: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
Date: Thu, 12 Sep 2019 14:54:05 +0200
|
||||
Subject: [PATCH] mingw: disallow backslash characters in tree objects' file
|
||||
names
|
||||
|
||||
The backslash character is not a valid part of a file name on Windows.
|
||||
Hence it is dangerous to allow writing files that were unpacked from
|
||||
tree objects, when the stored file name contains a backslash character:
|
||||
it will be misinterpreted as directory separator.
|
||||
|
||||
This not only causes ambiguity when a tree contains a blob `a\b` and a
|
||||
tree `a` that contains a blob `b`, but it also can be used as part of an
|
||||
attack vector to side-step the careful protections against writing into
|
||||
the `.git/` directory during a clone of a maliciously-crafted
|
||||
repository.
|
||||
|
||||
Let's prevent that, addressing CVE-2019-1354.
|
||||
|
||||
Note: we guard against backslash characters in tree objects' file names
|
||||
_only_ on Windows (because on other platforms, even on those where NTFS
|
||||
volumes can be mounted, the backslash character is _not_ a directory
|
||||
separator), and _only_ when `core.protectNTFS = true` (because users
|
||||
might need to generate tree objects for other platforms, of course
|
||||
without touching the worktree, e.g. using `git update-index
|
||||
--cacheinfo`).
|
||||
|
||||
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
---
|
||||
t/t1450-fsck.sh | 1 +
|
||||
t/t7415-submodule-names.sh | 8 +++++---
|
||||
t/t9350-fast-export.sh | 1 +
|
||||
tree-walk.c | 6 ++++++
|
||||
4 files changed, 13 insertions(+), 3 deletions(-)
|
||||
|
||||
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
|
||||
index cb4b66e..33c955f 100755
|
||||
--- a/t/t1450-fsck.sh
|
||||
+++ b/t/t1450-fsck.sh
|
||||
@@ -419,6 +419,7 @@ while read name path pretty; do
|
||||
(
|
||||
git init $name-$type &&
|
||||
cd $name-$type &&
|
||||
+ git config core.protectNTFS false &&
|
||||
echo content >file &&
|
||||
git add file &&
|
||||
git commit -m base &&
|
||||
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
|
||||
index e1cd0a3..7c65e7a 100755
|
||||
--- a/t/t7415-submodule-names.sh
|
||||
+++ b/t/t7415-submodule-names.sh
|
||||
@@ -89,16 +89,18 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
|
||||
git hash-object -w --stdin)" &&
|
||||
rev="$(git rev-parse --verify HEAD)" &&
|
||||
hash="$(echo x | git hash-object -w --stdin)" &&
|
||||
- git update-index --add \
|
||||
+ git -c core.protectNTFS=false update-index --add \
|
||||
--cacheinfo 100644,$modules,.gitmodules \
|
||||
--cacheinfo 160000,$rev,c \
|
||||
--cacheinfo 160000,$rev,d\\a \
|
||||
--cacheinfo 100644,$hash,d./a/x \
|
||||
--cacheinfo 100644,$hash,d./a/..git &&
|
||||
test_tick &&
|
||||
- git commit -m "module"
|
||||
+ git -c core.protectNTFS=false commit -m "module" &&
|
||||
+ test_must_fail git show HEAD: 2>err &&
|
||||
+ test_i18ngrep backslash err
|
||||
) &&
|
||||
- test_must_fail git \
|
||||
+ test_must_fail git -c core.protectNTFS=false \
|
||||
clone --recurse-submodules squatting squatting-clone 2>err &&
|
||||
test_i18ngrep "directory not empty" err &&
|
||||
! grep gitdir squatting-clone/d/a/git~2
|
||||
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
|
||||
index 866ddf6..e606207 100755
|
||||
--- a/t/t9350-fast-export.sh
|
||||
+++ b/t/t9350-fast-export.sh
|
||||
@@ -421,6 +421,7 @@ test_expect_success 'directory becomes symlink' '
|
||||
|
||||
test_expect_success 'fast-export quotes pathnames' '
|
||||
git init crazy-paths &&
|
||||
+ test_config -C crazy-paths core.protectNTFS false &&
|
||||
(cd crazy-paths &&
|
||||
blob=$(echo foo | git hash-object -w --stdin) &&
|
||||
git update-index --add \
|
||||
diff --git a/tree-walk.c b/tree-walk.c
|
||||
index d459fed..54ff959 100644
|
||||
--- a/tree-walk.c
|
||||
+++ b/tree-walk.c
|
||||
@@ -41,6 +41,12 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l
|
||||
strbuf_addstr(err, _("empty filename in tree entry"));
|
||||
return -1;
|
||||
}
|
||||
+#ifdef GIT_WINDOWS_NATIVE
|
||||
+ if (protect_ntfs && strchr(path, '\\')) {
|
||||
+ strbuf_addf(err, _("filename in tree entry contains backslash: '%s'"), path);
|
||||
+ return -1;
|
||||
+ }
|
||||
+#endif
|
||||
len = strlen(path) + 1;
|
||||
|
||||
/* Initialize the descriptor entry */
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,181 +0,0 @@
|
||||
From a8dee3ca610f5a1d403634492136c887f83b59d2 Mon Sep 17 00:00:00 2001
|
||||
From: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
Date: Tue, 1 Oct 2019 23:27:18 +0200
|
||||
Subject: [PATCH] Disallow dubiously-nested submodule git directories
|
||||
|
||||
Currently it is technically possible to let a submodule's git
|
||||
directory point right into the git dir of a sibling submodule.
|
||||
|
||||
Example: the git directories of two submodules with the names `hippo`
|
||||
and `hippo/hooks` would be `.git/modules/hippo/` and
|
||||
`.git/modules/hippo/hooks/`, respectively, but the latter is already
|
||||
intended to house the former's hooks.
|
||||
|
||||
In most cases, this is just confusing, but there is also a (quite
|
||||
contrived) attack vector where Git can be fooled into mistaking remote
|
||||
content for file contents it wrote itself during a recursive clone.
|
||||
|
||||
Let's plug this bug.
|
||||
|
||||
To do so, we introduce the new function `validate_submodule_git_dir()`
|
||||
which simply verifies that no git dir exists for any leading directories
|
||||
of the submodule name (if there are any).
|
||||
|
||||
Note: this patch specifically continues to allow sibling modules names
|
||||
of the form `core/lib`, `core/doc`, etc, as long as `core` is not a
|
||||
submodule name.
|
||||
|
||||
This fixes CVE-2019-1387.
|
||||
|
||||
Reported-by: Nicolas Joly <Nicolas.Joly@microsoft.com>
|
||||
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
---
|
||||
builtin/submodule--helper.c | 4 ++++
|
||||
submodule.c | 49 +++++++++++++++++++++++++++++++++++++++++++--
|
||||
submodule.h | 5 +++++
|
||||
t/t7415-submodule-names.sh | 23 +++++++++++++++++++++
|
||||
4 files changed, 79 insertions(+), 2 deletions(-)
|
||||
|
||||
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
|
||||
index f3ba6aa..a083b99 100644
|
||||
--- a/builtin/submodule--helper.c
|
||||
+++ b/builtin/submodule--helper.c
|
||||
@@ -1416,6 +1416,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)
|
||||
} else
|
||||
path = xstrdup(path);
|
||||
|
||||
+ if (validate_submodule_git_dir(sm_gitdir, name) < 0)
|
||||
+ die(_("refusing to create/use '%s' in another submodule's "
|
||||
+ "git dir"), sm_gitdir);
|
||||
+
|
||||
if (!file_exists(sm_gitdir)) {
|
||||
if (safe_create_leading_directories_const(sm_gitdir) < 0)
|
||||
die(_("could not create directory '%s'"), sm_gitdir);
|
||||
diff --git a/submodule.c b/submodule.c
|
||||
index 0f199c5..9da7181 100644
|
||||
--- a/submodule.c
|
||||
+++ b/submodule.c
|
||||
@@ -1993,6 +1993,47 @@ int submodule_move_head(const char *path,
|
||||
return ret;
|
||||
}
|
||||
|
||||
+int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
|
||||
+{
|
||||
+ size_t len = strlen(git_dir), suffix_len = strlen(submodule_name);
|
||||
+ char *p;
|
||||
+ int ret = 0;
|
||||
+
|
||||
+ if (len <= suffix_len || (p = git_dir + len - suffix_len)[-1] != '/' ||
|
||||
+ strcmp(p, submodule_name))
|
||||
+ BUG("submodule name '%s' not a suffix of git dir '%s'",
|
||||
+ submodule_name, git_dir);
|
||||
+
|
||||
+ /*
|
||||
+ * We prevent the contents of sibling submodules' git directories to
|
||||
+ * clash.
|
||||
+ *
|
||||
+ * Example: having a submodule named `hippo` and another one named
|
||||
+ * `hippo/hooks` would result in the git directories
|
||||
+ * `.git/modules/hippo/` and `.git/modules/hippo/hooks/`, respectively,
|
||||
+ * but the latter directory is already designated to contain the hooks
|
||||
+ * of the former.
|
||||
+ */
|
||||
+ for (; *p; p++) {
|
||||
+ if (is_dir_sep(*p)) {
|
||||
+ char c = *p;
|
||||
+
|
||||
+ *p = '\0';
|
||||
+ if (is_git_directory(git_dir))
|
||||
+ ret = -1;
|
||||
+ *p = c;
|
||||
+
|
||||
+ if (ret < 0)
|
||||
+ return error(_("submodule git dir '%s' is "
|
||||
+ "inside git dir '%.*s'"),
|
||||
+ git_dir,
|
||||
+ (int)(p - git_dir), git_dir);
|
||||
+ }
|
||||
+ }
|
||||
+
|
||||
+ return 0;
|
||||
+}
|
||||
+
|
||||
/*
|
||||
* Embeds a single submodules git directory into the superprojects git dir,
|
||||
* non recursively.
|
||||
@@ -2000,7 +2041,7 @@ int submodule_move_head(const char *path,
|
||||
static void relocate_single_git_dir_into_superproject(const char *path)
|
||||
{
|
||||
char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
|
||||
- const char *new_git_dir;
|
||||
+ char *new_git_dir;
|
||||
const struct submodule *sub;
|
||||
|
||||
if (submodule_uses_worktrees(path))
|
||||
@@ -2018,10 +2059,14 @@ static void relocate_single_git_dir_into_superproject(const char *path)
|
||||
if (!sub)
|
||||
die(_("could not lookup name for submodule '%s'"), path);
|
||||
|
||||
- new_git_dir = git_path("modules/%s", sub->name);
|
||||
+ new_git_dir = git_pathdup("modules/%s", sub->name);
|
||||
+ if (validate_submodule_git_dir(new_git_dir, sub->name) < 0)
|
||||
+ die(_("refusing to move '%s' into an existing git dir"),
|
||||
+ real_old_git_dir);
|
||||
if (safe_create_leading_directories_const(new_git_dir) < 0)
|
||||
die(_("could not create directory '%s'"), new_git_dir);
|
||||
real_new_git_dir = real_pathdup(new_git_dir, 1);
|
||||
+ free(new_git_dir);
|
||||
|
||||
fprintf(stderr, _("Migrating git directory of '%s%s' from\n'%s' to\n'%s'\n"),
|
||||
get_super_prefix_or_empty(), path,
|
||||
diff --git a/submodule.h b/submodule.h
|
||||
index 8072e6d..c81ec1a 100644
|
||||
--- a/submodule.h
|
||||
+++ b/submodule.h
|
||||
@@ -124,6 +124,11 @@ int push_unpushed_submodules(struct repository *r,
|
||||
*/
|
||||
int submodule_to_gitdir(struct strbuf *buf, const char *submodule);
|
||||
|
||||
+/*
|
||||
+ * Make sure that no submodule's git dir is nested in a sibling submodule's.
|
||||
+ */
|
||||
+int validate_submodule_git_dir(char *git_dir, const char *submodule_name);
|
||||
+
|
||||
#define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0)
|
||||
#define SUBMODULE_MOVE_HEAD_FORCE (1<<1)
|
||||
int submodule_move_head(const char *path,
|
||||
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
|
||||
index 3008ba8..a44578f 100755
|
||||
--- a/t/t7415-submodule-names.sh
|
||||
+++ b/t/t7415-submodule-names.sh
|
||||
@@ -224,4 +224,27 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
|
||||
! grep gitdir squatting-clone/d/a/git~2
|
||||
'
|
||||
|
||||
+test_expect_success 'git dirs of sibling submodules must not be nested' '
|
||||
+ git init nested &&
|
||||
+ test_commit -C nested nested &&
|
||||
+ (
|
||||
+ cd nested &&
|
||||
+ cat >.gitmodules <<-EOF &&
|
||||
+ [submodule "hippo"]
|
||||
+ url = .
|
||||
+ path = thing1
|
||||
+ [submodule "hippo/hooks"]
|
||||
+ url = .
|
||||
+ path = thing2
|
||||
+ EOF
|
||||
+ git clone . thing1 &&
|
||||
+ git clone . thing2 &&
|
||||
+ git add .gitmodules thing1 thing2 &&
|
||||
+ test_tick &&
|
||||
+ git commit -m nested
|
||||
+ ) &&
|
||||
+ test_must_fail git clone --recurse-submodules nested clone 2>err &&
|
||||
+ test_i18ngrep "is inside git dir" err
|
||||
+'
|
||||
+
|
||||
test_done
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,148 +0,0 @@
|
||||
From e904deb89d9a9669a76a426182506a084d3f6308 Mon Sep 17 00:00:00 2001
|
||||
From: Jonathan Nieder <jrnieder@gmail.com>
|
||||
Date: Thu, 5 Dec 2019 01:28:28 -0800
|
||||
Subject: [PATCH] submodule: reject submodule.update = !command in .gitmodules
|
||||
|
||||
Since ac1fbbda2013 (submodule: do not copy unknown update mode from
|
||||
.gitmodules, 2013-12-02), Git has been careful to avoid copying
|
||||
|
||||
[submodule "foo"]
|
||||
update = !run an arbitrary scary command
|
||||
|
||||
from .gitmodules to a repository's local config, copying in the
|
||||
setting 'update = none' instead. The gitmodules(5) manpage documents
|
||||
the intention:
|
||||
|
||||
The !command form is intentionally ignored here for security
|
||||
reasons
|
||||
|
||||
Unfortunately, starting with v2.20.0-rc0 (which integrated ee69b2a9
|
||||
(submodule--helper: introduce new update-module-mode helper,
|
||||
2018-08-13, first released in v2.20.0-rc0)), there are scenarios where
|
||||
we *don't* ignore it: if the config store contains no
|
||||
submodule.foo.update setting, the submodule-config API falls back to
|
||||
reading .gitmodules and the repository-supplied !command gets run
|
||||
after all.
|
||||
|
||||
This was part of a general change over time in submodule support to
|
||||
read more directly from .gitmodules, since unlike .git/config it
|
||||
allows a project to change values between branches and over time
|
||||
(while still allowing .git/config to override things). But it was
|
||||
never intended to apply to this kind of dangerous configuration.
|
||||
|
||||
The behavior change was not advertised in ee69b2a9's commit message
|
||||
and was missed in review.
|
||||
|
||||
Let's take the opportunity to make the protection more robust, even in
|
||||
Git versions that are technically not affected: instead of quietly
|
||||
converting 'update = !command' to 'update = none', noisily treat it as
|
||||
an error. Allowing the setting but treating it as meaning something
|
||||
else was just confusing; users are better served by seeing the error
|
||||
sooner. Forbidding the construct makes the semantics simpler and
|
||||
means we can check for it in fsck (in a separate patch).
|
||||
|
||||
As a result, the submodule-config API cannot read this value from
|
||||
.gitmodules under any circumstance, and we can declare with confidence
|
||||
|
||||
For security reasons, the '!command' form is not accepted
|
||||
here.
|
||||
|
||||
Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com>
|
||||
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
||||
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
|
||||
---
|
||||
Documentation/gitmodules.txt | 5 ++---
|
||||
submodule-config.c | 10 +++++++++-
|
||||
t/t7406-submodule-update.sh | 14 ++++++++------
|
||||
3 files changed, 19 insertions(+), 10 deletions(-)
|
||||
|
||||
diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
|
||||
index a66e95b..f260b55 100644
|
||||
--- a/Documentation/gitmodules.txt
|
||||
+++ b/Documentation/gitmodules.txt
|
||||
@@ -44,9 +44,8 @@ submodule.<name>.update::
|
||||
submodule init` to initialize the configuration variable of
|
||||
the same name. Allowed values here are 'checkout', 'rebase',
|
||||
'merge' or 'none'. See description of 'update' command in
|
||||
- linkgit:git-submodule[1] for their meaning. Note that the
|
||||
- '!command' form is intentionally ignored here for security
|
||||
- reasons.
|
||||
+ linkgit:git-submodule[1] for their meaning. For security
|
||||
+ reasons, the '!command' form is not accepted here.
|
||||
|
||||
submodule.<name>.branch::
|
||||
A remote branch name for tracking updates in the upstream submodule.
|
||||
diff --git a/submodule-config.c b/submodule-config.c
|
||||
index 4264ee2..3acc6ad 100644
|
||||
--- a/submodule-config.c
|
||||
+++ b/submodule-config.c
|
||||
@@ -405,6 +405,13 @@ struct parse_config_parameter {
|
||||
int overwrite;
|
||||
};
|
||||
|
||||
+/*
|
||||
+ * Parse a config item from .gitmodules.
|
||||
+ *
|
||||
+ * This does not handle submodule-related configuration from the main
|
||||
+ * config store (.git/config, etc). Callers are responsible for
|
||||
+ * checking for overrides in the main config store when appropriate.
|
||||
+ */
|
||||
static int parse_config(const char *var, const char *value, void *data)
|
||||
{
|
||||
struct parse_config_parameter *me = data;
|
||||
@@ -482,7 +489,8 @@ static int parse_config(const char *var, const char *value, void *data)
|
||||
warn_multiple_config(me->treeish_name, submodule->name,
|
||||
"update");
|
||||
else if (parse_submodule_update_strategy(value,
|
||||
- &submodule->update_strategy) < 0)
|
||||
+ &submodule->update_strategy) < 0 ||
|
||||
+ submodule->update_strategy.type == SM_UPDATE_COMMAND)
|
||||
die(_("invalid value for %s"), var);
|
||||
} else if (!strcmp(item.buf, "shallow")) {
|
||||
if (!me->overwrite && submodule->recommend_shallow != -1)
|
||||
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
|
||||
index c973278..031ae85 100755
|
||||
--- a/t/t7406-submodule-update.sh
|
||||
+++ b/t/t7406-submodule-update.sh
|
||||
@@ -407,12 +407,12 @@ test_expect_success 'submodule update - command in .git/config' '
|
||||
)
|
||||
'
|
||||
|
||||
-test_expect_success 'submodule update - command in .gitmodules is ignored' '
|
||||
+test_expect_success 'submodule update - command in .gitmodules is rejected' '
|
||||
test_when_finished "git -C super reset --hard HEAD^" &&
|
||||
git -C super config -f .gitmodules submodule.submodule.update "!false" &&
|
||||
git -C super commit -a -m "add command to .gitmodules file" &&
|
||||
git -C super/submodule reset --hard $submodulesha1^ &&
|
||||
- git -C super submodule update submodule
|
||||
+ test_must_fail git -C super submodule update submodule
|
||||
'
|
||||
|
||||
cat << EOF >expect
|
||||
@@ -481,6 +481,9 @@ test_expect_success 'recursive submodule update - command in .git/config catches
|
||||
'
|
||||
|
||||
test_expect_success 'submodule init does not copy command into .git/config' '
|
||||
+ test_when_finished "git -C super update-index --force-remove submodule1" &&
|
||||
+ test_when_finished git config -f super/.gitmodules \
|
||||
+ --remove-section submodule.submodule1 &&
|
||||
(cd super &&
|
||||
git ls-files -s submodule >out &&
|
||||
H=$(cut -d" " -f2 out) &&
|
||||
@@ -489,10 +492,9 @@ test_expect_success 'submodule init does not copy command into .git/config' '
|
||||
git config -f .gitmodules submodule.submodule1.path submodule1 &&
|
||||
git config -f .gitmodules submodule.submodule1.url ../submodule &&
|
||||
git config -f .gitmodules submodule.submodule1.update !false &&
|
||||
- git submodule init submodule1 &&
|
||||
- echo "none" >expect &&
|
||||
- git config submodule.submodule1.update >actual &&
|
||||
- test_cmp expect actual
|
||||
+ test_must_fail git submodule init submodule1 &&
|
||||
+ test_expect_code 1 git config submodule.submodule1.update >actual &&
|
||||
+ test_must_be_empty actual
|
||||
)
|
||||
'
|
||||
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,63 +0,0 @@
|
||||
From a88dbd2f8c7fd8c1e2f63483da03bd6928e8791f Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Sun, 19 Apr 2020 01:36:02 -0400
|
||||
Subject: [PATCH] t0300: make "quit" helper more realistic
|
||||
|
||||
We test a toy credential helper that writes "quit=1" and confirms that
|
||||
we stop running other helpers. However, that helper is unrealistic in
|
||||
that it does not bother to read its stdin at all.
|
||||
|
||||
For now we don't send any input to it, because we feed git-credential a
|
||||
blank credential. But that will change in the next patch, which will
|
||||
cause this test to racily fail, as git-credential will get SIGPIPE
|
||||
writing to the helper rather than exiting because it was asked to.
|
||||
|
||||
Let's make this one-off helper more like our other sample helpers, and
|
||||
have it source the "dump" script. That will read stdin, fixing the
|
||||
SIGPIPE problem. But it will also write what it sees to stderr. We can
|
||||
make the test more robust by checking that output, which confirms that
|
||||
we do run the quit helper, don't run any other helpers, and exit for the
|
||||
reason we expected.
|
||||
|
||||
Signed-off-by: Jeff King <peff@peff.net>
|
||||
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
||||
---
|
||||
t/t0300-credentials.sh | 16 +++++++++++++---
|
||||
1 file changed, 13 insertions(+), 3 deletions(-)
|
||||
|
||||
--- a/t/t0300-credentials.sh
|
||||
+++ b/t/t0300-credentials.sh
|
||||
@@ -22,6 +22,11 @@ test_expect_success 'setup helper script
|
||||
exit 0
|
||||
EOF
|
||||
|
||||
+ write_script git-credential-quit <<-\EOF &&
|
||||
+ . ./dump
|
||||
+ echo quit=1
|
||||
+ EOF
|
||||
+
|
||||
write_script git-credential-verbatim <<-\EOF &&
|
||||
user=$1; shift
|
||||
pass=$1; shift
|
||||
@@ -291,10 +296,15 @@ test_expect_success 'http paths can be p
|
||||
|
||||
test_expect_success 'helpers can abort the process' '
|
||||
test_must_fail git \
|
||||
- -c credential.helper="!f() { echo quit=1; }; f" \
|
||||
+ -c credential.helper=quit \
|
||||
-c credential.helper="verbatim foo bar" \
|
||||
- credential fill >stdout &&
|
||||
- test_must_be_empty stdout
|
||||
+ credential fill >stdout 2>stderr &&
|
||||
+ test_must_be_empty stdout &&
|
||||
+ cat >expect <<-\EOF &&
|
||||
+ quit: get
|
||||
+ fatal: credential helper '\''quit'\'' told us to quit
|
||||
+ EOF
|
||||
+ test_i18ncmp expect stderr
|
||||
'
|
||||
|
||||
test_expect_success 'empty helper spec resets helper list' '
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,285 +0,0 @@
|
||||
From 73aafe9bc27585554181c58871a25e6d0f58a3dc Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Sat, 18 Apr 2020 20:47:30 -0700
|
||||
Subject: [PATCH] t0300: use more realistic inputs
|
||||
|
||||
Many of the tests in t0300 give partial inputs to git-credential,
|
||||
omitting a protocol or hostname. We're checking only high-level things
|
||||
like whether and how helpers are invoked at all, and we don't care about
|
||||
specific hosts. However, in preparation for tightening up the rules
|
||||
about when we're willing to run a helper, let's start using input that's
|
||||
a bit more realistic: pretend as if http://example.com is being
|
||||
examined.
|
||||
|
||||
This shouldn't change the point of any of the tests, but do note we have
|
||||
to adjust the expected output to accommodate this (filling a credential
|
||||
will repeat back the protocol/host fields to stdout, and the helper
|
||||
debug messages and askpass prompt will change on stderr).
|
||||
|
||||
Signed-off-by: Jeff King <peff@peff.net>
|
||||
Reviewed-by: Taylor Blau <me@ttaylorr.com>
|
||||
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
||||
---
|
||||
t/t0300-credentials.sh | 89 ++++++++++++++++++++++++++++++++++++++++--
|
||||
1 file changed, 85 insertions(+), 4 deletions(-)
|
||||
|
||||
--- a/t/t0300-credentials.sh
|
||||
+++ b/t/t0300-credentials.sh
|
||||
@@ -40,43 +40,71 @@ test_expect_success 'setup helper script
|
||||
|
||||
test_expect_success 'credential_fill invokes helper' '
|
||||
check fill "verbatim foo bar" <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=foo
|
||||
password=bar
|
||||
--
|
||||
verbatim: get
|
||||
+ verbatim: protocol=http
|
||||
+ verbatim: host=example.com
|
||||
EOF
|
||||
'
|
||||
|
||||
test_expect_success 'credential_fill invokes multiple helpers' '
|
||||
check fill useless "verbatim foo bar" <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=foo
|
||||
password=bar
|
||||
--
|
||||
useless: get
|
||||
+ useless: protocol=http
|
||||
+ useless: host=example.com
|
||||
verbatim: get
|
||||
+ verbatim: protocol=http
|
||||
+ verbatim: host=example.com
|
||||
EOF
|
||||
'
|
||||
|
||||
test_expect_success 'credential_fill stops when we get a full response' '
|
||||
check fill "verbatim one two" "verbatim three four" <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=one
|
||||
password=two
|
||||
--
|
||||
verbatim: get
|
||||
+ verbatim: protocol=http
|
||||
+ verbatim: host=example.com
|
||||
EOF
|
||||
'
|
||||
|
||||
test_expect_success 'credential_fill continues through partial response' '
|
||||
check fill "verbatim one \"\"" "verbatim two three" <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=two
|
||||
password=three
|
||||
--
|
||||
verbatim: get
|
||||
+ verbatim: protocol=http
|
||||
+ verbatim: host=example.com
|
||||
verbatim: get
|
||||
+ verbatim: protocol=http
|
||||
+ verbatim: host=example.com
|
||||
verbatim: username=one
|
||||
EOF
|
||||
'
|
||||
@@ -102,14 +130,20 @@ test_expect_success 'credential_fill pas
|
||||
|
||||
test_expect_success 'credential_approve calls all helpers' '
|
||||
check approve useless "verbatim one two" <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=foo
|
||||
password=bar
|
||||
--
|
||||
--
|
||||
useless: store
|
||||
+ useless: protocol=http
|
||||
+ useless: host=example.com
|
||||
useless: username=foo
|
||||
useless: password=bar
|
||||
verbatim: store
|
||||
+ verbatim: protocol=http
|
||||
+ verbatim: host=example.com
|
||||
verbatim: username=foo
|
||||
verbatim: password=bar
|
||||
EOF
|
||||
@@ -117,6 +151,8 @@ test_expect_success 'credential_approve
|
||||
|
||||
test_expect_success 'do not bother storing password-less credential' '
|
||||
check approve useless <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=foo
|
||||
--
|
||||
--
|
||||
@@ -126,14 +162,20 @@ test_expect_success 'do not bother stori
|
||||
|
||||
test_expect_success 'credential_reject calls all helpers' '
|
||||
check reject useless "verbatim one two" <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=foo
|
||||
password=bar
|
||||
--
|
||||
--
|
||||
useless: erase
|
||||
+ useless: protocol=http
|
||||
+ useless: host=example.com
|
||||
useless: username=foo
|
||||
useless: password=bar
|
||||
verbatim: erase
|
||||
+ verbatim: protocol=http
|
||||
+ verbatim: host=example.com
|
||||
verbatim: username=foo
|
||||
verbatim: password=bar
|
||||
EOF
|
||||
@@ -141,33 +183,49 @@ test_expect_success 'credential_reject c
|
||||
|
||||
test_expect_success 'usernames can be preserved' '
|
||||
check fill "verbatim \"\" three" <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=one
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=one
|
||||
password=three
|
||||
--
|
||||
verbatim: get
|
||||
+ verbatim: protocol=http
|
||||
+ verbatim: host=example.com
|
||||
verbatim: username=one
|
||||
EOF
|
||||
'
|
||||
|
||||
test_expect_success 'usernames can be overridden' '
|
||||
check fill "verbatim two three" <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=one
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=two
|
||||
password=three
|
||||
--
|
||||
verbatim: get
|
||||
+ verbatim: protocol=http
|
||||
+ verbatim: host=example.com
|
||||
verbatim: username=one
|
||||
EOF
|
||||
'
|
||||
|
||||
test_expect_success 'do not bother completing already-full credential' '
|
||||
check fill "verbatim three four" <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=one
|
||||
password=two
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=one
|
||||
password=two
|
||||
--
|
||||
@@ -179,23 +237,31 @@ test_expect_success 'do not bother compl
|
||||
# askpass helper is run, we know the internal getpass is working.
|
||||
test_expect_success 'empty helper list falls back to internal getpass' '
|
||||
check fill <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=askpass-username
|
||||
password=askpass-password
|
||||
--
|
||||
- askpass: Username:
|
||||
- askpass: Password:
|
||||
+ askpass: Username for '\''http://example.com'\'':
|
||||
+ askpass: Password for '\''http://askpass-username@example.com'\'':
|
||||
EOF
|
||||
'
|
||||
|
||||
test_expect_success 'internal getpass does not ask for known username' '
|
||||
check fill <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=foo
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=foo
|
||||
password=askpass-password
|
||||
--
|
||||
- askpass: Password:
|
||||
+ askpass: Password for '\''http://foo@example.com'\'':
|
||||
EOF
|
||||
'
|
||||
|
||||
@@ -207,7 +273,11 @@ HELPER="!f() {
|
||||
test_expect_success 'respect configured credentials' '
|
||||
test_config credential.helper "$HELPER" &&
|
||||
check fill <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=foo
|
||||
password=bar
|
||||
--
|
||||
@@ -298,10 +368,15 @@ test_expect_success 'helpers can abort t
|
||||
test_must_fail git \
|
||||
-c credential.helper=quit \
|
||||
-c credential.helper="verbatim foo bar" \
|
||||
- credential fill >stdout 2>stderr &&
|
||||
+ credential fill >stdout 2>stderr <<-\EOF &&
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
+ EOF
|
||||
test_must_be_empty stdout &&
|
||||
cat >expect <<-\EOF &&
|
||||
quit: get
|
||||
+ quit: protocol=http
|
||||
+ quit: host=example.com
|
||||
fatal: credential helper '\''quit'\'' told us to quit
|
||||
EOF
|
||||
test_i18ncmp expect stderr
|
||||
@@ -310,11 +385,17 @@ test_expect_success 'helpers can abort t
|
||||
test_expect_success 'empty helper spec resets helper list' '
|
||||
test_config credential.helper "verbatim file file" &&
|
||||
check fill "" "verbatim cmdline cmdline" <<-\EOF
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
--
|
||||
+ protocol=http
|
||||
+ host=example.com
|
||||
username=cmdline
|
||||
password=cmdline
|
||||
--
|
||||
verbatim: get
|
||||
+ verbatim: protocol=http
|
||||
+ verbatim: host=example.com
|
||||
EOF
|
||||
'
|
||||
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,86 +0,0 @@
|
||||
From 24036686c4af84c9e84e486ef3debab6e6d8e6b5 Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Sat, 18 Apr 2020 20:48:05 -0700
|
||||
Subject: [PATCH] credential: parse URL without host as empty host, not unset
|
||||
|
||||
We may feed a URL like "cert:///path/to/cert.pem" into the credential
|
||||
machinery to get the key for a client-side certificate. That
|
||||
credential has no hostname field, which is about to be disallowed (to
|
||||
avoid confusion with protocols where a helper _would_ expect a
|
||||
hostname).
|
||||
|
||||
This means as of the next patch, credential helpers won't work for
|
||||
unlocking certs. Let's fix that by doing two things:
|
||||
|
||||
- when we parse a url with an empty host, set the host field to the
|
||||
empty string (asking only to match stored entries with an empty
|
||||
host) rather than NULL (asking to match _any_ host).
|
||||
|
||||
- when we build a cert:// credential by hand, similarly assign an
|
||||
empty string
|
||||
|
||||
It's the latter that is more likely to impact real users in practice,
|
||||
since it's what's used for http connections. But we don't have good
|
||||
infrastructure to test it.
|
||||
|
||||
The url-parsing version will help anybody using git-credential in a
|
||||
script, and is easy to test.
|
||||
|
||||
Signed-off-by: Jeff King <peff@peff.net>
|
||||
Reviewed-by: Taylor Blau <me@ttaylorr.com>
|
||||
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
||||
---
|
||||
credential.c | 3 +--
|
||||
http.c | 1 +
|
||||
t/t0300-credentials.sh | 17 +++++++++++++++++
|
||||
3 files changed, 19 insertions(+), 2 deletions(-)
|
||||
|
||||
--- a/credential.c
|
||||
+++ b/credential.c
|
||||
@@ -376,8 +376,7 @@ int credential_from_url_gently(struct cr
|
||||
|
||||
if (proto_end - url > 0)
|
||||
c->protocol = xmemdupz(url, proto_end - url);
|
||||
- if (slash - host > 0)
|
||||
- c->host = url_decode_mem(host, slash - host);
|
||||
+ c->host = url_decode_mem(host, slash - host);
|
||||
/* Trim leading and trailing slashes from path */
|
||||
while (*slash == '/')
|
||||
slash++;
|
||||
--- a/http.c
|
||||
+++ b/http.c
|
||||
@@ -554,6 +554,7 @@ static int has_cert_password(void)
|
||||
return 0;
|
||||
if (!cert_auth.password) {
|
||||
cert_auth.protocol = xstrdup("cert");
|
||||
+ cert_auth.host = xstrdup("");
|
||||
cert_auth.username = xstrdup("");
|
||||
cert_auth.path = xstrdup(ssl_cert);
|
||||
credential_fill(&cert_auth);
|
||||
--- a/t/t0300-credentials.sh
|
||||
+++ b/t/t0300-credentials.sh
|
||||
@@ -413,4 +413,21 @@ test_expect_success 'url parser ignores
|
||||
EOF
|
||||
'
|
||||
|
||||
+test_expect_success 'host-less URLs are parsed as empty host' '
|
||||
+ check fill "verbatim foo bar" <<-\EOF
|
||||
+ url=cert:///path/to/cert.pem
|
||||
+ --
|
||||
+ protocol=cert
|
||||
+ host=
|
||||
+ path=path/to/cert.pem
|
||||
+ username=foo
|
||||
+ password=bar
|
||||
+ --
|
||||
+ verbatim: get
|
||||
+ verbatim: protocol=cert
|
||||
+ verbatim: host=
|
||||
+ verbatim: path=path/to/cert.pem
|
||||
+ EOF
|
||||
+'
|
||||
+
|
||||
test_done
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,164 +0,0 @@
|
||||
From 8ba8ed568e2a3b75ee84c49ddffb026fde1a0a91 Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Sat, 18 Apr 2020 20:50:48 -0700
|
||||
Subject: [PATCH] credential: refuse to operate when missing host or protocol
|
||||
|
||||
The credential helper protocol was designed to be very flexible: the
|
||||
fields it takes as input are treated as a pattern, and any missing
|
||||
fields are taken as wildcards. This allows unusual things like:
|
||||
|
||||
echo protocol=https | git credential reject
|
||||
|
||||
to delete all stored https credentials (assuming the helpers themselves
|
||||
treat the input that way). But when helpers are invoked automatically by
|
||||
Git, this flexibility works against us. If for whatever reason we don't
|
||||
have a "host" field, then we'd match _any_ host. When you're filling a
|
||||
credential to send to a remote server, this is almost certainly not what
|
||||
you want.
|
||||
|
||||
Prevent this at the layer that writes to the credential helper. Add a
|
||||
check to the credential API that the host and protocol are always passed
|
||||
in, and add an assertion to the credential_write function that speaks
|
||||
credential helper protocol to be doubly sure.
|
||||
|
||||
There are a few ways this can be triggered in practice:
|
||||
|
||||
- the "git credential" command passes along arbitrary credential
|
||||
parameters it reads from stdin.
|
||||
|
||||
- until the previous patch, when the host field of a URL is empty, we
|
||||
would leave it unset (rather than setting it to the empty string)
|
||||
|
||||
- a URL like "example.com/foo.git" is treated by curl as if "http://"
|
||||
was present, but our parser sees it as a non-URL and leaves all
|
||||
fields unset
|
||||
|
||||
- the recent fix for URLs with embedded newlines blanks the URL but
|
||||
otherwise continues. Rather than having the desired effect of
|
||||
looking up no credential at all, many helpers will return _any_
|
||||
credential
|
||||
|
||||
Our earlier test for an embedded newline didn't catch this because it
|
||||
only checked that the credential was cleared, but didn't configure an
|
||||
actual helper. Configuring the "verbatim" helper in the test would show
|
||||
that it is invoked (it's obviously a silly helper which doesn't look at
|
||||
its input, but the point is that it shouldn't be run at all). Since
|
||||
we're switching this case to die(), we don't need to bother with a
|
||||
helper. We can see the new behavior just by checking that the operation
|
||||
fails.
|
||||
|
||||
We'll add new tests covering partial input as well (these can be
|
||||
triggered through various means with url-parsing, but it's simpler to
|
||||
just check them directly, as we know we are covered even if the url
|
||||
parser changes behavior in the future).
|
||||
|
||||
[jn: changed to die() instead of logging and showing a manual
|
||||
username/password prompt]
|
||||
|
||||
Reported-by: Carlo Arenas <carenas@gmail.com>
|
||||
Signed-off-by: Jeff King <peff@peff.net>
|
||||
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
||||
---
|
||||
credential.c | 20 ++++++++++++++------
|
||||
t/t0300-credentials.sh | 34 ++++++++++++++++++++++++++--------
|
||||
2 files changed, 40 insertions(+), 14 deletions(-)
|
||||
|
||||
--- a/credential.c
|
||||
+++ b/credential.c
|
||||
@@ -89,6 +89,11 @@ static int proto_is_http(const char *s)
|
||||
|
||||
static void credential_apply_config(struct credential *c)
|
||||
{
|
||||
+ if (!c->host)
|
||||
+ die(_("refusing to work with credential missing host field"));
|
||||
+ if (!c->protocol)
|
||||
+ die(_("refusing to work with credential missing protocol field"));
|
||||
+
|
||||
if (c->configured)
|
||||
return;
|
||||
git_config(credential_config_callback, c);
|
||||
@@ -191,8 +196,11 @@ int credential_read(struct credential *c
|
||||
return 0;
|
||||
}
|
||||
|
||||
-static void credential_write_item(FILE *fp, const char *key, const char *value)
|
||||
+static void credential_write_item(FILE *fp, const char *key, const char *value,
|
||||
+ int required)
|
||||
{
|
||||
+ if (!value && required)
|
||||
+ BUG("credential value for %s is missing", key);
|
||||
if (!value)
|
||||
return;
|
||||
if (strchr(value, '\n'))
|
||||
@@ -202,11 +210,11 @@ static void credential_write_item(FILE *
|
||||
|
||||
void credential_write(const struct credential *c, FILE *fp)
|
||||
{
|
||||
- credential_write_item(fp, "protocol", c->protocol);
|
||||
- credential_write_item(fp, "host", c->host);
|
||||
- credential_write_item(fp, "path", c->path);
|
||||
- credential_write_item(fp, "username", c->username);
|
||||
- credential_write_item(fp, "password", c->password);
|
||||
+ credential_write_item(fp, "protocol", c->protocol, 1);
|
||||
+ credential_write_item(fp, "host", c->host, 1);
|
||||
+ credential_write_item(fp, "path", c->path, 0);
|
||||
+ credential_write_item(fp, "username", c->username, 0);
|
||||
+ credential_write_item(fp, "password", c->password, 0);
|
||||
}
|
||||
|
||||
static int run_credential_helper(struct credential *c,
|
||||
--- a/t/t0300-credentials.sh
|
||||
+++ b/t/t0300-credentials.sh
|
||||
@@ -399,18 +399,16 @@ test_expect_success 'empty helper spec r
|
||||
EOF
|
||||
'
|
||||
|
||||
-test_expect_success 'url parser ignores embedded newlines' '
|
||||
- check fill <<-EOF
|
||||
+test_expect_success 'url parser rejects embedded newlines' '
|
||||
+ test_must_fail git credential fill 2>stderr <<-\EOF &&
|
||||
url=https://one.example.com?%0ahost=two.example.com/
|
||||
- --
|
||||
- username=askpass-username
|
||||
- password=askpass-password
|
||||
- --
|
||||
+ EOF
|
||||
+ cat >expect <<-\EOF &&
|
||||
warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/
|
||||
warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/
|
||||
- askpass: Username:
|
||||
- askpass: Password:
|
||||
+ fatal: refusing to work with credential missing host field
|
||||
EOF
|
||||
+ test_i18ncmp expect stderr
|
||||
'
|
||||
|
||||
test_expect_success 'host-less URLs are parsed as empty host' '
|
||||
@@ -430,4 +428,24 @@ test_expect_success 'host-less URLs are
|
||||
EOF
|
||||
'
|
||||
|
||||
+test_expect_success 'credential system refuses to work with missing host' '
|
||||
+ test_must_fail git credential fill 2>stderr <<-\EOF &&
|
||||
+ protocol=http
|
||||
+ EOF
|
||||
+ cat >expect <<-\EOF &&
|
||||
+ fatal: refusing to work with credential missing host field
|
||||
+ EOF
|
||||
+ test_i18ncmp expect stderr
|
||||
+'
|
||||
+
|
||||
+test_expect_success 'credential system refuses to work with missing protocol' '
|
||||
+ test_must_fail git credential fill 2>stderr <<-\EOF &&
|
||||
+ host=example.com
|
||||
+ EOF
|
||||
+ cat >expect <<-\EOF &&
|
||||
+ fatal: refusing to work with credential missing protocol field
|
||||
+ EOF
|
||||
+ test_i18ncmp expect stderr
|
||||
+'
|
||||
+
|
||||
test_done
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,205 +0,0 @@
|
||||
From a2b26ffb1a81aa23dd14453f4db05d8fe24ee7cc Mon Sep 17 00:00:00 2001
|
||||
From: Jonathan Nieder <jrnieder@gmail.com>
|
||||
Date: Sat, 18 Apr 2020 20:52:34 -0700
|
||||
Subject: [PATCH] fsck: convert gitmodules url to URL passed to curl
|
||||
|
||||
In 07259e74ec1 (fsck: detect gitmodules URLs with embedded newlines,
|
||||
2020-03-11), git fsck learned to check whether URLs in .gitmodules could
|
||||
be understood by the credential machinery when they are handled by
|
||||
git-remote-curl.
|
||||
|
||||
However, the check is overbroad: it checks all URLs instead of only
|
||||
URLs that would be passed to git-remote-curl. In principle a git:// or
|
||||
file:/// URL does not need to follow the same conventions as an http://
|
||||
URL; in particular, git:// and file:// protocols are not succeptible to
|
||||
issues in the credential API because they do not support attaching
|
||||
credentials.
|
||||
|
||||
In the HTTP case, the URL in .gitmodules does not always match the URL
|
||||
that would be passed to git-remote-curl and the credential machinery:
|
||||
Git's URL syntax allows specifying a remote helper followed by a "::"
|
||||
delimiter and a URL to be passed to it, so that
|
||||
|
||||
git ls-remote http::https://example.com/repo.git
|
||||
|
||||
invokes git-remote-http with https://example.com/repo.git as its URL
|
||||
argument. With today's checks, that distinction does not make a
|
||||
difference, but for a check we are about to introduce (for empty URL
|
||||
schemes) it will matter.
|
||||
|
||||
.gitmodules files also support relative URLs. To ensure coverage for the
|
||||
https based embedded-newline attack, urldecode and check them directly
|
||||
for embedded newlines.
|
||||
|
||||
Helped-by: Jeff King <peff@peff.net>
|
||||
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
||||
Reviewed-by: Jeff King <peff@peff.net>
|
||||
---
|
||||
fsck.c | 94 +++++++++++++++++++++++++++++++++--
|
||||
t/t7416-submodule-dash-url.sh | 29 +++++++++++
|
||||
2 files changed, 118 insertions(+), 5 deletions(-)
|
||||
|
||||
--- a/fsck.c
|
||||
+++ b/fsck.c
|
||||
@@ -9,6 +9,7 @@
|
||||
#include "tag.h"
|
||||
#include "fsck.h"
|
||||
#include "refs.h"
|
||||
+#include "url.h"
|
||||
#include "utf8.h"
|
||||
#include "decorate.h"
|
||||
#include "oidset.h"
|
||||
@@ -983,17 +984,100 @@ static int fsck_tag(struct tag *tag, con
|
||||
return fsck_tag_buffer(tag, data, size, options);
|
||||
}
|
||||
|
||||
+/*
|
||||
+ * Like builtin/submodule--helper.c's starts_with_dot_slash, but without
|
||||
+ * relying on the platform-dependent is_dir_sep helper.
|
||||
+ *
|
||||
+ * This is for use in checking whether a submodule URL is interpreted as
|
||||
+ * relative to the current directory on any platform, since \ is a
|
||||
+ * directory separator on Windows but not on other platforms.
|
||||
+ */
|
||||
+static int starts_with_dot_slash(const char *str)
|
||||
+{
|
||||
+ return str[0] == '.' && (str[1] == '/' || str[1] == '\\');
|
||||
+}
|
||||
+
|
||||
+/*
|
||||
+ * Like starts_with_dot_slash, this is a variant of submodule--helper's
|
||||
+ * helper of the same name with the twist that it accepts backslash as a
|
||||
+ * directory separator even on non-Windows platforms.
|
||||
+ */
|
||||
+static int starts_with_dot_dot_slash(const char *str)
|
||||
+{
|
||||
+ return str[0] == '.' && starts_with_dot_slash(str + 1);
|
||||
+}
|
||||
+
|
||||
+static int submodule_url_is_relative(const char *url)
|
||||
+{
|
||||
+ return starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);
|
||||
+}
|
||||
+
|
||||
+/*
|
||||
+ * Check whether a transport is implemented by git-remote-curl.
|
||||
+ *
|
||||
+ * If it is, returns 1 and writes the URL that would be passed to
|
||||
+ * git-remote-curl to the "out" parameter.
|
||||
+ *
|
||||
+ * Otherwise, returns 0 and leaves "out" untouched.
|
||||
+ *
|
||||
+ * Examples:
|
||||
+ * http::https://example.com/repo.git -> 1, https://example.com/repo.git
|
||||
+ * https://example.com/repo.git -> 1, https://example.com/repo.git
|
||||
+ * git://example.com/repo.git -> 0
|
||||
+ *
|
||||
+ * This is for use in checking for previously exploitable bugs that
|
||||
+ * required a submodule URL to be passed to git-remote-curl.
|
||||
+ */
|
||||
+static int url_to_curl_url(const char *url, const char **out)
|
||||
+{
|
||||
+ /*
|
||||
+ * We don't need to check for case-aliases, "http.exe", and so
|
||||
+ * on because in the default configuration, is_transport_allowed
|
||||
+ * prevents URLs with those schemes from being cloned
|
||||
+ * automatically.
|
||||
+ */
|
||||
+ if (skip_prefix(url, "http::", out) ||
|
||||
+ skip_prefix(url, "https::", out) ||
|
||||
+ skip_prefix(url, "ftp::", out) ||
|
||||
+ skip_prefix(url, "ftps::", out))
|
||||
+ return 1;
|
||||
+ if (starts_with(url, "http://") ||
|
||||
+ starts_with(url, "https://") ||
|
||||
+ starts_with(url, "ftp://") ||
|
||||
+ starts_with(url, "ftps://")) {
|
||||
+ *out = url;
|
||||
+ return 1;
|
||||
+ }
|
||||
+ return 0;
|
||||
+}
|
||||
+
|
||||
static int check_submodule_url(const char *url)
|
||||
{
|
||||
- struct credential c = CREDENTIAL_INIT;
|
||||
- int ret;
|
||||
+ const char *curl_url;
|
||||
|
||||
if (looks_like_command_line_option(url))
|
||||
return -1;
|
||||
|
||||
- ret = credential_from_url_gently(&c, url, 1);
|
||||
- credential_clear(&c);
|
||||
- return ret;
|
||||
+ if (submodule_url_is_relative(url)) {
|
||||
+ /*
|
||||
+ * This could be appended to an http URL and url-decoded;
|
||||
+ * check for malicious characters.
|
||||
+ */
|
||||
+ char *decoded = url_decode(url);
|
||||
+ int has_nl = !!strchr(decoded, '\n');
|
||||
+ free(decoded);
|
||||
+ if (has_nl)
|
||||
+ return -1;
|
||||
+ }
|
||||
+
|
||||
+ else if (url_to_curl_url(url, &curl_url)) {
|
||||
+ struct credential c = CREDENTIAL_INIT;
|
||||
+ int ret = credential_from_url_gently(&c, curl_url, 1);
|
||||
+ credential_clear(&c);
|
||||
+ return ret;
|
||||
+ }
|
||||
+
|
||||
+ return 0;
|
||||
}
|
||||
|
||||
struct fsck_gitmodules_data {
|
||||
--- a/t/t7416-submodule-dash-url.sh
|
||||
+++ b/t/t7416-submodule-dash-url.sh
|
||||
@@ -60,6 +60,20 @@ test_expect_success 'trailing backslash
|
||||
test_i18ngrep ! "unknown option" err
|
||||
'
|
||||
|
||||
+test_expect_success 'fsck permits embedded newline with unrecognized scheme' '
|
||||
+ git checkout --orphan newscheme &&
|
||||
+ cat >.gitmodules <<-\EOF &&
|
||||
+ [submodule "foo"]
|
||||
+ url = "data://acjbkd%0akajfdickajkd"
|
||||
+ EOF
|
||||
+ git add .gitmodules &&
|
||||
+ git commit -m "gitmodules with unrecognized scheme" &&
|
||||
+ test_when_finished "rm -rf dst" &&
|
||||
+ git init --bare dst &&
|
||||
+ git -C dst config transfer.fsckObjects true &&
|
||||
+ git push dst HEAD
|
||||
+'
|
||||
+
|
||||
test_expect_success 'fsck rejects embedded newline in url' '
|
||||
# create an orphan branch to avoid existing .gitmodules objects
|
||||
git checkout --orphan newline &&
|
||||
@@ -72,6 +86,21 @@ test_expect_success 'fsck rejects embedd
|
||||
test_when_finished "rm -rf dst" &&
|
||||
git init --bare dst &&
|
||||
git -C dst config transfer.fsckObjects true &&
|
||||
+ test_must_fail git push dst HEAD 2>err &&
|
||||
+ grep gitmodulesUrl err
|
||||
+'
|
||||
+
|
||||
+test_expect_success 'fsck rejects embedded newline in relative url' '
|
||||
+ git checkout --orphan relative-newline &&
|
||||
+ cat >.gitmodules <<-\EOF &&
|
||||
+ [submodule "foo"]
|
||||
+ url = "./%0ahost=two.example.com/foo.git"
|
||||
+ EOF
|
||||
+ git add .gitmodules &&
|
||||
+ git commit -m "relative url with newline" &&
|
||||
+ test_when_finished "rm -rf dst" &&
|
||||
+ git init --bare dst &&
|
||||
+ git -C dst config transfer.fsckObjects true &&
|
||||
test_must_fail git push dst HEAD 2>err &&
|
||||
grep gitmodulesUrl err
|
||||
'
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,75 +0,0 @@
|
||||
From fe29a9b7b0236d3d45c254965580d6aff7fa8504 Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Sat, 18 Apr 2020 20:53:09 -0700
|
||||
Subject: [PATCH] credential: die() when parsing invalid urls
|
||||
|
||||
When we try to initialize credential loading by URL and find that the
|
||||
URL is invalid, we set all fields to NULL in order to avoid acting on
|
||||
malicious input. Later when we request credentials, we diagonse the
|
||||
erroneous input:
|
||||
|
||||
fatal: refusing to work with credential missing host field
|
||||
|
||||
This is problematic in two ways:
|
||||
|
||||
- The message doesn't tell the user *why* we are missing the host
|
||||
field, so they can't tell from this message alone how to recover.
|
||||
There can be intervening messages after the original warning of
|
||||
bad input, so the user may not have the context to put two and two
|
||||
together.
|
||||
|
||||
- The error only occurs when we actually need to get a credential. If
|
||||
the URL permits anonymous access, the only encouragement the user gets
|
||||
to correct their bogus URL is a quiet warning.
|
||||
|
||||
This is inconsistent with the check we perform in fsck, where any use
|
||||
of such a URL as a submodule is an error.
|
||||
|
||||
When we see such a bogus URL, let's not try to be nice and continue
|
||||
without helpers. Instead, die() immediately. This is simpler and
|
||||
obviously safe. And there's very little chance of disrupting a normal
|
||||
workflow.
|
||||
|
||||
It's _possible_ that somebody has a legitimate URL with a raw newline in
|
||||
it. It already wouldn't work with credential helpers, so this patch
|
||||
steps that up from an inconvenience to "we will refuse to work with it
|
||||
at all". If such a case does exist, we should figure out a way to work
|
||||
with it (especially if the newline is only in the path component, which
|
||||
we normally don't even pass to helpers). But until we see a real report,
|
||||
we're better off being defensive.
|
||||
|
||||
Reported-by: Carlo Arenas <carenas@gmail.com>
|
||||
Signed-off-by: Jeff King <peff@peff.net>
|
||||
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
||||
---
|
||||
credential.c | 6 ++----
|
||||
t/t0300-credentials.sh | 3 +--
|
||||
2 files changed, 3 insertions(+), 6 deletions(-)
|
||||
|
||||
--- a/credential.c
|
||||
+++ b/credential.c
|
||||
@@ -408,8 +408,6 @@ int credential_from_url_gently(struct cr
|
||||
|
||||
void credential_from_url(struct credential *c, const char *url)
|
||||
{
|
||||
- if (credential_from_url_gently(c, url, 0) < 0) {
|
||||
- warning(_("skipping credential lookup for url: %s"), url);
|
||||
- credential_clear(c);
|
||||
- }
|
||||
+ if (credential_from_url_gently(c, url, 0) < 0)
|
||||
+ die(_("credential url cannot be parsed: %s"), url);
|
||||
}
|
||||
--- a/t/t0300-credentials.sh
|
||||
+++ b/t/t0300-credentials.sh
|
||||
@@ -405,8 +405,7 @@ test_expect_success 'url parser rejects
|
||||
EOF
|
||||
cat >expect <<-\EOF &&
|
||||
warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/
|
||||
- warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/
|
||||
- fatal: refusing to work with credential missing host field
|
||||
+ fatal: credential url cannot be parsed: https://one.example.com?%0ahost=two.example.com/
|
||||
EOF
|
||||
test_i18ncmp expect stderr
|
||||
'
|
||||
--
|
||||
1.8.3.1
|
||||
@ -1,194 +0,0 @@
|
||||
From c44088ecc4b0722636e0a305f9608d3047197282 Mon Sep 17 00:00:00 2001
|
||||
From: Jonathan Nieder <jrnieder@gmail.com>
|
||||
Date: Sat, 18 Apr 2020 20:54:13 -0700
|
||||
Subject: [PATCH] credential: treat URL without scheme as invalid
|
||||
|
||||
libcurl permits making requests without a URL scheme specified. In
|
||||
this case, it guesses the URL from the hostname, so I can run
|
||||
|
||||
git ls-remote http::ftp.example.com/path/to/repo
|
||||
|
||||
and it would make an FTP request.
|
||||
|
||||
Any user intentionally using such a URL is likely to have made a typo.
|
||||
Unfortunately, credential_from_url is not able to determine the host and
|
||||
protocol in order to determine appropriate credentials to send, and
|
||||
until "credential: refuse to operate when missing host or protocol",
|
||||
this resulted in another host's credentials being leaked to the named
|
||||
host.
|
||||
|
||||
Teach credential_from_url_gently to consider such a URL to be invalid
|
||||
so that fsck can detect and block gitmodules files with such URLs,
|
||||
allowing server operators to avoid serving them to downstream users
|
||||
running older versions of Git.
|
||||
|
||||
This also means that when such URLs are passed on the command line, Git
|
||||
will print a clearer error so affected users can switch to the simpler
|
||||
URL that explicitly specifies the host and protocol they intend.
|
||||
|
||||
One subtlety: .gitmodules files can contain relative URLs, representing
|
||||
a URL relative to the URL they were cloned from. The relative URL
|
||||
resolver used for .gitmodules can follow ".." components out of the path
|
||||
part and past the host part of a URL, meaning that such a relative URL
|
||||
can be used to traverse from a https://foo.example.com/innocent
|
||||
superproject to a https::attacker.example.com/exploit submodule.
|
||||
Fortunately a leading ':' in the first path component after a series of
|
||||
leading './' and '../' components is unlikely to show up in other
|
||||
contexts, so we can catch this by detecting that pattern.
|
||||
|
||||
Reported-by: Jeff King <peff@peff.net>
|
||||
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
||||
Reviewed-by: Jeff King <peff@peff.net>
|
||||
---
|
||||
credential.c | 7 ++++--
|
||||
fsck.c | 47 +++++++++++++++++++++++++++++++++--
|
||||
t/t5550-http-fetch-dumb.sh | 7 ++----
|
||||
t/t7416-submodule-dash-url.sh | 32 ++++++++++++++++++++++++
|
||||
4 files changed, 84 insertions(+), 9 deletions(-)
|
||||
|
||||
--- a/credential.c
|
||||
+++ b/credential.c
|
||||
@@ -360,8 +360,11 @@ int credential_from_url_gently(struct cr
|
||||
* (3) proto://<user>:<pass>@<host>/...
|
||||
*/
|
||||
proto_end = strstr(url, "://");
|
||||
- if (!proto_end)
|
||||
- return 0;
|
||||
+ if (!proto_end) {
|
||||
+ if (!quiet)
|
||||
+ warning(_("url has no scheme: %s"), url);
|
||||
+ return -1;
|
||||
+ }
|
||||
cp = proto_end + 3;
|
||||
at = strchr(cp, '@');
|
||||
colon = strchr(cp, ':');
|
||||
--- a/fsck.c
|
||||
+++ b/fsck.c
|
||||
@@ -1013,6 +1013,34 @@ static int submodule_url_is_relative(con
|
||||
}
|
||||
|
||||
/*
|
||||
+ * Count directory components that a relative submodule URL should chop
|
||||
+ * from the remote_url it is to be resolved against.
|
||||
+ *
|
||||
+ * In other words, this counts "../" components at the start of a
|
||||
+ * submodule URL.
|
||||
+ *
|
||||
+ * Returns the number of directory components to chop and writes a
|
||||
+ * pointer to the next character of url after all leading "./" and
|
||||
+ * "../" components to out.
|
||||
+ */
|
||||
+static int count_leading_dotdots(const char *url, const char **out)
|
||||
+{
|
||||
+ int result = 0;
|
||||
+ while (1) {
|
||||
+ if (starts_with_dot_dot_slash(url)) {
|
||||
+ result++;
|
||||
+ url += strlen("../");
|
||||
+ continue;
|
||||
+ }
|
||||
+ if (starts_with_dot_slash(url)) {
|
||||
+ url += strlen("./");
|
||||
+ continue;
|
||||
+ }
|
||||
+ *out = url;
|
||||
+ return result;
|
||||
+ }
|
||||
+}
|
||||
+/*
|
||||
* Check whether a transport is implemented by git-remote-curl.
|
||||
*
|
||||
* If it is, returns 1 and writes the URL that would be passed to
|
||||
@@ -1059,15 +1087,30 @@ static int check_submodule_url(const cha
|
||||
return -1;
|
||||
|
||||
if (submodule_url_is_relative(url)) {
|
||||
+ char *decoded;
|
||||
+ const char *next;
|
||||
+ int has_nl;
|
||||
+
|
||||
/*
|
||||
* This could be appended to an http URL and url-decoded;
|
||||
* check for malicious characters.
|
||||
*/
|
||||
- char *decoded = url_decode(url);
|
||||
- int has_nl = !!strchr(decoded, '\n');
|
||||
+ decoded = url_decode(url);
|
||||
+ has_nl = !!strchr(decoded, '\n');
|
||||
+
|
||||
free(decoded);
|
||||
if (has_nl)
|
||||
return -1;
|
||||
+
|
||||
+ /*
|
||||
+ * URLs which escape their root via "../" can overwrite
|
||||
+ * the host field and previous components, resolving to
|
||||
+ * URLs like https::example.com/submodule.git that were
|
||||
+ * susceptible to CVE-2020-11008.
|
||||
+ */
|
||||
+ if (count_leading_dotdots(url, &next) > 0 &&
|
||||
+ *next == ':')
|
||||
+ return -1;
|
||||
}
|
||||
|
||||
else if (url_to_curl_url(url, &curl_url)) {
|
||||
--- a/t/t5550-http-fetch-dumb.sh
|
||||
+++ b/t/t5550-http-fetch-dumb.sh
|
||||
@@ -321,11 +321,8 @@ test_expect_success 'git client does not
|
||||
'
|
||||
|
||||
test_expect_success 'remote-http complains cleanly about malformed urls' '
|
||||
- # do not actually issue "list" or other commands, as we do not
|
||||
- # want to rely on what curl would actually do with such a broken
|
||||
- # URL. This is just about making sure we do not segfault during
|
||||
- # initialization.
|
||||
- test_must_fail git remote-http http::/example.com/repo.git
|
||||
+ test_must_fail git remote-http http::/example.com/repo.git 2>stderr &&
|
||||
+ test_i18ngrep "url has no scheme" stderr
|
||||
'
|
||||
|
||||
test_expect_success 'redirects can be forbidden/allowed' '
|
||||
--- a/t/t7416-submodule-dash-url.sh
|
||||
+++ b/t/t7416-submodule-dash-url.sh
|
||||
@@ -60,6 +60,38 @@ test_expect_success 'trailing backslash
|
||||
test_i18ngrep ! "unknown option" err
|
||||
'
|
||||
|
||||
+test_expect_success 'fsck rejects missing URL scheme' '
|
||||
+ git checkout --orphan missing-scheme &&
|
||||
+ cat >.gitmodules <<-\EOF &&
|
||||
+ [submodule "foo"]
|
||||
+ url = http::one.example.com/foo.git
|
||||
+ EOF
|
||||
+ git add .gitmodules &&
|
||||
+ test_tick &&
|
||||
+ git commit -m "gitmodules with missing URL scheme" &&
|
||||
+ test_when_finished "rm -rf dst" &&
|
||||
+ git init --bare dst &&
|
||||
+ git -C dst config transfer.fsckObjects true &&
|
||||
+ test_must_fail git push dst HEAD 2>err &&
|
||||
+ grep gitmodulesUrl err
|
||||
+'
|
||||
+
|
||||
+test_expect_success 'fsck rejects relative URL resolving to missing scheme' '
|
||||
+ git checkout --orphan relative-missing-scheme &&
|
||||
+ cat >.gitmodules <<-\EOF &&
|
||||
+ [submodule "foo"]
|
||||
+ url = "..\\../.\\../:one.example.com/foo.git"
|
||||
+ EOF
|
||||
+ git add .gitmodules &&
|
||||
+ test_tick &&
|
||||
+ git commit -m "gitmodules with relative URL that strips off scheme" &&
|
||||
+ test_when_finished "rm -rf dst" &&
|
||||
+ git init --bare dst &&
|
||||
+ git -C dst config transfer.fsckObjects true &&
|
||||
+ test_must_fail git push dst HEAD 2>err &&
|
||||
+ grep gitmodulesUrl err
|
||||
+'
|
||||
+
|
||||
test_expect_success 'fsck permits embedded newline with unrecognized scheme' '
|
||||
git checkout --orphan newscheme &&
|
||||
cat >.gitmodules <<-\EOF &&
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,104 +0,0 @@
|
||||
From e7fab62b736cca3416660636e46f0be8386a5030 Mon Sep 17 00:00:00 2001
|
||||
From: Jonathan Nieder <jrnieder@gmail.com>
|
||||
Date: Sat, 18 Apr 2020 20:54:57 -0700
|
||||
Subject: [PATCH] credential: treat URL with empty scheme as invalid
|
||||
|
||||
Until "credential: refuse to operate when missing host or protocol",
|
||||
Git's credential handling code interpreted URLs with empty scheme to
|
||||
mean "give me credentials matching this host for any protocol".
|
||||
|
||||
Luckily libcurl does not recognize such URLs (it tries to look for a
|
||||
protocol named "" and fails). Just in case that changes, let's reject
|
||||
them within Git as well. This way, credential_from_url is guaranteed to
|
||||
always produce a "struct credential" with protocol and host set.
|
||||
|
||||
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
||||
---
|
||||
credential.c | 5 ++---
|
||||
t/t5550-http-fetch-dumb.sh | 9 +++++++++
|
||||
t/t7416-submodule-dash-url.sh | 32 ++++++++++++++++++++++++++++++++
|
||||
3 files changed, 43 insertions(+), 3 deletions(-)
|
||||
|
||||
--- a/credential.c
|
||||
+++ b/credential.c
|
||||
@@ -360,7 +360,7 @@ int credential_from_url_gently(struct cr
|
||||
* (3) proto://<user>:<pass>@<host>/...
|
||||
*/
|
||||
proto_end = strstr(url, "://");
|
||||
- if (!proto_end) {
|
||||
+ if (!proto_end || proto_end == url) {
|
||||
if (!quiet)
|
||||
warning(_("url has no scheme: %s"), url);
|
||||
return -1;
|
||||
@@ -385,8 +385,7 @@ int credential_from_url_gently(struct cr
|
||||
host = at + 1;
|
||||
}
|
||||
|
||||
- if (proto_end - url > 0)
|
||||
- c->protocol = xmemdupz(url, proto_end - url);
|
||||
+ c->protocol = xmemdupz(url, proto_end - url);
|
||||
c->host = url_decode_mem(host, slash - host);
|
||||
/* Trim leading and trailing slashes from path */
|
||||
while (*slash == '/')
|
||||
--- a/t/t5550-http-fetch-dumb.sh
|
||||
+++ b/t/t5550-http-fetch-dumb.sh
|
||||
@@ -325,6 +325,15 @@ test_expect_success 'remote-http complai
|
||||
test_i18ngrep "url has no scheme" stderr
|
||||
'
|
||||
|
||||
+# NEEDSWORK: Writing commands to git-remote-curl can race against the latter
|
||||
+# erroring out, producing SIGPIPE. Remove "ok=sigpipe" once transport-helper has
|
||||
+# learned to handle early remote helper failures more cleanly.
|
||||
+test_expect_success 'remote-http complains cleanly about empty scheme' '
|
||||
+ test_must_fail ok=sigpipe git ls-remote \
|
||||
+ http::${HTTPD_URL#http}/dumb/repo.git 2>stderr &&
|
||||
+ test_i18ngrep "url has no scheme" stderr
|
||||
+'
|
||||
+
|
||||
test_expect_success 'redirects can be forbidden/allowed' '
|
||||
test_must_fail git -c http.followRedirects=false \
|
||||
clone $HTTPD_URL/dumb-redir/repo.git dumb-redir &&
|
||||
--- a/t/t7416-submodule-dash-url.sh
|
||||
+++ b/t/t7416-submodule-dash-url.sh
|
||||
@@ -92,6 +92,38 @@ test_expect_success 'fsck rejects relati
|
||||
grep gitmodulesUrl err
|
||||
'
|
||||
|
||||
+test_expect_success 'fsck rejects empty URL scheme' '
|
||||
+ git checkout --orphan empty-scheme &&
|
||||
+ cat >.gitmodules <<-\EOF &&
|
||||
+ [submodule "foo"]
|
||||
+ url = http::://one.example.com/foo.git
|
||||
+ EOF
|
||||
+ git add .gitmodules &&
|
||||
+ test_tick &&
|
||||
+ git commit -m "gitmodules with empty URL scheme" &&
|
||||
+ test_when_finished "rm -rf dst" &&
|
||||
+ git init --bare dst &&
|
||||
+ git -C dst config transfer.fsckObjects true &&
|
||||
+ test_must_fail git push dst HEAD 2>err &&
|
||||
+ grep gitmodulesUrl err
|
||||
+'
|
||||
+
|
||||
+test_expect_success 'fsck rejects relative URL resolving to empty scheme' '
|
||||
+ git checkout --orphan relative-empty-scheme &&
|
||||
+ cat >.gitmodules <<-\EOF &&
|
||||
+ [submodule "foo"]
|
||||
+ url = ../../../:://one.example.com/foo.git
|
||||
+ EOF
|
||||
+ git add .gitmodules &&
|
||||
+ test_tick &&
|
||||
+ git commit -m "relative gitmodules URL resolving to empty scheme" &&
|
||||
+ test_when_finished "rm -rf dst" &&
|
||||
+ git init --bare dst &&
|
||||
+ git -C dst config transfer.fsckObjects true &&
|
||||
+ test_must_fail git push dst HEAD 2>err &&
|
||||
+ grep gitmodulesUrl err
|
||||
+'
|
||||
+
|
||||
test_expect_success 'fsck permits embedded newline with unrecognized scheme' '
|
||||
git checkout --orphan newscheme &&
|
||||
cat >.gitmodules <<-\EOF &&
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,106 +0,0 @@
|
||||
From 1a3609e402a062ef7b11f197fe96c28cabca132c Mon Sep 17 00:00:00 2001
|
||||
From: Jonathan Nieder <jrnieder@gmail.com>
|
||||
Date: Sat, 18 Apr 2020 20:57:22 -0700
|
||||
Subject: [PATCH] fsck: reject URL with empty host in .gitmodules
|
||||
|
||||
Git's URL parser interprets
|
||||
|
||||
https:///example.com/repo.git
|
||||
|
||||
to have no host and a path of "example.com/repo.git". Curl, on the
|
||||
other hand, internally redirects it to https://example.com/repo.git. As
|
||||
a result, until "credential: parse URL without host as empty host, not
|
||||
unset", tricking a user into fetching from such a URL would cause Git to
|
||||
send credentials for another host to example.com.
|
||||
|
||||
Teach fsck to block and detect .gitmodules files using such a URL to
|
||||
prevent sharing them with Git versions that are not yet protected.
|
||||
|
||||
A relative URL in a .gitmodules file could also be used to trigger this.
|
||||
The relative URL resolver used for .gitmodules does not normalize
|
||||
sequences of slashes and can follow ".." components out of the path part
|
||||
and to the host part of a URL, meaning that such a relative URL can be
|
||||
used to traverse from a https://foo.example.com/innocent superproject to
|
||||
a https:///attacker.example.com/exploit submodule. Fortunately,
|
||||
redundant extra slashes in .gitmodules are rare, so we can catch this by
|
||||
detecting one after a leading sequence of "./" and "../" components.
|
||||
|
||||
Helped-by: Jeff King <peff@peff.net>
|
||||
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
||||
Reviewed-by: Jeff King <peff@peff.net>
|
||||
---
|
||||
fsck.c | 10 +++++++---
|
||||
t/t7416-submodule-dash-url.sh | 32 ++++++++++++++++++++++++++++++++
|
||||
2 files changed, 39 insertions(+), 3 deletions(-)
|
||||
|
||||
--- a/fsck.c
|
||||
+++ b/fsck.c
|
||||
@@ -1105,17 +1105,21 @@ static int check_submodule_url(const cha
|
||||
/*
|
||||
* URLs which escape their root via "../" can overwrite
|
||||
* the host field and previous components, resolving to
|
||||
- * URLs like https::example.com/submodule.git that were
|
||||
+ * URLs like https::example.com/submodule.git and
|
||||
+ * https:///example.com/submodule.git that were
|
||||
* susceptible to CVE-2020-11008.
|
||||
*/
|
||||
if (count_leading_dotdots(url, &next) > 0 &&
|
||||
- *next == ':')
|
||||
+ (*next == ':' || *next == '/'))
|
||||
return -1;
|
||||
}
|
||||
|
||||
else if (url_to_curl_url(url, &curl_url)) {
|
||||
struct credential c = CREDENTIAL_INIT;
|
||||
- int ret = credential_from_url_gently(&c, curl_url, 1);
|
||||
+ int ret = 0;
|
||||
+ if (credential_from_url_gently(&c, curl_url, 1) ||
|
||||
+ !*c.host)
|
||||
+ ret = -1;
|
||||
credential_clear(&c);
|
||||
return ret;
|
||||
}
|
||||
--- a/t/t7416-submodule-dash-url.sh
|
||||
+++ b/t/t7416-submodule-dash-url.sh
|
||||
@@ -124,6 +124,38 @@ test_expect_success 'fsck rejects relati
|
||||
grep gitmodulesUrl err
|
||||
'
|
||||
|
||||
+test_expect_success 'fsck rejects empty hostname' '
|
||||
+ git checkout --orphan empty-host &&
|
||||
+ cat >.gitmodules <<-\EOF &&
|
||||
+ [submodule "foo"]
|
||||
+ url = http:///one.example.com/foo.git
|
||||
+ EOF
|
||||
+ git add .gitmodules &&
|
||||
+ test_tick &&
|
||||
+ git commit -m "gitmodules with extra slashes" &&
|
||||
+ test_when_finished "rm -rf dst" &&
|
||||
+ git init --bare dst &&
|
||||
+ git -C dst config transfer.fsckObjects true &&
|
||||
+ test_must_fail git push dst HEAD 2>err &&
|
||||
+ grep gitmodulesUrl err
|
||||
+'
|
||||
+
|
||||
+test_expect_success 'fsck rejects relative url that produced empty hostname' '
|
||||
+ git checkout --orphan messy-relative &&
|
||||
+ cat >.gitmodules <<-\EOF &&
|
||||
+ [submodule "foo"]
|
||||
+ url = ../../..//one.example.com/foo.git
|
||||
+ EOF
|
||||
+ git add .gitmodules &&
|
||||
+ test_tick &&
|
||||
+ git commit -m "gitmodules abusing relative_path" &&
|
||||
+ test_when_finished "rm -rf dst" &&
|
||||
+ git init --bare dst &&
|
||||
+ git -C dst config transfer.fsckObjects true &&
|
||||
+ test_must_fail git push dst HEAD 2>err &&
|
||||
+ grep gitmodulesUrl err
|
||||
+'
|
||||
+
|
||||
test_expect_success 'fsck permits embedded newline with unrecognized scheme' '
|
||||
git checkout --orphan newscheme &&
|
||||
cat >.gitmodules <<-\EOF &&
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,61 +0,0 @@
|
||||
From 10372307c11d0128c40d730418d3776f1a959ce3 Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Wed, 11 Mar 2020 17:53:41 -0400
|
||||
Subject: [PATCH] credential: avoid writing values with newlines
|
||||
|
||||
The credential protocol that we use to speak to helpers can't represent
|
||||
values with newlines in them. This was an intentional design choice to
|
||||
keep the protocol simple, since none of the values we pass should
|
||||
generally have newlines.
|
||||
|
||||
However, if we _do_ encounter a newline in a value, we blindly transmit
|
||||
it in credential_write(). Such values may break the protocol syntax, or
|
||||
worse, inject new valid lines into the protocol stream.
|
||||
|
||||
The most likely way for a newline to end up in a credential struct is by
|
||||
decoding a URL with a percent-encoded newline. However, since the bug
|
||||
occurs at the moment we write the value to the protocol, we'll catch it
|
||||
there. That should leave no possibility of accidentally missing a code
|
||||
path that can trigger the problem.
|
||||
|
||||
At this level of the code we have little choice but to die(). However,
|
||||
since we'd not ever expect to see this case outside of a malicious URL,
|
||||
that's an acceptable outcome.
|
||||
|
||||
Reported-by: Felix Wilhelm <fwilhelm@google.com>
|
||||
---
|
||||
credential.c | 2 ++
|
||||
t/t0300-credentials.sh | 6 ++++++
|
||||
2 files changed, 8 insertions(+)
|
||||
|
||||
diff --git a/credential.c b/credential.c
|
||||
index 62be651..a79aff0 100644
|
||||
--- a/credential.c
|
||||
+++ b/credential.c
|
||||
@@ -195,6 +195,8 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)
|
||||
{
|
||||
if (!value)
|
||||
return;
|
||||
+ if (strchr(value, '\n'))
|
||||
+ die("credential value for %s contains newline", key);
|
||||
fprintf(fp, "%s=%s\n", key, value);
|
||||
}
|
||||
|
||||
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
|
||||
index 82eaaea..26f3c3a 100755
|
||||
--- a/t/t0300-credentials.sh
|
||||
+++ b/t/t0300-credentials.sh
|
||||
@@ -308,4 +308,10 @@ test_expect_success 'empty helper spec resets helper list' '
|
||||
EOF
|
||||
'
|
||||
|
||||
+test_expect_success 'url parser rejects embedded newlines' '
|
||||
+ test_must_fail git credential fill <<-\EOF
|
||||
+ url=https://one.example.com?%0ahost=two.example.com/
|
||||
+ EOF
|
||||
+'
|
||||
+
|
||||
test_done
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,151 +0,0 @@
|
||||
From c716fe4bd917e013bf376a678b3a924447777b2d Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Thu, 12 Mar 2020 01:31:11 -0400
|
||||
Subject: [PATCH] credential: detect unrepresentable values when parsing urls
|
||||
|
||||
The credential protocol can't represent newlines in values, but URLs can
|
||||
embed percent-encoded newlines in various components. A previous commit
|
||||
taught the low-level writing routines to die() when encountering this,
|
||||
but we can be a little friendlier to the user by detecting them earlier
|
||||
and handling them gracefully.
|
||||
|
||||
This patch teaches credential_from_url() to notice such components,
|
||||
issue a warning, and blank the credential (which will generally result
|
||||
in prompting the user for a username and password). We blank the whole
|
||||
credential in this case. Another option would be to blank only the
|
||||
invalid component. However, we're probably better off not feeding a
|
||||
partially-parsed URL result to a credential helper. We don't know how a
|
||||
given helper would handle it, so we're better off to err on the side of
|
||||
matching nothing rather than something unexpected.
|
||||
|
||||
The die() call in credential_write() is _probably_ impossible to reach
|
||||
after this patch. Values should end up in credential structs only by URL
|
||||
parsing (which is covered here), or by reading credential protocol input
|
||||
(which by definition cannot read a newline into a value). But we should
|
||||
definitely keep the low-level check, as it's our final and most accurate
|
||||
line of defense against protocol injection attacks. Arguably it could
|
||||
become a BUG(), but it probably doesn't matter much either way.
|
||||
|
||||
Note that the public interface of credential_from_url() grows a little
|
||||
more than we need here. We'll use the extra flexibility in a future
|
||||
patch to help fsck catch these cases.
|
||||
---
|
||||
credential.c | 36 ++++++++++++++++++++++++++++++++++--
|
||||
credential.h | 16 ++++++++++++++++
|
||||
t/t0300-credentials.sh | 12 ++++++++++--
|
||||
3 files changed, 60 insertions(+), 4 deletions(-)
|
||||
|
||||
diff --git a/credential.c b/credential.c
|
||||
index 00ee4d6..eeeac32 100644
|
||||
--- a/credential.c
|
||||
+++ b/credential.c
|
||||
@@ -321,7 +321,22 @@ void credential_reject(struct credential *c)
|
||||
c->approved = 0;
|
||||
}
|
||||
|
||||
-void credential_from_url(struct credential *c, const char *url)
|
||||
+static int check_url_component(const char *url, int quiet,
|
||||
+ const char *name, const char *value)
|
||||
+{
|
||||
+ if (!value)
|
||||
+ return 0;
|
||||
+ if (!strchr(value, '\n'))
|
||||
+ return 0;
|
||||
+
|
||||
+ if (!quiet)
|
||||
+ warning(_("url contains a newline in its %s component: %s"),
|
||||
+ name, url);
|
||||
+ return -1;
|
||||
+}
|
||||
+
|
||||
+int credential_from_url_gently(struct credential *c, const char *url,
|
||||
+ int quiet)
|
||||
{
|
||||
const char *at, *colon, *cp, *slash, *host, *proto_end;
|
||||
|
||||
@@ -335,7 +350,7 @@ void credential_from_url(struct credential *c, const char *url)
|
||||
*/
|
||||
proto_end = strstr(url, "://");
|
||||
if (!proto_end)
|
||||
- return;
|
||||
+ return 0;
|
||||
cp = proto_end + 3;
|
||||
at = strchr(cp, '@');
|
||||
colon = strchr(cp, ':');
|
||||
@@ -370,4 +385,21 @@ void credential_from_url(struct credential *c, const char *url)
|
||||
while (p > c->path && *p == '/')
|
||||
*p-- = '\0';
|
||||
}
|
||||
+
|
||||
+ if (check_url_component(url, quiet, "username", c->username) < 0 ||
|
||||
+ check_url_component(url, quiet, "password", c->password) < 0 ||
|
||||
+ check_url_component(url, quiet, "protocol", c->protocol) < 0 ||
|
||||
+ check_url_component(url, quiet, "host", c->host) < 0 ||
|
||||
+ check_url_component(url, quiet, "path", c->path) < 0)
|
||||
+ return -1;
|
||||
+
|
||||
+ return 0;
|
||||
+}
|
||||
+
|
||||
+void credential_from_url(struct credential *c, const char *url)
|
||||
+{
|
||||
+ if (credential_from_url_gently(c, url, 0) < 0) {
|
||||
+ warning(_("skipping credential lookup for url: %s"), url);
|
||||
+ credential_clear(c);
|
||||
+ }
|
||||
}
|
||||
diff --git a/credential.h b/credential.h
|
||||
index 6b0cd16..122a23c 100644
|
||||
--- a/credential.h
|
||||
+++ b/credential.h
|
||||
@@ -28,7 +28,23 @@ struct credential {
|
||||
|
||||
int credential_read(struct credential *, FILE *);
|
||||
void credential_write(const struct credential *, FILE *);
|
||||
+
|
||||
+/*
|
||||
+ * Parse a url into a credential struct, replacing any existing contents.
|
||||
+ *
|
||||
+ * Ifthe url can't be parsed (e.g., a missing "proto://" component), the
|
||||
+ * resulting credential will be empty but we'll still return success from the
|
||||
+ * "gently" form.
|
||||
+ *
|
||||
+ * If we encounter a component which cannot be represented as a credential
|
||||
+ * value (e.g., because it contains a newline), the "gently" form will return
|
||||
+ * an error but leave the broken state in the credential object for further
|
||||
+ * examination. The non-gentle form will issue a warning to stderr and return
|
||||
+ * an empty credential.
|
||||
+ */
|
||||
void credential_from_url(struct credential *, const char *url);
|
||||
+int credential_from_url_gently(struct credential *, const char *url, int quiet);
|
||||
+
|
||||
int credential_match(const struct credential *have,
|
||||
const struct credential *want);
|
||||
|
||||
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
|
||||
index 15cc3c5..3bec445 100755
|
||||
--- a/t/t0300-credentials.sh
|
||||
+++ b/t/t0300-credentials.sh
|
||||
@@ -309,9 +309,17 @@ test_expect_success 'empty helper spec resets helper list' '
|
||||
EOF
|
||||
'
|
||||
|
||||
-test_expect_success 'url parser rejects embedded newlines' '
|
||||
- test_must_fail git credential fill <<-\EOF
|
||||
+test_expect_success 'url parser ignores embedded newlines' '
|
||||
+ check fill <<-EOF
|
||||
url=https://one.example.com?%0ahost=two.example.com/
|
||||
+ --
|
||||
+ username=askpass-username
|
||||
+ password=askpass-password
|
||||
+ --
|
||||
+ warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/
|
||||
+ warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/
|
||||
+ askpass: Username:
|
||||
+ askpass: Password:
|
||||
EOF
|
||||
'
|
||||
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,88 +0,0 @@
|
||||
From e5e484f5bb9690ac2aaab2d0ca23e7433b2ac8c0 Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Thu, 29 Aug 2019 13:33:48 -0400
|
||||
Subject: [PATCH 05/30] fast-import: delay creating leading directories for
|
||||
export-marks
|
||||
|
||||
When we parse the --export-marks option, we don't immediately open the
|
||||
file, but we do create any leading directories. This can be especially
|
||||
confusing when a command-line option overrides an in-stream one, in
|
||||
which case we'd create the leading directory for the in-stream file,
|
||||
even though we never actually write the file.
|
||||
|
||||
Let's instead create the directories just before opening the file, which
|
||||
means we'll create only useful directories. Note that this could change
|
||||
the handling of relative paths if we chdir() in between, but we don't
|
||||
actually do so; the only permanent chdir is from setup_git_directory()
|
||||
which runs before either code path (potentially we should take the
|
||||
pre-setup dir into account to avoid surprising the user, but that's an
|
||||
orthogonal change).
|
||||
|
||||
The test just adapts the existing "override" test to use paths with
|
||||
leading directories. This checks both that the correct directory is
|
||||
created (which worked before but was not tested), and that the
|
||||
overridden one is not (our new fix here).
|
||||
|
||||
While we're here, let's also check the error result of
|
||||
safe_create_leading_directories(). We'd presumably notice any failure
|
||||
immediately after when we try to open the file itself, but we can give a
|
||||
more specific error message in this case.
|
||||
|
||||
Signed-off-by: Jeff King <peff@peff.net>
|
||||
---
|
||||
fast-import.c | 7 ++++++-
|
||||
t/t9300-fast-import.sh | 13 +++++++++++--
|
||||
2 files changed, 17 insertions(+), 3 deletions(-)
|
||||
|
||||
diff --git a/fast-import.c b/fast-import.c
|
||||
index b05d560d0a..92e84d28a4 100644
|
||||
--- a/fast-import.c
|
||||
+++ b/fast-import.c
|
||||
@@ -1826,6 +1826,12 @@ static void dump_marks(void)
|
||||
if (!export_marks_file || (import_marks_file && !import_marks_file_done))
|
||||
return;
|
||||
|
||||
+ if (safe_create_leading_directories_const(export_marks_file)) {
|
||||
+ failure |= error_errno("unable to create leading directories of %s",
|
||||
+ export_marks_file);
|
||||
+ return;
|
||||
+ }
|
||||
+
|
||||
if (hold_lock_file_for_update(&mark_lock, export_marks_file, 0) < 0) {
|
||||
failure |= error_errno("Unable to write marks file %s",
|
||||
export_marks_file);
|
||||
@@ -3238,7 +3244,6 @@ static void option_active_branches(const char *branches)
|
||||
static void option_export_marks(const char *marks)
|
||||
{
|
||||
export_marks_file = make_fast_import_path(marks);
|
||||
- safe_create_leading_directories_const(export_marks_file);
|
||||
}
|
||||
|
||||
static void option_cat_blob_fd(const char *fd)
|
||||
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
|
||||
index f79fcf4c1e..f5f3e2cb71 100755
|
||||
--- a/t/t9300-fast-import.sh
|
||||
+++ b/t/t9300-fast-import.sh
|
||||
@@ -2132,8 +2132,17 @@ test_expect_success 'R: export-marks feature results in a marks file being creat
|
||||
'
|
||||
|
||||
test_expect_success 'R: export-marks options can be overridden by commandline options' '
|
||||
- git fast-import --export-marks=other.marks <input &&
|
||||
- grep :1 other.marks
|
||||
+ cat >input <<-\EOF &&
|
||||
+ feature export-marks=feature-sub/git.marks
|
||||
+ blob
|
||||
+ mark :1
|
||||
+ data 3
|
||||
+ hi
|
||||
+
|
||||
+ EOF
|
||||
+ git fast-import --export-marks=cmdline-sub/other.marks <input &&
|
||||
+ grep :1 cmdline-sub/other.marks &&
|
||||
+ test_path_is_missing feature-sub
|
||||
'
|
||||
|
||||
test_expect_success 'R: catch typo in marks file name' '
|
||||
--
|
||||
2.17.1
|
||||
|
||||
@ -1,40 +0,0 @@
|
||||
From 9d09df88970ab91a08de15f0b2f9ab9b77ffd2cd Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Thu, 29 Aug 2019 13:07:04 -0400
|
||||
Subject: [PATCH 04/30] fast-import: stop creating leading directories for
|
||||
import-marks
|
||||
|
||||
When asked to import marks from "subdir/file.marks", we create the
|
||||
leading directory "subdir" if it doesn't exist. This makes no sense for
|
||||
importing marks, where we only ever open the path for reading.
|
||||
|
||||
Most of the time this would be a noop, since if the marks file exists,
|
||||
then the leading directories exist, too. But if it doesn't (e.g.,
|
||||
because --import-marks-if-exists was used), then we'd create the useless
|
||||
directory.
|
||||
|
||||
This dates back to 580d5f83e7 (fast-import: always create marks_file
|
||||
directories, 2010-03-29). Even then it was useless, so it seems to have
|
||||
been added in error alongside the --export-marks case (which _is_
|
||||
helpful).
|
||||
|
||||
Signed-off-by: Jeff King <peff@peff.net>
|
||||
---
|
||||
fast-import.c | 1 -
|
||||
1 file changed, 1 deletion(-)
|
||||
|
||||
diff --git a/fast-import.c b/fast-import.c
|
||||
index 30b9479a75..b05d560d0a 100644
|
||||
--- a/fast-import.c
|
||||
+++ b/fast-import.c
|
||||
@@ -3198,7 +3198,6 @@ static void option_import_marks(const char *marks,
|
||||
}
|
||||
|
||||
import_marks_file = make_fast_import_path(marks);
|
||||
- safe_create_leading_directories_const(import_marks_file);
|
||||
import_marks_file_from_stream = from_stream;
|
||||
import_marks_file_ignore_missing = ignore_missing;
|
||||
}
|
||||
--
|
||||
2.17.1
|
||||
|
||||
@ -1,39 +0,0 @@
|
||||
From a4d15d0cfb630844eb48a11f69ccaef9fa8719f4 Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Thu, 29 Aug 2019 11:25:45 -0400
|
||||
Subject: [PATCH 03/30] fast-import: tighten parsing of boolean command line
|
||||
options
|
||||
|
||||
We parse options like "--max-pack-size=" using skip_prefix(), which
|
||||
makes sense to get at the bytes after the "=". However, we also parse
|
||||
"--quiet" and "--stats" with skip_prefix(), which allows things like
|
||||
"--quiet-nonsense" to behave like "--quiet".
|
||||
|
||||
This was a mistaken conversion in 0f6927c229 (fast-import: put option
|
||||
parsing code in separate functions, 2009-12-04). Let's tighten this to
|
||||
an exact match, which was the original intent.
|
||||
|
||||
Signed-off-by: Jeff King <peff@peff.net>
|
||||
---
|
||||
fast-import.c | 4 ++--
|
||||
1 file changed, 2 insertions(+), 2 deletions(-)
|
||||
|
||||
diff --git a/fast-import.c b/fast-import.c
|
||||
index 69886687ce..30b9479a75 100644
|
||||
--- a/fast-import.c
|
||||
+++ b/fast-import.c
|
||||
@@ -3282,9 +3282,9 @@ static int parse_one_option(const char *option)
|
||||
option_active_branches(option);
|
||||
} else if (skip_prefix(option, "export-pack-edges=", &option)) {
|
||||
option_export_pack_edges(option);
|
||||
- } else if (starts_with(option, "quiet")) {
|
||||
+ } else if (!strcmp(option, "quiet")) {
|
||||
show_stats = 0;
|
||||
- } else if (starts_with(option, "stats")) {
|
||||
+ } else if (!strcmp(option, "stats")) {
|
||||
show_stats = 1;
|
||||
} else {
|
||||
return 0;
|
||||
--
|
||||
2.17.1
|
||||
|
||||
@ -1,99 +0,0 @@
|
||||
From 07259e74ec1237c836874342c65650bdee8a3993 Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Wed, 11 Mar 2020 18:48:24 -0400
|
||||
Subject: [PATCH] fsck: detect gitmodules URLs with embedded newlines
|
||||
|
||||
The credential protocol can't handle values with newlines. We already
|
||||
detect and block any such URLs from being used with credential helpers,
|
||||
but let's also add an fsck check to detect and block gitmodules files
|
||||
with such URLs. That will let us notice the problem earlier when
|
||||
transfer.fsckObjects is turned on. And in particular it will prevent bad
|
||||
objects from spreading, which may protect downstream users running older
|
||||
versions of Git.
|
||||
|
||||
We'll file this under the existing gitmodulesUrl flag, which covers URLs
|
||||
with option injection. There's really no need to distinguish the exact
|
||||
flaw in the URL in this context. Likewise, I've expanded the description
|
||||
of t7416 to cover all types of bogus URLs.
|
||||
---
|
||||
fsck.c | 16 +++++++++++++++-
|
||||
t/t7416-submodule-dash-url.sh | 18 +++++++++++++++++-
|
||||
2 files changed, 32 insertions(+), 2 deletions(-)
|
||||
|
||||
diff --git a/fsck.c b/fsck.c
|
||||
index 0741e62..5b437c2 100644
|
||||
--- a/fsck.c
|
||||
+++ b/fsck.c
|
||||
@@ -14,6 +14,7 @@
|
||||
#include "packfile.h"
|
||||
#include "submodule-config.h"
|
||||
#include "config.h"
|
||||
+#include "credential.h"
|
||||
#include "help.h"
|
||||
|
||||
static struct oidset gitmodules_found = OIDSET_INIT;
|
||||
@@ -941,6 +942,19 @@ static int fsck_tag(struct tag *tag, const char *data,
|
||||
return fsck_tag_buffer(tag, data, size, options);
|
||||
}
|
||||
|
||||
+static int check_submodule_url(const char *url)
|
||||
+{
|
||||
+ struct credential c = CREDENTIAL_INIT;
|
||||
+ int ret;
|
||||
+
|
||||
+ if (looks_like_command_line_option(url))
|
||||
+ return -1;
|
||||
+
|
||||
+ ret = credential_from_url_gently(&c, url, 1);
|
||||
+ credential_clear(&c);
|
||||
+ return ret;
|
||||
+}
|
||||
+
|
||||
struct fsck_gitmodules_data {
|
||||
struct object *obj;
|
||||
struct fsck_options *options;
|
||||
@@ -965,7 +979,7 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata)
|
||||
"disallowed submodule name: %s",
|
||||
name);
|
||||
if (!strcmp(key, "url") && value &&
|
||||
- looks_like_command_line_option(value))
|
||||
+ check_submodule_url(value) < 0)
|
||||
data->ret |= report(data->options, data->obj,
|
||||
FSCK_MSG_GITMODULES_URL,
|
||||
"disallowed submodule url: %s",
|
||||
diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh
|
||||
index 5ba041f..41431b1 100755
|
||||
--- a/t/t7416-submodule-dash-url.sh
|
||||
+++ b/t/t7416-submodule-dash-url.sh
|
||||
@@ -1,6 +1,6 @@
|
||||
#!/bin/sh
|
||||
|
||||
-test_description='check handling of .gitmodule url with dash'
|
||||
+test_description='check handling of disallowed .gitmodule urls'
|
||||
. ./test-lib.sh
|
||||
|
||||
test_expect_success 'create submodule with protected dash in url' '
|
||||
@@ -60,4 +60,20 @@ test_expect_success 'trailing backslash is handled correctly' '
|
||||
test_i18ngrep ! "unknown option" err
|
||||
'
|
||||
|
||||
+test_expect_success 'fsck rejects embedded newline in url' '
|
||||
+ # create an orphan branch to avoid existing .gitmodules objects
|
||||
+ git checkout --orphan newline &&
|
||||
+ cat >.gitmodules <<-\EOF &&
|
||||
+ [submodule "foo"]
|
||||
+ url = "https://one.example.com?%0ahost=two.example.com/foo.git"
|
||||
+ EOF
|
||||
+ git add .gitmodules &&
|
||||
+ git commit -m "gitmodules with newline" &&
|
||||
+ test_when_finished "rm -rf dst" &&
|
||||
+ git init --bare dst &&
|
||||
+ git -C dst config transfer.fsckObjects true &&
|
||||
+ test_must_fail git push dst HEAD 2>err &&
|
||||
+ grep gitmodulesUrl err
|
||||
+'
|
||||
+
|
||||
test_done
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
Binary file not shown.
Binary file not shown.
BIN
git-2.27.0.tar.sign
Normal file
BIN
git-2.27.0.tar.sign
Normal file
Binary file not shown.
BIN
git-2.27.0.tar.xz
Normal file
BIN
git-2.27.0.tar.xz
Normal file
Binary file not shown.
41
git.spec
41
git.spec
@ -1,7 +1,7 @@
|
||||
%global gitexecdir %{_libexecdir}/git-core
|
||||
Name: git
|
||||
Version: 2.23.0
|
||||
Release: 16
|
||||
Version: 2.27.0
|
||||
Release: 1
|
||||
Summary: A popular and widely used Version Control System
|
||||
License: GPLv2+ or LGPLv2.1
|
||||
URL: https://git-scm.com/
|
||||
@ -12,36 +12,6 @@ Source100: git-gui.desktop
|
||||
Source101: git@.service.in
|
||||
Source102: git.socket
|
||||
|
||||
Patch0: t9300-drop-some-useless-uses-of-cat.patch
|
||||
Patch1: fast-import-tighten-parsing-of-boolean-command-line-.patch
|
||||
Patch2: fast-import-stop-creating-leading-directories-for-im.patch
|
||||
Patch3: fast-import-delay-creating-leading-directories-for-e.patch
|
||||
Patch4: CVE-2019-1348.patch
|
||||
Patch5: CVE-2019-1349.patch
|
||||
Patch6: CVE-2019-1350.patch
|
||||
Patch7: CVE-2019-1351.patch
|
||||
Patch8: path.c-document-the-purpose-of-is_ntfs_dotgit.patch
|
||||
Patch9: CVE-2019-1352.patch
|
||||
Patch10: CVE-2019-1353.patch
|
||||
Patch11: CVE-2019-1354.patch
|
||||
Patch12: CVE-2019-1387.patch
|
||||
Patch13: CVE-2019-19604.patch
|
||||
# skip updating the gpg preference during running the test
|
||||
# suite, so we add this patch to skip the updating of preference
|
||||
Patch14: skip-updating-the-preference.patch
|
||||
Patch15: CVE-2020-5260.patch
|
||||
Patch16: credential-detect-unrepresentable-values-when-parsin.patch
|
||||
Patch17: fsck-detect-gitmodules-URLs-with-embedded-newlines.patch
|
||||
Patch18: CVE-2020-11008-1.patch
|
||||
Patch19: CVE-2020-11008-2.patch
|
||||
Patch20: CVE-2020-11008-3.patch
|
||||
Patch21: CVE-2020-11008-4.patch
|
||||
Patch22: CVE-2020-11008-5.patch
|
||||
Patch23: CVE-2020-11008-6.patch
|
||||
Patch24: CVE-2020-11008-7.patch
|
||||
Patch25: CVE-2020-11008-8.patch
|
||||
Patch26: CVE-2020-11008-9.patch
|
||||
|
||||
BuildRequires: openssl-devel libcurl-devel expat-devel systemd asciidoc xmlto glib2-devel libsecret-devel pcre-devel desktop-file-utils
|
||||
BuildRequires: python3-devel perl-generators perl-interpreter perl-Error perl(Test::More) perl-MailTools perl(Test) gdb
|
||||
Requires: less zlib openssh-clients perl(Term::ReadKey) perl-Git
|
||||
@ -172,6 +142,7 @@ sed -i '1s@#![ ]*/usr/bin/env python@#!%{__python3}@' \
|
||||
%make_build -C contrib/subtree/
|
||||
%make_build -C contrib/contacts/
|
||||
%make_build -C contrib/credential/libsecret/
|
||||
%make_build -C contrib/credential/netrc/
|
||||
%make_build -C contrib/diff-highlight/
|
||||
|
||||
%install
|
||||
@ -288,6 +259,12 @@ make test
|
||||
%{_mandir}/man7/git*.7.*
|
||||
|
||||
%changelog
|
||||
* Tue Jul 28 2020 yang_zhuang_zhuang <yangzhuangzhuang1@huawei.com> - 2.27.0-1
|
||||
- Type:enhancement
|
||||
- ID:NA
|
||||
- SUG:NA
|
||||
- DESC:update version to 2.27.0
|
||||
|
||||
* Thu May 14 2020 gaihuiying <gaihuiying1@huawei.com> - 2.23.0-16
|
||||
- Type:cves
|
||||
- ID:CVE-2020-11008
|
||||
|
||||
@ -1,56 +0,0 @@
|
||||
From 525e7fba7854c23ee3530d0bf88d75f106f14c95 Mon Sep 17 00:00:00 2001
|
||||
From: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
Date: Mon, 16 Sep 2019 20:44:31 +0200
|
||||
Subject: [PATCH] path.c: document the purpose of `is_ntfs_dotgit()`
|
||||
|
||||
Previously, this function was completely undocumented. It is worth,
|
||||
though, to explain what is going on, as it is not really obvious at all.
|
||||
|
||||
Suggested-by: Garima Singh <garima.singh@microsoft.com>
|
||||
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
||||
---
|
||||
path.c | 28 ++++++++++++++++++++++++++++
|
||||
1 file changed, 28 insertions(+)
|
||||
|
||||
diff --git a/path.c b/path.c
|
||||
index 9ac0531..22bd0b6 100644
|
||||
--- a/path.c
|
||||
+++ b/path.c
|
||||
@@ -1302,6 +1302,34 @@ static int only_spaces_and_periods(const char *path, size_t len, size_t skip)
|
||||
return 1;
|
||||
}
|
||||
|
||||
+/*
|
||||
+ * On NTFS, we need to be careful to disallow certain synonyms of the `.git/`
|
||||
+ * directory:
|
||||
+ *
|
||||
+ * - For historical reasons, file names that end in spaces or periods are
|
||||
+ * automatically trimmed. Therefore, `.git . . ./` is a valid way to refer
|
||||
+ * to `.git/`.
|
||||
+ *
|
||||
+ * - For other historical reasons, file names that do not conform to the 8.3
|
||||
+ * format (up to eight characters for the basename, three for the file
|
||||
+ * extension, certain characters not allowed such as `+`, etc) are associated
|
||||
+ * with a so-called "short name", at least on the `C:` drive by default.
|
||||
+ * Which means that `git~1/` is a valid way to refer to `.git/`.
|
||||
+ *
|
||||
+ * Note: Technically, `.git/` could receive the short name `git~2` if the
|
||||
+ * short name `git~1` were already used. In Git, however, we guarantee that
|
||||
+ * `.git` is the first item in a directory, therefore it will be associated
|
||||
+ * with the short name `git~1` (unless short names are disabled).
|
||||
+ *
|
||||
+ * When this function returns 1, it indicates that the specified file/directory
|
||||
+ * name refers to a `.git` file or directory, or to any of these synonyms, and
|
||||
+ * Git should therefore not track it.
|
||||
+ *
|
||||
+ * This function is intended to be used by `git fsck` even on platforms where
|
||||
+ * the backslash is a regular filename character, therefore it needs to handle
|
||||
+ * backlash characters in the provided `name` specially: they are interpreted
|
||||
+ * as directory separators.
|
||||
+ */
|
||||
int is_ntfs_dotgit(const char *name)
|
||||
{
|
||||
size_t len;
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,26 +0,0 @@
|
||||
From 2153fd2197b86d2b2146e0a1320d1e23ad25c074 Mon Sep 17 00:00:00 2001
|
||||
From: openEuler Buildteam <buildteam@openeuler.or
|
||||
g>
|
||||
Date: Thu, 19 Mar 2020 21:06:15 +0800
|
||||
Subject: [PATCH] skip updating the preference
|
||||
|
||||
---
|
||||
t/lib-gpg.sh | 2 +-
|
||||
1 file changed, 1 insertion(+), 1 deletion(-)
|
||||
|
||||
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
|
||||
index 8d28652..ffc7f7c 100755
|
||||
--- a/t/lib-gpg.sh
|
||||
+++ b/t/lib-gpg.sh
|
||||
@@ -32,7 +32,7 @@ then
|
||||
GNUPGHOME="$(pwd)/gpghome" &&
|
||||
export GNUPGHOME &&
|
||||
(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
|
||||
- gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
|
||||
+ gpg --homedir "${GNUPGHOME}" 2>/dev/null --import --batch \
|
||||
"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
|
||||
gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
|
||||
"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
@ -1,61 +0,0 @@
|
||||
From f94804c1f2626831c6bdf8cc269a571324e3f2f2 Mon Sep 17 00:00:00 2001
|
||||
From: Jeff King <peff@peff.net>
|
||||
Date: Thu, 29 Aug 2019 11:19:18 -0400
|
||||
Subject: [PATCH] t9300: drop some useless uses of cat
|
||||
|
||||
These waste a process, and make the line longer than it needs to be.
|
||||
|
||||
Signed-off-by: Jeff King <peff@peff.net>
|
||||
---
|
||||
t/t9300-fast-import.sh | 10 +++++-----
|
||||
1 file changed, 5 insertions(+), 5 deletions(-)
|
||||
|
||||
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
|
||||
index d47560b..1d2a751 100755
|
||||
--- a/t/t9300-fast-import.sh
|
||||
+++ b/t/t9300-fast-import.sh
|
||||
@@ -2125,12 +2125,12 @@ test_expect_success 'R: export-marks feature results in a marks file being creat
|
||||
|
||||
EOF
|
||||
|
||||
- cat input | git fast-import &&
|
||||
+ git fast-import <input &&
|
||||
grep :1 git.marks
|
||||
'
|
||||
|
||||
test_expect_success 'R: export-marks options can be overridden by commandline options' '
|
||||
- cat input | git fast-import --export-marks=other.marks &&
|
||||
+ git fast-import --export-marks=other.marks <input &&
|
||||
grep :1 other.marks
|
||||
'
|
||||
|
||||
@@ -2242,7 +2242,7 @@ test_expect_success 'R: import to output marks works without any content' '
|
||||
feature export-marks=marks.new
|
||||
EOF
|
||||
|
||||
- cat input | git fast-import &&
|
||||
+ git fast-import <input &&
|
||||
test_cmp marks.out marks.new
|
||||
'
|
||||
|
||||
@@ -2252,7 +2252,7 @@ test_expect_success 'R: import marks prefers commandline marks file over the str
|
||||
feature export-marks=marks.new
|
||||
EOF
|
||||
|
||||
- cat input | git fast-import --import-marks=marks.out &&
|
||||
+ git fast-import --import-marks=marks.out <input &&
|
||||
test_cmp marks.out marks.new
|
||||
'
|
||||
|
||||
@@ -2560,7 +2560,7 @@ test_expect_success 'R: quiet option results in no stats being output' '
|
||||
|
||||
EOF
|
||||
|
||||
- cat input | git fast-import 2> output &&
|
||||
+ git fast-import 2>output <input &&
|
||||
test_must_be_empty output
|
||||
'
|
||||
|
||||
--
|
||||
1.8.3.1
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user