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.

Sign up for GitHub

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

Jump to bottom

[BUG] Blake2b hashing for Masked Fields does not apply salt correctly #4274

Open
terryquigleysas opened this issue Apr 20, 2024 · 1 comment · May be fixed by #5089
Open

[BUG] Blake2b hashing for Masked Fields does not apply salt correctly #4274

terryquigleysas opened this issue Apr 20, 2024 · 1 comment · May be fixed by #5089
Labels
breaking This issue is or proposes a breaking change triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@terryquigleysas
Copy link
Contributor

terryquigleysas commented Apr 20, 2024 •
edited
Loading

I think there is possibly a bug in the current Blake2b code. The defaultSalt variable is not passed to the Blake2bDigest constructor using the salt parameter as I would expect. It uses personalization instead. Is this correct?

image

image

In order to proceed which of these options is recommended?

  1. Change to pass the defaultSalt variable to the Blake2bDigest constructor using the salt parameter. This will change the hash values. OR
  2. Continue to pass the defaultSalt variable to the Blake2bDigest constructor using the personalization parameter, assuming there is a valid reason for this. The hash values generated will remain the same.

Originally posted by @terryquigleysas in #4212 (comment)

Approach #1 would appear to be the correct thing to do but there are concerns that changing hashes, even to the correct values, may impact existing users.

This bug is derived from discussions on #4271 and #4212
@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Apr 20, 2024
This was referenced Apr 20, 2024
Closed
Closed
@stephen-crawford
Copy link
Contributor

[Triage] Hi @terryquigleysas thank you for filing this issue. This sounds like a worthwhile change which could help correct some unexpected behavior. We will need to handle the issues around the backwards compatibility of the code when reviewing the PR.

@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Apr 22, 2024
@peternied peternied added the breaking This issue is or proposes a breaking change label Apr 23, 2024
@terryquigleysas terryquigleysas mentioned this issue May 14, 2024
Merged
3 tasks
@prabhask5 prabhask5 mentioned this issue Jul 5, 2024
Closed
4 tasks
@terryquigleysas terryquigleysas linked a pull request Feb 5, 2025 that will close this issue
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking This issue is or proposes a breaking change triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix Blake2b hash terryquigleysas/security
3 participants
@peternied @stephen-crawford @terryquigleysas