Generate and store salt

[shapkarin]

Hello. It there any way to make salt configurable?

cosmjs/packages/proto-signing/src/wallet.ts

Line 15 in f198f0a

export const cosmjsSalt = toAscii("The CosmJS salt.");

I found a way to replace that file. And put salt to the env variable:

new webpack.NormalModuleReplacementPlugin(
  new RegExp('@cosmjs/proto-signing/build/wallet.js'),
  path.resolve(
    __dirname,
    'replaced/wallet.js'
  )
)

Of course someone can find the value in a builded js, but it looks harder.

[webmaster128]

Hello. It there any way to make salt configurable?

Heyho! Why do you want to do that? I'd be curious about a detailed motivation and usecase description. If it makes sense to do, it should be trivial to make it configurable through proper APIs.

Of course someone can find the value in a builded js, but it looks harder.

Are you sure you're on the right path here? The salt is not a secret value. It's public and that's perfectly fine. It just makes rainbow attacks harder.

[shapkarin]

While wallet development, I decided that If It just makes rainbow attacks harder, why not make it a bit more harder with "hidden" salt from env variable.

[webmaster128]

Okay, so one scenario could be this: All CosmJS users use the same salt. One can come up with a rainbow table of passwords that are all hashed with one salt in order to attack CosmJS users. Now what you can do is override the salt in each application. Still no need for env variables and hiding. Then you need application specific tables instead of lib specific tables, making them much harder to generate or less attractive.

I'm unsure if this is worth it. Changing the argon 2 parameters should be enough to get unique hashes and at the same time tune the difficulty of the KDF. Then if you push this computation to a WebWorker to not freeze the UI you can go really high with your params. Development effort is much more worth there. Also this heavily reduces the risk that an encrypted wallet cannot be decrypted anymore because you remain compatible with standard CosmJS tooling.

[shapkarin]

Then you need application specific tables instead of lib specific tables, making them much harder to generate or less attractive.

That was the goal. Instead of lib specific tables has different application specific tables so one table can't be used to any app.

[webmaster128]

Yeah. This can be done and there is nothing wrong with it. But I highly recommend not making this a priority and don't implement costy and risky hacks for it.

I leave this ticket open to make the salt configurable. This is easy to implement.

[webmaster128]

I wonder if we could just store the salt together with all other argon2 options. The options need to be read anyways before the decryption. Then we can automatically generate a new salt for every encryption.

[shapkarin]

Agree, it could be an option with default value "The CosmJS salt.".

[shapkarin]

@webmaster128 I don't have problems to provide a new PR. But not right now or today. Maybe tomorrow's evening.

[shapkarin]

@webmaster128 maybe it will be ready even at tomorrows daytime. Looks easy to do. But next should be carefully rechecked at review.

[webmaster128]

Alright, go ahead. No rush. I'll look at it then.

[shapkarin]

will try to add *.spec.ts
if my PR will not include it, feel free to add yours.

[webmaster128]

Looking at the new title, I just want to make sure we are on the same page:

• Salt is randomly generated, not configured at encryption time
• It is stored together with the other Argon2 options in a field salt?: string (hex or base64 encoded)
• When decrypting, the salt is used. When non-existent, the default salt is used.

This means we don't have any additional work for the user of the library – just upgrade.

[shapkarin]

@webmaster128 so with random generated salt we will get not an app specific salt, but encryption specific salt and it should be a part of encrypted result. Right?

[webmaster128]

we will get not an app specific salt, but encryption specific salt

Exactly. Better salting with no configuration.

and it should be a part of encrypted result

It would be stored next to the encrypted data in the config that stores the Argon2 options. The salt is unencrypted (can be public) but unique then.

[shapkarin]

Thanks. Got that. To create random salt for each encryption and store it next to a cipher.

[shapkarin]

@webmaster128 it should be just serialized.salt?

here is the example:

const serialized = {
  type: "directsecp256k1hdwallet-v1",
  kdf: {
    algorithm: "argon2id",
    params: {
      outputLength: 32,
      opsLimit: 24,
      memLimitKib: 12 * 1024,
    },
  },
  encryption: {
    algorithm: "xchacha20poly1305-ietf",
  },
  data: "",
  salt: "",
}

Here is tests. Will improve them and change it('') titles to lowercased, as all spec.

[webmaster128]

The steps are done as follows:

