393082 - Warn on unsafe usage of LIKE

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Shawn Wilsher :sdwilsh]

As per the meeting today, warn about unsafe usage of LIKE.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This should be sufficient.

Comment 2

[(not reading, please use [e-mail] instead)]

shawn, thanks for taking a look at this.

questions:

1)  Shouldn't you do a case insensitve compare on like?
2)  what if LIKE appears more than once in your statement, and the first time is ok, but the second is not?
3)  what if I do "select * where name = slikep"

I am not sure we really want to do this.  do we worry about this with any other sql statements?  I think you could shoot yourself in the foot appending any user input (and not using bind parameters).

I'm thinking this might be wontfix.

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> 1)  Shouldn't you do a case insensitve compare on like?
Probably, but I couldn't seem to find the case insensitive comparator.

> 2)  what if LIKE appears more than once in your statement, and the first time
> is ok, but the second is not?
I'd really hope that if they do it once the smart way, they keep doing it that way.  This is just a heuristic so it may miss it sometimes...

> 3)  what if I do "select * where name = slikep"
adding a space before and after the LIKE in the initial check should take care of that.

> I am not sure we really want to do this.  do we worry about this with any other
> sql statements?  I think you could shoot yourself in the foot appending any
> user input (and not using bind parameters).
No, but it's also considered common practice to use the binding functions instead of doing it yourself.  This is a bit different because it is our own function.

Comment 4

[(not reading, please use [e-mail] instead)]

if we decide we still want to do this, perhaps we should also ensure that our statement contains "escape", as the safe way to use LIKE is:

... x LIKE ?1 ESCAPE 'y' ...

Comment 5

[(not reading, please use [e-mail] instead)]

Comment on attachment 277586 [details] [diff] [review]
v1.0

clearing review request per review comments

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> if we decide we still want to do this, perhaps we should also ensure that our
> statement contains "escape", as the safe way to use LIKE is:
> 
> ... x LIKE ?1 ESCAPE 'y' ...
ESCAPE isn't always necessary, as they could try to bind 1 to |foo| which wouldn't need to be escaped.

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Address review comments.

I understand your concern about this seeming like a bit much, but I figure it is worth it so we (mozilla) don't shoot ourselves in the foot.

Comment 8

[(not reading, please use [e-mail] instead)]

Comment on attachment 278514 [details] [diff] [review]
v1.1

r=sspitzer

note, there are still ways around this test (for example, two spaces after LIKE), but let's not spend any more time on this.

you're more active in mozilla/storage than me, so if you want this in there, r=sspitzer.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> you're more active in mozilla/storage than me, so if you want this in there,
> r=sspitzer.
The way I see it, it's trivial enough to do, and it can at least catch it most of the time.

Checking in storage/src/mozStorageStatement.cpp;
new revision: 1.26; previous revision: 1.25

Comment 10

[(not reading, please use [e-mail] instead)]

hey shawn, it looks like our tests in the storage test suite trigger this warning.

WARNING: Unsafe use of LIKE detected!  Please ensure that you are usi
ng mozIStorageConnection::escapeStringForLIKE and that you are binding that resu
lt to the statement to prevent SQL injection attacks.: file c:/builds/trunk/mozi
lla/storage/src/mozStorageStatement.cpp, line 175

for example:

var stmt = createStatement("SELECT x FROM t1 WHERE x LIKE 'abc';");

Comment 11

[Shawn Wilsher :sdwilsh]

Yeah, I noticed that when I had some failed tests too ;)

We could properly bind the parameters, but at the same time, we don't need to here, so I'm rather indifferent.

Comment 12

[(not reading, please use [e-mail] instead)]

given that we have this this warning on usage usuage change, and that people tend to copy and paste code (even from tests), I think we should fix it.

but it is not a high priority.  I'll start a new bug on it.

Comment 13

[(not reading, please use [e-mail] instead)]

> I'll start a new bug on it.

see bug #395974