The topic is important but there is several errors in the first part of the article so its obvious the author does not really know what hes talking about other than repeating what hes read on other webpages.
I have to say I agree with svenarild, the author of this article lacks any real experience with either SQL or PHP to properly write an article on protection against SQL Injections. There was a lot of bad/improper information and methods.
I appreciate both of your replies but I'm curious: Where in my blog post was there an error? I also have to correct exsecror: I do have "real experience" with SQL and PHP. Perhaps my article does not correctly show my level of skill in these two subjects, but I am definitely not a newbie. Finally, I would like to ask that if there were "bad/improper information and methods" that you post them here so the general public can benefit from the knowledge of how to correctly prevent MySQL injection attacks.
Sure John, here is a few errors and issues from your article.
The first issue is your injection example, if you take a closer look you will notice it would not work. In the query you md5 hash the password field, so the "injection" would be hashed into a 32 character long string. You also might want to use an example closer to a real world situation, no one checks for the username and password at once in a query anymore as it makes it difficult to log attempts. Also if you show hashing of a password at least add in a salt.
The second error is that you mention "Even more unscrupulous users (read: the real jackasses) could send in multiple queries including DELETE queries." even through you mention MySQL all over the article. MySQL does not permit query stacking nor does it allow executing multiple queries in a single function call. If you try this, the entire query fails. I.e. "user'; DELETE FROM tbl_Users;" will not work on MySQL even if your able to inject it properly (Keep in mind that this is an important point, so it should be mentioned. But you should let your readers know its not an issue with MySQL).
For escaping the values, mysql_real_escape_string() works; you can also cast any bool/integer/float values. But you should really use prepared statements instead, either PDO or mysqli (note that PDO has its weaknesses, so if you use stored procedures in MySQL that return multiple columns you are better off with mysqli).
In your final point, number three. You mention proper MySQL syntax; there is no need to use the reverse apostraphe at all in queries. The only reason its there is so you are able to use reserved words as table and column names. Though you should never use reserved words for that anyway. For the single quotes, you should always use it around fields expecting a string/character. For values (int/float) you dont need to use them. (Note, the query you used as the first example would fail as a username usally contain characters as well)
I know my reply might seem a little harsh, but I personally feel the issues I mentioned are major ones that you should have noticed when reviewing the article before posting it as they are imo very obvious. Though I applaud you for asking what the issues are, I hope you will update your article to reflect what I mentioned above. If you need anyone to proof read the updated version, let me know.
I will definitely take you up on your offer to proof the revised version . The things you mention in your reply are things that would very much help to improve my article and indeed the level of detail required when preventing mysql injection attacks. I have spent a bit of time researching the subject and have never really been satisfied with what's out there. I guess in my attempt to improve that it ended up being "more of the same." So, I would like to take the opportunity to write a more comprehensive article based off of the information in your reply.
Hate to reply to my own post, but I've updated the article to take advantage of your generosity. Please take a look and let me know if you want to yell at me some more :)
John:
Sorry for the delay, did not notice you had posted a comment.
There is only thing that you should consider changing and that is:
Moving the prepared statements under step 1, or move it to step 2.
The key with prepared statements is that you do not need to escape the value with mysql_real_escape_string() etc. As they are escaped automatically when you bind the value. With other words, you should make it clear that they can use either one, but that they should not use both at the same time.
I would also recommend that you add PDO as an option in the prepared statments text. Unless your using advanced stored procedures, its a very good choice as well.
Other than this the article looks ok. There is a few things that I would have written different, but there is no issues or errors in the content; so I would say its good to go :)
Thanks Sven, I will implement your changes. Thank you again for the great feedback.
Dmitryx: I realize that my original post may not have been the most informative or intelligent article in the world, but thanks to some of the feedback I've received here on Dzone I've managed to improve both my techniques as well as the article I wrote. Isn't that the point of this whole endeavor? To be a better coder in the evening than you were in the morning? I think so.
Well, this way you wont be any better.
The right thing to do is open php manual and read it. Just one or two pages, no big deal. And there is also google, with millions articles on SQL injection. And sure, people who find this article any good had never read official manual. Well, bad for them. What kind of programmer any person is, if he or she doesnt even know things written on top of the manual?
http://php.net/results.php?q=injection&p=manual&l=en
Comments
Sven Arild Helleland replied ago:
The topic is important but there is several errors in the first part of the article so its obvious the author does not really know what hes talking about other than repeating what hes read on other webpages.
exsecror.pip.verisignlabs.com replied ago:
I have to say I agree with svenarild, the author of this article lacks any real experience with either SQL or PHP to properly write an article on protection against SQL Injections. There was a lot of bad/improper information and methods.
John Rockefeller replied ago:
I appreciate both of your replies but I'm curious: Where in my blog post was there an error? I also have to correct exsecror: I do have "real experience" with SQL and PHP. Perhaps my article does not correctly show my level of skill in these two subjects, but I am definitely not a newbie. Finally, I would like to ask that if there were "bad/improper information and methods" that you post them here so the general public can benefit from the knowledge of how to correctly prevent MySQL injection attacks.
Thanks!
Sven Arild Helleland replied ago:
Sure John, here is a few errors and issues from your article.
The first issue is your injection example, if you take a closer look you will notice it would not work. In the query you md5 hash the password field, so the "injection" would be hashed into a 32 character long string. You also might want to use an example closer to a real world situation, no one checks for the username and password at once in a query anymore as it makes it difficult to log attempts. Also if you show hashing of a password at least add in a salt.
The second error is that you mention "Even more unscrupulous users (read: the real jackasses) could send in multiple queries including DELETE queries." even through you mention MySQL all over the article. MySQL does not permit query stacking nor does it allow executing multiple queries in a single function call. If you try this, the entire query fails. I.e. "user'; DELETE FROM tbl_Users;" will not work on MySQL even if your able to inject it properly (Keep in mind that this is an important point, so it should be mentioned. But you should let your readers know its not an issue with MySQL).
For escaping the values, mysql_real_escape_string() works; you can also cast any bool/integer/float values. But you should really use prepared statements instead, either PDO or mysqli (note that PDO has its weaknesses, so if you use stored procedures in MySQL that return multiple columns you are better off with mysqli).
In your final point, number three. You mention proper MySQL syntax; there is no need to use the reverse apostraphe at all in queries. The only reason its there is so you are able to use reserved words as table and column names. Though you should never use reserved words for that anyway. For the single quotes, you should always use it around fields expecting a string/character. For values (int/float) you dont need to use them. (Note, the query you used as the first example would fail as a username usally contain characters as well)
I know my reply might seem a little harsh, but I personally feel the issues I mentioned are major ones that you should have noticed when reviewing the article before posting it as they are imo very obvious. Though I applaud you for asking what the issues are, I hope you will update your article to reflect what I mentioned above. If you need anyone to proof read the updated version, let me know.
John Rockefeller replied ago:
Hi Sven, thanks for your reply.
I will definitely take you up on your offer to proof the revised version . The things you mention in your reply are things that would very much help to improve my article and indeed the level of detail required when preventing mysql injection attacks. I have spent a bit of time researching the subject and have never really been satisfied with what's out there. I guess in my attempt to improve that it ended up being "more of the same." So, I would like to take the opportunity to write a more comprehensive article based off of the information in your reply.
How do I contact you with my updated article?
John Rockefeller replied ago:
Hate to reply to my own post, but I've updated the article to take advantage of your generosity. Please take a look and let me know if you want to yell at me some more :)
dmitryx replied ago:
here we go.
d in dzone is goind to stand for dumb-people.
scriptone replied ago:
yeah, the d in dimistry as well (check out this site if you don't believe me http://dmitryx.com/)
webid replied ago:
finally some productive 'discussion' in the comments!
Dmitryx: only one D? so humble
Sven Arild Helleland replied ago:
John:
Sorry for the delay, did not notice you had posted a comment.
There is only thing that you should consider changing and that is:
Moving the prepared statements under step 1, or move it to step 2.
The key with prepared statements is that you do not need to escape the value with mysql_real_escape_string() etc. As they are escaped automatically when you bind the value. With other words, you should make it clear that they can use either one, but that they should not use both at the same time.
I would also recommend that you add PDO as an option in the prepared statments text. Unless your using advanced stored procedures, its a very good choice as well.
Other than this the article looks ok. There is a few things that I would have written different, but there is no issues or errors in the content; so I would say its good to go :)
dmitryx:
Got to love your positive thinking :)
John Rockefeller replied ago:
Thanks Sven, I will implement your changes. Thank you again for the great feedback.
Dmitryx: I realize that my original post may not have been the most informative or intelligent article in the world, but thanks to some of the feedback I've received here on Dzone I've managed to improve both my techniques as well as the article I wrote. Isn't that the point of this whole endeavor? To be a better coder in the evening than you were in the morning? I think so.
Thanks to all who have helped and replied.
dmitryx replied ago:
Well, this way you wont be any better.
The right thing to do is open php manual and read it. Just one or two pages, no big deal. And there is also google, with millions articles on SQL injection. And sure, people who find this article any good had never read official manual. Well, bad for them. What kind of programmer any person is, if he or she doesnt even know things written on top of the manual?
http://php.net/results.php?q=injection&p=manual&l=en
Voters For This Link (13)
Voters Against This Link (5)