- 10 Dec, 2019 15 commits
-
-
Patrick Steinhardt committed
-
Windows/DOS only supports drive letters that are alpha characters A-Z. However, you can `subst` any one-character as a drive letter, including numbers or even emoji. Test that we can identify emoji as drive letters.
Edward Thomson committed -
Users may want to turn off core.protectNTFS, perhaps to import (and then repair) a broken tree. Ensure that core.protectNTFS=false is honored.
Edward Thomson committed -
Edward Thomson committed
-
Enable core.protectNTFS by default everywhere and in every codepath, not just on checkout.
Edward Thomson committed -
Test that when we enable core.protectNTFS that we cannot add platform-specific invalid paths to the index.
Edward Thomson committed -
The name of the `add_invalid_filename` function suggests that we _want_ to add an invalid filename. Rename the function to show that we expect to _fail_ to add the invalid filename.
Edward Thomson committed -
Ensure that the new protection around .git::$INDEX_ALLOCATION rules are enabled for using the treebuilder when core.protectNTFS is set.
Edward Thomson committed -
Ensure that the new protection around .git::$INDEX_ALLOCATION rules are enabled for adding to the index when core.protectNTFS is set.
Edward Thomson committed -
The name of the `write_invalid_filename` function suggests that we _want_ to write an invalid filename. Rename the function to show that we expect to _fail_ to write the invalid filename.
Edward Thomson committed -
The function `only_spaces_and_dots` used to detect the end of the filename on win32. Now we look at spaces and dots _before_ the end of the string _or_ a `:` character, which would signify a win32 alternate data stream. Thus, rename the function `ntfs_end_of_filename` to indicate that it detects the (virtual) end of a filename, that any further characters would be elided to the given path.
Edward Thomson committed -
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>
Johannes Schindelin committed -
A little-known feature of NTFS is that it offers to store metadata in so-called "Alternate Data Streams" (inspired by Apple's "resource forks") that are copied together with the file they are associated with. These Alternate Data Streams can be accessed via `<file name>:<stream name>:<stream type>`. Directories, too, have Alternate Data Streams, and they even have a default stream type `$INDEX_ALLOCATION`. Which means that `abc/` and `abc::$INDEX_ALLOCATION/` are actually equivalent. This is of course another attack vector on the Git directory that we definitely want to prevent. On Windows, we already do this incidentally, by disallowing colons in file/directory names. While it looks as if files'/directories' Alternate Data Streams are not accessible in the Windows Subsystem for Linux, and neither via CIFS/SMB-mounted network shares in Linux, it _is_ possible to access them on SMB-mounted network shares on macOS. Therefore, let's go the extra mile and prevent this particular attack _everywhere_. To keep things simple, let's just disallow *any* Alternate Data Stream of `.git`. This is libgit2's variant of CVE-2019-1352. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Johannes Schindelin committed -
The Windows Subsystem for Linux (WSL) is getting increasingly popular, in particular because it makes it _so_ easy to run Linux software on Windows' files, via the auto-mounted Windows drives (`C:\` is mapped to `/mnt/c/`, no need to set that up manually). Unfortunately, files/directories on the Windows drives can be accessed via their _short names_, if that feature is enabled (which it is on the `C:` drive by default). Which means that we have to safeguard even our Linux users against the short name attacks. Further, while the default options of CIFS/SMB-mounts seem to disallow accessing files on network shares via their short names on Linux/macOS, it _is_ possible to do so with the right options. So let's just safe-guard against short name attacks _everywhere_. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Johannes Schindelin committed -
When we expect a checkout operation to fail, but it succeeds, we actually do not want to see the error messages that were generated in the meantime for errors that were handled gracefully by the code (e.g. when an object could not be found in a pack: in this case, the next backend would have been given a chance to look up the object, and probably would have found it because the checkout succeeded, after all). Which means that in the specific case of `cl_git_fail()`, we actually want to clear the global error state _after_ evaluating the command: we know that any still-available error would be bogus, seeing as the command succeeded (unexpectedly). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Johannes Schindelin committed
-
- 05 Aug, 2019 4 commits
-
-
Edward Thomson committed
-
Edward Thomson committed
-
The function `commit_quick_parse` provides a way to quickly parse parts of a commit without storing or verifying most of its metadata. The first thing it does is calculating the number of parents by skipping "parent " lines until it finds the first non-parent line. Afterwards, this parent count is passed to `alloc_parents`, which will allocate an array to store all the parent. To calculate the amount of storage required for the parents array, `alloc_parents` simply multiplicates the number of parents with the respective elements's size. This already screams "buffer overflow", and in fact this problem is getting worse by the result being cast to an `uint32_t`. In fact, triggering this is possible: git-hash-object(1) will happily write a commit with multiple millions of parents for you. I've stopped at 67,108,864 parents as git-hash-object(1) unfortunately soaks up the complete object without streaming anything to disk and thus will cause an OOM situation at a later point. The point here is: this commit was about 4.1GB of size but compressed down to 24MB and thus easy to distribute. The above doesn't yet trigger the buffer overflow, thus. As the array's elements are all pointers which are 8 bytes on 64 bit, we need a total of 536,870,912 parents to trigger the overflow to `0`. The effect is that we're now underallocating the array and do an out-of-bound writes. As the buffer is kindly provided by the adversary, this may easily result in code execution. Extrapolating from the test file with 67m commits to the one with 536m commits results in a factor of 8. Thus the uncompressed contents would be about 32GB in size and the compressed ones 192MB. While still easily distributable via the network, only servers will have that amount of RAM and not cause an out-of-memory condition previous to triggering the overflow. This at least makes this attack not an easy vector for client-side use of libgit2.
Patrick Steinhardt committed -
When the VirtualStore feature is in effect, it is safe to let random users write into C:\ProgramData because other users won't see those files. This seemed to be the case when we introduced support for C:\ProgramData\Git\config. However, when that feature is not in effect (which seems to be the case in newer Windows 10 versions), we'd rather not use those files unless they come from a trusted source, such as an administrator. This change imitates the strategy chosen by PowerShell's native OpenSSH port to Windows regarding host key files: if a system file is owned neither by an administrator, a system account, or the current user, it is ignored.
Johannes Schindelin committed
-
- 28 Jan, 2019 2 commits
-
-
Edward Thomson committed
-
Release v0.27.8
Edward Thomson committed
-
- 25 Jan, 2019 11 commits
-
-
Edward Thomson committed
-
Edward Thomson committed
-
When checking whether a rule negates another rule, we were checking whether a rule had the `GIT_ATTR_FNMATCH_LEADINGDIR` flag set and, if so, added a "/*" to its end before passing it to `fnmatch`. Our code now sets `GIT_ATTR_FNMATCH_NOLEADINGDIR`, thus the `LEADINGDIR` flag shall never be set. Furthermore, due to the `NOLEADINGDIR` flag, trailing globs do not get consumed by our ignore parser anymore. Clean up code by just dropping this now useless logic.
Patrick Steinhardt committed -
Patrick Steinhardt committed
-
When computing whether a file is ignored, we simply search for the first matching rule and return whether it is a positive ignore rule (the file is really ignored) or whether it is a negative ignore rule (the file is being unignored). Each rule has a set of flags which are being passed to `fnmatch`, depending on what kind of rule it is. E.g. in case it is a negative ignore we add a flag `GIT_ATTR_FNMATCH_NEGATIVE`, in case it contains a glob we set the `GIT_ATTR_FNMATCH_HASGLOB` flag. One of these flags is the `GIT_ATTR_FNMATCH_LEADINGDIR` flag, which is always set in case the pattern has a trailing "/*" or in case the pattern is negative. The flag causes the `fnmatch` function to return a match in case a string is a leading directory of another, e.g. "dir/" matches "dir/foo/bar.c". In case of negative patterns, this is wrong in certain cases. Take the following simple example of a gitignore: dir/ !dir/ The `LEADINGDIR` flag causes "!dir/" to match "dir/foo/bar.c", and we correctly unignore the directory. But take this example: *.test !dir/* We expect everything in "dir/" to be unignored, but e.g. a file in a subdirectory of dir should be ignored, as the "*" does not cross directory hierarchies. With `LEADINGDIR`, though, we would just see that "dir/" matches and return that the file is unignored, even if it is contained in a subdirectory. Instead, we want to ignore leading directories here and check "*.test". Afterwards, we have to iterate up to the parent directory and do the same checks. To fix the issue, disallow matching against leading directories in gitignore files. This can be trivially done by just adding the `GIT_ATTR_FNMATCH_NOLEADINGDIR` to the spec passed to `git_attr_fnmatch__parse`. Due to a bug in that function, though, this flag is being ignored for negative patterns, which is fixed in this commit, as well. As a last fix, we need to ignore rules that are supposed to match a directory when our path itself is a file. All together, these changes fix the described error case.
Patrick Steinhardt committed -
The function `parse_number` was replaced by `git_parse_advance_digit` which is provided by the parser interface in commit 252f2eee (parse: implement and use `git_parse_advance_digit`, 2017-07-14). As there are no remaining callers, remove it.
Patrick Steinhardt committed -
When parsing a number, we accept a leading plus or minus sign to return a positive or negative number. When the parsed string has such a leading sign, we set up a flag indicating that the number is negative and advance the pointer to the next character in that string. This misses updating the number of bytes in the string, though, which is why the parser may later on do an out-of-bounds read. Fix the issue by correctly updating both the pointer and the number of remaining bytes. Furthermore, we need to check whether we actually have any bytes left after having advanced the pointer, as otherwise the auto-detection of the base may do an out-of-bonuds access. Add a test that detects the out-of-bound read. Note that this is not actually security critical. While there are a lot of places where the function is called, all of these places are guarded or irrelevant: - commit list: this operates on objects from the ODB, which are always NUL terminated any may thus not trigger the off-by-one OOB read. - config: the configuration is NUL terminated. - curl stream: user input is being parsed that is always NUL terminated - index: the index is read via `git_futils_readbuffer`, which always NUL terminates it. - loose objects: used to parse the length from the object's header. As we check previously that the buffer contains a NUL byte, this is safe. - rebase: this parses numbers from the rebase instruction sheet. As the rebase code uses `git_futils_readbuffer`, the buffer is always NUL terminated. - revparse: this parses a user provided buffer that is NUL terminated. - signature: this parser the header information of objects. As objects read from the ODB are always NUL terminated, this is a non-issue. The constructor `git_signature_from_buffer` does not accept a length parameter for the buffer, so the buffer needs to be NUL terminated, as well. - smart transport: the buffer that is parsed is NUL terminated - tree cache: this parses the tree cache from the index extension. The index itself is read via `git_futils_readbuffer`, which always NUL terminates it. - winhttp transport: user input is being parsed that is always NUL terminated
Patrick Steinhardt committed -
The `parse_mode` option uses an open-coded octal number parser. The parser is quite naive in that it simply parses until hitting a character that is not in the accepted range of '0' - '7', completely ignoring the fact that we can at most accept a 16 bit unsigned integer as filemode. If the filemode is bigger than UINT16_MAX, it will thus overflow and provide an invalid filemode for the object entry. Fix the issue by using `git__strntol32` instead and doing a bounds check. As this function already handles overflows, it neatly solves the problem. Note that previously, `parse_mode` was also skipping the character immediately after the filemode. In proper trees, this should be a simple space, but in fact the parser accepted any character and simply skipped over it. As a consequence of using `git__strntol32`, we now need to an explicit check for a trailing whitespace after having parsed the filemode. Because of the newly introduced error message, the test object::tree::parse::mode_doesnt_cause_oob_read needs adjustment to its error message check, which in fact is a good thing as it demonstrates that we now fail looking for the whitespace immediately following the filemode. Add a test that shows that we will fail to parse such invalid filemodes now.
Patrick Steinhardt committed -
The `git__strntol` family of functions has the ability to auto-detect a number's base if the string has either the common '0x' prefix for hexadecimal numbers or '0' prefix for octal numbers. The detection of such prefixes and following handling has two major issues though that are being fixed in one go now. - We do not do any bounds checking previous to verifying the '0x' base. While we do verify that there is at least one digit available previously, we fail to verify that there are two digits available and thus may do an out-of-bounds read when parsing this two-character-prefix. - When skipping the prefix of such numbers, we only update the pointer length without also updating the number of remaining bytes. Thus if we try to parse a number '0x1' of total length 3, we will first skip the first two bytes and then try to read 3 bytes starting at '1'. Fix both issues by disentangling the logic. Instead of doing the detection and skipping of such prefixes in one go, we will now first try to detect the base while also honoring how many bytes are left. Only if we have a valid base that is either 8 or 16 and have one of the known prefixes, we will now advance the pointer and update the remaining bytes in one step. Add some tests that verify that no out-of-bounds parsing happens and that autodetection works as advertised.
Patrick Steinhardt committed -
When parsing a tree entry's mode, we will eagerly parse until we hit a character that is not in the accepted set of octal digits '0' - '7'. If the provided buffer is not a NUL terminated one, we may thus read out-of-bounds. Fix the issue by passing the buffer length to `parse_mode` and paying attention to it. Note that this is not a vulnerability in our usual code paths, as all object data read from the ODB is NUL terminated.
Patrick Steinhardt committed -
The `git__strntol` family of functions accepts leading spaces and will simply skip them. The skipping will not honor the provided buffer's length, though, which may lead it to read outside of the provided buffer's bounds if it is not a simple NUL-terminated string. Furthermore, if leading space is trimmed, the function will further advance the pointer but not update the number of remaining bytes, which may also lead to out-of-bounds reads. Fix the issue by properly paying attention to the buffer length and updating it when stripping leading whitespace characters. Add a test that verifies that we won't read past the provided buffer length.
Patrick Steinhardt committed
-
- 18 Jan, 2019 8 commits
-
-
When parsing a signature's timezone offset, we first check whether there is a timezone at all by verifying that there are still bytes left to read following the time itself. The check thus looks like `time_end + 1 < buffer_end`, which is actually correct in this case. After setting the timezone's start pointer to that location, we compute the remaining bytes by using the formula `buffer_end - tz_start + 1`, re-using the previous `time_end + 1`. But this is in fact missing the braces around `(tz_start + 1)`, thus leading to an overestimation of the remaining bytes by a length of two. In case of a non-NUL terminated buffer, this will result in an overflow. The function `git_signature__parse` is only used in two locations. First is `git_signature_from_buffer`, which only accepts a string without a length. The string thus necessarily has to be NUL terminated and cannot trigger the issue. The other function is `git_commit__parse_raw`, which can in fact trigger the error as it may receive non-NUL terminated commit data. But as objects read from the ODB are always NUL-terminated by us as a cautionary measure, it cannot trigger the issue either. In other words, this error does not have any impact on security.
Patrick Steinhardt committed -
Handle two null argument cases that occur in the unit tests. One is in library code, the other is in test code. Detected by running unit tests with undefined behavior sanitizer: ```bash # build mkdir build && cd build cmake -DBUILD_CLAR=ON -DCMAKE_C_FLAGS="-fsanitize=address \ -fsanitize=undefined -fstack-usage -static-libasan" .. cmake --build . # run with asan ASAN_OPTIONS="allocator_may_return_null=1" ./libgit2_clar ... ............../libgit2/src/apply.c:316:3: runtime error: null pointer \ passed as argument 1, which is declared to never be null ...................../libgit2/tests/apply/fromfile.c:46:3: runtime \ error: null pointer passed as argument 1, which is declared to never be null ```
Noah Pendleton committed -
While commit objects usually should have only one author field, our commit parser actually handles the case where a commit has multiple author fields because some tools that exist in the wild actually write them. Detection of those additional author fields is done by using a simple `git__prefixcmp`, checking whether the current line starts with the string "author ". In case where we are handed a non-NUL-terminated string that ends directly after the space, though, we may have an out-of-bounds read of one byte when trying to compare the expected final NUL byte. Fix the issue by using `git__prefixncmp` instead of `git_prefixcmp`. Unfortunately, a test cannot be easily written to catch this case. While we could test the last error message and verify that it didn't in fact fail parsing a signature (because that would indicate that it has in fact tried to parse the additional "author " field, which it shouldn't be able to detect in the first place), this doesn't work as the next line needs to be the "committer" field, which would error out with the same error message even if we hadn't done an out-of-bounds read. As objects read from the object database are always NUL terminated, this issue cannot be triggered in normal code and thus it's not security critical.
Patrick Steinhardt committed -
When we try to add a configuration file with `git_config_add_file_ondisk`, we treat nonexisting files as empty. We do this by performing a stat call, ignoring ENOENT errors. This works just fine in case the file or any of its parents simply does not exist, but there is also the case where any of the parent directories is not a directory, but a file. So e.g. trying to add a configuration file "/dev/null/.gitconfig" will fail, as `errno` will be ENOTDIR instead of ENOENT. Catch ENOTDIR in addition to ENOENT to fix the issue. Add a test that verifies we are able to add configuration files with such an invalid path file just fine.
Patrick Steinhardt committed -
Joe Rabinoff committed
-
Joe Rabinoff committed
-
If the routine exits with error before stream or hash_ctx is initialized, the program will segfault when trying to free them.
Joe Rabinoff committed -
When git_filter_apply_fn callback returns a error while smudging proxy_stream_close ends up returning without closing the stream. This is turn makes blob_content_to_file crash as it asserts the stream being closed whether there are errors or not. Closing the target stream on error fixes this problem.
Anders Borum committed
-