419642 - WARNING: Unsafe use of LIKE detected!

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
1. start Firefox trunk debug build on MacOSX and leave it open for a while

ACTUAL RESULTS
After a couple of minutes a bunch of these warnings appear on the console:

WARNING: Unsafe use of LIKE detected!  Please ensure that you are using mozIStorageConnection::escapeStringForLIKE and that you are binding that result to the statement to prevent SQL injection attacks.: file /Users/mats/ff-trunk/mozilla/storage/src/mozStorageStatement.cpp, line 176

(I have a couple of minor patches in this tree but I doubt they caused it)

Comment 1

[Carsten Book [:Tomcat]]

confirmed on a new Linux Debug Build on Fedora 8, with the same steps from mats

WARNING: Unsafe use of LIKE detected!  Please ensure that you are
using mozIStorageConnection::escapeStringForLIKE and that you are
binding that result to the statement to prevent SQL injection
attacks.: file /opt/debug/mozilla/storage/src/mozStorageStatement.cpp,
line 176

Comment 2

[Bob Clary [:bc] (inactive)]

I've hit this over 1000 times loading 7800 pages from the top sites on linux. 

This warning is pretty damn scary. We shouldn't depend on a warning and convention to prevent SQL injections.

See also bug 395974

Comment 3

[Shawn Wilsher :sdwilsh]

bug 395974 isn't so important - but it looks like some consumer of storage is not escaping their like statements like they should be :(

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just noticed that this warning is showing up during the trace-malloc leak test on fxdebug-win32-tbox (which just starts firefox and goes through a few pages in an iframe).

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Assigning to sdwilsh so he can figure out which components aren't escaping their statements.

Shawn, if this isn't actually opening us up to SQL injections, please renom and we'll de-block.

Comment 6

[Shawn Wilsher :sdwilsh]

Anyone know offhand if there is a way to print X frames of stack with warnings too?  That'd make this real easy to find...

Comment 7

[Shawn Wilsher :sdwilsh]

A reproducible way to hit this would be nice too!

Comment 8

[Shawn Wilsher :sdwilsh]

Maybe places?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/places/src/nsNavHistory.cpp&rev=1.266#3062
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/places/src/nsNavHistory.cpp&rev=1.266#3090
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/places/src/nsNavHistory.cpp&rev=1.266#4404

a bunch of times in calendar code here:
http://mxr.mozilla.org/seamonkey/source/calendar/providers/storage/calStorageCalendar.js

Seeing as that's all I'm seeing, I'm not terribly worried.

Next comment will be looking at each of the places uses...

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/places/src/nsNavHistory.cpp&rev=1.266#3062
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/places/src/nsNavHistory.cpp&rev=1.266#3090
I don't even want to get into the fact that we are building the query up with a sprintf...
The LIKE here is safe

> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/places/src/nsNavHistory.cpp&rev=1.266#4404
This one is safe as well

the warning is just that - a warning.  There are safe usages of LIKE without binding parameters, and these fall into that case.  Renoming with new information.

Comment 10

[Reed Loden [:reed]]

Still, if the warning is going to fire that often (such as what comment #2 says), something needs to be fixed. This doesn't just need to be ignored as "another warning."

Comment 11

[Damon Sicore (:damons)]

So do we have any idea of which lines of code are causing the warnings?

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> So do we have any idea of which lines of code are causing the warnings?
See comment 9.  Someone more familiar with places would be better off explaining when those code paths are run.

Comment 13

[Damon Sicore (:damons)]

OK, blocking1.9+ P2 so we can make sure this is investigated.  Dietrich, can you take a look (just looking for anyone on the places team to help out here)?

Moving this to Firefox/Places as well.

Comment 14

[Dietrich Ayala (:dietrich)]

Ondrej, possibly from your sidebar patch?

Comment 15

[Ondrej Brablc]

(In reply to comment #14)
> Ondrej, possibly from your sidebar patch?
> 

I know that my patch generates some of those false alarms. But I was not able to avoid them unless I would refactor a lot of code I did not need to touch. In my code it is "url LIKE 'file://%'" that is triggering them.

The warning should most likely be removed. It is basically any non-sanitized textual input that could be used for sql injection, LIKE may be just statistical exception. Anyway, I will analyze this in bug 405920. 

Comment 16

[Ondrej Brablc]

This warning was implemented with bug 395974 and there is opened bug 395974 about unit tests generating the same warning in very similar case.

The easiest solution would be to replace this "url LIKE 'file://%'" with "url BETWEEN 'file://' AND 'file:/~'"

The readability is little worse, injection risks are none as well as they were before and performance is the same (the indexes are already exhausted by other conditions, otherwise BETWEEN could be faster than LIKE).

I suggest to keep the warning there, it increases awareness of the problem and use BETWEEN.

Comment 17

[Ondrej Brablc]

Attachment: Use BETWEEN instead of LIKE to avoid false alarm

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 307455 [details] [diff] [review]
Use BETWEEN instead of LIKE to avoid false alarm

r=me, thanks for following this up.

Comment 19

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.270; previous revision: 1.269
done

Comment 20

[Damon Sicore (:damons)]

Cool.  Thanks for taking this to the finish line.  :)

Comment 21

[Mats Palmgren (inactive)]

Another one: bug 423159

Comment 22

[Bob Clary [:bc] (inactive)]

several js tests reproduce this warning. See bug 423159 comment 15.

Comment 23

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv