[vim/vim] Encryption: Provide better alternative using libsodium for encryption (#8394)
Christian Brabandt
After the issues #638 and #639 I thought about exploring alternatives to
make the crypt code more modern and robust. So for the last couple of
weeks and spend my spare time exploring things and improving my crypt
skills ;)
One of the mentioned alternatives was
libsodium, which is a
library providing encryption, hashing and other cryptographic features
for end-users. Fortunately it is cross plattform so can be used across a
variety of platforms.
This is currently work in progress, it compiles and seems to work for
me, but is not yet considered stable. Also, I am currently not sure, the
changes to memline/memfile are actually correct. I don't really that
low-level code of the data blocks :/
The reason I am posting it here is, that I'd like to get feedback about
the current scope.
In addition I am not so sure, how to properly hook into encrypting the
blocks for swapfiles and undo-files. For stream-encoding, libsodium
needs to know, when a stream of messages is completed and adds an
additional tag for verification. However given the current design of the
block-writing code, I am not sure how to detect whether the current call
for encrypting will be the last one. So I would like to have some
guidance here.
For now, I have just sprinkled a couple of static TRUE and FALSE for the
swap file encryption and for undo-file encryption I just disabled
complete encryption.
Also the current read-file and write-file end up using different block
sizes, so I made some slight changes to the read-file code, to ensure,
that blocks of 8K will be read to make sure that the crypt layer can read
the same block size when reading as when writing. Not sure if there is a
better way to achieve this.
And of course, I'd like to get some CI builds done :)
I'll have a look at the windows changes, once I get my windows build
system ready. Not sure how much work this requires.
Todo:
- fix error numbers
- write tests
- make changes to the windows build system
- enable complete undofile encryption (?)
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/8394
Commit Summary
- Encryption: Provide alternative using libsodium
- some last minute changes and cleanup
File Changes
- M runtime/doc/eval.txt (1)
- M runtime/doc/options.txt (12)
- M runtime/doc/various.txt (1)
- M src/auto/configure (66)
- M src/blowfish.c (7)
- M src/bufwrite.c (18)
- M src/config.h.in (1)
- M src/configure.ac (37)
- M src/crypt.c (316)
- M src/crypt_zip.c (6)
- M src/evalfunc.c (7)
- M src/feature.h (7)
- M src/fileio.c (28)
- M src/memline.c (35)
- M src/optionstr.c (2)
- M src/proto/blowfish.pro (4)
- M src/proto/crypt.pro (14)
- M src/proto/crypt_zip.pro (4)
- M src/structs.h (5)
- M src/undo.c (18)
- M src/version.c (5)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
codecov[bot]
Codecov Report
Merging #8394 (4bbf846) into master (b90ac5e) will decrease coverage by
87.30%.
The diff coverage is0.00%.
❗ Current head 4bbf846 differs from pull request most recent head 5329dd3. Consider uploading reports for the commit 5329dd3 to get more accurate results
@@ Coverage Diff @@ ## master #8394 +/- ## =========================================== - Coverage 89.81% 2.50% -87.31% =========================================== Files 149 147 -2 Lines 167465 162369 -5096 =========================================== - Hits 150407 4068 -146339 - Misses 17058 158301 +141243
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | ? |
|
| huge-gcc-none | ? |
|
| huge-gcc-testgui | ? |
|
| huge-gcc-unittests | 2.50% <0.00%> (-0.01%) |
⬇️ |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/blowfish.c | 0.00% <ø> (-87.61%) |
⬇️ |
| src/bufwrite.c | 0.00% <0.00%> (-84.77%) |
⬇️ |
| src/crypt.c | 0.00% <0.00%> (-93.20%) |
⬇️ |
| src/crypt_zip.c | 0.00% <ø> (-97.06%) |
⬇️ |
| src/evalfunc.c | 0.00% <ø> (-96.33%) |
⬇️ |
| src/fileio.c | 0.00% <0.00%> (-84.99%) |
⬇️ |
| src/memline.c | 0.00% <0.00%> (-88.82%) |
⬇️ |
| src/optionstr.c | 21.55% <ø> (-73.50%) |
⬇️ |
| src/register.c | 0.21% <ø> (-91.81%) |
⬇️ |
| src/undo.c | 0.00% <0.00%> (-84.58%) |
⬇️ |
| ... and 146 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update b90ac5e...5329dd3. Read the comment docs.
Christian Brabandt
Oh and for development I have used libsodium-dev version 1.0.18, which is from january 2019 and readily available in my ubuntu box.
Bram Moolenaar
Christian wrote:
> After the issues #638 and #639 I thought about exploring alternatives to
> make the crypt code more modern and robust. So for the last couple of
> weeks and spend my spare time exploring things and improving my crypt
> skills ;)
>
> One of the mentioned alternatives was
> library providing encryption, hashing and other cryptographic features
> for end-users. Fortunately it is cross plattform so can be used across a
> variety of platforms.
>
> This is currently work in progress, it compiles and seems to work for
> me, but is not yet considered stable. Also, I am currently not sure, the
> low-level code of the data blocks :/
>
> the current scope.
>
> In addition I am not so sure, how to properly hook into encrypting the
> blocks for swapfiles and undo-files. For stream-encoding, libsodium
> needs to know, when a stream of messages is completed and adds an
> additional tag for verification. However given the current design of the
> block-writing code, I am not sure how to detect whether the current call
> for encrypting will be the last one. So I would like to have some
> guidance here.
>
> For now, I have just sprinkled a couple of static TRUE and FALSE for the
> swap file encryption and for undo-file encryption I just disabled
> complete encryption.
>
> Also the current read-file and write-file end up using different block
> sizes, so I made some slight changes to the read-file code, to ensure,
> that blocks of 8K will be read to make sure that the crypt layer can read
> the same block size when reading as when writing. Not sure if there is a
> better way to achieve this.
>
>
> I'll have a look at the windows changes, once I get my windows build
> system ready. Not sure how much work this requires.
>
> Todo:
>
> - [ ] write tests
> - [ ] make changes to the windows build system
> - [ ] enable complete undofile encryption (?)
Thanks for taking the time to dive into this.
The swap file implementation really expects fixed size blocks. Changing
that is going to be very complicated. How about disabling the swap file
when using this type of encryption? This also avoids making the
encryption weaker, since each block is encrypted separately.
I'm afraid I won't have time to look into the details. We should have
several people check that the way the crypt library is used doesn't open
up any holes or weakens the crypt mechanism.
--
The average life of an organization chart is six months. You can safely
ignore any order from your boss that would take six months to complete.
(Scott Adams - The Dilbert principle)
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Aaron Toponce
I see the Blowfish -> XChaCha20-Poly1305 change fixing #638, but I don't see a KDF replacement for sha256_key() in blowfish.c fixing #639. You've imported libsodium, so you can take advantage of the crypto_pwhash_* API which uses Argon2id, or crypto_pwhash_scryptsalsa208sha256_* which uses Scrypt.
Christian Brabandt
Yes, blowfish is kept separate. But the XChaCha20-Poly1305 uses the crypt_pwhash API and it also uses the randombytes API for the salt and the seed.
I did not want to touch the blowfish code for now, as I don't know, if this is backwards compatible (e.g. if this will can still decrypt existing blowfish encrypted files (swap files, undofile, files))
Christian Brabandt
okay, let's disable the swapfile encryption. I also made a couple of other small adjustments (such as proper key derivation). Concrete further suggestions welcome!
Bram Moolenaar
Christian wrote:
> Yes, blowfish is kept separate. But the `XChaCha20-Poly1305` uses the
> `crypt_pwhash` API and it also uses the `randombytes` API for the salt
> and the seed.
>
> I did not want to touch the blowfish code for now, as I don't know, if
> this is backwards compatible (e.g. if this will can still decrypt
> existing blowfish encrypted files (swap files, undofile, files))
other than reading existing encrypted files? Probably not, thus
changing the blowfish crypt is not helping.
--
Often you're less important than your furniture. If you think about it, you
can get fired but your furniture stays behind, gainfully employed at the
company that didn't need _you_ anymore.
Bram Moolenaar
Christian wrote:
> okay, let's disable the swapfile encryption. I also made a couple of
> other small adjustments (such as proper key derivation). Concrete
> further suggestions welcome!
writing the swapfile (accidentally) leaks the text, thus I think we
should make it impossible to enable the swapfile.
The swapfile is temporary, thus some people may think it's OK. But
these days with SSD and other types of disks deleting a file doesn't
mean the text is gone. Even reformatting the disk may not help.
--
A)bort, R)etry, B)ang it with a large hammer
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Thaddée Tyl
@espadrine commented on this pull request.
> @@ -2384,8 +2384,18 @@ A jump table for the options with a short description can be found at |Q_op|. you write the file the encrypted bytes will be different. The whole undo file is encrypted, not just the pieces of text. - - You should use "blowfish2", also to re-encrypt older files. + xchacha20 XChaCha20 Cipher with Poly1305 Message Authentication + Code. Medium strong till strong encryption. + Encryption is provided by the libsodium library, so it + requires Vim to be build with |+sodium|⬇️ Suggested change
- requires Vim to be build with |+sodium| + requires Vim to be built with |+sodium|
Thaddée Tyl
> @@ -2384,8 +2384,18 @@ A jump table for the options with a short description can be found at |Q_op|. you write the file the encrypted bytes will be different. The whole undo file is encrypted, not just the pieces of text. - - You should use "blowfish2", also to re-encrypt older files. + xchacha20 XChaCha20 Cipher with Poly1305 Message Authentication + Code. Medium strong till strong encryption. + Encryption is provided by the libsodium library, so it + requires Vim to be build with |+sodium| + It adds a seed and a message authentication code (MAC) + to the file. Only the text in the undo-file is + encrypted. This needs at least a Vim 8.XXXX to read + such a file. Encryption of Swapfiles not supported, + therefore swap files will be disabled. + Currently experiemental.⬇️ Suggested change
- Currently experiemental. + Currently experimental.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -2384,8 +2384,18 @@ A jump table for the options with a short description can be found at |Q_op|. you write the file the encrypted bytes will be different. The whole undo file is encrypted, not just the pieces of text. - - You should use "blowfish2", also to re-encrypt older files. + xchacha20 XChaCha20 Cipher with Poly1305 Message Authentication + Code. Medium strong till strong encryption. + Encryption is provided by the libsodium library, so it + requires Vim to be build with |+sodium|
thanks!
Christian Brabandt
> @@ -2384,8 +2384,18 @@ A jump table for the options with a short description can be found at |Q_op|. you write the file the encrypted bytes will be different. The whole undo file is encrypted, not just the pieces of text. - - You should use "blowfish2", also to re-encrypt older files. + xchacha20 XChaCha20 Cipher with Poly1305 Message Authentication + Code. Medium strong till strong encryption. + Encryption is provided by the libsodium library, so it + requires Vim to be build with |+sodium| + It adds a seed and a message authentication code (MAC) + to the file. Only the text in the undo-file is + encrypted. This needs at least a Vim 8.XXXX to read + such a file. Encryption of Swapfiles not supported, + therefore swap files will be disabled. + Currently experiemental.
will be fixed on next push
Christian Brabandt
I think you mean "disable the swapfile". When using strong encryption
Yes of course.
writing the swapfile (accidentally) leaks the text, thus I think we
should make it impossible to enable the swapfile.
I fixed a couple of more problems to make sure Vim closes the swapfile
and won't try to encrypt it. I also fixed a couple of edge cases that come from
the fact, that since the new crypt code allocates a new buffer, converting
could cause a buffer-overflow. Also properly free the allocated buffer.
The swapfile is temporary, thus some people may think it's OK. But
these days with SSD and other types of disks deleting a file doesn't
mean the text is gone. Even reformatting the disk may not help.
I also disabled the undofile. I thought it worked before, but when writing the tests,
I got a couple of overflows, most likely, because the size was too small.
I added some tests to ensure the buffer size for en/decoding is large enough.
Tests should be there in a minute , once I push. They probably won't yet run,
I need to make changes to the CI, so that vim is build with libsodium.
Christian Brabandt
@chrisbra pushed 16 commits.
- 1a65701 Use proper Vim style for braces
- 0c3b18a do not return 0 when the final tag is missing
- 1fcace4 correctly calculate when to expect eof when reading a file
- 7290a69 prevent segfault when trying to update block0 in swapfile
- cebacac also close the swapfile when setting the cryptmethod
- 9fab61e Fix memory leak
- a5b26cf reset filesize counter on retry when reloading file
- 2e16b6b always reset filesize on initial load (e.g. when retryigin)
- b95b796 correctly free pointer when linerest is set
- f44b116 only set finish flag on writing, when ptr is null
- 05bfe10 reset real_size when a new buffer was allocated
- 3ff4cdd xchacha20: Add tests
- b3d5326 xchacha20: disable undofile when using sodium crypt
- 7159fbe xchacha20: make sure, buffer does not overflow
- de06a3c xchacha20: Add comment to testfile
- 99078bd xchacha20: review -> fix typos in documentation
—
You are receiving this because you are subscribed to this thread.
Christian Brabandt
On Do, 17 Jun 2021, Bram Moolenaar wrote:
> > Yes, blowfish is kept separate. But the `XChaCha20-Poly1305` uses the
> > `crypt_pwhash` API and it also uses the `randombytes` API for the salt
> > and the seed.
> >
> > I did not want to touch the blowfish code for now, as I don't know, if
> > this is backwards compatible (e.g. if this will can still decrypt
> > existing blowfish encrypted files (swap files, undofile, files))
>
> If libsodium is available, would there be any reason to use blowfish,
> other than reading existing encrypted files? Probably not, thus
> changing the blowfish crypt is not helping.
Best,
Christian
--
Linux: die Schweizer Offizierskettensäge
Bram Moolenaar
Christian wrote:
> > I think you mean "disable the swapfile". When using strong encryption
>
> Yes of course.
>
> > writing the swapfile (accidentally) leaks the text, thus I think we
> > should make it impossible to enable the swapfile.
>
> I fixed a couple of more problems to make sure Vim closes the swapfile
> and won't try to encrypt it. I also fixed a couple of edge cases that
> come from the fact, that since the new crypt code allocates a new
> buffer, converting could cause a buffer-overflow. Also properly free
> the allocated buffer.
>
> > The swapfile is temporary, thus some people may think it's OK. But
> > these days with SSD and other types of disks deleting a file doesn't
> > mean the text is gone. Even reformatting the disk may not help.
>
> I also disabled the undofile. I thought it worked before, but when
> writing the tests, I got a couple of overflows, most likely, because
> the size was too small.
>
> I added some tests to ensure the buffer size for en/decoding is large enough.
>
> Tests should be there in a minute , once I push. They probably won't yet run,
> I need to make changes to the CI, so that vim is build with libsodium.
work at some point. I use the undo file all the time, and rely on being
able to undo changes after abandoning Vim and editing a file again.
It would be unexpected that this fails only because a file is encrypted.
It may require using a different file format for the undo file.
--
hundred-and-one symptoms of being an internet addict:
6. You refuse to go to a vacation spot with no electricity and no phone lines.
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Gary Johnson
disabling the use of blowfish2 for updating existing encrypted
files, are you? That would make it very difficult if not impossible
to use various versions of Vim to view and update encrypted files.
There are various needs for encryption. I don't need to hide
anything from foreign agent crypto experts. I just want to keep
some information hidden from other people who might use my
computers. A reasonably secure code will do just fine, but having
to keep track of which Vim can access which encrypted files is not
OK.
Regards,
Gary
Christian Brabandt
On Do, 17 Jun 2021, Gary Johnson wrote:
> I may not understand what you're saying, but you aren't thinking of
> disabling the use of blowfish2 for updating existing encrypted
> files, are you? That would make it very difficult if not impossible
> to use various versions of Vim to view and update encrypted files.
>
> There are various needs for encryption. I don't need to hide
> anything from foreign agent crypto experts. I just want to keep
> some information hidden from other people who might use my
> computers. A reasonably secure code will do just fine, but having
> to keep track of which Vim can access which encrypted files is not
> OK.
keeping the blowfish code *except* for being able to read older
encrypted files (and maybe also for as long as it is marked
experimental). I was not trying to imply, that we should remove the
blowfish code.
We also still carry the old crypt-zip code around, even so I doubt
anybody is using that one. But at least I do not plan on removing it
(and I think it would be a mistake to do so for the reasons you have
mentioned).
Best,
Christian
--
Wo es den Rednern an Tiefe fehlt, da gehen sie in die Breite.
-- Charles-Louis de Montesquieu
Christian Brabandt
It may require using a different file format for the undo file.
Yes, lets leave that for later. Personally, I think it may make sense to disable undo files for sensitive data.
I have also added some slight changes to the MSVC and MSYS Makefiles. I cannot test the MSVC build however. The MSYS build works however.
—
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
The change in eval.txt uses spaces instead of tabs.
Is this now good enough to include? I think we should mark this "experimental", so that users know that the encoding may still change and files might not decrypt with a later version of Vim. This will give users some time to try it out and hopefully some crypt experts can check if we are using the crypt method properly.
—
You are receiving this because you are subscribed to this thread.
Christian Brabandt
The change in eval.txt uses spaces instead of tabs.
Fixed that.
Is this now good enough to include? I think we should mark this "experimental", so that users know that the encoding may still change and files might not decrypt with a later version of Vim. This will give users some time to try it out and hopefully some crypt experts can check if we are using the crypt method properly.
Yes, should be good now. I made a couple of minor adjustments (added proper error numbers) and adjusted the documentation a bit. I think it is good to be tried out now.
—
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> > The change in eval.txt uses spaces instead of tabs.
>
> Fixed that.
>
> > Is this now good enough to include? I think we should mark this "experimental", so that users know that the encoding may still change and files might not decrypt with a later version of Vim. This will give users some time to try it out and hopefully some crypt experts can check if we are using the crypt method properly.
>
> Yes, should be good now. I made a couple of minor adjustments (added
> proper error numbers) and adjusted the documentation a bit. I think it
> is good to be tried out now.
The 'swapfile' option is disabled when 'cryptmethod' is set, but no key
has been entered yet. The swapfile should only be disabled when
encryption is actually setup, thus the 'key' option is not empty.
When typing a wrong password the error is:
E1200: Decryption failed: corrupted chunk!
The "corrupted chunk" note is incorrect. We may not be able to
distinguish between a wrong password (which actually makes a brute-force
attack quite a bit easier) and a corrupted file (or changed binary
format). Just saying "Decryption failed" should be sufficient.
I hope you can improve this soon.
I wonder why the invalid sample text is so long.
--
hundred-and-one symptoms of being an internet addict:
your children are.
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Christian Brabandt
Hi Bram, thanks for merging.
The 'swapfile' option is disabled when 'cryptmethod' is set, but no key
has been entered yet. The swapfile should only be disabled when
encryption is actually setup, thus the 'key' option is not empty.
Are you talking about manually setting the key? E.g. not after :X? It makes sense,
but manually setting the key should not be done.
The "corrupted chunk" note is incorrect. We may not be able to
distinguish between a wrong password
Okay makes sense, will change.
I wonder why the invalid sample text is so long.
I was trying to get a file that is written in 2 chunks, so 16384 K. I made some mistakes, but thought it would be a good test sample nevertheless.
I'll create a follow-up PR implementing those changes.
Yegappan Lakshmanan
Hi Bram, thanks for merging.
The 'swapfile' option is disabled when 'cryptmethod' is set, but no key
has been entered yet. The swapfile should only be disabled when
encryption is actually setup, thus the 'key' option is not empty.Are you talking about manually setting the key? E.g. not after
:X? It makes sense,
but manually setting the key should not be done.The "corrupted chunk" note is incorrect. We may not be able to
distinguish between a wrong passwordOkay makes sense, will change.
I wonder why the invalid sample text is so long.
I was trying to get a file that is written in 2 chunks, so 16384 K. I made some mistakes, but thought it would be a good test sample nevertheless.
I'll create a follow-up PR implementing those changes.
vim-dev ML
Christian Brabandt
On So, 20 Jun 2021, Yegappan Lakshmanan wrote:
> After merging these changes, the code coverage has gone down (as expected).
> Can you also take a look at adding additional tests for the new crypto code?
actually error-handling. I already added an invalid sample to have at
least some coverage in the error case. I'll have another look however.
Best,
Christian