summaryrefslogtreecommitdiff
AgeCommit message (Collapse)AuthorFilesLines
2019-12-05is_ntfs_dotgit(): only verify the leading segmentLibravatar Johannes Schindelin3-5/+19
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>
2019-12-05test-path-utils: offer to run a protectNTFS/protectHFS benchmarkLibravatar Garima Singh1-0/+96
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>
2019-12-04path.c: document the purpose of `is_ntfs_dotgit()`Libravatar Johannes Schindelin1-0/+28
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>
2019-12-04mingw: disallow backslash characters in tree objects' file namesLibravatar Johannes Schindelin4-3/+13
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>
2019-12-04clone --recurse-submodules: prevent name squatting on WindowsLibravatar Johannes Schindelin4-2/+50
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>
2019-12-04fast-import: disallow "feature import-marks" by defaultLibravatar Jeff King3-6/+21
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>
2019-12-04fast-import: disallow "feature export-marks" by defaultLibravatar Jeff King4-8/+55
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>
2019-12-04fast-import: delay creating leading directories for export-marksLibravatar Jeff King2-3/+17
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>
2019-12-04fast-import: stop creating leading directories for import-marksLibravatar Jeff King1-1/+0
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>
2019-12-04fast-import: tighten parsing of boolean command line optionsLibravatar Jeff King1-2/+2
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>
2019-12-04t9300: create marks files for double-import-marks testLibravatar Jeff King1-0/+2
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>
2019-12-04t9300: drop some useless uses of catLibravatar Jeff King1-5/+5
These waste a process, and make the line longer than it needs to be. Signed-off-by: Jeff King <peff@peff.net>
2018-09-27Git 2.14.5Libravatar Junio C Hamano3-2/+18
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-09-27submodule-config: ban submodule paths that start with a dashLibravatar Jeff King2-0/+22
We recently banned submodule urls that look like command-line options. This is the matching change to ban leading-dash paths. As with the urls, this should not break any use cases that currently work. Even with our "--" separator passed to git-clone, git-submodule.sh gets confused. Without the code portion of this patch, the clone of "-sub" added in t7417 would yield results like: /path/to/git-submodule: 410: cd: Illegal option -s /path/to/git-submodule: 417: cd: Illegal option -s /path/to/git-submodule: 410: cd: Illegal option -s /path/to/git-submodule: 417: cd: Illegal option -s Fetched in submodule path '-sub', but it did not contain b56243f8f4eb91b2f1f8109452e659f14dd3fbe4. Direct fetching of that commit failed. Moreover, naively adding such a submodule doesn't work: $ git submodule add $url -sub The following path is ignored by one of your .gitignore files: -sub even though there is no such ignore pattern (the test script hacks around this with a well-placed "git mv"). Unlike leading-dash urls, though, it's possible that such a path _could_ be useful if we eventually made it work. So this commit should be seen not as recommending a particular policy, but rather temporarily closing off a broken and possibly dangerous code-path. We may revisit this decision later. There are two minor differences to the tests in t7416 (that covered urls): 1. We don't have a "./-sub" escape hatch to make this work, since the submodule code expects to be able to match canonical index names to the path field (so you are free to add submodule config with that path, but we would never actually use it, since an index entry would never start with "./"). 2. After this patch, cloning actually succeeds. Since we ignore the submodule.*.path value, we fail to find a config stanza for our submodule at all, and simply treat it as inactive. We still check for the "ignoring" message. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-09-27submodule-config: ban submodule urls that start with dashLibravatar Jeff King2-0/+42
The previous commit taught the submodule code to invoke our "git clone $url $path" with a "--" separator so that we aren't confused by urls or paths that start with dashes. However, that's just one code path. It's not clear if there are others, and it would be an easy mistake to add one in the future. Moreover, even with the fix in the previous commit, it's quite hard to actually do anything useful with such an entry. Any url starting with a dash must fall into one of three categories: - it's meant as a file url, like "-path". But then any clone is not going to have the matching path, since it's by definition relative inside the newly created clone. If you spell it as "./-path", the submodule code sees the "/" and translates this to an absolute path, so it at least works (assuming the receiver has the same filesystem layout as you). But that trick does not apply for a bare "-path". - it's meant as an ssh url, like "-host:path". But this already doesn't work, as we explicitly disallow ssh hostnames that begin with a dash (to avoid option injection against ssh). - it's a remote-helper scheme, like "-scheme::data". This _could_ work if the receiver bends over backwards and creates a funny-named helper like "git-remote--scheme". But normally there would not be any helper that matches. Since such a url does not work today and is not likely to do anything useful in the future, let's simply disallow them entirely. That protects the existing "git clone" path (in a belt-and-suspenders way), along with any others that might exist. Our tests cover two cases: 1. A file url with "./" continues to work, showing that there's an escape hatch for people with truly silly repo names. 2. A url starting with "-" is rejected. Note that we expect case (2) to fail, but it would have done so even without this commit, for the reasons given above. So instead of just expecting failure, let's also check for the magic word "ignoring" on stderr. That lets us know that we failed for the right reason. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-09-27submodule--helper: use "--" to signal end of clone optionsLibravatar Jeff King1-0/+1
When we clone a submodule, we call "git clone $url $path". But there's nothing to say that those components can't begin with a dash themselves, confusing git-clone into thinking they're options. Let's pass "--" to make it clear what we expect. There's no test here, because it's actually quite hard to make these names work, even with "git clone" parsing them correctly. And we're going to restrict these cases even further in future commits. So we'll leave off testing until then; this is just the minimal fix to prevent us from doing something stupid with a badly formed entry. Reported-by: joernchen <joernchen@phenoelit.de> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-22Git 2.14.4Libravatar Junio C Hamano3-2/+7
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-22Sync with Git 2.13.7Libravatar Junio C Hamano16-41/+492
* maint-2.13: Git 2.13.7 verify_path: disallow symlinks in .gitmodules update-index: stat updated files earlier verify_dotfile: mention case-insensitivity in comment verify_path: drop clever fallthrough skip_prefix: add case-insensitive variant is_{hfs,ntfs}_dotgitmodules: add tests is_ntfs_dotgit: match other .git files is_hfs_dotgit: match other .git files is_ntfs_dotgit: use a size_t for traversing string submodule-config: verify submodule names as paths
2018-05-22Git 2.13.7Libravatar Junio C Hamano3-2/+22
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-22Merge branch 'jk/submodule-fix-loose' into maint-2.13Libravatar Junio C Hamano15-41/+472
* jk/submodule-fix-loose: verify_path: disallow symlinks in .gitmodules update-index: stat updated files earlier verify_dotfile: mention case-insensitivity in comment verify_path: drop clever fallthrough skip_prefix: add case-insensitive variant is_{hfs,ntfs}_dotgitmodules: add tests is_ntfs_dotgit: match other .git files is_hfs_dotgit: match other .git files is_ntfs_dotgit: use a size_t for traversing string submodule-config: verify submodule names as paths
2018-05-21verify_path: disallow symlinks in .gitmodulesLibravatar Jeff King4-15/+37
There are a few reasons it's not a good idea to make .gitmodules a symlink, including: 1. It won't be portable to systems without symlinks. 2. It may behave inconsistently, since Git may look at this file in the index or a tree without bothering to resolve any symbolic links. We don't do this _yet_, but the config infrastructure is there and it's planned for the future. With some clever code, we could make (2) work. And some people may not care about (1) if they only work on one platform. But there are a few security reasons to simply disallow it: a. A symlinked .gitmodules file may circumvent any fsck checks of the content. b. Git may read and write from the on-disk file without sanity checking the symlink target. So for example, if you link ".gitmodules" to "../oops" and run "git submodule add", we'll write to the file "oops" outside the repository. Again, both of those are problems that _could_ be solved with sufficient code, but given the complications in (1) and (2), we're better off just outlawing it explicitly. Note the slightly tricky call to verify_path() in update-index's update_one(). There we may not have a mode if we're not updating from the filesystem (e.g., we might just be removing the file). Passing "0" as the mode there works fine; since it's not a symlink, we'll just skip the extra checks. Signed-off-by: Jeff King <peff@peff.net>
2018-05-21update-index: stat updated files earlierLibravatar Jeff King1-8/+17
In the update_one(), we check verify_path() on the proposed path before doing anything else. In preparation for having verify_path() look at the file mode, let's stat the file earlier, so we can check the mode accurately. This is made a bit trickier by the fact that this function only does an lstat in a few code paths (the ones that flow down through process_path()). So we can speculatively do the lstat() here and pass the results down, and just use a dummy mode for cases where we won't actually be updating the index from the filesystem. Signed-off-by: Jeff King <peff@peff.net>
2018-05-21verify_dotfile: mention case-insensitivity in commentLibravatar Jeff King1-1/+4
We're more restrictive than we need to be in matching ".GIT" on case-sensitive filesystems; let's make a note that this is intentional. Signed-off-by: Jeff King <peff@peff.net>
2018-05-21verify_path: drop clever fallthroughLibravatar Jeff King1-4/+4
We check ".git" and ".." in the same switch statement, and fall through the cases to share the end-of-component check. While this saves us a line or two, it makes modifying the function much harder. Let's just write it out. Signed-off-by: Jeff King <peff@peff.net>
2018-05-21skip_prefix: add case-insensitive variantLibravatar Jeff King1-0/+17
We have the convenient skip_prefix() helper, but if you want to do case-insensitive matching, you're stuck doing it by hand. We could add an extra parameter to the function to let callers ask for this, but the function is small and somewhat performance-critical. Let's just re-implement it for the case-insensitive version. Signed-off-by: Jeff King <peff@peff.net>
2018-05-21is_{hfs,ntfs}_dotgitmodules: add testsLibravatar Johannes Schindelin2-0/+106
This tests primarily for NTFS issues, but also adds one example of an HFS+ issue. Thanks go to Congyi Wu for coming up with the list of examples where NTFS would possibly equate the filename with `.gitmodules`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Jeff King <peff@peff.net>
2018-05-21is_ntfs_dotgit: match other .git filesLibravatar Johannes Schindelin2-1/+93
When we started to catch NTFS short names that clash with .git, we only looked for GIT~1. This is sufficient because we only ever clone into an empty directory, so .git is guaranteed to be the first subdirectory or file in that directory. However, even with a fresh clone, .gitmodules is *not* necessarily the first file to be written that would want the NTFS short name GITMOD~1: a malicious repository can add .gitmodul0000 and friends, which sorts before `.gitmodules` and is therefore checked out *first*. For that reason, we have to test not only for ~1 short names, but for others, too. It's hard to just adapt the existing checks in is_ntfs_dotgit(): since Windows 2000 (i.e., in all Windows versions still supported by Git), NTFS short names are only generated in the <prefix>~<number> form up to number 4. After that, a *different* prefix is used, calculated from the long file name using an undocumented, but stable algorithm. For example, the short name of .gitmodules would be GITMOD~1, but if it is taken, and all of ~2, ~3 and ~4 are taken, too, the short name GI7EBA~1 will be used. From there, collisions are handled by incrementing the number, shortening the prefix as needed (until ~9999999 is reached, in which case NTFS will not allow the file to be created). We'd also want to handle .gitignore and .gitattributes, which suffer from a similar problem, using the fall-back short names GI250A~1 and GI7D29~1, respectively. To accommodate for that, we could reimplement the hashing algorithm, but it is just safer and simpler to provide the known prefixes. This algorithm has been reverse-engineered and described at https://usn.pw/blog/gen/2015/06/09/filenames/, which is defunct but still available via https://web.archive.org/. These can be recomputed by running the following Perl script: -- snip -- use warnings; use strict; sub compute_short_name_hash ($) { my $checksum = 0; foreach (split('', $_[0])) { $checksum = ($checksum * 0x25 + ord($_)) & 0xffff; } $checksum = ($checksum * 314159269) & 0xffffffff; $checksum = 1 + (~$checksum & 0x7fffffff) if ($checksum & 0x80000000); $checksum -= (($checksum * 1152921497) >> 60) * 1000000007; return scalar reverse sprintf("%x", $checksum & 0xffff); } print compute_short_name_hash($ARGV[0]); -- snap -- E.g., running that with the argument ".gitignore" will result in "250a" (which then becomes "gi250a" in the code). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Jeff King <peff@peff.net>
2018-05-21is_hfs_dotgit: match other .git filesLibravatar Jeff King2-12/+51
Both verify_path() and fsck match ".git", ".GIT", and other variants specific to HFS+. Let's allow matching other special files like ".gitmodules", which we'll later use to enforce extra restrictions via verify_path() and fsck. Signed-off-by: Jeff King <peff@peff.net>
2018-05-21is_ntfs_dotgit: use a size_t for traversing stringLibravatar Jeff King1-1/+1
We walk through the "name" string using an int, which can wrap to a negative value and cause us to read random memory before our array (e.g., by creating a tree with a name >2GB, since "int" is still 32 bits even on most 64-bit platforms). Worse, this is easy to trigger during the fsck_tree() check, which is supposed to be protecting us from malicious garbage. Note one bit of trickiness in the existing code: we sometimes assign -1 to "len" at the end of the loop, and then rely on the "len++" in the for-loop's increment to take it back to 0. This is still legal with a size_t, since assigning -1 will turn into SIZE_MAX, which then wraps around to 0 on increment. Signed-off-by: Jeff King <peff@peff.net>
2018-05-21submodule-config: verify submodule names as pathsLibravatar Jeff King5-0/+143
Submodule "names" come from the untrusted .gitmodules file, but we blindly append them to $GIT_DIR/modules to create our on-disk repo paths. This means you can do bad things by putting "../" into the name (among other things). Let's sanity-check these names to avoid building a path that can be exploited. There are two main decisions: 1. What should the allowed syntax be? It's tempting to reuse verify_path(), since submodule names typically come from in-repo paths. But there are two reasons not to: a. It's technically more strict than what we need, as we really care only about breaking out of the $GIT_DIR/modules/ hierarchy. E.g., having a submodule named "foo/.git" isn't actually dangerous, and it's possible that somebody has manually given such a funny name. b. Since we'll eventually use this checking logic in fsck to prevent downstream repositories, it should be consistent across platforms. Because verify_path() relies on is_dir_sep(), it wouldn't block "foo\..\bar" on a non-Windows machine. 2. Where should we enforce it? These days most of the .gitmodules reads go through submodule-config.c, so I've put it there in the reading step. That should cover all of the C code. We also construct the name for "git submodule add" inside the git-submodule.sh script. This is probably not a big deal for security since the name is coming from the user anyway, but it would be polite to remind them if the name they pick is invalid (and we need to expose the name-checker to the shell anyway for our test scripts). This patch issues a warning when reading .gitmodules and just ignores the related config entry completely. This will generally end up producing a sensible error, as it works the same as a .gitmodules file which is missing a submodule entry (so "submodule update" will barf, but "git clone --recurse-submodules" will print an error but not abort the clone. There is one minor oddity, which is that we print the warning once per malformed config key (since that's how the config subsystem gives us the entries). So in the new test, for example, the user would see three warnings. That's OK, since the intent is that this case should never come up outside of malicious repositories (and then it might even benefit the user to see the message multiple times). Credit for finding this vulnerability and the proof of concept from which the test script was adapted goes to Etienne Stalmans. Signed-off-by: Jeff King <peff@peff.net>
2017-10-23Git 2.14.3Libravatar Junio C Hamano2-1/+25
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-10-23Merge branch 'jk/info-alternates-fix' into maintLibravatar Junio C Hamano1-20/+11
A regression fix for 2.11 that made the code to read the list of alternate object stores overrun the end of the string. * jk/info-alternates-fix: read_info_alternates: warn on non-trivial errors read_info_alternates: read contents into strbuf
2017-10-23Merge branch 'jc/fetch-refspec-doc-update' into maintLibravatar Junio C Hamano1-2/+4
"git fetch <there> <src>:<dst>" allows an object name on the <src> side when the other side accepts such a request since Git v2.5, but the documentation was left stale. * jc/fetch-refspec-doc-update: fetch doc: src side of refspec could be full SHA-1
2017-10-23Merge branch 'jk/write-in-full-fix' into maintLibravatar Junio C Hamano22-67/+65
Many codepaths did not diagnose write failures correctly when disks go full, due to their misuse of write_in_full() helper function, which have been corrected. * jk/write-in-full-fix: read_pack_header: handle signed/unsigned comparison in read result config: flip return value of store_write_*() notes-merge: use ssize_t for write_in_full() return value pkt-line: check write_in_full() errors against "< 0" convert less-trivial versions of "write_in_full() != len" avoid "write_in_full(fd, buf, len) != len" pattern get-tar-commit-id: check write_in_full() return against 0 config: avoid "write_in_full(fd, buf, len) < len" pattern
2017-10-23Merge branch 'rj/no-sign-compare' into maintLibravatar Junio C Hamano10-24/+25
Many codepaths have been updated to squelch -Wsign-compare warnings. * rj/no-sign-compare: ALLOC_GROW: avoid -Wsign-compare warnings cache.h: hex2chr() - avoid -Wsign-compare warnings commit-slab.h: avoid -Wsign-compare warnings git-compat-util.h: xsize_t() - avoid -Wsign-compare warnings
2017-10-23Merge branch 'ma/ts-cleanups' into maintLibravatar Junio C Hamano6-3/+37
Assorted bugfixes and clean-ups. * ma/ts-cleanups: ThreadSanitizer: add suppressions strbuf_setlen: don't write to strbuf_slopbuf pack-objects: take lock before accessing `remaining` convert: always initialize attr_action in convert_attrs
2017-10-23Merge branch 'ls/travis-scriptify' into maintLibravatar Junio C Hamano10-81/+154
The scripts to drive TravisCI has been reorganized and then an optimization to avoid spending cycles on a branch whose tip is tagged has been implemented. * ls/travis-scriptify: travis-ci: fix "skip_branch_tip_with_tag()" string comparison travis: dedent a few scripts that are indented overly deeply travis-ci: skip a branch build if equal tag is present travis-ci: move Travis CI code into dedicated scripts
2017-10-23Merge branch 'er/fast-import-dump-refs-on-checkpoint' into maintLibravatar Junio C Hamano2-3/+145
The checkpoint command "git fast-import" did not flush updates to refs and marks unless at least one object was created since the last checkpoint, which has been corrected, as these things can happen without any new object getting created. * er/fast-import-dump-refs-on-checkpoint: fast-import: checkpoint: dump branches/tags/marks even if object_count==0
2017-10-23Merge branch 'jt/fast-export-copy-modify-fix' into maintLibravatar Junio C Hamano2-15/+51
"git fast-export" with -M/-C option issued "copy" instruction on a path that is simultaneously modified, which was incorrect. * jt/fast-export-copy-modify-fix: fast-export: do not copy from modified file
2017-10-23Merge branch 'nd/worktree-kill-parse-ref' into maintLibravatar Junio C Hamano3-3/+17
"git branch -M a b" while on a branch that is completely unrelated to either branch a or branch b misbehaved when multiple worktree was in use. This has been fixed. * nd/worktree-kill-parse-ref: branch: fix branch renaming not updating HEADs correctly
2017-10-18Prepare for 2.14.3Libravatar Junio C Hamano2-1/+76
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-10-18Merge branch 'jk/ref-filter-colors-fix' into maintLibravatar Junio C Hamano23-71/+103
This is the "theoretically more correct" approach of simply stepping back to the state before plumbing commands started paying attention to "color.ui" configuration variable. * jk/ref-filter-colors-fix: tag: respect color.ui config Revert "color: check color.ui in git_default_config()" Revert "t6006: drop "always" color config tests" Revert "color: make "always" the same as "auto" in config" color: make "always" the same as "auto" in config provide --color option for all ref-filter users t3205: use --color instead of color.branch=always t3203: drop "always" color test t6006: drop "always" color config tests t7502: use diff.noprefix for --verbose test t7508: use test_terminal for color output t3701: use test-terminal to collect color output t4015: prefer --color to -c color.diff=always test-terminal: set TERM=vt100
2017-10-18Merge branch 'jc/doc-checkout' into maintLibravatar Junio C Hamano1-14/+16
Doc update. * jc/doc-checkout: checkout doc: clarify command line args for "checkout paths" mode
2017-10-18Merge branch 'tb/complete-describe' into maintLibravatar Junio C Hamano1-1/+1
Docfix. * tb/complete-describe: completion: add --broken and --dirty to describe
2017-10-18Merge branch 'rs/rs-mailmap' into maintLibravatar Junio C Hamano1-0/+1
* rs/rs-mailmap: .mailmap: normalize name for René Scharfe
2017-10-18Merge branch 'rs/fsck-null-return-from-lookup' into maintLibravatar Junio C Hamano2-4/+26
Improve behaviour of "git fsck" upon finding a missing object. * rs/fsck-null-return-from-lookup: fsck: handle NULL return of lookup_blob() and lookup_tree()
2017-10-18Merge branch 'jk/sha1-loose-object-info-fix' into maintLibravatar Junio C Hamano1-2/+6
Leakfix and futureproofing. * jk/sha1-loose-object-info-fix: sha1_loose_object_info: handle errors from unpack_sha1_rest
2017-10-18Merge branch 'sb/branch-avoid-repeated-strbuf-release' into maintLibravatar Junio C Hamano1-2/+3
* sb/branch-avoid-repeated-strbuf-release: branch: reset instead of release a strbuf
2017-10-18Merge branch 'rs/qsort-s' into maintLibravatar Junio C Hamano1-1/+1
* rs/qsort-s: test-stringlist: avoid buffer underrun when sorting nothing
2017-10-18Merge branch 'jn/strbuf-doc-re-reuse' into maintLibravatar Junio C Hamano1-2/+9
* jn/strbuf-doc-re-reuse: strbuf doc: reuse after strbuf_release is fine