Comment On SQL Injection Prevention 101

Seems that his whole life is an "Oh Duh" moment.

Senior Consultant?!  The fanciest resume in the world apparently can replace ability and experience. Go figure.

Well, duh!  He is obviously still missing a lot:

CREATE PROCEDURE [SADynamicTPSBuilder]
(
  @SELECTS VARCHAR(8000),
  @JOINS VARCHAR(8000)
  @WHERES VARCHAR(8000)
  @ORDERS VARCHAR(8000)
) AS
BEGIN

  SET NOCOUNT ON
  SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED

  EXEC (@SELECTS + @JOINS + @WHERES + @ORDERS)
  
  RETURN(-1)

END

I mean seriously, what's the point if I can't specify my where and order by clause.



"I sense the sarcasm."

"Good because I'm laying it on pretty thick."

I get it.  -1 is a magic number and constants should be used.  Geez, what an idiot.

Uh, why would a mere full-time employee review a consultant's code?  That makes no sense.  All consultants are super-geniuses.  That's why they charge such high hourly-rates.  That's the way we do it where we work.  For example, we paid contractors two million dollars to write a system.  They just stopped working on it after 2 years.  Here's the best part.  It doesn't work at all.  This saves the company a lot of money in the long-run because there's no maintainance cost.

I think they rejected it because he didn't use sp_executesql.

Why do I think the next iteration will involve xp_readmail...

dubwai:

Uh, why would a mere full-time
employee review a consultant's code?  That makes no
sense.  All consultants are super-geniuses.  That's why they
charge such high hourly-rates.  That's the way we do it where we
work.  For example, we paid contractors two million dollars to
write a system.  They just stopped working on it after 2
years.  Here's the best part.  It doesn't work at all.  This saves the company a lot of money in the long-run because there's no maintainance cost.

Man, that makes me want to make a consulting company.  Think of
all the seemingly impossible promises you can make to your clients:



"I guarantee you that you will not need to invest valuable time maintaining the software we produce for you" (your example)



"After we're done producing software for you, you'll never need us for anything ever again!"



"The software we'll produce will not put any kind of strain whatsoever on your servers or network."



"Our software is guaranteed to work as intended."

Ytram:

dubwai:

Uh, why would a mere full-time employee review a consultant's code?  That makes no sense.  All consultants are super-geniuses.  That's why they charge such high hourly-rates.  That's the way we do it where we work.  For example, we paid contractors two million dollars to write a system.  They just stopped working on it after 2 years.  Here's the best part.  It doesn't work at all.  This saves the company a lot of money in the long-run because there's no maintainance cost.

Man, that makes me want to make a consulting company.  Think of all the seemingly impossible promises you can make to your clients:

Well, they had already produced code that sort of worked or worked with a babysitter and a lot of prodding.  But they finally got some kids right out of college and built the ultimate in software.  As you say, it uses no resources and costs nothing to maintain.  you can understand why we are expanding the use of contractors.  We want to see if other companies can produce this type of high-caliber software.

Man do I love SQL WTFs !!!

Perhaps the next attempt will involve

STEP 1: Inserting the SQL statement into some SQLLibrary table

STEP 2: Present a stored proc that retrieves the SQL statement from SQLLibrary by a passed ID and executes the retrieved SQL string

bye bye SQL Injection .. really ...

 

CREATE PROCEDURE [DynamicTPSBuilder]
(
  @SQLID INT
) AS
BEGIN

  SET NOCOUNT ON
  SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED

  DECLARE @SQL VARCHAR(8000)
  --Get SQL
  SELECT @SQL = SQLString FROM SQLLibrary WHERE SQLID = @SQLID
  --Exec SQL
  EXEC (@SQL)

  RETURN(-1)

END

Alex Papadimoulis:

CREATE PROCEDURE [SADynamicTPSBuilder]
(
  @SELECTS VARCHAR(8000),
  @JOINS VARCHAR(8000)
) AS
BEGIN

  SET NOCOUNT ON
  SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED

  EXEC (@SELECTS + @JOINS)
  
  RETURN(-1)

END

The only thing he did was allow a 16k character sql statement instead
of 8k.  He/she must be one of those dot com throwbacks who got
into it for the money but shouldn't even be out of the house without
their hickey helmet...let alone programming.  I thought they all
went back to their hr/marketing/landscaping/etc. jobs by now.

One point to consider though with this is that I've been to Security
presentations where they said if you used parameterized stored
procedures you would protect yourself from SQL Injection attacks. That
is obviously not the case here. So make sure you take necessary steps
like a regex against the input strings to check for Sql Injection
anyways.

Its for TPS Reports, no-one is gonna call that from outside so its ok right :P

 

 

titltn21:

Man do I love SQL WTFs !!!

Perhaps the next attempt will involve

STEP 1: Inserting the SQL statement into some SQLLibrary table

STEP 2: Present a stored proc that retrieves the SQL statement from SQLLibrary by a passed ID and executes the retrieved SQL string

bye bye SQL Injection .. really ...

 

CREATE PROCEDURE [DynamicTPSBuilder]
(
  @SQLID INT
) AS
BEGIN

  SET NOCOUNT ON
  SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED

  DECLARE @SQL VARCHAR(8000)
  --Get SQL
  SELECT @SQL = SQLString FROM SQLLibrary WHERE SQLID = @SQLID
  --Exec SQL
  EXEC (@SQL)

  RETURN(-1)

END

I worked for a company where that was pretty standard practice. Of course it wasn't for web ASP applications, but rather for SQL batch jobs. And no Return(-1) at the end.

Anonymous:

One point to consider though with this is that I've been to Security
presentations where they said if you used parameterized stored
procedures you would protect yourself from SQL Injection attacks. That
is obviously not the case here. So make sure you take necessary steps
like a regex against the input strings to check for Sql Injection
anyways.

Any time you execute dynamically-built SQL, you have to look out for a SQL injection possibility.

This applies to client-to-server calls where SQL is passed to a SQL engine... as the parameterized stored procedure sentence above indicates...

But it ALSO applies to dynamically built SQL within a stored procedure.  Any time you use EXEC or sp_executesql or any of their cousins, you have to stop and consider the possibility of injection.

Usually all you need is a simple Replace(., '''', '''''') to fix things.  And usually you only need that for character-type parameters.

But watch out for things like EXECUTE SearchSomething... @OrderBy = 'ID DESC'

Mung Kee:

The only thing he did was allow a 16k character sql statement instead
of 8k.  He/she must be one of those dot com throwbacks who got
into it for the money but shouldn't even be out of the house without
their hickey helmet...let alone programming.  I thought they all
went back to their hr/marketing/landscaping/etc. jobs by now.

sign me up for hickey helmet please.

Hang on....  This is really freaky.  A couple of weeks ago, we discovered a veritable dearth of READ UNCOMMITED statements in our stored procedure (that's worth a chortle all on its own), and yesterday I was discussing SQL injection with a colleague of mine, and how it would affect our planned buctracking/knowledge base system if we allowed free text searches.

 

Spooky, or what?

Oh, I know what the WTF is. They should have called it sp_SADynamicTPSBuilder, of course.

But, seriously...

That EXEC(sql string) function might possibly be the worst idea ever thought of in the entire history of MS SQL Server. If you really really need your SQL to be that dynamic (and I would contend there's absolutely no reason you should) then at least build your strings in the ASP or VB code.

 

Mung Kee:

The only thing he did was allow a 16k character sql statement instead
of 8k.

Does that really work?  (Tries it...) wow, it works...

declare @a varchar(8000)
declare @b varchar(8000)

declare @prefix varchar(500)
declare @postfix varchar(500)

select @prefix = 'select datalength('''
select @postfix = ''')'

select @a = @prefix + replicate('a', 8000 - len(@prefix))
select @b = replicate('b', 8000 - len(@postfix)) + @postfix

exec (@a + @b)
-- result: 15979

Str8Dog:

Mung Kee:

The only thing he did was allow a 16k character sql statement instead
of 8k.  He/she must be one of those dot com throwbacks who got
into it for the money but shouldn't even be out of the house without
their hickey helmet...let alone programming.  I thought they all
went back to their hr/marketing/landscaping/etc. jobs by now.

sign me up for hickey helmet please.

doh!  I forgot to put mine on this AM.

El Duderino:

Seems that his whole life is an "Oh Duh" moment.

Probably his parents', too.

Home Security Guy has a meetiing with this SQL Consultant:



HSG: You know, if you leave your door open like that someone will steal your car.

SQLC: .......

HSG: Ummm... someone can get in the door, and steal the car.

SQLC: ..... oh, I get it.



(5 minutes later)



SQLCL: Right, I went back to my office and closed the doo.... huh, where's my car gone?

He didn't get the memo about the TPS reports...  [:D]

Wtf?   Code reviews?   Worried about injection
attacks?  What universe does Louis work in?  I want to
switch.  Don't they know that code review is something to pay lip
service to, but is too hard to actually do?  The few times I've
seen it, I've had to force
it down their throats. 



Here, fresh out of school, and I wrote this code, will you review it
and tell me if I'm doing ok?   "Looks like code written in
our language and it seems like it will compile - commit it."
   Come to think of it, those guys quit a few months latter,
and I ended up maintaining their code - it would have been sent here
if this site existed back then.

Allowing only stored procedure access to a database is a huge WTF and should only be favored by psycho DBAs.  It's like the cops fighting speeders by making everyone take the bus.  Then only the bus drivers can speed.  Just try and implement a sensible caching or object relational mapping strategy when you are in this scenario...there are lots of ways to prevent SQL injection attacks.  Allowing only stored procedures is not one on its own, as this case proves.  Who is to say that the consultant wasn't filtering for SQL injection attacks on the code that invoked the stored procedure?

 

Anonymous:

Allowing only stored procedure access to a database is a huge WTF and should only be favored by psycho DBAs.  It's like the cops fighting speeders by making everyone take the bus.  Then only the bus drivers can speed.

Yeah, I mean, it's just like have a controlled production environment.  WTF, why shouldn't anybody be able to go and do whatever they please.  Encapsulation, that's stupid too.

Anonymous:

Just try and implement a sensible caching or object relational mapping strategy when you are in this scenario...

Huh?  Why would that be a problem?  Stored procedures are just as relational as any other type of SQL.

Anonymous:

there are lots of ways to prevent SQL injection attacks.  Allowing only stored procedures is not one on its own, as this case proves. 

Again, huh?  The WTF is that the person was executing a statement outside of a stored procedure.  If the developer only called a stored procedure, he wouldn't be able to create one.  How do you create a SQL injection attack if all you do is call stored procedures?

Anonymous:

Who is to say that the consultant wasn't filtering for SQL injection attacks on the code that invoked the stored procedure?

That the consultant wasn't aware of them is a big clue.  And why reinvent the wheel?

A stored procdure is a great way to create clean interfaces between code and the DB.  It's a lot easier to refactor code to call the same stored procedure in a new DB than to refactor all the SQL in your code.  In Java, for example, it's just a matter of changing drivers to switch DB vendors if you only use stored procedures.  It also insulates the code from table structure changes.

johnl:

...and how it would affect our planned buctracking/knowledge base system if we allowed free text searches.

Oh man. One typo away from butcracking.

Obviously the WTF is that he included no exception handling in the stored procedure.
This code needs to be added to the end:

EXCEPTION
  WHEN others 
  THEN
    NULL;

[8-)]

"Buy a car from us and you'll never go anywhere else again!"

This is not a rhetorical question. 



How could anyone possible think that this would fix sql injection problems?

Sean Connery:

johnl:

...and how it would affect our planned buctracking/knowledge base system if we allowed free text searches.

Oh man. One typo away from butcracking.

Sean Connery: "I'll take Anal Bum Cover for 7000."

Trebek: "That's An Album Cover."

The only thing he did was allow a 16k character sql statement instead of 8k.

Actually, not even that. If the database in question has an 8000 character limit on the varchars, as is quite likely, then concatenating two big strings that add up to over 8000 characters will truncate. So this is *even dumber* than it first appears.

dubwai:

For example, we paid contractors two million dollars to write a system.  They just stopped working on it after 2 years.  Here's the best part.  It doesn't work at all.  This saves the company a lot of money in the long-run because there's no maintainance cost.

 

I've seen that scenario in the double-digit millions a couple times now. 

Maybe I shouldn't feel so guilty for my consulting rates...

dubwai:

Anonymous:

Allowing only stored procedure access to a database is a
huge WTF and should only be favored by psycho DBAs.  It's
like the cops fighting speeders by making everyone take the bus. 
Then only the bus drivers can speed.

Yeah, I mean, it's just like have a controlled production
environment.  WTF, why shouldn't anybody be able to go and do
whatever they please.  Encapsulation, that's stupid too.

Anonymous:

Just try and implement a sensible caching or object relational mapping strategy when you are in this scenario...

Huh?  Why would that be a problem?  Stored procedures are just as relational as any other type of SQL.

Anonymous:

there are lots of ways to prevent SQL injection attacks. 
Allowing only stored procedures is not one on its own, as this case
proves. 

Again, huh?  The WTF is that the person was executing a
statement outside of a stored procedure.  If the developer only
called a stored procedure, he wouldn't be able to create one.  How
do you create a SQL injection attack if all you do is call stored
procedures?

Anonymous:

Who is to say that the consultant wasn't filtering for SQL injection attacks on the code that invoked the stored procedure?

That the consultant wasn't aware of them is a big clue.  And why reinvent the wheel?

A stored procdure is a great way to create clean interfaces between
code and the DB.  It's a lot easier to refactor code to
call the same stored procedure in a new DB than to refactor
all the SQL in your code.  In Java, for example, it's just a
matter of changing drivers to switch DB vendors if you only use stored
procedures.  It also insulates the code from table structure
changes.

Portability is a good point but I've never been able to change a schema
and NOT change the SPs. Plus the poster is just saying that a lot of
ORM systems have a hard time with SPs. I agree.



Here:



http://www.hibernate.org/hib_docs/v3/reference/en/html_single/#sp_query

Anonymous:

The only thing he did was allow a 16k character sql statement instead of 8k.

Actually, not even that. If the database in
question has an 8000 character limit on the varchars, as is quite
likely, then concatenating two big strings that add up to over 8000
characters will truncate. So this is *even dumber* than it first
appears.

Not in T-SQL, no. Even though there is a definite limit to the size of
CHARs/VARCHARs, you can concatenate them and it won't truncate.

For instance (for Sybase, the limit is 255),

DECLARE @p VARCHAR(255), @q VARCHAR(255)

SELECT @p = REPLICATE('A',255), @q = REPLICATE('B',255)

SELECT SUBSTRING(@p+@q, 254, 4)

would return "AABB".

Ok, I'll admit to writing a sproc similar to this, but there is a
perfectly good reason for this.  My former employer, who will go
unnamed (but basically Germany's largest bank),  put a procedure
in place where all the procs had to be reviewed and approved by at
least 2 different groups (one of which didn't have a foggiest idea of
what SQL was and the other one merely knew how to put together a basic
SELECT) before being commited to the database.  This had to be
accompanied by a multitude online forms, all of which had expiration
dates.  If one of the groups didn't approve it in time, you had to
resubmit.  So having gone through a couple of these sproc
submissions, I had enough and simply wrote a proc like the one
above.  As I expected, the sproc got approved and never had to
submit another sproc again.



Chances of SQL Injection attack on my apps?  All of them were
internal, the likelyhood is small.  And if it happens, that's just
the cost of putting stupid policies in place.  Would serve them
right.

WTF

A Wizard A True Star:

Oh, I know what the WTF is. They should have called it sp_SADynamicTPSBuilder, of course.

But, seriously...

That EXEC(sql string) function might possibly be the worst idea ever thought of in the entire history of MS SQL Server. If you really really need your SQL to be that dynamic (and I would contend there's absolutely no reason you should) then at least build your strings in the ASP or VB code.

 

We do use EXEC in one of our stored procedures. We use it to flip a table so that rows become columns and columns become rows. This is not possible in SQL 2000 in another way as far as I (and I'm not a databaser, actually) and my boss know...

Ofcourse if there is another way, now would be the rihgt time to mention it [;)]

Drak

One nasty side effect of ExecuteSql is that it runs with the Public permission set.

This means all database objects (tables, other sprocs, user-defined functions, jobs, extended stored procedures, etc.) that the dynamic SQL refers to must have the permissions opened up to Public. The same fools that allow direct table access, are typically too lazy, or too uninformed, to do other than grant ALL to Public.

It's fairly trivial to then take over the server by using xp_CmdShell [1], or a number of other methods.

The WTF here is that there isn't a massive amount of these attacks being published, given how many systems are vulnerable to it.

[1] For those of you unfamiliar with this gem, it can be invoked by anybody with perms to invoke it, yet when it runs, it can execute any operating-system command that the account running Sql Server has the privileges to execute. The typical account used for the SQL Server process is the "System" account. (READ: God Account).

Probably just a typo. It should be senile consultant, not senior consultant.

I bet the params are passed to application via HTTP GET query params and WWW is public accessed. Didn't?



Seriously, this is one of my favourite WTFs. It hurts my eyes and my mind hard.

Eric:

He didn't get the memo about the TPS reports...  [:D]

Can you forward him a copy of the memo about the new TPS report cover sheets? [;)]

After seeing all the SQL-related WTFs here, would any sane DBA (I'll
leave proof of existence as an exercise) allow senior conslutants to
write and execute ad-hoc queries from the code?

> It's fairly trivial to then take over the server by using
> xp_CmdShell [1], or a number of other methods.
> The WTF here is that there isn't a massive amount of these
> attacks being published, given how many systems are
> vulnerable to it.

Possibly those who are r00ting such machines are not doing so for the 'zombie bot' aspect but rather for industrial espionage purposes. In which case, victims might be somewhat reluctant to come forward - after all, if your sex toy customer database has just been pwned, you're probably not going to be over-eager to let everyone know. Just a thought.

Otherwise, yeah, hacking into a box with SQL Server on it is not terribly difficult by all accounts. To be honest, most of the web app installations I've come across using SQL Server use 'sa' as the account to connect as, for fuck's sake.

Simon

dubwai:

A stored procdure is a great way to create
clean interfaces between code and the DB.  It's a lot easier to
refactor code to call the same stored procedure in a new DB
than to refactor all the SQL in your code.  In Java, for example,
it's just a matter of changing drivers to switch DB vendors if you only
use stored procedures.  It also insulates the code from table
structure changes.

It depends, actually. In a lot of my systems, it's much easier to go
through the code and refactor embedded SQL than it is to change stored
procedures. This is mostly because it's easier to write and change Ruby
code than it is to write and change SQL code.

richleick:

Well, duh!  He is obviously still missing a lot:

CREATE PROCEDURE [SADynamicTPSBuilder]
(
  @SELECTS VARCHAR(8000),
  @JOINS VARCHAR(8000)
  @WHERES VARCHAR(8000)
  @ORDERS VARCHAR(8000)
) AS
BEGIN

  SET NOCOUNT ON
  SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED

  EXEC (@SELECTS + @JOINS + @WHERES + @ORDERS)
  
  RETURN(-1)

END

I mean seriously, what's the point if I can't specify my where and order by clause.



"I sense the sarcasm."

"Good because I'm laying it on pretty thick."

How about this?

CREATE PROCEDURE [SADynamicTPSBuilder]
(
  @SELECTS VARCHAR(8000),
  @JOINS VARCHAR(8000),
  @OTHER_NON_INJECTION_CODE_REALLY VARCHAR(8000),
) AS
BEGIN

  SET NOCOUNT ON
  SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED

  EXEC (@SELECTS + @JOINS + @OTHER_NON_INJECTION_CODE_REALLY)
  
  RETURN(-1)

END

    dZ.

Oh noooo!  The Brillant Paula Bean (tm) got a job as an DBA consultant... Gawd help us all.



    dZ.

Anonymous:

johnl:

...and how it would affect our planned buctracking/knowledge base system if we allowed free text searches.

Oh man. One typo away from butcracking.

Ewww.  Take your filthy programming fetishes outta here!



    dZ.

But then you lose the performance benefit stored procedures have (cached query plans etc)

evnafets:

Obviously the WTF is that he included no exception handling in the stored procedure.
This code needs to be added to the end:

EXCEPTION
  WHEN others 
  THEN
    NULL;

[8-)]

Hmm, which RDBMS is that for then?

Anonymous:

Ok, I'll admit to writing a sproc similar to this, but there is a
perfectly good reason for this.  My former employer, who will go
unnamed (but basically Germany's largest bank),  put a procedure
in place where all the procs had to be reviewed and approved by at
least 2 different groups (one of which didn't have a foggiest idea of
what SQL was and the other one merely knew how to put together a basic
SELECT) before being commited to the database.  This had to be
accompanied by a multitude online forms, all of which had expiration
dates.  If one of the groups didn't approve it in time, you had to
resubmit.  So having gone through a couple of these sproc
submissions, I had enough and simply wrote a proc like the one
above.  As I expected, the sproc got approved and never had to
submit another sproc again.





Chances of SQL Injection attack on my apps?  All of them were
internal, the likelyhood is small.  And if it happens, that's just
the cost of putting stupid policies in place.  Would serve them
right.

I worked on a project for a very large uk company performing support of
their main processing systems. Due to one nonce doing an update to a
table, but somehow losing the 'where' clause, we were all subsequently
made to have an 'sql buddy' watching over us whenever we needed to fire
sql against the tables. I don't think it mattered whether they knew sql
or not. The managers told us it was to 'protect us'....