Comment On Security by Posterity

Practical joke that never got reversed?

Cue 50 references to Bobby Tables...

And then...?

I want to know if he looked in the source control logs, or asked someone, to find out *why* it was changed. The answer would be interesting.

The most likely reason was either "New programmer didn't understand old code and wanted to do it his own way" or "Programmer figured he was going to be fired soon and wanted to go out with a bang".

I don't know. It looks like they replaced 12 lines of code with 4 lines, so in that sense, this refactoring produced more efficient code. I don't know who uses ODBC anymore though, it is all OLEDB nowadays.

Is that code in Javascript though? If so, that is a horrible practice because someone could just modify your SQL. It is better to just build a parameter string and then do the replacement on the server side.

Here is what I'm talking about:

Dim strSQL As String = "select * from [~TABLENAME~] where ~COLUMN~ = ~VALUE~"
strSQL = strSQL.Replace("~TABLENAME~", Request.QueryString("t"))
strSQL = strSQL.Replace("~COLUMN~", Request.QueryString("c"))
strSQL = strSQL.Replace("~VALUE~", Request.QueryString("v"))
'Now you can execute strSQL

This is a simple example, but you get the picture. Most of our queries have many more parameters, so you have c1/v1, c2/v2, c3/v3, etc.

The bottom line is you always want to keep your data layer on the SERVER and never on the CLIENT, I cannot stress this enough. Well most of the time not on the client, there are some exceptions that I can think of, but those are advanced scenarios.

insertComment = insertComment.Replace("@Text", "'Somebody has to do it. Might as well be me.'");

Parameters can be finicky. My guess is that the code written the "right way" did not work, and so to meet their deadline they commented it out and they did the quick and dirty replace with strings method.

TopCod3r:

I don't know. It looks like they replaced 12 lines of code with 4 lines, so in that sense, this refactoring produced more efficient code. I don't know who uses ODBC anymore though, it is all OLEDB nowadays.

Is that code in Javascript though? If so, that is a horrible practice because someone could just modify your SQL. It is better to just build a parameter string and then do the replacement on the server side.

Here is what I'm talking about:

Dim strSQL As String = "select * from [~TABLENAME~] where ~COLUMN~ = ~VALUE~"
strSQL = strSQL.Replace("~TABLENAME~", Request.QueryString("t"))
strSQL = strSQL.Replace("~COLUMN~", Request.QueryString("c"))
strSQL = strSQL.Replace("~VALUE~", Request.QueryString("v"))
'Now you can execute strSQL

This is a simple example, but you get the picture. Most of our queries have many more parameters, so you have c1/v1, c2/v2, c3/v3, etc.

The bottom line is you always want to keep your data layer on the SERVER and never on the CLIENT, I cannot stress this enough. Well most of the time not on the client, there are some exceptions that I can think of, but those are advanced scenarios.

Your SQL is wrong. To allow it to be even more dynamic, it should be more like so:
Dim strSQL As String = "sp_execsql 'select * from [~TABLENAME~] ...

TopCod3r:

...there are some exceptions that I can think of, but those are advanced scenarios.

I advise you to switch to regressed scenarios. That way you can do proper regression testing.

Teehee. I bet there's a hilarious webcomic out there that illustrates the fun of SQL injection.

Let me see if I can find the link...

WhiskeyJack:

Teehee. I bet there's a hilarious webcomic out there that illustrates the fun of SQL injection.

Let me see if I can find the link...

One that turns the Tables on DB-folks?

WhiskeyJack:

Teehee. I bet there's a hilarious webcomic out there that illustrates the fun of SQL injection.

Let me see if I can find the link...

Too late. TopCod3r has posted. You are now to flame him, not to bother with little Bobby Tables.

An Error Occured

Not sure what it was, but it was logged. A human will eventually look at it. If the problem persists, please Contact Us. If the problem is on the contact form, then ... well ... that pretty much sucks. You can email instead: alexp-at-WorseThanFailure.com.

WTF???

WhiskeyJack:

Teehee. I bet there's a hilarious webcomic out there that illustrates the fun of SQL injection.

Let me see if I can find the link...

Yeah, I remember that one too!
It was something about Bobby, wasn't it?

