Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 10, 2018
Merged

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

merged 2 commits into from
Oct 10, 2018

Conversation

mgrojo
Copy link
Member

@mgrojo mgrojo commented Oct 5, 2018 •
edited
Loading

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
Copy link
Member 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 October 5, 2018 17:23
justinclift
justinclift reviewed Oct 5, 2018
View reviewed changes
src/sqlitedb.cpp Outdated
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/left/leave/ :smile:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote that? My eyes hurt 😄

@mgrojo
Leaving the loading of extensions enabled might be a security risk
1381a41 
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 Compare October 5, 2018 18:10
@MKleusberg
Copy link
Member

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
Copy link
Member 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
Copy link
Member

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
Copy link
Member Author

mgrojo commented Oct 9, 2018

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

@mgrojo
Preference for allowing loading extensions from SQL code
0dc7777 
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
Copy link
Member

justinclift commented Oct 10, 2018 •
edited
Loading

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
Copy link
Member

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
@MKleusberg MKleusberg deleted the disable_sql_load_extension branch October 10, 2018 19:27
MKleusberg pushed a commit that referenced this pull request Oct 10, 2018
@mgrojo @MKleusberg
Leaving the loading of extensions enabled might be a security risk (#…
de9874d 
…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
Copy link
Member

Excellent. 😄

@mgrojo
Copy link
Member 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
Copy link
Member

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 mentioned this pull request Oct 15, 2018
Merged
mgrojo added a commit that referenced this pull request Oct 16, 2018
@mgrojo
Update of translation files for PR #1558
d6c08d4 
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
@mgrojo
Update of translation files for PR #1558
a321f43 
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 mgrojo mentioned this pull request Oct 16, 2018
Closed
mgrojo added a commit that referenced this pull request Oct 17, 2018
@mgrojo
Update es_ES translation for PR #1558
ef99ea2
justinclift pushed a commit that referenced this pull request Oct 18, 2018
@mgrojo @justinclift
Update es_ES translation for PR #1558
a1ce0f8
@FriedrichFroebel FriedrichFroebel mentioned this pull request Oct 18, 2018
Merged
mgrojo added a commit that referenced this pull request Oct 19, 2018
@mgrojo
Merge pull request #1579 from FriedrichFroebel/German_translation
92295a3 
Update German translation for PR #1558
mgrojo pushed a commit that referenced this pull request Oct 19, 2018
@FriedrichFroebel @mgrojo
update German translations for PR #1558
1f407b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinclift justinclift justinclift left review comments

@MKleusberg MKleusberg Awaiting requested review from MKleusberg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.11.0 - Where we take over the world...
Development

Successfully merging this pull request may close these issues.
3 participants
@mgrojo @MKleusberg @justinclift