Skip to content

Improve Libsodium API usage and use better xchacha20 version#12279

Closed
chrisbra wants to merge 15 commits intovim:masterfrom
chrisbra:gh_12204_v3
Closed

Improve Libsodium API usage and use better xchacha20 version#12279
chrisbra wants to merge 15 commits intovim:masterfrom
chrisbra:gh_12204_v3

Conversation

@chrisbra
Copy link
Member

This is the PR for fixing #12204

I have tried to split up the changes to make reviewing easier. It starts by refactoring some of the crypt code to make integrating the new libsodium method easier.

There are 2 gotchas:

  • I am not sure if the way the hashing parameters are stored is portable. It works on my system, but I suppose on different architectures this could be problematic.
  • Reading the file was a bit tricky. Since the XChaCha20 encryption uses a fixed block size, but the file header changes, it's tricky to make decryption work between both versions. The way it works currently is, to use the slightly larger block size, but when reading the first block and we notice we are using the previous xchacha20 encryption, to first decrypting the internal block size (when decrypting the first block), but we also need to rewind the file descriptor by those bytes, else on the next block it would start reading those 20 bytes too later and we have to adjust for that.

I have added some tests to verify that the current and old encryption works when more than 1 block is read. Also added one test to make sure decrypting works with custom hashing parameters used, to verify that reading those parameters from the file work.

@chrisbra chrisbra force-pushed the gh_12204_v3 branch 3 times, most recently from 21b5866 to 2e4bc5b Compare April 19, 2023 12:32
@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #12279 (3776091) into master (1be4b81) will increase coverage by 9.81%.
The diff coverage is 80.74%.

@@            Coverage Diff             @@
##           master   #12279      +/-   ##
==========================================
+ Coverage   72.15%   81.96%   +9.81%     
==========================================
  Files         164      164              
  Lines      190725   194247    +3522     
  Branches    43573    43856     +283     
