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

Vim cryptmethod uses SHA-256 for password-based key derivation #639

Open
atoponce opened this issue Feb 16, 2016 · 29 comments

Comments

Projects
None yet
7 participants
@atoponce
Copy link

commented Feb 16, 2016

On L421-L423 of src/blowfish.c, a sha256_key() function is created for password-based key derivation with a salt for blowfish. Unfortunately, even with 1,000 rounds, SHA-256 is designed to be fast, and can be parallelized with GPUs when brute forcing a file. Instead, the Blowfish key should be derived using bcrypt or scrypt. Both defeat parallelization on GPUs, and scrypt further defeats FPGAs.

@brammool

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

Aaron Toponce wrote:

On
L421-L423
of src/blowfish.c, a sha256_key() function is created for
password-based key derivation with a salt for blowfish. Unfortunately,
even with 1,000 rounds, SHA-256 is designed to be fast, and can be
parallelized with GPUs when brute forcing a file. Instead, the
Blowfish key should be derived using
bcrypt or
scrypt. Both defeat
parallelization on GPUs, and scrypt further defeats FPGAs.

There have been ideas for a new crypt mechanism that is much stronger
than Blowfish. Unfortunately nobody has implemented it yet. Note that
this requires experience and/or knowledge about encryption, it's easy to
make mistakes that weaken the encryption significantly.

Microsoft: "Windows NT 4.0 now has the same user-interface as Windows 95"
Windows 95: "Press CTRL-ALT-DEL to reboot"
Windows NT 4.0: "Press CTRL-ALT-DEL to login"

/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \
\ an exciting new programming language -- http://www.Zimbu.org ///
\ help me help AIDS victims -- http://ICCF-Holland.org ///

@tarcieri

This comment has been minimized.

Copy link

commented Feb 16, 2016

Note that unlike scrypt (or PBKDF2, or Argon2), bcrypt is not designed to be a KDF.

@atoponce

This comment has been minimized.

Copy link
Author

commented Feb 16, 2016

Note that unlike scrypt (or PBKDF2, or Argon2), bcrypt is not designed to be a KDF.

Indeed. Bad recommendation on my part, although I wouldn't recommend Argon2 quite yet either. Scrypt seems to be the most fitting here.

@DemiMarie

This comment has been minimized.

Copy link

commented Mar 19, 2016

Argon2 is implemented in libsodium and is the winner of the Password Hashing Competition. It is designed as a KDF.

However, note that the rest of Vim's cryptmethod is also poorly implemented. My suggestion is to use Argon2 as a KDF and either XSalsa20-Poly1305 or XChaCha20-Poly1305 (with a strong, random nonce) for authenticated encryption.

To guard against possible corruption on disk, it might be useful to apply an error-correcting code to the ciphertext + auth tag prior to writing on disc.

libsodium provides high-level APIs for password hashing and authenticated encryption and is my strong suggestion.

@brammool

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2016

Demetri Obenour wrote:

Argon2 is implemented in libsodium and is the winner of the
Password Hashing Competition. It is designed as a KDF.

However, note that the rest of Vim's cryptmethod is also poorly
implemented. My suggestion is to use Argon2 as a KDF and either
XSalsa20-Poly1305 or XChaCha20-Poly1305 (with a strong, random nonce)
for authenticated encryption.

libsodium provides high-level APIs for password hashing and
authenticated encryption and is my strong suggestion.

Libsodium looks interesting. The License is very liberal, it would
allow us to include the parts that we need in Vim. Although it appears
some individual files have a more restrictive license, need to check
each one.

This is C code, thus it should not be too difficult to add as a new
crypt algorithm in Vim.

From "know your smileys":
!-| I-am-a-Cylon-Centurian-with-one-red-eye-bouncing-back-and-forth

/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \
\ an exciting new programming language -- http://www.Zimbu.org ///
\ help me help AIDS victims -- http://ICCF-Holland.org ///

@tarcieri

This comment has been minimized.

Copy link

commented Mar 19, 2016

There's also TweetNaCl if you want something smaller to vendor, although you'd have to vendor Argon2 separately:

https://tweetnacl.cr.yp.to/software.html

@vim-ml

This comment has been minimized.

Copy link

commented Mar 21, 2016

On Saturday, March 19, 2016 at 1:43:30 PM UTC-5, Demetri Obenour wrote:

Argon2 is implemented in libsodium and is the winner of the Password Hashing Competition. It is designed as a KDF.

