Security proposal - dealing with SQL injection exploits

Discuss features as they are added to the new version. Give us your feedback. Don't post bug reports, feature requests, support questions or suggestions here.
Forum rules
Discuss features as they are added to the new version. Give us your feedback. Don't post bug reports, feature requests, support questions or suggestions here. Feature requests are closed.
Post Reply
blobber
Registered User
Posts: 96
Joined: Wed Mar 16, 2005 6:28 pm

Security proposal - dealing with SQL injection exploits

Post by blobber »

I have been advised to post my original inquiry here:
blobber wrote: Hi !

This is not really a feature request or bug report, it's rather an idea that we had some time ago because our phpBB-Forum was "hacked" by some sick soul who thought it would be cool to exploit some of the -back then- well documented security weaknesses of phpBB in order to put some harm to our forum.

So, said person used an exploit to inject malicious SQL code into phpBB - which resulted in the loss of a couple of hundreds of postings - cause the last backup was already 3 weeks old.

Meanwhile, we have tried to permanently update our board software, however we realize that something like that could happen anytime again, hence we do regular daily backups, too - on the other hand, most of us being familiar with php, too - we thought about potential ways to make such an attack at least (a bit) more complicated for the average script kiddie.

So, the following is what we have come up with, so far - and we'd really like to get some feedback and opinions from you guys about all this, and how feasible you think it would be to implement the following functionality within phpBB:

The major idea is to use a central database access object that's inherited from any of PHP's standard db access objects, however we'd like to extend the baseclass in a manner to allow SQL statement verfication and -validation.

Basically, this would mean that any function/method in any script that sends SQL queries to the database, would have some hardcoded way to determine what SQL statements are valid for the corresponding function and method.

So, despite from the usual query string, there would be also an associative array containing SQL statements/commands that are valid for a function/method.

The actual query method would then not directly execute the query string by sending it to the db, but rather do some simple "pre-parsing", or checking - by comparing the actual query string with the conditions that are additionally provided by each function.

While most of the parsing could become relatively complex with some functions, some more advanced usage of regex checking should be suitable to determine whether a SQL statement matches the conditions or not.

Basically, this would mean that each SQL query would additionally provide its own "context" to the parent SQL object, which is first checked - so that the SQL query is only run if the conditions are met.

So, we would end up having a simple syntax checker for most purposes that could also check the type of arguments and possibly even contain a "blacklist" of non-allowed statements.

The latter might come in handy by default for those scripts or functions that need only read-access and don't need write access, for example: to display the member list, there won't be any need to make use of SQL statements such as "UPDATE, INSERT, DROP" etc.

An attacker would hence not only have to find a security hole within the source code itself, but would also need to find such a security hole within certain modules of the source code that do have the necessary "priviledges" to actually pass such "active" (writing/modifying) SQL statements to the database.

Despite from the general layout as described above, one could also additionally provide custom regex statements that apply only to certain functions/methods, this would be mainly useful to really check complex SQL statements that might rely on a variable subset of SQL statements, depending on the current execution mode - so, one could even do some simple "structure checking" in order to validate a SQL statement, despite from the actual "vocabulary" of the query string itself.

So, even though this could theoretically also be overridden, we'd think that it might make some sorts of attacks a lot harder, however we are not too familiar with phpBB's internals and would hence love to get some feedback of those people who are really involved in the development process.

Please feel free to make any comments or ask for further clarification - if necessary.

P.S.: Sorry if this is a double posting, I didn seem to succeed posting at first ... either there's some activation thing required or I am doing something else wrong ...

SamG
Registered User
Posts: 1241
Joined: Fri Aug 31, 2001 6:35 pm

Re: Security proposal - dealing with SQL injection exploits

Post by SamG »

A couple of notes for the community:

1) Given the type of discussion that followed an earlier security-related proposal, I'd like to see our involvement (as a community) limited to any technical issues within the proposal we'd like to politely discuss.

2) All decisions about how phpBB works are made by the development team, so we as a community need not address the proposal's merits beyond discussing any technical points we find interesting, as per #1 above.

A note for blobber:

Please note that the developers are currently pressed for time, so please don't take a lack of quick response on their part as a sign of a lack of interest. I can't guarantee they will respond, but please don't assume that that means a lack of interest in your idea. I've actually thought through an idea something like this for an entirely different project, so I don't know if great minds think alike or if we're just both on the same rabbit trail :) . Thanks for taking the time to post.
"I hate trolls!" - Willow Ufgood

blobber
Registered User
Posts: 96
Joined: Wed Mar 16, 2005 6:28 pm

Re: Security proposal - dealing with SQL injection exploits

Post by blobber »

SamG wrote: A couple of notes for the community:

1) Given the type of discussion that followed an earlier security-related proposal, I'd like to see our involvement (as a community) limited to any technical issues within the proposal we'd like to politely discuss.

2) All decisions about how phpBB works are made by the development team, so we as a community need not address the proposal's merits beyond discussing any technical points we find interesting, as per #1 above.
I just took some time to actually catch up with some of the earlier discussions, and I did also stumble across said thread about the "code review proposal":

I'd like to make clear that my posting wasn't intended to have any critical background, and taking said posting into account, I do realize that my posting or even wording might appear kind of annoying to you -
I'm sorry for that.

I did merely mention our own negative experiences in order to illustrate why we were giving some thoughts to the whole idea and how the whole idea was born.

So, even though I personally have no doubts that such a "code review" would probably be quite a useful thing, I didn't mean to imply anything similar and I am aware of the fact that such a thing would be quite an undertaking, and being a programmer myself I understand that it's much more fun to actually do coding than spending your time with reviewing.

Hence, I was mainly trying to get some feedback from you folks about the idea, to assess how feasible the "concept" is so far. And to see what kind of suggestions those people "in the know" might come up with.
So, I was at no time talking of any reviewing or even rewriting of phpBB code, currently I am merely envisioning a modification of the DBAL in order to abstract the mentioned verification/validation procedures accordingly.
Theoretically, it should be possible to start modifying the DBAL without breaking any dependencies in the other files.

So, this whole thing -if relevant for this project- could be merely optional in the beginning, and one could start to slowly port scripts/functions to actually make use of these features, which would mainly come down to additionally providing limitations for each SQL query that's to be executed, which could be done by adding a couple of methods to the DBAL.

SamG wrote: A note for blobber:

Please note that the developers are currently pressed for time, so please don't take a lack of quick response on their part as a sign of a lack of interest.
No problem, likewise I might not check in with the board for some time, too.
So the same applies for me, I do understand that all this is an opensource project and I don't have any unrealistic expectations, however I'd love to see some ideas from you guys - depending on the feedback that I receive here, I might very well decide to make a first stab at it.
SamG wrote: I can't guarantee they will respond, but please don't assume that that means a lack of interest in your idea.
don't worry: you folks won't put any harm to my self-esteem ;-)
SamG wrote: I've actually thought through an idea something like this for an entirely different project, so I don't know if great minds think alike or if we're just both on the same rabbit trail :) . Thanks for taking the time to post.
lol, well - actually, I've meanwhile used the same basic approach already for another project, too -although, admittedly on a much smaller scale. phpBB would certainly be quite a challenge.

I mean, I do realize that the developers seem to be rather busy and that it's quite unlikely to see some of the more involved features to be implemented anytime soon, but personally I wouldn't even mind to discuss concept and design ideas for a couple of months, I think such a thing requires some time to mature - either at a later time, or simply in the beginning of the design phase.

R. U. Serious
Registered User
Posts: 32
Joined: Mon Feb 11, 2002 2:07 pm
Contact:

Re: Security proposal - dealing with SQL injection exploits

Post by R. U. Serious »

blobber wrote: however we'd like to extend the baseclass in a manner to allow SQL statement verfication and -validation.
To do this, you are basically reinventing a secon language that needs to have almost the same flexibility as SQL. You are adding another layer if complexity (which always brings with it more possibility for bugs - even security realted bugs), with IMHO little benefit. SQL Injections are actually a topic that is well known/researched,and there are effective ways of checking userdata before passing it to queries.
Basically, this would mean that any function/method in any script that sends SQL queries to the database, would have some hardcoded way to determine what SQL statements are valid for the corresponding function and method.

This doesn't handle several classes of injection related attacks, like e.g. escalating user privileges by manipulating where clauses.
If you are worried about dropping/truncating tables, the mysql-user should not have those rights in the first place, since it is not needed for operation of phpBB anyway.
While most of the parsing could become relatively complex with some functions, some more advanced usage of regex checking should be suitable to determine whether a SQL statement matches the conditions or not.
Is this easier than just checking the user-submitted data and escaping it where appropiate?


I am not saying that there could not be cases where your approach would prevent an injection attack - you provide valid examples. However overall I don't think the added complexity and effort for developing and maintance for such a solution (as well as the Potential for bugs that go along with it) is worth it. That, of course, is just my humble opinion. ;)
Das Kölsch zum Handy.

blobber
Registered User
Posts: 96
Joined: Wed Mar 16, 2005 6:28 pm

Re: Security proposal - dealing with SQL injection exploits

Post by blobber »

R. U. Serious wrote:
blobber wrote: however we'd like to extend the baseclass in a manner to allow SQL statement verfication and -validation.
To do this, you are basically reinventing a secon language that needs to have almost the same flexibility as SQL.
You are of course right about that, at least if we are talking of a really flexible and fully dynamic approach, however -personally- I'm kind of pragmatic and try to remain realistic, so I was thinking of a semi-dynamic approach:

If you take a look at some of the source files, you'll notice that most SQL queries follow a certain scheme, basically most queries seem to be static - often, it's only a few variables that actually change a queries' behaviour, so one could simply take that very sql query string and use it as sort of a "reference" to see whether the final query string (containing resolved variables) actually matches the format that's suggested by the original query string (with unresolved variables).

So, let's take the following query (taken from phpBB2/viewtopic.php) as an example:

Code: Select all

$sql = "SELECT vd.vote_id, vd.vote_text, vd.vote_start, vd.vote_length, vr.vote_option_id, vr.vote_option_text, vr.vote_result
		FROM " . VOTE_DESC_TABLE . " vd, " . VOTE_RESULTS_TABLE . " vr
		WHERE vd.topic_id = $topic_id
			AND vr.vote_id = vd.vote_id
		ORDER BY vr.vote_option_id ASC";
We'd see directly that there's only one variable that influences the result: $topic_id.
Taking this into account, we can derive that any query that's based on this one, should be structurally identical with the original query, the only difference being the resolved variable "$topic_id".

Despite from the possibility to put in some simple typechecking for the variable, like:
  • $topic_id must be numerical
  • $topic_id must not exceed a certain length and value (determined base on db)
One could then easily check whether the original query's structure is identical to the one that phpBB wants to execute.

So, basically the same query would at first serve as some sort of "template", in order to check the validity of the dynamically determined resulting query.
This approach - while still being relatively basic and static, would at least create some additional safety potential for phpBB queries.

This would then be handled on a function/method-specific level, so one would additionally have to provide at least one more method to actually pass an array that contains the requirements for the query, or alternatively overload the $db->sql_query() method, in order to handle the corresponding calls directly.

R. U. Serious wrote: You are adding another layer if complexity (which always brings with it more possibility for bugs - even security realted bugs), with IMHO little benefit. SQL Injections are actually a topic that is well known/researched,and there are effective ways of checking userdata before passing it to queries.
Basically, this would mean that any function/method in any script that sends SQL queries to the database, would have some hardcoded way to determine what SQL statements are valid for the corresponding function and method.
This doesn't handle several classes of injection related attacks, like e.g. escalating user privileges by manipulating where clauses.
You are again right, however you don't really need to implement the level of complexity that you are thinking of, rather you would simply provide runtime validation of sql queries by using a sort of "template" query and comparing this then with the actual query to determine whether the query is legal.
R. U. Serious wrote: If you are worried about dropping/truncating tables, the mysql-user should not have those rights in the first place, since it is not needed for operation of phpBB anyway.
Again right, I was only providing examples - but if I remember correctly, there have been cases in the past where phpBB exploits made use of SQL statements/queries that the actual files themselves, didn't even make any use of.

So, using an approach like the one that I described above, would at least reduce the possibility to manipulate SQL queries in a manner to run something absolutely different from what the file actually does, because each query would first be checked against some sort of template.

Additionally, it would be relatively straight forward to provide file-global priviledges for each phpBB source file, so that each file includes some header that defines an array of (in)valid SQL-statements, as well as -structures.

For example:

Code: Select all

//specifies what databases this file needs access to
$sql_restriction['viewtopic.php']['databases']	=	array('phpbb');

//specifies the accessing user for db
$sql_restriction['viewtopic.php']['users']	=	array('phpbb-user');

//specifies what tables this file needs access to
$sql_restriction['viewtopic.php']['tables]']	=	array('phpbb_forum','phpbb_config');

//specifies which SQL statements should be blocked
$sql_restriction['viewtopic.php']['sql']['block']=	array('UPDATE','INSERT');

//optionally allow only specific queries/formats
$sql_restriction['viewtopic.php']['sql'][exclusive]=	array('SELECT [fields] FROM [tables] WHERE [condition]');
...
...
...

Of course, as soon as one really wants to support fully dynamic SQL statement parsing, things would become really complex and one would really end up inventing some sort of entirely new syntax, however - as long as SQL statements remain mostly simplistic, they could be typechecked without having to do much abstract parsing.

Also, one could theoretically still provide custom eregi() conditions for each (in)valid SQL statement, so it would not really be necessary to really intelligently deal with those statements at runtime, but rather provide a static framework for each query that can be tailored while programming.

So, any calls to the $db->sql_query() method could additionally also check for the presence of a $sql_restriction array and take its contents into account.


R. U. Serious wrote:
While most of the parsing could become relatively complex with some functions, some more advanced usage of regex checking should be suitable to determine whether a SQL statement matches the conditions or not.
Is this easier than just checking the user-submitted data and escaping it where appropiate?
I'm not saying anything would be "easier", I was just trying to show that some former exploits wouldn't have worked if there was some sort of hard-coded simmple "security model" for all queries BEFORE actually running them.

R. U. Serious wrote: I am not saying that there could not be cases where your approach would prevent an injection attack - you provide valid examples. However overall I don't think the added complexity and effort for developing and maintance for such a solution (as well as the Potential for bugs that go along with it) is worth it. That, of course, is just my humble opinion. ;)
I think your point is a valid one, and actually I share it - but as you can see above, I wasn't talking of such a complicated approach, I was really trying to keep things simple: why fight with the necessary logics to really parse all those SQL statements when all queries can be reduced to a simple template-like format, so that you could simply check whether the generated query obeys the format of the template and whether the query variables have valid types ?

Naturally, it would be pretty cool to really have some sort of more powerful mechanism to parse the SQL statements - e.g. in order to dynamically restrict a SQL query (in an abitrary -legal- format) to certain tables, fields and legal statements for each table/field - however, without establishing some standard format for all of phpBB's SQL queries, this would be an entirely new project - and as soon as any query wouldn't follow a pre-defined syntax, the logics wouldn't be able to differentiate between legal and illegal queries.

R. U. Serious
Registered User
Posts: 32
Joined: Mon Feb 11, 2002 2:07 pm
Contact:

Re: Security proposal - dealing with SQL injection exploits

Post by R. U. Serious »

Hi blobber,
blobber wrote: If you take a look at some of the source files, you'll notice that most SQL queries follow a certain scheme, basically most queries seem to be static - often, it's only a few variables that actually change a queries' behaviour, [...]
So basically you're reinventing (sort of) prepared statements, which - granted - is not available in every DBMS that Olympus will support.
So, let's take the following query (taken from phpBB2/viewtopic.php) as an example:
Code:$sql = "SELECT vd.vote_id, vd.vote_text, vd.vote_start, vd.vote_length, vr.vote_option_id, vr.vote_option_text, vr.vote_result
FROM " . VOTE_DESC_TABLE . " vd, " . VOTE_RESULTS_TABLE . " vr
WHERE vd.topic_id = $topic_id
AND vr.vote_id = vd.vote_id
ORDER BY vr.vote_option_id ASC";

[...]

One could then easily check whether the original query's structure is identical to the one that phpBB wants to execute.

So, basically the same query would at first serve as some sort of "template", in order to check the validity of the dynamically determined resulting query.
This approach - while still being relatively basic and static, would at least create some additional safety potential for phpBB queries.
And when you are comparing the two queries, how exactly are you going to do that? Most injections don't remove anything from the query, they add ("insert") stuff like
' AND 1=1#
.
Everything else from the original query will still be there. So in effect you are only checking the passed variables for certain keywords/characters etc. - which is basically as effective as (or equivalent to) just checking and escaping the user-provided data.
you would simply provide runtime validation of sql queries by using a sort of "template" query and comparing this then with the actual query to determine whether the query is legal.
[...]
So, using an approach like the one that I described above, would at least reduce the possibility to manipulate SQL queries in a manner to run something absolutely different from what the file actually does, because each query would first be checked against some sort of template.
So what if you would simply checked every query before executing wether it had more than one the keywords (SELECT; INSERT; DELETE;) - if it did we don't execute it. This would replicate a lot of the "benefits" of your appraoch, and could be transparently added to the current code of the DB-classes in phpBB2.x without affecting any of the "clients" that use the class - because there would be no difference in the classes method signature.
(I am putting benefits in quotations marks, because, as I wrote above for your appraoch, it doesn't catch a whole class of injections.)

Is this easier than just checking the user-submitted data and escaping it where appropiate?
I'm not saying anything would be "easier", I was just trying to show that some former exploits wouldn't have worked if there was some sort of hard-coded simmple "security model" for all queries BEFORE actually running them.
Yes, and my suggestion is that

a) the whole code-bureacracy you're creating will likely introduce new bugs and create limits that might negatively affect performance as well as maintenance at a later date.

b) the approach basically relies (again) on checking the user-procided data. But instead of doing it straightforward, it checks it after inserting it into another string.


Just by ontroducing additional layers the problem does not go away - you still have to check the userdata, you're just obscuring the way you are doing it.

Naturally, it would be pretty cool to really have some sort of more powerful mechanism to parse the SQL statements - e.g. in order to dynamically restrict a SQL query (in an abitrary -legal- format)
To be honest, this reminds of the philosophy of all those template-languages that are being used in some PHP-applications. They start out, severly limiting what you can do, by primising to make it "easier" to use the newly invented language and that it will "look better". Than they find out, that the language is too limited, so that you have to work around it in your code all the time. They go back and add more and more to the language until it is powerful and flexible enough that in effect it is just as powerful and thus has the same [supposed] negative side effects that were first attributed to PHP and were the reason for inventing the new lanugage in the first place. This of course is not suprising, since PHP itself started out as being a template language...

I guess some people are layer-happy and some are not. I beling to the latter camp. ;)
Das Kölsch zum Handy.

vanderaj
Registered User
Posts: 29
Joined: Sat Oct 25, 2003 6:57 am
Location: Melbourne, Australia
Contact:

Re: Security proposal - dealing with SQL injection exploits

Post by vanderaj »

There are several approaches to this. I don't think phpBB is ready for a Hibernate approach, but what you're suggesting something akin to its functionality. Hibernate is a bit slower than normal SQL, and best suits CRUD transactional data, akin to e-commerce or Internet Banking sites. Code like forums need a lot of indexes, and referential integrity, which Hibernate struggles with.

Now, this doesn't mean that the code shouldn't be using prepared statements where it's available. It's possible to test if you're using MySQL 4.1 or later. If so, you should try using the MySQLi interface, which supports prepared statements, just as Postgres has for a while.

Prepared statements fail cached SQL execution plans when injected, which is the reason they're safer (but not 100% safe). However, this still requires application rule checks. For example, if someone injects -1 into a field expecting 1..n where n is the set maximum, then the code needs to fail gracefully or prevent it being injected.

That's why its a good idea to move away from dynamic queries in PHP code and move to only allowing model code to query the database. In the viewthread case, a facade class would need to exist to mitigate the slowdown caused by the model abstraction.

However, the abstraction itself is very cache friendly, so any slowdown caused by moving to a model / facade and passing back record set results that way would be offset by a greater level of security, across all database types.

The cool thing about using models is that business rules can be implemented once, done well, and can be validated using SimpleUnit.

Andrew

R. U. Serious
Registered User
Posts: 32
Joined: Mon Feb 11, 2002 2:07 pm
Contact:

Re: Security proposal - dealing with SQL injection exploits

Post by R. U. Serious »

vanderaj wrote: I don't think phpBB is ready for a Hibernate approach
The key feauture of hibernate is that it allows easy Persistence of Objects, it is successful because Persistence in Relational Databases from OO can be a real pain. phpBB is not "ready" for the Hibernate Approach, because it is not and does not want to be a purely OO-Application.
Code like forums need a lot of indexes, and referential integrity, which Hibernate struggles with.
I agree very much with this statement. Hence the question is what are the benefits, and if we weigh benefits and costs - is it worth it? For corporate users the trade-off may be valued differently than for a lot of private forums that grow to respectable sizes, yet have not ready money to throw at more hardware.
Prepared statements fail cached SQL execution plans when injected, which is the reason they're safer
Yes, prepared statements as provided by a DBMS. However a self-baked PHP solution is not. AFAIK some of databases, and most prominently the widely used mysql pre4.1 versions, do not have prepared statements. So I think it's unlikely that it will be made use of. In previous discussion on the search system, it was argued that the search-system as provided by the DBMS might be a better fit, than the self-baked one - IIRC the Team explained that they wished to have a solution that works across all supported DBMSs, even though the most widely deployed basis of mysql-installations did provide that feature. (Please someone correct me, if I am mistaken on this).
would be offset by a greater level of security, across all database types
only those that have support for prepared staatements, or did I miss something?


Don't get me wrong: It is likely that our sites will move to mysql4.1 in the following months, so I'd certainly welcome prepared statements (mainly for performance reasons, e.g. wrt the search table inserts). But a) I don't think it's a security benefit, if the database does not support it and b) I suppose it is unlikely that non-common DBMS features, that would require self-baked solutions for the other supported databases, will be used.
Das Kölsch zum Handy.

Post Reply