#20 CVE-2018-11255 - NULL pointer dereference in PdfPage::GetPageNumber()

[Mattia Rizzolo]

https://security-tracker.debian.org/tracker/CVE-2018-11255
PoC: https://bugzilla.redhat.com/show_bug.cgi?id=1575502

The function PdfPage::GetPageNumber() in PdfPage.cpp:538 in PoDoFo 0.9.5 allows remote attackers to cause a denial of service (NULL pointer dereference and application crash) via a crafted PDF document.

[Matthew Brincke]

I couldn't reproduce this with the given PoC in svn r1937 (built with GCC 4.9). There is a PdfError exception being thrown in tools/podopdfinfo/pdfinfo.cpp:212 with the code ePdfError_PageNotFound, i.e. PdfDocument::GetPage(int) returned NULL already.

[Matthew Brincke]

I don't have a working PoC yet, sorry, so this is pending verification of the fix committed (by me) in svn r1952. I committed because AFAICS it's easier for me to fix the issue (hopefully correctly) than to came up with a PoC right now.

[Mattia Rizzolo]

Indeed I confirm I can't reproduce anymore this crash.

I haven't done a full bisect, but I believe it is fixed by one of 1840, 1836, 1837, 1842 - most likely 1836 :)

Incidentally, reverting that commit 1836 and including 1952 that you thought would properly fix the crash (instead of working it around in the client like 1836 does), still causes the crash, so I think your fix there is not enough.

[Matthew Brincke]