However, note that the rest of Vim's cryptmethod is also poorly implemented. My suggestion is to use Argon2 as a KDF and either XSalsa20-Poly1305 or XChaCha20-Poly1305 (with a strong, random nonce) for authenticated encryption.

libsodium provides high-level APIs for password hashing and authenticated encryption and is my strong suggestion.

My thoughts on this are:

It's best to use a library. With a library we're going to get any security updates from people who presumably know what they're doing and spend time learning about exploits so they can address them in their code. Also researchers are actually likely to test an external library, but not Vim's internal code.

However it's useful to have something available on every system Vim supports, so I think something should be baked in, still. Also Vim needs to be able to read old files.

I think adding a 'cryptkdf' option would be a decent idea. Include something built-in; either copied from a library with a license that allows it (probably best), or something simple like PDKDF2 (we already have a SHA256 we could use a a building block). But also include an option for a library implementation of a few algorithms. Argon2 is a good idea but it's still pretty new so if a flaw is found another method would probably be nice to have already available, so maybe the choices could be something like 'pdkdf2', 'lib-scrypt', and 'lib-argon2' for starters.

The number of iterations is key for the security of these algorithms. I'm vaguely aware that scrypt and argon2 are also configurable in memory use, but I know a lot less about that. But regardless we'll want to allow configuring the KDF to scale with hardware without recompiling Vim. So a 'cryptkdfcount' or similar option should also be provided. I think a good way to do this would be to allow both numeric values, but also time values. For example, the user could do ":set cryptkdfcount=1s" and Vim would estimate how many iterations would take a full second on the current hardware and use that. 1-3 seconds is probably a reasonable default value as well.

Speaking of defaults: I think Vim should default to the strongest method available. I additionally think Vim should warn on saving with a known broken format such as the original blowfish implementation, or the zip algorithm, or even blowfish2 without a decent KDF. Maybe even compile without the broken algorithms altogether unless the user specifically passes --include-bad-crypto to the configure script or something.

As for blowfish: I'm not very concerned about the weaknesses in the algorithm for the specific uses Vim is likely to use it for, however having a better encryption algorithm available is much nicer. Nevertheless, I am a little concerned whether we know the algorithm is implemented properly. I'd like to see output from Vim's implementation matched up against output from a well-known library implementation. In theory with the same salt, same IV, and same key, I think they should be compatible.

I should note here that I'm also a little uneasy with the current salt/IV generation. Basically it's just one system time (in microseconds) and a hash (I doubt the rand() adds anything useful). That could be a lot better, using /dev/urandom for example where supported, or the equivalent. Or maybe hash system time (still in microseconds) with a previous hash periodically, say while waiting for user input or something; I'm not familiar with best practices when a good system-provided random source is missing. I say I'm "uneasy" because I doubt there's any good exploit for this in Vim's use scenarios, but it's documented widely that a good unpredictable IV is required for CBC mode encryption, so I don't get any warm fuzzies from the "random" code here.

Bram, are you already working on any of this? I have been thinking about starting an implementation of some of the above but that's a lot of work I don't want going to waste if you have other ideas. Maybe hashing out a top-level interface and goals, then making a fork for more distributed development, would be a decent idea? I don't think any halfway implementations will be useful, especially since we'll probably need to grow a blowfish3 or other cryptmethod to use the new KDF (preferably in a forwards-compatible way to handle other new KDFs that may come along in the future).

@paragonie-scott

This comment has been minimized.

Copy link

commented Mar 21, 2016

