Commit 8606a7c0 authored by Julian Andres Klode's avatar Julian Andres Klode
Browse files

Merge branch 'pu/security-lp-1899193'

parents 9c2f60a1 68862323
......@@ -77,5 +77,6 @@ extern "C" void initapt_inst()
ADDTYPE(module,"DebFile",&PyDebFile_Type);
ADDTYPE(module,"TarFile",&PyTarFile_Type);
ADDTYPE(module,"TarMember",&PyTarMember_Type);
ADDTYPE(module,"__FileFd",&PyFileFd_Type);
RETURN(module);
}
......@@ -20,7 +20,7 @@ extern PyTypeObject PyArArchive_Type;
extern PyTypeObject PyDebFile_Type;
extern PyTypeObject PyTarFile_Type;
extern PyTypeObject PyTarMember_Type;
extern PyTypeObject PyFileFd_Type;
struct PyTarFileObject : public CppPyObject<ExtractTar*> {
int min;
FileFd Fd;
......
......@@ -127,6 +127,35 @@ PyTypeObject PyArMember_Type = {
armember_getset, // tp_getset
};
static const char *filefd_doc=
"Internal helper type, representing a FileFd.";
PyTypeObject PyFileFd_Type = {
PyVarObject_HEAD_INIT(&PyType_Type, 0)
"apt_inst.__FileFd" , // tp_name
sizeof(CppPyObject<FileFd>), // tp_basicsize
0, // tp_itemsize
// Methods
CppDealloc<FileFd>, // tp_dealloc
0, // tp_print
0, // tp_getattr
0, // tp_setattr
0, // tp_compare
0, // tp_repr
0, // tp_as_number
0, // tp_as_sequence
0, // tp_as_mapping
0, // tp_hash
0, // tp_call
0, // tp_str
0, // tp_getattro
0, // tp_setattro
0, // tp_as_buffer
Py_TPFLAGS_DEFAULT, // tp_flags
filefd_doc, // tp_doc
};
// We just add an inline method and should thus be ABI compatible in a way that
// we can simply cast ARArchive instances to PyARArchiveHack.
class PyARArchiveHack : public ARArchive
......@@ -138,7 +167,7 @@ public:
};
struct PyArArchiveObject : public CppPyObject<PyARArchiveHack*> {
FileFd Fd;
CppPyObject<FileFd> *Fd;
};
static const char *ararchive_getmember_doc =
......@@ -184,7 +213,7 @@ static PyObject *ararchive_extractdata(PyArArchiveObject *self, PyObject *args)
"Member '%s' is too large to read into memory",name.path);
return 0;
}
if (!self->Fd.Seek(member->Start))
if (!self->Fd->Object.Seek(member->Start))
return HandleErrors();
char* value;
......@@ -195,7 +224,7 @@ static PyObject *ararchive_extractdata(PyArArchiveObject *self, PyObject *args)
"Member '%s' is too large to read into memory",name.path);
return 0;
}
self->Fd.Read(value, member->Size, true);
self->Fd->Object.Read(value, member->Size, true);
PyObject *result = PyBytes_FromStringAndSize(value, member->Size);
delete[] value;
return result;
......@@ -273,7 +302,7 @@ static PyObject *ararchive_extract(PyArArchiveObject *self, PyObject *args)
PyErr_Format(PyExc_LookupError,"No member named '%s'",name.path);
return 0;
}
return _extract(self->Fd, member, target);
return _extract(self->Fd->Object, member, target);
}
static const char *ararchive_extractall_doc =
......@@ -292,7 +321,7 @@ static PyObject *ararchive_extractall(PyArArchiveObject *self, PyObject *args)
const ARArchive::Member *member = self->Object->Members();
do {
if (_extract(self->Fd, member, target) == 0)
if (_extract(self->Fd->Object, member, target) == 0)
return 0;
} while ((member = member->Next));
Py_RETURN_TRUE;
......@@ -319,10 +348,10 @@ static PyObject *ararchive_gettar(PyArArchiveObject *self, PyObject *args)
return 0;
}
PyTarFileObject *tarfile = (PyTarFileObject*)CppPyObject_NEW<ExtractTar*>(self,&PyTarFile_Type);
new (&tarfile->Fd) FileFd(self->Fd.Fd());
PyTarFileObject *tarfile = (PyTarFileObject*)CppPyObject_NEW<ExtractTar*>(self->Fd,&PyTarFile_Type);
new (&tarfile->Fd) FileFd(self->Fd->Object.Fd());
tarfile->min = member->Start;
tarfile->Object = new ExtractTar(self->Fd, member->Size, comp);
tarfile->Object = new ExtractTar(self->Fd->Object, member->Size, comp);
return HandleErrors(tarfile);
}
......@@ -389,36 +418,38 @@ static PyObject *ararchive_new(PyTypeObject *type, PyObject *args,
PyObject *kwds)
{
PyObject *file;
PyArArchiveObject *self;
PyApt_Filename filename;
int fileno;
if (PyArg_ParseTuple(args,"O:__new__",&file) == 0)
return 0;
PyApt_UniqueObject<PyArArchiveObject> self(NULL);
// We receive a filename.
if (filename.init(file)) {
self = (PyArArchiveObject *)CppPyObject_NEW<ARArchive*>(0,type);
new (&self->Fd) FileFd(filename,FileFd::ReadOnly);
self.reset((PyArArchiveObject*) CppPyObject_NEW<ARArchive*>(0,type));
self->Fd = CppPyObject_NEW<FileFd>(NULL, &PyFileFd_Type);
new (&self->Fd->Object) FileFd(filename,FileFd::ReadOnly);
}
// We receive a file object.
else if ((fileno = PyObject_AsFileDescriptor(file)) != -1) {
// Clear the error set by PyObject_AsString().
PyErr_Clear();
self = (PyArArchiveObject *)CppPyObject_NEW<ARArchive*>(file,type);
new (&self->Fd) FileFd(fileno,false);
self->Fd = CppPyObject_NEW<FileFd>(NULL, &PyFileFd_Type);
self.reset((PyArArchiveObject*) CppPyObject_NEW<ARArchive*>(file,type));
new (&self->Fd->Object) FileFd(fileno,false);
}
else {
return 0;
}
self->Object = (PyARArchiveHack*)new ARArchive(self->Fd);
self->Object = (PyARArchiveHack*)new ARArchive(self->Fd->Object);
if (_error->PendingError() == true)
return HandleErrors();
return self;
return self.release();
}
static void ararchive_dealloc(PyObject *self)
{
((PyArArchiveObject *)(self))->Fd.~FileFd();
Py_CLEAR(((PyArArchiveObject *)(self))->Fd);
CppDeallocPtr<ARArchive*>(self);
}
......@@ -528,10 +559,10 @@ static PyObject *_gettar(PyDebFileObject *self, const ARArchive::Member *m,
{
if (!m)
return 0;
PyTarFileObject *tarfile = (PyTarFileObject*)CppPyObject_NEW<ExtractTar*>(self,&PyTarFile_Type);
new (&tarfile->Fd) FileFd(self->Fd.Fd());
PyTarFileObject *tarfile = (PyTarFileObject*)CppPyObject_NEW<ExtractTar*>(self->Fd,&PyTarFile_Type);
new (&tarfile->Fd) FileFd(self->Fd->Object.Fd());
tarfile->min = m->Start;
tarfile->Object = new ExtractTar(self->Fd, m->Size, comp);
tarfile->Object = new ExtractTar(self->Fd->Object, m->Size, comp);
return tarfile;
}
......@@ -578,16 +609,16 @@ static PyObject *debfile_get_tar(PyDebFileObject *self, const char *Name)
static PyObject *debfile_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
PyDebFileObject *self = (PyDebFileObject*)ararchive_new(type, args, kwds);
PyApt_UniqueObject<PyDebFileObject> self((PyDebFileObject*)ararchive_new(type, args, kwds));
if (self == NULL)
return NULL;
// DebFile
self->control = debfile_get_tar(self, "control.tar");
self->control = debfile_get_tar(self.get(), "control.tar");
if (self->control == NULL)
return NULL;
self->data = debfile_get_tar(self, "data.tar");
self->data = debfile_get_tar(self.get(), "data.tar");
if (self->data == NULL)
return NULL;
......@@ -596,14 +627,14 @@ static PyObject *debfile_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
return PyErr_Format(PyAptError, "No debian archive, missing %s",
"debian-binary");
if (!self->Fd.Seek(member->Start))
if (!self->Fd->Object.Seek(member->Start))
return HandleErrors();
char* value = new char[member->Size];
self->Fd.Read(value, member->Size, true);
self->Fd->Object.Read(value, member->Size, true);
self->debian_binary = PyBytes_FromStringAndSize(value, member->Size);
delete[] value;
return self;
return self.release();
}
static int debfile_traverse(PyObject *_self, visitproc visit, void* arg)
......
......@@ -309,4 +309,23 @@ public:
}
};
/**
* Basic smart pointer to hold initial objects.
*
* This is like a std::unique_ptr<PyObject, decltype(&Py_DecRef)> to some extend,
* but it is for initialization only, and hence will also clear out any members
* in case it deletes the instance (the error case).
*/
template <class T, bool clear=true> struct PyApt_UniqueObject {
T *self;
explicit PyApt_UniqueObject(T *self) : self(self) { }
~PyApt_UniqueObject() { reset(NULL); }
void reset(T *newself) { if (clear && self && Py_TYPE(self)->tp_clear) Py_TYPE(self)->tp_clear(self); Py_XDECREF(self); self = newself; }
PyApt_UniqueObject<T> operator =(PyApt_UniqueObject<T>) = delete;
bool operator ==(void *other) { return self == other; }
T *operator ->() { return self; }
T *get() { return self; }
T *release() { T *ret = self; self = NULL; return ret; }
};
#endif
......@@ -534,7 +534,6 @@ static PyObject *TagSecNew(PyTypeObject *type,PyObject *Args,PyObject *kwds) {
static PyObject *TagFileNew(PyTypeObject *type,PyObject *Args,PyObject *kwds)
{
TagFileData *New;
PyObject *File = 0;
char Bytes = 0;
......@@ -558,7 +557,7 @@ static PyObject *TagFileNew(PyTypeObject *type,PyObject *Args,PyObject *kwds)
return 0;
}
New = (TagFileData*)type->tp_alloc(type, 0);
PyApt_UniqueObject<TagFileData> New((TagFileData*)type->tp_alloc(type, 0));
if (fileno != -1)
{
#ifdef APT_HAS_GZIP
......@@ -596,7 +595,7 @@ static PyObject *TagFileNew(PyTypeObject *type,PyObject *Args,PyObject *kwds)
// Create the section
New->Section = (TagSecData*)(&PyTagSection_Type)->tp_alloc(&PyTagSection_Type, 0);
new (&New->Section->Object) pkgTagSection();
New->Section->Owner = New;
New->Section->Owner = New.get();
Py_INCREF(New->Section->Owner);
New->Section->Data = 0;
New->Section->Bytes = Bytes;
......@@ -605,7 +604,7 @@ static PyObject *TagFileNew(PyTypeObject *type,PyObject *Args,PyObject *kwds)
Py_XINCREF(New->Section->Encoding);
#endif
return HandleErrors(New);
return HandleErrors(New.release());
}
/*}}}*/
......
......@@ -341,7 +341,6 @@ PyTypeObject PyTarMember_Type = {
static PyObject *tarfile_new(PyTypeObject *type,PyObject *args,PyObject *kwds)
{
PyObject *file;
PyTarFileObject *self;
PyApt_Filename filename;
int fileno;
int min = 0;
......@@ -353,7 +352,7 @@ static PyObject *tarfile_new(PyTypeObject *type,PyObject *args,PyObject *kwds)
&max,&comp) == 0)
return 0;
self = (PyTarFileObject*)CppPyObject_NEW<ExtractTar*>(file,type);
PyApt_UniqueObject<PyTarFileObject> self((PyTarFileObject*)CppPyObject_NEW<ExtractTar*>(file,type));
// We receive a filename.
if (filename.init(file))
......@@ -364,15 +363,14 @@ static PyObject *tarfile_new(PyTypeObject *type,PyObject *args,PyObject *kwds)
new (&self->Fd) FileFd(fileno,false);
}
else {
Py_DECREF(self);
return 0;
}
self->min = min;
self->Object = new ExtractTar(self->Fd,max,comp);
if (_error->PendingError() == true)
return HandleErrors(self);
return self;
return HandleErrors(self.release());
return self.release();
}
static const char *tarfile_extractall_doc =
......
#!/usr/bin/python
# -*- coding: utf-8 -*-
#
# Copyright (C) 2020 Canonical Ltd
#
# Copying and distribution of this file, with or without modification,
# are permitted in any medium without royalty provided the copyright
# notice and this notice are preserved.
"""Unit tests for verifying the correctness of DebFile descriptor handling."""
import os
import unittest
from test_all import get_library_dir
import sys
libdir = get_library_dir()
if libdir:
sys.path.insert(0, libdir)
import apt_inst
import subprocess
import tempfile
@unittest.skipIf(
not os.path.exists("/proc/self/fd"), "no /proc/self/fd available"
)
class TestCVE_2020_27351(unittest.TestCase):
""" test the debfile """
GOOD_DEB = "data/test_debs/utf8-package_1.0-1_all.deb"
def test_success(self):
"""opening package successfully should not leak fd"""
before = os.listdir("/proc/self/fd")
apt_inst.DebFile(self.GOOD_DEB)
after = os.listdir("/proc/self/fd")
self.assertEqual(before, after)
def test_success_a_member(self):
"""fd should be kept around as long as a tarfile member"""
before = os.listdir("/proc/self/fd")
data = apt_inst.DebFile(self.GOOD_DEB).data
after = os.listdir("/proc/self/fd")
self.assertEqual(len(before), len(after) - 1)
del data
after = os.listdir("/proc/self/fd")
self.assertEqual(before, after)
def _create_deb_without(self, member):
temp = tempfile.NamedTemporaryFile(mode="wb")
try:
with open(self.GOOD_DEB, "rb") as deb:
temp.write(deb.read())
temp.flush()
subprocess.check_call(["ar", "d", temp.name, member])
return temp
except Exception as e:
temp.close()
raise e
def test_nocontrol(self):
"""opening package without control.tar.gz should not leak fd"""
before = os.listdir("/proc/self/fd")
with self._create_deb_without("control.tar.gz") as temp:
try:
apt_inst.DebFile(temp.name)
except SystemError as e:
self.assertIn("control.tar", str(e))
else:
self.fail("Did not raise an exception")
after = os.listdir("/proc/self/fd")
self.assertEqual(before, after)
def test_nodata(self):
"""opening package without data.tar.gz should not leak fd"""
before = os.listdir("/proc/self/fd")
with self._create_deb_without("data.tar.gz") as temp:
try:
apt_inst.DebFile(temp.name)
except SystemError as e:
self.assertIn("data.tar", str(e))
else:
self.fail("Did not raise an exception")
after = os.listdir("/proc/self/fd")
self.assertEqual(before, after)
def test_no_debian_binary(self):
"""opening package without debian-binary should not leak fd"""
before = os.listdir("/proc/self/fd")
with self._create_deb_without("debian-binary") as temp:
try:
apt_inst.DebFile(temp.name)
except SystemError as e:
self.assertIn("missing debian-binary", str(e))
else:
self.fail("Did not raise an exception")
after = os.listdir("/proc/self/fd")
self.assertEqual(before, after)
if __name__ == "__main__":
# logging.basicConfig(level=logging.DEBUG)
unittest.main()
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment