Re: [poppler] [PATCH] Catalog::getNumPages(): validate page count

[Even Rouault]

Hi,

here's a V2 implementing suggested changes.

Best regards,

Even

-- 
Spatialys - Geospatial professional services
http://www.spatialys.com
From 6e9a9a45550a320b97197f9c0142e8d9cd2b1f24 Mon Sep 17 00:00:00 2001
From: Even Rouault 
Date: Fri, 11 Sep 2015 13:56:05 +0200
Subject: [PATCH 2/2] Catalog::cachePageTree(): recover from out of memory condition

---
 poppler/Catalog.cc |   12 +---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/poppler/Catalog.cc b/poppler/Catalog.cc
index 2426bfd..cc579fc 100644
--- a/poppler/Catalog.cc
+++ b/poppler/Catalog.cc
@@ -171,8 +171,8 @@ Catalog::~Catalog() {
   }
 }
 gfree(pages);
-gfree(pageRefs);
   }
+  gfree(pageRefs);
   names.free();
   dests.free();
   delete destNameTree;
@@ -305,8 +305,14 @@ GBool Catalog::cachePageTree(int page)
 }
 
 pagesSize = getNumPages();
-pages = (Page **)gmallocn(pagesSize, sizeof(Page *));
-pageRefs = (Ref *)gmallocn(pagesSize, sizeof(Ref));
+pages = (Page **)gmallocn_checkoverflow(pagesSize, sizeof(Page *));
+pageRefs = (Ref *)gmallocn_checkoverflow(pagesSize, sizeof(Ref));
+if (pages == NULL || pageRefs == NULL ) {
+  error(errSyntaxError, -1, "Cannot allocate page cache");
+  pagesDict->decRef();
+  pagesSize = 0;
+  return gFalse;
+}
 for (int i = 0; i < pagesSize; ++i) {
   pages[i] = NULL;
   pageRefs[i].num = -1;
-- 
1.7.0.4

From cc882151563b50acdf827401add81c27bb4fbd23 Mon Sep 17 00:00:00 2001
From: Even Rouault 
Date: Fri, 11 Sep 2015 13:30:32 +0200
Subject: [PATCH 1/2] Catalog::getNumPages(): validate page count

---
 poppler/Catalog.cc |   12 
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/poppler/Catalog.cc b/poppler/Catalog.cc
index 04caa1c..2426bfd 100644
--- a/poppler/Catalog.cc
+++ b/poppler/Catalog.cc
@@ -856,6 +856,18 @@ int Catalog::getNumPages()
   }
 } else {
   numPages = (int)obj.getNum();
+  if (numPages <= 0 ) {
+error(errSyntaxError, -1,
+  "Invalid page count {0:d}", numPages);
+numPages = 0;
+  }
+  else if (numPages > xref->getNumObjects()) {
+error(errSyntaxError, -1,
+  "Page count ({0:d}) larger than number of objects ({1:d})",
+  numPages, xref->getNumObjects());
+numPages = 0;
+  }
+
 }
 
 catDict.free();
-- 
1.7.0.4

___
poppler mailing list
poppler@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/poppler

Re: [poppler] [PATCH] Catalog::getNumPages(): validate page count

[Albert Astals Cid]

El Divendres, 11 de setembre de 2015, a les 13:59:51, Even Rouault va 
escriure:
> Hi,
> 
> here's a V2 implementing suggested changes.

Do you have files that show the need of these patches?

Cheers,
  Albert

> 
> Best regards,
> 
> Even

___
poppler mailing list
poppler@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/poppler

Re: [poppler] [PATCH] Catalog::getNumPages(): validate page count

[Jason Crain]

On 2015-09-11 06:59, Even Rouault wrote:

Hi,

here's a V2 implementing suggested changes.

Best regards,

Even


From 0001-Catalog-getNumPages-validate-page-count.patch:

@@ -856,6 +856,18 @@ int Catalog::getNumPages()
   }
 } else {
   numPages = (int)obj.getNum();
+  if (numPages <= 0 ) {
+error(errSyntaxError, -1,
+  "Invalid page count {0:d}", numPages);
+numPages = 0;
+  }
+  else if (numPages > xref->getNumObjects()) {
+error(errSyntaxError, -1,
+  "Page count ({0:d}) larger than number of objects 
({1:d})",

+  numPages, xref->getNumObjects());
+numPages = 0;
+  }
+
 }

 catDict.free();


I know I said I liked the idea of comparing the number of pages to the 
number
of objects, but on second thought, I think I could make a PDF that was 
just
many copies of the same page so it would be a completely valid PDF that 
has
more pages than objects.  I don't know of a good way to validate the 
page
count.  Even going through the page tree might be hard to do right 
without

leading to an infinite loop, in addition to being slow.
___
poppler mailing list
poppler@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/poppler

Re: [poppler] [PATCH] Catalog::getNumPages(): validate page count

[Even Rouault]

Le vendredi 11 septembre 2015 14:57:54, Albert Astals Cid a écrit :
> El Divendres, 11 de setembre de 2015, a les 13:59:51, Even Rouault va
> 
> escriure:
> > Hi,
> > 
> > here's a V2 implementing suggested changes.
> 
> Do you have files that show the need of these patches?

Sure. Completely made on purpose admitedly. Feel free to use/distribute it as 
needed.

My aim was to test GDAL robustness (with poppler backend for PDF driver) 
against hostile files.

> 
> Cheers,
>   Albert
> 
> > Best regards,
> > 
> > Even

-- 
Spatialys - Geospatial professional services
http://www.spatialys.com


one_billion_page.pdf
Description: Adobe PDF document
___
poppler mailing list
poppler@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/poppler