Closed Bug 1270882 Opened 3 years ago Closed 8 months ago

Be aware of and control SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER for system SQLite purposes

Categories

(Toolkit :: Storage, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: asuth, Assigned: axs)

References

Details

Attachments

(1 file, 3 obsolete files)

We should expose a means of enabling/disabling SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER on connections and disable it by default.

## Context / Background ##

In bug 1049920 comment 6 the issue of Thunderbird and its need for either SQLITE_ENABLE_FTS3_TOKENIZER to be defined or sqlite3_db_config(conn, SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER, &memoryLocationOfAOne) to be called was raised.  Although Gecko isn't so concerned with Thunderbird's needs these days, this is relevant because linux distros like to ship system SQLite and that means that all consumers of the system SQLite become relevant through the defines they impose on system SQLite.

This matters because enabling the tokenizer by default is arguably bad from a security/crashiness perspective.  This becomes relevant when attackers are able to impact the SQL run by SQLite.  You've sorta already lost at that point, but it's a question of mitigating damage.

## What to do ##

Because Firefox can run on a distro where system SQLite is used and SQLITE_ENABLE_FTS3_TOKENIZER may be defined and we don't want the fts3_tokenizer command exposed, we want our connections to invoke sqlite3_db_config(conn, SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER, &memoryLocationOfAZero) by default.

I propose we do this by adding the following signature on mozIStorageConnection:
  [noscript] enableFts3Tokenizer(in boolean enable);

Storage calls EnableFts3Tokenizer(false) in Connection::initializerInternal where we do all our pragma stuff.

Thunderbird calls EnableFts3Tokenizer(true), registers its tokenizer, then calls EnableFts3Tokenizer(false) again in its registration logic at https://dxr.mozilla.org/comm-central/source/mailnews/extensions/fts3/src/nsFts3Tokenizer.cpp#33

I'm proposing the very specific method signature versus trying to expose sqlite3_db_config since:
* Pointers are involved in the sqlite3_db_config signature
* Enum values are needed as well, and that means we're either modifying mozIStorageConnection/Connection every time or callers are directly including sqlite3.h.
* [noscript] since you need to be native code to register a tokenizer.

We could arguably go a step further and implement "registerFts3Tokenizer" as a native method that does the enable/register/disable dance, but we don't have any in-tree consumers at this point, so it seems safer to stick to a reusable primitive that we want anyways.
Priority: -- → P3
See Also: → 1340155
This patch provides a middle-ground solution, that no longer requires enabling FTS3_TOKENIZER on the sqlite3 build and instead does do via db_config.  To match existing behaviour, the new db_config calls are only added for MOZ_THUNDERBIRD and MOZ_SUITE builds, just as it was for sqlite3 bundling.

The principle advantage here is that thunderbird can use a system sqlite on linux that no longer requires FTS3_TOKENIZER to be compile-time default-enabled.  It may not be a perfect solution but it is better than the status-quo.

Asked glandium for review to confirm the #ifdef and moz.build changes are acceptable.
Assignee: nobody → axs
Attachment #9028965 - Flags: review?(mh+mozilla)
(previous patch contained typos)
Attachment #9028965 - Attachment is obsolete: true
Attachment #9028965 - Flags: review?(mh+mozilla)
Attachment #9029024 - Flags: review?(mh+mozilla)
the sqlite3_db_config symbol needs to be exposed in the bundled sqlite for this to work.  Added to the patch.
Attachment #9029024 - Attachment is obsolete: true
Attachment #9029024 - Flags: review?(mh+mozilla)
Attachment #9029336 - Flags: review?(mh+mozilla)
Comment on attachment 9029336 [details] [diff] [review]
Move FTS3_TOKENIZER enablement to mozStorageConnection.cpp

Review of attachment 9029336 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good, but I'd like some storage peer looking into whether this is hooking in the right places to initialize the tokenizer "manually".

::: storage/mozStorageConnection.cpp
@@ +679,5 @@
>      return convertResultCode(srv);
>    }
>  
> +#ifdef INIT_SQLITE_FTS3_TOKENIZER
> +  ::sqlite3_db_config(mDBConn, SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER, 1, 0);

maybe NULL instead of 0?
Attachment #9029336 - Flags: review?(mh+mozilla)
Attachment #9029336 - Flags: review?(mak77)
Attachment #9029336 - Flags: review+
(In reply to Ian Stakenvicius from comment #3)
> Created attachment 9029336 [details] [diff] [review]
> Move FTS3_TOKENIZER enablement to mozStorageConnection.cpp

Since names of macros in Mozilla code often start with "MOZ_" prefix, I suggest to rename the macro added here to "MOZ_SQLITE_ENABLE_FTS3_TOKENIZER".


(In reply to Mike Hommey [:glandium] from comment #4)
> Sounds good, but I'd like some storage peer looking into whether this is
> hooking in the right places to initialize the tokenizer "manually".

It seems good for me to call sqlite3_db_config directly after calling a sqlite3_open* function (in this case sqlite3_open_v2).

> ::: storage/mozStorageConnection.cpp
> @@ +679,5 @@
> >      return convertResultCode(srv);
> >    }
> >  
> > +#ifdef INIT_SQLITE_FTS3_TOKENIZER
> > +  ::sqlite3_db_config(mDBConn, SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER, 1, 0);
> 
> maybe NULL instead of 0?

Perhaps a pointer is theoretically better, but SQLite documentation mentions 0:
https://sqlite.org/fts3.html#custom_application_defined_tokenizers
(SQLite internally handles the third and later arguments of sqlite3_db_config using va_start/va_arg/va_end and when the second argument is SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER, then the third argument is read as int and the fourth argument is read as int*.)
Comment on attachment 9029336 [details] [diff] [review]
Move FTS3_TOKENIZER enablement to mozStorageConnection.cpp

Review of attachment 9029336 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is ok, Andrew's approach doesn't seem to add much to this, surely it would reduce times at which the 2 arguments version is exposed to code, but anyway the only vector I see is a legacy extension, but then the same could use the proposed API as well. New code should not use fts3 anyway.
Having his fix is better than the status-quo, because at least system sqlite can avoid the security concerns.

::: storage/mozStorageConnection.cpp
@@ +680,5 @@
>    }
>  
> +#ifdef INIT_SQLITE_FTS3_TOKENIZER
> +  ::sqlite3_db_config(mDBConn, SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER, 1, 0);
> +#endif

In all the callpoints, please asseign the return value to srv, and then add a
MOZ_ASSERT(srv == SQLITE_OK);
after the call, inside the ifdef/endif

::: storage/moz.build
@@ +107,5 @@
>          DEFINES['MOZ_STORAGE_MEMORY'] = True
>  
> +# Thunderbird needs the 2-argument version of fts3_tokenizer()
> +if CONFIG['MOZ_THUNDERBIRD'] or CONFIG['MOZ_SUITE']:
> +    DEFINES['INIT_SQLITE_FTS3_TOKENIZER'] = 1

I agree with previous comment about a moz_ prefix, probably I'd just short this to MOZ_SQLITE_FTS3_TOKENIZER
Attachment #9029336 - Flags: review?(mak77) → review+
Agreed that this fix is fine and better than the status quo.  And it doesn't involve exposing additional API surface in mozStorage, which is nice!

(The downside is that fts3_tokenizer() stays active all the time in Thunderbird.  But as noted before, if an attacker can control the SQL invoked, Thunderbird is already compromised, so that particular issue doesn't really matter.)
Blocks: 1514408
No longer blocks: 1514408
Hey Ian, I'd like to piggyback on this patch over in bug 1514683. Is this close to landing?
Flags: needinfo?(axs)
Apologies, been AFK for the past week.  Yes it's ready to go, once I'm back at my rig (tomorrow) I will upload the new patch with all comments addressed.

This update should incorporate all recommendations.

Attachment #9029336 - Attachment is obsolete: true
Flags: needinfo?(axs)
Attachment #9034831 - Flags: review+
Keywords: checkin-needed

Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c69cf0bb19cd
Enable support for SQLite custom FTS3 tokenizers at run time. r=mak

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.