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.

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

Bug: SEGV on unknown address still exists in Assimp::XFileImporter::CreateMeshes #4662

Open
0xdd96 opened this issue Jul 26, 2022 · 1 comment
Labels
Bug Global flag to mark a deviation from expected behaviour

Comments

@0xdd96
Copy link

0xdd96 commented Jul 26, 2022

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();
}

@0xdd96 0xdd96 added the Bug Global flag to mark a deviation from expected behaviour label Jul 26, 2022
@krop
Copy link

krop commented Sep 7, 2022

CVE-2022-38528 was published yesterday and references this bug report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Global flag to mark a deviation from expected behaviour
Projects
Status: 🆕 New
Development

No branches or pull requests

2 participants