Skip to content
This repository was archived by the owner on Nov 24, 2024. It is now read-only.

Fixed insecure cryptography in PBECipher.java #23

Merged
merged 1 commit into from
Apr 22, 2023

Conversation

gL1nt
Copy link
Contributor

@gL1nt gL1nt commented Dec 20, 2022

By default (with the default constants and settings) the DefaultPlexusCipher object takes a plaintext and a human-readable password that I will refer to as “pwd”. It then generates a key and an IV to be used with AES/CBC/PKCS5Padding. The encrypt64 method generates a random 8 byte salt, computes SHA256(pwd|salt), and then uses the first 16 bytes of the hash as the AES key and the last 16 bytes of the hash as the IV. The salt is then prepended to the ciphertext and the result is base64 encoded and outputted.
 
This key derivation algorithm is extremely insecure and ciphertexts produced by plexus-cipher are currently easy to brute force. SHA256 is meant to be fast. During a brute force attempt, identifying the correct key and IV is fairly easy: the correct key and IV will yield a decryption with significantly less Shannon entropy than all of the failed decryption attempts, which will produce outputs indistinguishable from random bytes (which will have high Shannon entropy). Data that needs to be encrypted tends to have some sort of structure, and is often times composed of ASCII characters.

An ideal fix here is to just use a well-established key derivation function such as PBKDF2 (as I have done). OWASP recommends to use 310000+ iterations with PBKDF2.

https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html

This fix is not backwards compatible and will will cause people using Plexus Cipher to have to re-encrypt their information. The existing cryptography provides little protection in its current state though.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@slachiewicz slachiewicz requested a review from cstamas January 1, 2023 19:58
@cstamas cstamas requested review from gnodet and michael-o January 2, 2023 14:12
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

I am not an expect on this, therefore I will not make a qualified statement, though I think this must be removed from Maven because it does not provide any security at all.

@slachiewicz slachiewicz merged commit 176289f into codehaus-plexus:master Apr 22, 2023
slachiewicz added a commit that referenced this pull request Jul 30, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
slachiewicz added a commit that referenced this pull request Jul 30, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
slachiewicz added a commit that referenced this pull request Aug 1, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@cstamas cstamas mentioned this pull request Sep 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants