https://bugzilla.wikimedia.org/show_bug.cgi?id=20275
--- Comment #8 from Max Semenik <[email protected]> 2009-08-26 18:11:29 UTC --- (In reply to comment #7) Thanks, Aryeh. I'll make another patch in a couple of days. > Okay, it seems like escapeLike() does strencode() as well. So you've got > like() doing no escaping at all. I think this is potentially quite > surprising; > I'd prefer to see it doing addQuotes() on the input. Generally speaking, APIs > that escape more than expected are better than those that escape less than > expected -- the former leads to double-escaping, the latter leads to SQL > injection (or XSS, etc.). escapeLike() clearly can't be used in that case, of > course, some new and confusingly similar method would have to be made that > only > does the replacement of _ and % and \ and nothing else. 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. > When reviewing your patch, I got seriously confused about escaping. I had to > rewrite the whole thing twice as I figured out what your method did and what > escapeLike() did and how LIKE works (I didn't realize \\\\ was necessary for a > single slash in LIKE). \\\\ appears to be a mysqlism, in SQLite it looks two times more sane:D > As far as I understand it, the correct way to do things > currently is 'LIKE ' . $db->escapeLike( $foo ) . '%', while with your patch > the > correct way would be $db->like( $db->escapeLike( $foo ) . '%' ), and I'd like > to see $db->like( $db->someNewName( $foo ) . '%' ). If I'm wrong, then most > of > my review comments here are nonsense. :D Yes, except that your first snippet has no quotes around the pattern. > Don't these add double-escaping of some kind? (Or was $likeprefix not already > escaped, and this would previously have been SQL injection if it weren't from > a > maintenance script?) On further review, it's a MySQL-specific code for upgrade between versions that had not supported anything else, so I'll just revert it back. > So these were double-escaping before? Yes, it seems. This script is pretty obscure though, and non-encoded quotes are non-standard in URLs. > Syntax error? A dot is missing. Thanks, it will be corrected. > These all remove addQuotes()/strencode(). Are you adding SQL injection here? > Even if theoretically you don't think the variables should contain characters > that need to be escaped, it's better to call addQuotes()/strencode() to be on > the safe side, and the closer to the point of use, the better. 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. > Try to avoid making unrelated changes like this, it makes patches harder to > review. 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. > It's better if you make sure to use the normal spacing here, with an extra > space before the * starting each line of the comment after the first, so the > *'s on the left all line up. > I would add an extra space before and after the return value (also for the > SQLite modified version). It can't hurt, and it might avoid SQL syntax errors > from stuff like "tablename" . $db->like( $foo ). Will do. Notepad++ made the misaligned *'s unnoticeable. -- 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
