Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segmentation Fault (stack exhaustion) on a crafted PDF file #202

Closed
pushdword opened this issue Apr 10, 2018 · 7 comments
Closed

Segmentation Fault (stack exhaustion) on a crafted PDF file #202

pushdword opened this issue Apr 10, 2018 · 7 comments

Comments

@pushdword
Copy link

pushdword commented Apr 10, 2018

A crafted pdf file causes a segmentation fault, stack-overflow reported by LLVM ASan - but it seems stack exhaustion, causing a denial of service and there is a chance of code execution.
qpdflibsegfault.zip
Details are attached inside the zip called 'gdb log'.
I have tested with libqpdf and qpdf itself, debugged and used LLVM ASan to identify the possible root cause.
A pdf POC is inside the zip.
Versions tested: 6.0.0 and 8.0.2.
c code to test:

QPDF pdf;
pdf.processFile("crafted pdf.pdf"); //segmentation fault here.

running:

== simple source code using libqpdf ==
$ cat openpdf.cc
#include
#include <string.h>
#include <stdlib.h>
#include <qpdf/QPDF.hh>
#include <qpdf/QUtil.hh>
#include <stdint.h>

int main(void)
{
int pageno = 2;
try
{
QPDF pdf;
pdf.processFile("fuzzed.pdf");
std::vector pages = pdf.getAllPages();
if ((pageno < 1) || (static_cast<size_t>(pageno) > pages.size()))
{
exit(1);
}
}
catch (std::exception& e)
{
return 0;
}
return 0;
}
$ clang++-6.0 -L/usr/local/lib -lz -ljpeg -lqpdf -lpthread *.cc -g -o openpdf
$ ./openpdf
WARNING: fuzzed.pdf (trailer, offset 191702): expected dictionary key but found non-name object; inserting key /QPDFFake1
WARNING: fuzzed.pdf (trailer, offset 191700): expected dictionary key but found non-name object; inserting key /QPDFFake1
WARNING: fuzzed.pdf (trailer, offset 191698): expected dictionary key but found non-name object; inserting key /QPDFFake1
(...)
WARNING: fuzzed.pdf (trailer, offset 328678): unknown token while reading object; treating as string
WARNING: fuzzed.pdf (trailer, offset 328700): unexpected EOF
WARNING: fuzzed.pdf (trailer, offset 328700): parse error while reading object
Segmentation fault (core dumped)

Testing qpdf 6.0.0:

[pushdword@localhost qpdflib]$ qpdf fuzzedpdf.pdf test.pdf
Segmentation fault (core dumped)
[pushdword@localhost qpdflib]$ qpdf --version
qpdf version 6.0.0
Copyright (c) 2005-2015 Jay Berkenbilt
This software may be distributed under the terms of version 2 of the
Artistic License which may be found in the source distribution. It is
provided "as is" without express or implied warranty.

testing qpdf 8.0.2

$ qpdf fuzzed.pdf test.pdf
WARNING: fuzzed.pdf (trailer, offset 191702): expected dictionary key but found non-name object; inserting key /QPDFFake1
WARNING: fuzzed.pdf (trailer, offset 191700): expected dictionary key but found non-name object; inserting key /QPDFFake1
WARNING: fuzzed.pdf (trailer, offset 191698): expected dictionary key but found non-name object; inserting key /QPDFFake1
WARNING: fuzzed.pdf (trailer, offset 191696): expected dictionary key but found non-name object; inserting key /QPDFFake1
(... a very long warning repetition ...)
WARNING: fuzzed.pdf (trailer, offset 54774): expected dictionary key but found non-name object; inserting key /QPDFFake1
WARNING: fuzzed.pdf (trailer, offset 328678): unknown token while reading object; treating as string
WARNING: fuzzed.pdf (trailer, offset 328700): unexpected EOF
WARNING: fuzzed.pdf (trailer, offset 328700): parse error while reading object
Segmentation fault (core dumped)

@pushdword
Copy link
Author

Use CVE-2018-9918

@pushdword pushdword changed the title Segmentation Fault (stack-overflow) on a crafted PDF file Segmentation Fault (stack exhaustion) on a crafted PDF file Apr 11, 2018
@jberkenbilt
Copy link
Contributor

Thanks. I'll take a look when I can. Pretty soon.

@jberkenbilt
Copy link
Contributor

How depressing. I put a lot of effort into getting rid of recursion in parsing to avoid falling for this kind of trap. Now qpdf recovers well enough that it is actually able to successfully parse the file, and then it all blows up because delete ends up recursively calling destructors, which is what is blowing out the stack. What's happening is basically the same as in this program:

class A
{
  public:
    A(A* a)
    {
        this->a = a;
    }
    ~A()
    {
        delete a;
    }
  private:
    A* a;
};

int main()
{
    A* a = new A(0);
    for (int i = 0; i < 300000; ++i)
    {
        a = new A(a);
    }
    delete a;
    return 0;
}

Even though the recursive data structure is created iteratively, destruction is recursive. This happens to be an invalid file because there are dictionaries without keys that qpdf is supplying fake keys for, but you could create this exact problem using a syntactically valid PDF that just has a very deeply nested object. For example, changing each << and >> to [ and ] and putting it in a valid spot in the file, such as as the value of a unused dictionary key in trailer, would produce a valid file that would cause this same problem. A core feature of qpdf is that it uses smart pointers and STL data structures so that the user almost never has to think about memory allocation. It is not clear to me how I'm going to fix this other than perhaps imposing some arbitrary limit on the depth that any object is allowed to go to.

@jberkenbilt
Copy link
Contributor

I'm going to fix it by just imposing an arbitrary limit on how deeply nested the data structure expressed in a direct object can be. This is a very easy fix.

@jberkenbilt
Copy link
Contributor

In lieu of immediately releasing 8.0.3, I am going to prepare a patch relative to 8.0.2 that fixes this. It will be this commit without the additional test case.

@jberkenbilt
Copy link
Contributor

Index: qpdf/ChangeLog
===================================================================
--- qpdf.orig/ChangeLog
+++ qpdf/ChangeLog
@@ -1,3 +1,8 @@
+2018-04-15  Jay Berkenbilt  <ejb@ql.org>
+
+	* Arbitrarily limit the depth of data structures represented by
+	direct object. This is CVE-2018-9918. Fixes #202.
+
 2018-03-06  Jay Berkenbilt  <ejb@ql.org>
 
 	* 8.0.2: release
Index: qpdf/libqpdf/QPDFObjectHandle.cc
===================================================================
--- qpdf.orig/libqpdf/QPDFObjectHandle.cc
+++ qpdf/libqpdf/QPDFObjectHandle.cc
@@ -1487,12 +1487,26 @@ QPDFObjectHandle::parseInternal(PointerH
 
 	  case QPDFTokenizer::tt_array_open:
 	  case QPDFTokenizer::tt_dict_open:
-            olist_stack.push_back(std::vector<QPDFObjectHandle>());
-            state = st_start;
-            offset_stack.push_back(input->tell());
-            state_stack.push_back(
-                (token.getType() == QPDFTokenizer::tt_array_open) ?
-                st_array : st_dictionary);
+            if (olist_stack.size() > 500)
+            {
+		QTC::TC("qpdf", "QPDFObjectHandle too deep");
+                warn(context,
+                     QPDFExc(qpdf_e_damaged_pdf, input->getName(),
+                             object_description,
+                             input->getLastOffset(),
+                             "ignoring excessively deeply nested data structure"));
+                object = newNull();
+                state = st_top;
+            }
+            else
+            {
+                olist_stack.push_back(std::vector<QPDFObjectHandle>());
+                state = st_start;
+                offset_stack.push_back(input->tell());
+                state_stack.push_back(
+                    (token.getType() == QPDFTokenizer::tt_array_open) ?
+                    st_array : st_dictionary);
+            }
 	    break;
 
 	  case QPDFTokenizer::tt_bool:
Index: qpdf/qpdf/qpdf.testcov
===================================================================
--- qpdf.orig/qpdf/qpdf.testcov
+++ qpdf/qpdf/qpdf.testcov
@@ -335,3 +335,4 @@ QPDFObjectHandle numeric non-numeric 0
 QPDFObjectHandle erase array bounds 0
 qpdf-c called qpdf_check_pdf 0
 QPDF xref loop 0
+QPDFObjectHandle too deep 0
Index: qpdf/qpdf/qtest/qpdf/issue-146.out
===================================================================
--- qpdf.orig/qpdf/qtest/qpdf/issue-146.out
+++ qpdf/qpdf/qtest/qpdf/issue-146.out
@@ -1,7 +1,5 @@
 WARNING: issue-146.pdf: file is damaged
 WARNING: issue-146.pdf: can't find startxref
 WARNING: issue-146.pdf: Attempting to reconstruct cross-reference table
-WARNING: issue-146.pdf (trailer, offset 20728): unknown token while reading object; treating as string
-WARNING: issue-146.pdf (trailer, offset 20732): unexpected EOF
-WARNING: issue-146.pdf (trailer, offset 20732): parse error while reading object
+WARNING: issue-146.pdf (trailer, offset 695): ignoring excessively deeply nested data structure
 issue-146.pdf: unable to find trailer dictionary while recovering damaged file

@pushdword
Copy link
Author

Hello Jay,
Thanks for fixing :)

woodsts pushed a commit to woodsts/buildroot that referenced this issue Jun 30, 2018
Fixes CVE-2018-9918: mishandle certain "expected dictionary key but
found non-name object" cases, allowing remote attackers to cause a
denial of service (stack exhaustion)

qpdf/qpdf#202

Drop local SHA256 hash since we use upstream provided SHA512.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this issue Jul 19, 2018
Fixes CVE-2018-9918: mishandle certain "expected dictionary key but
found non-name object" cases, allowing remote attackers to cause a
denial of service (stack exhaustion)

qpdf/qpdf#202

Drop local SHA256 hash since we use upstream provided SHA512.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
(cherry picked from commit 473390a)
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
woodsts pushed a commit to woodsts/buildroot that referenced this issue Jul 19, 2018
Fixes CVE-2018-9918: mishandle certain "expected dictionary key but
found non-name object" cases, allowing remote attackers to cause a
denial of service (stack exhaustion)

qpdf/qpdf#202

Drop local SHA256 hash since we use upstream provided SHA512.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
(cherry picked from commit 473390a)
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants