Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Sometimes you get busy and delegate small tasks to junior programmers. But if you don't pay close enough attention you find yourself with this kind of code in production:

class DivtoggleController extends Zend_Controller_Action {

    public function closeAction() {
        /* ... code removed for brevity ... */

        $req = $this->getRequest();
        $formData = $req->getPost();

        $d = $formData['div'];
        $i = $formData['id'];

        $dm = new Model_DivtoggleManager();
        $rs = $dm->setDivToggleById($d, $i);

    }

}


class Model_DivtoggleManager extends Zend_Db_Table {

    public function setDivToggleById($div, $id) {
        $result = $this->getAdapter()->query(
           "update div_toggle set " . $div . "=1 where id=" . $id
        );
    }

}

So, given that I have removed the authentication / session management logic for brevity, who can tell me what possible problem there might be with this sample?

share|improve this question

migrated from stackoverflow.com Apr 14 '11 at 20:06

This question came from our site for professional and enthusiast programmers.

    
Why is this not stored in a simple cookie? –  Darknight Apr 18 '11 at 12:47
1  
@Darknight, you are assuming there is a session in which to store the cookie. Whether there is or not is irrelevant to the larger security issue. –  Roger Halliburton Apr 18 '11 at 22:51

7 Answers 7

You can teach them. Everyone does this in the beginning, even you. If this type of code makes it into production, it's the senior folks fault; not the junior.

Edit:

One of the things that I have done is I personally have taken to pro-actively asking people to review my code (including the juniors) before a release. The code gets reviewed, the junior folks see it as a learning experience, people lose the fear of code review as a punishment, and they start doing the same thing.

share|improve this answer
2  
+1 Have to agree. You can't blame a (presumably) junior programmer because you've assumed a level of knowledge they may not possess. (That said, you'd like to think such issues are well known in this day and age. Then again, I'd like to think that I was talented and good looking, etc.) :-) –  middaparka Apr 14 '11 at 20:18
    
A fair enough statement. I guess I should ask a follow up question asking why management would insist on launching code into production that hasn't been approved by the senior programmers. Do I risk my job over a minor security issue? –  Roger Halliburton Apr 14 '11 at 20:27
1  
@Roger Halliburton: Well, if that security issue does somehow get exploited, what's the worst that could happen? And why would pointing out security problems cost you your job? Does management not want to have this dealt with? –  FrustratedWithFormsDesigner Apr 14 '11 at 20:33
1  
@Roger - it's only minor until you get hacked. :) I think that putting code into production without a review (no matter the level of the developer) is a failure. Unfortunately, it is a very common practice in a lot of companies; and, ime, it usually takes a major oh-$4!t before management starts to value things like code reviews. –  John Kraft Apr 14 '11 at 20:36
1  
@Roger: All code should be reviewed, not just code written by "junior" programmers. Even senior programmers make mistakes. –  Dean Harding Apr 18 '11 at 7:53

Assuming that it is the insecurity to which others have referred, as a developer of any level, it's easy to forget that getPost() is not securing the data first.

One way around this is to:

  1. Write a class that gets all POST/GET data and writes it as is to a Singleton class called 'insecure_data'. Then clear the POST/GET arrays.
  2. Developers than have to retrieve POST/GET data from the 'insecure_data' array, not POST/GET arrays.

Any developer that retrieves something from an array called 'insecure_data' and doesn't bother to secure it is either ignorant or lazy. If it's the former, provide training, after which it must be the latter - and then you have a disciplinary issue, not a programming one.

share|improve this answer
    
Wow, that is needlessly convoluted and still doesn't solve the problem. It isn't a matter of scrubbing the input just for fun, but of scrubbing the data that you are putting into the database, and that data could theoretically come from anywhere, from a web form, or an email, or an XML document that someone uploads. In all of these cases you need to scrub when it goes to the database. –  Roger Halliburton Apr 14 '11 at 21:54
    
@RogerHalliburton Of course it doesn't solve everything, but it's a concept (a sort of design pattern, if you will) rather than a fully-fleshed out security measure to make insecure data look insecure. –  Blowski Apr 14 '11 at 22:04
    
