Understanding the use of Sql command parameters

Question

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?

Answer 1

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.

Answer 2

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!