This challenge appeared on an internal alias dedicated to C++. It was issued by Mike Vine, a developer here at Microsoft who agreed to let us share it with the mighty Visual C++ blog readers:
This challenge came from me thinking about a simple bug which could be turned into a security vulnerability, so I thought I'd give it a go and try to code a plausibly deniable piece of code which looks innocent but is actually dangerous. I managed to actually go further than that, and produced something, that whilst unlikely, could possibly have come from non-malicious but sloppy coding.
So your challenge is – if you choose to accept it – analyze the sample code file "main.c" (attached) and try to find the (fairly obvious) security faux pas and 'accidental' bug which causes the security faux pas to be exploitable.
Try to analyze it first before running it, like you would in a Code Review, to try to spot the issue. As it is code which could've come from a sloppy programmer everything is pretty much what it seems – there's no misnamed functions or anything lame like that.
The security vulnerability comes from the file it tries to load which we assume is attacker controlled (e.g. on a CD for a console, or downloaded from the internet for a browser).
I'd assume experienced developers and security folk should be able to get this pretty quickly. In that case, try to analyze how it's possible to really exploit the issue – the attached "Background.dat" is an example exploit (its benign enough to try out, but save your work first!). See if you can come up with that exploit yourself, or alternatively come up with something more fun / smaller / etc. I'm really interested with what's possible here!
To run the code, create a new win32 console app and add the code, and make sure you run it with the 'Background.dat' next to the exe [or in the working directory if running under the VS debugger]
Does your commit process let this code through? Does you coding standards ban the dangerous parts of this code? Would this code pass your team's code review? Does your automated CR tool pick up anything fishy here?
Send me an email if you've got the answer and/or an interesting exploit and I'll reply back in a few days with the best of the responses.
Thanks, and good luck,
Our readers are some of the best developers out there, so when you find an answer, email it to Mike before the end of the week. Look for an update in the next few weeks.
I love mystery with award-winning :)
( Tablet , phone , ...)
Getting newbies interested in this? Is there really nothing better to do, to cover, here? Really?
Sorry, but this is terribly uninteresting. Most of the sample code is pointless gunk that serves no interest, much of the code style is cripplingly primitive, and the only place with any real logic in it (the CustomImage constructor) has nothing to do with C or C++, but WinAPI.
We are always looking for topic ideas. What would you like to see covered in future posts? Do you have favorite authors? Let us know!
Poor coding style is a fact of life for most of us working on codebases that have been around a while or which have been worked on by many people. Finding stuff like this is important. That said, I don't know anyone that would catch this in a normal code review.
The only error I've spotted without running or compiling the code is ImageFactory::DebugPrintDimensions((ImageFactory::CustomImage*)image) uses reinterpret_cast instead of static_cast because the compiler doesn't know CustomImage derives from Image at this point in the program. And because CustomImage has virtual functions the offset is adjusted differently between code that knows that it derives from it.
> Does your commit process let this code through? Would this code pass your team's code review?
Depends on the size of the patch, who checked it in, and how much sleep we have. I caught the code easily because I'm trained to be suspicious about casts and to isolate or eliminate them and because the code is fairly short.
> Does you coding standards ban the dangerous parts of this code?
My team never uses C style casts or reinterpret_casts (instead of reinterpret_cast we static_cast<void *> then to the destination type). These parts of the code look fishy so we use it as a trigger to eliminate them. In this case, the code could have never been written in the first place.
> Does your automated CR tool pick up anything fishy here?
While the code you mention is indeed problematic, the problem happens earlier when converting Derived to Base. At this point, all information that Base was actually a Derived is lost and a cast back to Derived, whatever the form, cannot solve this.
Allowing the supposed attacker to control the flProtect argument to VirtualAlloc is a terrible idea. Please never do this in production code! This should have been PAGE_READWRITE, which would allow modern security mitigations (NX/DEP) to prevent the payload from executing!
Nice post and fun challenge! Cheers to Mike Vine and @Essentia
Nice one. In addition to the security problem shown by Background.dat, there is also a logic problem - only the low part of the file size is used for memory allocation. That means that some file content (for large files) is not read.
Besides the security vulnerability, another error is passing MEM_FREE to VirtualFree. It will set ERROR_INVALID_PARAMETER and the memory will leak.
C casts are evil, secretly changing their meaning on you. This code would actually work if reinterpret_cast had been used consistently. It still wouldn't be good code, but it would work.
I read this on a tablet, and got my laptop because I was in for a challenge! Then I downloaded the source, only to conclude that.... "windows.h".... Was that absolutely necessary? Probably not.
If you liked this, look up the International Obfuscated C Code Contest - it's a real eye-opener, and educational, too.
Wait a minute - isn't it a problem that it doesn't check the memory type on the VirtualAlloc and the data passed in gets to set that parameter? Isn't the data file is giving itself Execute access?
This is still interesting to me (non-guru). It is verified this security vulnerability is automatically fixed by compiler of Visual Studio 2013, only if we move the main() function definition to bottom of code, so when the flawed line is reached, compiler has already got full information of base and derived classes.