Skip to content

clamp argon2 memory when parsing untrusted packets - #324

Open
knQzx wants to merge 1 commit into
ProtonMail:mainfrom
knQzx:clamp-argon2-memory
Open

clamp argon2 memory when parsing untrusted packets#324
knQzx wants to merge 1 commit into
ProtonMail:mainfrom
knQzx:clamp-argon2-memory

Conversation

@knQzx

@knQzx knQzx commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

The Argon2 memory parameter from an untrusted S2K packet is used to size an allocation with no upper bound, so a small packet can request terabytes of memory. This clamps the memory parameter to a sane maximum.

@twiss

twiss commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Hi 👋 Thanks for the PR. However, I'd argue this should be a config parameter, because not everybody might agree what is a "sane maximum". For example, we could add a MaxMemory field to Argon2Config, and extend it to be passed during decryption as well as during generation. (It would be best not to outright fail when parsing such packets.)

@knQzx
knQzx force-pushed the clamp-argon2-memory branch from 1fce29b to 6b2a083 Compare August 13, 2026 17:01
@knQzx

knQzx commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

done. MaxMemory on Argon2Config, default 2 GiB. the check moved into Params.FunctionWithConfig, so a huge memoryExp parses fine and only fails at derivation. Serialize passes the config too

getting it down to decrypt needed a few *WithConfig variants, old ones delegate with nil like EncryptWithConfig. side effect: we keep s2kParams now instead of the built closure, so s2k errors surface at decrypt instead of parse

@knQzx

knQzx commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

if you have any other suggestions, I'm fully open!!

@twiss twiss left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Awesome, thanks! Some small nits below.

Comment thread openpgp/packet/private_key.go Outdated
Comment thread openpgp/s2k/s2k_config.go Outdated
Comment thread openpgp/s2k/s2k_config.go Outdated
Comment thread openpgp/s2k/s2k_config.go Outdated
@knQzx

knQzx commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

all four applied. MaxMemoryUsage is public now and sits at the bottom of s2k_config.go, and I tweaked the S2KConfig doc in packet/config.go too so it mentions decryption

@knQzx
knQzx force-pushed the clamp-argon2-memory branch from 6b2a083 to 7832436 Compare August 14, 2026 16:19
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