custom rank function with FTS3/FTS4: potential crash.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

custom rank function with FTS3/FTS4: potential crash.

Kent Williams
I'll send a note to the git repo owner as well, but it's worth
mentioning to any potential writer of custom ranking function.

Background here: We implemented a custom ranking function, let's call it
XRank.  An end user complained that a query along these lines was
crashing Sqlite:

CREATE VIRTUAL TABLE t USING fts4 (a,b,tokenize=porter);
INSERT INTO T(docid,a,b) VALUES(1,"gumby", "pokey");
INSERT INTO T(docid,a,b) VALUES(2,"blockhead", "pokey");
INSERT INTO T(docid, a,b) VALUES(2,"goo", "gumby");
SELECT * FROM t WHERE docid IN (1,2) ORDER BY XRank(matchinfo(t)) DESC;

Now once I thought about it, this is an absurd query -- matchinfo
returns an empty array if there's no MATCH predicate.  But the standard
examples of custom rank functions ASSUME that there will be non-empty
match info:

static void rankfunc(sqlite3_context *pCtx, int nVal, sqlite3_value
**apVal){
   unsigned int *aMatchinfo =  (unsigned int *)sqlite3_value_blob(apVal[0]);
   nPhrase = aMatchinfo[0]; // SEGFAULT HERE, sqlite3_value_blob returns
NULL.
}

So I added a test of aMatchinfo in my custom function for NULL:
     if(aMatchInfo == 0) {
         sqlite3_result_error(cx, "matchinfo blob data is NULL", -1);
         return;
     }

This null test is missing from the "Appendix A" custom ranking function
example, as contained in this git repo:

https://git.daplie.com/coolaj86/sqlite3-fts4-rank

So a naive implementer -- me for example, before this week -- will
implement this in such a way that a query SQLite has no objection to
will crash any program with SQLite embedded.

So this suggests A) Appendix A needs to be updated B) Any code based on
this example needs to be updated.


_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: custom rank function with FTS3/FTS4: potential crash.

Jens Alfke-2
Thanks for posting this — my adaptation of that sample code in my project had the same bug.

I’m not sure how the SQL in your application gets generated, but if you allow untrusted SQL, it’s still possible to create a query that can cause the rank function to crash. For example (assuming I have my SQL blob-literal syntax correct) a call to XRank(x’77777777’). The function assumes the blob passed to it is valid output from matchinfo, where the initial 4 bytes are an array count; but if you pass a custom blob you can specify an overly large count that causes the function to read past the end of the blob … probably into unmapped address space if the count is big enough.

—Jens
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: custom rank function with FTS3/FTS4: potential crash.

Richard Hipp-3
On 10/6/17, Jens Alfke <[hidden email]> wrote:
> Thanks for posting this — my adaptation of that sample code in my project
> had the same bug.
>
> I’m not sure how the SQL in your application gets generated, but if you
> allow untrusted SQL, it’s still possible to create a query that can cause
> the rank function to crash.

We (developers) have already made a note to update the code example in
the documentation to include lots of validity checking on the
matchinfo() blob.

--
D. Richard Hipp
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: custom rank function with FTS3/FTS4: potential crash.

Kent Williams
In reply to this post by Jens Alfke-2
Luckily, any SQL we generate is done by our own developers, and runs
through QA.  Not only that, we've got a guy who likes to find exploits
for our backend software.

As for 'untrusted SQL' -- if you open your databases (or our clients'
databases) to unrestricted queries, you wouldn't need a malicious use of
full-text search to ruin everybody's day ;-)


On 10/06/2017 11:42 AM, Jens Alfke wrote:
> Thanks for posting this — my adaptation of that sample code in my project had the same bug.
>
> I’m not sure how the SQL in your application gets generated, but if you allow untrusted SQL, it’s still possible to create a query that can cause the rank function to crash. For example (assuming I have my SQL blob-literal syntax correct) a call to XRank(x’77777777’). The function assumes the blob passed to it is valid output from matchinfo, where the initial 4 bytes are an array count; but if you pass a custom blob you can specify an overly large count that causes the function to read past the end of the blob … probably into unmapped address space if the count is big enough.
>
> —Jens
> _______________________________________________
> sqlite-users mailing list
> [hidden email]
> http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: custom rank function with FTS3/FTS4: potential crash.

Richard Hipp-3
On 10/6/17, Kent Williams <[hidden email]> wrote:
>
> As for 'untrusted SQL' -- if you open your databases (or our clients'
> databases) to unrestricted queries, you wouldn't need a malicious use of
> full-text search to ruin everybody's day ;-)
>

That was my thinking too, for a long time.  I figured that any exploit
in SQLite's language was far less severe than the SQL injection
vulnerability that you create by giving users access to the language.

But some apps allow this.  Example:  The WebSQL implementation in
webkit, used in Chrome and Safari.  Earlier this year, a group of
hackers figured out how to root a Mac using a chain of 6 exploits, one
of which was a language exploit in SQLite that was accessed using
WebSQL.  Since then, I have taken a more cautious approach and assumed
that the bad guys do have unrestricted SQL access.
--
D. Richard Hipp
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: custom rank function with FTS3/FTS4: potential crash.

Dan Kennedy-4
In reply to this post by Richard Hipp-3
On 10/06/2017 11:58 PM, Richard Hipp wrote:

> On 10/6/17, Jens Alfke <[hidden email]> wrote:
>> Thanks for posting this — my adaptation of that sample code in my project
>> had the same bug.
>>
>> I’m not sure how the SQL in your application gets generated, but if you
>> allow untrusted SQL, it’s still possible to create a query that can cause
>> the rank function to crash.
> We (developers) have already made a note to update the code example in
> the documentation to include lots of validity checking on the
> matchinfo() blob.

New version on the draft website here:

   http://sqlite.org/draft/fts3.html#appendix_a

Any further bug reports or feedback welcome!

Dan.


_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: custom rank function with FTS3/FTS4: potential crash.

Dominique Pellé
Dan Kennedy <[hidden email]> wrote:

On 10/06/2017 11:58 PM, Richard Hipp wrote:

>
>> On 10/6/17, Jens Alfke <[hidden email]> wrote:
>>
>>> Thanks for posting this — my adaptation of that sample code in my project
>>> had the same bug.
>>>
>>> I’m not sure how the SQL in your application gets generated, but if you
>>> allow untrusted SQL, it’s still possible to create a query that can cause
>>> the rank function to crash.
>>>
>> We (developers) have already made a note to update the code example in
>> the documentation to include lots of validity checking on the
>> matchinfo() blob.
>>
>
> New version on the draft website here:
>
>   http://sqlite.org/draft/fts3.html#appendix_a
>
> Any further bug reports or feedback welcome!
>
> Dan.



A few corrections on this page about FTS (hopefully not too nit-picky):

1)
=== BEGIN QUOTE ===
-- ("driver" may also appear in the title, but this alone will not satisfy
the.
-- query criteria).
=== END QUOTE ===

There should not be a dot in "the."

2)
=== BEGIN QUOTE ===
putting the keyword "NEAR" between two phrase
=== END QUOTE ===

two phase -> two phrases

3)
=== BEGIN QUOTE ===
The do not influence the results
=== END QUOTE ===

The do no -> They do not...

Regards
Dominique
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: custom rank function with FTS3/FTS4: potential crash.

Dan Kennedy-4

>
> A few corrections on this page about FTS (hopefully not too nit-picky):
>
> 1)
> === BEGIN QUOTE ===
> -- ("driver" may also appear in the title, but this alone will not satisfy
> the.
> -- query criteria).
> === END QUOTE ===
>
> There should not be a dot in "the."
>
> 2)
> === BEGIN QUOTE ===
> putting the keyword "NEAR" between two phrase
> === END QUOTE ===
>
> two phase -> two phrases
>
> 3)
> === BEGIN QUOTE ===
> The do not influence the results
> === END QUOTE ===
>
> The do no -> They do not...

Thanks for these. Now fixed.

Dan.

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: custom rank function with FTS3/FTS4: potential crash.

Kent Williams
In reply to this post by Dan Kennedy-4
I like that it checks aMatchinfo for internal consistency.

The one thing that isn't in the new rankfunk.

Instead of

aMatchinfo = (unsigned int *)sqlite3_value_blob(apVal[0]);

Something like:

if((aMatchinfo = (unsigned int *)sqlite3_value_blob(apVal[0])) == 0) {
        sqlite3_result_error(pCtx,
                "invalid matchinfo blob passed to function rank()", -1);
}

On 10/06/2017 01:22 PM, Dan Kennedy wrote:

> On 10/06/2017 11:58 PM, Richard Hipp wrote:
>> On 10/6/17, Jens Alfke <[hidden email]> wrote:
>>> Thanks for posting this — my adaptation of that sample code in my
>>> project
>>> had the same bug.
>>>
>>> I’m not sure how the SQL in your application gets generated, but if you
>>> allow untrusted SQL, it’s still possible to create a query that can
>>> cause
>>> the rank function to crash.
>> We (developers) have already made a note to update the code example in
>> the documentation to include lots of validity checking on the
>> matchinfo() blob.
>
> New version on the draft website here:
>
>   http://sqlite.org/draft/fts3.html#appendix_a
>
> Any further bug reports or feedback welcome!
>
> Dan.
>
>
> _______________________________________________
> sqlite-users mailing list
> [hidden email]
> http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: custom rank function with FTS3/FTS4: potential crash.

Jens Alfke-2


> On Oct 6, 2017, at 1:32 PM, Kent Williams <[hidden email]> wrote:
>
> Instead of
>
> aMatchinfo = (unsigned int *)sqlite3_value_blob(apVal[0]);
>
> Something like:
>
> if((aMatchinfo = (unsigned int *)sqlite3_value_blob(apVal[0])) == 0) {
> sqlite3_result_error(pCtx,
> "invalid matchinfo blob passed to function rank()", -1);
> }

That’s not necessary: if aMatchinfo is NULL, then nMatchInfo will be 0, which will be detected as an error.

—Jens
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users