Skip to content

Metadata encryption bug#5037

Merged
paweljakubas merged 3 commits intomasterfrom
paweljakubas/5017/metadata-encryption-bug
Mar 11, 2025
Merged

Metadata encryption bug#5037
paweljakubas merged 3 commits intomasterfrom
paweljakubas/5017/metadata-encryption-bug

Conversation

@paweljakubas
Copy link
Collaborator

@paweljakubas paweljakubas commented Mar 11, 2025

The bug was triggered when metadata password shorter than 64 bytes was compared with the same password padded with /NUL (in hex 0x00). It turn out that this is corner case that could appear, indeed!

Reason:
In impl of KDF we are using, ie. PBKDF2 (https://www.ietf.org/rfc/rfc2898.txt) is about successive hashing of the message. And here password (P) and salt (S) are undergoing :

H1= Hash( P || S)
H2=Hash (H1)
....

The culprit is bitwise OR operation. As /NUL is 0x00 it is the same as automatic padding that is done when P is not 64 bytes!

  • I have added property with description to document the case in cyrptographic-primitives
  • MetadataPassword was introduced in MetadataEncryptionSpec to enforce using alphanumeric character (btw. recommended by openssl):
    -> see https://docs.openssl.org/3.1/man1/openssl-kdf/#options
pass:string
Specifies the password as an alphanumeric string (use if the password contains printable characters only). The password must be specified for PBKDF2 and scrypt.

Comments

Issue Number

fix #5017

@paweljakubas paweljakubas requested a review from abailly March 11, 2025 13:55
@paweljakubas paweljakubas self-assigned this Mar 11, 2025
Copy link
Collaborator

@paolino paolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

=> PBKDF2Config h
-> ByteString
-- ^ payload
-- ^ password
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

\(TestCase algo' iters' keyL' ivL' passwd' salt') -> do
let nulsToAdd = 64 - BS.length passwd'
let passwdPadded = passwd' <> BS.replicate nulsToAdd 0
generateKey (PBKDF2Config algo' iters' keyL' ivL') passwd' salt'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

fromHexToM :: Text -> Maybe ByteString
fromHexToM = rightToMaybe . convertFromBase Base16 . T.encodeUtf8

newtype MetadataPassword = MetadataPassword {unPassword :: ByteString}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use a generator and avoid creating types ? Just 2c

@paweljakubas paweljakubas added this pull request to the merge queue Mar 11, 2025
Merged via the queue into master with commit c41d95b Mar 11, 2025
25 checks passed
@paweljakubas paweljakubas deleted the paweljakubas/5017/metadata-encryption-bug branch March 11, 2025 16:57
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.

Metadata encryption property test fails

2 participants