Help with cipher salt/storage?

[ethindp]

Asking this here because this is, I'm pretty sure, not an issue with SQLite3MC at all, so issues to my knowledge isn't the most appropriate forum for it.

I'm trying to add salting to my cipher implementation. However, I can't seem to quite get encryption/decryption right. I've looked at the various cipher implementations and they all handle this differently in non-legacy mode: some use OTKs, some are pure legacy ciphers, some copy the SQLite file header into data[0..16]... The result of all of this is that currently WAL mode is breaking with my cipher. My current code for encryption/decryption is:

static int encrypt_page(void* cipher, int page, unsigned char* data, int len, int reserved) {
	struct xchacha20_cipher* c = (struct xchacha20_cipher*)cipher;
	if (c->counter == ULLONG_MAX) {
		sqlite3_log(SQLITE_ERROR, "Nonce overflow in encryption/decryption routine; aborting");
		return SQLITE_ABORT;
	}
	if (reserved != 40 || (len - reserved) < 1 || page < 0) {
		return SQLITE_IOERR_CORRUPTFS;
	}
	const int actual_size = len - reserved;
	uint8_t nonce[24];
	memset(nonce, 0, 24);
	const uint64_t n = c->counter;
	const uint64_t n2 = (uint64_t)page;
	nonce[8] = (n >> (8 * 0)) & 0xff;
	nonce[9] = (n >> (8 * 1)) & 0xff;
	nonce[10] = (n >> (8 * 2)) & 0xff;
	nonce[11] = (n >> (8 * 3)) & 0xff;
	nonce[12] = (n >> (8 * 4)) & 0xff;
	nonce[13] = (n >> (8 * 5)) & 0xff;
	nonce[14] = (n >> (8 * 6)) & 0xff;
	nonce[15] = (n >> (8 * 7)) & 0xff;
	nonce[16] = (n2 >> (8 * 0)) & 0xff;
	nonce[17] = (n2 >> (8 * 1)) & 0xff;
	nonce[18] = (n2 >> (8 * 2)) & 0xff;
	nonce[19] = (n2 >> (8 * 3)) & 0xff;
	nonce[20] = (n2 >> (8 * 4)) & 0xff;
	nonce[21] = (n2 >> (8 * 5)) & 0xff;
	nonce[22] = (n2 >> (8 * 6)) & 0xff;
	nonce[23] = (n2 >> (8 * 7)) & 0xff;
	if (page == 1) {
		crypto_aead_lock(data + 24, &data[24 + actual_size + 24], c->key, nonce, NULL, 0, data + 24, actual_size - 24);
		for (int i = 0; i < 24; ++i) {
			data[24 + actual_size + i] = nonce[i];
		}
		memcpy(data, c->salt, 16);
	} else {
		crypto_aead_lock(data, &data[actual_size + 24], c->key, nonce, NULL, 0, data, actual_size);
		for (int i = 0; i < 24; ++i) {
			data[actual_size + i] = nonce[i];
		}
	}
	c->counter++;
	return SQLITE_OK;
}

static int decrypt_page(void* cipher, int page, unsigned char* data, int len, int reserved, int hmac_check) {
	struct xchacha20_cipher* c = (struct xchacha20_cipher*)cipher;
	if (reserved != 40 || ((len - reserved) < 1) || page < 0) {
		return SQLITE_IOERR_CORRUPTFS;
	}
	const int actual_size = len - reserved;
	uint8_t nonce[24];
	uint8_t mac[16];
	memset(nonce, 0, 24);
	memset(mac, 0, 16);
	if (page == 1) {
		for (int i = 0; i < 24; ++i) {
			nonce[i] = data[24 + actual_size + i];
		}
		for (int i = 0; i < 16; ++i) {
			mac[i] = data[24 + actual_size + 24 + i];
		}
		if (crypto_aead_unlock(data + 24, mac, c->key, nonce, NULL, 0, data + 24, actual_size - 24) == -1) {
			return SQLITE_IOERR_CORRUPTFS;
		}
		memcpy(data, "SQLite format 3\0", 16);
	} else {
		for (int i = 0; i < 24; ++i) {
			nonce[i] = data[actual_size + i];
		}
		for (int i = 0; i < 16; ++i) {
			mac[i] = data[actual_size + 24 + i];
		}
		if (crypto_aead_unlock(data, mac, c->key, nonce, NULL, 0, data, actual_size) == -1) {
			return SQLITE_IOERR_CORRUPTFS;
		}
	}
	return SQLITE_OK;
}

