Encryption: Provide better alternative using libsodium for encryption#8394
Encryption: Provide better alternative using libsodium for encryption#8394chrisbra wants to merge 1 commit intovim:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8394 +/- ##
==========================================
- Coverage 89.85% 89.35% -0.50%
==========================================
Files 149 145 -4
Lines 167551 165750 -1801
==========================================
- Hits 150548 148113 -2435
- Misses 17003 17637 +634
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
d8e8dc7 to
44e3b70
Compare
|
Oh and for development I have used |
|
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
[libsodium](https://libsodium.gitbook.io/doc/), 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 (?)
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 -- Bram@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 ///
|
3f769d5 to
262db22
Compare
|
I see the Blowfish -> XChaCha20-Poly1305 change fixing #638, but I don't see a KDF replacement for |
262db22 to
1e9942b
Compare
|
Yes, blowfish is kept separate. But the 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)) |
|
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! |
|
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!
I think you mean "disable the swapfile". When using strong encryption
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 -- Bram@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 ///
|
runtime/doc/options.txt
Outdated
| 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| |
There was a problem hiding this comment.
| requires Vim to be build with |+sodium| | |
| requires Vim to be built with |+sodium| |
runtime/doc/options.txt
Outdated
| 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. |
There was a problem hiding this comment.
| Currently experiemental. | |
| Currently experimental. |
There was a problem hiding this comment.
will be fixed on next push
Yes of course.
I fixed a couple of more problems to make sure Vim closes the swapfile
I also disabled the undofile. I thought it worked before, but when writing the tests, 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, |
99078bd to
ad7ed4a
Compare
|
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.
Disabling the undo file for now is OK, but we should really make this
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 -- Bram@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 ///
|
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. |
32e0f65 to
5c94c59
Compare
|
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. |
5c94c59 to
c7e43ea
Compare
After the issues vim#638 and vim#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](https://libsodium.gitbook.io/doc/), which is a library providing encryption, hashing and other cryptographi 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. Tests are written and enabled in the CI system. Also, a couple of minor bugs have been fixed in the reading and writing of buffers code. Todo: - [ ] fix error numbers - [ ] write tests - [ ] make changes to the windows build system - [ ] enable complete undofile encryption (?)
c7e43ea to
c9255fd
Compare
Fixed that.
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 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.
I included the patch now, but alread found a couple of problems:
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:
24. You realize there is not a sound in the house and you have no idea where
your children are.
/// Bram Moolenaar -- Bram@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 ///
|
|
Hi Bram, thanks for merging.
Are you talking about manually setting the key? E.g. not after
Okay makes sense, will change.
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. |
|
Hi Christian,
On Sun, Jun 20, 2021 at 9:34 AM Christian Brabandt < ***@***.***> wrote:
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.
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?
Thanks,
Yegappan
|
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: