Hi Mark, hi all,
> Mark Rogers <mark.rog...@powermapper.com> has written on 8 April 2017 at
> 10:01:
>
> Hi
>
> There are the CVEs in ReadXRefSubsection:
>
> CVE-2017-6844 global buffer overflow in PdfParser::ReadXRefSubsection
> CVE-2017-5855 NULL pointer dereference in PdfParser::ReadXRefSubsection
> CVE-2017-5853 signed integer overflow in PdfParser.cpp
>
> CVE-2017-6844 and CVE-2017-5853 are caused by 3 related problems:
>
> [snip ...]
your explanation is appreciated, I just snipped it for conserving space.
>
> I don’t think mabri’s patch handles all of these cases because it still
> has the nFirstObject + nNumObjects addition without an overflow guard
>
I don't think so: In line 18 of my patch you replied to (line 22 of my new
patch attached attached here) there is AFAIK an overflow guard, and it should
work without changes to PdfParser's member variable types like your (Mark's)
patch has and which AFAIK (could) affect binary compatibility.
IMHO for security updates that kind of compatibility is important.
About overflow guards, if mine isn't one I'd welcome some well-reasoned and
-sourced instruction about how to write them correctly.
My (new) patch is meant to be applicable to all recent branches (0.94, 0.9.5)
in addition to the trunk.
> I’ve attached a patch that should resolve the 32-bit and 64-bit overflows
> above (CVE-2017-6844 and CVE-2017-5853)
>
If I may criticize your patch: it contains superfluous asserts (pdf_int64 is
always the same size as itself if the compiler isn't seriously buggy or there
is undefined behaviour, in which latter case they don't help AFAIK) and as I
mentioned above the type changes are bad for (binary/drop-in) compatibility
(also AFAIK, instruction also welcomed as per above).
> The patch may also resolve CVE-2017-5855, but I’ve not been able to confirm
> that yet.
>
> Best Regards
> Mark
>
> --
> Mark Rogers - mark.rog...@powermapper.com
> PowerMapper Software Ltd - www.powermapper.com
> Registered in Scotland No 362274 Quartermile 2 Edinburgh EH3 9GL
>
> On 08/04/2017, 01:16, "Matthew Brincke" <ma...@mailbox.org> wrote:
>
> Hi zyx, hi all,
>
> I've addressed the below concerns, and run-tested as I could, pity
> my GCC 5.2.1 didn't find a difference with/out patch with the option
> -fsanitize=undefined, except for my diagnostic message ;-(. I used
> the same test program as earlier. Please review the patch attached.
>
> > zyx <z...@litepdf.cz> has written on 28 February 2017 at 08:38:
> >
> > On Tue, 2017-02-28 at 00:14 +0100, Matthew Brincke wrote:
> >
> > > I haven't completed testing yet
> >
> > Hi,
> > thanks for the patch. Just from a quick read of the proposed change:
> >
> > > * const pdf_int64 maxNum
> > > * = static_cast(std::numeric_limits::max());
> >
> > As far as I know, 'long' type is architectural dependant, 32 bits on
> > 32bit arch and 64 bits on 64bit arch, thus it produces different
> > values. Avoiding a 'long' usage might be a general benefit.
> >
> > > * "(%ld)!\n",
> > > * nFirstObject + nNumObjects, m_nNumObjects ); // 2nd arg is long!
> >
> > The %ld is incorrect for the same reason. There are defines for proper
> > formats, or cast the second argument to pdf_int64 instead and use the
> > format specifier as before.
> >
> > > * ") in this XRef table than supported by this version of PoDoFo, "
> >
> > This sounds odd to me, are you sure it's about what PoDoFo supports,
> > not about what the standard supports? I mean, the standard suggests to
> > stay in those limits even if the writer runs on a system which can
> > cover more objects, to be compatible with 32-bit systems (because you
> > never know on which system the reader runs).
> > Bye,
> > zyx
>
--- PdfParser.cpp (revision 1837)
+++ PdfParser.cpp (working copy)
@@ -745,21 +745,34 @@ 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 );
#endif // PODOFO_VERBOSE_DEBUG
- if ( nFirstObject + nNumObjects > m_nNumObjects )
+ PODOFO_ASSERT( nFirstObject > 0 );
+ PODOFO_ASSERT( nNumObjects > 0 );
+ PODOFO_ASSERT( sizeof(PdfParser::s_nMaxObjects) <= sizeof(size_t) );
+
+ const pdf_int64 maxNum
+ = static_cast<pdf_int64>(PdfParser::s_nMaxObjects);
+
+ // overflow guard, fixes CVE-2017-5853, CVE-2017-5855 and CVE-2017-6844
+ 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 ));
+ }
+ PODOFO_ASSERT( nFirstObject + nNumObjects <= SIZE_MAX ); // resize-safe
#ifdef _WIN32
m_nNumObjects = static_cast<long>(nFirstObject + nNumObjects);
@@ -768,7 +781,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
