Hello F.E., hello all, > On 11 March 2019 at 09:52 "F. E." <exler7...@gmail.com> wrote: > > > I think "\n\r" combination is unnecessary. > On principal I agree. But without the patch, Podofo loads PDFs > with '\n\r' just fine and I don't want to change that without need. > Furthermore, the standard states 'if the marker is 2 characters > (both a carriage return and a line feed)'. Imo there's no restriction > to the order of CR+LF. > > > This would be better: (e1 == '\r' && e2 == '\n') || (e1 == ' ' && > > (e2 == '\r' || e2 == '\n')) > I added two patches with your simplification, one with check for '\n\r' > and one without it. > Greetings,F.E.
thanks for your patch (F.E.), I've committed your patch with the check for LF+CR to trunk in svn r1973 [1] with the slight changes of making the function CheckEOL() "static" to hide it to all outside its compilation unit and re-wrapping comment lines to make them shorter, because I tested it with four compilers: g++ 4.8, clang++ 3.8 and (on a newer system) g++ 7.3 and clang++ 7.0, test/unit/podofo-test (both with and without --selftest) didn't report any errors (no new warnings were reported by the compilers either), and I also think that LF+CR xref line endings are legitimate. I'm very sorry to have forgot the patch e-mail metadata in the commit message. Best regards, mabri [1] https://sourceforge.net/p/podofo/code/1973/ > Am So., 10. März 2019 um 10:02 Uhr schrieb Michal Sudolsky < > sudols...@gmail.com>: > > I think "\n\r" combination is unnecessary. > > Also CheckEOL is too complicated there is unnecessary allocation and four > > string comparisons while it just had to compare two char values. This would > > be better: > > > > > > (e1 == '\r' && e2 == '\n') || (e1 == ' ' && (e2 == '\r' || e2 == '\n')) > > > > > > On Fri, Mar 8, 2019 at 8:16 PM F. E. < exler7...@gmail.com> wrote: > > > Hello again, > > > I created another patch for checking the xref size. With this iteration, > > > '\r\n', '\n\r', ' \r' and ' \n' are considered valid as EOL, anythign > > > else will fail the parsing of the current subsection. > > > I checked the patch with some of the researchers pdf files, it works > > > fine, but the error description from podofo could be a bit more clear, > > > though. > > > I also checked the patch against the unit tests. The tests from the SVN, > > > although with fixes for MSCV compaired to the 0.9.6 tests, somehow > > > produced a "dereference out of range deque iterator" error. So I used the > > > tests form v0.9.6, had to fix some stuff, but tests were running through. > > > I had one failing test ('testDifferencesEncoding', last assert fails, > > > somehow the differences of the encoding are not applied), but thats > > > totally unrelated to my chances (I think). > > > It would be nice if someone could check the patch again with a proper > > > test environment, mine feels very thrown together and quick-n-dirty (had > > > to dl and build cppunit as well and I'm not sure if I set everything up > > > correctly .. at least it built). > > > GreetingsF.E. > > > > > > Am Do., 7. März 2019 um 13:13 Uhr schrieb F. E. < exler7...@gmail.com>: > > > > > > I disagree. To much leeway only creates more attack surface for > > > > > > malicious pdfs, since parsing is alwas a delicate procedure.> I do > > > > > > not think that allowing xref entries shorter than 20 chars is > > > > > > vulnerability. On the other side it could extend supported PDFs (if > > > > > > such PDFs really exists).My remark is more of a general advise. > > > > > > Parsers in general are often the weakest link and primary attack > > > > > > vector for attackers. Easy to exploit as well, just send a > > > > > > malicious input and a vulnerable parser will ultimately grant you > > > > > > RCE or what else. So the premise for writing a parser should always > > > > > > be, keep it as simple as possible and stick to the requirements.So > > > > > > i would say, if the pdf standard defines that xref entries have to > > > > > > be exactly 20 chacracters long, we should strictly stick to that. > > > > > > And if there are some PDFs with different-sized xref entries, they > > > > > > are to be rejected.Nevertheless, even if we would like to allow > > > > > > xref entries shorter than 20 chars, Podofos current parsing code > > > > > > still handles this wrong, so it has to be fixed regardless. And I > > > > > > think the easier solution right now is to just check for the > > > > > > correct size and fail consistantly otherwise. Changing the code to > > > > > > allow shorter xref entries requires a bigger change. > > > > > Or there is hole in standard: https://pdfsig-collision.florz.de > > > > > In contracts with first two attacks seems SWA does not violate > > > > > standard, at least not with ByteRange key. From what I see in pdf > > > > > reference there is nothing what states that byte range must not > > > > > include only contents. And there is also not stated that there must > > > > > be always exactly two ranges. But seems for example acrobat reader > > > > > does not accept more than 2 ranges and checks whether byte range > > > > > covers what it should.I've got to read this link, looks interesting. > > > > > Yeah, the SWA attack is quite a nifty trick and looks totally fine > > > > > from the signature validation perspective. The ByteRange to the > > > > > signature clearly states which parts to use for the check, which all > > > > > adds up just fine. It may be an oversight of the standard, yeah, and > > > > > what the acrobat reader is imposing sounds very reasonable, I think I > > > > > will add something like that to my signature check as well. > > > > > rom what I read about SWA I really do not think that too short xref > > > > > entry had role in that attack. I rather think that this is simply > > > > > mistake or bug in software which manipulated these pdf files. Only > > > > > one entry is shorter and there is sequence CR+LF+LF at the end of > > > > > xref right before "trailer". I am sending fixed version. Can you try > > > > > it?Yes, this particular file can be repaired, because it has only one > > > > > shorter xref entry. I fixed it myself by "moving" a CR from the last > > > > > line up.But the exploits from the researchers contain 5 genuinely > > > > > different SWA pdf files (there are 27 swa pdfs in total, but many > > > > > duplicates in different folders). Only the first I linked can be > > > > > easily fixed, with the other four pdfs ALL xref entries of the forged > > > > > xraf table are to short! > > > > > See:https://www.pdf-insecurity.org/download/exploits/3_eXpert_PDF_12_Ultimate/siwa.pdf > > > > https://www.pdf-insecurity.org/download/exploits/5_Foxit_Reader/siwa.pdf > > > > https://www.pdf-insecurity.org/download/exploits/10_Nuance_Power_PDF_Standard/siwa-2.pdf > > > > > > > > https://www.pdf-insecurity.org/download/exploits/18_Perfect_PDF_10_Premium/siwa.pdf > > > > https://www.pdf-insecurity.org/download/exploits/20_Soda_PDF_Desktop/siwa.pdf > > > > <- Thats the one that can be repaired! > > > > Podofo is unable to load the first three, all because the xref entry > > > > parsing unalignes bc of the to-short entries:ePdfError_NoNumber: Unable > > > > to load objects from file.: Error while loading object 0 0 Offset = 380 > > > > Index = 2: Object and generation number cannot be > > > > read.:ePdfError_NoNumber: Unable to load objects from file.: Error > > > > while loading object 0 0 Offset = 71 Index = 2: Object and generation > > > > number cannot be read.:ePdfError_NoNumber: Unable to load objects from > > > > file.: Error while loading object 0 0 Offset = 189 Index = 2: Object > > > > and generation number cannot be read.:The forth can be loaded, but the > > > > existing signature object does not get parsed and misses when trying to > > > > resolve the reference to it. > > > > Here i wanted to challenge you to fix the files, but actually, thats > > > > not really difficult either, it turned out. You can just add > > > > whitespaces before the LF and adjust the forged /ByteRange to point to > > > > the real /ByteRange again. And optionally fix some offsets in the xref > > > > entries itself (Foxit Reader one was totally broken, the offsets of the > > > > forged table totally out of place). With this, Podofo can load the > > > > files and my code finds the signature and checks it to valid. So I've > > > > got to agree with you, the xref entry size is not needed for the > > > > attack. If the researchers had paid attention to it, the pdf files > > > > would have been loadable from the start. > > > > > These attack pdf files are supposed to load correctly, signature > > > > > verification part must handle and resist them. > > > > Yes they should be loadable, but mostly are not due to the incorrect > > > > xref size, at least for podofo. I think I should mail the researchers > > > > my fixed versions. I already contacted them regarding this issue, but > > > > they were unimpressed :-/. > > > > I'm currently also working on a new patch for the size check, but that > > > > will come in a later mail, gotta test that first. > > > > Greetings,F.E. > > > > > > > > Am Do., 7. März 2019 um 10:14 Uhr schrieb Michal Sudolsky < > > > > sudols...@gmail.com>: > > > > > > > On the contrary I think podofo is too strict. Maybe would be > > > > > > > better that it actually can open these files with xref entries > > > > > > > shorter or longer than 20 chars as readers from that page do. I > > > > > > > think some readers are implemented to read xref table by lines > > > > > > > (by using for example c++ function getline).I disagree. To much > > > > > > > leeway only creates more attack surface for malicious pdfs, since > > > > > > > parsing is alwas a delicate procedure. > > > > > I do not think that allowing xref entries shorter than 20 chars is > > > > > vulnerability. On the other side it could extend supported PDFs (if > > > > > such PDFs really exists). > > > > > > A lax handling of the standard is the reason why the researchers > > > > > > had so much success with tinkering pdf files for circumventing the > > > > > > signature check. > > > > > Or there is hole in standard: https://pdfsig-collision.florz.de > > > > > In contracts with first two attacks seems SWA does not violate > > > > > standard, at least not with ByteRange key. From what I see in pdf > > > > > reference there is nothing what states that byte range must not > > > > > include only contents. And there is also not stated that there must > > > > > be always exactly two ranges. But seems for example acrobat reader > > > > > does not accept more than 2 ranges and checks whether byte range > > > > > covers what it should. > > > > > > To be more specific, all the shown SIWA attacks use to-short xref > > > > > > entries. And it looks that this is actually a requirement for this > > > > > > attacks, otherwise the restrictions regarding offsets (secured by > > > > > > the signature) are much harder to meet.> > I checked this file and > > > > > > all 3 xref tables has CR+LFNope. Check it again, there is one entry > > > > > > with only LF:In the second xref table, right before the '8 5' > > > > > > subsection, the entry '0000005131 00000 n' is missing it. > This > > > > > > one tiny error totally breaks the loading of the whole file, bc > > > > > > Podofo does not read the next subsection properly. > > > > > > Instead of '8 5', he reads '5 958' as next sub section! > > > > > From what I read about SWA I really do not think that too short xref > > > > > entry had role in that attack. I rather think that this is simply > > > > > mistake or bug in software which manipulated these pdf files. Only > > > > > one entry is shorter and there is sequence CR+LF+LF at the end of > > > > > xref right before "trailer". I am sending fixed version. Can you try > > > > > it? > > > > > > Trust me, I checked every pdf file provided by the researchers, > > > > > > more than half of it had this error. Mostly the Load failed, but > > > > > > two of them could be loaded (e.g. the one I linked earlier). For > > > > > > this two pdf files, I could not find any siganture fields at all, > > > > > > bc. Podofo did not parse properly. The reference to the signature > > > > > > object was there, but it pointed to a not existing object. And > > > > > > thats the REAL issue here, when Podofo does NOT fail at loading, > > > > > > but the parsed document is insufficient. > > > > > These attack pdf files are supposed to load correctly, signature > > > > > verification part must handle and resist them. > > > > > > GreetingsF.E. > > > > > > _______________________________________________ > > > > > > Podofo-users mailing list > > > > > > Podofo-users@lists.sourceforge.net > > > > > > https://lists.sourceforge.net/lists/listinfo/podofo-users_______________________________________________ > > > > > > > > > Podofo-users mailing list > > > Podofo-users@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/podofo-users_______________________________________________ > > > > Podofo-users mailing list > Podofo-users@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/podofo-users _______________________________________________ Podofo-users mailing list Podofo-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/podofo-users