==========================================
+ Hits       137611   159214   +21603     
+ Misses      40929    22199   -18730     
- Partials    12185    12834     +649     
Flag Coverage Δ
huge-clang-none 82.68% <80.00%> (?)
huge-gcc-none 53.86% <30.30%> (-0.01%) ⬇️
huge-gcc-testgui 51.98% <30.30%> (?)
huge-gcc-unittests 0.29% <0.00%> (-0.01%) ⬇️
linux 82.40% <80.00%> (+28.90%) ⬆️
mingw-x64-HUGE 76.55% <82.14%> (?)
mingw-x86-HUGE 77.02% <82.14%> (+0.01%) ⬆️
windows 78.14% <82.14%> (+1.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/crypt_zip.c 94.11% <ø> (ø)
src/optionstr.c 91.78% <ø> (+0.50%) ⬆️
src/crypt.c 74.85% <75.96%> (+38.24%) ⬆️
src/memline.c 80.48% <94.73%> (+0.75%) ⬆️
src/blowfish.c 79.33% <100.00%> (ø)
src/buffer.c 87.58% <100.00%> (+11.15%) ⬆️
src/fileio.c 76.08% <100.00%> (+1.14%) ⬆️
src/option.c 88.68% <100.00%> (+4.35%) ⬆️

... and 140 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

chrisbra added 13 commits April 19, 2023 19:26
Refactor the crypt_create functions and instead of passing separate
arguments for salt, salt_len, seed, seed_len into a single struct and
pass that around.

This makes it easier, to later enhance the struct and add an additional
parameter to pass around.
This adds a new Xchacha20v2 Encryption algorithm. This is basically the
same as the existing xchacha20 algorithm, except that it will store the
pwhashing parameters in the encrypted file header.

The libsodium API strongely suggests to store those parameters alongside
the encrypted to file to be able to decrypt files later on.

This is required, because the libsodium API could change those
parameters and in that case it would no longer be possible to decrypt
files that were created before those constants change.

For now, this is just a copy of the xchacha20 definition. We will make
implementation changes in a later commit
Amend the cryptmethod_T struct and store the size of additional data
that needs to be stored in the crypt header. This will be used later on
to store the libsodium xchachav20 pwhashing parameters in the enrypted
file header.

For all previous methods, this is 0, except for xchacha20v2 version,
which needs to store:

  - long long opslimit (8 bytes) +
  - size_t memlimit (8 bytes) +
  - int algorithm (4 bytes)

which means we need 20 bytes additional storage.
Mention explicitly, that the swap file encryption is not supported when
using the xchacha20 encryption methods. This is because those are no
fixed size encryption and would therefore change the swap file format.
Instead of checking for crypt_method == CRYPT_M_SOD add a new function
crypt_method_is_sodium, that will check for the 2 sodium encryption
methods, CRYPT_M_SOD or CRYPT_M_SOD2.

Most of the time, when checking for xchacha20 they need to be handled
similarly (e.g. disable swapfile, disable undofile, etc), so encapsulate
the actual check in a separate function
Instead of using the libsodium API constants for the password hashing,
use variables.

Later on, depending on whether the encryption is initialized from a file
or not, we may need to pass either the default values or the values
found from the header.
In order to allow the user to use the new xchacha20v2 cryptmethod, we
need to allow to set the 'cryptmethod' option.

So now add the new value 'xchacha20v2' for the new version fixed
libsodium encryption value
Store the password hashing parameters opslimit, memlimit and algorithm
in the encrypted file header for the xchacha20v2 version and use those
values when decrypting the file contents.

Note: I am not sure, if this works correctly when reading/writing
across different architectures
Now that we have xchacha20v2 working and storing the relevant parameters
in the file header, we can mark the previous version as deprecated (even
so it is not really weak).
Amend the documentation for the new xchacha20v2 encryption method.

While at it, also be explicit and directly mention for each weak
encryption to no longer use it in the description of the option value
'cryptmethod'
We need to be careful when changing the header format. Enryption and
Decryption of file contents depends on the same block size.
Unfortunately, with xchacha20v2, the file header changes and adds
additional 20 bytes and we will use that block size to read the first
block.

However, when starting to read an enrypted file, we do not know yet,
whether it uses the xchacha20 or xchacha20v2 encryption method.

We will initially use the slightly larger block size of the xchacha20v2
version to read the file, but when detecting xchacha20 format and the
file did not fit into the 8K + plus metadata, we need to rewind the
file-descriptor by those additional 20 bytes (add_len bytes of
xchacha20v2), to allow to properly decode it.

Also when decoding the block using xchacha20, we need to adjust the
blocksize and remove the 20 bytes again, since those are only there for
the new xchacha20v2 version, but not the old version.
output the different hashing parameters when using ':verbose' to open a
xchacha20v2 enrypted file.
Now we need to make sure, that the new encryption method is functional.
Add a couple of tests to verify we can successfully decrypt those files
(even when reading several blocks)
@chrisbra
Copy link
Member Author

Tests looked good, except for one failure in macos huge which seemed unrelated:

	Test timed out: Test_spelldump_bang()
	From test_spell.vim:
	Found errors in Test_spelldump_bang():
	Test timed out: Test_spelldump_bang()
	Test caused Vim to exit: Test_spelldump_bang()

rebased to latest master now

@brammool
Copy link
Contributor

I have the habit of using a prefix for struct members, so that it's easy to change them and avoid mistakes with using the wrong name. The "cat_" prefix should work well for "crypt_arg_T".

The "#ifdef FEAT_EVAL" before using p_verbose doesn't make sense. Since this appears to be for debugging, putting the #ifdef around the use of crypt_sodium_report_hash_params() should work better (and make the tiny build a bit smaller).

Move the #ifdef FEAT_SODIUM around the whole function definition.
@chrisbra
Copy link
Member Author

I pushed 2 commits to address those two points.

@brammool brammool closed this in aae5834 Apr 23, 2023
@chrisbra chrisbra deleted the gh_12204_v3 branch June 26, 2023 20:45
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.

2 participants