Possible Argon2id incorrect derived key

[huven]

Posting this as there might be an issue with the Argon2id KDF (currently in beta).

I'm not sure if there is an issue (I might be making a mistake myself) , but if there is I think it is important to get this fixed before this feature leaves beta.

Note: I'm the author of Disk Decipher, iOS app that supports VeraCrypt format, so I think I have a pretty good understanding of how this is supposed to work.

Expected behavior

I expect to be able to derive the key manually and use it to decrypt the header

Observed behavior

Header decryption fails when I derive the key myself.

Steps to reproduce

1. I created a test container on Win10 (x64) using VeraCrypt Portable 1.26.27.
   To reproduce, you can download it here. The password is 1234. It mounts without issue on the device where it was created as expected. It uses AES cipher and Argon2 KDF.
2. Next step is to derive the header key, I tried it with two different implementations, both arrive at the same result. Here is a command line version on Linux for easy reproduction:

$ echo -n 1234 | argon2 "$(head -c 64 test_argon2id.hc)" -id -t 6 -k 425984 -p 1 -l 64 -r -v 13
48bf17e760158d23a02b123e31bb2fd78d9b55f7bd530d8be5a5f0c8feea508ce3072386adb5d932701ec2c04a0ebc51a24cbd8d754385fe7f9a44d29f90e12b

3. I then feed this key to the header-decryption routine, which fails to produce the expected VERA signature. This same routine is used for the other KDF's, it just takes the header and the key as input (has no knowledge how the key was derived).
   I don't have a command line version for this yet, will add when I have one.

I've triple checked all steps, code and also looked at the implementation in VeraCrypt. All looks correct, yet the decryption fails, so there is an issue hidden somewhere. This may well be an error on my side, but could also be an issue in VeraCrypt. The latter is important to fix before people start using this to encrypt their data.

To debug this, it would be great if someone with a Windows x64 build environment loads the test container linked above, and sets a breakpoint at line 482 of Volumes.c (after magic number verification). Please inspect the first 64 bytes of cryptoInfo.ks and cryptoInfo.ks2 to see if they match the (combined) derived key shown above in hex.

If they are different then that would explain why the manual decryption fails, though we still need to find out why they are different.

Screenshots

Your Environment

Please tell us more about your environment

VeraCrypt version: VeraCrypt Portable 1.26.27

Operating system and version: Windows 10 build 19045

System type: x64

[huven]

Here is a command line script to reproduce the decryption failure, output matches my other implementation (using Botan library).

export KEY=`echo -n 1234 | argon2 "$(head -c 64 test_argon2id.hc)" -id -t 6 -k 425984 -p 1 -l 64 -r -v 13`
echo $KEY
echo $KEY | xxd -r -p > key.bin

# Encrypted header
dd if=test_argon2id.hc bs=1 skip=64 count=448 of=partial.bin
truncate -s 512 partial.bin
losetup /dev/loop0 partial.bin

# Setup header decrypt
cryptsetup open --type plain \
  --cipher aes-xts-plain64 \
  --key-size 512 \
  --key-file key.bin \
  /dev/loop0 decrypted

# Read decrypted
hexdump -C -n 16 < /dev/mapper/decrypted

# Cleanup
cryptsetup close decrypted
losetup -d /dev/loop0

Output (incorrect signature):