(Slight nit: it's PBKDF2. The acronym stands for "Password-Based Key Derivation Function #2".)

@vim-ml

This comment has been minimized.

Copy link

commented Mar 21, 2016

On Monday, March 21, 2016 at 11:00:31 AM UTC-5, Ben Fritz wrote:

On Saturday, March 19, 2016 at 1:43:30 PM UTC-5, Demetri Obenour wrote:

Argon2 is implemented in libsodium and is the winner of the Password Hashing Competition. It is designed as a KDF.

However, note that the rest of Vim's cryptmethod is also poorly implemented. My suggestion is to use Argon2 as a KDF and either XSalsa20-Poly1305 or XChaCha20-Poly1305 (with a strong, random nonce) for authenticated encryption.

libsodium provides high-level APIs for password hashing and authenticated encryption and is my strong suggestion.

My thoughts on this are:

Oh, and I forgot one more:

While there may not be any exploitable weakness related to non-authenticated encryption in Vim's case, we can't prove it. And, if such an attack exists, Vim will be vulnerable. If we're already modifying the crypto code (and we should, because the KDF is way too weak) then we may as well throw in a MAC to prevent the possibility of a chosen-ciphertext attack. It shouldn't harm anything and it could potentially help a lot. I say we re-open the issue that got created for non-authenticated encryption.

@vim-ml

This comment has been minimized.

Copy link

commented Mar 21, 2016

On Monday, March 21, 2016 at 11:04:15 AM UTC-5, Scott wrote:

(Slight nit: it's PBKDF2. The acronym stands for "Password-Based Key Derivation Function #2".)

Thanks, I knew that, I just was typing faster than I was thinking apparently. That's one of those acronyms I really need to expand out in my head before typing correctly. I do the same thing with IMDB which occasionally lands me on some interesting cyber-squatter pages. :-(

@vim-ml

This comment has been minimized.

Copy link

commented Mar 23, 2016

Ben Fritz wrote:

On Saturday, March 19, 2016 at 1:43:30 PM UTC-5, Demetri Obenour wrote:

Argon2 is implemented in libsodium and is the winner of the Password Hashing Competition. It is designed as a KDF.

However, note that the rest of Vim's cryptmethod is also poorly implemented. My suggestion is to use Argon2 as a KDF and either XSalsa20-Poly1305 or XChaCha20-Poly1305 (with a strong, random nonce) for authenticated encryption.

libsodium provides high-level APIs for password hashing and authenticated encryption and is my strong suggestion.

My thoughts on this are:

It's best to use a library. With a library we're going to get any
security updates from people who presumably know what they're doing
and spend time learning about exploits so they can address them in
their code. Also researchers are actually likely to test an external
library, but not Vim's internal code.

libsodium is currently at version 1.0.8. The version included with
Ubuntu is 1.0.3 ...

However it's useful to have something available on every system Vim
supports, so I think something should be baked in, still. Also Vim
needs to be able to read old files.

We could use configure to find a library and when it's not available or
older fall back to the built-in code.

Speaking of defaults: I think Vim should default to the strongest
method available. I additionally think Vim should warn on saving with
a known broken format such as the original blowfish implementation, or
the zip algorithm, or even blowfish2 without a decent KDF. Maybe even
compile without the broken algorithms altogether unless the user
specifically passes --include-bad-crypto to the configure script or
something.

This has the danger of writing a file on one system, go on holiday and
find out you can't open it on your laptop (that actually happened to me).

I think there should be some number of months between making a new
method available and making it the default.

The original blowfish encryption is not broken, it's just weaker than it
should be. It's still a lot stronger than zip.

Bram, are you already working on any of this? I have been thinking
about starting an implementation of some of the above but that's a lot
of work I don't want going to waste if you have other ideas. Maybe
hashing out a top-level interface and goals, then making a fork for
more distributed development, would be a decent idea? I don't think
any halfway implementations will be useful, especially since we'll
probably need to grow a blowfish3 or other cryptmethod to use the new
KDF (preferably in a forwards-compatible way to handle other new KDFs
that may come along in the future).

No, several people have given their opinion but nobody has done actual
work...

Laughing helps. It's like jogging on the inside.

/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \
\ an exciting new programming language -- http://www.Zimbu.org ///
\ help me help AIDS victims -- http://ICCF-Holland.org ///

@vim-ml

This comment has been minimized.

Copy link

commented Mar 23, 2016

On Wed, Mar 23, 2016 at 4:58 PM, Bram Moolenaar Bram@moolenaar.net wrote:

Speaking of defaults: I think Vim should default to the strongest
method available. I additionally think Vim should warn on saving with
a known broken format such as the original blowfish implementation, or
the zip algorithm, or even blowfish2 without a decent KDF. Maybe even
compile without the broken algorithms altogether unless the user
specifically passes --include-bad-crypto to the configure script or
something.

This has the danger of writing a file on one system, go on holiday and
find out you can't open it on your laptop (that actually happened to me).

That makes some sense, however it only applies to people who edit the same
file on multiple systems, AND they don't have the same version of Vim on
each of those systems.

If you don't need that capability, having the old broken systems still in
the code makes it too easy to make a mistake that could compromise your
security.

DROWN recently showed some of the problems with keeping older known-broken
crypto code enabled by default. I think it makes sense to keep the code
there for people who deliberately choose to use it. But getting rid of it
by default can potentially remove an attack vector.

At the very least, we should warn when saving in an insecure format.

I think there should be some number of months between making a new
method available and making it the default.

OK, that's fair.

The original blowfish encryption is not broken, it's just weaker than it
should be. It's still a lot stronger than zip.

Is it? This page makes it sound like "blowfish" was pretty much completely
broken if you knew any of the plaintext: https://dgl.cx/2014/10/vim-blowfish

For example, if you had plugin that always writes a predictable header text
to an encrypted file before the actual sensitive data, the attacker would
know some plaintext. I'm certainly not comfortable using "blowfish",
knowing it had exploitable flaws fixed in blowfish2. I thought "blowfish"
was just around to let people read their old data (and hopefully convert to
blowfish2).

And while I can probably personally use a strong-enough passphrase to let
the current too-fast KDF for blowfish2 suffice, I wouldn't recommend it to
anyone else, since I know most people choose passwords that can fall way
too fast with modern tools and techniques. I'd consider "blowfish2" to be
broken for general use as well since you need a REALLY good password for
it to provide any long-term security guarantees. Once we increase the KDF
iterations sufficiently I would warn when saving in blowfish2 as well, in
favor of the new method(s) using a better KDF.

@vim-ml

This comment has been minimized.

Copy link

commented Mar 23, 2016

On Wed, Mar 23, 2016 at 5:51 PM, Benjamin Fritz fritzophrenic@gmail.com
wrote:

I thought "blowfish" was just around to let people read their old data
(and hopefully convert to blowfish2).

That reminds me of something else. Why isn't 'modified' set when you change
cryptmethod or the encryption password?

@vim-ml

This comment has been minimized.

Copy link

commented Mar 24, 2016

Ben Fritz wrote:

On Wed, Mar 23, 2016 at 4:58 PM, Bram Moolenaar Bram@moolenaar.net wrote:

Speaking of defaults: I think Vim should default to the strongest
method available. I additionally think Vim should warn on saving with
a known broken format such as the original blowfish implementation, or
the zip algorithm, or even blowfish2 without a decent KDF. Maybe even
compile without the broken algorithms altogether unless the user
specifically passes --include-bad-crypto to the configure script or
something.

This has the danger of writing a file on one system, go on holiday and
find out you can't open it on your laptop (that actually happened to me).

That makes some sense, however it only applies to people who edit the same
file on multiple systems, AND they don't have the same version of Vim on
each of those systems.

If you don't need that capability, having the old broken systems still in
the code makes it too easy to make a mistake that could compromise your
security.

DROWN recently showed some of the problems with keeping older known-broken
crypto code enabled by default. I think it makes sense to keep the code
there for people who deliberately choose to use it. But getting rid of it
by default can potentially remove an attack vector.

At the very least, we should warn when saving in an insecure format.

I think there should be some number of months between making a new
method available and making it the default.

OK, that's fair.

The original blowfish encryption is not broken, it's just weaker than it
should be. It's still a lot stronger than zip.

Is it? This page makes it sound like "blowfish" was pretty much completely
broken if you knew any of the plaintext: https://dgl.cx/2014/10/vim-blowfish

Your definition of broken is wrong. Broken means it doesn't work at
all. e.g., Vim crashes when using it, or when decrypting you can't get
back the original text. When do you call a car broken? When you can't
drive. Not when you can't open the window.

For example, if you had plugin that always writes a predictable header text
to an encrypted file before the actual sensitive data, the attacker would
know some plaintext. I'm certainly not comfortable using "blowfish",
knowing it had exploitable flaws fixed in blowfish2. I thought "blowfish"
was just around to let people read their old data (and hopefully convert to
blowfish2).

And while I can probably personally use a strong-enough passphrase to let
the current too-fast KDF for blowfish2 suffice, I wouldn't recommend it to
anyone else, since I know most people choose passwords that can fall way
too fast with modern tools and techniques. I'd consider "blowfish2" to be
broken for general use as well since you need a REALLY good password for
it to provide any long-term security guarantees. Once we increase the KDF
iterations sufficiently I would warn when saving in blowfish2 as well, in
favor of the new method(s) using a better KDF.

My favorite example is when I have some text that I don't want my
neighbor to read. Any encryption that Vim provides works for that.

Also keep in mind that, no matter how strong your encryption is, there
is always a weak point. Rembember key loggers? There never ever is
100% reliable encryption.

So please don't overreact.

hundred-and-one symptoms of being an internet addict:
107. When using your phone you forget that you don't have to use your
keyboard.

/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \
\ an exciting new programming language -- http://www.Zimbu.org ///
\ help me help AIDS victims -- http://ICCF-Holland.org ///

@paragonie-scott

This comment has been minimized.

Copy link

commented Mar 24, 2016

Your definition of broken is wrong. Broken means it doesn't work at all.

Not quite. Think "broken" in the sense of "code-breaker". Is it easy for a skilled analyst to subvert? What about a large team of skilled analysts and mathematicians with billions of dollars of specialized computer hardware?

@vim-ml

This comment has been minimized.

Copy link

commented Mar 24, 2016

On Thu, Mar 24, 2016 at 6:08 AM, Bram Moolenaar Bram@moolenaar.net wrote:

Ben Fritz wrote:

On Wed, Mar 23, 2016 at 4:58 PM, Bram Moolenaar Bram@moolenaar.net
wrote:

The original blowfish encryption is not broken, it's just weaker than
it
should be. It's still a lot stronger than zip.

Is it? This page makes it sound like "blowfish" was pretty much
completely
broken if you knew any of the plaintext:
https://dgl.cx/2014/10/vim-blowfish

Your definition of broken is wrong. Broken means it doesn't work at
all. e.g., Vim crashes when using it, or when decrypting you can't get
back the original text. When do you call a car broken? When you can't
drive. Not when you can't open the window.

I call something "broken" when it cannot serve its intended purpose.
Cryptography's purpose is to keep data secret. If that article is correct,
then with "blowfish" (not "blowfish2") there is a trivial attack that can
expose at least 64 characters of text in a file without ever knowing the
password. I'm not clear on the details (the article hand-waves a bit) but
potentially the rest of the file may also be recoverable.

If the encrypted file is the basis of a password manager for example, then
this is quite bad. At least the first password is probably compromised if
an attacker gains access to the file.

If anything, this makes "blowfish" worse than zip in many scenarios. At
least with zip the attacker needs to work for it.

For example, if you had plugin that always writes a predictable header
text
to an encrypted file before the actual sensitive data, the attacker
would
know some plaintext. I'm certainly not comfortable using "blowfish",
knowing it had exploitable flaws fixed in blowfish2. I thought
"blowfish"
was just around to let people read their old data (and hopefully
convert to
blowfish2).

And while I can probably personally use a strong-enough passphrase to
let
the current too-fast KDF for blowfish2 suffice, I wouldn't recommend it
to
anyone else, since I know most people choose passwords that can fall way
too fast with modern tools and techniques. I'd consider "blowfish2" to
be
broken for general use as well since you need a REALLY good password
for
it to provide any long-term security guarantees. Once we increase the
KDF
iterations sufficiently I would warn when saving in blowfish2 as well,
in
favor of the new method(s) using a better KDF.

My favorite example is when I have some text that I don't want my
neighbor to read. Any encryption that Vim provides works for that.

That's a straw-man argument. You could also do ggg?G to rot13 the buffer
which would keep my neighbor or my kids from reading the file. If that's
not enough then a simple plugin to do a Caesar cipher as a
BufWritePre/BufRead would also do the trick. But nobody would seriously
suggest either of those are secure. Encryption must have a higher standard
than keeping out your non-hacker friends.

The help entry blowfish and blowfish2 both say "medium strong encryption".
An "implementation flaw" is mentioned for blowfish, but IIUC the flaw is
severe enough to make it much, much weaker than blowfish2. Why are they
both summarized as the same strength?

Also keep in mind that, no matter how strong your encryption is, there
is always a weak point. Rembember key loggers? There never ever is
100% reliable encryption.

So please don't overreact.

This is true. Neither Vim nor any other encryption tool will protect from a
compromised system. I don't expect Vim to keep me safe from everything. But
I do expect, if I encrypt a file with any modern crypto tool, that I don't
need to worry if that file is snagged off my cloud storage, or someone
steals my USB stick, or something. Representing a crypto implementation as
"secure" when it is trivial to recover some number of bytes in a file is
going to leak someone's sensitive data because they trusted you. I don't
think it's overreacting to say "we shouldn't present weak crypto as strong
enough to keep using".

Maybe "broken" is too changed a word, I'll stop using it. I'll put it this
way:

zip - insecure, all of your file can be recovered with commonly available
tools. Not recommended.
blowfish - insecure, part or all of your file can be recovered with known
methods. Not recommended.
blowfish2 - secure for small files if you use a very strong password and
nobody else has write access.
proposed new methods - secure if you don't use a very weak password like
"123456", "password", or "letmein".

@vim-ml

This comment has been minimized.

Copy link

commented Mar 24, 2016

On Thu, Mar 24, 2016 at 9:54 AM, Benjamin Fritz fritzophrenic@gmail.com
wrote:

The help entry blowfish and blowfish2 both say "medium strong
encryption". An "implementation flaw" is mentioned for blowfish, but IIUC
the flaw is severe enough to make it much, much weaker than blowfish2. Why
are they both summarized as the same strength?

Ah, I see some stronger warnings in :help encrypt. I think those should be
emphasized more and maybe cross-referenced in the help for 'cryptmethod'.

@brammool

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

Scott wrote:

Your definition of broken is wrong. Broken means it doesn't work at all.

Not quite. Think "broken" in the sense of "code-breaker". Is it easy
for a skilled analyst to subvert? What about a large team of skilled
analysts and mathematicians with billions of dollars of specialized
computer hardware?

It's more like breaking in. Or breakable. That skilled person doesn't
break the encryption, it finds the password. For the next password you
have to start over. English can be quite confusing.

hundred-and-one symptoms of being an internet addict:
114. You are counting items, you go "0,1,2,3,4,5,6,7,8,9,A,B,C,D...".

/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \
\ an exciting new programming language -- http://www.Zimbu.org ///
\ help me help AIDS victims -- http://ICCF-Holland.org ///

@paragonie-scott

This comment has been minimized.

Copy link

commented Mar 24, 2016

@brammool

It's more like breaking in. Or breakable. That skilled person doesn't
break the encryption, it finds the password. For the next password you
have to start over. English can be quite confusing.

Let's also consider the case of, say, the recent compression oracle against iMessage attachments. You don't "figure out the password", you grab an encrypted message, alter it, send it, and look for errors in the response. Keep doing this (the paper estimated 2^18 messages) until you have the original plaintext message.

In a similar vein, Vaudenay's CBC mode padding oracle.

The consequence of these attacks? Cryptographers say "unauthenticated encryption is broken". Someone else responds: "No it's not; look, it still works".

@tomrist

This comment has been minimized.

Copy link

commented Jun 8, 2016

Few thoughts from a cryptographer (https://rist.tech.cornell.edu). I use vim as my primary editor for everything (thanks for all the great work on it!). I wanted to know if there was built in password-based encryption for exactly the Dropbox use case mentioned on the thread and lead me eventually to this here.

  1. Modern authenticated encryption is now universally thought by cryptographers to be necessary for secure encryption. Encrypt-then-MAC (e.g., CTR mode followed by HMAC) is a good way to go, or an all-in-one scheme like GCM or OCB. Using an authenticated encryption scheme provides resistance against any active attacks that manipulate ciphertexts. This is in my opinion clearly in the threat model for untrusted remote storage (such as Dropbox) --- I for one would not want a malicious insider at Dropbox to be able to undetectably modify at will what my encrypted text document contains.

  2. Reiterating something from earlier: Looking at the existing code, the randomness used to generate salts and seeds (usually called IVs by cryptographers) does indeed look very weak. Really this should be replaced as indicated above by a call to a system RNG. If no good system RNG is available, probably safest to just disallow use of encryption (decryption would still be ok). It is a pain to integrate portable RNG code, as Windows, BSD, and Linux all do different things and direct reads from devices like /dev/urandom can introduce race conditions, but it is possible to do well and several other libraries out there have done it. This should be a relatively modest change to sha2_seed() given the RNG logic. By the way, the sha2_seed() function has a latent bug unless I am misunderstanding something, as it repeats random data when 32 < salt_len or 32 < header_len. One shouldn't be repeating random data if it is to be used with crypto algorithms. Doesn't appear to currently be an issue since largest salt_len and header_len is 8 bytes, but an 8 byte salt is too small for security these days even if one has a good RNG --- I'd increase it to 16 or 32.

  3. I started playing with the code to understand how hard it would be to build in a good authenticated encryption scheme. Right now I have an Encrypt-then-MAC module built that supports authenticated encryption using AES CTR mode and HMAC-SHA256. But this doesn't seem to work for anything but the main file encryption, as there are several places where encryption is assumed to be length-preserving. (Note that authenticated encryption always means that ciphertexts are longer than decryption, by a fixed number of bytes dependent on mode. For my example it is 32 bytes extra.) These calls are (at least) in undo.c and memline.c, and would need to be modified to support authenticated encryption of (I believe) the undo and swap data. I don't understand the implementation of these modules so not sure how invasive a change this will be. Right now I have my toy implementation default to just using CTR mode for these calls and things seem to work though obviously with weaker guarantees for those files. (I haven't done extensive testing.)

  4. Side point: the messages one gets when recovering from an encrypted swap file is really confusing. I think I may understand the issue, but this would actually be dealt with in a seemingly more user friendly way if one used authenticated encryption since entering the wrong password would be detectable.

  5. The issue of validating crypto implementations is an important one, but this should be easy to do with standard test vectors for low-level primitives like AES and SHA-256 (NIST gives out some, for example). The whole encryption format would do well to be documented as well, though, so one could, e.g., implement a separate utility that just decrypts vim encrypted files.

Happy to help someone with developing a good solution. I think having a built in strong password-based encryption would be a very nice feature for vim, and one that I would use.

@DemiMarie

This comment has been minimized.

Copy link

commented Jun 9, 2016

Some more thoughts:

  • My personal preference is to use a Git submodule to bundle libsodium as a dependency and statically link to it. This avoids all issues relating to libsodium not being present on the user's system. Distributions that provide a recent enough libsodium can substitute system libsodium with a configure switch. Substituting a too-old version will result in a compile-time error.
  • libsodium provides high-level APIs, so this should be easy. My preference (assuming that we only need to worry about symmetric-key encryption) is the ChaCha20-Poly1305 AEAD, with Argon2 for password-based key derivation. libsodium also provides a portable CSPRNG that gets entropy from the operating system; this solves the portability problems. It even works around bugs such as Linux /dev/urandom not blocking when not seeded.
  • Argon2 requires (by design) a large amount of RAM. This is a security feature, because high-speed ASIC password crackers are often limited by available on-chip high-speed storage. libsodium offers no way to interrupt Argon2 asynchronously. Therefore, it is best to run Argon2 in a subprocess that is given the password, salt, and other parameters over stdin (in binary form) and returns the hash on stdout (again in binary form). The subprocess can be killed with a SIGKILL on Unix or TerminateProcess on Windows in the event of a timeout, with no risk of leaking resources (since it owns none other than memory).
  • As I understand it, the salt doesn't need to be separately authenticated, in that using a different salt will produce a wrong key (and thus authentication failure) with overwhelming probability. But it is best if it is authenticated, for conservatism – hence the use of the ChaCha20-Poly1305 AEAD.
  • Each time a file is encrypted, a fresh salt must be generated uniformly at random. Therefor, the ChaCha20-Poly1305 nonce can safely be hardcoded to 0. But it would be better if it is generated as part of the key.
  • Do not compress the file. This leaks information if part of the file is attacker-controlled.
  • The Poly1305 MAC is a polynomial authenticator, and its strength is inversely proportional to message length – but I don't think this is an issue (for 1TB files, the forgery probability is 2^-66)
@tomrist

This comment has been minimized.

Copy link

commented Jun 14, 2016

I think using a library wouldn't be a bad way to go, and libsodium seems quite reasonable. Choosing ChaCha20-Poly1305 wouldn't enable use of AES-NI, available on most desktop platforms, which would have a negative performance penalty.

Would it make sense to try to upstream to libsodium a proper password-based encryption mechanism so all of the crypto appears in the library? I.e., if we go with your suggestion of argon2 + an AEAD then we build that into libsodium as opposed to having to do it within vim. A couple reasons being that (1) people can analyze it within libsodium and provide updates there; and (2) if libsodium gets used elsewhere then the password-based encryption will be the same. (I could for example build a simple utility to just decrypt version XX vim-encrypted files using libsodium just by parsing the header and throwing it at libsodium.)

For the point about using a subprocess, you are just worried about the responsiveness of the UI while the encryption key is derived? Or stability in case of memory errors? Unless we set very high security parameters on argon2 I'd hope a synchronous solution would be responsive on most platforms in tens to hundreds of milliseconds. Not sure what portability implications this has, and we could potentially allow expose configurability of argon2 within vim while choosing a reasonable and secure default.

The salt does not need to be included in the authentication scope but it shouldn't matter if it is included. (This is exactly the type of design decision that would be best left to a cryptographic library, rather than having application developers have to make the decision, if at all possible.)

Compression indeed should be avoided.

In any case we'll still need to deal with the fact that the functions calling out to encryption are forcing a length-preserving encryption.

@tarcieri

This comment has been minimized.

Copy link

commented Jun 14, 2016

@tomrist generally I don't think @jedisct1 likes incorporating complicated high-level protocols like the one you're describing into libsodium

@paragonie-scott

This comment has been minimized.

Copy link

commented Jun 14, 2016

// 1. Derive a key from a password and salt:
if (crypto_pwhash(key, crypto_secretbox_KEYBYTES, password, strlen(password), salt,
                  crypto_pwhash_OPSLIMIT_INTERACTIVE, crypto_pwhash_MEMLIMIT_INTERACTIVE,
                  crypto_pwhash_ALG_DEFAULT) != 0) {
    // Out of memory! Error
}

// 2. Encrypt
randombytes_buf(nonce, sizeof nonce);
crypto_secretbox_easy(ciphertext, MESSAGE, MESSAGE_LEN, nonce, key);

// Then write the ciphertext and nonce to the file, and persist the salt elsewhere
// or make it part of the ciphertext.

I mean, there's not a heck of a lot that needs to be done for the actual encryption itself. Using libsodium takes care of 99% of the problem; the rest is deciding on message formats and writing very little wrapper code.

@tarcieri

This comment has been minimized.

Copy link

commented Jun 14, 2016

Choosing ChaCha20-Poly1305 wouldn't enable use of AES-NI, available on most desktop platforms, which would have a negative performance penalty.

Yes, using ChaCha20-Poly1305 cuts you out of the blazing fast speed of AES-NI, but the use case here isn't 10GbE line rate encryption or full disk encryption, it's generally going to be encrypting reasonably small text files.

ChaCha20-Poly1305 is still quite fast in software, and also has a robust software implementation which works portably practically everywhere.

Unless you can leverage fixed-function AES implementations like AES-NI on Intel or the ARMv8 (or other proprietary ARM) implementations, you're left with software implementations of AES, which are notorious for side-channels in e.g. table lookups, and generally require architecture-specific implementations to be implemented in a sidechannel-free manner.

For those reasons, I think ChaCha20-Poly1305 is the best highly portable, one-size-fits-all solution.

@paragonie-scott

This comment has been minimized.

Copy link

commented Jun 14, 2016

ChaCha20-Poly1305 gets my vote as well. Best choice for both speed and side-channel-resistance without specialized hardware, which is what software should in general prioritize.

@atoponce

This comment has been minimized.

Copy link
Author

commented Jun 14, 2016

As a reminder, this bug is about the KDF using SHA-256 with 1k rounds, where bcrypt would be a better fit. #638 is the ticket for on-disk file encryption, where ChaCha20-Poly1305 would be a better fit than Blowfish. Perhaps @brammool can reopen #638, and we could move the on-disk encryption discussion there.

However, libsodium does ship Argon2 as a KDF, which was recently the Password Hashing Contest winner. So, standardizing on libsodium for both #638 and #639 could kill two birds with one stone. And it could be a great place for more analysis on Argon2 as a KDF.

@brammool Would you mind re-opening #638 to engage on-disk encryption discussion there? I feel it might have been closed prematurely due to some charged emotions, when in reality, we're just trying to help improve Vim.

Thanks.

@tarcieri

This comment has been minimized.

Copy link

commented Jun 15, 2016

@atoponce yeah, +1 re: on-disk encryption. Are swap files and .viminfo properly encrypted? (I guess #638 is the place to follow up, or does that deserve a new ticket?)

@tomrist

This comment has been minimized.

Copy link

commented Jun 15, 2016

I mean, there's not a heck of a lot that needs to be done for the actual encryption itself. Using libsodium takes care of 99% of the problem; the rest is deciding on message formats and writing very little wrapper code.

Yeah that's true, it's only a few lines of code, but they are security critical and I wouldn't be surprised if people misuse the API. My hope was to help that this might encourage easier compatibility with other tools by pushing upstream --- could be naive. In any case it was just an idea for food for thought, and as @tarcieri says libsodium may not like it anyway.

@tarcieri : yes the side-channel free difficulty of AES in software is a huge detractor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.