Good luck finding it, maybe some of the other commenters can help us finding the link...

Hey guys, I found this hilarious comic:

It had to be done.
http://xkcd.com/327/

Downfall:

Hey guys, I found this hilarious comic:

(snip XKCD)

+1 Relevant

TopCod3r:

It looks like they replaced 12 lines of code with 4 lines, so in that sense, this refactoring produced more efficient code.

Everything on this page is a parameterized query even though it doesn't look like it. (Bonus: the query statements are automatically prepared and cached, then are finalized when no longer reachable.) Using this particular API would have replaced the 12 lines with 1 line without sacrificing security or performance.

Anon:

It had to be done.
http://xkcd.com/327/

no, it really didn't

ounos:

An Error Occured

Not sure what it was, but it was logged. A human will eventually look at it. If the problem persists, please Contact Us. If the problem is on the contact form, then ... well ... that pretty much sucks. You can email instead: alexp-at-WorseThanFailure.com.

WTF???

You'll get used to it. I see this error page about once a week.

I hope you are joking about using the above code to handle your SQL queries. That is still very susceptible to SQL injection.

TopCod3r:

I don't know. It looks like they replaced 12 lines of code with 4 lines, so in that sense, this refactoring produced more efficient code. I don't know who uses ODBC anymore though, it is all OLEDB nowadays.

Is that code in Javascript though? If so, that is a horrible practice because someone could just modify your SQL. It is better to just build a parameter string and then do the replacement on the server side.

Here is what I'm talking about:

Dim strSQL As String = "select * from [~TABLENAME~] where ~COLUMN~ = ~VALUE~"
strSQL = strSQL.Replace("~TABLENAME~", Request.QueryString("t"))
strSQL = strSQL.Replace("~COLUMN~", Request.QueryString("c"))
strSQL = strSQL.Replace("~VALUE~", Request.QueryString("v"))
'Now you can execute strSQL

This is a simple example, but you get the picture. Most of our queries have many more parameters, so you have c1/v1, c2/v2, c3/v3, etc.

The bottom line is you always want to keep your data layer on the SERVER and never on the CLIENT, I cannot stress this enough. Well most of the time not on the client, there are some exceptions that I can think of, but those are advanced scenarios.

I hope you are joking about using the above code to handle your SQL queries. That is still very susceptible to SQL injection.

Andy Goth:

TopCod3r:

It looks like they replaced 12 lines of code with 4 lines, so in that sense, this refactoring produced more efficient code.

Everything on this page is a parameterized query even though it doesn't look like it. (Bonus: the query statements are automatically prepared and cached, then are finalized when no longer reachable.) Using this particular API would have replaced the 12 lines with 1 line without sacrificing security or performance.

Thanks for the link. That reminds me of the data access library that I wrote at my last job. It basically wrote the SQL for you, and made it so you almost didn't even need to know how to write code in order to write a program. I would have probably been able to sell to other developers and make some money, but I had to sign an intellectual property agreement when I was hired. That link you gave me might be the motivation I need to try to write a newer more powerful version of what I did before. I just won't be able to use any of the same code. Except my version will work with VB.NET, not Tcl, so it will be usable by many more people.

The only secure application is one that doesn't use the internet, or a computer. I would just mail a product catalog to all potential clients and ask them to pay with a money order.

SecCodr:

TopCod3r:

blah, blah

I hope you are joking about using the above code to handle your SQL queries. That is still very susceptible to SQL injection.

And you are very susceptible to TopCod3rs comments.

SecCodr:

TopCod3r:

...

I hope you are joking...

Shark Tank has JIM THE BOSS. TDWTF has TopCod3r.

mjk340:

The only secure application is one that doesn't use the internet, or a computer. I would just mail a product catalog to all potential clients and ask them to pay with a money order.

Mail fraud?

Simple fix (assumes that the DB is SQL Server)...

insertQuery = Queries.InsertForumSignUp;

insertQuery = insertQuery.Replace("@Name", "'" + signUpEntity.Name.Replace("'", "''") + "'");

insertQuery = insertQuery.Replace("@Email", "'" + signUpEntity.Email.Replace("'", "''") + "'");

