Take the 2-minute tour ×
Database Administrators Stack Exchange is a question and answer site for database professionals who wish to improve their database skills and learn from others in the community. It's 100% free, no registration required.

I am using MySql database and I have a stored procedure to insert data, like this-

CREATE DEFINER=`root`@`localhost` PROCEDURE `adduser`(
    IN id varchar(10),
    IN pwd varchar(10),
    IN fname varchar(45),
    IN lname varchar(45),
    IN email varchar(45),
    IN phone varchar(13),
    IN address varchar(145),
    IN r1 varchar(45),
    IN r2 varchar(45),
    IN r3 varchar(45),
    IN r4 varchar(45),
    IN r5 varchar(45),
    IN r6 varchar(45),
    IN r7 varchar(45),
    IN r8 varchar(45)
)
BEGIN
    insert into users 
    values(id,pwd,fname,lname,email,phone,address,'yes',r1,r2,r3,r4,r5,r6,r7,r8);
END

In my Asp.net website I have some code like this which calls the stored procedure-

using (cmd = new MySqlCommand("CALL vtsdb.adduser('" + txt_userId.Text + "','" + txt_pwd.Text + "','" + txt_firstName.Text + "','" + txt_lastName.Text + "','" + txt_emailId.Text + "','" + txt_phoneNo.Text + "','" + txt_address.Text + "','" + txt_f1.Text + "','" + txt_f2.Text + "','" + txt_f3.Text + "','" + txt_f4.Text + "','" + txt_f5.Text + "','" + txt_f6.Text + "','" + txt_f7.Text + "','" + txt_f8.Text + "')", con)) 
            {
                cmd.ExecuteNonQuery();
                con.Close();
            }

What I would like to know-

Do I need to use sql parameters in the above code? I dont see my stored procedure vulnerable to a Sql injection attack. Please guide me if I am wrong.

Also what is the use of mentioning CommandType.StoredProcedure in my c# code while my code still executes well?

share|improve this question
    
I would rewrite this into using parameters. As written now it is quite unreadable. And you can never be really sure that you are prone to SQL injection or not. (Well, you can, but why to invite the devil?) –  dezso Nov 23 '12 at 9:32
add comment

2 Answers 2

up vote 1 down vote accepted

Your stored procedure, itself, in a vacuum, as written now, does not appear vulnerable to SQL injection... but that is not the only consideration, and is not the final answer.

It's always possible that it could be updated later and become vulnerable, but there's yet something else to consider.

For a much bigger potential problem, take one step back and look at your calling convention.

If someone could manage to pass the following string value to your application as txt_f8.Text... it would be obediently concatenated into your MySqlCommand()...

stuff'); GRANT ALL PRIVILEGES ON *.* TO 'hacker'@'%' IDENTIFIED BY 'pwn3d'; SELECT ('1

...then what would be executed by your server?

Possibly, nothing. Possibly, way too much.

This randomly-googled tutorial reinforces the point of not crafting queries with string concatenation, and using SqlParameter instead:

This situation invites a hacker to replace that string with something malicious. In the worst case, you could give full control of your computer away.

Instead of dynamically building a string, as shown in the bad example above, use parameters. Anything placed into a parameter will be treated as field data, not part of the SQL statement, which makes your application much more secure.

Certainly not a good trade-off for having to do less typing.

I'm a MySQL DBA and not a .NET person, but from what I can tell from a quick search it appears that declaring CommandType.StoredProcedure allows you to take better advantage of the capabilities of stored procedures, by making some of the parameters INOUT or OUT, and not just IN. This might be useful, for example, if you wanted one of the parameters to be the ID of the newly-inserted user, which the SP could return to your code.

If you don't need return values, you don't have to do it that way... but feedback/confirmation isn't usually a bad thing.

share|improve this answer
    
That's pleasing enough and thanks for a detailed answer. Your example has given me insights on sql injection –  Cdeez Nov 27 '12 at 4:58
add comment

You need to use the SqlParameters in your code, If your sure that there is no possibility of the Sql injection attack, you code is perfect. There might chance for the code maintenance problem to understand what are all the parameters passed. The best practice is use the SqlParameter to pass the values to the SQL statement from out .NET code.

Stored procedures are pre-compiled. If there are any changes to the query we can directly connect to the database server and edit. Code maintenance becomes easier.

Hope this helps!

share|improve this answer
add comment

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.