Am I safe against SQL injection

Question

Hello

I would like to know if I'm safe against SQL injection when I use something like that with PostgresSQL:

CREATE or REPLACE FUNCTION sp_list_name( VARCHAR )
RETURNS SETOF v_player AS '
   DECLARE
      v_start_name ALIAS FOR $1;
      r_player  v_player%ROWTYPE;
      v_temp VARCHAR;
   BEGIN
      v_temp := v_start_name || ''%'';
      FOR r_player IN
         SELECT first_name, last_name FROM v_player WHERE last_name like v_temp
      LOOP
         RETURN NEXT r_player;
      END LOOP;
      RETURN;
   END;
' LANGUAGE 'plpgsql' VOLATILE;

I want to use this function to list player's name beginning with a letter.

select * from sp_list_name( 'A' );

gives me players with last name beginning with A.

I tried to inject sql with

select * from sp_list_name( 'A; delete from t_player;--' );
select * from sp_list_name( '''; delete from t_player;--' );

Am I safe ?

Which case I could be injected ?

Regards

Answer 1

You are not generating SQL for yourself, so this looks safe (to me).

I do not know where the data you are calling your stored procedure is coming from. So you still have to guard against buffer overflows etc.

Answer 2

Rule #1 to prevent against sql injection: Sanitize all input that is coming from someone/something you cannot trust/have no control over.

The problem itself does not lie within the database code, but from the application that is executing those statements.

Answer 3

In terms of your procedure you seem safe as the variable in the SP won't be expanded into code, but you can still expose yourself if you don't use a parameterized query like "SELECT * FROM sp_list_name(?);" in your appplication code. Something like "SELECT * FROM sp_list_name('$start_name');" could be subverted by a user passing a start name of "');delete from t_player where last_name NOT IN ('". So use a parameterized query or sanity check your inputs in your program.

NB: To others, please note that a variable in a stored procedure will not expand into code even if it contains a ' or ;, (excluding passing it to EXECUTE, for which you would use quote_literal, not hand-rolled replace functions) so replacing ; or ' is totally unnecessary (in the stored procedure, the application using it is a different story, of course) and would prevent you from always finding the "tl;dr" or "O'Grady" teams.

Leo Moore, Karl, LFSR Consulting: v_temp_name in the stored procedure will NOT be expanded into code in the SP (no EXECUTE), the check would need to be done n the application, not the SP (or the OP could just use a parameterized query in their app code, instead). What others are suggesting is similar to worrying about

my $bar = "foo; unlink('/etc/password');"; 
my $baz = $bar;

actually running the unlink in the absence of an eval.

Answer 4

You could consider validating the content of

 v_start_name 

. Check the string for semi colons, comment chars, equals, etc. Remember to check for both the Chars and the Hex values. Remember to allow for a hyphenated name, e.g. 'Smith-Brown' is likely acceptable 'Smith--Brown' is potentially injection.

If not familiar with Hex in SQL Injection the following are quick intros

http://www.arejae.com/blog/sql-injection-attack-using-t-sql-and-hexadecimal.html

http://www.securityfocus.com/infocus/1768

DECLARE
      v_start_name ALIAS FOR $1;
      r_player  v_player%ROWTYPE;
      v_temp VARCHAR;
   BEGIN
      --  new pseudo code here
      if v_start_name has bad chars exit with error message
      -- end pseudo code here
      v_temp := v_start_name || ''%'';
      FOR r_player IN
         SELECT first_name, last_name FROM v_player WHERE last_name like v_temp
      LOOP
         RETURN NEXT r_player;
      END LOOP;
      RETURN;
   END;

Regards Karl

Answer 5

Just do a replace on your v_start_name to get rid of ";" etc. ie

v_clean_name VARCHAR;
Select v_clean_name = Replace(v_start_name,';','');

This will replace the ; with blanks foiling a SQL injection attack

For more details see String Functions in PostgresSQL

As LFSR Consulting has also commented. It is better to WhiteList (ie Do not process any input with invalid characters such a ';') rather than BlackList (ie try to clean the data as the user could do a SQL injection attack on your Replace too).

For more info have a look at SQL Injection Attacks

Answer 6

The proper way to protect against SQL Injection is via White Listing* - the long and short is set the characters that you're going to accept and filter them out.

The incorrect way is to Black List - Black listing what characters aren't accepted is going to lead to trouble, because you can't keep up with attackers. There are ways around black lists via ASCII tables, escape characters and what not.

Also, here's a nice cheat sheet to try out on your site. Run some tests and try to get things to fail.

* In the application, not the DB (Thanks James)

Answer 7

Ref. White Listing. This is OK if certain other conditions are followed. It is absolutely imperative to break all input down into it's simplest form so don't just check for SQL query names, single quotes etc. They can be represented or encoded using other character sets and it's this that forms part of the white list not specifically SQL key words themselves.

I was working for a particular client that allowed username /password to a protected resource (which ultimately could get you an access pass into secure parts of an Airport!). You could circumvent the logon field by entering ' and then building SQL queries from there to retrieve user accounts and passwords.

The problem was the client had already blown £200k building the site with a vendor that appeared to have never done web development before. The fix was a further £60k which was a validate func() that was only checking for union, select, having et al key words. When asked about what they did for canicolisation /encoding (which I had to then explain) it was tumbleweed time.

The Dev house and the (expensive) Project got canned.