Package: git / 1:2.20.1-2+deb10u3

Metadata

Package Version Patches format
git 1:2.20.1-2+deb10u3 3.0 (quilt)

Patch series

view the series file
Patch File delta Description
0001 t9300 drop some useless uses of cat.diff | (download)

t/t9300-fast-import.sh | 10 5 + 5 - 0 !
1 file changed, 5 insertions(+), 5 deletions(-)

 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>
(cherry picked from commit f94804c1f2626831c6bdf8cc269a571324e3f2f2)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0002 t9300 create marks files for double import marks test.diff | (download)

t/t9300-fast-import.sh | 2 2 + 0 - 0 !
1 file changed, 2 insertions(+)

 t9300: create marks files for double-import-marks test

Our tests confirm that providing two "import-marks" options in a
fast-import stream is an error. However, the invoked command would fail
even without covering this case, because the marks files themselves do
not actually exist.  Let's create the files to make sure we fail for the
right reason (we actually do, because the option parsing happens before
we open anything, but this future-proofs our test).

Signed-off-by: Jeff King <peff@peff.net>
(cherry picked from commit 816f806786e12435163c591942a204c5a3bdd795)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0003 fast import tighten parsing of boolean command line o.diff | (download)

fast-import.c | 4 2 + 2 - 0 !
1 file changed, 2 insertions(+), 2 deletions(-)

 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>
(cherry picked from commit 11e934d56e46875b24d8a047d44b45ff243f6715)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0004 fast import stop creating leading directories for imp.diff | (download)

fast-import.c | 1 0 + 1 - 0 !
1 file changed, 1 deletion(-)

 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>
(cherry picked from commit e075dba3723875f478654068609f69b2a5af8566)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0005 fast import delay creating leading directories for ex.diff | (download)

fast-import.c | 7 6 + 1 - 0 !
t/t9300-fast-import.sh | 13 11 + 2 - 0 !
2 files changed, 17 insertions(+), 3 deletions(-)

 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>
(cherry picked from commit 019683025f1b14d7cb671312ab01f7330e9b33e7)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0006 fast import disallow feature export marks by default.diff | (download)

Documentation/git-fast-import.txt | 14 14 + 0 - 0 !
fast-import.c | 25 25 + 0 - 0 !
t/t9300-fast-import.sh | 23 15 + 8 - 0 !
transport-helper.c | 1 1 + 0 - 0 !
4 files changed, 55 insertions(+), 8 deletions(-)

 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>
(cherry picked from commit 68061e3470210703cb15594194718d35094afdc0)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0007 fast import disallow feature import marks by default.diff | (download)

Documentation/git-fast-import.txt | 3 2 + 1 - 0 !
fast-import.c | 2 2 + 0 - 0 !
t/t9300-fast-import.sh | 22 17 + 5 - 0 !
3 files changed, 21 insertions(+), 6 deletions(-)

 fast-import: disallow "feature import-marks" by default

As with export-marks in the previous commit, import-marks can access the
filesystem. This is significantly less dangerous than export-marks
because it only involves reading from arbitrary paths, rather than
writing them. However, it could still be surprising and have security
implications (e.g., exfiltrating data from a service that accepts
fast-import streams).

Let's lump it (and its "if-exists" counterpart) in with export-marks,
and enable the in-stream version only if --allow-unsafe-features is set.

Signed-off-by: Jeff King <peff@peff.net>
(cherry picked from commit a52ed76142f6e8d993bb4c50938a408966eb2b7c)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0008 clone recurse submodules prevent name squatting on Wi.diff | (download)

builtin/clone.c | 2 1 + 1 - 0 !
builtin/submodule--helper.c | 14 12 + 2 - 0 !
git-submodule.sh | 6 6 + 0 - 0 !
t/t7415-submodule-names.sh | 31 31 + 0 - 0 !
4 files changed, 50 insertions(+), 3 deletions(-)

 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
0009 mingw disallow backslash characters in tree objects f.diff | (download)

