Skip to content
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.

Sign up for GitHub

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] Fix potential double-free in usage of ReaderMgr::pushReader() #47

Closed
wants to merge 1 commit into from
Closed

[XERCESC-2188] Fix potential double-free in usage of ReaderMgr::pushReader() #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/xercesc/internal/DGXMLScanner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
* $Id$
*/

// SPDX-FileCopyrightText: Portions Copyright 2021 Siemens
// Modified on 15-Jul-2021 by Siemens and/or its affiliates to fix CVE-2018-1311: Apache Xerces-C use-after-free vulnerability scanning external DTD. Copyright 2021 Siemens.

// ---------------------------------------------------------------------------
// Includes
Expand Down Expand Up @@ -1052,13 +1054,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.pushReader(reader, declDTD, true);

// Tell it its not in an include section
dtdScanner.scanExtSubsetDecl(false, true);
Expand Down Expand Up @@ -2131,13 +2132,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.pushReader(newReader, declDTD, true);

// If we have a doc type handler and advanced callbacks are enabled,
// call the doctype event.
Expand Down Expand Up @@ -2476,7 +2476,7 @@ void DGXMLScanner::scanReset(const InputSource& src)
}

// Push this read onto the reader manager
fReaderMgr.pushReader(newReader, 0);
fReaderMgr.pushReader(newReader, 0, false);

// and reset security-related things if necessary:
if(fSecurityManager != 0)
Expand Down Expand Up @@ -3481,7 +3481,7 @@ DGXMLScanner::scanEntityRef( const bool inAttVal

// Push the reader. If its a recursive expansion, then emit an error
// and return an failure.
if (!fReaderMgr.pushReader(reader, decl))
if (!fReaderMgr.pushReader(reader, decl, false))
{
emitError(XMLErrs::RecursiveEntity, decl->getName());
return EntityExp_Failed;
Expand Down Expand Up @@ -3542,7 +3542,7 @@ DGXMLScanner::scanEntityRef( const bool inAttVal
// where it will become the subsequent input. If it fails, that
// means the entity is recursive, so issue an error. The reader
// will have just been discarded, but we just keep going.
if (!fReaderMgr.pushReader(valueReader, decl))
if (!fReaderMgr.pushReader(valueReader, decl, false))
emitError(XMLErrs::RecursiveEntity, decl->getName());

// here's where we need to check if there's a SecurityManager,
Expand Down
9 changes: 5 additions & 4 deletions src/xercesc/internal/IGXMLScanner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
* $Id$
*/

// SPDX-FileCopyrightText: Portions Copyright 2021 Siemens
// Modified on 15-Jul-2021 by Siemens and/or its affiliates to fix CVE-2018-1311: Apache Xerces-C use-after-free vulnerability scanning external DTD. Copyright 2021 Siemens.

// ---------------------------------------------------------------------------
// Includes
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -1535,13 +1538,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.pushReader(reader, declDTD, true);

// Tell it its not in an include section
dtdScanner.scanExtSubsetDecl(false, true);
Expand Down Expand Up @@ -3098,13 +3100,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.pushReader(newReader, declDTD, true);

// If we have a doc type handler and advanced callbacks are enabled,
// call the doctype event.
Expand Down
6 changes: 3 additions & 3 deletions src/xercesc/internal/IGXMLScanner2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1302,7 +1302,7 @@ void IGXMLScanner::scanReset(const InputSource& src)
}

// Push this read onto the reader manager
fReaderMgr.pushReader(newReader, 0);
fReaderMgr.pushReader(newReader, 0, false);

// and reset security-related things if necessary:
if(fSecurityManager != 0)
Expand Down Expand Up @@ -3201,7 +3201,7 @@ IGXMLScanner::scanEntityRef( const bool inAttVal

// Push the reader. If its a recursive expansion, then emit an error
// and return an failure.
if (!fReaderMgr.pushReader(reader, decl))
if (!fReaderMgr.pushReader(reader, decl, false))
{
emitError(XMLErrs::RecursiveEntity, decl->getName());
return EntityExp_Failed;
Expand Down Expand Up @@ -3262,7 +3262,7 @@ IGXMLScanner::scanEntityRef( const bool inAttVal
// where it will become the subsequent input. If it fails, that
// means the entity is recursive, so issue an error. The reader
// will have just been discarded, but we just keep going.
if (!fReaderMgr.pushReader(valueReader, decl))
if (!fReaderMgr.pushReader(valueReader, decl, false))
emitError(XMLErrs::RecursiveEntity, decl->getName());

// here's where we need to check if there's a SecurityManager,
Expand Down
73 changes: 47 additions & 26 deletions src/xercesc/internal/ReaderMgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ namespace XERCES_CPP_NAMESPACE {
ReaderMgr::ReaderMgr(MemoryManager* const manager) :

fCurEntity(0)
, fOwnEntity(false)
, fCurReader(0)
, fEntityHandler(0)
, fEntityStack(0)
, fNextReaderNum(1)
, fReaderStack(0)
, fThrowEOE(false)
Expand All @@ -72,8 +72,9 @@ ReaderMgr::~ReaderMgr()
// entities it still references!)
//
delete fCurReader;
if (fOwnEntity)
delete fCurEntity;
delete fReaderStack;
delete fEntityStack;
}


Expand Down Expand Up @@ -357,9 +358,7 @@ void ReaderMgr::cleanStackBackTo(const XMLSize_t readerNum)
if (fReaderStack->empty())
ThrowXMLwithMemMgr(RuntimeException, XMLExcepts::RdrMgr_ReaderIdNotFound, fMemoryManager);

delete fCurReader;
fCurReader = fReaderStack->pop();
fCurEntity = fEntityStack->pop();
popReaderAndEntity();
}
}

Expand Down Expand Up @@ -814,15 +813,15 @@ XMLEntityDecl* ReaderMgr::getCurrentEntity()
XMLSize_t ReaderMgr::getReaderDepth() const
{
// If the stack doesn't exist, its obviously zero
if (!fEntityStack)
if (fEntityStack.empty())
return 0;

//
// The return is the stack size, plus one if there is a current
// 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 = fEntityStack.size();
if (fCurReader)
retVal++;
return retVal;
Expand Down Expand Up @@ -874,8 +873,12 @@ bool ReaderMgr::isScanningPERefOutOfLiteral() const
}


// Caller should set transferEntityOwnership to true if the ReaderMgr object
// should assume ownership of the passed entity, and delete it when it no
// longer needs it.
bool ReaderMgr::pushReader( XMLReader* const reader
, XMLEntityDecl* const entity)
, XMLEntityDecl* const entity
, bool transferEntityOwnership)
{
//
// First, if an entity was passed, we have to confirm that this entity
Expand All @@ -887,19 +890,21 @@ 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 && !fEntityStack.empty())
{
const XMLSize_t count = fEntityStack->size();
const XMLSize_t count = fEntityStack.size();
const XMLCh* const theName = entity->getName();
for (XMLSize_t index = 0; index < count; index++)
{
const XMLEntityDecl* curDecl = fEntityStack->elementAt(index);
const XMLEntityDecl* curDecl = fEntityStack[index];
if (curDecl)
{
if (XMLString::equals(theName, curDecl->getName()))
{
// Oops, already there so delete reader and return
delete reader;
if (transferEntityOwnership)
delete entity;
return false;
}
}
Expand All @@ -913,10 +918,6 @@ bool ReaderMgr::pushReader( XMLReader* const reader
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);

//
// Push the current reader and entity onto their respective stacks.
// Note that the the current entity can be null if the current reader
Expand All @@ -925,7 +926,7 @@ bool ReaderMgr::pushReader( XMLReader* const reader
if (fCurReader)
{
fReaderStack->push(fCurReader);
fEntityStack->push(fCurEntity);
fEntityStack.emplace_back(EntityPtrWithOwnershipFlag(fCurEntity, fOwnEntity));
}

//
Expand All @@ -934,6 +935,7 @@ bool ReaderMgr::pushReader( XMLReader* const reader
//
fCurReader = reader;
fCurEntity = entity;
fOwnEntity = transferEntityOwnership;

return true;
}
Expand All @@ -954,9 +956,11 @@ void ReaderMgr::reset()
// And do the same for the entity stack, but don't delete the current
// entity (if any) since we don't own them.
//
if (fOwnEntity)
delete fCurEntity;
fCurEntity = 0;
if (fEntityStack)
fEntityStack->removeAllElements();
fOwnEntity = false;
fEntityStack.clear();
}


Expand Down Expand Up @@ -1030,7 +1034,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 = fEntityStack[index];

//
// If its null or its an external entity, then this reader
Expand Down Expand Up @@ -1059,6 +1063,21 @@ ReaderMgr::getLastExtEntity(const XMLEntityDecl*& itsEntity) const
}


void ReaderMgr::popReaderAndEntity()
{
delete fCurReader;
fCurReader = fReaderStack->pop();

if (fOwnEntity)
delete fCurEntity;
fCurEntity = fEntityStack.back().fEntity;
fOwnEntity = fEntityStack.back().fOwnEntity;
// Make sure to disown the last entity before trimming the stack.
fEntityStack.back().fOwnEntity = false;
fEntityStack.resize(fEntityStack.size() - 1);
}


bool ReaderMgr::popReader()
{
//
Expand All @@ -1080,10 +1099,7 @@ bool ReaderMgr::popReader()
// Delete the current reader and pop a new reader and entity off
// the stacks.
//
delete fCurReader;
fCurReader = fReaderStack->pop();
fCurEntity = fEntityStack->pop();

popReaderAndEntity();
Copy link

@labossip labossip Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to popReaderAndEntity may delete fCurEntity. This will result in the deleted pointer being passed to the EndOfEntityException through prevEntity resulting in potential issues when this pointer is dereferenced in the code that handles the exception.

//
// 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.
Expand Down Expand Up @@ -1119,11 +1135,16 @@ bool ReaderMgr::popReader()
return false;

// Else pop again and try it one more time
delete fCurReader;
fCurReader = fReaderStack->pop();
fCurEntity = fEntityStack->pop();
popReaderAndEntity();
}
return true;
}


ReaderMgr::EntityPtrWithOwnershipFlag::~EntityPtrWithOwnershipFlag()
{
if (fOwnEntity)
delete fEntity;
}

}
Loading