4 ms·

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 o

by icefox 17y ago

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 17y ago
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.
- icefox 17y ago
  On the topic of NULL the Stroustrup answer: http://www.research.att.com/~bs/bs_faq2.html#null http://www.research.att.com/~bs/bs_faq2.html#null Really it is just another hint that this is made by a C programmer.
- tptacek 17y ago
  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 17y ago
    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 17y ago
      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?
tptacek 17y ago
This code has a vulnerability that is only present because it is C++, not C. Find it, and then talk about the hygiene.
- icefox 17y ago
  Hard to read code hides bugs and issues. The challenge is about finding hard c++ bugs, not about being able to read bad code. If the developer you are interviewing can't find the bug you wont know if it is because of your horrible code or if he didn't know about that aspect of c++. Fix the hygiene and then lets talk about the actual code
  - tptacek 17y ago
    No. We didn't write the example. We're not going to clean it up. Find the bug in this messy code, or (allegorically) ship code that attackers can take over and explain why you did it to your customers. Now you're starting to see what a nightmare these problems are for developers.
    - jrockway 17y ago
      I'm starting to see why I don't use intrinsically unsafe languages like C and C++.
    - huhtenberg 17y ago
      > Find the bug in this messy code, or (allegorically) ship code , or rewrite this code to use proper checking of untrusted data. I don't know the specifics of your code audits, but the only verdict for the code like this should be to redo it. If the client is uncooperative or if the reviewer has spare time, then, yeah, try and find the actual exploit. Perhaps, if it is an interview question, it's understandable too. Other than that I don't see a clear reason for going this deep into analyzing dirty code. It may or may not contain an exploitable bug, so just clean it up and move on.
  - brown9-2 17y ago
    I think you are really missing the point of this exercise...

by icefox 17y ago

viraptor 17y ago

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.

icefox 17y ago
On the topic of NULL the Stroustrup answer: http://www.research.att.com/~bs/bs_faq2.html#null http://www.research.att.com/~bs/bs_faq2.html#null Really it is just another hint that this is made by a C programmer.
tptacek 17y ago
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 17y ago
  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 17y ago
    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?

tptacek 17y ago

This code has a vulnerability that is only present because it is C++, not C. Find it, and then talk about the hygiene.

icefox 17y ago
Hard to read code hides bugs and issues. The challenge is about finding hard c++ bugs, not about being able to read bad code. If the developer you are interviewing can't find the bug you wont know if it is because of your horrible code or if he didn't know about that aspect of c++. Fix the hygiene and then lets talk about the actual code
- tptacek 17y ago
  No. We didn't write the example. We're not going to clean it up. Find the bug in this messy code, or (allegorically) ship code that attackers can take over and explain why you did it to your customers. Now you're starting to see what a nightmare these problems are for developers.
  - jrockway 17y ago
    I'm starting to see why I don't use intrinsically unsafe languages like C and C++.
  - huhtenberg 17y ago
    > Find the bug in this messy code, or (allegorically) ship code , or rewrite this code to use proper checking of untrusted data. I don't know the specifics of your code audits, but the only verdict for the code like this should be to redo it. If the client is uncooperative or if the reviewer has spare time, then, yeah, try and find the actual exploit. Perhaps, if it is an interview question, it's understandable too. Other than that I don't see a clear reason for going this deep into analyzing dirty code. It may or may not contain an exploitable bug, so just clean it up and move on.
- brown9-2 17y ago
  I think you are really missing the point of this exercise...