t/t1450-fsck.sh | 1 1 + 0 - 0 !
t/t7415-submodule-names.sh | 8 5 + 3 - 0 !
t/t9350-fast-export.sh | 1 1 + 0 - 0 !
tree-walk.c | 6 6 + 0 - 0 !
4 files changed, 13 insertions(+), 3 deletions(-)

 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>
(cherry picked from commit e1d911dd4c7b76a5a8cec0f5c8de15981e34da83)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0010 path.c document the purpose of is_ntfs_dotgit.diff | (download)

path.c | 28 28 + 0 - 0 !
1 file changed, 28 insertions(+)

 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>
(cherry picked from commit 525e7fba7854c23ee3530d0bf88d75f106f14c95)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0011 test path utils offer to run a protectNTFS protectHFS.diff | (download)

t/helper/test-path-utils.c | 96 96 + 0 - 0 !
1 file changed, 96 insertions(+)

 test-path-utils: offer to run a protectntfs/protecthfs benchmark

In preparation to flipping the default on `core.protectNTFS`, let's have
some way to measure the speed impact of this config setting reliably
(and for comparison, the `core.protectHFS` config setting).

For now, this is a manual performance benchmark:

	./t/helper/test-path-utils protect_ntfs_hfs [arguments...]

where the arguments are an optional number of file names to test with,
optionally followed by minimum and maximum length of the random file
names. The default values are one million, 3 and 20, respectively.

Just like `sqrti()` in `bisect.c`, we introduce a very simple function
to approximation the square root of a given value, in order to avoid
having to introduce the first user of `<math.h>` in Git's source code.

Note: this is _not_ implemented as a Unix shell script in t/perf/
because we really care about _very_ precise timings here, and Unix shell
scripts are simply unsuited for precise and consistent benchmarking.

Signed-off-by: Garima Singh <garima.singh@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit a62f9d1ace8c6556cbc1bb7df69eff0a0bb9e774)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0012 is_ntfs_dotgit only verify the leading segment.diff | (download)

fsck.c | 18 17 + 1 - 0 !
path.c | 5 1 + 4 - 0 !
read-cache.c | 8 8 + 0 - 0 !
3 files changed, 26 insertions(+), 5 deletions(-)

 is_ntfs_dotgit(): only verify the leading segment

The config setting `core.protectNTFS` is specifically designed to work
not only on Windows, but anywhere, to allow for repositories hosted on,
say, Linux servers to be protected against NTFS-specific attack vectors.

As a consequence, `is_ntfs_dotgit()` manually splits backslash-separated
paths (but does not do the same for paths separated by forward slashes),
under the assumption that the backslash might not be a valid directory
separator on the _current_ Operating System.

However, the two callers, `verify_path()` and `fsck_tree()`, are
supposed to feed only individual path segments to the `is_ntfs_dotgit()`
function.

