Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(wallet): set Argon2 derived bytes for AES IV #1703

Merged
merged 3 commits into from
Mar 11, 2025

Conversation

b00f
Copy link
Collaborator

@b00f b00f commented Mar 9, 2025

Description

This PR updates the encryption logic to use Argon2-derived bytes for the Initialization Vector (IV) in AES, replacing the legacy approach of reusing the salt as the IV.

Explanation:

In the legacy or previous approach, we used the same salt in the Password Hasher (Argon2 in this case) as the Initialization Vector (IV) for the AES encryption algorithm. The seed is random and should not be an issue, but some documentation recommends never reusing a seed, even for different purposes.

To address this, we propose extending the key length from 32 bytes to 48 bytes, using the first 32 bytes as the encryption key and the remaining 16 bytes as the IV.

In OpenSSL, the salt is public. I found this code.
The encrypted files usually start with Salted__.

Based on this documentation, the IV is derived from the password in OpenSSL. Check the -iv section:

The actual IV to use: this must be represented as a string comprised only of hex digits. When only the key is specified using the -K option, the IV must explicitly be defined. When a password is being specified using one of the other options, the IV is generated from this password.

We follow the same approach in this PR. Here I found the code that generates IV in OpenSSL.

Copy link

codecov bot commented Mar 9, 2025

Codecov Report

Attention: Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.

Project coverage is 76.18%. Comparing base (86a8ba9) to head (b43284c).
Report is 57 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1703      +/-   ##
==========================================
+ Coverage   75.07%   76.18%   +1.10%     
==========================================
  Files         234      242       +8     
  Lines       12156    18668    +6512     
==========================================
+ Hits         9126    14222    +5096     
- Misses       2582     3983    +1401     
- Partials      448      463      +15     
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@b00f b00f changed the title fix(wallet): use derived bytes from Argon2 for AES IV fix(wallet): Argon2 derived bytes for AES IV Mar 9, 2025
@b00f b00f force-pushed the feat/wallet-iv branch from e5ac04e to 0afa81c Compare March 9, 2025 13:49
@b00f b00f force-pushed the feat/wallet-iv branch from 0afa81c to 9dc99a8 Compare March 9, 2025 13:50
@b00f b00f changed the title fix(wallet): Argon2 derived bytes for AES IV fix(wallet): set Argon2 derived bytes for AES IV Mar 9, 2025
@Ja7ad Ja7ad self-requested a review March 10, 2025 05:14
defaultIterations = 3
defaultMemory = 65536 // 2 ^ 16
defaultParallelism = 4
defaultKeyLen = 48
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find recommended value for KeyLen and what is it keylen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Check above explanation

@b00f b00f merged commit 7eb0b0a into pactus-project:main Mar 11, 2025
14 checks passed
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