update version to 2.27.0
This commit is contained in:
parent
fec82d92e8
commit
9cc7c20d15
@ -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
|
%global gitexecdir %{_libexecdir}/git-core
|
||||||
Name: git
|
Name: git
|
||||||
Version: 2.23.0
|
Version: 2.27.0
|
||||||
Release: 16
|
Release: 1
|
||||||
Summary: A popular and widely used Version Control System
|
Summary: A popular and widely used Version Control System
|
||||||
License: GPLv2+ or LGPLv2.1
|
License: GPLv2+ or LGPLv2.1
|
||||||
URL: https://git-scm.com/
|
URL: https://git-scm.com/
|
||||||
@ -12,36 +12,6 @@ Source100: git-gui.desktop
|
|||||||
Source101: git@.service.in
|
Source101: git@.service.in
|
||||||
Source102: git.socket
|
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: 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
|
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
|
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/subtree/
|
||||||
%make_build -C contrib/contacts/
|
%make_build -C contrib/contacts/
|
||||||
%make_build -C contrib/credential/libsecret/
|
%make_build -C contrib/credential/libsecret/
|
||||||
|
%make_build -C contrib/credential/netrc/
|
||||||
%make_build -C contrib/diff-highlight/
|
%make_build -C contrib/diff-highlight/
|
||||||
|
|
||||||
%install
|
%install
|
||||||
@ -288,6 +259,12 @@ make test
|
|||||||
%{_mandir}/man7/git*.7.*
|
%{_mandir}/man7/git*.7.*
|
||||||
|
|
||||||
%changelog
|
%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
|
* Thu May 14 2020 gaihuiying <gaihuiying1@huawei.com> - 2.23.0-16
|
||||||
- Type:cves
|
- Type:cves
|
||||||
- ID:CVE-2020-11008
|
- 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