New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leaving the loading of extensions enabled might be a security risk #1558

Merged
merged 2 commits into from Oct 10, 2018

Conversation

Projects
None yet
3 participants
@mgrojo
Copy link
Contributor

mgrojo commented Oct 5, 2018

Using sqlite3_enable_load_extension not only allows loading extensions
through the C-API but also through the SQL function load_extension().
That might be a security risk if the user is unaware that executing an
SQL file can lead to native code execution and not only to database file
modification.

See issue #1551

@mgrojo

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Oct 5, 2018

I open this PR since I'm not sure of the implications: are our users expecting to being able to load extensions through SQL code; and is it a good idea to call sqlite3_enable_load_extension for disabling and enabling instead of only enabling the C-API using the newer sqlite3_db_config?

@mgrojo mgrojo requested a review from MKleusberg Oct 5, 2018

if(sqlite3_load_extension(_db, filePath.toUtf8(), nullptr, &error) == SQLITE_OK)
int result = sqlite3_load_extension(_db, filePath.toUtf8(), nullptr, &error);

// Disable extension loading (we don't want to left the possibility of calling load_extension() from SQL)

This comment has been minimized.

@justinclift

justinclift Oct 5, 2018

Member

s/left/leave/ 😄

This comment has been minimized.

@mgrojo

mgrojo Oct 5, 2018

Author Contributor

I wrote that? My eyes hurt 😄

Leaving the loading of extensions enabled might be a security risk
Using sqlite3_enable_load_extension not only allows loading extensions
through the C-API but also through the SQL functioon load_extension().
That might be a security risk if the user is unaware that executing an
SQL file can lead to native code execution and not only to database file
modification.

See issue #1551

@mgrojo mgrojo force-pushed the disable_sql_load_extension branch from fae12ba to 1381a41 Oct 5, 2018

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Oct 9, 2018

I don't know about load_extension() but I remember that there are some users who don't use any of the GUI features we offer and only rely on the Execute SQL tab. No idea if they're loading extensions though but the preferable option for everybody might be a dialog which pops up and asks for an extra confirmation or something.

That said, I just tried to run this in the Execute SQL tab

load_extension('/path/to/extension.so');

and it returned a syntax error. In the SQL log and in the debugger I could see that SQLite splits this line into two statements: load_extension and ('/path/to/extension.so'); and the first one alone obviously is a syntax error. Does anybody else have the same problem?

@mgrojo

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Oct 9, 2018

The correct syntax would be:

SELECT load_extension('/path/to/extension.so');

Another option could be a preference setting for enabling the load of extensions in SQL. By default it should be disabled (following the design decision of SQLite itself).

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Oct 9, 2018

Haha, you're right! I forgot about the SELECT part 🙄 And yeah, a config option would probably be the easiest solution. But one could still argue that you should get a warning when importing a malicious SQL file, even with the config option turned on. But before overthinking this, adding a config option might be a good first step 😉

@mgrojo

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Oct 9, 2018

Ok, I'll add the setting to the branch.

Preference for allowing loading extensions from SQL code
New setting that authorizes the execution of load_extension() from SQL
code. Default value, false, following the design decision of SQLite, that
disables this function unless by default.

Added notice about the option in the calltips of the two function
variants.
@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Oct 10, 2018

When this gets merged (into master) we should cherry-pick it across to the v3.11.x branch too, so it gets into the next alpha or beta build as appropriate. 😄

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Oct 10, 2018

Awesome, this is looking pretty good. Good description in the Preferences dialog too 😄 I'll merge this and cherry-pick it to the v3.11.x branch as @justinclift suggested 😄

@MKleusberg MKleusberg merged commit 5cf00dd into master Oct 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MKleusberg MKleusberg deleted the disable_sql_load_extension branch Oct 10, 2018

MKleusberg added a commit that referenced this pull request Oct 10, 2018

Leaving the loading of extensions enabled might be a security risk (#…
…1558)

* Leaving the loading of extensions enabled might be a security risk

Using sqlite3_enable_load_extension not only allows loading extensions
through the C-API but also through the SQL functioon load_extension().
That might be a security risk if the user is unaware that executing an
SQL file can lead to native code execution and not only to database file
modification.

See issue #1551

* Preference for allowing loading extensions from SQL code

New setting that authorizes the execution of load_extension() from SQL
code. Default value, false, following the design decision of SQLite, that
disables this function unless by default.

Added notice about the option in the calltips of the two function
variants.
@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Oct 11, 2018

Excellent. 😄

@mgrojo

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Oct 14, 2018

I just noticed that we haven't updated the translation files with the new strings present in this PR. Updating now might raise conflicts with the present or ongoing PR, so I don't know which is the best way to go.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Oct 15, 2018

Update the strings in master and v3.11.x then let the translators know.

In theory, it shouldn't happen toooooo many times before release. 😄

@mgrojo mgrojo referenced this pull request Oct 15, 2018

Merged

Update German translation #1569

mgrojo added a commit that referenced this pull request Oct 16, 2018

Update of translation files for PR #1558
There are two new strings in Preferences (an option and its tooltip) and
two functions call-tips that are updated with this appendix:
"Use of this function must be authorized from Preferences."

Line number changes have been discarded after lupdate so the amount of
changes for merging of translation pull requests are minimised.

mgrojo added a commit that referenced this pull request Oct 16, 2018

Update of translation files for PR #1558
There are two new strings in Preferences (an option and its tooltip) and
two functions call-tips that are updated with this appendix:
"Use of this function must be authorized from Preferences."

Line number changes have been discarded after lupdate so the amount of
changes for merging of translation pull requests are minimised.

mgrojo added a commit that referenced this pull request Oct 17, 2018

justinclift added a commit that referenced this pull request Oct 18, 2018

mgrojo added a commit that referenced this pull request Oct 19, 2018

mgrojo added a commit that referenced this pull request Oct 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment