SQL Injections be gone!

steve1 · Regular Joined: Dec 26, 2003 Posts: 50

Hi,
I came upon this. Please let me know if there are more threads related.

Only registered users can see links on this board!
Get registered or login to the forums!

Towards the end.... all it says is to change the name of two user tables to something unrecognizable, and change the constants.php accordingly.

All sql injection attacks be gone, i.e. no more unauthorized unions to retrieve user/admin info...

Any thoughts?

sixonetonoffun · Posted: Wed Feb 25, 2004 2:31 pm

Its only going to work if you do something like this:
error_reporting( E_ERROR | E_WARNING | E_PARSE | E_COMPILE_ERROR );
In mainfile.php as early as you can or they will find a small info disclosure hole and figure out the relevent table names easily. (or shut warnings off in .htaccess)

Code:

php_flag display_errors off

Not saying its at all a bad idea. Its just not a bullet proof solution. Good thing is of course scripts running against your site using fixed names will fail.

I think djmaze was on the right track with using better checking of variables. I was looking at Geeklog they are using kses oop version. Which is outstanding but a lot of code to go through. Not sure of the performance of it as a class opposed to a function but... they took a step in the right direction there!

steve1 · Regular Joined: Dec 26, 2003 Posts: 50

There should not be any errors from this, as the symbolic table names would be changed via constants.php, and I assume the code only uses symbolic table names.

This would be akin to someone trying to guess your password, BUT here you can make the table names very very long (unlike password). So it would be a mathematical impossibility to break into it using unions.

steve

sixonetonoffun · Posted: Wed Feb 25, 2004 3:06 pm

Not if MySQL returns an error that reveals this improbable name. Thats all I was trying to point out. Not that it would cause an error but just a hiccup in the MySQL server could reveal this improbable name.

steve1 · Regular Joined: Dec 26, 2003 Posts: 50

Good point. Right now, there is a centralized point which returns this error. So we can change it to instead of returning table name, (at least in the case of these two tables), return the symbolic nam Razz

Good add.

steve

sixonetonoffun · Posted: Wed Feb 25, 2004 3:44 pm

Yeah instead of returning the table name use internal errors much like phpbb trys to do.

Now thats creative thinking steve!
or error( mysql_error() );
This is great for debugging but...

steve1 · Regular Joined: Dec 26, 2003 Posts: 50

There is a function called like this:
message_die(GENERAL_ERROR, $msg, '', __LINE__, __FILE__, $sql);

It is just a matter of making sure there is a conditional insider the function that does not return table's name if it is auth or user table, i.e. if the words "auth" or "user" show up in the sql, mask that part of the error report.

I don't know if this is consistent between nuke and bb.

steve

Raven · Posted: Wed Feb 25, 2004 4:41 pm

Without a standard API to write to, and I am guilty of this too for helping the user, I almost always code stuff like

@mysql_query($sql) or die('Unable to query table. MySQL said '.mysql_error());

While I agree conceptually here, enforcing is impossible. You, as the webmaster, will have to modify probably most of the addons for compliance. Not everyone users the abstration layer. Maintenance and upgrades just became more ugly than it is now.

Frankly, since it doesn't appear that FB will ever get it, I suggest the following:

- Reject all future upgrades from phpnuke.org
- Take 6.9 and fork it
- Strip it down to the bare bones and separate the 'CORE'
- Prioritize the CORE rewrite addressing security and efficiency
- Once CORE is secure and working prioritize the other modules rewrite
- Release the fork as Beta to the world
- As soon as possible release RC's
- Then release version 1.0 of the secure portal (to be named later). Keep addons separate from the core. Maintain regression fucntionality as much as possible, but security and efficiency become the mantra - not useless, trivial addons.

Many details inbetween, but it may be time. Of course, the other alternative is Mambo Smile

steve1 · Regular Joined: Dec 26, 2003 Posts: 50

Raven wrote:

While I agree conceptually here, enforcing is impossible. You, as the webmaster, will have to modify probably most of the addons for compliance. Not everyone users the abstration layer. Maintenance and upgrades just became more ugly than it is now.

Raven, you are right about this. Any sql call that does not use a standard "die" function call if a security issue. There are probably a lot of these that have to be plugged. There are simply too many ways to squeeze parameters into the script using post, get, and user input vehicles.

Quote:

Then release version 1.0 of the secure portal (to be named later).

So the time is ripe for a security release, not a performance release, and not a feature release.

Are you volunteering, Raven?

steve

steve1 · Regular Joined: Dec 26, 2003 Posts: 50

On second thought, there is only one way to query mysql, mysql_query... so it would be easy to grep all the occurances of this throughout your code, and replease it with a function that traps the calls, and strips sensitive table names from error responses.

Not hard to do, IMHO. One grep, and a bit of work to include the function def.

steve

Raven · Posted: Wed Feb 25, 2004 5:42 pm

But you would have to allow for the cases where a die() is already called, or some other OR type logic.

steve1 wrote:

Are you volunteering, Raven?

I have been talking to people already. If you are familiar with the EDP process, then you would say I'm somewhere between the Ideation and Concept phase Smile

Are you volunteering for a team spot?

steve1 · Regular Joined: Dec 26, 2003 Posts: 50

sure, I will do this anyways.

As far as sql injection, I grep'ed for mysql_query, and found the following:
backup.php **
forums.php **
mysql.php
mysql4.php
sql_layer.php

I am only running scrolling forums, and amazon blocks.

It is very easy to patch those (actuall these **). This would solve the *core* problem. Every "wild" add-on would have to be checked before acceptance.

steve