Bug#729064: [Secure-testing-team] Bug#729064: poppler: CVE-2013-4473 CVE-2013-4474

2013-11-17 Thread Pino Toscano 
Hi,

In data martedì 12 novembre 2013 23:47:21, hai scritto:
 On Fri, Nov 8, 2013 at 8:32 AM, Moritz Muehlenhoff wrote:
  Two security issues were found in the pdfseparate tool shipped by 
  poppler-utils:
 Hi, I've uploaded an nmu fixing these two issue to delayed/5.  Please
 see attached patch.

Unfortunately, one of your patches introduces the same issues it is
supposed to fix:

 +@@ -65,9 +66,37 @@
 +   if (firstPage == 0)
 + firstPage = 1;
 +   if (firstPage != lastPage  strstr(destFileName, %d) == NULL) {
 +-error(-1, '%s' must contain '%%d' if more than one page should be 
 extracted, destFileName);
 ++error(-1, '%s' must contain '%d' if more than one page should be 
 extracted, destFileName);
 + return false;

error() in poppler  0.19 takes a printf-like format, so changing from
%%d to %d will make printf expect an int, which is not passed as
argument (and thus a we run into a new format string issue).
For the same reason, also...

 ++  if (p != NULL) {
 ++error(-1, '%s' can only contain one '%d' pattern, destFileName);
 ++free(auxDestFileName);
 ++return false;
 ++  }

... this error() contains the same issue.

Oh, and btw:

 +poppler (0.18.4-8+nmu1) unstable; urgency=high

The NMU version is wrong, since it is not a native package; it should
have been 0.18.4-8.1 instead, as also DevRef §5.11.2 says (but I see
you spread this wrong versioning when NMUing, so hardly something you
will change...)

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.

Bug#729064: [Secure-testing-team] Bug#729064: poppler: CVE-2013-4473 CVE-2013-4474

2013-11-17 Thread Michael Gilbert 
On Sun, Nov 17, 2013 at 1:31 PM, Pino Toscano wrote:
 Unfortunately, one of your patches introduces the same issues it is
 supposed to fix:

 +@@ -65,9 +66,37 @@
 +   if (firstPage == 0)
 + firstPage = 1;
 +   if (firstPage != lastPage  strstr(destFileName, %d) == NULL) {
 +-error(-1, '%s' must contain '%%d' if more than one page should be 
 extracted, destFileName);
 ++error(-1, '%s' must contain '%d' if more than one page should be 
 extracted, destFileName);
 + return false;

 error() in poppler  0.19 takes a printf-like format, so changing from
 %%d to %d will make printf expect an int, which is not passed as
 argument (and thus a we run into a new format string issue).

Thanks for spotting and correcting that.

 Oh, and btw:

 +poppler (0.18.4-8+nmu1) unstable; urgency=high

 The NMU version is wrong, since it is not a native package; it should
 have been 0.18.4-8.1 instead, as also DevRef §5.11.2 says (but I see
 you spread this wrong versioning when NMUing, so hardly something you
 will change...)

Remember developers reference is a set of guidelines.  The switch over
to +debXuY for stable uploads opened up more flexibility to use a
consistent versioning scheme for both native and non-native packages.

There was discussion on developers-reference about that over a year
ago that may be worth catching up on.

Best wishes,
Mike


--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#729064: [Secure-testing-team] Bug#729064: poppler: CVE-2013-4473 CVE-2013-4474

2013-11-12 Thread Michael Gilbert 
control: tag -1 patch
control: tag -1 pending

On Fri, Nov 8, 2013 at 8:32 AM, Moritz Muehlenhoff wrote:
 Two security issues were found in the pdfseparate tool shipped by 
 poppler-utils:

Hi, I've uploaded an nmu fixing these two issue to delayed/5.  Please
see attached patch.

Best wishes,
Mike
diff -Nru poppler-0.18.4/debian/changelog poppler-0.18.4/debian/changelog
--- poppler-0.18.4/debian/changelog	2013-08-20 13:12:45.0 -0400
+++ poppler-0.18.4/debian/changelog	2013-11-10 16:13:27.0 -0500
@@ -1,3 +1,11 @@
+poppler (0.18.4-8+nmu1) unstable; urgency=high
+
+  * Non-maintainer upload by the Security Team.
+  * Fix cve-2013-4473 and cve-2013-4474: buffer overflow and format string
+issues in the pdfseparate tool (closes: 729064).
+
+ -- Michael Gilbert mgilb...@debian.org  Sun, 10 Nov 2013 20:42:23 +
+
 poppler (0.18.4-8) unstable; urgency=low
 
   * Remove the .la files from debian/tmp, to shorten the --list-missing output.
diff -Nru poppler-0.18.4/debian/patches/cve-2013-4473.patch poppler-0.18.4/debian/patches/cve-2013-4473.patch
--- poppler-0.18.4/debian/patches/cve-2013-4473.patch	1969-12-31 19:00:00.0 -0500
+++ poppler-0.18.4/debian/patches/cve-2013-4473.patch	2013-11-10 16:13:33.0 -0500
@@ -0,0 +1,24 @@
+description: buffer overflow in pdfseparate tool
+origin: https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2013-4473
+Index: poppler-0.18.4/utils/pdfseparate.cc
+===
+--- poppler-0.18.4.orig/utils/pdfseparate.cc	2013-11-10 21:08:46.366640853 +
 poppler-0.18.4/utils/pdfseparate.cc	2013-11-10 21:08:46.366640853 +
+@@ -43,7 +43,7 @@
+ };
+ 
+ bool extractPages (const char *srcFileName, const char *destFileName) {
+-  char pathName[1024];
++  char pathName[4096];
+   GooString *gfileName = new GooString (srcFileName);
+   PDFDoc *doc = new PDFDoc (gfileName, NULL, NULL, NULL);
+ 
+@@ -69,7 +69,7 @@
+ return false;
+   }
+   for (int pageNo = firstPage; pageNo = lastPage; pageNo++) {
+-sprintf (pathName, destFileName, pageNo);
++snprintf (pathName, sizeof (pathName) - 1, destFileName, pageNo);
+ GooString *gpageName = new GooString (pathName);
+ int errCode = doc-savePageAs(gpageName, pageNo);
+ if ( errCode != errNone) {
diff -Nru poppler-0.18.4/debian/patches/cve-2013-4474.patch poppler-0.18.4/debian/patches/cve-2013-4474.patch
--- poppler-0.18.4/debian/patches/cve-2013-4474.patch	1969-12-31 19:00:00.0 -0500
+++ poppler-0.18.4/debian/patches/cve-2013-4474.patch	2013-11-10 16:14:12.0 -0500
@@ -0,0 +1,53 @@
+description: format string issue
+origin: https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2013-4474
+Index: poppler-0.18.4/utils/pdfseparate.cc
+===
+--- poppler-0.18.4.orig/utils/pdfseparate.cc	2013-11-10 21:13:42.114643334 +
 poppler-0.18.4/utils/pdfseparate.cc	2013-11-10 21:14:08.754643558 +
+@@ -18,6 +18,7 @@
+ #include PDFDoc.h
+ #include ErrorCodes.h
+ #include GlobalParams.h
++#include ctype.h
+ 
+ static int firstPage = 0;
+ static int lastPage = 0;
+@@ -65,9 +66,37 @@
+   if (firstPage == 0)
+ firstPage = 1;
+   if (firstPage != lastPage  strstr(destFileName, %d) == NULL) {
+-error(-1, '%s' must contain '%%d' if more than one page should be extracted, destFileName);
++error(-1, '%s' must contain '%d' if more than one page should be extracted, destFileName);
+ return false;
+   }
++
++  // destFileName can have multiple %% and one %d
++  // We use auxDestFileName to replace all the valid % appearances
++  // by 'A' (random char that is not %), if at the end of replacing
++  // any of the valid appearances there is still any % around, the
++  // pattern is wrong
++  char *auxDestFileName = strdup(destFileName);
++  // %% can appear as many times as you want
++  char *p = strstr(auxDestFileName, %%);
++  while (p != NULL) {
++*p = 'A';
++*(p + 1) = 'A';
++p = strstr(p, %%);
++  }
++  // %d can appear only one time
++  p = strstr(auxDestFileName, %d);
++  if (p != NULL) {
++*p = 'A';
++  }
++  // at this point any other % is wrong
++  p = strstr(auxDestFileName, %);
++  if (p != NULL) {
++error(-1, '%s' can only contain one '%d' pattern, destFileName);
++free(auxDestFileName);
++return false;
++  }
++  free(auxDestFileName);
++
+   for (int pageNo = firstPage; pageNo = lastPage; pageNo++) {
+ snprintf (pathName, sizeof (pathName) - 1, destFileName, pageNo);
+ GooString *gpageName = new GooString (pathName);
diff -Nru poppler-0.18.4/debian/patches/series poppler-0.18.4/debian/patches/series
--- poppler-0.18.4/debian/patches/series	2013-08-20 09:53:24.0 -0400
+++ poppler-0.18.4/debian/patches/series	2013-11-10 16:13:39.0 -0500
@@ -10,3 +10,5 @@
 upstream_Initialize-refLine-totally.patch
 CVE-2012-2142.diff
 upstream_pdfseparate.1-Syntax-fixes.patch
+cve-2013-4473.patch
+cve-2013-4474.patch