Closed Bug 393082 Opened 15 years ago Closed 15 years ago

Warn on unsafe usage of LIKE

Categories

(Toolkit :: Storage, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 1 obsolete file)

As per the meeting today, warn about unsafe usage of LIKE.
Flags: blocking1.9?
Attached patch v1.0 (obsolete) — Splinter Review
This should be sufficient.
Attachment #277586 - Flags: review?(sspitzer)
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.
(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.
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 on attachment 277586 [details] [diff] [review]
v1.0

clearing review request per review comments
Attachment #277586 - Flags: review?(sspitzer)
(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.
Attached patch v1.1Splinter Review
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.
Attachment #277586 - Attachment is obsolete: true
Attachment #278514 - Flags: review?(sspitzer)
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.
Attachment #278514 - Flags: review?(sspitzer) → review+
(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

Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: blocking1.9?
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';");
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.
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.
> I'll start a new bug on it.

see bug #395974
You need to log in before you can comment on or make changes to this bug.