• Edss (unregistered)

    Practical joke that never got reversed?

  • Ozz (unregistered)

    Cue 50 references to Bobby Tables...

  • Rachael (unregistered)

    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.

  • D C Ross (unregistered) in reply to Rachael

    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".

  • (cs)

    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.

  • (cs)

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

  • ChiefCrazyTalk (unregistered)

    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.

  • ShatteredArm (unregistered) in reply to TopCod3r
    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~] ...

  • (cs) in reply to TopCod3r
    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.
  • (cs)

    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...

  • (cs) in reply to WhiskeyJack
    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?

  • (cs) in reply to WhiskeyJack
    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.

  • (cs)
    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???

  • (cs) in reply to WhiskeyJack
    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...

  • Downfall (unregistered)

    Hey guys, I found this hilarious comic:

    [image]
  • ~Anon (unregistered)

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

  • (cs) in reply to Downfall
    Downfall:
    Hey guys, I found this hilarious comic:

    (snip XKCD)

    +1 Relevant

  • (cs) in reply to TopCod3r
    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.
  • (cs) in reply to ~Anon
    Anon:
    It had to be done. http://xkcd.com/327/
    no, it really didn't
  • Anonymous (unregistered) in reply to ounos
    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.

  • SecCodr (unregistered) in reply to TopCod3r

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

  • SecCodr (unregistered) in reply to TopCod3r
    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.

  • (cs) in reply to Andy Goth
    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.

  • (cs)

    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.

  • File Not Found (unregistered) in reply to SecCodr
    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.

  • (cs) in reply to SecCodr
    SecCodr:
    TopCod3r:
    ...

    I hope you are joking...

    Shark Tank has JIM THE BOSS. TDWTF has TopCod3r.

  • St Mary's Hospital for the Grizzly Bears (unregistered) in reply to mjk340
    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?

  • Dominic (unregistered)

    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.

  • (cs)
    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.
  • (cs) in reply to ParkinT
    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!

  • K (unregistered) in reply to TopCod3r
    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???

  • diaphanein (unregistered) in reply to Maurits
    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.

  • Kender (unregistered) in reply to Dominic
    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 :(

  • anonymous (unregistered) in reply to ShatteredArm

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

  • (cs) in reply to anonymous
    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.

  • anonymous (unregistered) in reply to TopCod3r
    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;

  • (cs) in reply to TopCod3r
    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!

  • Max (unregistered)

    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.

  • (cs)

    In Soviet Russia, strings escape you!

  • ClutchDude (unregistered)

    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..."

  • (cs) in reply to Ozz
    Cue 50 references to Bobby Tables..

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

  • Mod Vinson (unregistered) in reply to anonymous
    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?

  • Walleye (unregistered) in reply to diaphanein
    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!

  • (cs) in reply to Kender
    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.

  • Bernie (unregistered) in reply to TopCod3r
    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.
  • Ross (unregistered)

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

  • M (unregistered) in reply to TopCod3r

    Please submit my request:

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

  • (cs) in reply to derula
    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.

  • Random (unregistered)

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

  • James (unregistered)

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

Leave a comment on “Security by Posterity”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article