New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
XERCESC-2188 - Use-after-free on external DTD scan #54
Conversation
These are the instructions for observing the bug (before this commit): $ git clone https://github.com/apache/xerces-c.git $ cd xerces-c $ mkdir build $ cd build $ cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug .. $ make -j8 $ cp ../samples/data/personal.xml . $ cat <<EOF >personal.dtd <?xml encoding="ISO-8859-1"?> <!ENTITY % nonExistentEntity SYSTEM "non-existent.ent"> %nonExistentEntity; EOF $ gdb samples/StdInParse (gdb) b IGXMLScanner.cpp:1544 (gdb) run <personal.xml 1544 fReaderMgr.pushReader(reader, declDTD); (gdb) p declDTD $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) n 1547 dtdScanner.scanExtSubsetDecl(false, true); (gdb) n 1548 } (gdb) s ... (gdb) s # The Janitor is about to delete the above declDTD. 90 delete fData; (gdb) p fData $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) b ReaderMgr.cpp:1024 (gdb) n ... (gdb) n # Now we about to dereference the deleted declDTD. 1024 if (curEntity && !curEntity->isExternal()) (gdb) p curEntity $2 = (const xercesc_4_0::XMLEntityDecl *) 0x49ac68
This fix follows the same overall idea as #47 with the following key differences:
Besides the instructions for observing the bug under the debugger (and confirming that it is no longer observed after the fix), we've also added a direct test for So the fix is reasonably well testes and we haven't observed any regressions. We've also run our CI which covers all the major platforms/compilers (but not in C++98): https://ci.stage.build2.org/@2177ad08-5621-4300-807f-8861b54c54c0 I've also reviewed this patch and it looks good to me. Please review and/or test and let us know if there are any issues. Note that while this commit is against the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting: 4 spaces indents
delete reader; | ||
|
||
if (adoptEntity) | ||
delete entity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete entity; | |
delete entity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks, we will fixup the final version of the commit.
@@ -1020,7 +1070,9 @@ ReaderMgr::getLastExtEntity(const XMLEntityDecl*& itsEntity) const | |||
// search the stack; else, keep the reader that we've got since its | |||
// either an external entity reader or the main file reader. | |||
// | |||
const XMLEntityDecl* curEntity = fCurEntity; | |||
const XMLEntityDecl* curEntity = | |||
fCurReaderData? fCurReaderData->getEntity() : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fCurReaderData? fCurReaderData->getEntity() : 0; | |
fCurReaderData? fCurReaderData->getEntity() : 0; |
// --------------------------------------------------------------------- | ||
// Constructors and Destructor | ||
// --------------------------------------------------------------------- | ||
ReaderData | ||
( XMLReader* const reader | ||
, XMLEntityDecl* const entity | ||
, const bool adoptEntity | ||
); | ||
|
||
~ReaderData(); | ||
|
||
// ---------------------------------------------------------------------- | ||
// Getter methods | ||
// ---------------------------------------------------------------------- | ||
XMLReader* getReader() const; | ||
XMLEntityDecl* getEntity() const; | ||
bool getEntityAdopted() const; | ||
|
||
XMLEntityDecl* releaseEntity(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// --------------------------------------------------------------------- | |
// Constructors and Destructor | |
// --------------------------------------------------------------------- | |
ReaderData | |
( XMLReader* const reader | |
, XMLEntityDecl* const entity | |
, const bool adoptEntity | |
); | |
~ReaderData(); | |
// ---------------------------------------------------------------------- | |
// Getter methods | |
// ---------------------------------------------------------------------- | |
XMLReader* getReader() const; | |
XMLEntityDecl* getEntity() const; | |
bool getEntityAdopted() const; | |
XMLEntityDecl* releaseEntity(); | |
// --------------------------------------------------------------------- | |
// Constructors and Destructor | |
// --------------------------------------------------------------------- | |
ReaderData | |
( XMLReader* const reader | |
, XMLEntityDecl* const entity | |
, const bool adoptEntity | |
); | |
~ReaderData(); | |
// ---------------------------------------------------------------------- | |
// Getter methods | |
// ---------------------------------------------------------------------- | |
XMLReader* getReader() const; | |
XMLEntityDecl* getEntity() const; | |
bool getEntityAdopted() const; | |
XMLEntityDecl* releaseEntity(); |
// --------------------------------------------------------------------- | ||
// Unimplemented constructors and operators | ||
// --------------------------------------------------------------------- | ||
ReaderData(); | ||
ReaderData(const ReaderData&); | ||
ReaderData& operator=(const ReaderData&); | ||
|
||
// --------------------------------------------------------------------- | ||
// Private data members | ||
// | ||
// fReader | ||
// This is the pointer to the reader object that must be destroyed | ||
// when this object is destroyed. | ||
// | ||
// fEntity | ||
// fEntityAdopted | ||
// This is the pointer to the entity object that, if adopted, must | ||
// be destroyed when this object is destroyed. | ||
// | ||
// Note that we need to keep up with which of the pushed readers | ||
// are pushed entity values that are being spooled. This is done | ||
// to avoid the problem of recursive definitions. | ||
// --------------------------------------------------------------------- | ||
XMLReader* fReader; | ||
XMLEntityDecl* fEntity; | ||
bool fEntityAdopted; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// --------------------------------------------------------------------- | |
// Unimplemented constructors and operators | |
// --------------------------------------------------------------------- | |
ReaderData(); | |
ReaderData(const ReaderData&); | |
ReaderData& operator=(const ReaderData&); | |
// --------------------------------------------------------------------- | |
// Private data members | |
// | |
// fReader | |
// This is the pointer to the reader object that must be destroyed | |
// when this object is destroyed. | |
// | |
// fEntity | |
// fEntityAdopted | |
// This is the pointer to the entity object that, if adopted, must | |
// be destroyed when this object is destroyed. | |
// | |
// Note that we need to keep up with which of the pushed readers | |
// are pushed entity values that are being spooled. This is done | |
// to avoid the problem of recursive definitions. | |
// --------------------------------------------------------------------- | |
XMLReader* fReader; | |
XMLEntityDecl* fEntity; | |
bool fEntityAdopted; | |
// --------------------------------------------------------------------- | |
// Unimplemented constructors and operators | |
// --------------------------------------------------------------------- | |
ReaderData(); | |
ReaderData(const ReaderData&); | |
ReaderData& operator=(const ReaderData&); | |
// --------------------------------------------------------------------- | |
// Private data members | |
// | |
// Reader | |
// This is the pointer to the reader object that must be destroyed | |
// when this object is destroyed. | |
// | |
// fEntity | |
// fEntityAdopted | |
// This is the pointer to the entity object that, if adopted, must | |
// be destroyed when this object is destroyed. | |
// | |
// Note that we need to keep up with which of the pushed readers | |
// are pushed entity values that are being spooled. This is done | |
// to avoid the problem of recursive definitions. | |
// --------------------------------------------------------------------- | |
XMLReader* Reader; | |
XMLEntityDecl* fEntity; | |
bool fEntityAdopted; |
These are the instructions for observing the bug (before this commit): $ git clone https://github.com/apache/xerces-c.git $ cd xerces-c $ mkdir build $ cd build $ cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug .. $ make -j8 $ cp ../samples/data/personal.xml . $ cat <<EOF >personal.dtd <?xml encoding="ISO-8859-1"?> <!ENTITY % nonExistentEntity SYSTEM "non-existent.ent"> %nonExistentEntity; EOF $ gdb samples/StdInParse (gdb) b IGXMLScanner.cpp:1544 (gdb) run <personal.xml 1544 fReaderMgr.pushReader(reader, declDTD); (gdb) p declDTD $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) n 1547 dtdScanner.scanExtSubsetDecl(false, true); (gdb) n 1548 } (gdb) s ... (gdb) s # The Janitor is about to delete the above declDTD. 90 delete fData; (gdb) p fData $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) b ReaderMgr.cpp:1024 (gdb) n ... (gdb) n # Now we about to dereference the deleted declDTD. 1024 if (curEntity && !curEntity->isExternal()) (gdb) p curEntity $2 = (const xercesc_4_0::XMLEntityDecl *) 0x49ac68 Origin: apache/xerces-c@e002426 Bug: apache/xerces-c#47 Bug: apache/xerces-c#54 Bug: https://issues.apache.org/jira/browse/XERCESC-2188 Bug-Debian: https://security-tracker.debian.org/tracker/CVE-2018-1311 Bug-Debian: https://bugs.debian.org/947431 Gbp-Pq: Name CVE-2018-1311.patch
These are the instructions for observing the bug (before this commit): $ git clone https://github.com/apache/xerces-c.git $ cd xerces-c $ mkdir build $ cd build $ cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug .. $ make -j8 $ cp ../samples/data/personal.xml . $ cat <<EOF >personal.dtd <?xml encoding="ISO-8859-1"?> <!ENTITY % nonExistentEntity SYSTEM "non-existent.ent"> %nonExistentEntity; EOF $ gdb samples/StdInParse (gdb) b IGXMLScanner.cpp:1544 (gdb) run <personal.xml 1544 fReaderMgr.pushReader(reader, declDTD); (gdb) p declDTD $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) n 1547 dtdScanner.scanExtSubsetDecl(false, true); (gdb) n 1548 } (gdb) s ... (gdb) s # The Janitor is about to delete the above declDTD. 90 delete fData; (gdb) p fData $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) b ReaderMgr.cpp:1024 (gdb) n ... (gdb) n # Now we about to dereference the deleted declDTD. 1024 if (curEntity && !curEntity->isExternal()) (gdb) p curEntity $2 = (const xercesc_4_0::XMLEntityDecl *) 0x49ac68 Origin: apache/xerces-c@e002426 Bug: apache/xerces-c#47 Bug: apache/xerces-c#54 Bug: https://issues.apache.org/jira/browse/XERCESC-2188 Bug-Debian: https://security-tracker.debian.org/tracker/CVE-2018-1311 Bug-Debian: https://bugs.debian.org/947431 Gbp-Pq: Name CVE-2018-1311.patch
These are the instructions for observing the bug (before this commit): $ git clone https://github.com/apache/xerces-c.git $ cd xerces-c $ mkdir build $ cd build $ cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug .. $ make -j8 $ cp ../samples/data/personal.xml . $ cat <<EOF >personal.dtd <?xml encoding="ISO-8859-1"?> <!ENTITY % nonExistentEntity SYSTEM "non-existent.ent"> %nonExistentEntity; EOF $ gdb samples/StdInParse (gdb) b IGXMLScanner.cpp:1544 (gdb) run <personal.xml 1544 fReaderMgr.pushReader(reader, declDTD); (gdb) p declDTD $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) n 1547 dtdScanner.scanExtSubsetDecl(false, true); (gdb) n 1548 } (gdb) s ... (gdb) s # The Janitor is about to delete the above declDTD. 90 delete fData; (gdb) p fData $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) b ReaderMgr.cpp:1024 (gdb) n ... (gdb) n # Now we about to dereference the deleted declDTD. 1024 if (curEntity && !curEntity->isExternal()) (gdb) p curEntity $2 = (const xercesc_4_0::XMLEntityDecl *) 0x49ac68 Origin: apache/xerces-c@e002426 Bug: apache/xerces-c#47 Bug: apache/xerces-c#54 Bug: https://issues.apache.org/jira/browse/XERCESC-2188 Bug-Debian: https://security-tracker.debian.org/tracker/CVE-2018-1311 Bug-Debian: https://bugs.debian.org/947431 Gbp-Pq: Name CVE-2018-1311.patch
These are the instructions for observing the bug (before this commit):