This causes a lot of duplicate scanning (and very inefficient scanning,
too, as the inner loop of `is_ntfs_dotgit()` was optimized for
readability rather than for speed.

Let's simplify the design of `is_ntfs_dotgit()` by putting the burden of
splitting the paths by backslashes as directory separators on the
callers of said function.

Consequently, the `verify_path()` function, which already splits the
path by directory separators, now treats backslashes as directory
separators _explicitly_ when `core.protectNTFS` is turned on, even on
platforms where the backslash is _not_ a directory separator.

Note that we have to repeat some code in `verify_path()`: if the
backslash is not a directory separator on the current Operating System,
we want to allow file names like `\`, but we _do_ want to disallow paths
that are clearly intended to cause harm when the repository is cloned on
Windows.

The `fsck_tree()` function (the other caller of `is_ntfs_dotgit()`) now
needs to look for backslashes in tree entries' names specifically when
`core.protectNTFS` is turned on. While it would be tempting to
completely disallow backslashes in that case (much like `fsck` reports
names containing forward slashes as "full paths"), this would be
overzealous: when `core.protectNTFS` is turned on in a non-Windows
setup, backslashes are perfectly valid characters in file names while we
_still_ want to disallow tree entries that are clearly designed to
exploit NTFS-specific behavior.

This simplification will make subsequent changes easier to implement,
such as turning `core.protectNTFS` on by default (not only on Windows)
or protecting against attack vectors involving NTFS Alternate Data
Streams.

Incidentally, this change allows for catching malicious repositories
that contain tree entries of the form `dir\.gitmodules` already on the
server side rather than only on the client side (and previously only on
Windows): in contrast to `is_ntfs_dotgit()`, the
`is_ntfs_dotgitmodules()` function already expects the caller to split
the paths by directory separators.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 288a74bcd28229a00c3632f18cba92dbfdf73ee9)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0013 path safeguard .git against NTFS Alternate Streams Ac.diff | (download)

path.c | 12 11 + 1 - 0 !
t/t1014-read-tree-confusing.sh | 1 1 + 0 - 0 !
2 files changed, 12 insertions(+), 1 deletion(-)

 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>
(cherry picked from commit 7c3745fc6185495d5765628b4dfe1bd2c25a2981)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0014 path also guard .gitmodules against NTFS Alternate Da.diff | (download)

path.c | 2 1 + 1 - 0 !
t/t0060-path-utils.sh | 7 6 + 1 - 0 !
2 files changed, 7 insertions(+), 2 deletions(-)

 path: also guard `.gitmodules` against ntfs alternate data streams

We just safe-guarded `.git` against NTFS Alternate Data Stream-related
attack vectors, and now it is time to do the same for `.gitmodules`.

Note: In the added regression test, we refrain from verifying all kinds
of variations between short names and NTFS Alternate Data Streams: as
the new code disallows _all_ Alternate Data Streams of `.gitmodules`, it
is enough to test one in order to know that all of them are guarded
against.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 91bd46588e6959e6903e275f78b10bd07830d547)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0015 is_ntfs_dotgit speed it up.diff | (download)

path.c | 55 30 + 25 - 0 !
1 file changed, 30 insertions(+), 25 deletions(-)

 is_ntfs_dotgit(): speed it up

Previously, this function was written without focusing on speed,
intending to make reviewing the code as easy as possible, to avoid any
bugs in this critical code.

Turns out: we can do much better on both accounts. With this patch, we
make it as fast as this developer can make it go:

- We avoid the call to `is_dir_sep()` and make all the character
  comparisons explicit.

- We avoid the cost of calling `strncasecmp()` and unroll the test for
  `.git` and `git~1`, not even using `tolower()` because it is faster to
  compare against two constant values.

- We look for `.git` and `.git~1` first thing, and return early if not
  found.

- We also avoid calling a separate function for detecting chains of
  spaces and periods.

Each of these improvements has a noticeable impact on the speed of
`is_ntfs_dotgit()`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 3a85dc7d534fc2d410ddc0c771c963b20d1b4857)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0016 protect_ntfs turn on NTFS protection by default.diff | (download)

config.mak.uname | 3 1 + 2 - 0 !
environment.c | 2 1 + 1 - 0 !
2 files changed, 2 insertions(+), 3 deletions(-)

 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>
(cherry picked from commit 9102f958ee5254b10c0be72672aa3305bf4f4704)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0017 Disallow dubiously nested submodule git directories.diff | (download)

builtin/submodule--helper.c | 4 4 + 0 - 0 !
submodule.c | 49 47 + 2 - 0 !
submodule.h | 5 5 + 0 - 0 !
t/t7415-submodule-names.sh | 23 23 + 0 - 0 !
4 files changed, 79 insertions(+), 2 deletions(-)

 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>
(cherry picked from commit a8dee3ca610f5a1d403634492136c887f83b59d2)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0018 mingw fix quoting of arguments.diff | (download)

compat/mingw.c | 9 6 + 3 - 0 !
t/t7416-submodule-dash-url.sh | 14 14 + 0 - 0 !
2 files changed, 20 insertions(+), 3 deletions(-)

 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>
(cherry picked from commit 6d8684161ee9c03bed5cb69ae76dfdddb85a0003)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0019 tests add a helper to stress test argument quoting.diff | (download)

t/helper/test-run-command.c | 118 116 + 2 - 0 !
1 file changed, 116 insertions(+), 2 deletions(-)

 tests: add a helper to stress test argument quoting

On Windows, we have to do all the command-line argument quoting
ourselves. Worse: we have to have two versions of said quoting, one for
MSYS2 programs (which have their own dequoting rules) and the rest.

We care mostly about the rest, and to make sure that that works, let's
have a stress test that comes up with all kinds of awkward arguments,
verifying that a spawned sub-process receives those unharmed.

Signed-off-by: Garima Singh <garima.singh@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit ad1559252945179e28fba7d693494051352810c5)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0020 quote stress test accept arguments to test via the co.diff | (download)

t/helper/test-run-command.c | 31 20 + 11 - 0 !
1 file changed, 20 insertions(+), 11 deletions(-)

 quote-stress-test: accept arguments to test via the command-line

When the stress test reported a problem with quoting certain arguments,
it is helpful to have a facility to play with those arguments in order
to find out whether variations of those arguments are affected, too.

Let's allow `test-run-command quote-stress-test -- <args>` to be used
for that purpose.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 55953c77c0bfcb727ffd7e293e4661b7a24b791b)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0021 quote stress test allow skipping some trials.diff | (download)

t/helper/test-run-command.c | 6 5 + 1 - 0 !
1 file changed, 5 insertions(+), 1 deletion(-)

 quote-stress-test: allow skipping some trials

When the, say, 93rd trial run fails, it is a good idea to have a way to
skip the first 92 trials and dig directly into the 93rd in a debugger.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 7530a6287e20a74b9fe6d4ca3a66df0f0f5cc52c)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0022 quote stress test offer to test quoting arguments for.diff | (download)

t/helper/test-run-command.c | 13 10 + 3 - 0 !
1 file changed, 10 insertions(+), 3 deletions(-)

 quote-stress-test: offer to test quoting arguments for msys2 sh

0023 t6130 t9350 prepare for stringent Win32 path validati.diff | (download)

t/t6130-pathspec-noglob.sh | 1 1 + 0 - 0 !
t/t9350-fast-export.sh | 2 1 + 1 - 0 !
2 files changed, 2 insertions(+), 1 deletion(-)

 t6130/t9350: prepare for stringent win32 path validation

On Windows, file names cannot contain asterisks nor newline characters.
In an upcoming commit, we will make this limitation explicit,
disallowing even the creation of commits that introduce such file names.

However, in the test scripts touched by this patch, we _know_ that those
paths won't be checked out, so we _want_ to allow such file names.

Happily, the stringent path validation will be guarded via the
`core.protectNTFS` flag, so all we need to do is to force that flag off
temporarily.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 35edce205615c553fdc49bcf10b0c91f061c56c9)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0024 unpack trees let merged_entry pass through do_add_ent.diff | (download)

unpack-trees.c | 3 2 + 1 - 0 !
1 file changed, 2 insertions(+), 1 deletion(-)

 unpack-trees: let merged_entry() pass through do_add_entry()'s errors

A `git clone` will end with exit code 0 when `merged_entry()` returns a
positive value during a call of `unpack_trees()` to `traverse_trees()`.
The reason is that `unpack_trees()` will interpret a positive value not
to be an error.

The problem is, however, that `add_index_entry()` (which is called by
`merged_entry()` can report an error, and we really should fail the
entire clone in such a case.

Let's fix this problem, in preparation for a Windows-specific patch
disallowing `mkdir()` with directory names that contain a trailing space
(which is illegal on NTFS): we want `git clone` to abort when a path
cannot be checked out due to that condition.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit cc756edda63769cf6d7acc99e6ad3a9cbb5dc3ec)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0025 mingw refuse to access paths with trailing spaces or .diff | (download)

compat/mingw.c | 57 56 + 1 - 0 !
compat/mingw.h | 11 11 + 0 - 0 !
git-compat-util.h | 4 4 + 0 - 0 !
read-cache.c | 3 3 + 0 - 0 !
t/helper/test-path-utils.c | 17 17 + 0 - 0 !
t/t0060-path-utils.sh | 14 14 + 0 - 0 !
t/t7415-submodule-names.sh | 2 1 + 1 - 0 !
t/t7417-submodule-path-url.sh | 17 17 + 0 - 0 !
8 files changed, 123 insertions(+), 2 deletions(-)

 mingw: refuse to access paths with trailing spaces or periods

When creating a directory on Windows whose path ends in a space or a
period (or chains thereof), the Win32 API "helpfully" trims those. For
example, `mkdir("abc ");` will return success, but actually create a
directory called `abc` instead.

This stems back to the DOS days, when all file names had exactly 8
characters plus exactly 3 characters for the file extension, and the
only way to have shorter names was by padding with spaces.

Sadly, this "helpful" behavior is a bit inconsistent: after a successful
`mkdir("abc ");`, a `mkdir("abc /def")` will actually _fail_ (because
the directory `abc ` does not actually exist).

Even if it would work, we now have a serious problem because a Git
repository could contain directories `abc` and `abc `, and on Windows,
they would be "merged" unintentionally.

As these paths are illegal on Windows, anyway, let's disallow any
accesses to such paths on that Operating System.

For practical reasons, this behavior is still guarded by the
config setting `core.protectNTFS`: it is possible (and at least two
regression tests make use of it) to create commits without involving the
worktree. In such a scenario, it is of course possible -- even on
Windows -- to create such file names.

Among other consequences, this patch disallows submodules' paths to end
in spaces on Windows (which would formerly have confused Git enough to
try to write into incorrect paths, anyway).

While this patch does not fix a vulnerability on its own, it prevents an
attack vector that was exploited in demonstrations of a number of
recently-fixed security bugs.

The regression test added to `t/t7417-submodule-path-url.sh` reflects
that attack vector.

Note that we have to adjust the test case "prevent git~1 squatting on
Windows" in `t/t7415-submodule-names.sh` because of a very subtle issue.
0026 mingw refuse to access paths with illegal characters.diff | (download)

compat/mingw.c | 10 10 + 0 - 0 !
compat/mingw.h | 7 5 + 2 - 0 !
t/t0060-path-utils.sh | 4 3 + 1 - 0 !
3 files changed, 18 insertions(+), 3 deletions(-)

 mingw: refuse to access paths with illegal characters

Certain characters are not admissible in file names on Windows, even if
Cygwin/MSYS2 (and therefore, Git for Windows' Bash) pretend that they
are, e.g. `:`, `<`, `>`, etc

Let's disallow those characters explicitly in Windows builds of Git.

Note: just like trailing spaces or periods, it _is_ possible on Windows
to create commits adding files with such illegal characters, as long as
the operation leaves the worktree untouched. To allow for that, we
continue to guard `is_valid_win32_path()` behind the config setting
`core.protectNTFS`, so that users _can_ continue to do that, as long as
they turn the protections off via that config setting.

Among other problems, this prevents Git from trying to write to an "NTFS
Alternate Data Stream" (which refers to metadata stored alongside a
file, under a special name: "<filename>:<stream-name>"). This fix
therefore also prevents an attack vector that was exploited in
demonstrations of a number of recently-fixed security bugs.

Further reading on illegal characters in Win32 filenames:
https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 817ddd64c20b29b2d86b3a0589f7ff88d1279109)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0027 mingw handle subst ed DOS drives.diff | (download)

compat/mingw.c | 24 24 + 0 - 0 !
compat/mingw.h | 4 2 + 2 - 0 !
connect.c | 2 1 + 1 - 0 !
t/t0060-path-utils.sh | 9 9 + 0 - 0 !
4 files changed, 36 insertions(+), 3 deletions(-)

 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>
(cherry picked from commit f82a97eb9197c1e3768e72648f37ce0ca3233734)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0028 Git 2.14.6.diff | (download)

Documentation/RelNotes/2.14.6.txt | 54 54 + 0 - 0 !
1 file changed, 54 insertions(+)

 git 2.14.6

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 66d2a6159f511924e7e0b8a21c93538879bfd622)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0029 submodule reject submodule.update command in .gitmodu.diff | (download)

Documentation/gitmodules.txt | 5 2 + 3 - 0 !
submodule-config.c | 12 10 + 2 - 0 !
t/t7406-submodule-update.sh | 14 8 + 6 - 0 !
3 files changed, 20 insertions(+), 11 deletions(-)

 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>
(cherry picked from commit e904deb89d9a9669a76a426182506a084d3f6308)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0030 Git 2.15.4.diff | (download)

Documentation/RelNotes/2.15.4.txt | 11 11 + 0 - 0 !
1 file changed, 11 insertions(+)

 git 2.15.4

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 7cdafcaacf677b9e0700fa988c247bda192db48d)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0031 test drop caches use has_dos_drive_prefix.diff | (download)

t/helper/test-drop-caches.c | 11 7 + 4 - 0 !
1 file changed, 7 insertions(+), 4 deletions(-)

 test-drop-caches: use `has_dos_drive_prefix()`

This is a companion patch to 'mingw: handle `subst`-ed "DOS drives"':
use the DOS drive prefix handling that is already provided by
`compat/mingw.c` (and which just learned to handle non-alphabetical
"drive letters").

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 68440496c77c6d3a606537c78ea4b62eb895a64a)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0032 Git 2.16.6.diff | (download)

Documentation/RelNotes/2.16.6.txt | 8 8 + 0 - 0 !
1 file changed, 8 insertions(+)

 git 2.16.6

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit eb288bc455ac67e3ceeff90daf6f25972bb586d0)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0033 fsck reject submodule.update command in .gitmodules.diff | (download)

fsck.c | 7 7 + 0 - 0 !
t/t7406-submodule-update.sh | 14 14 + 0 - 0 !
2 files changed, 21 insertions(+)

 fsck: reject submodule.update = !command in .gitmodules

This allows hosting providers to detect whether they are being used
to attack users using malicious 'update = !command' settings in
.gitmodules.

Since ac1fbbda2013 (submodule: do not copy unknown update mode from
.gitmodules, 2013-12-02), in normal cases such settings have been
treated as 'update = none', so forbidding them should not produce any
collateral damage to legitimate uses.  A quick search does not reveal
any repositories making use of this construct, either.

Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit bb92255ebe6bccd76227e023d6d0bc997e318ad0)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0034 Git 2.17.3.diff | (download)

Documentation/RelNotes/2.17.3.txt | 12 12 + 0 - 0 !
1 file changed, 12 insertions(+)

 git 2.17.3

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit a5ab8d03173458b76b8452efd90a7173f490c132)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0035 Git 2.18.2.diff | (download)

Documentation/RelNotes/2.18.2.txt | 8 8 + 0 - 0 !
1 file changed, 8 insertions(+)

 git 2.18.2

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 9877106b01cbd346b862cc8cd2c52e496dd40ed5)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0036 Git 2.19.3.diff | (download)

Documentation/RelNotes/2.19.3.txt | 8 8 + 0 - 0 !
1 file changed, 8 insertions(+)

 git 2.19.3

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit caccc527ca7f4b3e6f4bb6775cbff94b27741482)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0037 t7415 adjust test for dubiously nested submodule gitd.diff | (download)

t/t7415-submodule-names.sh | 2 1 + 1 - 0 !
1 file changed, 1 insertion(+), 1 deletion(-)

 t7415: adjust test for dubiously-nested submodule gitdirs for v2.20.x

In v2.20.x, Git clones submodules recursively by first creating the
submodules' gitdirs and _then_ "updating" the submodules. This can lead
to the situation where the clone path is taken because the directory
(while it exists already) is not a git directory, but then the clone
fails because that gitdir is unexpectedly already a directory.

This _also_ works around the vulnerability that was fixed in "Disallow
0038 submodule defend against submodule.update command in .diff | (download)

builtin/submodule--helper.c | 2 2 + 0 - 0 !
1 file changed, 2 insertions(+)

 submodule: defend against submodule.update = !command in .gitmodules

In v2.15.4, we started to reject `submodule.update` settings in
`.gitmodules`. Let's raise a BUG if it somehow still made it through
from anywhere but the Git config.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
(cherry picked from commit c1547450748fcbac21675f2681506d2d80351a19)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0039 Git 2.20.2.diff | (download)

Documentation/RelNotes/2.20.2.txt | 18 18 + 0 - 0 !
GIT-VERSION-GEN | 2 1 + 1 - 0 !
2 files changed, 19 insertions(+), 1 deletion(-)

 git 2.20.2

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
(cherry picked from commit 4cd1cf31efed9b16db5035c377bfa222f5272458)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0040 credential avoid writing values with newlines.diff | (download)

credential.c | 2 2 + 0 - 0 !
t/t0300-credentials.sh | 6 6 + 0 - 0 !
2 files changed, 8 insertions(+)

 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>
(cherry picked from commit 9a6bbee8006c24b46a85d29e7b38cfa79e9ab21b)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0041 t lib credential use test_i18ncmp to check stderr.diff | (download)

t/lib-credential.sh | 2 1 + 1 - 0 !
1 file changed, 1 insertion(+), 1 deletion(-)

 t/lib-credential: use test_i18ncmp to check stderr

The credential tests have a "check" function which feeds some input to
git-credential and checks the stdout and stderr. We look for exact
matches in the output. For stdout, this makes sense; the output is
the credential protocol. But for stderr, we may be showing various
diagnostic messages, or the prompts fed to the askpass program, which
could be translated. Let's mark them as such.

(cherry picked from commit 17f1c0b8c7e447aa62f85dc355bb48133d2812f2)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0042 credential detect unrepresentable values when parsing.diff | (download)

credential.c | 36 34 + 2 - 0 !
credential.h | 16 16 + 0 - 0 !
t/t0300-credentials.sh | 12 10 + 2 - 0 !
3 files changed, 60 insertions(+), 4 deletions(-)

 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.

(cherry picked from commit c716fe4bd917e013bf376a678b3a924447777b2d)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0043 fsck detect gitmodules URLs with embedded newlines.diff | (download)

fsck.c | 16 15 + 1 - 0 !
t/t7416-submodule-dash-url.sh | 18 17 + 1 - 0 !
2 files changed, 32 insertions(+), 2 deletions(-)

 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.

(cherry picked from commit 07259e74ec1237c836874342c65650bdee8a3993)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0044 Git 2.17.4.diff | (download)

Documentation/RelNotes/2.17.4.txt | 16 16 + 0 - 0 !
1 file changed, 16 insertions(+)

 git 2.17.4

Signed-off-by: Junio C Hamano <gitster@pobox.com>
(cherry picked from commit c42c0f12972194564f039dcf580d89ca14ae72d6)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0045 Git 2.18.3.diff | (download)

Documentation/RelNotes/2.18.3.txt | 5 5 + 0 - 0 !
1 file changed, 5 insertions(+)

 git 2.18.3

Signed-off-by: Junio C Hamano <gitster@pobox.com>
(cherry picked from commit 21a3e5016bb218dc9b016284c88ba685bc446b70)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0046 Git 2.19.4.diff | (download)

Documentation/RelNotes/2.19.4.txt | 5 5 + 0 - 0 !
1 file changed, 5 insertions(+)

 git 2.19.4

Signed-off-by: Junio C Hamano <gitster@pobox.com>
(cherry picked from commit a5979d7009017c79b0100b7b66e8567b3ad7b022)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0047 Git 2.20.3.diff | (download)

Documentation/RelNotes/2.20.3.txt | 5 5 + 0 - 0 !
GIT-VERSION-GEN | 2 1 + 1 - 0 !
2 files changed, 6 insertions(+), 1 deletion(-)

 git 2.20.3

Signed-off-by: Junio C Hamano <gitster@pobox.com>
(cherry picked from commit d1259ce117d0714f1441956805612fb36d7ce7f8)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0048 t0300 make quit helper more realistic.diff | (download)

t/t0300-credentials.sh | 16 13 + 3 - 0 !
1 file changed, 13 insertions(+), 3 deletions(-)

 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>
(cherry picked from commit a88dbd2f8c7fd8c1e2f63483da03bd6928e8791f)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0049 t0300 use more realistic inputs.diff | (download)

t/t0300-credentials.sh | 89 85 + 4 - 0 !
1 file changed, 85 insertions(+), 4 deletions(-)

 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>
0050 credential parse URL without host as empty host not u.diff | (download)

credential.c | 3 1 + 2 - 0 !
http.c | 1 1 + 0 - 0 !
t/t0300-credentials.sh | 17 17 + 0 - 0 !
3 files changed, 19 insertions(+), 2 deletions(-)

 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>
0051 credential refuse to operate when missing host or pro.diff | (download)

credential.c | 20 14 + 6 - 0 !
t/t0300-credentials.sh | 34 26 + 8 - 0 !
2 files changed, 40 insertions(+), 14 deletions(-)

 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>
(cherry picked from commit 8ba8ed568e2a3b75ee84c49ddffb026fde1a0a91)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0052 fsck convert gitmodules url to URL passed to curl.diff | (download)

fsck.c | 94 89 + 5 - 0 !
t/t7416-submodule-dash-url.sh | 29 29 + 0 - 0 !
2 files changed, 118 insertions(+), 5 deletions(-)

 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
0053 credential die when parsing invalid urls.diff | (download)

credential.c | 6 2 + 4 - 0 !
t/t0300-credentials.sh | 3 1 + 2 - 0 !
2 files changed, 3 insertions(+), 6 deletions(-)

 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>
(cherry picked from commit fe29a9b7b0236d3d45c254965580d6aff7fa8504)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0054 credential treat URL without scheme as invalid.diff | (download)

credential.c | 7 5 + 2 - 0 !
fsck.c | 47 45 + 2 - 0 !
t/t5550-http-fetch-dumb.sh | 7 2 + 5 - 0 !
t/t7416-submodule-dash-url.sh | 32 32 + 0 - 0 !
4 files changed, 84 insertions(+), 9 deletions(-)

 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>
0055 credential treat URL with empty scheme as invalid.diff | (download)

credential.c | 5 2 + 3 - 0 !
t/t5550-http-fetch-dumb.sh | 9 9 + 0 - 0 !
t/t7416-submodule-dash-url.sh | 32 32 + 0 - 0 !
3 files changed, 43 insertions(+), 3 deletions(-)

 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>
(cherry picked from commit e7fab62b736cca3416660636e46f0be8386a5030)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0056 fsck reject URL with empty host in .gitmodules.diff | (download)

fsck.c | 10 7 + 3 - 0 !
t/t7416-submodule-dash-url.sh | 32 32 + 0 - 0 !
2 files changed, 39 insertions(+), 3 deletions(-)

 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>
0057 Git 2.17.5.diff | (download)

Documentation/RelNotes/2.17.5.txt | 22 22 + 0 - 0 !
1 file changed, 22 insertions(+)

 git 2.17.5

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
(cherry picked from commit df5be6dc3fd18c294ec93a9af0321334e3f92c9c)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0058 Git 2.18.4.diff | (download)

Documentation/RelNotes/2.18.4.txt | 5 5 + 0 - 0 !
1 file changed, 5 insertions(+)

 git 2.18.4

This merges up the security fix from v2.17.5.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
(cherry picked from commit ba6f0905fdb9e65c1ac5fbc79c9a4ef0b59b3e68)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0059 Git 2.19.5.diff | (download)

Documentation/RelNotes/2.19.5.txt | 5 5 + 0 - 0 !
1 file changed, 5 insertions(+)

 git 2.19.5

This merges up the security fix from v2.17.5.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
(cherry picked from commit 76b54ee9b9944ee70422ac24884f78769cf264f1)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

0060 Git 2.20.4.diff | (download)

Documentation/RelNotes/2.20.4.txt | 5 5 + 0 - 0 !
GIT-VERSION-GEN | 2 1 + 1 - 0 !
2 files changed, 6 insertions(+), 1 deletion(-)

 git 2.20.4

This merges up the security fix from v2.17.5.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
(cherry picked from commit 041bc65923e13313ca1b77681c3b2950b5e0a163)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>