Implement proper salts in encrypted journals

[KarimPwnz]

Since we're discussing changing the storage format in #1107, perhaps we could implement proper salting. Currently, encrypted journals use a hard-coded salt that is passed into PBKDF2HMAC. However, it is not technically a salt but a pepper: a salt is unique for every encryption.

To implement this, we should store the unencrypted, base64-encoded salt in the journal file—it is not meant to be secret.

[wren]

This is a good point to bring up. I'm completely open to discussion, so this seems like a good place to do it.

You're absolutely right that the current implementation isn't really a salt. It's fixed for all journals, and in no way increases the difficulty of a pre-computed attack on a journal file.

In the past, I've sided against moving the salt into the config file (to make it actually a salt) for a few reasons:

1. Moving the salt (or even getting rid of it) would break backwards compatibility with jrnl's current storage format. So, then we would have a v1, v2, and v3 storage format to deal with. Not necessarily a deal-breaker, but I would want to see some concrete advantages before committing jrnl to a larger maintenance burden, and increased user confusion.
2. Currently, a user only needs their password in order to be able to decrypt jrnl files. If the salt is moved to the config, then users would need their password and the config file to do the same thing. This isn't bad in and of itself, but it is more difficult to keep track of two things than it is to keep track of one. And historically, keeping track of key files along with passwords has proven to be too difficult for most non-technical users (and many technical ones). For example, look at PGP and it's waning popularity in the security community. If we do use a salt, I would strongly prefer to store it in the journal file itself.
3. I believe this would significantly complicate managing journals in jrnl both for users and for maintainers.
   • For users: a user can currently run --decrypt and --encrypt over and over again with the same password, and be confident they'll always be able to decrypt that journal with that password, even if they restore from a backup. If there's a salt in the config, it would change each time --encrypt is run. So now a user has to keep track of exactly when a backups was taken, and sync the config file with the journal itself. And since the config is most often kept in a separate location from the journal files, we risk a user not backing up the config, and rendering their backups useless.
   • For maintainers: jrnl currently defers responsibility for decrypting journals to two places. First, we use established libraries to handle the actual encryption/decryption. We recognize that the maintainers and communities of these libraries are more knowledgeable than we are about these topics, and we defer to their expertise. Second, we rely on the user to keep track of their own password (whether they are memorizing it, and using a keyring is completely their choice). This both empowers users to be in control of their data, and allows us to freely manage the config file. In my view, it's jrnl responsibility to manage the config file, and it's much easier knowing that no matter what happens to the config file, the worst case scenario is a user loses a few preferences, but can never lose a journal because of this. While losing preferences is still not ideal, it's a significantly lower burden for us than knowing we can potentially lock users out of their journal files permanently.
4. Speaking of libraries, the one we use currently already helps us fend against hash attacks. Before encrypting a journal, it prepends a timestamp of when the command was run to the file, then encrypts the contents with the timestamp attached. You can see an example of this by encrypting the same journal twice, and comparing the encrypted contents (they don't match). This isn't a salt, but does make it more difficult crack an encrypted journal.
5. Salts really only help against hash attacks, which makes them ideal in securing passwords. We don't store passwords for the user, so I'm not sure it would actually make a journal file more secure. The attacks I'm aware of that salts would guard against (e.g. dictionary, rainbow tables) are pretty useless against a journal file. For those attacks to succeed you need to know the contents of the hash (i.e. tables of known passwords, dictionary words, etc). To apply those same attacks to a even a fairly short journal file (say 100 lines long with 80 line width) would be the equivalent attempting to crack an 8,000 character long password. And that password also contains at very least one timestamp, but probably several. That approach seems very impractical to me, and seems like attacking a journal's password is much easier (even if it's stored in a keyring with a salt of its own).

That all being said, I'm not against salts, but would want to be absolutely sure we're addressing all of these points if and when we implement them.

[KarimPwnz]

I think my proposal deals with some ideas in points 1 to 3 by storing the salt in the journal instead of the config file. Additionally, for backwards compatibility, I think us changing #1107 already breaks it.

However, I do acknowledge that point 4 is significant: rainbow tables are no longer as feasible; as you said, timestamps do almost act as salts.

Still, pedantically, I disagree on point 5. If it weren't the case that timestamps were prepended, a rainbow table could be generated by running password lists (such as rockyou.txt) through jrnl's encryption function and creating decryption keys. Nevertheless, since point 4 is significant enough, I do not think salting is entirely necessary. Perhaps the only change I could motivate now is to rename the variable from salt to pepper. Other than that, you've convinced me.

[wren]

I think my proposal deals with some ideas in points 1 to 3 by storing the salt in the journal instead of the config file. Additionally, for backwards compatibility, I think us changing #1107 already breaks it.

Yes, you're right. I'm not sure where I got the config file storage in my head from (it must have been an earlier conversation with someone else). Sorry about that!

I do agree that #1107 already breaks it, so if we do implement this, it should be at the same time as that issue. Sounds like we're on the same page about that, but I wanted to clarify it here for anyone that stumbles into this issue.

Still, pedantically, I disagree on point 5. If it weren't the case that timestamps were prepended, a rainbow table could be generated by running password lists (such as rockyou.txt) through jrnl's encryption function and creating decryption keys.

I definitely meant point 5 in context of point 4. Sorry if I wasn't clear about that. This is still a good point to bring up, though, so thank you for that. It's always good to have more sets of eyes on jrnl's security.

Nevertheless, since point 4 is significant enough, I do not think salting is entirely necessary. Perhaps the only change I could motivate now is to rename the variable from salt to pepper. Other than that, you've convinced me.

I'm always for more clarity in our codebase, so renaming this pepper is great for now. In the long term (if/when we change the jrnl storage format), we'll either remove it entirely, or add a salt in the file.

[KarimPwnz]

Awesome. I'll push the pepper change and some comments clarifying the implementation.

[KarimPwnz]

Wait... @wren, I am not sure point 4 is right. Fernet does have a timestamp as part of the token that it generates, but that has nothing to do with the hash. The timestamp is not meant to add cryptographic strength (it only exists to track expiry, etc.). Further, the timestamp is not involved in the hashing, so dictionary and rainbow table attacks are certainly possible. And on second thought, I am not sure how prepending a timestamp to a hash wouldn't break everything.

As such, I am again convinced that implementing a rotating salt is necessary.

[wren]

Hrmm, I think I misunderstood how fernet works. Thanks for pointing this out! I think you're right that this merits reimplemenation.

Either way, I'm fine with salts as long as our implementation 1. stores it in the journal file, and 2. we only make the change at the same time as a storage format change.