Skip to content

refactor: rewrite aes settings#815

Merged
Pr0methean merged 19 commits intomasterfrom
rewrite-aes-settings-2
May 5, 2026
Merged

refactor: rewrite aes settings#815
Pr0methean merged 19 commits intomasterfrom
rewrite-aes-settings-2

Conversation

@Its-Just-Nans
Copy link
Copy Markdown
Member

@Its-Just-Nans Its-Just-Nans commented May 4, 2026

@Its-Just-Nans Its-Just-Nans self-assigned this May 4, 2026
Copy link
Copy Markdown
Contributor

@amazon-q-developer amazon-q-developer Bot left a comment

Choose a reason for hiding this comment

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

Code Review Summary

This PR introduces buffer reuse functionality to improve memory efficiency, but contains critical security and logic issues that must be fixed before merge:

Critical Issues Found:

  1. Logic Error in AES padding validation (src/aes.rs:116-121): The padding validation is inverted - panics on valid padding and accepts invalid padding
  2. Security vulnerabilities (4 instances): Password data is not zeroized after use in multiple functions, leaving sensitive data in memory

Required Actions:

  • Fix the inverted padding validation logic in AES decryption
  • Add proper password zeroization after use in all affected functions (read.rs and write.rs)

All identified issues have specific code suggestions attached for easy remediation.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

Comment thread src/write.rs
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors encryption handling by moving the EncryptWith enum to src/types.rs and removing the redundant AesModeOptions struct. FileOptions and ZipWriter now use an updated EncryptWith::Aes variant that includes vendor_version and an optional password, simplifying metadata management and CRC logic. Feedback suggests renaming the crc_in_header variable to improve clarity regarding its role in data descriptor logic.

Comment thread src/write.rs Outdated
@Its-Just-Nans Its-Just-Nans changed the base branch from rewrite-aes-settings to master May 4, 2026 21:22
Comment thread src/read.rs Fixed
Comment thread src/types.rs Fixed
Comment thread src/types.rs Fixed
@Its-Just-Nans
Copy link
Copy Markdown
Member Author

Its-Just-Nans commented May 4, 2026

In this MR

I move EncryptWith to src/types.rs and use it instead of aes_mode (it was kinda duplicates)

@Pr0methean Pr0methean added this to the 9.0.0 milestone May 4, 2026
Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
Copy link
Copy Markdown
Member

@Pr0methean Pr0methean left a comment

Choose a reason for hiding this comment

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

Looks good; just one nitpick (the variable where Gemini complained about the misleading name can just be inlined).

Comment thread src/write.rs Outdated
@Pr0methean Pr0methean enabled auto-merge May 4, 2026 22:47
@Pr0methean Pr0methean mentioned this pull request May 4, 2026
2 tasks
@Pr0methean Pr0methean modified the milestones: 9.0.0, 9.0.0-pre1 May 4, 2026
@Pr0methean Pr0methean added this pull request to the merge queue May 5, 2026
Merged via the queue into master with commit 04531fc May 5, 2026
133 checks passed
@Pr0methean Pr0methean deleted the rewrite-aes-settings-2 branch May 5, 2026 05:08
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.

3 participants