1. User provides password string, possibly low entropy
2. The KDF turns this string into a byte series using a hard to bruteforce algorithm that is configurable. This KDF may or may not have a salt. A output length for the encryption key is configured.
3. The encryption is performed using the encryption key from 2.

Step 3 does not know where the encryption key comes from. It does not know which KDF is used and what a KDF even is. It does not know what a salt is because encryption has no salt.

So the salt clearly belongs to the KDF step. And it is algorithm specific. For this reason I would pout it in kdf.params and Argon2idOptions.

[shapkarin]

What do you think about to code that? Does't looks like I have a lot of time for that. In any case will back to it this week, please mark this issue in a commit #903

[webmaster128]

What do you think about to code that? Does't looks like I have a lot of time for that.

So far I only see two tests. Without a (draft) PR it's hard to see what you did.

But if you don't make it, I'll implement this at some point.

[shapkarin]

@webmaster128 I got emergency things to do. In any case I'm glad of the participation, feel free to implement. Tests are not so good, at least the second one.

[shapkarin]

@webmaster128 I removed second test, and change the first one.

[shapkarin]

@webmaster128 please, have a check my PR: added backward compatibility test: serialize with current DirectSecp256k1HdWallet version, and deserialize with the updated one. Please, review it. Maybe I need to add some other cases for other packages?

[shapkarin]

@webmaster128 link #906

[webmaster128]

Nice start, but no implementation in #906.

I started working on this myself now. It's a bit tricky becasue of the API design that allows KDF execution on a different thread (e.g. WebWorker). Will keep you updated on progress.

[shapkarin]

@webmaster128 I appreciate that, thanks. I can't find any time for that.

[shapkarin]

@webmaster128 hello, can you please closer describe the problem? I will try to find some time for the help.

[shapkarin]

@webmaster128 hello I will try to prepare the PR.
I guess that I can just use something like:

import { Random } from '@cosmjs/crypto'

const salt = Random.getBytes(sodium.crypto_pwhash_SALTBYTES);

[shapkarin]

@webmaster128 it may be like that

But next I should return the salt itself with

cosmjs/packages/crypto/src/libsodium.ts

Line 46 in 663d39c

return sodium.crypto_pwhash(

and it use libsodium-wrappers package

[shapkarin]

@webmaster128 hello. random salt is already used in the wallet module

cosmjs/packages/proto-signing/src/wallet.ts

Lines 55 to 72 in 486ab65

	export async function encrypt(
	plaintext: Uint8Array,
	encryptionKey: Uint8Array,
	config: EncryptionConfiguration,
	): Promise<Uint8Array> {
	switch (config.algorithm) {
	case supportedAlgorithms.xchacha20poly1305Ietf: {
	const nonce = Random.getBytes(xchacha20NonceLength);
	// Prepend fixed-length nonce to ciphertext as suggested in the example from https://github.com/jedisct1/libsodium.js#api
	return new Uint8Array([
	...nonce,
	...(await Xchacha20poly1305Ietf.encrypt(plaintext, encryptionKey, nonce)),
	]);
	}
	default:
	throw new Error(`Unsupported encryption algorithm: '${config.algorithm}'`);
	}
	}

, but not in the directsecp256k1hdwallet

cosmjs/packages/proto-signing/src/directsecp256k1hdwallet.ts

Lines 294 to 298 in fcf01d4

	public async serialize(password: string): Promise<string> {
	const kdfConfiguration = basicPasswordHashingOptions;
	const encryptionKey = await executeKdf(password, kdfConfiguration);
	return this.serializeWithEncryptionKey(encryptionKey, kdfConfiguration);
	}

cosmjs/packages/proto-signing/src/wallet.ts

Lines 26 to 36 in fcf01d4

	export async function executeKdf(password: string, configuration: KdfConfiguration): Promise<Uint8Array> {
	switch (configuration.algorithm) {
	case "argon2id": {
	const options = configuration.params;
	if (!isArgon2idOptions(options)) throw new Error("Invalid format of argon2id params");
	return Argon2id.execute(password, cosmjsSalt, options);
	}
	default:
	throw new Error("Unsupported KDF algorithm");
	}
	}

[shapkarin]

@webmaster128 hello. I finally found time to make that contribution better. xchacha20poly1305Ietf salt also gets type Uint8Array right now.

[webmaster128]

Wallet encryption might be removed entirely from CosmJS. If you use this in production code, please let us know in #1479.