Sanitisation of input for a stored MySQL database procedure

Question

I am using a database someone else produced (and I am not really authorised to change it). However, as I was looking into the stored procedures within the database I noticed the following procedure:

DELIMITER $$

CREATE PROCEDURE `logIn`(userName varChar(50), userPass varChar(50))
BEGIN
  declare userID int;

  SELECT 
    u.userID INTO userID 
  FROM
    users u
  WHERE
    u.userName=userName 
    AND u.userPassword=MD5(userPass);

  IF (IFNULL(uID,-1) > 0) THEN
    select 1 as outMsg;
  ELSE
    select 0 as outMsg;
  END IF;
END$$

with the corresponding table users having three columns: userID INT, userName VARCHAR(50) and userPassword VARCHAR(50).

As I am not very good at this, could someone let me know whether the input for such a function needs to be sanitised as to not allow any SQL injections and if not - why? A general rule of thumb would be very much appreciated.

P.S. This function will be called from a JS script on a form submit.

The procedure itself is not vulnerable because the parameters `uName` and `uPass` will be correctly handled by the RDBMS and you are not executing a dynamic SQL string. However, _how you call the procedure_ in your code could be vulnerable... We would need to see that. — Michael Berkowski, Dec 17 '12 at 11:56
Sanitizing input with javascript will never be secure. This has to be done on the server. — Jørgen, Dec 17 '12 at 11:58

score 1 · Accepted Answer · answered Dec 17 '12 at 12:25

1

There are a few rules of thumb here that depend on the underlying datatype and how it's inserted into the database.

First, Parameterized queries are always best for SQL Injection protection.. but.. if you can't change that..

String type:

Remove any single quotes OR Replace any single quotes with the single quote twice.
Replace any of the following characters with their encoded alternative;
- >
- <
- "
- ;
- (chr 34)
- )
- (
- For example.. ) is replaced with & #x29;
  
  -(the space in the above example is so you'll see the code, remove it to get ")")

For a datatype other then string, check that the datatype is sane and remove any character that shouldn't be in the datatype. If it's an integer, make sure the string that you're passing in is an integer. This can commonly be done by casting to the type in code. The cast will either work.. or cause an error. It's also good to check that the datatype min and maxes have not been exceeded. For example.. If I was checking for an integer, I might use code similar to this:

var myInt = parseInt(param);

Then I might check it's bounds to be sure it's less then the maximum integer value and greater then the minimum integer value.

That should be good enough to prevent a SQL Injection attack...

And.. since you have not posted the code that actually interfaces with the database... As an added precaution.. you may also want to remove --,`,%,",", "".

You only want 'sane' values getting to the database call.. so an integer like, $309 wouldn't make sense, you'd want to remove the $.. . probably by using a regex replace for any non numeric characters a comma and a period. [^[0-9,.]]

Be extra cautious.

answered Dec 17 '12 at 12:25

D CO

91
4

for php, follow: http://stackoverflow.com/questions/60174/how-to-prevent-sql-injection – D CO Dec 17 '12 at 12:36
From what I can gather, the simple fact that the above php uses mysqli should be sufficient to ensure that the query is safe. Am I correct in that conclusion? – YEAellyn Dec 17 '12 at 13:00
No. For that to be the case, you must bind the parameter. $stmt = $mysqli->prepare("INSERT INTO table (column) VALUES (?)"); // TODO check that $stmt creation succeeded // "s" means the database expects a string $stmt->bind_param("s", $unsafe_variable); – D CO Dec 17 '12 at 13:08
Yeah I get that the `prepare` statement actually makes the code safe. However, doesn't `sprintf` ensure that the one I provided is safe, too? Please, if possible, provide a short example if that isn't that case. – YEAellyn Dec 17 '12 at 13:10
What you need to do is $stmt = $mysqli->prepare("call logIn(?,?)"); $stmt->bind_param('ss',$uName, $uPSW); $stmt->Execute(); $stmt->bind_result($result); $stmt->fetch(); echo $result; $stmt->close(); $mysqli-close(); – D CO Dec 17 '12 at 13:28
I am about to mark this as the accepted answer as it is exactly what I was looking for, just one last thing if you don't mind showing me a potentially malicious call with the setup I currently have so that I can see it in practice, that would be great! Thank you! – YEAellyn Dec 17 '12 at 13:43
$uPSW = "x'); DROP TABLE members; --" would delete a table called members. be careful with this.. for obvious reasons. You might also be able to get away with: $uPSW = "x'); select 1 as outMsg; --" and log in as any user.. depending on how the results are processed. – D CO Dec 17 '12 at 14:19

score 0 · Answer 2 · answered Dec 17 '12 at 11:56

0

Yes, the input must be sanitized before trying to run the procedure.

You might want to share the actual calling point for the procedure to get more help here, since there is no way that the procedure is called directly from JS on form submit. You probably have a Servlet, PHP page or some HTTP friendly intermediary to make the database call somehow.

answered Dec 17 '12 at 11:56

Pablo Santa Cruz

176,835
32
241
292

I have included the PHP page where the call is made, I had not noticed before that it was actually called from within PHP code. Sorry for the confusion, as I said I am not very good at this. – YEAellyn Dec 17 '12 at 12:05

Sanitisation of input for a stored MySQL database procedure

2 Answers2