Skip to content

Conversation

@thevilledev
Copy link
Contributor

Motivation

This change addresses an issue where modifying the Config.WorkFactor after creating a credential could inadvertently affect existing credentials because they shared the same WorkFactor pointer.

Changes

The fix introduces a cloneWorkFactor helper that performs a shallow copy of the WorkFactor struct using a type switch (avoiding reflection and marshal overhead). It also updates Config.NewCredential to store an independent copy in the Credential.

Testing

All tests (including linters) pass. Benchmark shows a slight difference in allocations, which is understandable:

goos: darwin
goarch: arm64
pkg: github.com/dhui/passhash
cpu: Apple M1 Pro
                                 │   before    │               after                │
                                 │   sec/op    │   sec/op     vs base               │
DefaultWorkFactorPbkdfSha256-8     11.24m ± 0%   11.31m ± 0%  +0.62% (p=0.004 n=10)
DefaultWorkFactorPbkdfSha512-8     27.16m ± 0%   27.16m ± 0%       ~ (p=0.631 n=10)
DefaultWorkFactorPbkdfSha3_256-8   36.81m ± 0%   36.95m ± 1%  +0.39% (p=0.015 n=10)
DefaultWorkFactorPbkdfSha3_512-8   38.12m ± 0%   38.17m ± 0%       ~ (p=0.218 n=10)
DefaultWorkFactorBcrypt-8          250.9m ± 2%   252.1m ± 4%       ~ (p=0.481 n=10)
DefaultWorkFactorScrypt-8          101.8m ± 1%   101.7m ± 5%       ~ (p=0.912 n=10)
geomean                            47.11m        47.23m       +0.25%

                                 │    before    │                after                │
                                 │     B/op     │     B/op      vs base               │
DefaultWorkFactorPbkdfSha256-8       884.0 ± 0%     896.0 ± 0%  +1.36% (p=0.000 n=10)
DefaultWorkFactorPbkdfSha512-8     1.426Ki ± 0%   1.438Ki ± 0%  +0.82% (p=0.000 n=10)
DefaultWorkFactorPbkdfSha3_256-8   1.426Ki ± 0%   1.438Ki ± 0%  +0.82% (p=0.000 n=10)
DefaultWorkFactorPbkdfSha3_512-8   1.363Ki ± 0%   1.375Ki ± 0%  +0.86% (p=0.000 n=10)
DefaultWorkFactorBcrypt-8          5.381Ki ± 1%   5.393Ki ± 1%  +0.22% (p=0.007 n=10)
DefaultWorkFactorScrypt-8          64.01Mi ± 0%   64.01Mi ± 0%  +0.00% (p=0.001 n=10)
geomean                            9.721Ki        9.787Ki       +0.68%

                                 │   before   │               after               │
                                 │ allocs/op  │ allocs/op   vs base               │
DefaultWorkFactorPbkdfSha256-8     13.00 ± 0%   14.00 ± 0%  +7.69% (p=0.000 n=10)
DefaultWorkFactorPbkdfSha512-8     13.00 ± 0%   14.00 ± 0%  +7.69% (p=0.000 n=10)
DefaultWorkFactorPbkdfSha3_256-8   13.00 ± 0%   14.00 ± 0%  +7.69% (p=0.000 n=10)
DefaultWorkFactorPbkdfSha3_512-8   13.00 ± 0%   14.00 ± 0%  +7.69% (p=0.000 n=10)
DefaultWorkFactorBcrypt-8          11.00 ± 9%   12.00 ± 8%  +9.09% (p=0.000 n=10)
DefaultWorkFactorScrypt-8          25.00 ± 0%   26.00 ± 0%  +4.00% (p=0.000 n=10)
geomean                            14.10        15.13       +7.30%

Backwards compatibility

No API changes.

This change addresses an issue where modifying the Config.WorkFactor
after creating a credential could inadvertently affect existing
credentials because they shared the same WorkFactor pointer.

The fix introduces a cloneWorkFactor helper that performs a shallow
copy of the WorkFactor struct using a type switch (avoiding reflection
and marshal overhead) and updates Config.NewCredential to store
an independent copy in the Credential.

Signed-off-by: Ville Vesilehto <[email protected]>
Copy link
Owner

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Thanks @thevilledev for finding another security issue & fixing it!
I appreciate the responsible disclosures.

@dhui dhui merged commit 986292f into dhui:master Nov 26, 2025
3 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