16

We have a PHP application that we want to get code reviewed from an external security consultant, but I'm not clear on "how to" go about that process.

We did specify what kind of tests he should be doing, and the first part of his submitted report just points out VERY standard 'issues' in the use of eval(), fopen(), etc - under the heading of 'Input validation issues'.

I saw ALL of these reported problems on my own when I ran an automated security code review tool.

(A) I dont know if that's the extent of the problem within my code and the guy did a good job, OR
(B) he's just running these automated tools and just filtering the output to remove noise and that's it!

Questions:

  1. What should I be asking him to do?
  2. How can I cross check his work so that I know he's actually doing a good job?

(copied from https://stackoverflow.com/questions/4311151/getting-a-manual-security-code-review-done-what-to-watch-out-for)

  • Thanks guys for your responses - I seem to have lost my cookies for this account and can't acccess this question as the OP...! @AviD, @Graham Lee - we 'ended' it with our previous guy and are on the lookout for a more competent person - any thoughts on that? See http://security.stackexchange.com/questions/926/finding-security-consultant-for-doing-in-depth-code-review – siliconpi Dec 02 '10 at 02:14

3 Answers3

7

Realizing that you even have such a question already puts you ahead of the game.
I would also add:

(C) He's not very good at reviewing code, so you'll only get "low hanging fruit"
(D) He's really a pentester, that thinks its "oh so easy" to branch out into code, so you'll get similar results as a pentest, only traced to the actual code.
(E) He's a developer, without much understanding of security, but knows some bad code.
etc...

Point of the matter is, it seems that he's not being very transparent with you - so it doesnt really matter which option it is. Even (A) My code has no problems is not worth very much, unless he can back it up with proof of which problems your code doesn't have (sic).
And no, running an automated tool does not count as a security code review, as you apparently felt, correctly.

So, to your questions:

1. What should you ask him to do "Security code review". He should be telling you what he will do, and/or recommend what else you need (this could/should be a pretty long list). Of course, you can/should give him more input, such as what your application does, who has access, how sensitive the data is, etc etc. But even if you don't, he should ask for this information. Without it, it's simply not a professional-level code review.

2. How do I cross-check his work - I would say that in general, there are 3 possiblities:

  • Learn enough about the subject to understand what he claims to be doing, and to ask the hard questions - or get somebody who does (Always true when dealing with provider of a service you don't quite understand...)
  • Have another consultant do a competing review - even if only on a subset of the application.
  • (And this is the good one) Perform an external blackbox penetration test. This is the best way of validating the code review. Of course, have someone else do the pentest, and compare results. Note that not all vulnerabilities found in pentest are relevant to be found in a code review, and vice versa, but it should give you some indication.
AviD
  • 72,138
  • 22
  • 136
  • 218
5

I'm coming at this from the position of being the guy who performs the security code review (though I didn't do yours ;-). So what you want to find out is whether the code could be deployed with low risk, and if not then what the risks are. Therefore you should ask your consultant that, rather than asking "can you find some vulnerabilities in my code". The answer to the risk-related question should include:

  • what he's tried in order to identify the risks
  • which attacks he's considered (I assume that a PHP app will be web-based, in which case the OWASP top 10 list is appropriate as a starter)
  • why he considered those attacks (in other words, what threat model he used)
  • what residual risk remains
  • 1
    Though these are great suggestions, this does not correlate directly with "doing a good security code review". Of course, if he can answer these satisfactorily, it's likely he knows what he's doing... but that's just figuring out if he knows security. Also, in many consulting companies, the code review might be done by a junior consultant, and the answers to the "tough" risk / scoping questions would be handled by a senior - which would not affect the quality of product. Still, it is a good sign. – AviD Nov 30 '10 at 12:02
  • 1
    @avid: I would argue that a good security code review is risk-focussed, because the whole point is to address risk. For instance, were I a UNIX vendor, I'd consider it a waste of money to find that a reviewer had spent a day looking for data leakage bugs in telnetd when telnet _is_ a data leakage service. –  Dec 01 '10 at 17:35
  • 1
    Oh absolutely, I agree completely. However, just cuz you understand *risk*, doesn't mean you understand *code*. In fact, if you dont understand code, you can't really analyze the risk, can you? – AviD Dec 01 '10 at 21:04
  • @AviD: right, but it doesn't follow that you _don't_ need to understand risk to do a good _security_ code review. –  Dec 08 '10 at 10:46
  • True, I think we both agree on the opposite of that... – AviD Dec 08 '10 at 12:24
2

You need to consider both the breadth and depth of the review. By breadth, I mean what range of risks/attacks/threats will be covered in the review. You should start with the OWASP Application Security Verification Standard to get an idea of what ought to be covered here. By depth, I mean how well will the consultant verify that each area has been adequately defended. At the low end, an automated tool scan provides little assurance. At the high end, a manual inspection or actual test case should essentially prove that the correct defenses are in place, have been designed correctly, and are used everywhere that they need to be.

planetlevel
  • 335
  • 1
  • 7