Remove deprecated Nettle API calls

[npavlinek]

This should fix issue #35

[miekg]

Is it a stretch to just allow only one key-size?

[npavlinek]

Actually that would remove pretty much all of the key size checking and make this change much simpler and shorter.

We could just pick, say AES-256, and pad the keys to 32 bytes. There's two ways we could do that:

1. Pad with some pattern
2. Use something like PBKDF2 (preferable since it's more secure)

[miekg]

agreed, let's do that. So to recap:

1. 1 keysze
2. pad with pbkdf2

[npavlinek]

Ok, this is the first draft. There's a couple of problems:

1. Number of iterations probably shouldn't be hard-coded.

2. The use of a static salt means all users that decide to use encryption, could be attacked by the same rainbow table. (The salt needs to stay the same between encryption and decryption).

None of these issues are really that big, since with rdup you could use some external tool to do crypto.

[miekg]

[ Quoting <[email protected]> in "Re: [miekg/rdup] Remove deprecated ..." ]

Ok, this is the first draft. There's a couple of problems: 1. Number of iterations probably shouldn't be hard-coded. 2. The use of a static salt means all users that decide to use encryption, could be attacked by the same rainbow table. (The salt needs to stay the same between encryption and decryption). None of these issues are really that big, since with `rdup` you could use some external tool to do crypto.

Are these problems that pop up now with the rework you're doing, or were already present?

[npavlinek]

"Problems" with the rework. PBKDF2 requires a number of iterations and salt.

I had a look at rdup code and unless I'm missing something it doesn't seem to pad keys at all, since it gives an error message when given a key that is too short (e.g. 15 bytes). I'm confused because the comment above crypt_init mentions keys being zero-padded.

I also had a look at Nettle code and they don't seem to pad keys either, which is weird because when I tried passing a key without padding it, it worked. It could be that it (Nettle) was reading memory that it shouldn't have and just happened to not be a problem.

I don't have time right now, but I'll give this a better look later today and come up with something.

[npavlinek]

I've thought about this and I think the best course of action is to just use a static salt (i.e. hard-code a salt) and a set number of iterations. The rationale is that even if the attacker has access to the salt, they don't have access to the password or it's hash, since rdup doesn't store them anywhere (it's given by the user). Therefore us storing the salt instead of generating it for each password is not at all a problem.

If you agree, I'll proceed with the above plan and we can be done with this.

[miekg]

[ Quoting <[email protected]> in "Re: [miekg/rdup] Remove deprecated ..." ]

I've thought about this and I think the best course of action is to just use a static salt (i.e. hard-code a salt) and a set number of iterations. The rationale is that even if the attacker has access to the salt, they don't have access to the password or it's hash, since `rdup` doesn't store them anywhere (it's given by the user). Therefore us storing the salt instead of generating it for each password is not at all a problem. If you agree, I'll proceed with the above plan and we can be done with this.

A static salt means a rainbow table would work for all backups made, which is a lot of work and may be a problem. Should we expose an salt option, but default to a salt (for backwards compat reason)? And explain this in the readme and help?

[npavlinek]

This is going to be a long one. Bear with me...

After spending a few hours hacking Nettle and rdup, and manually testing rdup encryption (it's a bit difficult to automate this), I've come up with the following:

Nettle will zero-pad shorter keys. For example if a 16-byte key is passed to aes256_set_encrypt_key, the key will be split into 4-byte sub-keys (bytes 0-3, bytes 4-7, bytes 8-11, bytes 12-15, 16 bytes that are 0). If we disable our own "padding" any key that is smaller than 32-bytes will be padded (we chose 32-byte keys as default) in the new implementation.

Since the old implementation chose the correct function to call depending on key size, keys were never padded (if a user passed a key of a size other than 16-bytes, 24-bytes or 32-bytes, we rejected it and printed an error). Because of this any backups that are encrypted with the old implementation and use key sizes other than 32-bytes, cannot be decrypted since padding changes the key.

If we use our own "padding", which doesn't really pad, but instead uses the original key as an input into a hash function, which then generates a hash of appropriate size (in our case, 32-bytes), and uses a salt to make sure that hashes of the same key differ, keys are always different from the original implementation.

It seems that the only way to ensure backwards compatibility (with the old implementation) is to use the initial implementation of this PR (separate context for each key size), otherwise we would have to provide some sort of a script to "re-encrypt" all backups that were encrypted using the old implementation, with the new one.

If you remember, the problem with the initial implementation was that we had a context for each key size while only using one at a time. And it required additional key size checks in a few places (which is actually what the old code did, but instead of that logic being in rdup it was in Nettle itself). I believe I should be able to use a union to ensure that all contexts occupy the same memory location and therefore avoid memory waste. Although even if I'm unable to do so I think that using 16 extra bytes (on 64-bit) is not that big of a deal.

I tried being as clear as I could, but it's possible that I missed something. If something is unclear, I'd be happy to clear it up.

[miekg]

We should care about backwards compatibility:

It seems that the only way to ensure backwards compatibility (with the old implementation) is to use the initial implementation of this PR (separate context for each key size), otherwise we would have to provide some sort of a script to "re-encrypt" all backups that were encrypted using the old implementation, with the new one.

If this is what you propose. I think I'm OK with that.

[npavlinek]

Using a union to ensure that all three AES contexts occupy the same memory location. I also manually verified that a backup encrypted with the old implementation can be decrypted with the new one.

[miekg]

[ Quoting <[email protected]> in "Re: [miekg/rdup] Remove deprecated ..." ]

Using a union to ensure that all three AES contexts occupy the same memory location. I also manually verified that a backup encrypted with the old implementation can be decrypted with the new one.

Ok, I'll take another look. Thanks!