2

Just started a new job, and I have found an insanely complex call to sp_executesql (SQL Server 2008 R2). I can't decide whether it is vulnerable to SQL injection or not.

The following is defined as a single C# string (so no additional escaping):

declare @temp table(keyid int IDENTITY(1,1) PRIMARY KEY, name nvarchar(50));
insert into @temp (name) select distinct name from SOMEWHERE where code=@code;

declare @names nvarchar(max); 
declare @dyn nvarchar(max); 

-- dynamically build the columns
select @names = Stuff ((select '],['+name from @temp order by name for XML PATH('') ),1,2,'')+']'; 

-- dynamically build the pivot query
set @dyn = N'select '+ @names +' from (select name, score from TABLE where code='''+@code+''') as p PIVOT
    (max(score) for name in ('+ @names +')) as pivtab';

execute sp_executesql @dyn

And then the whole lot is executed as so

exec sp_executesql @query, N'@code nvarchar(500)',@code=N'..something..'

So @code is both used properly (in the insert statement) but also used badly in the construction of @dyn (and @names too, one of those name fields could contain malicious script).

It seems like a bad code smell, but I can't seem to write a value for @code that will execute arbitrary SQL. And I lack the authority to just force the issue.

Does anyone know if this type of thing is safe or not? Thanks

Lukasz Szozda
  • 162,964
  • 23
  • 234
  • 275
Hyjrt6534
  • 21
  • 1

1 Answers1

2

This looks like classic dynamic PIVOT.

declare @temp table(keyid int IDENTITY(1,1) PRIMARY KEY, name nvarchar(50));

insert into @temp (name) 
select distinct name from SOMEWHERE where code=@code;

declare @names nvarchar(max) 
       ,@dyn nvarchar(max); 

-- dynamically build the columns
select @names = Stuff ((select ','+ QUOTENAME(name) 
                        from @temp 
                        order by name 
                        for XML PATH('') ),1,1,''); 

-- dynamically build the pivot query
set @dyn = 
   N'select '+ @names +' from (select name, score from TABLE where code=@code) 
    as p PIVOT
    (max(score) for name in ('+ @names +')) as pivtab';

execute dbo.sp_executesql 
        @dyn,
        N'@code DATATYPE',
        @code;

I would:

  1. Use QUOTENAME instead of concatenating ],[ (just as good practice)
  2. Parametrize @code (possible vector of attack, user can pass malicious code)

If you can do PIVOT in application layer do it.

I recommed also to read The Curse and Blessings of Dynamic SQL

Lukasz Szozda
  • 162,964
  • 23
  • 234
  • 275
  • Thanks, that's very useful - I didn't think of specifying @code as a parameter of the internal query. – Hyjrt6534 Nov 21 '15 at 15:57
  • @jane0027 Doing it you are sure code like `blabla'; DROP TABLE ... ;--` is parametrized. – Lukasz Szozda Nov 21 '15 at 15:58
  • +1 . . . It is worth pointing out that with the changes suggested, the code should not be vulnerable because the names are coming from system tables and properly quoted. The original code would be vulnerable to an attach where `@code started with a single quote. – Gordon Linoff Nov 21 '15 at 16:04
  • @GordonLinoff I don't like concatenating `[` because there are edge cases like [demo](https://data.stackexchange.com/stackoverflow/query/396350) If you try concat version you will end up with invalid identifier. – Lukasz Szozda Nov 21 '15 at 16:22
  • Thanks for the useful advice. I will push for moving the pivot to C# (the dynamic query has a vast number of columns.. and they grow with time). Any useful resources for debugging dynamic SQL? All I could get out of SQLMS and Profiler was the initial call, but not the inner constructed calls. – Hyjrt6534 Nov 21 '15 at 16:29
  • @lad2025 . . . Perhaps you misunderstood my comment. I only meant that you should explicitly answer the question, as well as providing the additional, very useful, information. – Gordon Linoff Nov 21 '15 at 22:15