This is an automated email from the ASF dual-hosted git repository.
borisk pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/xerces-c.git
commit b38ab79e934b9c27de191ee7af6926c7af42069d
Author: Karen Arutyunov <ka...@codesynthesis.com>
AuthorDate: Wed Dec 13 09:59:07 2023 +0200
XERCESC-2188 - Use-after-free on external DTD scan (CVE-2018-1311)
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
---
src/xercesc/internal/DGXMLScanner.cpp | 6 +-
src/xercesc/internal/IGXMLScanner.cpp | 6 +-
src/xercesc/internal/ReaderMgr.cpp | 207 ++++++++++++++++++++++++----------
src/xercesc/internal/ReaderMgr.hpp | 92 ++++++++++++---
4 files changed, 229 insertions(+), 82 deletions(-)
diff --git a/src/xercesc/internal/DGXMLScanner.cpp
b/src/xercesc/internal/DGXMLScanner.cpp
index 38e58f49a..069598ecd 100644
--- a/src/xercesc/internal/DGXMLScanner.cpp
+++ b/src/xercesc/internal/DGXMLScanner.cpp
@@ -1052,13 +1052,12 @@ void DGXMLScanner::scanDocTypeDecl()
DTDEntityDecl* declDTD = new (fMemoryManager)
DTDEntityDecl(gDTDStr, false, fMemoryManager);
declDTD->setSystemId(sysId);
declDTD->setIsExternal(true);
- Janitor<DTDEntityDecl> janDecl(declDTD);
// Mark this one as a throw at end
reader->setThrowAtEnd(true);
// And push it onto the stack, with its pseudo name
- fReaderMgr.pushReader(reader, declDTD);
+ fReaderMgr.pushReaderAdoptEntity(reader, declDTD);
// Tell it its not in an include section
dtdScanner.scanExtSubsetDecl(false, true);
@@ -2131,13 +2130,12 @@ Grammar* DGXMLScanner::loadDTDGrammar(const
InputSource& src,
DTDEntityDecl* declDTD = new (fMemoryManager) DTDEntityDecl(gDTDStr,
false, fMemoryManager);
declDTD->setSystemId(src.getSystemId());
declDTD->setIsExternal(true);
- Janitor<DTDEntityDecl> janDecl(declDTD);
// Mark this one as a throw at end
newReader->setThrowAtEnd(true);
// And push it onto the stack, with its pseudo name
- fReaderMgr.pushReader(newReader, declDTD);
+ fReaderMgr.pushReaderAdoptEntity(newReader, declDTD);
// If we have a doc type handler and advanced callbacks are enabled,
// call the doctype event.
diff --git a/src/xercesc/internal/IGXMLScanner.cpp
b/src/xercesc/internal/IGXMLScanner.cpp
index 4417d44a0..7d1540948 100644
--- a/src/xercesc/internal/IGXMLScanner.cpp
+++ b/src/xercesc/internal/IGXMLScanner.cpp
@@ -1535,13 +1535,12 @@ void IGXMLScanner::scanDocTypeDecl()
DTDEntityDecl* declDTD = new (fMemoryManager)
DTDEntityDecl(gDTDStr, false, fMemoryManager);
declDTD->setSystemId(sysId);
declDTD->setIsExternal(true);
- Janitor<DTDEntityDecl> janDecl(declDTD);
// Mark this one as a throw at end
reader->setThrowAtEnd(true);
// And push it onto the stack, with its pseudo name
- fReaderMgr.pushReader(reader, declDTD);
+ fReaderMgr.pushReaderAdoptEntity(reader, declDTD);
// Tell it its not in an include section
dtdScanner.scanExtSubsetDecl(false, true);
@@ -3098,13 +3097,12 @@ Grammar* IGXMLScanner::loadDTDGrammar(const
InputSource& src,
DTDEntityDecl* declDTD = new (fMemoryManager) DTDEntityDecl(gDTDStr,
false, fMemoryManager);
declDTD->setSystemId(src.getSystemId());
declDTD->setIsExternal(true);
- Janitor<DTDEntityDecl> janDecl(declDTD);
// Mark this one as a throw at end
newReader->setThrowAtEnd(true);
// And push it onto the stack, with its pseudo name
- fReaderMgr.pushReader(newReader, declDTD);
+ fReaderMgr.pushReaderAdoptEntity(newReader, declDTD);
// If we have a doc type handler and advanced callbacks are enabled,
// call the doctype event.
diff --git a/src/xercesc/internal/ReaderMgr.cpp
b/src/xercesc/internal/ReaderMgr.cpp
index 4e59a4a19..cc08284ef 100644
--- a/src/xercesc/internal/ReaderMgr.cpp
+++ b/src/xercesc/internal/ReaderMgr.cpp
@@ -45,12 +45,61 @@
namespace XERCES_CPP_NAMESPACE {
+// ---------------------------------------------------------------------------
+// ReaderMgr::ReaderData: Constructors and Destructor
+// ---------------------------------------------------------------------------
+ReaderMgr::ReaderData::ReaderData( XMLReader* const reader
+ , XMLEntityDecl* const entity
+ , const bool adoptEntity) :
+
+ fReader(reader)
+ , fEntity(entity)
+ , fEntityAdopted(adoptEntity)
+{
+}
+
+ReaderMgr::ReaderData::~ReaderData()
+{
+ delete fReader;
+
+ if (fEntityAdopted)
+ delete fEntity;
+}
+
+// ---------------------------------------------------------------------------
+// ReaderMgr::ReaderData: Getter methods
+// ---------------------------------------------------------------------------
+XMLReader* ReaderMgr::ReaderData::getReader() const
+{
+ return fReader;
+}
+
+XMLEntityDecl* ReaderMgr::ReaderData::getEntity() const
+{
+ return fEntity;
+}
+
+bool ReaderMgr::ReaderData::getEntityAdopted() const
+{
+ return fEntityAdopted;
+}
+
+//
+// This method gives up the entity object ownership but leaves the fEntity
+// pointer intact.
+//
+XMLEntityDecl* ReaderMgr::ReaderData::releaseEntity()
+{
+ fEntityAdopted = false;
+ return fEntity;
+}
+
// ---------------------------------------------------------------------------
// ReaderMgr: Constructors and Destructor
// ---------------------------------------------------------------------------
ReaderMgr::ReaderMgr(MemoryManager* const manager) :
- fCurEntity(0)
+ fCurReaderData(0)
, fCurReader(0)
, fEntityHandler(0)
, fEntityStack(0)
@@ -66,12 +115,11 @@ ReaderMgr::ReaderMgr(MemoryManager* const manager) :
ReaderMgr::~ReaderMgr()
{
//
- // Clean up the reader and entity stacks. Note that we don't own the
- // entities, so we don't delete the current entity (and the entity stack
- // does not own its elements either, so deleting it will not delete the
- // entities it still references!)
+ // Clean up the reader stack and orphan entities container. Note that
+ // all adopted entities (potentially contained in fCurReaderData,
+ // fReaderStack, and fEntityStack) are deleted here.
//
- delete fCurReader;
+ delete fCurReaderData;
delete fReaderStack;
delete fEntityStack;
}
@@ -357,9 +405,9 @@ void ReaderMgr::cleanStackBackTo(const XMLSize_t readerNum)
if (fReaderStack->empty())
ThrowXMLwithMemMgr(RuntimeException,
XMLExcepts::RdrMgr_ReaderIdNotFound, fMemoryManager);
- delete fCurReader;
- fCurReader = fReaderStack->pop();
- fCurEntity = fEntityStack->pop();
+ delete fCurReaderData;
+ fCurReaderData = fReaderStack->pop();
+ fCurReader = fCurReaderData->getReader ();
}
}
@@ -801,20 +849,20 @@ const XMLCh* ReaderMgr::getCurrentEncodingStr() const
const XMLEntityDecl* ReaderMgr::getCurrentEntity() const
{
- return fCurEntity;
+ return fCurReaderData? fCurReaderData->getEntity() : 0;
}
XMLEntityDecl* ReaderMgr::getCurrentEntity()
{
- return fCurEntity;
+ return fCurReaderData? fCurReaderData->getEntity() : 0;
}
XMLSize_t ReaderMgr::getReaderDepth() const
{
// If the stack doesn't exist, its obviously zero
- if (!fEntityStack)
+ if (!fReaderStack)
return 0;
//
@@ -822,7 +870,7 @@ XMLSize_t ReaderMgr::getReaderDepth() const
// reader. So if there is no current reader and none on the stack,
// its zero, else its some non-zero value.
//
- XMLSize_t retVal = fEntityStack->size();
+ XMLSize_t retVal = fReaderStack->size();
if (fCurReader)
retVal++;
return retVal;
@@ -858,7 +906,7 @@ void ReaderMgr::getLastExtEntityInfo(LastExtEntityInfo&
lastInfo) const
bool ReaderMgr::isScanningPERefOutOfLiteral() const
{
// If the current reader is not for an entity, then definitely not
- if (!fCurEntity)
+ if (!fCurReaderData || !fCurReaderData->getEntity())
return false;
//
@@ -873,13 +921,19 @@ bool ReaderMgr::isScanningPERefOutOfLiteral() const
return false;
}
-
bool ReaderMgr::pushReader( XMLReader* const reader
, XMLEntityDecl* const entity)
+{
+ return pushReaderAdoptEntity(reader, entity, false);
+}
+
+bool ReaderMgr::pushReaderAdoptEntity( XMLReader* const reader
+ , XMLEntityDecl* const entity
+ , const bool adoptEntity)
{
//
// First, if an entity was passed, we have to confirm that this entity
- // is not already on the entity stack. If so, then this is a recursive
+ // is not already on the reader stack. If so, then this is a recursive
// entity expansion, so we issue an error and refuse to put the reader
// on the stack.
//
@@ -887,19 +941,30 @@ bool ReaderMgr::pushReader( XMLReader* const
reader
// nothing to do. If there is no entity stack yet, then of coures it
// cannot already be there.
//
- if (entity && fEntityStack)
+ if (entity && fReaderStack)
{
- const XMLSize_t count = fEntityStack->size();
+ // @@ Strangely, we don't check the entity at the top of the stack
+ // (fCurReaderData). Is it a bug?
+ //
+ const XMLSize_t count = fReaderStack->size();
const XMLCh* const theName = entity->getName();
for (XMLSize_t index = 0; index < count; index++)
{
- const XMLEntityDecl* curDecl = fEntityStack->elementAt(index);
+ const XMLEntityDecl* curDecl =
+ fReaderStack->elementAt(index)->getEntity();
+
if (curDecl)
{
if (XMLString::equals(theName, curDecl->getName()))
{
- // Oops, already there so delete reader and return
+ // Oops, already there so delete reader and entity and
+ // return.
+ //
delete reader;
+
+ if (adoptEntity)
+ delete entity;
+
return false;
}
}
@@ -911,52 +976,37 @@ bool ReaderMgr::pushReader( XMLReader* const
reader
// tell it it does own its elements.
//
if (!fReaderStack)
- fReaderStack = new (fMemoryManager) RefStackOf<XMLReader>(16, true,
fMemoryManager);
-
- // And the entity stack, which does not own its elements
- if (!fEntityStack)
- fEntityStack = new (fMemoryManager) RefStackOf<XMLEntityDecl>(16,
false, fMemoryManager);
+ fReaderStack = new (fMemoryManager) RefStackOf<ReaderData>(16, true,
fMemoryManager);
//
- // Push the current reader and entity onto their respective stacks.
- // Note that the the current entity can be null if the current reader
- // is not for an entity.
+ // Push the current reader and entity onto the stack. Note that
+ // the current entity can be null if the current reader is not for
+ // an entity.
//
- if (fCurReader)
- {
- fReaderStack->push(fCurReader);
- fEntityStack->push(fCurEntity);
- }
+ if (fCurReaderData)
+ fReaderStack->push(fCurReaderData);
//
// Make the passed reader and entity the current top of stack. The
// passed entity can (and often is) null.
//
+ fCurReaderData = new (fMemoryManager) ReaderData(reader, entity,
adoptEntity);
fCurReader = reader;
- fCurEntity = entity;
return true;
}
-
void ReaderMgr::reset()
{
// Reset all of the flags
fThrowEOE = false;
// Delete the current reader and flush the reader stack
- delete fCurReader;
+ delete fCurReaderData;
+ fCurReaderData = 0;
fCurReader = 0;
if (fReaderStack)
fReaderStack->removeAllElements();
-
- //
- // And do the same for the entity stack, but don't delete the current
- // entity (if any) since we don't own them.
- //
- fCurEntity = 0;
- if (fEntityStack)
- fEntityStack->removeAllElements();
}
@@ -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;
+
if (curEntity && !curEntity->isExternal())
{
XMLSize_t index = fReaderStack->size();
@@ -1030,7 +1082,7 @@ ReaderMgr::getLastExtEntity(const XMLEntityDecl*&
itsEntity) const
{
// Move down to the previous element and get a pointer to it
index--;
- curEntity = fEntityStack->elementAt(index);
+ curEntity = fReaderStack->elementAt(index)->getEntity();
//
// If its null or its an external entity, then this reader
@@ -1038,12 +1090,12 @@ ReaderMgr::getLastExtEntity(const XMLEntityDecl*&
itsEntity) const
//
if (!curEntity)
{
- theReader = fReaderStack->elementAt(index);
+ theReader = fReaderStack->elementAt(index)->getReader ();
break;
}
else if (curEntity->isExternal())
{
- theReader = fReaderStack->elementAt(index);
+ theReader = fReaderStack->elementAt(index)->getReader ();
break;
}
@@ -1054,6 +1106,11 @@ ReaderMgr::getLastExtEntity(const XMLEntityDecl*&
itsEntity) const
}
}
+ // @@ It feels like we may end up with theReader being from the top of
+ // the stack (fCurReader) and itsEntity being from the bottom of the
+ // stack (if there are no null or external entities on the stack).
+ // Is it a bug?
+ //
itsEntity = curEntity;
return theReader;
}
@@ -1065,31 +1122,59 @@ bool ReaderMgr::popReader()
// We didn't get any more, so try to pop off a reader. If the reader
// stack is empty, then we are at the end, so return false.
//
+ // @@ It feels like we never pop the reader pushed to the stack first
+ // (think of fReaderStack empty but fCurReader not null). Is it a
+ // bug?
+ //
if (fReaderStack->empty())
return false;
//
- // Remember the current entity, before we pop off a new one. We might
+ // Remember the current reader, before we pop off a new one. We might
// need this to throw the end of entity exception at the end.
//
- XMLEntityDecl* prevEntity = fCurEntity;
+ ReaderData* prevReaderData = fCurReaderData;
const bool prevReaderThrowAtEnd = fCurReader->getThrowAtEnd();
const XMLSize_t readerNum = fCurReader->getReaderNum();
//
- // Delete the current reader and pop a new reader and entity off
- // the stacks.
+ // Pop a new reader and entity off the stack.
//
- delete fCurReader;
- fCurReader = fReaderStack->pop();
- fCurEntity = fEntityStack->pop();
+ fCurReaderData = fReaderStack->pop();
+ fCurReader = fCurReaderData->getReader();
//
// If there was a previous entity, and either the fThrowEOE flag is set
- // or reader was marked as such, then throw an end of entity.
+ // or reader was marked as such, then throw an end of entity. Otherwise,
+ // delete the previous reader data.
//
- if (prevEntity && (fThrowEOE || prevReaderThrowAtEnd))
- throw EndOfEntityException(prevEntity, readerNum);
+ if (prevReaderData->getEntity() && (fThrowEOE || prevReaderThrowAtEnd))
+ {
+ //
+ // If the entity is adopted, then move it to fEntityStack so that
+ // its life-time is prolonged to the life-time of this reader
+ // manager. Also delete the previous reader data before throwing
+ // EndOfEntityException.
+ //
+ XMLEntityDecl* entity;
+
+ if (prevReaderData->getEntityAdopted())
+ {
+ if (!fEntityStack)
+ fEntityStack = new (fMemoryManager)
RefStackOf<XMLEntityDecl>(16, true, fMemoryManager);
+
+ entity = prevReaderData->releaseEntity();
+ fEntityStack->push(entity);
+ }
+ else
+ entity = prevReaderData->getEntity();
+
+ delete prevReaderData;
+
+ throw EndOfEntityException(entity, readerNum);
+ }
+ else
+ delete prevReaderData;
while (true)
{
@@ -1119,9 +1204,9 @@ bool ReaderMgr::popReader()
return false;
// Else pop again and try it one more time
- delete fCurReader;
- fCurReader = fReaderStack->pop();
- fCurEntity = fEntityStack->pop();
+ delete fCurReaderData;
+ fCurReaderData = fReaderStack->pop();
+ fCurReader = fCurReaderData->getReader();
}
return true;
}
diff --git a/src/xercesc/internal/ReaderMgr.hpp
b/src/xercesc/internal/ReaderMgr.hpp
index d362af566..b36dbd9b7 100644
--- a/src/xercesc/internal/ReaderMgr.hpp
+++ b/src/xercesc/internal/ReaderMgr.hpp
@@ -160,6 +160,12 @@ public :
XMLReader* const reader
, XMLEntityDecl* const entity
);
+ bool pushReaderAdoptEntity
+ (
+ XMLReader* const reader
+ , XMLEntityDecl* const entity
+ , const bool adoptEntity = true
+ );
void reset();
@@ -208,16 +214,72 @@ private :
ReaderMgr(const ReaderMgr&);
ReaderMgr& operator=(const ReaderMgr&);
+ // -----------------------------------------------------------------------
+ // Private data types
+ // -----------------------------------------------------------------------
+ class ReaderData : public XMemory
+ {
+ public :
+ //
---------------------------------------------------------------------
+ // 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();
+
+ private :
+ //
---------------------------------------------------------------------
+ // 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;
+ };
+
// -----------------------------------------------------------------------
// Private data members
//
- // fCurEntity
- // This is the current top of stack entity. We pull it off the stack
- // and store it here for efficiency.
+ // fCurReaderData
+ // This is the current top of the reader data stack. We pull it off
+ // the stack and store it here for efficiency.
//
// fCurReader
- // This is the current top of stack reader. We pull it off the
- // stack and store it here for efficiency.
+ // This is the reader of the current top of the reader data stack.
+ // It contains the same value as fCurReaderData->fReader or NULL,
+ // if fCurReaderData is NULL. We store it here for efficiency.
//
// fEntityHandler
// This is the installed entity handler. Its installed via the
@@ -225,10 +287,14 @@ private :
// process of creating external entity readers.
//
// fEntityStack
- // 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. This stack consists of refs to
- // EntityDecl objects for the pushed entities.
+ // This is a storage of orphaned XMLEntityDecl objects. The
+ // popReader() function adds a reader manager-adopted entity object
+ // to this storage before passing its pointer to the constructor
+ // of the being thrown EndOfEntityException exception. This makes
+ // sure that the life-time of an entity exposed to the exception
+ // handlers is the same as the life-time of reader manager (and so
+ // normally the life-time of the scanner which embeds the reader
+ // manager).
//
// fNextReaderNum
// This is the reader serial number value. Each new reader that is
@@ -236,8 +302,8 @@ private :
// us catch things like partial markup errors and such.
//
// fReaderStack
- // This is the stack of reader references. We own all the readers
- // and destroy them when they are used up.
+ // This is the stack of reader data references. We own all the
+ // entries and destroy them when they are used up.
//
// fThrowEOE
// This flag controls whether we throw an exception when we hit an
@@ -252,12 +318,12 @@ private :
// fStandardUriConformant
// This flag controls whether we force conformant URI
// -----------------------------------------------------------------------
- XMLEntityDecl* fCurEntity;
+ ReaderData* fCurReaderData;
XMLReader* fCurReader;
XMLEntityHandler* fEntityHandler;
RefStackOf<XMLEntityDecl>* fEntityStack;
unsigned int fNextReaderNum;
- RefStackOf<XMLReader>* fReaderStack;
+ RefStackOf<ReaderData>* fReaderStack;
bool fThrowEOE;
XMLReader::XMLVersion fXMLVersion;
bool fStandardUriConformant;
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscr...@xerces.apache.org
For additional commands, e-mail: c-dev-h...@xerces.apache.org
