[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