resilar / sqleet

SQLite3 encryption that sucks less
The Unlicense
378 stars 56 forks source link

Handling of URI parameters #23

Closed utelle closed 4 years ago

utelle commented 5 years ago

Sorry that I give rather late feedback regarding the new URI parameter feature of sqleet.

1) Handling of erroneous URI parameter values

You decided to use the return code SQLITE_MISUSE to signal the use of erroneous URI parameter values. Looking at the SQLite documentation, I'm not convinced that using this return code is really the best choice:

The SQLITE_MISUSE return code might be returned if the application uses any SQLite interface in a way that is undefined or unsupported. For example, using a prepared statement after that prepared statement has been finalized might result in an SQLITE_MISUSE error.

SQLite tries to detect misuse and report the misuse using this result code. However, there is no guarantee that the detection of misuse will be successful. Misuse detection is probabilistic. Applications should never depend on an SQLITE_MISUSE return value.

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.

Especially the last paragraph indicates that a shipped application should never happen to experience a return code SQLITE_MISUSE. However, this is difficult to accomplish, because the valid range of parameters could potentially change from version to version and there is no interface available to query the allowed range in advance, so that the application could check user input and only pass valid data to the interface.

Unfortunately, using another return code (for example, SQLITE_ERROR or SQLITE_CANTOPEN) wouldn't solve the problem either. IMHO the main problem is that no detailed error diagnostics are available, that is, there is no indication which URI parameter(s) is(are) in error.

Maybe one idea could be to add appropriate calls to sqlite3_log, thus giving an application a chance to get a more detailed information.

In my own implementation I chose to silently ignore invalid URI parameters. However, I admit that I'm not happy with that approach either.

One major problem in SQLite itself is that sqlite3_key_v2 is called internally in some cases (for example, if the URI parameter key is used, or if an encrypted database is attached), but its return code is silently ignored. That is, the application will not be informed properly, if anything goes wrong during initializing the codec.

2) Naming of URI parameters

Of course you are free to name the URI parameters used in sqleet as you please. However, in my own encryption extension (which handles various different ciphers) it makes things harder to understand and use. For example, what you called salt for sqleet is named cipher_salt for SQLCipher; and while you offer 2 variation of the salt parameter (namely, salt and hexsalt), SQLCipher only accepts hex values - and IMHO this makes sense, because the salt is usually a binary value which is difficult to pass as an URI parameter in non-hex form anyway.

Another problem can arise from name clashes, because any SQLite extension may allow configuration parameters via URI parameters. That's the main reason why I decided to silently ignore URI parameters which do not provide valid values for the encryption extension: they might belong to another extension, for which the given values are acceptable. At the moment I have no conclusive idea how this problem could or should be tackled.

3) URI parameter salt vs header

I have to admit that I have not yet fully understood the implications of specifying both parameters, salt and header, or just one of them. Could you please elaborate in greater detail how it is meant to use these 2 parameters in sqleet? Thanks.

resilar commented 5 years ago
  1. Handling of erroneous URI parameter values

From the user's perspective, I find SQLITE_MISUSE to be the most descriptive error code to indicate invalid URI parameters. However, the last paragraph about SQLITE_MISUSE from SQLite documentation is quite ominous, so SQLITE_CANTOPEN might be safer choice although less descriptive. I'll look into sqlite3_log to see if it could be used to return meaningful error messages.

  1. Naming of URI parameters

The probability of salt header kdf skip or page_size colliding with another extension's URI parameters is reasonably small in my estimation. Collisions can be handled on a case by case basis if they ever happen (I predict not).

I first considered using cipher_salt for compatibility's sake, but that felt wrong because the cipher_ prefix comes from the name SQLCipher (technically the salt should be named kdf_salt since it is passed as input to the key derivation algorithm). Maintaining compatibility with SQLCipher is a non-goal for sqleet so I decided to use brief parameter names that are more consistent with SQLite3's URI parameters.

Sorry if this causes extra effort in your project. Users of wxSQLite3 are probably capable of doing cipher_saltsalt style translations themselves if they are interested in accessing sqleet databases with wxSQLite3. Thus, you can probably stick with one good set of PRAGMA/parameter names and use them for all supported database formats.

  1. URI parameter salt vs header

