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 PowerMapper Software Ltd - www.powermapper.com Registered in Scotland No 362274 Quartermile 2 Edinburgh EH3 9GL From: Michal Sudolsky <sudols...@gmail.com> Date: Thursday, 25 November 2021 at 18:25 To: Christopher Creutzig <ccreu...@mathworks.com> Cc: "podofo-users@lists.sourceforge.net" <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
patch-recursion-guard.diff
Description: patch-recursion-guard.diff
patch-test-recursion.diff
Description: patch-test-recursion.diff
_______________________________________________ Podofo-users mailing list Podofo-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/podofo-users
