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).
>
> Greetings
> F.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+LF
>>>>
>>>> Nope. 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.
>>>
>>> Greetings
>>>> F.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
