Programmaticaly parse archive loaded with addFileArchive

[CuteAlien]

Tiny thing I forgot - Irrlicht has function core::hasFileExtension (in coreutil.h) - probably better than using find (as "tar.my.file.zip" would be recognized as .tar file with code above)

[Cube_]

Tiny thing I forgot - Irrlicht has function core::hasFileExtension (in coreutil.h) - probably better than using find (as "tar.my.file.zip" would be recognized as .tar file with code above)

mm probably should do that, will add a //@TODO for it, for now it's a trivial problem in prototype code (indeed, this wouldn't be a problem if irrlicht didn't keel over for unsupported types - it really should handle it more gracefully... I should write a patch for that, check file magic - if supported archive, load. if not print warning and move on)

Thanks for pointing it out though, I hadn't even considered that pitfall.

[CuteAlien]

check file magic - if supported archive, load. if not print warning and move on

Irrlicht actually does just that. Can't say much about any crashes unless I have a concrete example to reproduce.

[Cube_]

check file magic - if supported archive, load. if not print warning and move on

Irrlicht actually does just that. Can't say much about any crashes unless I have a concrete example to reproduce.

mmm it was crashing for me, but I was probably just doing something stupid - because can't get it to crash now (maybe I was trying to load meshes from the archive without checking if it was loaded first? I don't remember and I don't backup prototype code).

I wager I probably misattributed the crash since there was a warning unsupported archive then instant crash

[CuteAlien]

OK, let's hope no Irrlicht bug then :-)

[Cube_]

OK, let's hope no Irrlicht bug then

probably not, can't reproduce it - and my prototype code is full of unsafe operations like assuming a file load worked correctly.

I did notice that model loading will crash on models it can't parse though; at least for obj files (try with a 0 byte obj file if you try to make a node of it - it should just say invalid model and move on but doesn't so I had to hack around it)

Code: Select all

 
        //let's play spot the bug!
        //the answer is quite simple.
        IAnimatedMesh* levelMesh = Game->smgr->getMesh(strArray[i].c_str());
        //the bug is here, what we SHOULD do is probably check if the mesh is NULL
        //because if we don't we crash when we try to make a node of it, even though
        //this should return a null node and the conditional should replace it with a cube node
        //the more you know [insert gif here]
        IMeshSceneNode* level = Game->smgr->addMeshSceneNode(levelMesh);
        levelMesh->drop();
        if (level)
        {
            level->setMaterialFlag(EMF_LIGHTING, false);
            level->setPosition(level->getPosition() +
                vector3df(0.0f, 0.0f, 50.0f));
        }
        else
        {
            level->drop();
            level = Game->smgr->addCubeSceneNode(10.0f, 0, -1);
            level->setMaterialFlag(EMF_LIGHTING, false);
            level->setPosition(level->getPosition() +
                vector3df(0.0f, 0.0f, 50.0f));
        }
 

Specifically what I consider a bug is that either the mesh loader crashes or creating the node crashes but if it's the node it shouldn't; it should return NULL if passed an invalid mesh so that if(node) fails for a null node. (iirc it also crashes if the path doesn't exist but I'm not 100% sure on that and frankly don't care enough to test it right this moment)
Now if the mesh loader is the source of the crash then naturally there's a bug there; it should return null so that we get null as a parameter to the node so that the node returns null - this prevents a crash (yes, this could be fixed pretty easily by simply doing a check on both the mesh and node but that's... frankly overkill)

Now, I'm well aware this is likely a debate point for bug vs. feature - you could call it either but I'd lean towards bug, any arbitrary cause can corrupt a model and crashing over it is too much in my opinion.

[CuteAlien]

