Hi Mark, hi all,
I've run-tested a hopefully-final version of my patch fixing
CVE-2017-5853 (tested with the original reproducer PDF file
using the test program whose source is also attached) and two
other CVEs (sorry, I didn't have enough time to test those)
probably also, so please review, test and please mention my full
name and the CVE IDs in the commit message if you (committer)
accept. Used compiler: GCC 5.2.1 with -fsanitize=address
I'll probably have too little time for PoDoFo in the foreseeable
future, sorry, I need to do report/thesis work for studies.
Best regards, mabri
> Mark Rogers <mark.rog...@powermapper.com> has written on 21 April 2017 at
> 20:39:
>
>
> The revised patch doesn’t compile because it uses:
>
> + PODOFO_ERROR_INFO( ePdfError_ValueOutOfRange,
> + "xref subsection's given entry numbers together too large" );
>
> instead of
>
> + PODOFO_RAISE_ERROR_INFO ( ePdfError_ValueOutOfRange,
> + "xref subsection's given entry numbers together too large" );
>
>
> On 09/04/2017, 22:17, "Matthew Brincke" <ma...@mailbox.org> wrote:
>
> Hi zyx, hi all,
>
> I've replaced the asserts with PODOFO_RAISE_ERROR_INFO calls
> in if checks and removed the size_t check (replacing it with
> a check more to the point and C++-like), so I hope the change
> is now ready for inclusion as attached to this e-mail (or maybe
> with minor edits still crediting me, please).
> I, like Mark, also haven't tested if it actually fixes
> CVE-2017-5855, if it wouldn't, please still accept it for fixing
> the other two.)
>
> Best regards, mabri
>
> zyx <z...@litepdf.cz> has written on 9 April 2017 at 13:33:
> >
> > On Sat, 2017-04-08 at 18:32 +0200, Matthew Brincke wrote:
> >
> > > * PODOFO_ASSERT( nFirstObject > 0 );
> >
> > Hi,
> > I do not like asserts, unless being used in unit tests or such places.
> > Especially this place is used to parse random data from outside, which
> > the library has no control of, then it's not a good idea to abort whole
> > application due to the broken/unexpected input. I know PODOFO_ASSERT()
> > is sensitive for debug builds, but anyway.
> >
> > > * PODOFO_ASSERT( sizeof(PdfParser::s_nMaxObjects) <= sizeof(size_t)
> );
> >
> > sizeof() tells you how many bytes the argument holds. Is there a typo
> > in this test?
> >
> > I didn't run either of the proposed patches yet, though I agree with
> > Matthew that if the checks can be done without ABI changes, then it'll
> > be a better option.
> >
> > Bye,
> > zyx
> >
>
--- PdfParser.cpp (revision 1834)
+++ PdfParser.cpp (working copy)
@@ -745,21 +745,44 @@ void PdfParser::ReadXRefContents( pdf_lo
void PdfParser::ReadXRefSubsection( pdf_int64 & nFirstObject, pdf_int64 & nNumObjects )
{
- int count = 0;
+ pdf_int64 count = 0;
#ifdef PODOFO_VERBOSE_DEBUG
- PdfError::DebugMessage("Reading XRef Section: %" PDF_FORMAT_INT64 " with %" PDF_FORMAT_INT64 " Objects.\n", nFirstObject, nNumObjects );
+ PdfError::DebugMessage("Reading XRef Section: %" PDF_FORMAT_INT64 " with %"
+ PDF_FORMAT_INT64 " Objects.\n", nFirstObject, nNumObjects );
#endif // PODOFO_VERBOSE_DEBUG
- if ( nFirstObject + nNumObjects > m_nNumObjects )
+ if ( nFirstObject < 0 )
+ PODOFO_RAISE_ERROR_INFO( ePdfError_ValueOutOfRange,
+ "ReadXRefSubsection: nFirstObject is negative" );
+ if ( nNumObjects < 0 )
+ PODOFO_RAISE_ERROR_INFO( ePdfError_ValueOutOfRange,
+ "ReadXRefSubsection: nNumObjects is negative" );
+
+ const pdf_int64 maxNum
+ = static_cast<pdf_int64>(PdfParser::s_nMaxObjects);
+
+ // overflow guard, fixes CVE-2017-5853 (signed integer overflow)
+ // also fixes CVE-2017-6844 (buffer overflow) together with below size check
+ if( (maxNum >= nNumObjects) && (nFirstObject <= maxNum - nNumObjects) )
{
- // Total number of xref entries to read is greater than the /Size
- // specified in the trailer if any. That's an error unless we're trying
- // to recover from a missing /Size entry.
- PdfError::LogMessage( eLogSeverity_Warning,
- "There are more objects (%" PDF_FORMAT_INT64 ") in this XRef table than "
- "specified in the size key of the trailer directory (%" PDF_FORMAT_INT64 ")!\n",
- nFirstObject + nNumObjects, m_nNumObjects );
+ if( nFirstObject + nNumObjects > m_nNumObjects )
+ {
+ // Total number of xref entries to read is greater than the /Size
+ // specified in the trailer if any. That's an error unless we're
+ // trying to recover from a missing /Size entry.
+ PdfError::LogMessage( eLogSeverity_Warning,
+ "\nThere are more objects (%" PDF_FORMAT_INT64 ") in this XRef "
+ "table than specified in the size key of the trailer directory "
+ "(%" PDF_FORMAT_INT64 ")!\n", nFirstObject + nNumObjects,
+ static_cast<pdf_int64>( m_nNumObjects ));
+ }
+ // this should fix CVE-2017-5855 together with the overflow guard above
+ if ( static_cast<pdf_uint64>( nFirstObject + nNumObjects )
+ // cast necessary to avoid signed/unsigned comparison (GCC warning)
+ > std::numeric_limits<size_t>::max() ) // if true, not resize-safe
+ PODOFO_RAISE_ERROR_INFO( ePdfError_ValueOutOfRange,
+ "xref subsection's given entry numbers together too large" );
#ifdef _WIN32
m_nNumObjects = static_cast<long>(nFirstObject + nNumObjects);
@@ -768,7 +791,16 @@ void PdfParser::ReadXRefSubsection( pdf_
m_nNumObjects = nFirstObject + nNumObjects;
m_offsets.resize(nFirstObject+nNumObjects);
#endif // _WIN32
- }
+
+ }
+ else
+ {
+ PdfError::LogMessage( eLogSeverity_Error, "There are more objects (%"
+ PDF_FORMAT_INT64 " + %" PDF_FORMAT_INT64 " seemingly) in this XRef"
+ " table\nthan supported by standard PDF, or it's inconsistent.\n",
+ nFirstObject, nNumObjects);
+ PODOFO_RAISE_ERROR( ePdfError_InvalidXRef );
+ }
// consume all whitespaces
int charcode;
#include <iostream>
#include "podofo/podofo.h"
int main()
{
PoDoFo::PdfVecObjects testObjVec;
PoDoFo::PdfParser p(&testObjVec, "00144-podofo-signintoverflow-PdfParser");
std::cout << "object count in PoC: " << testObjVec.GetObjectCount();
std::cout << std::endl;
return 0;
}
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Podofo-users mailing list
Podofo-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/podofo-users
