Yes, I’d considered a new ePdfError value but that needs more discussion since it affects the ABI. Even without the patch there are currently cases where ePdfError_InvalidXRef is thrown but an invalid xref isn’t the problem (e.g. trailer loops)
A new ePdfError value would break client code that has special handling when ePdfError_InvalidXRef is caught. Apart from the PoDoFo unit tests, the most likely case is client code mapping ePdfError_InvalidXRef to an error message like “This PDF is corrupt”. Possible options: 1. Add a new error code like ePdfError_RecursionTooDeep and throw that instead of ePdfError_InvalidXRef in the recursion guard – that might break client code as discussed above: ePdfError_RecursionTooDeep, /* recursion deeper than s_maxRecursionDepth */ 1. Keep ePdfError_InvalidXRef and document that it’s not always an invalid XRef: ePdfError_InvalidXRef, /* The XRef table is invalid or recursion is too deep */ 1. Don’t think replacing ePdfError_InvalidXRef completely is option since that gets thrown invalid xrefs and recursion isn’t involved Best Regards Mark -- Mark Rogers - mark.rog...@powermapper.com<mailto:mark.rog...@powermapper.com> PowerMapper Software Ltd - www.powermapper.com Registered in Scotland No 362274 Quartermile 2 Edinburgh EH3 9GL From: Michal Sudolsky <sudols...@gmail.com> Date: Wednesday, 2 February 2022 at 19:38 To: "PowerMapper.com" <mark.rog...@powermapper.com> Cc: Christopher Creutzig <ccreu...@mathworks.com>, "podofo-users@lists.sourceforge.net" <podofo-users@lists.sourceforge.net> Subject: Re: [Podofo-users] PoDoFo and recursive stack consumption CVEs + if ( s_nRecursionDepth > s_maxRecursionDepth ) + { + // avoid stack overflow on documents that have circular cross references, loops + // or very deeply nested structures, can happen with + // /Prev entries in trailer and XRef streams (possible via a chain of entries with a loop) + // /Kids entries that loop back to self or parent + // deeply nested Dictionary or Array objects (possible with lots of [[[[[[[[]]]]]]]] brackets) + // mutually recursive loops involving several objects are possible + PODOFO_RAISE_ERROR( ePdfError_InvalidXRef ); + } Not all these cases are invalid xref errors. On Wed, Feb 2, 2022 at 7:16 PM Mark Rogers <mark.rog...@powermapper.com<mailto:mark.rog...@powermapper.com>> wrote: Hi Everyone Here are patches for recursive stack consumption, which should fix CVE-2018-8002, CVE-2021-30470, CVE-2021-30471, CVE-2020-18971 This works by refactoring the recursion guard and making it a nested class of PdfTokenizer (as it’s mostly used by the tokenizer and parser). As agreed earlier in this thread the patch means that PoDoFo requires C++ 11 if compiled with PODOFO_MULTI_THREAD The patch has been tested against the CVE PoC files, and the new unit tests. It’s also been tested in production for 2 months on macOS (64-bit) and Windows (32-bit) We haven’t tested on Linux. This might be relevant for the ParserTest::getStackOverflowDepth() unit test method which calculates an overflow depth for each platform that causes stack overflow without exhausting the heap (although the calculation should be the same as macOS since they both use the same System V AMD64 ABI). Best Regards Mark Mark Rogers - mark.rog...@powermapper.com<mailto:mark.rog...@powermapper.com> PowerMapper Software Ltd - www.powermapper.com<http://www.powermapper.com> Registered in Scotland No 362274 Quartermile 2 Edinburgh EH3 9GL From: Michal Sudolsky <sudols...@gmail.com<mailto:sudols...@gmail.com>> Date: Thursday, 25 November 2021 at 18:25 To: Christopher Creutzig <ccreu...@mathworks.com<mailto:ccreu...@mathworks.com>> Cc: "podofo-users@lists.sourceforge.net<mailto:podofo-users@lists.sourceforge.net>" <podofo-users@lists.sourceforge.net<mailto:podofo-users@lists.sourceforge.net>> Subject: Re: [Podofo-users] PoDoFo and recursive stack consumption CVEs On Thu, Nov 25, 2021 at 4:58 PM Christopher Creutzig <ccreu...@mathworks.com<mailto:ccreu...@mathworks.com>> wrote: > Ok, I can submit a patch which uses C++11 thread_local when > PODOFO_MULTI_THREAD is defined. The recursion guard definition will look like > this: > > #if defined(PODOFO_MULTI_THREAD) > static int thread_local s_nRecursionDepth; // PoDoFo with threading support > requires C++11 compiler with thread_local > #else > static int s_nRecursionDepth; // PoDoFo is single threaded > #endif > > Does that work for everyone? Looks good to me, and the comment is hopefully explanation enough if anyone runs into a compile time error. Please do include a doc patch stating the requirement. Can we get a macro that creates this thread-local integer and the recursion guard object all in one go, with the connotation that the recursion guard is meant to usually be applied to each affected function entry point separately? (Unless that is not what it is meant to do. I think we could just as well make an argument for a single recursion depth counter per thread, which then probably should become a static member of the recursion guard class.) I think that it was meant that there will be just a single recursion counter per thread. Cheers, Christopher The MathWorks GmbH | Friedlandstr.18 | 52064 Aachen | District Court Aachen | HRB 8082 | Managing Directors: Bertrand Dissler, Steven D. Barbo, Jeanne O’Keefe From: Mark Rogers <mark.rog...@powermapper.com<mailto:mark.rog...@powermapper.com>> Sent: Thursday, November 25, 2021 16:33 To: Christopher Creutzig <ccreu...@mathworks.com<mailto:ccreu...@mathworks.com>>; Michal Sudolsky <sudols...@gmail.com<mailto:sudols...@gmail.com>> Cc: podofo-users@lists.sourceforge.net<mailto:podofo-users@lists.sourceforge.net> Subject: Re: [Podofo-users] PoDoFo and recursive stack consumption CVEs >>> I like this idea. As PODOFO_MULTI_THREAD will enable C++11 >>Is there a chance we might get there? Who would be able to make that decision? >consider the decision being made. Again, from my point of view. In >other words, feel free to provide a patch with the suggested changes. Ok, I can submit a patch which uses C++11 thread_local when PODOFO_MULTI_THREAD is defined. The recursion guard definition will look like this: #if defined(PODOFO_MULTI_THREAD) static int thread_local s_nRecursionDepth; // PoDoFo with threading support requires C++11 compiler with thread_local #else static int s_nRecursionDepth; // PoDoFo is single threaded #endif Does that work for everyone? > Not when the user of podofo already used some 64 KB before calling podofo. To > me it seems more reasonable to use a more conservative value which would not > consume more than some half (or tenth?) of the available stack in the worst > case. I’ll also reduce the 500 max recursion depth as suggested (probably to 256) And I’ll also include the new parser unit tests which test for deep recursion and reference loops We’re also testing a patch for CVE-2018-20797. This is caused by an invalid negative value for one of the FlateDecode compression parameters which results in a call to podofo_calloc( -14 ) == podofo_calloc( 0xfffffffffffffff2 ) Best Regards Mark Mark Rogers - mark.rog...@powermapper.com<mailto:mark.rog...@powermapper.com> PowerMapper Software Ltd - www.powermapper.com<http://www.powermapper.com> Registered in Scotland No 362274 Quartermile 2 Edinburgh EH3 9GL From: Christopher Creutzig <ccreu...@mathworks.com<mailto:ccreu...@mathworks.com>> Date: Thursday, 25 November 2021 at 07:16 To: Michal Sudolsky <sudols...@gmail.com<mailto:sudols...@gmail.com>> Cc: "PowerMapper.com" <mark.rog...@powermapper.com<mailto:mark.rog...@powermapper.com>>, "podofo-users@lists.sourceforge.net<mailto:podofo-users@lists.sourceforge.net>" <podofo-users@lists.sourceforge.net<mailto:podofo-users@lists.sourceforge.net>> Subject: RE: [Podofo-users] PoDoFo and recursive stack consumption CVEs >> If we want to avoid UB in the multithreaded world, I’m afraid we will have >> to make a C++11 compiler a requirement, as C++03 never acknowledged the >> existence of threads. (That is not limited to this place, a lot of methods >> like PdfEncodingFactory::GlobalPdfRomanEncodingInstance are not currently >> threadsafe in C++03, as discussed earlier.) > That is not thread-safe even in C++11. True, but C++11 or later would give us the tools to make it thread-safe. > Except that some things are not so available as threads like for example > thread_local and atomic operations. thread_local equivalents are available for g++, clang, and MSVC. That covers the compilers listed in https://github.com/NickNaso/PoDoFo/blob/master/README.md#installation_with_cmake. See my proposed PODOFO_THREAD_LOCAL in https://sourceforge.net/p/podofo/mailman/message/37389082/. > I like this idea. As PODOFO_MULTI_THREAD will enable C++11 Is there a chance we might get there? Who would be able to make that decision? Cheers, Christopher The MathWorks GmbH | Friedlandstr.18 | 52064 Aachen | District Court Aachen | HRB 8082 | Managing Directors: Bertrand Dissler, Steven D. Barbo, Jeanne O’Keefe _______________________________________________ Podofo-users mailing list Podofo-users@lists.sourceforge.net<mailto: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