I'm a bit confused and uncertain what I should try next. I could stick with a legacy cipher implementation, but that would make salting pretty much impossible and would make keys pretty much completely deterministic, which I don't want. I could reserve an extra 16 bytes and put the salt at the end of the page, but that also seems like a weird solution and an unnecessary hack given that SQLite3MC already provides this (but it is an option I suppose...). Am I just not copying something, or what should I change to get this to work right?

[utelle]

Asking this here because this is, I'm pretty sure, not an issue with SQLite3MC at all, so issues to my knowledge isn't the most appropriate forum for it.

You are right. It is not an issue with SQLite3MC. Therefore opening a discussion was the right thing to do.

I'm trying to add salting to my cipher implementation.

Why don't you use one of the cipher schemes provided by SQLite3MC? The cipher schemes chacha20, sqlcipher, and ascon128 all use a general key salt (stored at the beginning of the database header) and random nonce per database page.

However, I can't seem to quite get encryption/decryption right. I've looked at the various cipher implementations and they all handle this differently in non-legacy mode: some use OTKs, some are pure legacy ciphers, some copy the SQLite file header into data[0..16]...

Well, different cipher schemes use different approaches.

If a cipher scheme uses a general key salt, it is stored at the beginning of the database header (´data[0..15]` of database page 1) - this is the only space where up to 16 bytes can be stored, because the unencrypted content of those bytes is fixed.

On some platforms the first 16 bytes of the database header need to stay unencrypted. In such a case the application needs to store the salt elsewhere and has to provide it via URI parameter on opening the database.

The result of all of this is that currently WAL mode is breaking with my cipher. My current code for encryption/decryption is:

In short, your implementation is broken. And I doubt that it works in any journal mode.

static int encrypt_page(void* cipher, int page, unsigned char* data, int len, int reserved) {
	struct xchacha20_cipher* c = (struct xchacha20_cipher*)cipher;
	if (c->counter == ULLONG_MAX) {
		sqlite3_log(SQLITE_ERROR, "Nonce overflow in encryption/decryption routine; aborting");
		return SQLITE_ABORT;
	}
	if (reserved != 40 || (len - reserved) < 1 || page < 0) {
		return SQLITE_IOERR_CORRUPTFS;
	}

So, the number of reserved bytes is 40 - 16 bytes for a MAC and 24 bytes for nonce, if I interpreted the rest of your code

	const int actual_size = len - reserved;
	uint8_t nonce[24];
	memset(nonce, 0, 24);
	const uint64_t n = c->counter;
	const uint64_t n2 = (uint64_t)page;
	nonce[8] = (n >> (8 * 0)) & 0xff;
	nonce[9] = (n >> (8 * 1)) & 0xff;
	nonce[10] = (n >> (8 * 2)) & 0xff;
	nonce[11] = (n >> (8 * 3)) & 0xff;
	nonce[12] = (n >> (8 * 4)) & 0xff;
	nonce[13] = (n >> (8 * 5)) & 0xff;
	nonce[14] = (n >> (8 * 6)) & 0xff;
	nonce[15] = (n >> (8 * 7)) & 0xff;
	nonce[16] = (n2 >> (8 * 0)) & 0xff;
	nonce[17] = (n2 >> (8 * 1)) & 0xff;
	nonce[18] = (n2 >> (8 * 2)) & 0xff;
	nonce[19] = (n2 >> (8 * 3)) & 0xff;
	nonce[20] = (n2 >> (8 * 4)) & 0xff;
	nonce[21] = (n2 >> (8 * 5)) & 0xff;
	nonce[22] = (n2 >> (8 * 6)) & 0xff;
	nonce[23] = (n2 >> (8 * 7)) & 0xff;

Up to here the code seems to be ok, although I don't understand why you set the first 8 bytes of the nonce always to zero.

	if (page == 1) {
		crypto_aead_lock(data + 24, &data[24 + actual_size + 24], c->key, nonce, NULL, 0, data + 24, actual_size - 24);

The pointer to the MAC is wrong! Keep in mind that you use 40 reserved bytes, but &data[24 + actual_size + 24] equals &data[24 + len - 40 + 24] equals &data[len + 8] points to a location 8 bytes past the end of the page buffer. That is, you will corrupt some memory not belonging to the page buffer. Even if your application doesn't crash, you will not retrieve correct data on unencrypting page 1.

The correct expression would be &data[actual_size + 24] - assuming that you store the nonce first and the MAC second in the reserved space.

		for (int i = 0; i < 24; ++i) {
			data[24 + actual_size + i] = nonce[i];
		}

Same problem here: the correct expression would be data[actual_size + i].

		memcpy(data, c->salt, 16);
	} else {
		crypto_aead_lock(data, &data[actual_size + 24], c->key, nonce, NULL, 0, data, actual_size);
		for (int i = 0; i < 24; ++i) {
			data[actual_size + i] = nonce[i];
		}
	}
	c->counter++;
	return SQLITE_OK;
}

static int decrypt_page(void* cipher, int page, unsigned char* data, int len, int reserved, int hmac_check) {
	struct xchacha20_cipher* c = (struct xchacha20_cipher*)cipher;
	if (reserved != 40 || ((len - reserved) < 1) || page < 0) {
		return SQLITE_IOERR_CORRUPTFS;
	}
	const int actual_size = len - reserved;
	uint8_t nonce[24];
	uint8_t mac[16];
	memset(nonce, 0, 24);
	memset(mac, 0, 16);
	if (page == 1) {
		for (int i = 0; i < 24; ++i) {
			nonce[i] = data[24 + actual_size + i];
		}

Again: correct expression would be data[actual_size + i].

		for (int i = 0; i < 16; ++i) {
			mac[i] = data[24 + actual_size + 24 + i];
		}

Same here: correct expression data[actual_size + 24 + i].

		if (crypto_aead_unlock(data + 24, mac, c->key, nonce, NULL, 0, data + 24, actual_size - 24) == -1) {
			return SQLITE_IOERR_CORRUPTFS;
		}
		memcpy(data, "SQLite format 3\0", 16);
	} else {
		for (int i = 0; i < 24; ++i) {
			nonce[i] = data[actual_size + i];
		}
		for (int i = 0; i < 16; ++i) {
			mac[i] = data[actual_size + 24 + i];
		}
		if (crypto_aead_unlock(data, mac, c->key, nonce, NULL, 0, data, actual_size) == -1) {
			return SQLITE_IOERR_CORRUPTFS;
		}
	}
	return SQLITE_OK;
}

I'm a bit confused and uncertain what I should try next. I could stick with a legacy cipher implementation,

Indeed you could stick to one of the already implemented cipher schemes. However, you are not forced to use a legacy cipher. Legacy ciphers are supported, so that databases created by legacy applications using the original implementation of the corresponding cipher can be accessed. If compatibility with legacy applications is not an issue for you, you can use the (preferred) non-legacy mode of the implemented cipher schemes in SQLite3MC.

but that would make salting pretty much impossible and would make keys pretty much completely deterministic,

This is nonsense. As already said the cipher schemes chacha20, sqlcipher, and ascon128 all use general salt for the key and random nonce per page. That is, the key salt and the page salt is not deterministic. What makes you think that it is deterministic?

On the contrary, your implementation seems to be somewhat deterministic: you use a (most likely deterministic) counter and the (definitely deterministic) page number for the nonce.

which I don't want. I could reserve an extra 16 bytes and put the salt at the end of the page, but that also seems like a weird solution and an unnecessary hack given that SQLite3MC already provides this (but it is an option I suppose...).

Not sure, what you are referring to. Storing the general key salt in the reserved area of a page would be a complete waste of page space, because this salt is used only once for deriving the key. SQLite3MC uses the first 16 bytes of the database header for this purpose.

Am I just not copying something, or what should I change to get this to work right?

Adjust the buffer addresses as stated in my comments, and it should work. But to be frank, I would strongly recommend you to use one of the existing cipher schemes.

[ethindp]

So for the salt, I use random bytes, and then I derive the key with argon2I. It's a password hash algorithm, sure, but it also can be used as a KDF, so I thought why not?

As for the nonce being zero for the first 8 bytes, I did that because I didn't want to generate 8 random bytes for every nonce as that would exhaust most platform RNG pools (i.e. on Linux) especially for a large number of pages/encryption operations. The nonce may be deterministic, but it's not private, either. By being deterministic previously, I meant that the key would be deterministic for the KDF given input a (i.e., given input a, key b would always be generated). If I use Argon2I and salt it, ideally the key shouldn't be deterministic and therefore much harder to figure out, or at least that's the idea. (I do this because the people who are passing keys to this code are not very good with crypto, and so tend to use ASCII or mostly ASCII, and non-truly-random data, as their keys, which are at this point far more like passwords than actual keys. So sure, I'm "patching" that weakness, but nobody said key storage was simple.)

As for the offsets, I did that since I saw the built-in ciphers doing something like that, and I wasn't sure if "encrypt all the things" is what I should be doing or if I only need to encrypt bytes 24..actual_size. I'll add your corrections. (The cipher I'm implementing is XChaCha20, using the Monocypher library.) I would use the built-in ones if the main project maintainer didn't prefer I do this primarily for uniformity reasons if nothing else (Monocypher is being used for the majority of our other crypto stuff, so it made sense at least for me to try to fit it in here). If you believe however that this is generally a wasted effort and the built-in ones should be preferred, I can probably make that work.

[utelle]

So for the salt, I use random bytes, and then I derive the key with argon2I. It's a password hash algorithm, sure, but it also can be used as a KDF, so I thought why not?

Sure, you are free to use various hash algorithms for deriving a key. However, if possible you should make use of proven crypto algorithms for key derivation (like PBKDF2). Inventing your own algorithm is usually not a good idea from a security perspective.

As for the nonce being zero for the first 8 bytes, I did that because I didn't want to generate 8 random bytes for every nonce as that would exhaust most platform RNG pools (i.e. on Linux) especially for a large number of pages/encryption operations.

SQLite3MC uses a CSPRNG based on ChaCha20. It uses entropy from the system on initializing the CSPRNG only. So no risk to exhaust the system RNG pool.

The nonce may be deterministic, but it's not private, either.

Well, that still makes a difference. The nonce really should be random, not deterministic in any way. Yes, the nonce is public, but it makes it nevertheless much more difficult to break a cipher scheme.

By being deterministic previously, I meant that the key would be deterministic for the KDF given input a (i.e., given input a, key b would always be generated).

That's why the more modern cipher schemes supported by SQLite3MC all use a general key salt to avoid this problem.

If I use Argon2I and salt it, ideally the key shouldn't be deterministic

Actually, the algorithm for deriving the key is deterministic. The salt makes it harder to find the correct key by analyzing encrypted data.

and therefore much harder to figure out, or at least that's the idea. (I do this because the people who are passing keys to this code are not very good with crypto, and so tend to use ASCII or mostly ASCII, and non-truly-random data, as their keys, which are at this point far more like passwords than actual keys. So sure, I'm "patching" that weakness, but nobody said key storage was simple.)

Almost all cipher schemes supported by SQLite3MC do not use the passphrase given by the user directly, but derive the actual key by applying hash functions (mostly using PBKDF2). In this respect, SQLite3MC does not need to be patched.

The exception is the System.Data.SQLite cipher scheme, which is supported for allowing to access databases encrypted with that cipher scheme. Therefore using that cipher scheme for new applications is strongly discouraged.

As for the offsets, I did that since I saw the built-in ciphers doing something like that,

Yeah, something like that. You will have to understand what's going on under the hood to get things right.

and I wasn't sure if "encrypt all the things" is what I should be doing or if I only need to encrypt bytes 24..actual_size.

Well, this affects only the first database page, which starts with the database header. The first 16 bytes contain the fixed SQLite signature SQLite format 3\0, the next 8 bytes contain information that is read by SQLite before the encryption scheme is set up for the database - for example, the page size. If these bytes are encrypted SQLite can't detect the page size correctly and therefore an application needs to supply the page size explicitly if a non-default page size is used. This affects all legacy cipher scheme variants.

I'll add your corrections. (The cipher I'm implementing is XChaCha20, using the Monocypher library.)

I have to admit that I haven't come across Monocypher before. It seems to be a rather new crypto library being around since 2018. So I don't know how it compares to other crypto libraries (like OpenSSL which was founded in 1998). Anyway, you are free to replace the internal crypto implementation of SQLite3MC by another implementation. Actually, I was asked a few times in the past to support the use of different crypto libraries (like the original SQLCipher library does).

Personally, I don't need that feature, but if someone is willing to sponsor the effort, I would adjust SQLite3MC accordingly.

I would use the built-in ones if the main project maintainer didn't prefer I do this primarily for uniformity reasons if nothing else (Monocypher is being used for the majority of our other crypto stuff, so it made sense at least for me to try to fit it in here).

Sure, this is understandable.

If you believe however that this is generally a wasted effort and the built-in ones should be preferred, I can probably make that work.

Well, it is a matter of trust. If you believe the Monocypher library is more trust worthy, use it.

SQLite3MC uses reference implementations of the crypto algorithms. That is, I did not reimplement the algorithms on my own.

[ethindp]

Sure, you are free to use various hash algorithms for deriving a key. However, if possible you should make use of proven crypto algorithms for key derivation (like PBKDF2). Inventing your own algorithm is usually not a good idea from a security perspective.

I would agree with this if my algorithm was hand-coded. It is not. The algorithm is Argon2, a winner of the 2015 password hashing competition (PHC). Although it is primarily used for hashing passwords, it can also be used as a KDF. I may however switch to the built-in ciphers just so that everything integrates smoother. I would encourage you to switch to Argon2 at some point; see the OWASP recommendations on password storage for reasons why and good parameters to use if you aren't already using them.

[utelle]

Inventing your own algorithm is usually not a good idea from a security perspective.

I would agree with this if my algorithm was hand-coded. It is not.

Good.

The algorithm is Argon2, a winner of the 2015 password hashing competition (PHC). Although it is primarily used for hashing passwords, it can also be used as a KDF.

Sure. And as said you are free to implement your own cipher scheme for SQLite3MC.

I would encourage you to switch to Argon2 at some point; see the OWASP recommendations on password storage for reasons why and good parameters to use if you aren't already using them.

For one, SQLite3MC doesn't store passphrases, only salt, nonce and HMAC are stored. Secondly, I can't switch to Argon2 - at least not for the existing cipher schemes, because it would introduce an incompatible change. All I can do is to add further cipher schemes using different key derivation functions or to make the key derivation function configurable.

[ethindp]

To be clear, to set the default cipher to something like ASCON-128, do I define HAVE_CIPHER_ASCON128=1 and CODEC_TYPE=CODEC_TYPE_ASCON128? Just making sure. Looks like it's doing the encryption right but just checking to make sure I'm doing this move back to the built-in ones properly.

[utelle]

To be clear, to set the default cipher to something like ASCON-128, do I define HAVE_CIPHER_ASCON128=1 and CODEC_TYPE=CODEC_TYPE_ASCON128?

In principle, yes. However, for all built-in ciphers the corresponding symbol HAVE_CIPHER_*=1 is defined. You have to set HAVE_CIPHER_*=0 for all ciphers you want to exclude.

[ethindp]

In principle, yes. However, for all built-in ciphers the corresponding symbol HAVE_CIPHER_=1 is defined. You have to set HAVE_CIPHER_=0 for all ciphers you want to exclude.

On the currently release this doesn't appear to be correct, unless I'm mistaking the code or something. There's a #if 0 that omits all ciphers by default but enables dynamic ciphers. But I could've missed an outer conditional directive, so

Edit: my bad, I misinterpreted the directives. :P Might need more caffeine. :D Thank you for all your help, I greatly appreciate it.