11

Yesterday I took a part in interview for PHP developer postion. My job was to solve 15 questions quite simple test. One of the questions was to decide wether code similar to below should be treated as unsafe. I gave a wrong (as it turned out) answer and the argumentation from the other person on that interview was quite surprising (at least to me).

Code was something like that:

function someFunction($a)
{
    echo $a * 4;
}

someFunction($_GET['value']);

Possible answers were:

  • always,
  • only when register_globals is enabled,
  • never.

You could get one point for correct answer and second one for giving good explanation (argumentation) on answer chosen answer.

My answer was third: this code is never unsafe. Plus argumentation: Because, this is just a simple equation. There are no file or database operations here, no streams, protocols, no nothing. It's just an equation. Nothing else. Attacker is unable to do anything wrong with PHP script, not matter how malformed URL query he or she will try to execute. No chance.

I've got zero points. Neither my answer was correct, nor my argumentation was accepted. The correct answer was: this code is always unsafe -- you should always escape, what you got from URL query.

My question is: Is this really good point of view? Do we really have to always use a rule of thumb, that anything taken directly from query is unsafe, if not filtered, escaped or secured in any other way? Does this means, that I teach my students an unsefe coding methodologies, becuase on very first PHP lecture they write a script for calculating a triangle area and they're using unescaped, unfiltered params from URL in their task?

I understand, that security and writing safe code should be a matter of highest priority. But, on the other hand, isn't that a little bit of safe-code-fascism (forgive me, if I offended someone) to threat any code unsafe, even it no one is able to do any harm with it?

Or maybe I'm completely wrong and you can do some harm on function that echoes times four, what you gave to it?

Jk1
  • 11,233
  • 9
  • 54
  • 64
trejder
  • 17,148
  • 27
  • 124
  • 216
  • You were on a job during an interview? How interesting. – Salman A Oct 26 '12 at 20:10
  • 2
    A tip: if the interviewer gives a code and asks if unsafe, if some get variables are present, always say it is unsafe even if goes against everything in your logic. – itachi Oct 26 '12 at 20:21
  • Depends on your implementation of `$_GET`. But this is obviously a trick question. When presented with a specific implementation, you should judge on its specifica, not on generalizations or hypothetical dated server settings. Were all questions like this? – mario Oct 26 '12 at 20:32
  • @mario: No! :] Only four or five out of fifteen! But yes -- I had a small headache after that interview! :] – trejder Oct 26 '12 at 20:45

3 Answers3

7

The issue is that later someone may change the function 'somefunction' and do more than simply multiply it by 4.

The function in itself is not unsafe, but the line:

 someFunction($_GET['value']);

Is completely unsafe. Maybe someFunction gets refactored into another file or is way down in the code. You should alway check and scrub user supplied data to protect yourself and others working on a library or function somewhere not caught not expecting you to pass them pure $_GET array data.

This is especially true when working with others and is why it's being asked in the interview--to see if your looking ahead at future potential issues, not to see that you understand that currently someFunction is harmless when pass possibly dangerous GET data. It's becomes an issue when your coworker refactors someFunction to query a DB table.

Ray
  • 40,256
  • 21
  • 101
  • 138
  • 1
    +1, [Also check this answer](http://stackoverflow.com/questions/129677/whats-the-best-method-for-sanitizing-user-input-with-php). – The Alpha Oct 26 '12 at 20:17
  • I agree with in general performing input validation, but disagree that one should try to predict or anticipate the future contexts that result will be used in, or how someone may modify the code in the future. – goat Oct 26 '12 at 20:20
  • @rambocoder as a professional you should be expected to anticipate common future contexts such as 'the code may change so don't assume user data is safe'. – Ray Oct 26 '12 at 20:23
  • Though I generally agree with your answer, I also agree with _rambo coder_ comment. This reminds me a thinking, my friend have: you should always add ; even at the last rule in CSS, because _someone one day might want to add something and might not see that there is no ; at the end_. Let's not become paranoid. – trejder Oct 26 '12 at 20:23
  • 2
    @trejder Protecting against malicious user data is basic programming 101 not over-paranoia. This isn't a tough or trick question. – Ray Oct 26 '12 at 20:25
  • By _paranoia_ I'm not reffering to the fact that we have to protect against malicious user data, but to your thinking ahead: _Maybe someFunction gets refactored into another file_. But, as I wrote, this is only my small addition / comment, because I generally agree with your answer and your point of view. – trejder Oct 26 '12 at 20:27
  • As a professional you are expected to solve problems correctly. It's fundamental that you cannot escape a value without knowing the context in which the value will be placed, because a proper escaping requires a context. eg, escaping a value for database insertion, output as html, as a filename, or as a EXIF comment embedded into an binary image all require different, non-intersecting escaping schemes. – goat Oct 26 '12 at 20:29
  • @trejder yes, this exact situation may be a stretch, but all creators of new things should consider in general easily forseable ramifications of their creation. If I build a gun, it's not a stretch to guess someone might point it at their foot in the future. Some things you can protect against and avoid, others you can't reasonably expect to catch, but just because you can't protect against every possible outcome, doesn't mean don't protect against common ones to the media involved (web user data in this case) and for your future job interviews, please just assume $_GET is always unsafe. – Ray Oct 26 '12 at 20:31
  • @Ray: I must agree with you. Thank you for a great bunch of information. – trejder Oct 26 '12 at 20:47
0

Having not spent much time playing with your code example, I won't say that it could be used to 'do harm' however, your function will not work properly unless it is passed some form of number. In the long run, it is better to escape your code, and handle erroneous data then wait for the day when an unsuspecting user puts the wrong type of value in your box and breaks things. I'm sure that the company you were interviewing for was just looking for someone with a solid habit of making sure their code is complete and unbreakable.

SamHuckaby
  • 1,162
  • 1
  • 13
  • 35
  • Sarn, I'm not going to vote you down since you're a newbie and don't have a lot of points, but yes, the example in the code is dangerous. – Ray Oct 26 '12 at 20:21
0

NEVER trust anything that originates from a user. Just dont. Even when you cannot fathom a possibility of your code/class/package being misused, cover your own ass by ensuring the input to your product is exactly what you're expecting, no surprises. At the barest minimum, someone may supply bad input to that method just to screw with your app, to cause it to show an error or give the white screen of death. The code that does basic multiplication is a prime candidate for that kind of malevolence. It applies not just in PHP, but programming/design in general.

kolossus
  • 20,559
  • 3
  • 52
  • 104