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

Formalize specs for transcrypt configurations with pbkdf2 & configured salt. #134

Open
Erotemic opened this issue May 8, 2022 · 4 comments

Comments

@Erotemic
Copy link

Erotemic commented May 8, 2022

This issue is to discuss how pbkdf2 and new methods for generating salt should be handled in #132.

PBKDF2

At a high level the problem is that transcrypt could be far more secure by using pbkdf2. Right now if you have at least a 5-word diceware-style password you are probably ok, and with 6 words you should be ok, should be ok isn't enough. Regardless enabling pbkdf2 drastically increases security against brute force passwords, so we should be able to leverage that if we want. The tradeoff is that encrypt/decrypt round trips are slightly slower (although there are mitigations for this as mentioned in #132), and in practice it's not too bad.

Currently #132 allows the user to enable pbkdf2 by either

(1) they add the cli args-pbkdf2 or --use-pbkdf2=1. I like the first arg as it mirrors openssl, but I'm not sure about the second arg. I think it is important to have a flag it is a key/value pair. (key/value pair CLIs are far easier to work with problematically).

(2) if they do not use a CLI arg, then transcript prompts them, similar to what it does for cipher, and makes the default choice "0", or off.

Note that existing repos will have to be decrypted with the current default settings and then rekeyed to change any config variable. The value for pbkdf2 is set in a similar way when you rekey.

Using --pbkdf2 is a no brainier. Currently openssl warns if you don't do it. The details about the exact names of the CLI args are minor details. But there is one detail that is important to note: the current salt method that transcrypt uses renders the security of pbkdf2 moot. This brings us to the next topic, the salt method and ultimately the new versioned transcrypt configuration.

Salt Method

The current salt method has a security vulnerability. This is because it is partially derived from the password. A clever attacker can take advantage of this to perform a low-cost brute force attack on the password. It might be better to not writeup the details of how to perform the attack on a public page.

Ultimately the problem is that the password is used to determine the password along with the file name that is being encrypted and a hash of its contents. So how do we fix it? We need the salt to be deterministic so git doesn't think the file changed when it hasn't.

How can we do this? We could choose a random password when transcrypt is initialized and then save that to an unencrypted config file. If we check that file into the repo we can ensure different checkouts also operate in the same way. The current way I've done this is with the idea of a .transcrypt/config files that stores the values trancrypt.config-salt (this new file is managed with git's configuration tool, which is very versatile and perfect for this task).

There is also something to be said about storing other non-secret transcrypt config values in this .transcrypt/config folder. It could make initialization of a new checkout easier if transcrypt remembered the settings for all these new non-password configs (and the cipher as well). But that is not implemented yet, but getting feedback on that would be good here. For now the only thing that is stored in the "versioned" .transcrypt/config folder is the config-salt value, which currently can be supplied by the user with --config-salt=<VALUE>, but if that value is empty it will either read an existing value from the versioned config or generate a new random one and write it to the versioned config.

An additional note is that the value of custom-salt is rerandomized (unless explicitly given) when a --rekey happens.

So that's the high level idea of how to get a secure deterministic string to replace the password in the salt generation code.

If this option was on by default with the option of adding --config-salt='' if you wanted to, I'd be happy with the CLI. But being backwards compatible makes this tricky. To handle backwards compatibility I added yet another config option called --salt-method and it defaults to "password", which means the current default scheme is used, and --config-salt is ignored. The other option is --salt-method=configured, which enables this scheme.

This seems overly complex to me as I'm writing it out. Maybe it would be better to remove --salt-method and then just have special values of --config-salt. If --config-salt=password, then that enables the old behavior, if --config-salt=random or --config-salt='', it enables the randomized method, otherwise it uses exactly that specified --config-salt.

@Erotemic
Copy link
Author

I've added a writeup of the algorithm in #132 I've checked this into the repo as an RST document:

https://github.com/Erotemic/transcrypt/blob/enhance-configuration/docs/algorithm.rst

@dawidmachon
Copy link

Any updates on this topic? From my perspective is super important to follow it and develop this awesome tool in correct direction.

@Erotemic
Copy link
Author

I have a fork where I'm using these updates, and I haven't had the time to work through all of the details to land the changes as a backwards compatible and user friendly PR. 95% of the work is done though; it would be nice if someone could help with the last 5%, I simply don't forsee having the bandswith in the near future.

@jmurty
Copy link
Collaborator

jmurty commented Mar 19, 2023

@Erotemic has indeed done a lot of great work towards using the better and recommended PBKDF2 algorithm in Transcrypt, see in particular PR #136.

There is still a fair bit of work remaining however – probably more than just 5% unfortunately – to make it usable and entirely consistent and reliable across different platforms plus subtly different implementations of the OpenSSL CLI (OpenSSL and LibreSSL). There is for example a known issue per this comment hinting at compatibility problems.

Given the trouble I've had recently in #147 and #158 to maintain compatibility across systems and tool version with the current algorithm, I'm concerned the PBKDF2 work will be more difficult than it seems to get it completely right. And I am also short of time to work on this.

All of which is to say it would be great if some folks can help out, maybe by testing the PR #136 version to see if it works for them, and providing feedback there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants