Buffer overflow in Dragonfly module

orbitersim / orbiter

Open-source repository of Orbiter Space Flight Simulator

MIT License

1.61k stars 220 forks source link

Buffer overflow in Dragonfly module #105

Closed DarkWanderer closed 3 years ago

DarkWanderer commented 3 years ago

Found an issue while running Orbiter under ASAN:

https://github.com/orbitersim/orbiter/blob/17ae0ae7c621cf5129cf14bbfd4fa0cd99187bfd/Src/Vessel/Dragonfly/instruments.cpp#L429-L432

i_names is an array char[7][12], but is indexed from 0 to 11 - likely just overflows into the next array

mschweiger commented 3 years ago

Good find. This isn't actually my own code, but looking at it, it seems it can be fixed simply by increasing the first dimension to 12. Before I do, I'd like to see how ASAN is actually flagging this problem. I tried to run a scenario with a Dragonfly in it after building Orbiter with ASAN support, but didn't notice any warnings. Do I need to do something specific to trigger the issue? How is ASAN reporting problems it finds?

GLS-SSV commented 3 years ago

Turning this ticket into a more general one, Cppcheck finds a few more similar situations, here are some:

MfdDocking.cpp:378 (error) Array 'draw[5][2]' accessed at index draw[0][3], which is out of bounds. [arrayIndexOutOfBounds]
MfdHsi.cpp:42 (error) Array 'pen[2]' accessed at index 4, which is out of bounds. [arrayIndexOutOfBounds]
VectorMap.cpp:1160 (error) Array 'pt[64]' accessed at index 64, which is out of bounds. [arrayIndexOutOfBounds]

Some are "false positives", but others are real and should be fixed.

Cppcheck also shows situations like this "if ((extmode == CAMERA_TARGETTOOBJECT || extmode == CAMERA_TARGETTOOBJECT)" on Camera.cpp:390, which could be a typo or redundant code.

DarkWanderer commented 3 years ago

Good find. This isn't actually my own code, but looking at it, it seems it can be fixed simply by increasing the first dimension to 12. Before I do, I'd like to see how ASAN is actually flagging this problem. I tried to run a scenario with a Dragonfly in it after building Orbiter with ASAN support, but didn't notice any warnings. Do I need to do something specific to trigger the issue? How is ASAN reporting problems it finds?

I am away from my desktop where I can confirm this, but the way I stumbled into this was by launching Modules\Server\Orbiter.exe with --scenariox=Dragonfly\Dragonfly parameter under debugger (Visual Studio)

EDIT: this is how it looks in action. I think I might have made a mistake with ASAN confiugration currently in main - it doesn't seem to pass the flag to compiler correctly. Will check

DarkWanderer commented 3 years ago

Please check #124. Apologies for confusion