Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

SQL Injection in Ajax.php #4427

Closed
mihailowitsch opened this issue Jun 7, 2012 · 24 comments
Closed

SQL Injection in Ajax.php #4427

mihailowitsch opened this issue Jun 7, 2012 · 24 comments
Labels
Milestone

Comments

@mihailowitsch
Copy link

Hi guys,

the file 'contao-2.11.3\system\modules\backend\Ajax.php' contains the following two lines of code:

Removed for security reasons

Both use direct user input to prepare and later execute an SQL Statement.

Hence the original SQL Statement can be modified by an malicious user to change any database entries. The following very simple requests as an sample makes the user with id "3" admin:

Removed for security reasons

Though the code can only be reached once authenticated to the backend, this can be used to evelvate privileges, if the user is allowed to execute this lines of code.

Cheers,
Fabian Mihailowitsch

@leo-unglaub
Copy link

@mihailowitsch are you sure your snippet is working. I tryed this and i can't get it to work.
(intval($this->Input->post('state') == 1) ? 1 : '') is always killing my set value. But you are right about the fact, that simply gluing query's together like that is heighly ugly and dangerous.

@aschempp
Copy link
Member

aschempp commented Jun 8, 2012

@LeoUnglaub I suppose the risk is not in "state" but in "field" ?

@leo-unglaub
Copy link

The risk is in both values. The combination makes it dangerous. But
currently i don't see a quick solution for that. :(

@backbone87
Copy link
Contributor

In my backend_tabletree rewrite, i write out the widget config to html and post it pack via ajax (to reload subtrees). i verify correctness, by adding the enc key to the options array, then enc hash the serialization of the config array, after that i remove the enc key again and add the hash. on post back i do the same thing again, and check if both hashes are the same.

@aschempp
Copy link
Member

aschempp commented Jun 8, 2012

Please see my commit, it's very easy to sanitize the field name.

@mihailowitsch
Copy link
Author

Hi Leo, Andreas,

you guys are quick! :)

As Andreas said, yes I am talking about the value in "field" ($this->Input->post('field')) not "state", since this value is used directly to prepare the SQL query before it is executed. "state" only allows to be set to 1 or ''. I have successfully exploited this SQL Injection in a test installation of the latest contao release in my lab to make users to admins, by setting the field "admin" to "1" in the table "tl_user". This probably shouldn't happen! :) So yes, it works.

@andreas: The fix you commited should work yes. Now "field" can't be used anymore to inject malicious SQL code. However it is still possible to set any value in the table "tl_form" to 1 or '', since i can provide any existing field within "field" which then is filled with 1 or ''. This however should in the worst case just result in a DoS.

Cheers,

Fabian Mihailowitsch

@aschempp
Copy link
Member

aschempp commented Jun 8, 2012

Just improved the fix to not require a localconfig value to be predefined. It also fixes the issue mentioned by @mihailowitsch

@mihailowitsch
Copy link
Author

Yes, should work that way! :)

@andreas
Copy link

andreas commented Jun 8, 2012

@mihailowitsch, please use mentions properly -- when you use @andreas I get lots of emails. Cheers :)

@mihailowitsch
Copy link
Author

I guess we can close this security issue!

Thanks guys!

@aschempp
Copy link
Member

aschempp commented Jun 8, 2012

@mihailowitsch please reopen the ticket! @leofeyer has to implement this, my code is just a suggestion.

@mihailowitsch mihailowitsch reopened this Jun 8, 2012
@aschempp
Copy link
Member

aschempp commented Jun 8, 2012

While testing this, we also noticed there is no permission check. This allows a user to toggle field he should not be able to access.

@leofeyer
Copy link
Member

I just check and the example above only works if the user has access to the user module (which makes him a de facto admin anyway). I could not manage to edit the field as "h.lewis". Did you?

@aschempp
Copy link
Member

Unfortunately that is not true, it also works in the login module (just set table=tl_user).

@leofeyer
Copy link
Member

Tried it from the site structure, but ended up with

<br><strong>Fatal error</strong>: Uncaught exception <strong>Exception</strong> with message <strong>Query error: Unknown column 'admin' in 'field list' (UPDATE tl_page SET admin='1' WHERE id=NULL)</strong> thrown in <strong>/Users/leofeyer/Sites/contao/core/system/libraries/Database.php</strong> on line <strong>686</strong>
<pre style="margin:11px 0 0">
#0 /Users/leofeyer/Sites/contao/core/system/libraries/Database.php(633): Database_Statement->query()
#1 /Users/leofeyer/Sites/contao/core/system/modules/backend/Ajax.php(324): Database_Statement->execute(NULL)
#2 /Users/leofeyer/Sites/contao/core/system/modules/backend/Backend.php(232): Ajax->executePostActions(Object(DC_Table))
#3 /Users/leofeyer/Sites/contao/core/contao/main.php(120): Backend->getBackendModule('page')
#4 /Users/leofeyer/Sites/contao/core/contao/main.php(230): Main->run()
#5 {main}
</pre>

@aschempp
Copy link
Member

Not sure if you want us to tell you a detailed step how to reproduce it in the ticket system? ;-)
I can assure you it worked...

@leofeyer
Copy link
Member

I have added Andreas' patch in 2bf4fc3.

Now: Is this a serious issue which requires an immediate release of version 2.11.4? I'm not sure, since the requirements for the exploit are quite specific (only works if you are a logged in back end user with access to the user module).

What do you think?

@leofeyer
Copy link
Member

Unfortunately that is not true, it also works in the login module (just set table=tl_user).

You are right :( At least we know what to do then.

@aschempp
Copy link
Member

I think it is more urgent than the previous two security fixes, but as you say it only works for backend users (but even if they have no user module available). I would not thread it as immediate release, but also not wait a few weeks...

@leofeyer
Copy link
Member

I have just released version 2.11.4.

@mihailowitsch
Copy link
Author

@leofeyer: Cool thanks! :)

However could you please give credit in the Changelog and release...

Thanks,

Fabian Mihailowitsch

@aschempp
Copy link
Member

@mihailowitsch do you mean crediting me or crediting you? A bug report is not really something to credit, we would need to credit 4427 people (by now)...

@leofeyer
Copy link
Member

Added in 83c1ad9.

@psi-4ward
Copy link
Contributor

@aschempp and me wanna have credits too - for fixing the bug!
... just joking 【ツ】

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

7 participants