Hi I have some questions below. I think your approach looks sensible but I'm not sure I have understood the description correctly.
See below. On Fri, 24 Jan 2020 at 17:37, Hugo Lefeuvre <h...@owl.eu.com> wrote: > [c-dev senders: please CC me, I did not subscribe to the mailing list] > > Hi, > > I had a look at CVE-2018-1311/XERCESC-2188 with the intention to provide a > patch. > > This issue seems to be present at several places in the source code: > > $ ag "Janitor<DTDEntityDecl>" > src/xercesc/internal/IGXMLScanner.cpp > 1535: Janitor<DTDEntityDecl> janDecl(declDTD); > 3098: Janitor<DTDEntityDecl> janDecl(declDTD); > > src/xercesc/internal/DGXMLScanner.cpp > 1055: Janitor<DTDEntityDecl> janDecl(declDTD); > 2134: Janitor<DTDEntityDecl> janDecl(declDTD); > > > # Issue summary > > A DTDEntityDecl object is allocated and pushed into the ReaderMgr stack. > ReaderMgr does not own the stack's content, so objects neither get freed on > ReaderMgr::popReader(), nor on ReaderMgr::~ReaderMgr(). > And it should not be freed by the code popping the object? > A janitor is set to avoid leaking memory via the allocated DTDEntityDecl. > Unfortunately the object is freed when the janitor gets out of scope, at > which point a pointer to this object is still present within the stack. > Do you mean that the janitor should not free it at this point? Later this freed pointer is dereferenced and one of its methods is called. > Assuming that the attacker managed to manipulate the heap with the right > timing, this leads to code execution. The difficulty is that there is currently no good way to fix this: removing > the janitor will lead to a memory leak, and there is no callback called > when the ReaderMgr removes elements from the stack. > But is the pointer set to point to something else? # I suggest the following fix: > > Add a `bool adopt` parameter to ReaderMgr::pushReader() (default value of > false), and a `RefStackOf<XMLEntityDecl>* fAdoptedStack` private element to > the ReaderMgr class. > > Whenever ReaderMgr::pushReader() is called with `adopt` set to true, also > push passed object onto `fAdoptedStack`. > > Whenever ReaderMgr::popReader() is called, check whether the object being > removed is on top of `fAdoptedStack`. If so, remove and delete it. > > On ReaderMgr::~ReaderMgr(), delete `fAdoptedStack` and all possibly > remaining elements. > I assume that you also remove the janitor in this case? Best regards // Ola > > Feedback? > > cheers, > Hugo > > -- > Hugo Lefeuvre (hle) | www.owl.eu.com > RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD > ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C > -- --- Inguza Technology AB --- MSc in Information Technology ---- | o...@inguza.com o...@debian.org | | http://inguza.com/ Mobile: +46 (0)70-332 1551 | ---------------------------------------------------------------
