CVE-2022-38528: Bug: SEGV on unknown address still exists in Assimp::XFileImporter::CreateMeshes · Issue #4662 · assimp/assimp

Describe the bug

SEGV on unknown address still exists in Assimp::XFileImporter::CreateMeshes.

This is similar to issue #1728. Note that #1728 reported wrong type of the vulnerability, as it is not a NULL pointer dereference. Patch 39ce3e1 was misguided by #1728, leaving this vulnerability unfixed.

To Reproduce

Steps to reproduce the behavior:
version: latest commit 3c253ca
poc:null_CreateMeshes.zip

git clone https://github.com/assimp/assimp.git
cd assimp
mkdir build
cd build
CFLAGS="-g -O0" CXXFLAGS="-g -O0" cmake -G "Unix Makefiles" -DBUILD_SHARED_LIBS=OFF  -DASSIMP_BUILD_ASSIMP_TOOLS=ON  ..
./assimp info $POC

Expected behavior

user@c3ae4d510abb:$ ./bin/assimp info poc
Launching asset import ...           OK
Validating postprocessing flags ...  OK
0 %
Segmentation fault (core dumped)


user@c3ae4d510abb:$ ./bin/assimp info poc
Launching asset import ...           OK
Validating postprocessing flags ...  OK
0 %
AddressSanitizer:DEADLYSIGNAL
=================================================================
==20088==ERROR: AddressSanitizer: SEGV on unknown address 0x6120000301c0 (pc 0x555556872ed9 bp 0x7fffffffb4d0 sp 0x7fffffffb100 T0)
==20088==The signal is caused by a READ memory access.
    #0 0x555556872ed8  (bin/assimp+0x131eed8)
    #1 0x55555687151a  (bin/assimp+0x131d51a)
    #2 0x5555568716a0  (bin/assimp+0x131d6a0)
    #3 0x555556870ba0  (bin/assimp+0x131cba0)
    #4 0x555556870829  (bin/assimp+0x131c829)
    #5 0x555555c56ab5  (bin/assimp+0x702ab5)
    #6 0x55555580ecf2  (bin/assimp+0x2bacf2)
    #7 0x5555557f89af  (bin/assimp+0x2a49af)
    #8 0x5555557f5f42  (bin/assimp+0x2a1f42)
    #9 0x555555801399  (bin/assimp+0x2ad399)
    #10 0x5555557f59c8  (bin/assimp+0x2a19c8)
    #11 0x7ffff7070082  (/lib/x86_64-linux-gnu/libc.so.6+0x24082)
    #12 0x5555557cda7d  (bin/assimp+0x279a7d)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (bin/assimp+0x131eed8)
==20088==ABORTING
Aborted

Vulnerability analysis

Using gdb to trace this PoC, the vulnerability occurs in line 340 of XFileImporter.cpp, due to idx=16256 is larger than the capacity of sourceMesh->mNormals (24).

if ( mesh->HasNormals() ) {

if ( sourceMesh->mNormFaces[ f ].mIndices.size() > d ) {

const size_t idx( sourceMesh->mNormFaces[ f ].mIndices[ d ] );

mesh->mNormals[ newIndex ] = sourceMesh->mNormals[ idx ];

}

}

After tracing it, I found that pMesh->mNormals assigned numNormals elements in line 514-519 of XFileParser.cpp, then line 535-536 saved the result of ReadInt to pMesh->mNormFaces[a].mIndices without checking if it is in the correct boundary (<numNormals). This eventually leads to the bug above.

unsigned int numNormals = ReadInt();

pMesh->mNormals.resize(numNormals);

// read normal vectors

for (unsigned int a = 0; a < numNormals; ++a) {

pMesh->mNormals[a] = ReadVector3();

}

// read normal indices

unsigned int numFaces = ReadInt();

if (numFaces != pMesh->mPosFaces.size()) {

ThrowException(“Normal face count does not match vertex face count.”);

}

// do not crah when no face definitions are there

if (numFaces > 0) {

// normal face creation

pMesh->mNormFaces.resize(numFaces);

for (unsigned int a = 0; a < numFaces; ++a) {

unsigned int numIndices = ReadInt();

pMesh->mNormFaces[a] = Face();

Face &face = pMesh->mNormFaces[a];

for (unsigned int b = 0; b < numIndices; ++b) {

face.mIndices.push_back(ReadInt());

}

TestForSeparator();

}

}

Suggested fix

Add a boundary check after ReadInt following the convention in line 410 below. Line 410 ensures the number read by ReadInt does not exceed the size of the vector.

unsigned int numVertices = ReadInt();

pMesh->mPositions.resize(numVertices);

// read vertices

for (unsigned int a = 0; a < numVertices; a++)

pMesh->mPositions[a] = ReadVector3();

// read position faces

unsigned int numPosFaces = ReadInt();

pMesh->mPosFaces.resize(numPosFaces);

for (unsigned int a = 0; a < numPosFaces; ++a) {

// read indices

unsigned int numIndices = ReadInt();

Face &face = pMesh->mPosFaces[a];

for (unsigned int b = 0; b < numIndices; ++b) {

const int idx(ReadInt());

if (static_cast<unsigned int>(idx) <= numVertices) {

face.mIndices.push_back(idx);

}

}

TestForSeparator();

}