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

use an authenticated cipher mode by default [FIXED PULL] #3

Closed
wants to merge 1 commit into from

Conversation

ryancdotorg
Copy link
Collaborator

Use id-aes256-GCM by default instead of aes-256-cbc. This ensures that commits modifying the file without using the correct password will be detected.

@elasticdog
Copy link
Owner

Unfortunately, I want to remain compatible with OS X which currently has OpenSSL 0.9.8y out of the box, and thus no GCM support (added in OpenSSL 1.0.1). Sad that they're the lowest common denominator, but I'm not sure of a better way to handle the default value to not confuse mixed Linux/OS X clients.

Any suggestions aside from telling users to brew install openssl?

@ryancdotorg
Copy link
Collaborator Author

What about probing to see which ciphers the installed version of OpenSSL supports, choosing the most-preferred one supported, and print a warning if it's not an authenticated mode (aes-256-gcm aes-256-cbc-hmac-sha1) ?

Something like this?

choose_cipher() {
  for pref in aes-256-gcm aes-256-cbc-hmac-sha1 aes-256-xts aes-256-cbc
  do
    for cipher in `openssl enc -invalid 2>&1 | grep -A9999 'Cipher Types' | sed 's/  */\n/g' | grep -- - | sed 's/^-//'`
    do
      if [ "x$pref" == "x$cipher" ]
      then
        echo $pref
        return 0
      fi
    done
  done
  return 1
}

echo `choose_cipher`

Ugly and inefficient but seems to work.

@ryancdotorg
Copy link
Collaborator Author

Also, ideally I'd suggest falling back to transcrypt adding an hmac itself if it falls back to aes-256-xts or aes-256-cbc, but implementing that would probably be very fragile.

@ryancdotorg
Copy link
Collaborator Author

Even the latest version of OpenSSL appears to not actually work correctly when using aes-256-gcm (aka id-aes256-GCM), aes-256-cbc-hmac-sha1 or aes-256-xts.

http://openssl.6102.n7.nabble.com/id-aes256-GCM-command-line-encrypt-decrypt-fail-td27187.html#a27190

@elasticdog
Copy link
Owner

[that was me before] so the current default is the best we can hope for without bringing in other dependencies?

@ryancdotorg
Copy link
Collaborator Author

I don't see any better options without either doing some truly horrifying shell gymnastics or adding dependencies. You could clearly state that preventing modification of encrypted files by commiters without the password is not a design goal. The issue is CBC malleability - as-is someone can commit changes that manipulate the plaintext in limited ways without having the key[1].

It might be possible to abuse openssl's smime command to get authenticated encryption, but that needs RSA keys and certificates to work.

  1. http://www.jakoblell.com/blog/2013/12/22/practical-malleability-attack-against-cbc-encrypted-luks-partitions/

@elasticdog
Copy link
Owner

I'll think about this a bit and see how convoluted it would be to append an HMAC to the aes-256-cbc ciphertext myself. In that case, what would I want to use as the key for the HMAC?

@elasticdog
Copy link
Owner

Just so I have things straight, this would be a desirable scheme (in improper pseudo-code)?

plaintext
password
key1           = (SHA-256(password) | head -c 32)
key2           = (SHA-256(password) | tail -c 32)
salt           = HMAC-SHA256(<filename>:key1, plaintext)
ciphertext     = aes-256-cbc(key1, salt, plaintext)
authentication = HMAC-SHA256(key2, ciphetext)

filter.crypt.clean  = cat ciphertext authentication
filter.crypt.smudge = ???

What would the ideal process be for decryption, as far as checking the authentication and failing accordingly?

@Bren2010
Copy link

Just a quick thought: You don't actually need to split the user's password into key1 and key2. Especially not in that way--it doesn't protect against any attacks and it doesn't improve the keyspace. If you insist, though, I'd recommend a PBKDF. (I don't see a way to do that with the OpenSSL command line, though...)

Also, with these two lines:

git config filter.crypt.clean "openssl enc -$cipher_cmd -pass \"pass:$password_cmd\" -e -a -S $salt_cmd 2> /dev/null"
git config filter.crypt.smudge "openssl enc -$cipher_cmd -pass \"pass:$password_cmd\" -d -a 2> /dev/null || strings"

(Source)

Are you sure that there's no way you can put the encryption and decryption logic into separate bash files, rather than having a one-liner for each?

@elasticdog
Copy link
Owner

Are you sure that there's no way you can put the encryption and decryption logic into separate bash files, rather than having a one-liner for each?

You could put them into separate files, but I wanted to minimize the footprint of transcrypt and make it so once the repository was configured, you didn't need to have anything else special in your path for things to function. I agree it's not ideal to have them as long one-liners, but it's a tradeoff.

@ryancdotorg
Copy link
Collaborator Author

Best practice is to use statistically unrelated keys for encryption and authentication. You actually get this 'for free' just by using the password - for the enc command an ad-hoc KDF[1] is used which uses the salt and password. HMAC will use the password - null padded to the digest length (or its hash if it is longer than the digest length) as the key.

So, this scheme should be fine:

plaintext
password
salt           = HMAC-SHA256(<filename>:password, plaintext)
ciphertext     = aes-256-cbc-enc(password, salt, plaintext)
authentication = HMAC-SHA256(password, ciphertext)

filter.crypt.clean  = cat ciphertext authentication
filter.crypt.smudge = HMAC-SHA256(password, head -c -64 authciphertext) == tail -c 64 authciphertext && aes-256-cbc-dec(password, head -c -64 authciphertext)

Assuming the hmac is in hex, and that OS X head actually supports a negative number of characters to indicate "all but the last N". Note that the verification check is not constant time, however, I don't think a timing attack to get the correct HMAC value is even a little bit plausible in this application.

  1. https://www.openssl.org/docs/crypto/EVP_BytesToKey.html#KEY_DERIVATION_ALGORITHM

@elasticdog
Copy link
Owner

Are you sure that there's no way you can put the encryption and decryption logic into separate bash files, rather than having a one-liner for each?

To make things less convoluted (things are okay now, but adding an HMAC would make it crazy) , I'm thinking about storing the encryption/decryption commands in a dedicated script <path-to-repo>/.git/crypt-helper and then the filter configs could just be:

[filter "crypt"]
    clean = $(git rev-parse --show-toplevel)/.git/crypt-helper clean
    smudge = $(git rev-parse --show-toplevel)/.git/crypt-helper smudge
[diff "crypt"]
    textconv = $(git rev-parse --show-toplevel)/.git/crypt-helper textconv

@sirlancelot
Copy link

👍 for crypt-helper script!

@elasticdog
Copy link
Owner

The helper scripts have been implemented...unfortunately, the change did reveal that the previous salt generation was not working as intended due to the nested quoting shenanigans (another good reason to simplify!), so this will update the encrypted files but should be functioning correctly now.

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

Successfully merging this pull request may close these issues.

None yet

4 participants