Bug#889511: libpodofo: CVE-2018-5295
Control: tags -1 fixed-upstream Hello Mattia, hello all, > On 04 February 2018 at 20:57 Mattia Rizzolo wrote: > > > Control: forwarded -1 > https://sourceforge.net/p/podofo/mailman/message/36215539/ > > On Sun, Feb 04, 2018 at 01:24:53AM +0100, Matthias Brinke wrote: >> CVE-2018-5295 from the security-tracker.debian.org: >> In PoDoFo 0.9.5, there is an integer overflow in >> the PdfXRefStreamParserObject::ParseStream function >> (base/PdfXRefStreamParserObject.cpp). Remote attackers >> could leverage this vulnerability to cause a denial-of-service >> via a crafted pdf file. > > Right. > For cross-reference, this is being dealt upstream by this thread that > started the 6th of Jan: > https://sourceforge.net/p/podofo/mailman/message/36180168/ >> I've implemented a patch to fix this vulnerability, it is attached >> and tested > > Thank you! you're welcome, > I've forwarded it upstream, see the first url above. the patch has been accepted (committed in svn r1889 [1]), based on that I've set this bug to "fixed-upstream" (above). I plan to fix CVE-2018-5309 next, even though there was a bit of disagreement about that upstream [2]. > > -- > regards, > Mattia Rizzolo > Best regards, Matthias Brinke [1] https://sourceforge.net/p/podofo/code/1889/ [2] https://sourceforge.net/p/podofo/mailman/message/36189599/
Bug#860995: Correction for CVE-2017-8054 patch (was: Bug#860995: libpodofo: CVE-2017-8054 fix tested in unstable chroot, PoC generator source attached)
Hello Mattia,
> Mattia Rizzolo has written on 21 December 2017 at 22:54:
>
>
> Control: tag -1 patch
>
> On Thu, Dec 21, 2017 at 04:55:00PM +0100, Matthias Brinke wrote:
>> I have simplified my fix for CVE-2017-8054 (stack overflow
>> by infinite recursion from loop in pages tree) and tested
>> it again in an unstable chroot (entered by sbuild from
>> ... snip ...
>
> I could, but could you please send it upstream?
> The only way to interact with upstream is the podofo-users ML
> https://sourceforge.net/p/podofo/mailman/podofo-users/
There is a post on the podofo-users mailing list by zyx which
says that running test/unit/podofo-test a heap-use-after-free
bug was detected [1]. I've investigated that and implemented a
correction, which I tested with -fsanitize=address (ASan) in
a Debian sid chroot (up-to-date, mostly? minimal) through
sbuild (from jessie-backports), attached here. Only memory leaks
were detected, therefore the test log is not attached here, also
because it's 292 KiB large (many leaks, but that's OT here). As
far as I've already seen, the leaks seems to be elsewhere.
This patch is complete to add to the Debian package, so please
disregard the older (and incorrect) patch in this bug thread.
I'm sorry I'm answering only now, the divergence between the
package and the upstream svn repository (also through the partial
revert) presented some challenges in patch handling. Because of
this, the patch is also not for forwarding (it wouldn't apply there).
I'm not changing the fixed-upstream tag as the partial revert is
already mentioned in the security-tracker notes and would've been
reflected in the (removal of the) tag if it was necessary, right?
>
> --
> regards,
> Mattia Rizzolo
>
Best regards, Matthias Brinke
[1] https://sourceforge.net/p/podofo/mailman/message/36215307/Description: Fix CVE-2017-8054, also avoiding use-after-free
The recursive call in the case of the requested page index
having an array has been removed to prevent possibility of
stack overflow there, and replaced it by code setting the
variable which had been modified in the new stack frame in
the former revision, directly, after checking its validity.
The second hunk inserts cycle detection for the parent list.
Author: Matthias Brinke <podofo-sec-cont...@mailbox.org>
Bug-Debian: https://bugs.debian.org/860995
Forwarded: not-needed
Last-Update: 2018-02-05
---
Index: src/doc/PdfPagesTree.cpp
===
--- libpodofo-0.9.5.orig/src/doc/PdfPagesTree.cpp
+++ libpodofo-0.9.5/src/doc/PdfPagesTree.cpp
@@ -34,6 +34,7 @@
#include "PdfPagesTree.h"
#include "base/PdfDefinesPrivate.h"
+#include
#include "base/PdfArray.h"
#include "base/PdfDictionary.h"
@@ -478,7 +479,18 @@
if( rVar.IsArray() )
{
// Fixes some broken PDFs who have trees with 1 element kids arrays
-return GetPageNodeFromArray( 0, rVar.GetArray(), rLstParents );
+// Recursive call removed to prevent stack overflow, replaced by:
+// all the following inside this conditional, plus restart looping
+const PdfArray & rVarArray = rVar.GetArray();
+if (rVarArray.GetSize() == 0)
+{
+PdfError::LogMessage( eLogSeverity_Critical, "Trying to access"
+" first page index of empty array" );
+return NULL;
+}
+PdfVariant rVarFirstEntry = rVarArray[0]; // avoids use-after-free
+rVar = rVarFirstEntry; // in this line (rVar-ref'd array is freed)
+continue;
}
else if( !rVar.IsReference() )
{
@@ -502,6 +516,18 @@
if( !pgObject->GetDictionary().HasKey( "Kids" ) )
return NULL;
+if ( std::find( rLstParents.begin(), rLstParents.end(), pgObject )
+!= rLstParents.end() ) // cycle in parent list detected, fend
+{ // off security vulnerability CVE-2017-8054 (infinite recursion)
+std::ostringstream oss;
+oss << "Cycle in page tree: child in /Kids array of object "
+<< ( *(rLstParents.rbegin()) )->Reference().ToString()
+<< " back-references to object " << pgObject->Reference()
+.ToString() << " one of whose descendants the former is.";
+
+PODOFO_RAISE_ERROR_INFO( ePdfError_PageNotFound, oss.str() );
+}
+
rLstParents.push_back( pgObject );
rVar = *(pgObject->GetDictionary().GetKey( "Kids" ));
} else {
--- libpodofo-0.9.5.orig/src/base/PdfError.cpp
+++ libpodofo-0.9.5/src/base/PdfError.cpp
@@ -60,6 +60,12 @@ PdfErrorInfo::PdfErrorInfo()
Bug#889511: libpodofo: CVE-2018-5295
Source: libpodofo
Version: 0.9.5-8
Tags: upstream security patch
Severity: important
CVE-2018-5295 from the security-tracker.debian.org:
In PoDoFo 0.9.5, there is an integer overflow in
the PdfXRefStreamParserObject::ParseStream function
(base/PdfXRefStreamParserObject.cpp). Remote attackers
could leverage this vulnerability to cause a denial-of-service
via a crafted pdf file.
I've implemented a patch to fix this vulnerability, it is attached
and tested with the PoC from the report (RedHat Bugzilla #1531897)
and GCC 7 UBSan (-fsanitize=undefined in CXXFLAGS set via .sbuildrc).
The builds were done with sbuild in an up-to-date Debian sid chroot.
I've done the tests in a sandbox, where without the patch,
signed integer overflow was detected, with it, nothing from UBSan.
Otherwise, the same (expected, correct for the PoC) exception message
with detailed info and "call stack" (via PdfError method) was output
by podofoimgextract.
This bug is probably also present in version 0.9.4-6 in stretch, but I
haven't tested that, I don't use stretch (yet).
If you fix the vulnerability please also make sure to include the
CVE (Common Vulnerabilities & Exposures) id in your changelog entry.
Best regards, Matthias BrinkeDescription: Fix CVE-2018-5295
Author: Matthias Brinke <podofo-sec-cont...@mailbox.org>
Last-Updated: 2018-01-30
---
--- libpodofo-0.9.5.orig/src/base/PdfXRefStreamParserObject.cpp
+++ libpodofo-0.9.5/src/base/PdfXRefStreamParserObject.cpp
@@ -38,7 +38,9 @@
#include "PdfStream.h"
#include "PdfVariant.h"
-#include
+// #include
+
+#include
namespace PoDoFo {
@@ -122,12 +124,25 @@ void PdfXRefStreamParserObject::ParseStr
{
char*pBuffer;
pdf_long lBufferLen;
-const size_t entryLen = static_cast(nW[0] + nW[1] + nW[2]);
-if( nW[0] + nW[1] + nW[2] < 0 )
+for(pdf_int64 nLengthSum = 0, i = 0; i < W_ARRAY_SIZE; i++ )
{
-PODOFO_RAISE_ERROR_INFO( ePdfError_NoXRef, "Invalid entry length in XRef stream" );
+if ( nW[i] < 0 )
+{
+PODOFO_RAISE_ERROR_INFO( ePdfError_NoXRef,
+"Negative field length in XRef stream" );
+}
+if ( std::numeric_limits::max() - nLengthSum < nW[i] )
+{
+PODOFO_RAISE_ERROR_INFO( ePdfError_NoXRef,
+"Invalid entry length in XRef stream" );
+}
+else
+{
+nLengthSum += nW[i];
+}
}
+const size_t entryLen = static_cast(nW[0] + nW[1] + nW[2]);
this->GetStream()->GetFilteredCopy( , );
Bug#860995: libpodofo: CVE-2017-8054 fix tested in unstable chroot, PoC generator source attached
Dear maintainer, hello all,
I have simplified my fix for CVE-2017-8054 (stack overflow
by infinite recursion from loop in pages tree) and tested
it again in an unstable chroot (entered by sbuild from
jessie-backports), which was clean, up-to-date and maybe
also minimal (why does it contain GNU autotools?). It is,
of course, attached here. The source for a generator just
using PoDoFo outputting the simple/mostly-minimal PoC I
tested with is also attached. Please accept it for your
next upload (could you please do it before upstream puts
out its next release or rc tarball?) and bump the shlibs
version, too, because a PdfError constructor is added by it.
Best regards, MatthiasDescription: Fix CVE-2017-8054
The recursive call in the case of the requested page index
having an array has been removed to prevent possibility of
stack overflow there, and replaced it by code setting the
variable which had been modified in the new stack frame in
the former revision, directly, after checking its validity.
The second hunk inserts cycle detection for the parent list.
Author: Matthias Brinke <podofo-sec-cont...@mailbox.org>
Bug-Debian: https://bugs.debian.org/860995
Forwarded: no
Last-Update: 2017-12-21
---
--- libpodofo-0.9.5.orig/src/doc/PdfPagesTree.cpp
+++ libpodofo-0.9.5/src/doc/PdfPagesTree.cpp
@@ -34,6 +34,7 @@
#include "PdfPagesTree.h"
#include "base/PdfDefinesPrivate.h"
+#include
#include "base/PdfArray.h"
#include "base/PdfDictionary.h"
@@ -478,7 +479,17 @@ PdfObject* PdfPagesTree::GetPageNodeFrom
if( rVar.IsArray() )
{
// Fixes some broken PDFs who have trees with 1 element kids arrays
-return GetPageNodeFromArray( 0, rVar.GetArray(), rLstParents );
+// Recursive call removed to prevent stack overflow, replaced by:
+// all the following inside this conditional, plus restart looping
+const PdfArray & rVarArray = rVar.GetArray();
+if (rVarArray.GetSize() == 0)
+{
+PdfError::LogMessage( eLogSeverity_Critical, "Trying to access"
+" first page index of empty array" );
+return NULL;
+}
+rVar = rVarArray[0];
+continue;
}
else if( !rVar.IsReference() )
{
@@ -502,6 +513,18 @@ PdfObject* PdfPagesTree::GetPageNodeFrom
if( !pgObject->GetDictionary().HasKey( "Kids" ) )
return NULL;
+if ( std::find( rLstParents.begin(), rLstParents.end(), pgObject )
+!= rLstParents.end() ) // cycle in parent list detected, fend
+{ // off security vulnerability CVE-2017-8054 (infinite recursion)
+std::ostringstream oss;
+oss << "Cycle in page tree: child in /Kids array of object "
+<< ( *(rLstParents.rbegin()) )->Reference().ToString()
+<< " back-references to object " << pgObject->Reference()
+.ToString() << " one of whose descendants the former is.";
+
+PODOFO_RAISE_ERROR_INFO( ePdfError_PageNotFound, oss.str() );
+}
+
rLstParents.push_back( pgObject );
rVar = *(pgObject->GetDictionary().GetKey( "Kids" ));
} else {
--- libpodofo-0.9.5.orig/src/base/PdfError.h
+++ libpodofo-0.9.5/src/base/PdfError.h
@@ -158,8 +158,8 @@ enum ELogSeverity {
/** \def PODOFO_RAISE_ERROR_INFO( x, y )
*
* Set the value of the variable eCode (which has to exist in the current function) to x
- * and return the eCode. Additionally additional information on the error y is set. y has
- * to be an c-string.
+ * and return the eCode. Additionally additional information on the error y is set.
+ * y can be a C string, but can also be a C++ std::string.
*/
#define PODOFO_RAISE_ERROR_INFO( x, y ) throw ::PoDoFo::PdfError( x, __FILE__, __LINE__, y );
@@ -184,6 +184,7 @@ class PODOFO_API PdfErrorInfo {
public:
PdfErrorInfo();
PdfErrorInfo( int line, const char* pszFile, const char* pszInfo );
+PdfErrorInfo( int line, const char* pszFile, std::string pszInfo );
PdfErrorInfo( int line, const char* pszFile, const wchar_t* pszInfo );
PdfErrorInfo( const PdfErrorInfo & rhs );
@@ -195,6 +196,7 @@ class PODOFO_API PdfErrorInfo {
inline const std::wstring & GetInformationW() const { return m_swInfo; }
inline void SetInformation( const char* pszInfo ) { m_sInfo = pszInfo ? pszInfo : ""; }
+inline void SetInformation( std::string pszInfo ) { m_sInfo = pszInfo; }
inline void SetInformation( const wchar_t* pszInfo ) { m_swInfo = pszInfo ? pszInfo : L""; }
private:
@@ -252,12 +254,22 @@ class PODOFO_EXCEPTION_API_DOXYGEN PdfEr
* Use the compiler
Bug#860995: libpodofo: CVE-2017-8054 fix test and (for jessie) missing header attached
Hello,
I'm sorry to have had missed attaching (in my earlier post to
this bug) what is now attached here: the source code of the test
program for verifying the fix (for both jessie and sid, but the
fix doesn't work with sid's g++-7/libstdc++6, a corrected fix
which I tested in a sid chroot in the meantime will be sent
shortly) and the workaround header for jessie to make available
the method PdfFontFactory::CreateBase14Font(), which the program
generating my PoC needs on jessie. That program also didn't build
on sid, an adapted version will also be provided in my next post.
Best regards, Matthias/***
* Copyright (C) 2007 by Dominik Seichter*
* domseich...@web.de*
* *
* This program is free software; you can redistribute it and/or modify *
* it under the terms of the GNU Library General Public License as *
* published by the Free Software Foundation; either version 2 of the*
* License, or (at your option) any later version. *
* *
* This program is distributed in the hope that it will be useful, *
* but WITHOUT ANY WARRANTY; without even the implied warranty of*
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
* GNU General Public License for more details. *
* *
* You should have received a copy of the GNU Library General Public *
* License along with this program; if not, write to the *
* Free Software Foundation, Inc., *
* 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. *
***/
#ifndef _PDF_FONT_FACTORY_H_
#define _PDF_FONT_FACTORY_H_
#include
#include
#include
namespace PoDoFo {
class PdfFontMetrics;
class PdfVecObjects;
enum EPdfFontFlags {
ePdfFont_Normal = 0x00,
ePdfFont_Embedded = 0x01,
ePdfFont_Bold = 0x02,
ePdfFont_Italic = 0x04,
ePdfFont_BoldItalic = ePdfFont_Bold | ePdfFont_Italic,
ePdfFont_Subsetting = 0x08
};
/** This is a factory class which knows
* which implementation of PdfFont is required
* for a certain font type with certain features (like encoding).
*/
class PODOFO_DOC_API PdfFontFactory {
public:
/** Create a new PdfFont object.
*
* \param pMetrics pointer to a font metrics object. The font in the PDF
* file will match this fontmetrics object. The metrics object is
* deleted along with the created font. In case of an error, it is deleted
* here.
* \param nFlags font flags or'ed together, specifying the font style and if it should be embedded
* \param pEncoding the encoding of this font.
* \param pParent the parent of the created font.
*
* \returns a new PdfFont object or NULL
*/
static PdfFont* CreateFontObject( PdfFontMetrics* pMetrics, int nFlags,
const PdfEncoding* const pEncoding, PdfVecObjects* pParent );
/** Create a new PdfFont from an existing
* font in a PDF file.
*
* \param pLibrary handle to the FreeType library, so that a PdfFontMetrics
* can be constructed for this font
* \param pObject a PDF font object
*/
static PdfFont* CreateFont( FT_Library* pLibrary, PdfObject* pObject );
/** Create a new Base14 (PDF standard/reader built-in) font.
*
* \param pszFontName the name of the font (ASCII)
* \param nFlags font flags or'ed together, specifying the font style
* \param pEncoding the font's encoding
* \param pvecObjects the vector of PDF objects the font is to belong to
*/
static PdfFont* CreateBase14Font(const char* pszFontName, int nFlags,
const PdfEncoding * const pEncoding, PdfVecObjects *pvecObjects);
/** Try to guess the fonttype from a the filename of a font file.
*
* \param pszFilename filename of a fontfile
* \returns the font type
*/
static EPdfFontType GetFontType( const char* pszFilename );
private:
/** Actually creates the font object for the requested type.
* Throws an exception in case of an error.
*
* \returns a new PdfFont object or NULL
*/
static PdfFont* CreateFontForType( EPdfFontType eType, PdfFontMetrics* pMetrics,
const PdfEncoding* const pEncoding,
bool bEmbed, bool bSubsetting, PdfVecObjects* pParent );
};
};
// Workaround for missing static method, copied from part of
Bug#861597: libpodofo: CVE-2017-8378 was already fixed upstream
Hello, this bug was already fixed upstream before being reported here. I'm sorry to not have noted that here earlier. The fix is in svn r1833 [1] of the upstream PoDoFo repository trunk, committed on 2017-03-24 as "Fix a crash when passing a PDF file with an encryption dictionary reference to a nonexistent object". [1] https://sourceforge.net/p/podofo/code/1833/ Best regards, Matthias
Bug#860995: patch proposal
Hello,
attached is a patch (tested with jessie, I don't use Debian 9/stretch yet) to
fix CVE-2017-8054.
The source code for a program using PoDoFo to generate the PoC I tested with is
also attached.
Best regards, Matthias--- src/doc/PdfPagesTree.cpp (revision 1793)
+++ src/doc/PdfPagesTree.cpp (working copy)
@@ -34,6 +34,7 @@
#include "PdfPagesTree.h"
#include "base/PdfDefinesPrivate.h"
+#include
#include "base/PdfArray.h"
#include "base/PdfDictionary.h"
@@ -478,7 +479,17 @@ PdfObject* PdfPagesTree::GetPageNodeFrom
if( rVar.IsArray() )
{
// Fixes some broken PDFs who have trees with 1 element kids arrays
-return GetPageNodeFromArray( 0, rVar.GetArray(), rLstParents );
+// Recursive call removed to prevent stack overflow, replaced by:
+// all the following inside this conditional, plus restart looping
+const PdfArray & rVarArray = rVar.GetArray();
+if (rVarArray.GetSize() == 0)
+{
+PdfError::LogMessage( eLogSeverity_Critical, "Trying to access"
+" first page index of empty array" );
+return NULL;
+}
+rVar = rVarArray[0];
+continue;
}
else if( !rVar.IsReference() )
{
@@ -502,6 +513,19 @@ PdfObject* PdfPagesTree::GetPageNodeFrom
if( !pgObject->GetDictionary().HasKey( "Kids" ) )
return NULL;
+if ( std::find( rLstParents.begin(), rLstParents.end(), pgObject )
+!= rLstParents.end() ) // cycle in parent list detected, fend
+{ // off security vulnerability CVE-2017-8054 (infinite recursion)
+std::ostringstream oss;
+oss << "Cycle in page tree: child in /Kids array of object "
+<< ( *(rLstParents.rbegin()) )->Reference().ToString()
+<< " back-references to object " << pgObject->Reference()
+.ToString() << " one of whose descendants the former is.";
+const char* pszErrorDesc = oss.str().c_str();
+PODOFO_RAISE_ERROR_INFO( ePdfError_PageNotFound,
+pszErrorDesc );
+}
+
rLstParents.push_back( pgObject );
rVar = *(pgObject->GetDictionary().GetKey( "Kids" ));
} else {
#include "workaround_create_base14.h"
#include
using namespace PoDoFo;
void CreatePagesLoopPoC(PdfDocument& doc)
{
PdfPage* pFirstPage = doc.GetPage( 0 );
PdfObject* pFirstPageObject = pFirstPage->GetObject();
PdfObject* pParentObject = pFirstPageObject->MustGetIndirectKey("Parent");
PdfArray & rParentKidsArray = pParentObject->MustGetIndirectKey("Kids")
->GetArray();
rParentKidsArray.insert(rParentKidsArray.begin(),
pParentObject->Reference());
pdf_int64 ll2 = 2; // needed to make the following call unambiguous
pParentObject->GetDictionary().AddKey("Count", ll2); // 1 original, 1 loop
}
#define FONTNAME "Helvetica"
#define TEXTPOS_X 72.0
#define TEXTPOS_Y 72.0
#define SAMPLE_TEXT "Sample"
void DrawSampleTextOnPage(PdfPage* pPage)
{
PdfPainter painter;
painter.SetPage( pPage );
PdfFont* pFont = PdfFontFactory::CreateBase14Font( FONTNAME, 0,
PdfEncodingFactory::GlobalWinAnsiEncodingInstance(),
pPage->GetObject()->GetOwner() );
painter.SetFont( pFont );
painter.DrawText( TEXTPOS_X, TEXTPOS_Y, PdfString( SAMPLE_TEXT,
pFont->GetEncoding() ));
painter.FinishPage();
}
#define OUTFILE "my-CVE-2017-8054-PoC.pdf"
int main()
{
PdfMemDocument doc;
PdfPage* pPage
= doc.CreatePage( PdfPage::CreateStandardPageSize( ePdfPageSize_A4 ));
DrawSampleTextOnPage( pPage );
CreatePagesLoopPoC( doc );
doc.Write( OUTFILE );
return 0;
}
