utelle / wxsqlite3

wxSQLite3 - SQLite3 database wrapper for wxWidgets (including SQLite3 encryption extension)
http://utelle.github.io/wxsqlite3
Other
598 stars 181 forks source link

Alternatives for setting cipher parameters at runtime #46

Closed utelle closed 6 years ago

utelle commented 6 years ago

Currently selecting and/or configuring the encryption cipher at runtime requires to perform one or more calls to wxsqlite3_config resp wxsqlite3_config_cipher or to execute one or more SQL statements using the function wxsqlite3_config, before calling sqlite3_key or executing PRAGMA key='pass phrase';.

However, these approaches may be inconvenient or even not working under certain circumstances depending on the actual database interface.

Therefore I consider to implement an alternative for specifying cipher configuration parameters. The following 2 approaches come to mind:

1) Using a prefix for the encryption key (like for example [cipher=aes256cbc;kdf_iter=12345]actual_pass_phrase) This approach is rather easy to implement, but requires to manipulate the pass phrase in the application. 2) Using additional parameters in the database file URI (like for example file://database.db3?cipher=aes256cbc&kdf_iter=12345) This approach avoids to having to manipulate the pass phrase, but it is more difficult to intercept and could potentially cause conflicts in future versions of SQLite, if there happen parameter name clashes.

Questions:

1) Which approach should be chosen? 2) Are there other possible, maybe preferrable approaches? 3) How should accidentally incorrectly specified parameters be handled? a) ignore just the invalid parameter(s) b) ignore all given parameters c) let the operation fail with an error code d) handle an invalid key prefix just as a regular pass phrase (including the prefix)

Feedback is welcome!

Willena commented 6 years ago

Hi @utelle !

I did not had the time to implement the latest features of wxsqlite3 in the java connector I am currently maintaining.

I personally like the second one even if it involve more processing to parse the file URI. I'll have to check how the Java SQLite connector is working, but properties at the end of the filename seems to be accepted (https://docs.oracle.com/javase/tutorial/jdbc/basics/connecting.html ).

But there is one thing that make me thinking : It is about the fact that we are including the passphrase inside parameters used to configure the SQLite "engine". Why not keeping the pragma syntax to apply the key on the database and use URI parameters to select the cipher type ? (It might be easier to implement since it does not imply manipulation with the passphrase)

Here is an example in Java :

//Connect and quickly preconfigure the cipher used
 conn = DriverManager.getConnection("jdbc:sqlite:file.db?cipher=aes256cbc&kdf_iter=12345");
 //Decrypting the database
 Statement stmt = conn.createStatement()
 stmt.execute("PRAGMA key='myKey'");

Here is what I was thinking for the error management. I would maybe do a mix of 3a and 3c. Let me explain :

If doing what I described above is too complex to implement, I would simply prefer 3c (it means that only valid arguments will be accepted, and anything not valid will throw an error).

Any thought on that ?

utelle commented 6 years ago

Hi @Willena!

Thanks for your feedback.

I did not had the time to implement the latest features of wxsqlite3 in the java connector I am currently maintaining.

The approach for cipher configuration should certainly be compatible with SQLite JDBC drivers. Maybe you could share your knowledge how SQLite JDBC drivers (like Xerial) handle parameters given in the connection string.

I personally like the second one even if it involve more processing to parse the file URI.

There is one major problem associated with that approach: if the symbol SQLITE_HAS_CODEC is defined on compiling the original SQLite source code (as is the case for the wxSQLite3 encryption extension, of course), the option to add the parameter key or hexkey to the URI is already supported by SQLite itself. If this is done, the SQLite code for opening the database calls the function sqlite3_key_v2, before the wxSQLite3 encryption extension gets the chance to process any cipher configuration parameters that were appended to the URI. Intercepting this process would require changes to the original SQLite source code - something I definitely want to avoid. If cipher configuration parameters are passed via a key prefix, this would not impose such problems.

So maybe I should consider to implement both approaches, so that the user can choose the one that suits best for him.

I'll have to check how the Java SQLite connector is working, but properties at the end of the filename seems to be accepted (https://docs.oracle.com/javase/tutorial/jdbc/basics/connecting.html ).

Yes. However, how parameters are specified seems to be driver specific. The connection string for MySQL as described at the end of the web page you mentioned corresponds to what a SQLite connection string would look like.

But there is one thing that make me thinking : It is about the fact that we are including the passphrase inside parameters used to configure the SQLite "engine".

Not necessarily. Personally, I would prefer to separate configuration parameters from the actual passphrase.

Why not keeping the pragma syntax to apply the key on the database and use URI parameters to select the cipher type ? (It might be easier to implement since it does not imply manipulation with the passphrase)

The pragma syntax for applying the key is part of the original SQLite code. Support for this pragma will not be removed, of course.

The official SQLite Encryption Extension (SEE) uses key prefixes to let the user select the cipher algorithm (if the used SEE variant supports this). That is, one could say it is common practice to use key prefixes for manipulating the behaviour of the encryption extension.

Here is an example in Java :

//Connect and quickly preconfigure the cipher used
 conn = DriverManager.getConnection("jdbc:sqlite:file.db?cipher=aes256cbc&kdf_iter=12345");
 //Decrypting the database
 Statement stmt = conn.createStatement()
 stmt.execute("PRAGMA key='myKey'");

In fact, this is what I have in mind. However, since SQLite itself supports the key parameter in the connection string it will be a bit tricky to process the cipher configuration parameters before the key is set.

Here is what I was thinking for the error management. I would maybe do a mix of 3a and 3c. Let me explain :

  • If it is a mandatory argument, I would fail with the corresponding error code (for example : "(XXX) Cipher not found")

Since defaults are defined for all configuration parameters there are no mandatory parameters. However, parameter names could be misspelled. And in case of a key prefix the overall syntax of the prefix could be incorrect.

  • Else if it is not mandatory, I would return a negative error code that give the index of the first ignored parameter. if the parameter name is unknown we ignore the all association (name=value). If the name is known and the value is unknown (aka invalid) I would decide to use the default value and return a negative number.

Using negative error numbers could impose problems, because it would not conform to the default SQLite behaviour. Maybe it would be better to emit the error code SQLITE_MISUSE accompanied by an explanatory message.

If doing what I described above is too complex to implement, I would simply prefer 3c (it means that only valid arguments will be accepted, and anything not valid will throw an error).

Any thought on that ?

As far as I can tell SQLite itself simply ignores unknown URI parameters without emitting error or warning messages. I think I should follow that route. Thus, it will be the user's responsibility to correctly specify parameters.

Willena commented 6 years ago

Hi @utelle

The approach for cipher configuration should certainly be compatible with SQLite JDBC drivers.

When I said that I did not had the time to implement the new features in the JDBC connector it is not about the fact that it does not work. In the past when I've seen that it require to execute few SQL statement to configure how we open the database, I decided that it would be easier if I will add some method/new constructor to simplify the process. Now, your are trying yourself at library level to simply the process and that's very good 😄

Maybe you could share your knowledge how SQLite JDBC drivers (like Xerial) handle parameters given in the connection string.

As I said yesterday it something that I did not check. This morning I looked at it and here is some details. In my version of the JDBC which is a fork of Xerial one, here is how input parameters are processed: Lets suppose that we want to open with the following string jdbc:sqlite:file.db?cipher=aes256cbc&kdf_iter=12345:

  1. It extract the file name in order to open it.
  2. Each parameter are converted into a list of PRAGMAs In this case, we will have the following statements:
    • pragma cipher=aes256cbc
    • pragma kdf_iter=12345
  3. Once the file is opened, it execute the list of PRAGMA statements

So the code I gave you yesterday, can be simplified to :

//Connect and quickly pre-configure the cipher used and apply the key
 conn = DriverManager.getConnection("jdbc:sqlite:file.db?cipher=aes256cbc&kdf_iter=12345&key=mykey");

The JDBC even accept a Properties object. To be exact URI parameters are converted to the Properties object. So the java code given above is equivalent to this one

Properties props = new Properties();
props.setProperty("cipher","aes256cbc");
props.setProperty("kdf_iter","12345");
props.setProperty("key","mykey");
conn = DriverManager.getConnection("jdbc:sqlite:file.db", props);

and the previous one is equivalent to this one

conn = DriverManager.getConnection("jdbc:sqlite:file.db", props);
Statement stmt = conn.createStatement()
stmt.execute("PRAGMA cipher=aes256cbc");
stmt.execute("PRAGMA kdf_iter=12345");
stmt.execute("PRAGMA key=mykey");

After looking at the original SQLite java connector code, I can confirm that the URI method is not a good one (Adding PRAGMA statement will involve changes in the original SQLite (see #25), and if we don't want to use PRAGMA, it is still possible but I will have to change the connector so that it will execute the SQL function instead of PRAGMAs). At final I now think that you should implement the key prefix method.

There is one major problem associated with that approach: if the symbol SQLITE_HAS_CODEC is defined on compiling the original SQLite source code (as is the case for the wxSQLite3 encryption extension, of course), the option to add the parameter key or hexkey to the URI is already supported by SQLite itself. If this is done, the SQLite code for opening the database calls the function sqlite3_key_v2, before the wxSQLite3 encryption extension gets the chance to process any cipher configuration parameters that were appended to the URI. Intercepting this process would require changes to the original SQLite source code - something I definitely want to avoid. If cipher configuration parameters are passed via a key prefix, this would not impose such problems. So maybe I should consider to implement both approaches, so that the user can choose the one that suits best for him.

I'm not an expert about the SQLite original source code, but I also think that we should avoid changes to the original source code, it will be harder to update and maintain with newer SQLite versions. For those reason, it may be better to only implement the key prefix method.

The connection string for MySQL as described at the end of the web page you mentioned corresponds to what a SQLite connection string would look like.

I totally agree with what you said. I was talking about the mysql part of the page, but as I explain before it does not work as expected.

The official SQLite Encryption Extension (SEE) uses key prefixes to let the user select the cipher algorithm (if the used SEE variant supports this). That is, one could say it is common practice to use key prefixes for manipulating the behavior of the encryption extension.

If the official SEE uses key prefix, I would consider doing the same thing, so that it is "consistent" from one lib to another and it would not change a lot the way things work inside. Again, I think that for an easier development we should definitely choose to use prefix in the key.

Using negative error numbers could impose problems, because it would not conform to the default SQLite behavior. Maybe it would be better to emit the error code SQLITE_MISUSE accompanied by an explanatory message.

For error handling, since negative error number are not "standard", I will agree to the SQLITE_MISUSE error.

As far as I can tell SQLite itself simply ignores unknown URI parameters without emitting error or warning messages. I think I should follow that route. Thus, it will be the user's responsibility to correctly specify parameters.

If we use key prefixes, is it still relevant ?

utelle commented 6 years ago

Hi @Willena,

The approach for cipher configuration should certainly be compatible with SQLite JDBC drivers.

When I said that I did not had the time to implement the new features in the JDBC connector it is not about the fact that it does not work.

I have to admit that the last time I used Java and JDBC is quite a long time ago. Therefore your comments about how things are handled in known SQLite JDBC drivers are very welcome.

This morning I looked at it and here is some details. In my version of the JDBC which is a fork of Xerial one, here is how input parameters are processed: Lets suppose that we want to open with the following string jdbc:sqlite:file.db?cipher=aes256cbc&kdf_iter=12345:

  1. It extract the file name in order to open it.
  2. Each parameter are converted into a list of PRAGMAs In this case, we will have the following statements:
    • pragma cipher=aes256cbc
    • pragma kdf_iter=12345

I cloned your JDBC git repository and inspected the relevant Java code myself today. Fortunately, your observation is not completely correct. It is true that the query parameters (following after ?) are inspected, but they are only converted to pragmas if they are recognized as known pragmas. Only for known pragmas the driver generates pragma commands and executes them. All unknown parameters are kept in the query string and are forwarded to the SQLite library for further processing.

In fact, the behaviour described above is important, because it allows to pass parameters which are already now handled by SQLite itself (like mode=readonly).

So the code I gave you yesterday, can be simplified to :

//Connect and quickly pre-configure the cipher used and apply the key
 conn = DriverManager.getConnection("jdbc:sqlite:file.db?cipher=aes256cbc&kdf_iter=12345&key=mykey");

The JDBC even accept a Properties object. To be exact URI parameters are converted to the Properties object. So the java code given above is equivalent to this one

Properties props = new Properties();
props.setProperty("cipher","aes256cbc");
props.setProperty("kdf_iter","12345");
props.setProperty("key","mykey");
conn = DriverManager.getConnection("jdbc:sqlite:file.db", props);

I think this would be a bad idea:

1) It would require to adjust the SQLiteConfig class, so that it recognizes additional parameters, for which an SQL statement has to be executed. 2) Since setting cipher configuration parameters requires special handling, because pragmas can't be used for this purpose without modifying the SQLite code, it would introduce a strong coupling between the driver and the encryption extension

After looking at the original SQLite java connector code, I can confirm that the URI method is not a good one (Adding PRAGMA statement will involve changes in the original SQLite (see #25), and if we don't want to use PRAGMA, it is still possible but I will have to change the connector so that it will execute the SQL function instead of PRAGMAs). At final I now think that you should implement the key prefix method.

Now, knowing that unrecognized parameters are kept in the URI string, I think using the URI method would still be preferrable over the key prefix method.

I already found out that it is rather simple to access parameters in the URI query string. The SQLite library provides special functions for this purpose (originally meant to be used by VFS implementations, but usable in other places, too).

The tricky part will be to detect, when to apply which parameters, so that the current method of configuring the encryption extension will not be blocked.

At the moment I have the following strategy in mind:

1) If the URI query string contains the key or heykey parameter, SQLite will call sqlite3_key_v2 before the encryption extension gets the chance to analyze the query string. Therefore it would be necessary to extend the function sqlite3_key_v2 to handle the initialization of the encryption extension in this case, and to apply any encryption parameters given in the URI query string. 2) If the URI query string does not contain the key or heykey parameter, then the encryption extension will be initialized after the database handle was created by SQLite. Additionally, The parameters in the URI query string would be checked whether they affect the encryption extension. If yes, they would be set as default values for the database connection - this would allow to override them later on with explicit commands.

Still unclear is whether this approach will work properly for attaching encrypted databases, too.

I'm not an expert about the SQLite original source code, but I also think that we should avoid changes to the original source code, it will be harder to update and maintain with newer SQLite versions.

It's already hard enough to adjust the encryption extension for new SQLite versions, because it happens quite frequently that internal SQLite functions use by the encryption extension change their signature. Therefore I definitely want to avoid to be forced to maintain code changes to the SQLite library itself. Up to now it was possible to just drop in the SQLite amalgamation code (and to adjust the code of the encryption extension if necessary). I really intend to keep it that way.

For those reason, it may be better to only implement the key prefix method.

Well, for now I decided to start with the URI parameter method. If there will be a demand for the key prefix method, I will consider that later.

If the official SEE uses key prefix, I would consider doing the same thing, so that it is "consistent" from one lib to another and it would not change a lot the way things work inside.

IMHO key prefixes have a main drawback: it may be hard to detect syntax glitches and the user could then end up with an unintended database key. This can easily happen for the official SEE according to its documentation.

Using negative error numbers could impose problems, because it would not conform to the default SQLite behavior. Maybe it would be better to emit the error code SQLITE_MISUSE accompanied by an explanatory message.

For error handling, since negative error number are not "standard", I will agree to the SQLITE_MISUSE error.

Ok.

As far as I can tell SQLite itself simply ignores unknown URI parameters without emitting error or warning messages. I think I should follow that route. Thus, it will be the user's responsibility to correctly specify parameters.

If we use key prefixes, is it still relevant ?

The advantage of key prefixes would be that we know that all given parameters are meant to be applied to the encryption extension. So invalid/unknown parameters could be flagged as being in error.

Well, as said above I will now start with the URI method. And I hope I will get constructive feedback from other users when it will be available for testing.

Willena commented 6 years ago

@utelle

I cloned your JDBC git repository and inspected the relevant Java code myself today. Fortunately, your observation is not completely correct. It is true that the query parameters (following after ?) are inspected, but they are only converted to pragmas if they are recognized as known pragmas. Only for known pragmas the driver generates pragma commands and executes them. All unknown parameters are kept in the query string and are forwarded to the SQLite library for further processing.

I'm sorry, I read the code too quickly and I gave you (partially) wrong information

IMHO key prefixes have a main drawback: it may be hard to detect syntax glitches and the user could then end up with an unintended database key.

You're right, I never thought of this one !

Well, as said above I will now start with the URI method. And I hope I will get constructive feedback from other users when it will be available for testing.

Okay. If you need something I'll be happy to help !

utelle commented 6 years ago

@Willena,

I'm sorry, I read the code too quickly and I gave you (partially) wrong information

No problem. Luckily the URI approach seems to be a viable way after all. :smile:

Okay. If you need something I'll be happy to help !

Thanks in advance. I'll announce when the new approach is ready for testing.

utelle commented 6 years ago

Commit fd1e9e893f4d8628db4d7bd36a09121d590f87f7 of branch multi-cipher adds the option to configure the cipher scheme via the URI query string. Details, how to use the URI query parameters, can be found in the readme file.

Please give it a try and report any issues you experience.

Regarding error reporting currently only specifying an unknown cipher name results in a SQLITE_MISUSE error. Unknown cipher parameters and/or values out of range are silently ignored. Unknown cipher parameters will always be ignored, since it is impossible to detect whether a parameter name was misspelled or is maybe used by another extension. But values out of range could be detected and reported (causing a failure of the open or attach operation).

If no critical bugs show up and no essential features are missing, I will then merge the changes into master.

Willena commented 6 years ago

Hi @utelle

I've done some very quick tests on the java platform and everything seams to be fine. I'm able to open and close multiple databases using different parameters values in the URI. I'll do more tests during the week and I will let you know if I encounter any issue.

For the moment : the only issue that I found is related to the error handling in java. When I give an unknown cipher name, I get an exception that say the database was closed, But I don't have a specific message or code (e.g SQLITE_MISUSE followed by your message witch is "unknown cipher 'xxxx'" ). The thing that makes me think that it is java related is that the sqliteshell for the same URI, prints a valid error message (as expected) : "unknown cipher 'xxxx'".

utelle commented 6 years ago

Hi @Willena,

I've done some very quick tests on the java platform and everything seams to be fine. I'm able to open and close multiple databases using different parameters values in the URI.

Good news. Thanks.

I'll do more tests during the week and I will let you know if I encounter any issue.

Great. Thanks in advance.

For the moment : the only issue that I found is related to the error handling in java. When I give an unknown cipher name, I get an exception that say the database was closed,

Could you please post the exact message you get? Do you get a stack trace, too? That is, the call hierarchy?

But I don't have a specific message or code (e.g SQLITE_MISUSE followed by your message witch is "unknown cipher 'xxxx'" ). The thing that makes me think that it is java related is that the sqliteshell for the same URI, prints a valid error message (as expected) : "unknown cipher 'xxxx'".

If the native call to sqlite3_open_v2 in the JNI code fails, the database handle is closed immediately. The error code is forwarded in the exception, but the error message is not extracted. And since the database handle was closed, there is no chance to retrieve the error message later on. That is, it will require some work on the Java interface to correctly forward the SQLite error code and error message in case of failure.

However, there is also a problem in SQLite itself. If SQLite calls the function sqlite3_key_v2 internally, the returned error code is not checked at all. That is, obviously the function sqlite3_key_v2 of the official SQLite Encryption Extension never fails. Therefore errors resulting from invalid cipher configuration can be detected only later on, if accessing the opened or attached database fails. This can't be helped, unfortunately.

Willena commented 6 years ago

@utelle

Could you please post the exact message you get? Do you get a stack trace, too? That is, the call hierarchy?

I can post the java stacktrace :

