201030 - "All of the words" query shouldn't use quotemeta()

Status: NEW

(Bugzilla :: Query/Bug List, defect)

Description

[Dave Miller [:justdave]]

If you create a keyword that contains a single quote and then try to search on
it, it generates an SQL error because of an unclosed quote.

This sounds dangerous, but fortunately, it's not easily exploitable, because it
does check to make sure the keyword exists before it generates the SQL to query
on it, so the admin would have had to put something dangerous as part of the
keyword for it to break anything.

Comment 1

[Dave Miller [:justdave]]

This only happens with "all of the keywords".  Any other search method works.

Comment 2

[Dave Miller [:justdave]]

Securing.  It's more widespread than keywords.  This happens any time you put a
single quote in a search term on any of the text boxes and choose "all of the
words" as the search type.

Comment 3

[Dave Miller [:justdave]]

There is an SqlQuote call in there, but it also has a quotemeta call inside
that...   where is quotemeta defined?  I can't find it in DBI or any of
Bugzilla's modules....

Comment 4

[Dave Miller [:justdave]]

ok, found quotemeta, it's a perl builtin.

       quotemeta
               Returns the value of EXPR with all non-"word" characters
backslashed.  (That is, all characters not
               matching "/[A-Za-z_0-9]/" will be preceded by a backslash in the
returned string, regardless of any
               locale settings.)  This is the internal function implementing the
"\Q" escape in double-quoted strings.


Which makes me wonder, because the ' is getting prefixed with a period, not a
backslash, in the generated SQL

Comment 5

[Bradley Baetz (:bbaetz)]

I can't repro this - can you give me a search string.

(I did discover that Search.pm needs to |use Error| at the top, but thats a
separate bug I'll deal with this evening)

Comment 6

[Dave Miller [:justdave]]

OK, I think the exploitable part of this may be specific to Sybase...

We get away with it on MySQL because MySQL uses the same escaping method that
Perl does.

It's the interaction between SqlQuote and quotemeta that's doing it.

If you enter "can'tyell", then quotemeta turns it into "can\'tyell".
SqlQuote on MySQL then turns it into "'can\\\'tyell'".  SqlQuote on Sybase turns
it into "'can\''tyell'".  doubling the quote is the "official" method of
quoting, so that's what $dbh->quote does on Sybase, but under some
circumstances, \' works, too, so the first one looks like it's part of the
string, and the second one ends the string in the middle....

BTW, the regexp got changed to a LIKE $word with $word = SqlQuote('%$word%');

Is this worth bothering to fix on the trunk?  otherwise we should close as
invalid and just call it part of the Sybase translation headache...

Comment 7

[Bradley Baetz (:bbaetz)]

Well, quotemeta isn't the right thing to use, because using it assumes that perl
regexps are the same as db regexps, which they're not.

LIKE is probably better, anyway, although you have to escape _ and % inside the
string.

Comment 8

[Dave Miller [:justdave]]

Since the "security" aspect of this only affects Sybase, and that's not actually
public anywhere yet, removing the security flag.

Comment 9

[Firefox Bug Husbandry Bot]

Removing good-first-bug keyword because team does not have bandwidth to mentor at the moment.

Comment 10

[Firefox Bug Husbandry Bot]

Removing outreachy because team does not have bandwidth to mentor this cycle.