| Summary: | Problematic flow at poppler while 'startxref' is missing | ||
|---|---|---|---|
| Product: | poppler | Reporter: | Ilya Gorenbein <igorenbein> |
| Component: | general | Assignee: | poppler-bugs <poppler-bugs> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | major | ||
| Priority: | medium | CC: | axel.struebing, hib |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux (All) | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
NOTE MALICIOUS FILE!!!
missing startxref patch 0001-Handle-missing-startxref-properly.patch |
||
Created attachment 47867 [details] [review] missing startxref patch Hmmm, i do not think removing that loop is ok, i mean we are calculating that value to use it afterwards, i'll bring in two persons that might have some more knowledge about that code so they can give their opinion Hib can you have a look at the bug/patch it seems to touch something you might have some knowledge over Axel this touches something similar to the patch you sent to the mailing list, i'm adding you here in case you can give a look to it, i'd be interested in knowing your opinion. (In reply to comment #4) > Axel this touches something similar to the patch you sent to the mailing list, > i'm adding you here in case you can give a look to it, i'd be interested in > knowing your opinion. Only partly because my patch just covers a minor issue with linearized files. That means if we need to search for another xref near the beginning of the file. If the cross-refererence stream/table is completely missing - as in the attached file - the only thing we probably could meaningful do is gracefully exit. This is by no means an valid pdf file. The most important would be not to read or try to execute the suspicious javascript. You asked for my opinion ;-) If a file has no valid xref information, throw error and exit. Nothing can be done without this required information and 'startxref' is required in every valid pdf file. Ilya I can not reproduce the crash with this file. Hib will hopefully have a look at the code since he wrote it, but i would like to investigate why a "wrong" startXRefPos value can lead to a crash, so could you please post valgrind and gdb traces of the crashes? Created attachment 49495 [details] [review] 0001-Handle-missing-startxref-properly.patch I agree that it would be better if the strToUnsigned function is not called for the case that no startxref can be found. Looking back at were this code came from, I copied and adapted it from the original XRef.cc, but made a mistake in adapting it. Thanks for finding and reporting this issue. This new patch will correct my mistake. However, I also agree with Albert that there must be another issue here: in the test document, the startxref is missing completely, but any malicious document could just as well have a valid startxref section returning an arbitrary value like the value 1111000110 that was reported in the bug report. I've commited Hib's patch. Ilya please if you have time to provide the gdb/valgrind traces of the crash you were experiencing, attach them |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Created attachment 47866 [details] NOTE MALICIOUS FILE!!! Hello, Recently I suffered a crash in poppler version 0.16.0 library. The crash is very hardly reproducible. At the sample file, startxref keyword is missing. The code at poppler/PDFDoc.cc Guint PDFDoc::getStartXRef() function, tries to assign the value to startXRefPos. But for this specific sample it finally will equal to 1111000110. Which is wrong and leads to crash (in some cases). This huge value is assigned to the startXRefPos variable by the following code startXRefPos = strToUnsigned(p); I do not understand why the strToUnsigned function should be called. I think that it will be correctly to stop at: if (i < 0) { startXRefPos = 0; } so, startXRefPos will be equal to 0. The sample file is attached. NOTE, IT IS MALICIOUS!!! The file is password protected archive (pass is malicious). I checked the latest 0.16.6 version, but this, specific part of the code was not changed. Regards