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.

+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.) :-)
–
middaparkaApr 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 HalliburtonApr 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?
–
FrustratedWithFormsDesignerApr 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 KraftApr 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 HardingApr 18 '11 at 7:53

+1 for live demonstration of vulnerability.
–
MichaelApr 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"
–
AntApr 18 '11 at 7:55

2

@Ant: Be nice. Do it shortly after the scheduled backup, not shortly before.
–
Donal FellowsApr 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.
–
jwentingApr 18 '11 at 8:27

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.

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.
–
WoobleApr 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.
–
yanApr 14 '11 at 20:27

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:

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.

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.

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 HalliburtonApr 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.
–
BlowskiApr 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 HalliburtonApr 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.
–
BlowskiApr 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 HalliburtonApr 14 '11 at 22:45

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.

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.