48bf17e760158d23a02b123e31bb2fd78d9b55f7bd530d8be5a5f0c8feea508ce3072386adb5d932701ec2c04a0ebc51a24cbd8d754385fe7f9a44d29f90e12b
448+0 records in
448+0 records out
448 bytes copied, 0.0881701 s, 5.1 kB/s
00000000  ac 45 f3 28 fc 9b 27 e0  1d b2 13 11 60 3d ee 5b  |.E.(..'.....`=.[|
00000010

As proof-of-concept for the decryption script, the same code (except KDF) for another container using Blake2s, here I use OpenSSL for KDF:

export KEY=`openssl kdf -keylen 64 -kdfopt digest:blake2s256 -kdfopt pass:1234 -kdfopt hexsalt:"$(xxd -p -l 64 -c 0 test_blake2s.hc)" -kdfopt iter:500000 PBKDF2 | tr -d :`
echo $KEY
echo $KEY | xxd -r -p > key.bin

# Encrypted header
dd if=test_blake2s.hc bs=1 skip=64 count=448 of=partial.bin
truncate -s 512 partial.bin
losetup /dev/loop0 partial.bin

# Setup header decrypt
cryptsetup open --type plain \
  --cipher aes-xts-plain64 \
  --key-size 512 \
  --key-file key.bin \
  /dev/loop0 decrypted

# Read decrypted
hexdump -C -n 16 < /dev/mapper/decrypted

# Cleanup
cryptsetup close decrypted
losetup -d /dev/loop0

Output (correct signature):

AFC7A35DE3E0FE9EA1E8E7FD051BF22C3BE17A641C8A0619A2FA2B7DDF6DFDB11CB742C60960046B9B8BF9EBE70044AD1B91326B3A2D5F4F051A0BFB21AA3C81
448+0 records in
448+0 records out
448 bytes copied, 0.0889157 s, 5.0 kB/s
00000000  56 45 52 41 00 05 01 0b  2a 68 40 9a 00 00 00 00  |VERA....*h@.....|
00000010

[huven]

Here is third library producing the same derived key:

from argon2.low_level import hash_secret_raw, Type
import sys

password = b"1234"
with open("test_argon2id.hc", "rb") as f:
    salt = f.read(64)
key = hash_secret_raw(password, salt, time_cost=6, memory_cost=425984, parallelism=1, hash_len=64, type=Type.ID)
print(key.hex())

Output

# python3 test_argon2id.py
48bf17e760158d23a02b123e31bb2fd78d9b55f7bd530d8be5a5f0c8feea508ce3072386adb5d932701ec2c04a0ebc51a24cbd8d754385fe7f9a44d29f90e12b

[huven]

I think I tracked down the cause of the issue. In Volumes.c there is this logic (in two locations):

derive_key_argon2(keyInfo->userKey, keyInfo->keyLength, keyInfo->salt,
    PKCS5_SALT_SIZE, keyInfo->noIterations, keyInfo->memoryCost, dk, GetMaxPkcs5OutSize(), &abortKeyDerivation);

where the GetMaxPkcs5OutSize() currently always resolves to 192, being the maximum XTS key length needed for a cascade of 3 ciphers.

So a key is derived with length 192 and then truncated to e.g. 64 if only one cipher is used. While this produces the same key as deriving a 64 byte key when using PBKDF2, this is not equivalent for Argon2, i.e.

truncate(deriveKey(length: 192), 64) != deriveKey(length: 64)

So, I think the GetMaxPkcs5OutSize() should be replaced, at least for Argon2, by bytesNeeded.

@idrassi do you agree? Or do you consider the current behavior correct?

IMHO, deriving a longer key and then truncating is not future-proof. Just imagine introducing a 1024-bit cipher (cascade of 4 ciphers, or otherwise 1024). The header is large enough to store these keys, but all current Argon2 derived keys would become invalid when VeraCrypt starts using 256 for GetMaxPkcs5OutSize()..

Let me know if you agree and I'll send a PR.

[huven]

Just noticed that in ReadVolumeHeader the derive_key_argon2 call is outside the ea-loop. I previously focused on CreateVolumeHeaderInMemory which knows the ea when deriving the key.

So that would require refactoring ReadVolumeHeader to make the Argon2 derived keys dependent on the ea. That might be a bridge too far.

Alternatively, the argon2 key derivation could always use the maximum key material size supported by the header (256 instead of 192) and truncate afterwards as done currently. That would not break for future ea's with > 768-bit key size. This would not/hardly have any performance impact (for Argon2).

Thoughts?

[fzxx]

Since there is a discussion on cryptography, I have to mention a more secure derivation method and recommend upgrading the volume version to 3. @idrassi https://sourceforge.net/p/veracrypt/discussion/features/thread/adb29bdd88/ I hope everyone can provide suggestions for this kind of derivation method. https://github.com/fzxx/XiangYue/blob/main/SECURITY.md#v1300%E5%8F%8A%E4%BB%A5%E4%B8%8A

[huven]

Thanks @fzxx that's an interesting sourceforge post. I will reply to that post on sourceforge to keep this issue focussed.

[thericle]

This all makes sense to me, even though I'm far from an expert. So the difference is that with PBKDF2, creating a longer key and then truncating it would give you the same result as creating a shorter key in the first place, which is not the case for Argon2. So, for PBKDF2, which was used exclusively until now, that was a non-issue.

This is the kind of issue that, if fixed, needs to be fixed now, before the next release. It will cause problems even now that the beta has been out for months and some people are definitely using it. So if someone had created a volume with Argon2 with version 1.26.27 and it gets changed in, say, version 1.26.28, they would need to change the KDF to PBKDF2 using version 1.26.27 and then change back to Argon2 using version 1.26.28.

That all being said, I don't see VeraCrypt changing to a cascade of 4 ciphers in the future, and that would be the upper limit with the current volume header structure. I wonder if there are other code choices that intentionally or unintentionally make the assumption that 3 ciphers is the upper limit. But if not, I believe this change is worth making.

[huven]

That's exactly why I reported this in an early stage, it's now or never.

Apart from a cascade of 4 ciphers, it already becomes relevant when just one 512-bit cipher is added and used in a cascade of 3 ciphers. Or supporting a minimal cascade of two 512-bit ciphers.