insertQuery = insertQuery.Replace("@Type", "'" + signUpEntity.Type.Replace("'", "''") + "'");

insertQuery = insertQuery.Replace("@ForumsOption", signUpEntity.ForumsOption.ToString());

Checking that .Name .Email and .Type don't throw NullReference exceptions left as an exercise to the reader.

So at some point they had done it the right way, but later changed it to the wrong way, but preseved the correct method for posterity, I guess.

For backward compatibility, I am sure.

ParkinT:

So at some point they had done it the right way, but later changed it to the wrong way, but preseved the correct method for posterity, I guess.

For backward compatibility, I am sure.

Hey, redundancy is good, right?

Maybe they should uncomment the second query. Then, to make sure everything is working, execute BOTH sections of code. And compare the results. If the output from both is the same, then everything is working great!

TopCod3r:

I don't know. It looks like they replaced 12 lines of code with 4 lines, so in that sense, this refactoring produced more efficient code. I don't know who uses ODBC anymore though, it is all OLEDB nowadays.

Is that code in Javascript though? If so, that is a horrible practice because someone could just modify your SQL. It is better to just build a parameter string and then do the replacement on the server side.

Here is what I'm talking about:

Dim strSQL As String = "select * from [~TABLENAME~] where ~COLUMN~ = ~VALUE~"
strSQL = strSQL.Replace("~TABLENAME~", Request.QueryString("t"))
strSQL = strSQL.Replace("~COLUMN~", Request.QueryString("c"))
strSQL = strSQL.Replace("~VALUE~", Request.QueryString("v"))
'Now you can execute strSQL

This is a simple example, but you get the picture. Most of our queries have many more parameters, so you have c1/v1, c2/v2, c3/v3, etc.

The bottom line is you always want to keep your data layer on the SERVER and never on the CLIENT, I cannot stress this enough. Well most of the time not on the client, there are some exceptions that I can think of, but those are advanced scenarios.

That's friggin' brilliant... Let's see, how about something like http://.../query?t=users&c=1&v=1 which yields:

select * from users where ~1 = 1

Excellent example of secure query programming! It's on the server though, so it must be safe???

Maurits:

SecCodr:

TopCod3r:

...

I hope you are joking...

Shark Tank has JIM THE BOSS. TDWTF has TopCod3r.

Every forum should have a warning sign: PLEASE DON'T FEED THE TROLLS.

Dominic:

Simple fix (assumes that the DB is SQL Server)...

insertQuery = insertQuery.Replace("@Name", "'" + signUpEntity.Name.Replace("'", "''") + "'");

Yeah right. So:
select * from a where b = @name;
with signUpEntity.name = \' or 1 <> \
becomes
select * from a where b = '\'' or 1 <> \';
or whatever.
Doubling single quotes is *not* a solution :(

hmm. wouldn't this be easier ?
Dim strSQL as String = "sp_execsql '~SQL~'"
strSQL = strSQL.Replace("~SQL~",Request.QueryString("SQL"))
execute!

anonymous:

hmm. wouldn't this be easier ?
Dim strSQL as String = "sp_execsql '~SQL~'"
strSQL = strSQL.Replace("~SQL~",Request.QueryString("SQL"))
execute!

I think you are missing the point, but that's ok, I don't blame you. The reason you don't want to do that is so the client doesn't have to know how to build SQL code, and also so a hacker can't just stick whatever SQL he wants... like deleting your entire orders table.

TopCod3r:

anonymous:

hmm. wouldn't this be easier ?
Dim strSQL as String = "sp_execsql '~SQL~'"
strSQL = strSQL.Replace("~SQL~",Request.QueryString("SQL"))
execute!

I think you are missing the point, but that's ok, I don't blame you. The reason you don't want to do that is so the client doesn't have to know how to build SQL code, and also so a hacker can't just stick whatever SQL he wants... like deleting your entire orders table.

Ah. Now you see the problem with your code. replace the V in the query string with v=fake_value; delete from orders;

TopCod3r:

I don't know. It looks like they replaced 12 lines of code with 4 lines, so in that sense, this refactoring produced more efficient code. I don't know who uses ODBC anymore though, it is all OLEDB nowadays.

Is that code in Javascript though? If so, that is a horrible practice because someone could just modify your SQL. It is better to just build a parameter string and then do the replacement on the server side.

Here is what I'm talking about:

Dim strSQL As String = "select * from [~TABLENAME~] where ~COLUMN~ = ~VALUE~"
strSQL = strSQL.Replace("~TABLENAME~", Request.QueryString("t"))
strSQL = strSQL.Replace("~COLUMN~", Request.QueryString("c"))
strSQL = strSQL.Replace("~VALUE~", Request.QueryString("v"))
'Now you can execute strSQL

This is a simple example, but you get the picture. Most of our queries have many more parameters, so you have c1/v1, c2/v2, c3/v3, etc.

The bottom line is you always want to keep your data layer on the SERVER and never on the CLIENT, I cannot stress this enough. Well most of the time not on the client, there are some exceptions that I can think of, but those are advanced scenarios.

Why not replace it with strSQL = "DROP TABLE ORDERS"?
That's four lines to one line, think of the efficiency gain!

Note to all programmers:

What a company focuses on in its interview are three things:

1) Can you do the job?
2) Will you fit with the team?
3) Will you fix our stupid problems?

So if they focus on good design to the exclusion of all else, you can be very sure they are answering #3, not #1. Their code will suck.

In Soviet Russia, strings escape you!

sb.append("SELECT ");
sb.append(" COALESCE(BAG_APRS_AMT, 0.0), ");//1
sb.append("BAG_APRS_DT, ");//2
sb.append("BAG_APRS_DSC, ");//3
<snip>

My WTF is progress. When I asked about this code, the programmer said,"We've had a lot of folks work on this with their own conventions..."

Cue 50 references to Bobby Tables..

That's funny, we have no record of little Bobby Tables. Or any other students. Oh. My. GOD.

anonymous:

TopCod3r:

anonymous:

hmm. wouldn't this be easier ?
Dim strSQL as String = "sp_execsql '~SQL~'"
strSQL = strSQL.Replace("~SQL~",Request.QueryString("SQL"))
execute!

I think you are missing the point, but that's ok, I don't blame you. The reason you don't want to do that is so the client doesn't have to know how to build SQL code, and also so a hacker can't just stick whatever SQL he wants... like deleting your entire orders table.

Ah. Now you see the problem with your code. replace the V in the query string with v=fake_value; delete from orders;

Do you see what you have just done?

diaphanein:

Maurits:

SecCodr:

TopCod3r:

...

I hope you are joking...

Shark Tank has JIM THE BOSS. TDWTF has TopCod3r.

Every forum should have a warning sign: PLEASE DON'T FEED THE TROLLS.

At least JIM is witty in his trollery. Maybe TopCod3r should be FRIERED!

Kender:

Yeah right. So:
select * from a where b = @name;
with signUpEntity.name = \' or 1 <> \
becomes
select * from a where b = '\'' or 1 <> \';
or whatever.

which works just fine, the "or 1 <> \" is still inside the string. a backslash doesnt escape a single quote.

TopCod3r:

It looks like they replaced 12 lines of code with 4 lines, so in that sense, this refactoring produced more efficient code.

Exactly! All good coders know that fewer lines always produce more efficient code. In fact, the first thing I do when starting on an existing project is strip all LFs & CRs from the code. I could just replace all of the code with a command to restart the computer, but I don't like showing off.

From now on Bobby Table is 327 and 179 messes with my brain.
CAPTCHA: mara - a misspelling of a dwarf mine?

Please submit my request:

http://thedailywtf.com/query?t=dual; delete from comments&c=userid&v='TopCod3r' and user_type %3d 'dipsh*t'

derula:

WhiskeyJack:

Teehee. I bet there's a hilarious webcomic out there that illustrates the fun of SQL injection.

Let me see if I can find the link...

Too late. TopCod3r has posted. You are now to flame him, not to bother with little Bobby Tables.

Can't we do both? TopCod3r, you're completely wrong, watch what happens when Bobby Tables visits your site.

articleText = articleText.Replace("preseved", "preserved");

You whiny bastards. TopCod3r is the best thing about this site... Consider it free internet survival training.