Skip to content
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

Load SQLite extensions via SQLite C-API #319

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Load SQLite extensions via SQLite C-API #319

wants to merge 7 commits into from

Conversation

Andy-2639
Copy link
Contributor

This allows to load SQLite extensions without enabling the SQL function load_extension.

@bradh
Copy link

bradh commented May 23, 2019

Is there a way forward on this PR?

@StevenLooman
Copy link

I am interested in this as well. What is the way forward?

@alun
Copy link

alun commented May 23, 2020

I found it's relatively easy to do without this change:

  1. Enable load extension in the connection properties like here
    prop.setProperty("enable_load_extension", "true");

Also possible in DBeaver:
Screenshot 2020-05-23 at 16 12 20

  1. Use SQL to load the extension e.g. SELECT load_extension('/path/to/extension/md5', 'sqlite3_extension_init');

Note the shared library extension must be omitted.

P. S. I've compiled md5 from this source https://github.com/moisseev/sqlite-md5/blob/master/md5.c on macOS using clang CLI:

cc -c md5.c
cc -dynamiclib md5.o -o md5.dylib

@Andy-2639
Copy link
Contributor Author

I found it's relatively easy to do without this change:

Yes, it is. It just allows extensions - arbitrary code - to be loaded into your process by normal SQL queries or SQL injection attacks.
I mean, you have to take care of SQL injections anyway (prepared statements, ...) but humans make errors and enabling enable_load_extension increases the ability of an attacker in case of an possible injection attack IMO.

@alun
Copy link

alun commented May 23, 2020

@Andy-2639 cool, I see you point. I'd say those parts of a program calling load_extension stuff shouldn't be exposed to a non-authorised user's input data. And adding more native calls perhaps enlarges the attacking surface by itself.
What about this function here

public native synchronized int enable_load_extension(boolean enable);

can it be used to disable the feature once all required extensions are loaded? Although it might be added after your PR.

@Andy-2639
Copy link
Contributor Author

Andy-2639 commented May 25, 2020

@alun Yeah,

public native synchronized int enable_load_extension(boolean enable);

should work. However, I don't see it exposed.

So I guess when

public abstract class SQLiteConnection

exposes enable_load_extension and
public Connection createConnection(String url) throws SQLException {

would return a SQLiteConnection, the following would be possible:

  1. Create SQLite connection
  2. Enable extension loading
  3. Load extension
  4. Disable extension loading
  5. Run queries with user provided data (still properly escaping/quoting it - prepared statements(!) - to protect from sql injections)

Edit: Changing the return type is not an option because it breaks API compatibility: https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods

@Andy-2639
Copy link
Contributor Author

I sketched a possible solution to disable extension loading after loading them with the enable_load_extension method: #317 (comment)

@gotson gotson added impacts:JNI Has impact on JNI C code enhancement:SQLite Enhancement about sqlite features labels Jul 29, 2022
@gotson
Copy link
Collaborator

gotson commented Jul 29, 2022

@Andy-2639 given this is 4 years old, it needs to be updated to the latest version. If you can do that i can review the code, i think it would be a good addition to this project.

@gotson gotson added the waiting for feedback Waiting for a feedback from the issue creator label Jul 29, 2022
@Andy-2639
Copy link
Contributor Author

Sorry @gotson, as this PR needs the native libs rebuilt and I currently have no Linux system available to do this, caring about this PR takes too much time for me.

I'm fine with the workaround described in #317 (comment) despite the nasty downcast.
In this PR, I really don't like the serialization I needed to store the extensions to load in the properties ...

@gotson
Copy link
Collaborator

gotson commented Aug 1, 2022

this PR needs the native libs rebuilt and I currently have no Linux system available to do this,

Almost everything is built via Dockcross now, should work well on WSL on Windows too.


I will mark this as needing rework, if someone wants to pickup your work

@gotson gotson added needs rework and removed waiting for feedback Waiting for a feedback from the issue creator labels Aug 1, 2022
@gotson gotson marked this pull request as draft August 1, 2022 03:56
@gotson gotson removed the request for review from xerial August 1, 2022 03:56
@gotson gotson added the help wanted Contributions are welcome label Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement:SQLite Enhancement about sqlite features help wanted Contributions are welcome impacts:JNI Has impact on JNI C code needs rework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants