Vivek and I discovered a couple of security bugs in the git index handling mechanism in libgit2, an year back.

There were two CVEs assigned for the two issues discovered.

Following is the mail I originally sent out to libgit2 maintainers for reporting the vulnerabilities.

Double-Free⌗

In read_entry() function, git_decode_varint() can be made to fail with it returning varint_len as 0.

if (varint_len == 0)
    return index_error_invalid("incorrect prefix length");

This would make read_entry() return to the caller parse_index() with return value as -1. However, the caller only checks if entry_size == 0, for detecting error in callee. Since, the callee returns -1, caller assumes it is a valid entry_size as 0xffffffff (casting to size_t) and proceeds with the execution. Let us assume that the previous call to read_entry() in previous iteration returned success (can be easily done by having one valid index entry in the index file). Now, &entry will be pointing to the address of the previously read index entry. The caller just assumes that it is the newly read entry and will add it to the map.

So, on triggering a failure while reading an index entry, we can make the caller believe that it succeeded and add the same index entry pointer twice in the map. While freeing the index structure, it will result in a double-free as the same pointer is now freed twice in the map.

In addition to failing by varint_len == 0, this could also be triggered by making the following macros fail in read_entry(). All these macros end up making read_entry() return -1, which the caller assumes as success. This is completely exploitable as the attacker can control the values passed to these macros, which can make them fail.

    GITERR_CHECK_ALLOC_ADD(&path_len, prefix_len, suffix_len);
    GITERR_CHECK_ALLOC_ADD(&path_len, path_len, 1);
    GITERR_CHECK_ALLOC(tmp_path);

So, an attacker will be able to cause a double-free and also a DoS.

Integer overflow and out-of-bounds read⌗

In read_entry() when compressed == 1, memory is dynamically allocated and the path string is constructed.

    size_t strip_len = git_decode_varint((const unsigned char *)path_ptr,
                             &varint_len);
    size_t last_len = strlen(last);
    size_t prefix_len = last_len - strip_len;
    ....
    memcpy(tmp_path, last, prefix_len);
    memcpy(tmp_path + prefix_len, path_ptr + varint_len, suffix_len + 1);
    ....

Here strip_len is user-controlled as it is the result of varint decoding from the index file. For example, during the first call to read_entry(), last_len is 0 and so, any positive integer in strip_len will cause an integer overflow with prefix_len ending up being a huge number (casting to size_t). prefix_len is being used in memcpy to mention how many bytes to copy. Essentially the attacker can arbitrarily control how many ever bytes to read from *last. This causes an out-of-bounds read, reading data from outside the *last buffer and storing it in entry->path.

Note: This might seem trivial as *last is initialized to an empty string which is present in data section and no meaningful data could be leaked. But, after the first successful iteration (could be done by having a valid index entry), *last will be pointing to a heap address and the adjacent chunks if present could be read into entry->path.

Memory Exhaustion DoS⌗ 

    GITERR_CHECK_ALLOC_ADD(&path_len, prefix_len, suffix_len);
    GITERR_CHECK_ALLOC_ADD(&path_len, path_len, 1);
    tmp_path = git__malloc(path_len);
    GITERR_CHECK_ALLOC(tmp_path);

The same vulnerability detailed above, could be leveraged to cause a memory DoS. Since, an attacker controls the varint decoding, in turn they directly control prefix_len which is used in allocating memory. So, by carefully constructing a series of varints, one can make libgit2 request for huge memory allocations in a loop, causing memory exhaustion (and not requesting too much memory at once as that would cause alloc to fail with OOM). This would also pass through the error checks as entry_size returned will be a reasonable number and the seek will not fail.

Possible race condition⌗

In index_free(), the execution starts only after ensuring that there are no outstanding iterators.

assert(!git_atomic_get(&index->readers));

But, in release builds, assert() gets removed and the check is nullified, resulting in index structure getting freed even when there are outstanding index iterators still present.

Assert being used for validation⌗

There are a number of places where assert() is used for validating the input to the functions. In release builds, assert() gets removed and so, these functions do not have any validations present.

git_index_open(), git_index_clear(), git_index_set_caps(), git_index_write_tree() are some of the API functions which validates the parameters it receives by using assert. As these statements are essentially removed in release builds, if the programmers consuming libgit2 APIs could pass any values (say null pointers) which would essentially bypass the checks. So, instead of raising an assertion failure, the execution will continue and might cause unintended side effects in many functions as the execution does not stop on validation failure anymore. This might result in something as simple as DoS by null pointer dereference or resulting in an illegal state as the validation has been bypassed. We have not explored this part completely and there could be many instances where assert() is being used for validation.

Patches⌗

These were patched (p1, p2 and p3) in a few hours after the report was made and libgit2 team made a maintenance release (v0.26.2) immediately.