Exception in thread "main" java.sql.SQLException: The database has been closed
    at org.sqlite.core.NativeDB.throwex(NativeDB.java:478)
    at org.sqlite.core.NativeDB.errmsg_utf8(Native Method)
    at org.sqlite.core.NativeDB.errmsg(NativeDB.java:144)
    at org.sqlite.core.DB.newSQLException(DB.java:953)
    at org.sqlite.core.DB.throwex(DB.java:918)
    at org.sqlite.core.NativeDB._open_utf8(Native Method)
    at org.sqlite.core.NativeDB._open(NativeDB.java:78)
    at org.sqlite.core.DB.open(DB.java:195)
    at org.sqlite.SQLiteConnection.open(SQLiteConnection.java:243)
    at org.sqlite.SQLiteConnection.<init>(SQLiteConnection.java:61)
    at org.sqlite.jdbc3.JDBC3Connection.<init>(JDBC3Connection.java:28)
    at org.sqlite.jdbc4.JDBC4Connection.<init>(JDBC4Connection.java:21)
    at org.sqlite.JDBC.createConnection(JDBC.java:116)
    at org.sqlite.JDBC.connect(JDBC.java:90)
    at java.sql.DriverManager.getConnection(DriverManager.java:664)
    at java.sql.DriverManager.getConnection(DriverManager.java:270)
    at Main.main(Main.java:7)

I've been tracing things and it confirms what you were saying : JNI calls sqlite3_open_v2 but I've seen that the returned error code (21 for SQLITE_MISUSE) is forwarded to the Java code. BUT now I totally understand why I'm getting a The database has been closed exception. In the C code of JDBC we get the error code (the database is already closed since it failed to open it), then the java code get it and tries to get the error string that correspond to the error code. In the stack we can see this line : at org.sqlite.core.DB.newSQLException(DB.java:953). The corresponding instruction is this one return newSQLException(errorCode, this.errmsg()); (with errorCode equals to 21) but to fully complete the constructor calls it need the results of this.errmsg() which in facts is a call to this.errmsg_utf8() which is a call to the native interface that execute the following code (file NativeDB.c line 646)

 db = gethandle(env, this);
    if (!db)
    {
        throwex_db_closed(env);
        return NULL;
    }

As you said, the database is closed so it can't get the corresponding handle. And it create a new exception while generating the first one. And the second one is now The database is closed.

If the native call to sqlite3_open_v2 in the JNI code fails, the database handle is closed immediately. The error code is forwarded in the exception, but the error message is not extracted. And since the database handle was closed, there is no chance to retrieve the error message later on. That is, it will require some work on the Java interface to correctly forward the SQLite error code and error message in case of failure.

You were completely right and a quick dive in the JDBC implementation confirms it. It will need a little bit of work on the java side to get the correct error string.

utelle commented 6 years ago

In principle, it is a good approach of the JDBC code to clean up allocated memory (by closing the database handle). However, the last error message should be extracted first and passed along with the exception.

BTW, I will switch from the error code SQLITE_MISUSE to SQLITE_ERROR as the documentation for SQLITE_MISUSE reads:

If SQLite ever returns SQLITE_MISUSE from any interface, that means that the application is incorrectly coded and needs to be fixed. Do not ship an application that sometimes returns SQLITE_MISUSE from a standard SQLite interface because that application contains potentially serious bugs.

Willena commented 6 years ago

BTW, I will switch from the error code SQLITE_MISUSE to SQLITE_ERROR

No problem !

However, the last error message should be extracted first and passed along with the exception.

I'll update the java interface the retrieve the string message.

utelle commented 6 years ago

Commit aa0a669de08e0e3409140fc526c8bec0f6c99c20 includes the change of the error code as well as a few minor changes.

Willena commented 6 years ago

That's perfect. I'll test that as soon as possible. In the meantime I've managed to get and return a correct error message the the java code (See commit Willena/sqlite-jdbc-crypt@3c7a4e4).

utelle commented 6 years ago

Your modification of the JDBC code looks good. That improves the error diagnostics, not only for the encryption extension.

If your tests will be successful, I think it is then the time to merge the changes back to the master branch and prepare a new release.

Willena commented 6 years ago

Everything seems to be okay, with your latest commit. You can start to merge things.

utelle commented 6 years ago

Release wxSQLite3 4.3.0 now contains the new feature to configure ciphers via the database URI.