423159 - 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 few 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-dtrace/mozilla/storage/src/mozStorageStatement.cpp, line 176


Requesting blocking-firefox3 since bug 419642 was.
BTW, if this is a condition that never should occur then I suggest
we make that warning an error instead.

Comment 1

[Mats Palmgren (inactive)]

start = {
  mStart = 0x1c186d90 "SELECT name FROM sqlite_master WHERE type = 'index' AND name = 'moz_places_visitcount' AND sql LIKE '%rev_host%'", 
  mEnd = 0x1c186e00 "", 
  mPosition = 0x1c186dee " LIKE '%rev_host%'"
}

Comment 2

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

Possibly related to bug 420164?

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> Possibly related to bug 420164?
No, don't think so

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #1)
> start = {
>   mStart = 0x1c186d90 "SELECT name FROM sqlite_master WHERE type = 'index' AND
> name = 'moz_places_visitcount' AND sql LIKE '%rev_host%'", 

That query was added by bug 402161.

Comment 5

[Marco Bonardo [:mak]]

there's no injection possibility there, still we can workaround to avoid the warning

Comment 6

[Marco Bonardo [:mak]]

to clarify, it's a fixed query that is executed on idle, to cleanup bogus data from the db. being a fixed query nobody can inject anything.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch

Comment 8

[Ondrej Brablc]

Comment on attachment 310035 [details] [diff] [review]
patch

Escaping a constant does not make much sense, we should simply bind the whole string "%rev_host%".

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 310035 [details] [diff] [review]
patch

oh well, i thought it was looking for correct use of escapeStringForLike, but from what i see in the source it's only looking for a bind (so ?, :, @)... imho that warning does not make sense at all, i could take a user value and bind it, without even having a warning...

hwv due to that, you're right and i'm going removing that

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch

Comment 11

[Ondrej Brablc]

(In reply to comment #8)
> (From update of attachment 310035 [details] [diff] [review])
> Escaping a constant does not make much sense, we should simply bind the whole
> string "%rev_host%".

Hm, stupid me, you have underscore (as wildchar) in your constant. So the previous patch was correct. I did not realize that when looking at the diff before. So this bug is not just about removing warning, it fixes the problem, that your match was too broad. It looks like the warning is on good place ;-)

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 310035 [details] [diff] [review]
patch

doh! "An underscore _ in the pattern matches any single character in the string"

well should not be a problem in both cases, we should not have many indexes called visitcount on a rev(a random char)host column. So both of the attached patches are good, perf should not be a problem, is a single query on a compact table on a long idle. let dietrich choose his preferred way

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 310246 [details] [diff] [review]
patch

I'd prefer to have the underscore properly escaped. If it's not meant to be a wildcard, don't use it as one. Others, when later modifying this code, may also overlook it, creating the potential for future danger.

(In reply to comment #9)
> oh well, i thought it was looking for correct use of escapeStringForLike, but
> from what i see in the source it's only looking for a bind (so ?, :, @)... imho
> that warning does not make sense at all, i could take a user value and bind it,
> without even having a warning...

please file a followup against storage for this. Coding around a pointless warning is a waste of time. We should instead fix the warning to reduce false positives.

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> (From update of attachment 310246 [details] [diff] [review])
> I'd prefer to have the underscore properly escaped. If it's not meant to be a
> wildcard, don't use it as one. Others, when later modifying this code, may also

so, the first attachment should be fine :)

Comment 15

[Bob Clary [:bc] (inactive)]

additional cases of WARNING: Unsafe use of LIKE detected! :

macintel 10.4 
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=e4x/Regress/regress-319872.js;language=type;text/javascript >

macintel 10.4 
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=e4x/Regress/regress-394941.js;language=type;text/javascript>

macintel 10.5,macppc 10.4 
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5/extensions/regress-336409-1.js;language=type;text/javascript>

macpcc   10.4 
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5/extensions/regress-336410-1.js;language=type;text/javascript>

macintel 10.4 macppc 10.4 
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5/GC/regress-338653.js;language=type;text/javascript>

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 310035 [details] [diff] [review]
patch

r=me

Comment 17

[Dietrich Ayala (:dietrich)]

(In reply to comment #15)
> additional cases of WARNING: Unsafe use of LIKE detected! :
> 

please file a separate bug for those, assign to the test author.

Comment 18

[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.281; previous revision: 1.280
done

Comment 19

[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