Skip to content

Commit

Permalink
Fix two security issues.
Browse files Browse the repository at this point in the history 
* Add 02_cve_2015-0838: Fix buffer overflow in C implementation of
  apply_delta (CVE-2015-0838). Closes: #780958
* Add 03_cve_2014-9706: Don't allow writing to files under .git/ when
  checking out working trees (CVE-2014-9706). Closes: #780989
  • Loading branch information
@jelmer
jelmer committed Mar 24, 2015
1 parent c6ce6a3 commit e0514ec
Show file tree
Hide file tree
Showing 8 changed files with 291 additions and 6 deletions.
9 changes: 9 additions & 0 deletions debian/changelog
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
dulwich (0.9.7-3) UNRELEASED; urgency=medium

* Add 02_cve_2015-0838: Fix buffer overflow in C implementation of
apply_delta (CVE-2015-0838). Closes: #780958
* Add 03_cve_2014-9706: Don't allow writing to files under .git/ when
checking out working trees (CVE-2014-9706). Closes: #780989

-- Jelmer Vernooij <jelmer@debian.org> Tue, 24 Mar 2015 22:34:34 +0000

dulwich (0.9.7-2) unstable; urgency=medium

* Require at least Python 2.7. Closes: #753519
Expand Down
111 changes: 111 additions & 0 deletions debian/patches/02_cve_2015-0838
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
commit b25e8390074060ea2aed25cf070b8e98b85a3875
Author: Jelmer Vernooij <jelmer@google.com>
Date: Fri Mar 6 12:29:07 2015 +0000

Fix buffer overflow in C version of apply_delta().

This is CVE-2015-0838.

Thanks to Ivan Fratric of the Google Security Team for
reporting this issue.

diff --git a/dulwich/_pack.c b/dulwich/_pack.c
index d1534a5..440a9a9 100644
--- a/dulwich/_pack.c
+++ b/dulwich/_pack.c
@@ -20,6 +20,8 @@
#include <Python.h>
#include <stdint.h>

+static PyObject *PyExc_ApplyDeltaError = NULL;
+
static int py_is_sha(PyObject *sha)
{
if (!PyString_CheckExact(sha))
@@ -103,7 +105,7 @@ static PyObject *py_apply_delta(PyObject *self, PyObject *args)
index = 0;
src_size = get_delta_header_size(delta, &index, delta_len);
if (src_size != src_buf_len) {
- PyErr_Format(PyExc_ValueError,
+ PyErr_Format(PyExc_ApplyDeltaError,
"Unexpected source buffer size: %lu vs %d", src_size, src_buf_len);
Py_DECREF(py_src_buf);
Py_DECREF(py_delta);
@@ -146,12 +148,16 @@ static PyObject *py_apply_delta(PyObject *self, PyObject *args)
break;
memcpy(out+outindex, src_buf+cp_off, cp_size);
outindex += cp_size;
+ dest_size -= cp_size;
} else if (cmd != 0) {
+ if (cmd > dest_size)
+ break;
memcpy(out+outindex, delta+index, cmd);
outindex += cmd;
index += cmd;
+ dest_size -= cmd;
} else {
- PyErr_SetString(PyExc_ValueError, "Invalid opcode 0");
+ PyErr_SetString(PyExc_ApplyDeltaError, "Invalid opcode 0");
Py_DECREF(ret);
Py_DECREF(py_delta);
Py_DECREF(py_src_buf);
@@ -162,13 +168,13 @@ static PyObject *py_apply_delta(PyObject *self, PyObject *args)
Py_DECREF(py_delta);

if (index != delta_len) {
- PyErr_SetString(PyExc_ValueError, "delta not empty");
+ PyErr_SetString(PyExc_ApplyDeltaError, "delta not empty");
Py_DECREF(ret);
return NULL;
}

- if (dest_size != outindex) {
- PyErr_SetString(PyExc_ValueError, "dest size incorrect");
+ if (dest_size != 0) {
+ PyErr_SetString(PyExc_ApplyDeltaError, "dest size incorrect");
Py_DECREF(ret);
return NULL;
}
@@ -236,6 +242,15 @@ static PyMethodDef py_pack_methods[] = {
void init_pack(void)
{
PyObject *m;
+ PyObject *errors_module;
+
+ errors_module = PyImport_ImportModule("dulwich.errors");
+ if (errors_module == NULL)
+ return;
+
+ PyExc_ApplyDeltaError = PyObject_GetAttrString(errors_module, "ApplyDeltaError");
+ if (PyExc_ApplyDeltaError == NULL)
+ return;

m = Py_InitModule3("_pack", py_pack_methods, NULL);
if (m == NULL)
diff --git a/dulwich/tests/test_pack.py b/dulwich/tests/test_pack.py
index d33104f..393b931 100644
--- a/dulwich/tests/test_pack.py
+++ b/dulwich/tests/test_pack.py
@@ -28,6 +28,7 @@ import tempfile
import zlib

from dulwich.errors import (
+ ApplyDeltaError,
ChecksumMismatch,
)
from dulwich.file import (
@@ -181,6 +182,14 @@ class TestPackDeltas(TestCase):
self.skipTest("big strings don't work yet")
self._test_roundtrip(self.test_string_huge, self.test_string_huge)

+ def test_dest_overflow(self):
+ self.assertRaises(
+ ApplyDeltaError,
+ apply_delta, 'a'*0x10000, '\x80\x80\x04\x80\x80\x04\x80' + 'a'*0x10000)
+ self.assertRaises(
+ ApplyDeltaError,
+ apply_delta, '', '\x00\x80\x02\xb0\x11\x11')
+

class TestPackData(PackTests):
"""Tests getting the data from the packfile."""
92 changes: 92 additions & 0 deletions debian/patches/03_cve_2014-9706
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
commit 598be9170c5e21ba408ba139a2b7bd7da6a04c70
Author: Jelmer Vernooij <jelmer@samba.org>
Date: Thu Jan 15 23:30:28 2015 +0100

By default refuse to create index entries with a path starting with .git/.

diff --git a/dulwich/index.py b/dulwich/index.py
index f32d70f..3c7488e 100644
--- a/dulwich/index.py
+++ b/dulwich/index.py
@@ -402,8 +402,14 @@ def index_entry_from_stat(stat_val, hex_sha, flags, mode=None):
stat_val.st_gid, stat_val.st_size, hex_sha, flags)


+def validate_path_default(path):
+ """Default path validator that just checks for .git/."""
+ return not path.startswith(".git/")
+
+
def build_index_from_tree(prefix, index_path, object_store, tree_id,
- honor_filemode=True):
+ honor_filemode=True,
+ validate_path=validate_path_default):
"""Generate and materialize index from a tree

:param tree_id: Tree to materialize
@@ -412,6 +418,8 @@ def build_index_from_tree(prefix, index_path, object_store, tree_id,
:param object_store: Non-empty object store holding tree contents
:param honor_filemode: An optional flag to honor core.filemode setting in
config file, default is core.filemode=True, change executable bit
+ :param validate_path: Function to validate paths to check out;
+ default just refuses filenames starting with .git/.

:note:: existing index is wiped and contents are not merged
in a working dir. Suiteable only for fresh clones.
@@ -420,6 +428,8 @@ def build_index_from_tree(prefix, index_path, object_store, tree_id,
index = Index(index_path)

for entry in object_store.iter_tree_contents(tree_id):
+ if not validate_path(entry.path):
+ continue
full_path = os.path.join(prefix, entry.path)

if not os.path.exists(os.path.dirname(full_path)):
diff --git a/dulwich/tests/test_index.py b/dulwich/tests/test_index.py
index 89b5d54..1852b9a 100644
--- a/dulwich/tests/test_index.py
+++ b/dulwich/tests/test_index.py
@@ -284,6 +284,43 @@ class BuildIndexTests(TestCase):
# Verify no files
self.assertEqual(['.git'], os.listdir(repo.path))

+ def test_git_dir(self):
+ if os.name != 'posix':
+ self.skipTest("test depends on POSIX shell")
+
+ repo_dir = tempfile.mkdtemp()
+ repo = Repo.init(repo_dir)
+ self.addCleanup(shutil.rmtree, repo_dir)
+
+ # Populate repo
+ filea = Blob.from_string('file a')
+ filee = Blob.from_string('d')
+
+ tree = Tree()
+ tree['.git/a'] = (stat.S_IFREG | 0o644, filea.id)
+ tree['c/e'] = (stat.S_IFREG | 0o644, filee.id)
+
+ repo.object_store.add_objects([(o, None)
+ for o in [filea, filee, tree]])
+
+ build_index_from_tree(repo.path, repo.index_path(),
+ repo.object_store, tree.id)
+
+ # Verify index entries
+ index = repo.open_index()
+ self.assertEqual(len(index), 1)
+
+ # filea
+ apath = os.path.join(repo.path, '.git', 'a')
+ self.assertFalse(os.path.exists(apath))
+
+ # filee
+ epath = os.path.join(repo.path, 'c', 'e')
+ self.assertTrue(os.path.exists(epath))
+ self.assertReasonableIndexEntry(index['c/e'],
+ stat.S_IFREG | 0o644, 1, filee.id)
+ self.assertFileContents(epath, 'd')
+
def test_nonempty(self):
if os.name != 'posix':
self.skipTest("test depends on POSIX shell")
2 changes: 2 additions & 0 deletions debian/patches/series
View file
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
01_setup_cfg
02_cve_2015-0838
03_cve_2014-9706
25 changes: 20 additions & 5 deletions dulwich/_pack.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#include <Python.h>
#include <stdint.h>

static PyObject *PyExc_ApplyDeltaError = NULL;

static int py_is_sha(PyObject *sha)
{
if (!PyString_CheckExact(sha))
Expand Down Expand Up @@ -103,7 +105,7 @@ static PyObject *py_apply_delta(PyObject *self, PyObject *args)
index = 0;
src_size = get_delta_header_size(delta, &index, delta_len);
if (src_size != src_buf_len) {
PyErr_Format(PyExc_ValueError,
PyErr_Format(PyExc_ApplyDeltaError,
"Unexpected source buffer size: %lu vs %d", src_size, src_buf_len);
Py_DECREF(py_src_buf);
Py_DECREF(py_delta);
Expand Down Expand Up @@ -146,12 +148,16 @@ static PyObject *py_apply_delta(PyObject *self, PyObject *args)
break;
memcpy(out+outindex, src_buf+cp_off, cp_size);
outindex += cp_size;
dest_size -= cp_size;
} else if (cmd != 0) {
if (cmd > dest_size)
break;
memcpy(out+outindex, delta+index, cmd);
outindex += cmd;
index += cmd;
dest_size -= cmd;
} else {
PyErr_SetString(PyExc_ValueError, "Invalid opcode 0");
PyErr_SetString(PyExc_ApplyDeltaError, "Invalid opcode 0");
Py_DECREF(ret);
Py_DECREF(py_delta);
Py_DECREF(py_src_buf);
Expand All @@ -162,13 +168,13 @@ static PyObject *py_apply_delta(PyObject *self, PyObject *args)
Py_DECREF(py_delta);

if (index != delta_len) {
PyErr_SetString(PyExc_ValueError, "delta not empty");
PyErr_SetString(PyExc_ApplyDeltaError, "delta not empty");
Py_DECREF(ret);
return NULL;
}

if (dest_size != outindex) {
PyErr_SetString(PyExc_ValueError, "dest size incorrect");
if (dest_size != 0) {
PyErr_SetString(PyExc_ApplyDeltaError, "dest size incorrect");
Py_DECREF(ret);
return NULL;
}
Expand Down Expand Up @@ -236,6 +242,15 @@ static PyMethodDef py_pack_methods[] = {
void init_pack(void)
{
PyObject *m;
PyObject *errors_module;

errors_module = PyImport_ImportModule("dulwich.errors");
if (errors_module == NULL)
return;

PyExc_ApplyDeltaError = PyObject_GetAttrString(errors_module, "ApplyDeltaError");
if (PyExc_ApplyDeltaError == NULL)
return;

m = Py_InitModule3("_pack", py_pack_methods, NULL);
if (m == NULL)
Expand Down
12 changes: 11 additions & 1 deletion dulwich/index.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,8 +402,14 @@ def index_entry_from_stat(stat_val, hex_sha, flags, mode=None):
stat_val.st_gid, stat_val.st_size, hex_sha, flags)


def validate_path_default(path):
"""Default path validator that just checks for .git/."""
return not path.startswith(".git/")


def build_index_from_tree(prefix, index_path, object_store, tree_id,
honor_filemode=True):
honor_filemode=True,
validate_path=validate_path_default):
"""Generate and materialize index from a tree
:param tree_id: Tree to materialize
Expand All @@ -412,6 +418,8 @@ def build_index_from_tree(prefix, index_path, object_store, tree_id,
:param object_store: Non-empty object store holding tree contents
:param honor_filemode: An optional flag to honor core.filemode setting in
config file, default is core.filemode=True, change executable bit
:param validate_path: Function to validate paths to check out;
default just refuses filenames starting with .git/.
:note:: existing index is wiped and contents are not merged
in a working dir. Suiteable only for fresh clones.
Expand All @@ -420,6 +428,8 @@ def build_index_from_tree(prefix, index_path, object_store, tree_id,
index = Index(index_path)

for entry in object_store.iter_tree_contents(tree_id):
if not validate_path(entry.path):
continue
full_path = os.path.join(prefix, entry.path)

if not os.path.exists(os.path.dirname(full_path)):
Expand Down
37 changes: 37 additions & 0 deletions dulwich/tests/test_index.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,43 @@ def test_empty(self):
# Verify no files
self.assertEqual(['.git'], os.listdir(repo.path))

def test_git_dir(self):
if os.name != 'posix':
self.skipTest("test depends on POSIX shell")

repo_dir = tempfile.mkdtemp()
repo = Repo.init(repo_dir)
self.addCleanup(shutil.rmtree, repo_dir)

# Populate repo
filea = Blob.from_string('file a')
filee = Blob.from_string('d')

tree = Tree()
tree['.git/a'] = (stat.S_IFREG | 0o644, filea.id)
tree['c/e'] = (stat.S_IFREG | 0o644, filee.id)

repo.object_store.add_objects([(o, None)
for o in [filea, filee, tree]])

build_index_from_tree(repo.path, repo.index_path(),
repo.object_store, tree.id)

# Verify index entries
index = repo.open_index()
self.assertEqual(len(index), 1)

# filea
apath = os.path.join(repo.path, '.git', 'a')
self.assertFalse(os.path.exists(apath))

# filee
epath = os.path.join(repo.path, 'c', 'e')
self.assertTrue(os.path.exists(epath))
self.assertReasonableIndexEntry(index['c/e'],
stat.S_IFREG | 0o644, 1, filee.id)
self.assertFileContents(epath, 'd')

def test_nonempty(self):
if os.name != 'posix':
self.skipTest("test depends on POSIX shell")
Expand Down
9 changes: 9 additions & 0 deletions dulwich/tests/test_pack.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import zlib

from dulwich.errors import (
ApplyDeltaError,
ChecksumMismatch,
)
from dulwich.file import (
Expand Down Expand Up @@ -181,6 +182,14 @@ def test_overflow_64k(self):
self.skipTest("big strings don't work yet")
self._test_roundtrip(self.test_string_huge, self.test_string_huge)

def test_dest_overflow(self):
self.assertRaises(
ApplyDeltaError,
apply_delta, 'a'*0x10000, '\x80\x80\x04\x80\x80\x04\x80' + 'a'*0x10000)
self.assertRaises(
ApplyDeltaError,
apply_delta, '', '\x00\x80\x02\xb0\x11\x11')


class TestPackData(PackTests):
"""Tests getting the data from the packfile."""
Expand Down

0 comments on commit e0514ec

Please sign in to comment.