[apparently posting here by sending a reply a notification doesn't work even though the issue number is in the email address the notification comes from]

I don't think the change in svn r1836 is a workaround because
PdfDocument::GetPage(int) can return NULL: it's not documented
what happens when a page (or page object) isn't found, so unless
you mean a proper fix would be amending the documentation (and,
if it says to not return on error, but throw an exception, change
the implementation to match), the NULL return needs to be checked
for to prevent undefined behaviour (using a NULL page pointer to
call the method) in the following call to PdfPage::GetMediaBox().

AFAICS, the PdfDocument::GetPage(int) method doesn't call, either
directly or indirectly, PdfPage::GetPageNumber() so I further don't
think a crash dependent on svn r1836 being backed out is the same
issue as this #20 or the newer (reported about 0.9.6) issue #37 which
IMHO (I haven't verified yet) is a duplicate of this one. Therefore,
unless there's a hole in my argument, the crash observed by you doesn't
mean the fix for this issue in svn r1952 needs to be amended to work.

[Matthew Brincke]

I don't think the change in svn r1836 is a workaround because
PdfDocument::GetPage(int) can return NULL: it's not documented
what happens when a page (or page object) isn't found, so unless
you mean a proper fix would be amending the documentation (and,
if it says to not return on error, but throw an exception, change
the implementation to match), the NULL return needs to be checked
for to prevent undefined behaviour (using a NULL page pointer to
call the method) in the following call to PdfPage::GetMediaBox().

AFAICS, the PdfDocument::GetPage(int) method doesn't call, either
directly or indirectly, PdfPage::GetPageNumber() so I further don't
think a crash dependent on svn r1836 being backed out is the same
issue as this #20 or the newer (reported about 0.9.6) issue #37 which
IMHO (I haven't verified yet) is a duplicate of this one. Therefore,
unless there's a hole in my argument, the crash observed by you doesn't
mean the fix for this issue in svn r1952 needs to be amended to work.

[Matthew Brincke]

The svn r1836 fixes the Debian bug #854605 (as you had noted there). The PoC given above seems to rather be one for that bug. The change isn't "working it around in the client" because in C++ methods can't check whether they're called on a null object pointer (undefined behaviour is invoked by the call already and nothing done afterwards can heal it IIRC). To say "still causes the crash" in a comment on this issue (to which I'm replying here) without mentioniong that other bug https://bugs.debian.org/854605 as its cause misled me to think you mean the former rather than the latter, therefore I was rather confused.

[Mattia Rizzolo]

What I mean is that also the library should guard against functions passing invalid (NULL) values to it. Whether that means returning back NULL or throwing an exception is up to you, but really it should not segfault within the library code, no matter how clumsy the client is with null checking itself.

Indeed #37 looks like a duplicate of this one from a first glance.

[Mattia Rizzolo]

That said, after talking with my peers, I've been convinced that it's not so strictly necessary for the lib to be that safe… Actually that's not even expected.

Maybe it was just me being on the wrong train of thought here...

[Matthew Brincke]

With the reproduction recipe detailed at the
given "PoC" URL (invoking podofopdfinfo with
the PoC), svn r1820 (exactly version 0.9.5,
just not packaged) gives a null-pointer
dereference segfault in PdfInfo::GuessFormat()
with the PoC file attached there (crash1.pdf),
so this issue does not directly reproduce for me.
Compiler: GCC 4.8.4 on LMDE 2 (glibc 2.19) in a
firejail 0.9.44.8 sandbox.

[Matthew Brincke]

I've managed to reproduce this issue with svn r1820 (version 0.9.5 from svn) by changing the given PoC to avoid triggering the crash in PdfInfo::GuessFormat(). The changed PoC is attached. All else is unchanged except for an extra -ggdb option to g++ for line-number info in the debug information (both, with the old "PoC" and this, done in C++11 mode). The backtrace is (ASan output):

==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000018 (pc 0x7ff0261c92a6 sp 0x7ffe7dee7650 bp 0x7ffe7dee7660 T0)
    #0 0x7ff0261c92a5 in PoDoFo::PdfVariant::DelayedLoad() const /home/mabri/podofo-code/podofo/base/../../src/base/PdfVariant.h:545
    #1 0x7ff0261cbef7 in PoDoFo::PdfVariant::GetDictionary() /home/mabri/podofo-code/podofo/base/../../src/base/PdfVariant.h:851
    #2 0x7ff02628800d in PoDoFo::PdfPage::GetPageNumber() const /home/mabri/podofo-code/src/doc/PdfPage.cpp:538
    #3 0x406874 in PdfInfo::OutputPageInfo(std::ostream&) /home/mabri/podofo-code/tools/podofopdfinfo/pdfinfo.cpp:87
    #4 0x409f3c in main /home/mabri/podofo-code/tools/podofopdfinfo/podofopdfinfo.cpp:133
    #5 0x7ff02430fb44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b44)
    #6 0x405e98 (/home/mabri/podofo-code/build-r1820-issue20/tools/podofopdfinfo/podofopdfinfo+0x405e98)

[Matthew Brincke]

I can't reproduce this being fixed with the PoC crash-issue20.pdf anymore, because a different exception message is produced (very probably earlier, i.e. the program podofopdfinfo doesn't get to the code location this issue was in). Instead in PdfMemDocument.cpp:198 the error information is "Catalog object not found!"

[Matthew Brincke]

I've done a full bisection: r1900 (#20 present) -> r1963 (#20 absent, exception code 16 ePdfError_NoObject, info with the PoC crash-issue20.pdf: Object 35 0 R not found from Kids array 26 0 R) -> r1943 (#20 present) -> r1952 (not the "pure teaching" of binary search, but I knew where to look: absent, same code/info) -> r1951 (#20 present).

[Matthew Brincke]

I've now also done a full bisection from r1963 (already tested) -> r1975 (like current) -> r1969 (reproduced fixed) -> r1972 (also) -> r1973 (like current), so it turns out this is caused by svn r1973 "Patch by F.E. (mostly): Check xref entries' length, valid line-termination chars" which brought a stricter check of XRef entries. IMHO it's a bug (but a different one, this doesn't deserve to stay open) that the exception thrown thrown there isn't properly output before leading to an ePdfError_NoObject further up the stack.

[Matthew Brincke]

Now is the time to speak up: is this still a bug in current trunk? I'd like to close this issue the day after tomorrow (UTC) at the latest.

[Matthew Brincke]

I'll test this today (on two GNU/Linux systems) and if I don't find anything related to this issue, I'll close (so please test now where I can't: e.g. on Windows, to possibly find something and note it here so I can see anything why I shouldn't close). I'm going to start the tests ca. 4pm UTC.

[Matthew Brincke]

As already written, this should be closed because all the questions I had are now answered and it was open for long enough time to let anyone on another platform (e.g. Windows) chime in with their observations if there were any differences to my GNU/Linux.