SQL injection

[johnsonlim026]

Hi Experts,
My system is now having a serious problem that is SQL injection.Almost all the insert or update sql are written in this way:

Insert into tableName(field1, field2, field2) values('"& value1 &"','"& value2 &"','"& value3 &"')

I have founded out two solutions for this
(1). Add ' when ' is found.
Example :
Insert into tableName(field1, field2, field2) values('"& replace(value1,"'","''") &"','"& replace(value2,"'","''") &"','"& replace(value2,"'","''") &"')

(2).Use parameter
Example : dbconn.MyCommand.CommandText = "select id from tpuser.userid where id=?id "
dbconn.MyCommand.Parameters.Add("?id", Me.txtUserID.Text)
dbconn.MyCommand.Parameters.Add("?pwd", Me.txtPwd.Text)

I am now in the way of implementing the second solution into my system.I am thinking a fastest way to solve this but have no idea to do this.
Does anyone has any idea in this?Is it possible to set up a global function to be used for the whole system?
Thank you.

[TheCPUWizard]

#2 (neglecting some syntax/typo errors) is the only way to go.

ps: Fire the person who wrote the original code....

[Arjay]

Use stored procedures?

[hspc]

I'm not sure what technologies you use. If using .net, Enterprise Library application blocks can save some time.

[TheCPUWizard]

Stored Proedures - The actual SQL envrionment is nt specified, so this may or may not be a viable alternative.

Application Blocks - These are great and can help build on the basic structure.

[johnsonlim026]

Dear Experts,

I am using MYSQL.After reading the posts, i decide to use a store procedure to solve this issuse. My stored procedure is as below



DELIMITER $$

DROP PROCEDURE IF EXISTS `tphr`.`sp_SQL` $$
CREATE PROCEDURE `tphr`.`sp_SQL` (IN strSql varchar(100))
BEGIN


PREPARE stmt1 FROM 'strSql';
EXECUTE stmt1;
DEALLOCATE PREPARE stmt1;



END $$

DELIMITER ;



but this give me error unless i have to write the statement inside and this does not serve the purpose.
For example :PREPARE stmt1 FROM 'SELECT "ok"';
I have try with only EXECUTE strSql but this also give me error. Does anyone has any idea on this?

[Arjay]

What you would do is write specific store procedures that would do the operation for you passing in the required parameters.

Here's a MS SQL example of deleting an entry from a table.

CREATE PROCEDURE [dbo].[Schedule.Template.Delete]
@TemplateID UNIQUEIDENTIFIER
AS
BEGIN
DELETE FROM [dbo].[Schedule.Template] WHERE [ScheduleTemplateID] = @TemplateID
END

[johnsonlim026]

hi Arjay, thanks for your suggestion.I am thinking a way to solve SQL injection with the fastest time frame.One of my idea is to put all inside a stored procedure and execute it through the stored procedure and this may avoid SQL injection probelm that i face.My problem now is it possible to do this under MySql because i ahev try few way but it does not work.

[Arjay]

The problem with injection is that unwanted code gets executed. If you allow passing in a sql statement to your stored procedure, it really defeats the purpose of having a stored procedure (that is unless you verify the sql statement inside the sproc).

[johnsonlim026]

Public Function ExecuteNonQuery(ByVal strQry As String, Optional ByVal htParam As Hashtable = Nothing) As Integer
Try
Dim intRetVal As Integer
Dim sqlcom As New MySqlCommand(strQry, Conn)
sqlcom.CommandText = strQry

If Not htParam Is Nothing Then
Dim myEnumerator As IDictionaryEnumerator = htParam.GetEnumerator()
While myEnumerator.MoveNext()
If myEnumerator.Key.ToString() <> Nothing And myEnumerator.Value.ToString() <> Nothing Then
sqlcom.Parameters.Add(myEnumerator.Key.ToString(), myEnumerator.Value.ToString())
Else
sqlcom.Parameters.Add(myEnumerator.Key.ToString(), DBNull.Value)
End If

End While
End If
intRetVal = sqlcom.ExecuteNonQuery()
Return intRetVal
Catch ex As Exception
Throw ex
End Try
End Function




Public Sub ExecuteReader(ByVal strQry As String, ByVal htParam As Hashtable, ByRef dt As DataTable)
Try
Dim sqlcom As New MySqlCommand(strQry, Conn)
Dim myEnumerator As IDictionaryEnumerator = htParam.GetEnumerator()
While myEnumerator.MoveNext()
sqlcom.Parameters.Add(myEnumerator.Key.ToString(), myEnumerator.Value.ToString())
End While
Dim sqlda As New MySqlDataAdapter(sqlcom)
sqlda.Fill(dt)
Catch ex As Exception
Throw ex
End Try


End Sub


Hi, i have got the solution.Thanks all.

[TheCPUWizard]

Dear Experts,

I am using MYSQL.After reading the posts, i decide to use a store procedure to solve this issuse. My stored procedure is as below



DELIMITER $$

DROP PROCEDURE IF EXISTS `tphr`.`sp_SQL` $$
CREATE PROCEDURE `tphr`.`sp_SQL` (IN strSql varchar(100))
BEGIN


PREPARE stmt1 FROM 'strSql';
EXECUTE stmt1;
DEALLOCATE PREPARE stmt1;



END $$

DELIMITER ;



but this give me error unless i have to write the statement inside and this does not serve the purpose.
For example :PREPARE stmt1 FROM 'SELECT "ok"';
I have try with only EXECUTE strSql but this also give me error. Does anyone has any idea on this?

No, No, No, NO!!!!!!!!

That does not solve ANYTHING and only INCREASES the "attack surface". A person could still pass "DROP TABLE [CUSTOMER" and it would execute.

You MUST switch to Commands. When we have been talking about Stored Procedures, we have been talking about them being the TYPE of command used.

You MUST use paameter objects for each "variable"

You MUST NOT dynamically build the SQL Text (either in the program or in a stored proc, in any shape for for, it MUST be a single long hard coded string.

[johnsonlim026]

Thanks all !!

[KrisSimonis]

Argh.. SQL injection.. these days, this Should not be possible anymore, my suggestions:

1. Burn your original programmer at the stake
2. Hire a codemonkey to manually redo all your queries to use parameters
and/ore add stored procedures to the whole process. Not 1 SP for everything, but 1 for every specific query/nonquery you want to execute.
Things like Deleting specific records can be done globally by passing the table name and such to the SP in the code, but in the end, yes it will mean, you have to rewrite a LOT of your code to immunize yourself against this problem.
This is time consuming, annoying, and hard to test if it's all working properly, and if you changed all your queries to use parameters, hence my suggestion at 1 for your original programmer.

[TheCPUWizard]

1. Burn your original programmer at the stake


No, that just releases gases into the air that contribute to global warming.

MUCH better to slowly torture the programmer, then throw him/her into a landfill, where the useless body can actually do some good.... :D ;)

[Shuja Ali]

1. Burn your original programmer at the stake Or maybe just teach him what exactly needs to be done to avoid SQL injection.

[KrisSimonis]

No, that just releases gases into the air that contribute to global warming.

MUCH better to slowly torture the programmer, then throw him/her into a landfill, where the useless body can actually do some good.... :D ;)

Since when is burning at the stake wrong? It's been a tried and true method of getting rid of sinners and heathens since the dark ages. As for the torture, that happens before anyway, to get his confession.
NOBODY Expects the RIAA!! :eek:



Oh wait, that was supposed to be the spanish inquisition. :o