Yes, the difference can be confusing. salt is used in key derivation and header is what gets stored in the first 16 bytes of the database file. Most often these are the same value (hence header defaults to the value of salt if ungiven), but in some cases it is desirable to not store the salt in the database and use the first 16 bytes of the database file for some other purpose. This is currently required for creating iOS-compatible databases (header is set to SQLite3 header magic string and user provides salt for key derivation).

The documentation of sqleet could be clearer about the distinction between the two.

utelle commented 5 years ago
  1. Handling of erroneous URI parameter values

From the user's perspective, I find SQLITE_MISUSE to be the most descriptive error code to indicate invalid URI parameters.

Yes, I felt the same ... until I read the description of the error code in the SQLite documentation. In wxSQLite3 I chose to use SQLITE_ERROR, although it is a very generic error code.

However, the last paragraph about SQLITE_MISUSE from SQLite documentation is quite ominous, so SQLITE_CANTOPEN might be safer choice although less descriptive. I'll look into sqlite3_log to see if it could be used to return meaningful error messages.

IMHO using the SQLite logging mechanism at least allows to better debug a failing SQLite application. I definitely intend to enhance the wxSQLite3 encryption extension by using this feature. In the end however, it is up to the user/developer to implement a hook intercepting the logging messages.

  1. Naming of URI parameters

The probability of salt, header, kdf, skip or page_size colliding with another extension's URI parameters is reasonably small in my estimation. Collisions can be handled on a case by case basis if they ever happen (I predict not).

Well, for the multi-cipher implementation in wxSQLite3 it would certainly be nice to use the same parameter name for the same purpose for all affected cipher schemes (and probably I will follow this route somehow). However, I can understand your point.

I first considered using cipher_salt for compatibility's sake, but that felt wrong because the cipher_ prefix comes from the name SQLCipher (technically the salt should be named kdf_salt since it is passed as input to the key derivation algorithm).

Probably the SQLCipher developers will agree with you. However, from my point of view cipher is simply a technical term for a certain encryption cipher scheme.

Maintaining compatibility with SQLCipher is a non-goal for sqleet so I decided to use brief parameter names that are more consistent with SQLite3's URI parameters.

Naturally my objective is a bit different, because I intend to support the most common cipher schemes within a single library. Currently, SQLCipher doesn't make use of URI parameters at all. No idea whether they plan to change that in the future.

One option I'm considering is to use some sort of prefix for all encryption related parameters.

Sorry if this causes extra effort in your project. Users of wxSQLite3 are probably capable of doing cipher_saltsalt style translations themselves if they are interested in accessing sqleet databases with wxSQLite3. Thus, you can probably stick with one good set of PRAGMA/parameter names and use them for all supported database formats.

You are right: it is my problem to define a reasonable set of URI parameter names. If someone will be using the wxSQLite3 encryption extension he/she will have to know the differences between the various cipher schemes anyway.

  1. URI parameter salt vs header

Thanks for the clarifications.

resilar commented 4 years ago

Commit ea466e4 somewhat improved the README documentation of existing URI parameters. The distinction between header and salt parameters should be now clearer.

I'm closing this issue. The brief parameter names are nice IMO and I do not care so much about the common SQLCipher naming convention. The abuse of SQLITE_MISUSE error code feels problematic, but in practice it has been very useful in pinpointing mysterious bugs like #32 to the URI parameter handling. Plan is to switch to less controversial error code(s) after implementing a better and more descriptive error logging mechanism (sqlite3_log() looks promising, but I have not yet tried it).

Edit: typos

utelle commented 4 years ago

Regarding the error codes it would be nice if SQLite officially offered a few additional error codes to choose from. But since this is not the case, one either has to sort of abuse an existing error code or to choose a very generic error code.

Of course it helps to track down problems using a rather specific / exotic error code. However, probably the same could be achieved using additional logging.

BTW I will have to check the wxSQLite implementation as well whether it is affected by similar issues as sqleet, although I didn't get any error reports so far. Unfortunately, I currently have very limited chances to work on my projects due to personal matters.

resilar commented 4 years ago

The recent nasty bugs #26 #29 #32 are most likely sqleet-specific because the codec initialization and URI parameter handling code are not shared with wxSQLite or other projects. Testing wxSQLite with AddressSanitizer is still a good idea though.