https://bugzilla.wikimedia.org/show_bug.cgi?id=20275
--- Comment #9 from Aryeh Gregor <[email protected]> 2009-08-26 21:40:13 UTC --- (In reply to comment #8) > That would create a situation where a misleaded developer uses the new > function > in an unsafe context and gets an SQL injection. Giving it a scary name (e.g. > unsafeEscapeLike) may make it less likely, but the confusion "ZOMG I've seen > UNSAFE function being used everyewhere in the code base!!!" can't be avoided. > Will do it in the next patch if nothing better will be invented. > > There also could be made a second parameter to escapeLike() that defaults to > the current behaviour, but could instruct it not to escape quotes if > overridden. That would be pretty ugly though. > > Another idea would be to unescape quotes in the input before passing it to > addQuotes(). Although this may look insane, it will ensure injection-safety > and > will cause input corruption only where it's already incorrect - not passed > through escapeLike() and therefore with % and _ possibly unescaped. > > I'd like to see some comments from other developers on this. Yes, me too. I'm not sure what the best interface is here. > \\\\ appears to be a mysqlism, in SQLite it looks two times more sane:D Then SQLite needs to do more overriding of escapeLike(). We should check how other databases behave -- if MySQL is the odd one out, the base method should be changed. > Yes, except that your first snippet has no quotes around the pattern. Er, right. > Yes, an injection (along with syntax errors) in the first case > (SpecialListFiles.php) - fixed. In two other cases the input is already > sanitized via LinkFilter::makeLike (one execution path in mungeQuery() that > doesn't use it is sanitized by regexes that accept only IPs), I'll make it > clear by adding comments. It's best to move the escaping down as close as possible to where the query is actually built, so you can look in one place and be sure that everything's escaped. Otherwise maybe someone will add some code somewhere that will initialize the variable differently, to a non-escaped value, and it will be hard to spot. Alternatively, it's best to at least change the variable name from $foo to $encFoo so it will raise a red flag if the reader is familiar with that convention and sees that the variable is assigned an unescaped value. But this assumes the reader is familiar with the convention, and still scatters the escaping across more places. If it's a pain to change, though, don't bother, comments are okay for existing code. > Avoided it as best as I could, but two "$this->mDb->" in one line is pretty > ugly and hard to read, while introducing a new variable and using it only > selectively is a WTF on its own right. Okay. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug. You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
