480458 - tiki-users_watches.php has sql injection vector

Status: RESOLVED FIXED

(support.mozilla.org :: Knowledge Base Software, task)

Description

[E. Cooper [:ecooper]]

Stephen found this while testing for bug 444703. Apparently, a massive security hole exists in tiki-user_watches.php. One production, it seems to have been targeted before. Fortunately, from what it seems, it was approached as an XSS vulnerability instead of an SQL Injection.

It can be readily replicated: http://s5.tinypic.com/34hb3ht.jpg

This needs patched and put in 0.9. The tiki_user_watches table also needs to be cleaned.

Comment 1

[Reed Loden [:reed]]

Once I know the exact vector, I'll open a ticket with WhiteHat to see why they didn't detect it as an SQL injection.

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screenshot

Attaching here for posterity.

Comment 3

[Laura Thomson :laura]

This shouldn't block 0.9.  We will do an off schedule push to fix.

Please include details of the attack vector in this bug if you have them.

Comment 4

[E. Cooper [:ecooper]]

Attachment: v1

I spent a fair amount of time trying to figure out why this chunk of code was here...and couldn't. I looked around to see if it was used anywhere, and it didn't seem to be. As such, I've just commented out the entire block of offending code. I also escaped things in the template, but that is nothing more than a presumably unnecessary safeguard--after tiki-user_watches.php is fixed at least.

The vector comes from a crafted url <https://support-stage.mozilla.org/tiki-user_watches.php?add=1&event=whscheck()> where 'event' contains the potentially malicious bits. The value of 'event' is then saved into the tiki_user_watches table regardless of what its value is.

Post-patch, attempting to use the vector will result in no user watches being displayed. This happens because, for whatever reason, 'event' is also used for sorting. So if you use 'event=sdljfskdf' only 'sdljfskdf' watches will be displayed.

Comment 5

[E. Cooper [:ecooper]]

Attachment: Simple SQL statement to clean table

Something as simple as that will work. Watches added to the table through this vector all will have "*" in the object and title columns.

Comment 6

[Laura Thomson :laura]

Comment on attachment 364467 [details] [diff] [review]
v1

While this stops the attack, I think we have a regression that needs fixing as well. 

Before this patch, I can't add any watches except for the language wide ones - this functionality seems to have gotten stomped by that new piece of code.  If that worked properly, then this patch would need to be different.  Can you have another hack at it?

Comment 7

[E. Cooper [:ecooper]]

(In reply to comment #6)
> (From update of attachment 364467 [details] [diff] [review])
> While this stops the attack, I think we have a regression that needs fixing as
> well. 
> 
> Before this patch, I can't add any watches except for the language wide ones -
> this functionality seems to have gotten stomped by that new piece of code.  If
> that worked properly, then this patch would need to be different.  Can you have
> another hack at it?

Your problem was unable to be reproduced (from bug 444703).

The code added to webroot/tiki-user_watches.php in the bug you've mentioned is independent of the code being removed in this bug, so I'm not sure I understand the dependency.

Comment 8

[Reed Loden [:reed]]

What's the latest on this? I really don't like having any type of SQL injection vuln hanging around for too long... Please expedite reviews / research.

Comment 9

[E. Cooper [:ecooper]]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 364467 [details] [diff] [review] [details])
> > While this stops the attack, I think we have a regression that needs fixing as
> > well. 
> > 
> > Before this patch, I can't add any watches except for the language wide ones -
> > this functionality seems to have gotten stomped by that new piece of code.  If
> > that worked properly, then this patch would need to be different.  Can you have
> > another hack at it?
> 
> Your problem was unable to be reproduced (from bug 444703).
> 
> The code added to webroot/tiki-user_watches.php in the bug you've mentioned is
> independent of the code being removed in this bug, so I'm not sure I understand
> the dependency.

Based on my comments, Laura, could you take another look at this patch?

Comment 10

[Laura Thomson :laura]

Comment on attachment 364467 [details] [diff] [review]
v1

ok, must have been something weird going on

Comment 11

[E. Cooper [:ecooper]]

r23474/r23475

We'll need the SQL run on both staging and production to clean up the bad watches.

Comment 12

[David Tenser [:djst]]

Marc, is this fix in Tiki upstream?

Comment 13

[Marc Laporte]

The code has changed quit a bit. Assigning to LPH to see if it's OK.

http://tikiwiki.svn.sourceforge.net/viewvc/tikiwiki/trunk/tiki-user_watches.php
http://tikiwiki.svn.sourceforge.net/viewvc/tikiwiki/trunk/templates/tiki-user_watches.tpl

Comment 14

[Louis-Philippe Huberdeau]

I don't see exactly where the SQL injection comes from. The patch is essentially commenting out code.

I verified all the code on trunk now related to adding watches and everything runs through the standard parameter binding. No dynamic queries are built or anything.

Comment 15

[Will Kahn-Greene [:willkg] ET needinfo? me]

These bugs are all resolved, so I'm removing the security flag from them.