Ah, I see. But you are just fighting human nature, if you tell someone "This object is insecure unless it is flagged as secure" and they try to use it for something, most of the time they'll flag it as secure without actually checking. Well, enjoy the reputation boost. –  Roger Halliburton Apr 14 '11 at 22:26
    
From a junior developer's point of view, just seeing $insecure_data raises a flag in a way that just seeing $postdata (or whatever) does not. The idea comes from Joel Spolsky's "Make wrong code look wrong". If the human nature of your developers is such to ignore that red flag, then you either need to provide a lot of training or get some new developers. –  Blowski Apr 14 '11 at 22:32
    
I don't keep Spolsky on a pedestal. These are developers who easily ignore inconsistent tabbing, they aren't going to think twice about using something that is named "insecure". –  Roger Halliburton Apr 14 '11 at 22:45

In terms of your (presumably overriding) "how can I get programmers to stop doing this" question, I'd say that mentoring them on a regular basis, carefully explaining the issue in question (and the potential fallout, etc.) and emphasising the importance of code vulnerabilities (both in terms of SQL injection and cross site scripting, etc.) is probably the most sensible solution.

If they keep messing up despite all of the above (you might want to cast an eye over their commits, etc. rather than finding out "live"), then the problem is either that you're failing them as a mentor, or that they perhaps need to find something more suitable to do for a living.

share|improve this answer

You can mandate them to take a class as soon as they join your company, before they have source control access, that introduces them to SQL injections, cross site scripting, cross site request forgery, and other common vulnerabilities. Cover examples face to face, break bad code in front of them, have them break bad code, and point them to the OWASP site for more info once they "graduate".

You can additionally mandate the use of a custom library that handles this for you, but that's only a secondary solution as they'll be sure to run custom queries when that becomes more convenient.

If you have the resources, ensuring more senior members of the team verify their diffs before committing can be useful also.

Knowledge is power!

share|improve this answer
2  
Weeding them out before they join your company is better. If you're hiring someone to write anything that uses SQL, not asking interview questions about injection borders on incompetence. –  Wooble Apr 14 '11 at 20:26
    
I'd generally agree, but a very smart and ambitious junior hire is much, much cheaper than a "PHP developer with N years of experience," that isn't guaranteed to be that much better. Smart junior devs can pick this stuff up really quickly and be quite an asset. –  yan Apr 14 '11 at 20:27

One of the best guides that I have read on web security is this Ruby on Rails security guide. Although it is Ruby on Rails, a lot of the concepts apply to any web development. I would encourage anyone new to give that guide a read.

share|improve this answer
2  
Everyone knows Ruby isn't secure. –  Roger Halliburton Apr 14 '11 at 22:27
5  
@Roger: “Everybody knows” all sorts of things that might or might not be true. I advocate a reality-based approach to programming, not a hearsay-based one. –  Donal Fellows Apr 18 '11 at 8:11

Hack their code in front of their eyes then show them how to fix it. Over and over until they understand.

share|improve this answer
5  
+1 for live demonstration of vulnerability. –  Michael Apr 14 '11 at 20:12
4  
Email them every morning: "By the way, I dropped your database again last night via another SQL injection vulnerability you left in your code. I know how much you like restoring database backups! :D" –  Ant Apr 18 '11 at 7:55
2  
@Ant: Be nice. Do it shortly after the scheduled backup, not shortly before. –  Donal Fellows Apr 18 '11 at 8:09
    
@Donal: do it shortly after the backup, then tell them the backup failed and let them sweat for the rest of the day before restoring the backup overnight. –  jwenting Apr 18 '11 at 8:27

The code you've linked above is susceptible to an SQL injection attack, because the HTTP inputs you're using in the query have not been cleaned with mysql_real_escape_string or any other means.

share|improve this answer
1  
oh, i thought we were answering this like a test question :] –  Ryan Apr 14 '11 at 20:06
    
Downvotes are a bit lame, since the wording of the question was changed ex post :P –  Ryan Jun 30 '11 at 18:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.