
A C++ Challenge - wheels
http://chargen.matasano.com/chargen/2009/10/9/a-c-challenge.html
======
tptacek
I didn't post this to HN, once again the audience for this is security people,
not C++ neatfreaks, but if you want to make a go at it, remember the goal is
"find the vulnerability in this code that will allow you to take over the
program". It's not an abstract problem.

~~~
ice799
Fun challenge! I wonder when the contest closes? I'd like to blog about my
solution. Hopefully my email to matasano doesn't get eaten by a spam filter :/

------
icefox
The code is clearly written by someone who knows C first. If I was given that
to review I would reject it.

Using NULL, inconsistent indenting, putting int i outside the for loop,
struct's that should be a class and a class that has no reason being a class.
Not to mention variables like 'o'* and a class named Obj (woo saved 3 letters
and named it the most generic thing I could think of). one struct starts with
a _ the other does not... using printf rather then cout. Bad variable naming
in general.

And this is all before even reporting the real issues which from the looks of
it are more with the c code then the c++ code.

* o does not stand for object, but objects making it an even worse sin

~~~
viraptor
Isn't that out of scope of the challenge? It's a security challenge -- you've
got a project that's not created by yourself and you can't assume that the
programmer was sane either.

Isn't that what security researchers sometimes have to face -- horrid code
that's used by thousands of people anyways? They're not there to judge whether
it's nice and properly indented or not, only if it's exploitable...

Also, in C++ NULL == 0, it doesn't matter if you use the defined value or not.

~~~
tptacek
Yes, plenty of development teams are populated by programmers that treat C++
as "C with basic_string and vector". Chris' example here is a distillate of
some actual code he reviewed.

~~~
0wned
And that's OK. The high-level OOP stuff is there if you need/want it, but
unlike Java, no one is trying to force that mindset down your throat. There is
nothing wrong with C-like C++ code. Nothing at all.

~~~
copper

        There is nothing wrong with C-like C++ code. Nothing at all.
    

On the other hand, recognising that C and C++ treat 'unsigned' differently,
and that certain C code may be wrong C++ code... A C/C++ coder who does not
recognise that C++/C have different behaviour in certain cases is not much of
a coder. (Hm, I believe that's actually one of the rare occasions when saying
C/C++ is correct.)

I assume that's the bug in this code - it's neither idiomatic C++, nor is it
C, but written in a manner that brings out the neatfreak in me.

Chris and Thomas: something like coverity should catch this particular bug
without trouble. Have you guys verified this?

------
fhub
Slightly off topic, but I think its always wise to check argc before assuming
argv[0] is populated. Its convention only that argv[0] is the name of the
program. Got me once when using execve.

if(argv[1] == NULL)

------
maxklein
I'm not smart enough to know what the problem is. But you know what - I don't
need to be smart enough. If a problem is so obscure that I am unable to
discover it through analysis, either because I lack the knowledge or the
skill, then I should have another skill : be able to isolate the problem.

Once I know where the problem is coming from, I can change the affected
routines in such a manner that they don't cause the same error.

If I faced such a problem in the real world, it would take too long to really
understand the bug. It's much faster to isolate the bug and then force a whole
big area of the code out and rewrite it in a different and simpler manner that
would not cause such a bug.

~~~
tptacek
You can't isolate this problem, because it only occurs when you have an
adversary.

~~~
InclinedPlane
Exactly. That's the problem with security exploits. They are typically edge
cases, but whereas the typical edge case affects only a tiny subset of users
who accidentally run into it, a security exploit edge case is purposely sought
out by malicious users and can potentially affect your company and every other
user very negatively. It's the difference between a gas-powered range that has
a slightly uneven heat across the burners and a gas-powered range that can be
re-programmed through the internet to fill your house with gas that is then
set on fire.

~~~
pwmanagerdied
Wonderfully entertaining analogy.

------
vicaya
I'd argue that the integer overflow inside __new __implementation is a
compiler bug.

------
omouse
The problem is you're using C/C++ and dealing with pointers directly. What do
I win?

