Skip to content

Security updates from 0.28.3#5202

Merged
ethomson merged 3 commits intomasterfrom
users/ethomson/security_updates
Aug 13, 2019
Merged

Security updates from 0.28.3#5202
ethomson merged 3 commits intomasterfrom
users/ethomson/security_updates

Conversation

@ethomson
Copy link
Copy Markdown
Member

Security updates ported from the 0.28.3 branch:

  • A carefully constructed commit object with a very large number
    of parents may lead to potential out-of-bounds writes or
    potential denial of service.

  • The ProgramData configuration file is always read for compatibility
    with Git for Windows and Portable Git installations. The ProgramData
    location is not necessarily writable only by administrators, so we
    now ensure that the configuration file is owned by the administrator
    or the current user.

dscho and others added 3 commits August 13, 2019 17:56
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.
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.
@ethomson ethomson merged commit 08cfa43 into master Aug 13, 2019
@ddevault
Copy link
Copy Markdown
Contributor

Has this been assigned a CVE?

@ethomson
Copy link
Copy Markdown
Member Author

CVE-2019-1211

@ddevault
Copy link
Copy Markdown
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants