[xwiki-users] [Security Advisory] SQL injection issue.
In the 2.3 timeframe, a serious security bug was fixed. Hibernate treats backslashes differently from some database management systems and as a result native SQL can be injected through the searchDocuments function. This means members of a wiki can finish an SQL query, also this means that badly written searchDocuments or search queries go from small security issues to larger ones. NOTE: Passwords are hashed (encrypted) so they cannot easily be read from the database. Who's at risk? Database systems which treat backslash as an escape character and allow stacked queries are susceptible to arbitrary SELECT, INSERT, UPDATE, DELETE, and DROP statements. These include: MS-SQL Postgres Database systems which treat backslash as an escape character but disallow stacked queries are susceptible only to arbitrary SELECT statements. These include: MySql Oracle Database systems which do not treat backslash as an escape character are not vulnerable these include: HSQLDB (default XWiki zip/exe installation) You can get a small groovy snippet to test your database and see if it supports stacked queries here: http://dev.xwiki.org/xwiki/bin/view/Drafts/SecuringXWiki#HMitigationMethods-... What can be done: #1: XWiki-2.3 and XWiki-2.2.6 Are patched to convert \ to \\ in search queries so upgrading to them will negate the threat. You can download them here: http://www.xwiki.org/xwiki/bin/Main/Download #2: If you compile your own branch of XWiki and are unable to upgrade, you can integrate the patch which was used to fix the problem The patch is here: http://dev.xwiki.org/xwiki/bin/download/Drafts/SecuringXWiki/XWIKI%2D4755%2D... Since the database controller has changed, you will likely have to port this patch to your version, what's important is that api.XWiki.searchDocuments and api.XWiki.search have their input filtered. #3: You can and should make sure to log unexpected SQL at the database level. XWiki doesn't usually use the backslash character and queries containing backslashes should be logged specially. Also it is a good idea to log (or block if possible) any SQL comment syntax. Hibernate does not support comments and comment syntax is central to almost all SQL injection. Caleb
Very simple question: Instead of manually playing cats & dogs (i.e. escaping backslashes) - why don't you just use PreparedStatements? Just a thought... Rgds Gregor -- just because you're paranoid, don't mean they're not after you... gpgp-fp: 79A84FA526807026795E4209D3B3FE028B3170B2 gpgp-key available @ http://pgpkeys.pca.dfn.de:11371 @ http://pgp.mit.edu:11371/ skype:rc46fi
We do and users should, but there is a function which allows script authors to construct queries for document names so they are allowed to finish an HQL query. If the script author is malicious or if they don't properly use prepared statements then SQL can be injected into the HQL. see XWiki.searchDocuments http://maven.xwiki.org/site/xwiki-core-parent/xwiki-core/apidocs/com/xpn/xwi... I hope this clears up exactly what the issue is. Caleb Gregor Schneider wrote:
Very simple question:
Instead of manually playing cats & dogs (i.e. escaping backslashes) - why don't you just use PreparedStatements?
Just a thought...
Rgds
Gregor
On Apr 30, 2010, at 3:56 PM, Caleb James DeLisle wrote:
We do and users should, but there is a function which allows script authors to construct queries for document names so they are allowed to finish an HQL query. If the script author is malicious or if they don't properly use prepared statements then SQL can be injected into the HQL. see XWiki.searchDocuments http://maven.xwiki.org/site/xwiki-core-parent/xwiki-core/apidocs/com/xpn/xwi...
Actually Gregor might be right and we could decide to deprecate this method and recommend to use one which would take a varargs list of parameters, wdyt? Thanks -Vincent
I hope this clears up exactly what the issue is.
Caleb
Gregor Schneider wrote:
Very simple question:
Instead of manually playing cats & dogs (i.e. escaping backslashes) - why don't you just use PreparedStatements?
Just a thought...
Rgds
Gregor
Hi my request for today: a separate mailinglist for security-advisories so one can subscribe to only those. Thank you in advance Harald Kapper / kapper.net
Vincent Massol wrote:
On Apr 30, 2010, at 3:56 PM, Caleb James DeLisle wrote:
We do and users should, but there is a function which allows script authors to construct queries for document names so they are allowed to finish an HQL query. If the script author is malicious or if they don't properly use prepared statements then SQL can be injected into the HQL. see XWiki.searchDocuments http://maven.xwiki.org/site/xwiki-core-parent/xwiki-core/apidocs/com/xpn/xwi...
Actually Gregor might be right and we could decide to deprecate this method and recommend to use one which would take a varargs list of parameters, wdyt?
When there are no user supplied parameters the "bad" method is fine eg: $searchDocuments("where doc.space='Main'") Also a script author can still make a mistake with the "good" method and not parametrize enough eg: $searchDocuments("where doc.space='" + $userInput + "' and doc.name=?", [$moreUserInput]) Of course the advisory is considering the possibility of a malicious script author who could exploit either method. A good long term answer would be to make the query be written in type safe java where the code always knows what needs to be parametrized. This is an interesting project: http://source.mysema.com/static/querydsl/latest/reference/html/ch02s04.html Caleb
Thanks -Vincent
I hope this clears up exactly what the issue is.
Caleb
Gregor Schneider wrote:
Very simple question:
Instead of manually playing cats & dogs (i.e. escaping backslashes) - why don't you just use PreparedStatements?
Just a thought...
Rgds
Gregor
users mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/users
On 05/03/2010 12:07 PM, Vincent Massol wrote:
On Apr 30, 2010, at 3:56 PM, Caleb James DeLisle wrote:
We do and users should, but there is a function which allows script authors to construct queries for document names so they are allowed to finish an HQL query. If the script author is malicious or if they don't properly use prepared statements then SQL can be injected into the HQL. see XWiki.searchDocuments http://maven.xwiki.org/site/xwiki-core-parent/xwiki-core/apidocs/com/xpn/xwi...
Actually Gregor might be right and we could decide to deprecate this method and recommend to use one which would take a varargs list of parameters, wdyt?
That still won't fix the problem, since the query can still hold non-parameterized code. So, something like this would work: searchDocuments(" where doc.name like ? and doc.space = 'Main'", ['X%']) This will only encourage users (devs) to use parameterized queries, but will still leave the security problem wide open.
I hope this clears up exactly what the issue is.
Caleb
Gregor Schneider wrote:
Very simple question:
Instead of manually playing cats& dogs (i.e. escaping backslashes) - why don't you just use PreparedStatements?
Just a thought...
Rgds
Gregor
-- Sergiu Dumitriu http://purl.org/net/sergiu/
On May 3, 2010, at 12:34 PM, Sergiu Dumitriu wrote:
On 05/03/2010 12:07 PM, Vincent Massol wrote:
On Apr 30, 2010, at 3:56 PM, Caleb James DeLisle wrote:
We do and users should, but there is a function which allows script authors to construct queries for document names so they are allowed to finish an HQL query. If the script author is malicious or if they don't properly use prepared statements then SQL can be injected into the HQL. see XWiki.searchDocuments http://maven.xwiki.org/site/xwiki-core-parent/xwiki-core/apidocs/com/xpn/xwi...
Actually Gregor might be right and we could decide to deprecate this method and recommend to use one which would take a varargs list of parameters, wdyt?
That still won't fix the problem, since the query can still hold non-parameterized code. So, something like this would work:
searchDocuments(" where doc.name like ? and doc.space = 'Main'", ['X%'])
This will only encourage users (devs) to use parameterized queries, but will still leave the security problem wide open.
Yes but then we could also escape the quote characters... ;) We would have a backward compat issue to handle though but that's doable with some security setting for ex. -Vincent
I hope this clears up exactly what the issue is.
Caleb
Gregor Schneider wrote:
Very simple question:
Instead of manually playing cats& dogs (i.e. escaping backslashes) - why don't you just use PreparedStatements?
Just a thought...
Rgds
Gregor
Having read some of the comments here, this is my 0.02€: I feel that a generic method like seacrhDocuments is way too dangerous, because, as stated in the comments, malicious SQL can be inserted easily. Furthermore, if you try to escape certain characters, you might run into problems when such a character is part of any XWiki-object. Therefore, I'd rather have some specialized methods handy, internally based on PreparedStatements then such a generic problematic method. I'm aware that there are backward-compatibility-issue, also I'm aware that such a concept soesn't come as handy as a generic method, but better be safe than sorry.... Cheers Gregor -- just because you're paranoid, don't mean they're not after you... gpgp-fp: 79A84FA526807026795E4209D3B3FE028B3170B2 gpgp-key available @ http://pgpkeys.pca.dfn.de:11371 @ http://pgp.mit.edu:11371/ skype:rc46fi
participants (5)
-
Caleb James DeLisle -
Gregor Schneider -
Harald Kapper -
Sergiu Dumitriu -
Vincent Massol