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

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