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 1837)
+++ PdfParser.cpp (working copy)
@@ -745,21 +745,43 @@ 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 ( nFirstObject + nNumObjects > std::numeric_limits<size_t>::max() )
+ // not resize-safe
+ PODOFO_ERROR_INFO( ePdfError_ValueOutOfRange,
+ "xref subsection's given entry numbers together too large" );
#ifdef _WIN32
m_nNumObjects = static_cast<long>(nFirstObject + nNumObjects);
@@ -768,7 +790,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;
------------------------------------------------------------------------------
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
