ASP sanitising user input (SQL injection)

Durzel · 10 Sep 2008 at 14:42

EDIT: Have modified it - it was erroneously stripping off single - characters when it should only be wiping --, plus some other regexp tweaks

I've been working on some sites that haven't been properly validating user input and as a result all kinds of nastiness has been going on behind the scenes (e.g. UNION SELECT'ing to pull out admin details). I knocked together a function to sanitise user input and wanted to get some feedback (mainly as my regexp is hazy).

I designed it so that words like "SELECT", "DELETE", etc would be ok to pass so long as they weren't passed in combination with others to make up an SQL command (e.g. SELECT x FROM, DELETE FROM, etc)

Code:

Function SQLSafe(sTxt)
   'User input sanitizer to remove risk of SQL injection
   'by Darren Coleman ([email protected])
   'version 1.03 - 10/09/2008
   '  1.00 - Initial release
   '  1.01 - Modified regexp to wipe rest of string if SQL command sequence detected
   '  1.02 - Modified SQL sequence checks to handle multiple spaces between commands
   '  1.03 - Fixed broken "--" detection (was deleting all - characters), tweaked regexp slightly
   
   Dim uberSQLInjExp
   Set uberSQLInjExp = New RegExp
   
   'The following regexp will match HTML tags, sequence combinations of 
   'SQL commands as well as the more obvious control characters & SQL terminators
   With uberSQLInjExp
      .Pattern = 	"<[^>]+>|" & _
                  "SELECT((.|\s)*?)FROM((.|\s)*?)$|" & _
                  "UPDATE((.|\s)*?)SET((.|\s)*?)$|" & _
                  "INSERT[\s]+INTO((.|\s)*?)$|" & _
                  "DELETE[\s]+FROM((.|\s)*?)$|" & _
                  "(DROP|CREATE|ALTER|TRUNCATE)[\s]+TABLE[\s]+((.|\s)*?)$|" & _
                  "UNION[\s]+(ALL|SELECT){1}[\s]+((.|\s)*?)$|" & _
                  "[;|\r?\n|\r|\x00|\x1a]|[-]{2}"			
      .IgnoreCase = True
      .Global = True
   End With
   
   SQLSafe = uberSQLInjExp.Replace(sTxt,"")
   'Double-quote any single quotes in the text
   SQLSafe = Replace(SQLSafe,"'","''")
   
   Set uberSQLInjExp = Nothing
End Function

If anyone has any optimisations or suggestions for the above I'd be very grateful if they'd share their knowledge

GravyMonster · 10 Sep 2008 at 15:10

Very nice *saves*

Much nicer and neater than the function I've been using for the last couple of years. It's something I've been looking at improving for a while, but never had the chance.

Here's what I've been using, which I found on the net and modded slightly.

Code:

' =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= \/ \/ \/ \/ =-
'	ValidateInput
'		IN:	String:		User entered data for validation
'			Int:		Preset for determining which characters are allowed
'			Boolean:	Flag null values
'		OUT:	Boolean:	Validated?
' =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
const C_DataType_Alpha				= 1
const C_DataType_AlphaNumeric			= 2
const C_DataType_Numeric			= 4
const C_DataType_Hex				= 8
const C_DataType_Login				= 16
const C_DataType_Date				= 32
const C_DataType_FreeText			= 64

public function ValidateInput(p_Data, p_Datatype, p_AllowNulls)
	dim m_AlphaChars, m_AlphaNumChars, m_NumChars, m_HexChars, m_DateChars, m_IsValid, m_Char
	
	m_IsValid = true

	m_AlphaChars			= "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'-() "			' C_DataType_Alpha
	m_AlphaNumChars			= "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789&!,.'?()- "	' C_DataType_AlphaNumeric
	m_NumChars			= "0123456789."									' C_DataType_Numeric
	m_HexChars			= "ABCDEF0123456789"								' C_DataType_Hex
	m_DateChars			= "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 /\-."		' C_DataType_Date
	
	' Check for null value
	if not p_AllowNulls then
		if len(p_Data & "") <= 0 then
			m_IsValid = false
		end if
	end if
	
	if p_Datatype <> C_DataType_FreeText then
		if m_IsValid then
			' only perform checks for bad data if we are still valid to this point - for the sake of performance
			select case p_Datatype
				case C_DataType_Alpha
					for i = 1 to len(p_Data)
						m_Char = mid(p_Data, i, 1)
						if (instr(m_AlphaChars, m_Char) = 0) then
							m_IsValid = false
						end if
					next
					
				case C_DataType_AlphaNumeric
					for i = 1 to len(p_Data)
						m_Char = mid(p_Data, i, 1)
						if (instr(m_AlphaNumChars, m_Char) = 0) then
							m_IsValid = false
						end if
					next
					
				case C_DataType_Numeric
					for i = 1 to len(p_Data)
						m_Char = mid(p_Data, i, 1)
						if (instr(m_NumChars, m_Char) = 0) then
							m_IsValid = false
						end if
					next
					
				case C_DataType_Hex
					for i = 1 to len(p_Data)
						m_Char = mid(p_Data, i, 1)
						if (instr(m_HexChars, m_Char) = 0) then
							m_IsValid = false
						end if
					next
				
				case C_DataType_Date	
					for i = 1 to len(p_Data)
						m_Char = mid(p_Data, i, 1)
						if (instr(m_DateChars, m_Char) = 0) then
							m_IsValid = false
						end if
					next
					
				case else
					m_IsValid = false
					
			end select
		end if
	end if
	
	if m_IsValid then
		' only perform checks for bad data if we are still valid to this point - for the sake of performance
		if instr(p_Data, "--") then
			' CHECK FOR SQL COMMENT!
			m_IsValid = false
		end if
		
		' SQL Keywords 
		if ((instr(1, p_Data, "DROP ", 1) > 0) or _
			(instr(1, p_Data, "SELECT ", 1) > 0) or _
			(instr(1, p_Data, "UPDATE ", 1) > 0) or _
			(instr(1, p_Data, "INSERT ", 1) > 0) or _
			(instr(1, p_Data, "DELETE ", 1) > 0) or _
			(instr(1, p_Data, "CREATE ", 1) > 0)) then
			m_IsValid = false
		end if
	end if
	
	ValidateInput = m_IsValid
end function

Durzel · 10 Sep 2008 at 15:22

Cool

I should point out that the one I've written was done from scratch so I don't know how optimal it is - I haven't really gone deep into regexp for a while so there might be issues with it. It's provided "as is"

It's not as advanced as the one you've pasted, it doesn't actually validate the input (string, numeric, date, etc) as I do that elsewhere. Just needed a one-size-fits-all encapsulated function that would deal with some of the attacks we've been seeing. My function strips out HTML tags as well which may or may not be desireable.

Durzel · 16 Sep 2008 at 14:58

I've made some more changes/improvements to this but wanted to find out if anyone actually was bothered about me posting them or not - thought I'd be doing fellow ASP programmers a favour but no one seems to care about SQL injection problems?

(plus I still need help optimising my regexp)

markeh · 16 Sep 2008 at 18:03

Please do, I can't remember exactly what mine does, think it picks up the usual injected SQL commands (DELETE INSERT DECLARE etc) and blocks them, which can be annoying. It's basically the blacklist script which is on the net but modified a little.

RockLobster · 16 Sep 2008 at 18:38

I use SQL parameters so I shouldn't have a problem with SQL injection, right?

markeh · 16 Sep 2008 at 19:59

Yeah, as long as your not using variables in the SQL query string you should be fine.

Durzel · 17 Sep 2008 at 12:17

Sorry for delay replying, below is the latest version.

I've added two more regexps for "common SQL attacks" (the DECLARE .. CHAR and CONVERT .. SP_PASSWORD checks). I am not sure whether it is a good idea to add regexps for specialist attacks though as whilst it means it filters them out properly I am a little worried about the performance hit of more and more regexp'ing. Interested to hear thoughts...

(I've commented out a section in the function which links in with another function I've written but haven't included below - basically a system which emails the support team if a potential hack is detected)

Code:

Function SQLSafe(sTxt)
	'User input sanitizer to remove risk of SQL injection
	'by Darren Coleman ([email protected])
	'  1.00 - Initial release (10/09/2008)
	'  1.01 - Modified regexp to wipe rest of string if SQL command sequence detected
	'  1.02 - Modified SQL sequence checks to handle multiple spaces between commands
	'  1.03 - Fixed broken "--" detection (was deleting all - characters), tweaked regexp slightly
	'  1.04 - Added abuse-mail feature, added DECLARE x VARCHAR regexp, reclassified ; non-malicious
	'  1.05 - Added AND..CONVERT..SP_PASSWORD regexp
	
	Dim uberSQLInjExp
	Set uberSQLInjExp = New RegExp
	
	'The following regexp will match HTML tags, sequence combinations of 
	'SQL commands as well as the more obvious control characters & SQL terminators
	With uberSQLInjExp
		.Pattern = "<[^>]+>|" & _
			"SELECT((.|\s)*?)FROM((.|\s)*?)$|" & _
			"UPDATE((.|\s)*?)SET((.|\s)*?)$|" & _
			"INSERT[\s]+INTO((.|\s)*?)$|" & _
			"DELETE[\s]+FROM((.|\s)*?)$|" & _
			"(DROP|CREATE|ALTER|TRUNCATE)[\s]+TABLE[\s]+((.|\s)*?)$|" & _
			"UNION[\s]+(ALL|SELECT){1}[\s]+((.|\s)*?)$|" & _
			"DECLARE((.|\s)*?)[\s]+(NVARCHAR|VARCHAR|CHAR){1}((.|\s)*?)$|" & _
			"AND[\s]+((.|\s)*?)CONVERT((.|\s)*?)SP_PASSWORD$|" & _
			"[\r?\n|\r|\x00|\x1a]|[-]{2}"			
		.IgnoreCase = True
		.Global = True
	End With
	
	SQLSafe = uberSQLInjExp.Replace(sTxt,"")
	
	'Sanitize input which probably isn't malicious but we don't want anyway
	SQLSafe = Replace(SQLSafe,"'","''")
	SQLSafe = Replace(SQLSafe,";","")
	
	'If SEND_ABUSE_MAIL And uberSQLInjExp.Test(sTxt) Then
	'	sendAbuseMail sTxt, SQLSafe
	'End If
	
	Set uberSQLInjExp = Nothing
End Function

Durzel · 17 Sep 2008 at 12:33

I've also toyed with the idea of changing ((.|\s)*?) (match whitespace or any character zero or more times) to (\s+.+\s+) (match one or more spaces, any character one or more times and one or more spaces)...

The idea being that the first one would match:

SELECTS anything FROM sometable
SELECT anything SFROM sometable

..which I guess could be valid input, whereas the second would only match:

SELECT anything FROM sometable

Any thoughts?

Shoseki · 17 Sep 2008 at 14:11

To quote wikipedia :

Wikipedia said:
In PHP, it's usual to just escape the parameters before sending the SQL query:

$query = sprintf("SELECT * FROM Users where UserName='%s' and Password='%s'",
mysql_real_escape_string($Username),
mysql_real_escape_string($Password));
mysql_query($query);

For PHP version 5 and MySQL version 4.1 and above, however, you can use extension like msqli for "true" prepared statement queries: [3]

$db = new mysqli("localhost", "user", "pass", "database");
$stmt = $mysqli -> prepare("SELECT priv FROM testUsers WHERE username=? AND password=?");
$stmt -> bind_param("ss", $user, $pass);
$stmt -> execute();

as for html... I'll get back to you on that...

EDIT : DOH just seen this is for ASP XD

markeh · 17 Sep 2008 at 14:22

Cheers Durzel.

This is what I'm using btw:

Code:

<% 

Dim BlackList, ErrorPage, s

BlackList = Array("--", ";", "/*", "*/", "@@", "@",_
                  "char", "nchar", "varchar", "nvarchar",_
                  "alter", "begin", "cast", "create", "cursor",_
                  "declare", "delete", "drop", "end", "exec",_
                  "execute", "fetch", "insert", "kill", "open",_
                  "select", "sys", "sysobjects", "syscolumns",_
                  "table", "update", "<script", "</script>", "'")


ErrorPage = "ErrorPage.asp"
               
Function CheckStringForSQL(str) 
  On Error Resume Next 
  
  Dim lstr 
  
  ' If the string is empty, return true
  If ( IsEmpty(str) ) Then
    CheckStringForSQL = false
    Exit Function
  ElseIf ( StrComp(str, "") = 0 ) Then
    CheckStringForSQL = false
    Exit Function
  End If
  
  lstr = LCase(str)
  
  For Each s in BlackList
  
    If ( InStr (lstr, s) <> 0 ) Then
      CheckStringForSQL = true
      Exit Function
    End If
  
  Next
  
  CheckStringForSQL = false
  
End Function 


Dim AValue
Dim AVar
For Each s in Request.Form
AVar = Request.form.key(s)
AValue = Request.Form(s)
  If ( CheckStringForSQL(Request.Form(s)) ) Then
  
    ' Redirect to an error page
    Response.Redirect(ErrorPage & "?AVar=" & AVar & "&string=" & AValue & "&script-name=" & Request.Servervariables("SCRIPT_NAME"))
  
  End If
Next


For Each s in Request.QueryString
AVar = Request.QueryString.key(s)
AValue = Request.QueryString(s)
  If ( CheckStringForSQL(Request.QueryString(s)) ) Then
  
    Response.Redirect(ErrorPage & "?AVar=" & AVar & "&string=" & AValue & "&script-name=" & Request.Servervariables("SCRIPT_NAME"))

    End If
  
Next

For Each s in Request.Cookies
AValue = Request.Cookies(s)
  If ( CheckStringForSQL(Request.Cookies(s)) ) Then
  
    ' Redirect to error page
    Response.Redirect(ErrorPage & "?string=" & AValue)

  End If
  
Next


Function InjectCheck(theStr) 
Dim SQLexFound

	theStr = Server.HTMLEncode(theStr)
	SQLexFound = CheckStringForSQL(theStr)
	If(SQLexFound = False) Then
		InjectCheck = theStr
	End If
End Function
%>

The error page it redirects to logs the attempt (which variable it tried it on, the string etc) just to make it easier to find the flaws (if any).

Durzel · 17 Sep 2008 at 14:22

Yeah, if we were talking PHP then it wouldn't be a concern as mysql_real_escape_string() does the job well.

markeh · 17 Sep 2008 at 15:24

Also if you know the variable is going to a number (i.e. an ID) you can just use CLng() which converts the variable to a Long Int. It'll just error if non-numeric characters are passed to it.

Another bit of protection is to check string lengths, if you know the contents of the variable should always be under a certain length, check the length to see if it is under what you expect.