Is this stored procedure safe from sql injection?

Answer 1

Why do this in the stored procedure? A better solution might be to attack this on the client side, escaping strings and checking lengths before calling the stored proc. Libraries like the MS Enterprise DAAB (.NET) provide convenient ways to do this by specifying datatype and length of parameters when you add them to the command object.

Answer 2

You don't need dynamic SQL to make optional WHERE clauses... just use:

WHERE ((@x IS NULL) OR (@x = ...)) AND ...

Should be faster also, and no risk for overflowing strings, injection, or whatever.

Like this:

CREATE PROCEDURE ExecutePeopleFilter
  (
   @lastNameFilter varchar(20),
   @companyNameFilter varchar(20),
   @ageFilter int,
   @dateFilter datetime
  )
AS 
  BEGIN
    SELECT FirstName, LastName, CompanyName, Age, StartDate
      FROM People
      WHERE (
             (ISNULL(@lastNameFilter, '') = '')
             OR (LastName LIKE @lastNameFilter+'%')
            )
        AND (
             (ISNULL(@companyNameFilter, '') = '')
             OR (LastName LIKE @companyNameFilter+'%')
            )
        AND (
             (@ageFilter IS NULL)
             OR (Age = @ageFilter)
            )
        AND (
             (@dateFilter IS NULL)
             OR (StartDate = @dateFilter)
            ) ;
  END

Answer 3

Pretty much.

The key to preventing SQL injection is correct handling of parameters via an "approved" mechanism and avoiding string concatenation.

Your code does not build up a string with the parameters: they are separated and cleaned via sp_executesql.

Whether you'd do it this way is a different matter... as other answers show

Answer 4

You never concatenate anything besides well-known, hard-coded values into a SQL statement to be issued to the database engine, so it is safe from all currently known SQL injection approaches (and should be robust against future attacks). It does, however, have other problems (like @startDate not being declared).

Answer 5

Yes, your use of string concat and sp_executesql is correct and SQL injections won't be an issue.

And no, just because LINQ is the new hotness doesn't mean it is the right solution.

That said, your varchar(20) parameters can easily overflow, you may want to bump those up a bit, to at least the size of the actual field.

Answer 6

I think so. At least, it looks okay. But it's probably not how I'd go about it.

One of the key tenants of security is never never never ever write your own security code. You always want to lean as much as possible on the mechanisms provided by and built into your platform. You're doing that somewhat via your use of sp_executesql, but I expect you could do more as well.

---

By request, I'd probably do it more like this:

create procedure ExecutePeopleFilter 
    (@lastNameFilter varchar(20) = NULL, 
    @companyNameFilter varchar(20) = NULL, 
    @ageFilter int = NULL, 
    @dateFilter datetime = NULL)
as
begin

    if (LEN(@lastNameFilter) < 20) set @lastNameFilter += '%'
    if (LEN(@companyNameFilter) < 20) set @companyNameFilter += '%'

    SELECT FirstName, LastName, CompanyName, Age, StartDate 
    FROM People
    WHERE  LastName    LIKE COALESCE(@lastNameFilter, LastName) 
       AND CompanyName LIKE COALESCE(@companyNameFilter, CompanyName)
       AND Age          =   COALESCE(@ageFilter, Age)
       AND StartDate    =   COALESCE(@dateFilter, StartDate)

end

No dynamic sql needed, and no "OR"s in the WHERE clause.

Answer 7

Even if it is... "ugh".

Why not instead use a temp table, populate it with the IDs of the element results that match your non-optional parameters and then eliminate the rest from the temp table based on the parameters that have been specified? Once you've done that just join on the result set you're looking for.

CREATE TABLE #People
   (personid int)
INSERT INTO #People SELECT personid FROM people
IF NOT @lastNameParam IS NULL
   DELETE FROM #People WHERE personid NOT IN (SELECT personid FROM people WHERE lastname LIKE @lastNameParam + '%')
-- And so on...