Didn't test now, but your code has a bug if it's like that. Or 2 if levelMesh is 0 (as drop would crash then). You should only call drop() if you called grab(), if the Irrlicht function started with the word "create" or if you created the object with new. In your case if you drop() levelMesh then Irrlicht has from there on an invalid pointern internally (thought you might get lucky for a while as Irrlicht has usually grab'ed it 2 times - once for meshcache and once for scenemanager, so it likely will only crash at the end when trying to release both).

Similar for level->drop(). You call it after you ensured level is 0 - so that will definitely crash.

[Cube_]

the levelMesh->drop() was added after the crash (and works fine for models that don't crash it), but let's see... removing level->drop() does that fix the crash? Seemingly it does (for 0-byte files, but that's not a realistic corruption - actually corrupted models still crash, see below), I now get a placeholder cube (for 0-byte files). whelp I partially retract my previous statement. (I still consider the handling of *node->drop() to be sorta dodgy, a node is likely a template class which means a null node specialization can be written to handle (i.e discard) any calls to drop a note that's already null, I also disagree with the way corrupted models are handled)

here's the corrected code because I'm certain someone somewhere will eventually find use for it:

Code: Select all

    for (int i = 0; i < fileList->getFileCount(); i++)
    { 
        //note, going by getFileCount is not a brilliant idea if any non-model files are in the directory.
        IAnimatedMesh* levelMesh = Game->smgr->getMesh(strArray[i].c_str());
        IMeshSceneNode* level = Game->smgr->addMeshSceneNode(levelMesh);
        if (level)
        {
            //levelMesh->drop(); //we do not need levelMesh any longer but let's not drop it for sake of testing
            level->setMaterialFlag(EMF_LIGHTING, false);
            level->setPosition(level->getPosition() +
                vector3df(0.0f, 0.0f, 50.0f));
        }
        else
        {
            level = Game->smgr->addCubeSceneNode(10.0f, 0, -1);
            level->setMaterialFlag(EMF_LIGHTING, false);
            level->setPosition(level->getPosition() +
                vector3df(0.0f, 0.0f, 50.0f));
        }
    }

However it still crashes on trying to load an invalid obj file as model. (tested by taking a pretty simple model of an axe and intentionally corrupting it).
I can upload the corrupted model if you want (it's 12.8kb)

and this is simply without any kind of dropping at all.


In fact, it crashes on
"Loaded group start:" so it's definitely the obj parser crashing since it crashes during obj parsing, or as I said - crashing on corrupted files is simply not graceful - invalid data should cause it to return NULL so the programmer can deal with it (and a corrupt model can arise from any number of reasons)

[CuteAlien]

Yeah, corrupted files are probably not always handled correct in mesh-loaders. I think they are mostly written to load fast not to load secure.

Not sure what you mean about node being a template class. You can't handle pointer->function_call() when pointer is 0 automatically. That will always crash in c++, so the programmer must add a check (not to confuse with "delete pointer" where c++ does handle a null-pointer by doing nothing).

edit: Ah - you probably meant it's a smart-pointer or something like that. But Irrlicht doesn't use those, it's just plain old c-pointers and reference-counting base-class (http://irrlicht.sourceforge.net/docu/cl ... unted.html)

[Cube_]

Yeah, corrupted files are probably not always handled correct in mesh-loaders. I think they are mostly written to load fast not to load secure.

Not sure what you mean about node being a template class. You can't handle pointer->function_call() when pointer is 0 automatically. That will always crash in c++, so the programmer must add a check (not to confuse with "delete pointer" where c++ does handle a null-pointer by doing nothing).

edit: Ah - you probably meant it's a smart-pointer or something like that. But Irrlicht doesn't use those, it's just plain old c-pointers and reference-counting base-class (http://irrlicht.sourceforge.net/docu/cl ... unted.html)

Right, because it's assigned as a null pointer instead of a null node, my brain was thinking in terms of how I implement things (i.e never leave dangling pointers because of a failed assignment) and I completely forgot that irrlicht just returns a null pointer, this would be equivalent to "if we can't make a node, assign an empty node to it to make the pointer valid" that's just an implementation difference and isn't necessarily any more correct, smart pointers are a great way to accomplish this but it's not the only way (the easiest is simply that in failure case assign return a pointer to an empty node instead of returning NULL but again; implementation details)

As for speed vs security, this is probably a philosophy thing but secure code is far more important than fast code in my opinion, especially for things like file loading code that will usually run once and at a noncritical point in time.

I could have a look at patching that if anyone's interested in safe mesh loaders.

[CuteAlien]

I don't disagree necessarily with the security thing. Although fast loading models can be pretty important in games (and unless games allow users to exchange custom models it's not a security problem). And often it doesn't even conflict. But... with so many loaders in Irrlicht - going over all for security review is unfortunately beyond our capabilities. If someone improves it and patch looks fine I'm certainly going to apply those. But - don't excpect Irrlicht loaders all to be safe. I did run for example a bmp test-suite once for fun and there's also been 3 different images which crashed the bmp loader (badrle4ter, reallybig, rletopdown).

[Cube_]

For my use case, at least, it's imperative to have safe loading - I load campaigns from archives and these campaigns can internally reference whatever models they want. (or in other words, a clever user could potentially run arbitrary code with their mod if the bug is particularly nasty)

As for safety, it's relatively cheap to sanitize data for correctness before applying it, especially on modern hardware and most games do async loading anyway.

Whelp, I'll have a look at making at least a few of the loaders a bit less... potentially unsafe and crashy (even worse, if you throw... say a tar.gz file on the loader it'll spew "unsupported file" and crash anyway, where's for a 0-byte file it'll spew "unsupported file" and then proceed, this implies that it detects that it doesn't support this file and then proceeds to load it anyway), maybe I'll even add separate safe loaders and the user can choose what loaders they want (something like EFAT_GZIP_S for the gzip loader if it's unsafe, something I've yet to determine since I haven't thrown a corrupted archive at it)
Any specific style guidelines I should adhere to?

Worth noting: any game let's you use custom models by simply replacing a model, so technically any irrlicht powered game is vulnerable to such an exploit (if possible, I've yet to analyze it enough to determine if it is but if I can I'll elevate this to a proper bug report because the worst case is arbitrary code execution which is very bad)

[CuteAlien]

Vulnerable to crashing - sure. But on a local machine if you can replace model you can also simply replace the binary. Or start another binary doing whatever you want. There's no user escalation going on as there are no root-services running (unless you have a very strange game). That's not a security problem. You only get security problems in cases where users can supply data which then runs on another users system. Making that completely safe is not exactly trivial. For image formats I suspect the jpg and png loaders might be good as they use libraries which are checked by many security guys and regularly updated when problems are detected. For a bmp - as mention - I even know it's not crash-safe now (and crashes usually mean you can exploit them in some way). But unless data is exchanged over the net _inside_ the game I wouldn't care too much. I don't think many games out there are coded with secure mods in mind (not saying there isn't anyone caring about that, but I'd consider it the exception).

[Cube_]

Vulnerable to crashing - sure. But on a local machine if you can replace model you can also simply replace the binary. Or start another binary doing whatever you want.

Very few users are quite dumb enough to install binary replacement mods, but a model pack? Almost any user would find that to be a safe idea.

There's no user escalation going on as there are no root-services running (unless you have a very strange game). That's not a security problem.

I disagree, from the top of my head I can think of hot potato and UAC ASK privilege escalation exploits (windows 10, hot potato can be utilized on 7 and up - not sure if viable on a fully patched system) this is made all that much easier since almost every user and their grandmother run administrator accounts.

You only get security problems in cases where users can supply data which then runs on another users system. Making that completely safe is not exactly trivial.

Surely that's the entire point of a mod, no? I mean what good is a mod that you make and run only on your own system, it might give you amusement but that's simply not how modding communities work (not that I expect to ever release anything with a large enough playerbase to support modding, it's more of a general principle)
Hell, even if the bug is completely unexploitable I still disagree with just giving up and crashing because of unexpected input, always sanitize input - file system is evil by default, as are users, and network? Don't get me started on network being evil. (yes, you can definitely take this too far, it's a guideline - not a rule written in stone)

For image formats I suspect the jpg and png loaders might be good as they use libraries which are checked by many security guys and regularly updated when problems are detected. For a bmp - as mention - I even know it's not crash-safe now (and crashes usually mean you can exploit them in some way). But unless data is exchanged over the net _inside_ the game I wouldn't care too much. I don't think many games out there are coded with secure mods in mind (not saying there isn't anyone caring about that, but I'd consider it the exception).

In general this is a philosophy thing, as previously mentioned, I value safe code more than micro-optimized code (especially since it's not always a XOR, you can usually have both)

[CuteAlien]

*sigh* I should have quoted. My whole post was more or less about this:

Any game let's you use custom models by simply replacing a model, so technically any irrlicht powered game is vulnerable to such an exploit

I was only mentioning mods at end. And yes - they are a security problem. But they are that in most games really. If you install any game-mods you are usually putting yourself at risk (you have no chance knowing which games might actually have cared more or less about security as there are no public security reviews for games so far to my knowledge). But sure, it's nice if people made them more secure and we should help out when we can :-)

And in our case it's not really a philosophy thing. It's simply that we got lots of loaders written by lots of people - none of which are still active working at Irrlicht. We don't have the manpower and/or knowledge to do security reviews.