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

[security] Replace crypto-es with latest crypto-js v4.2.0 #244

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

sirtimid
Copy link

@sirtimid sirtimid commented Jan 23, 2024

Description

This update addresses a crucial security concern in the hashing strategy by altering the default hash algorithm and iterations for PBKDF2. The previous configuration, which relied on the defaults provided by crypto-es, posed potential security weaknesses due to less robust defaults.

Key Changes:

Switched to crypto-js: We have transitioned from using crypto-es to crypto-js. This decision was driven by the wider adoption and community support for crypto-js. Its extensive use in the industry makes it a more reliable choice for our cryptographic operations.

More info:

PBKDF2 is a key-derivation function that is used for two main purposes: (1) to stretch or squash a variable length password's entropy into a fixed size for consumption by another cryptographic operation and (2) to reduce the chance of downstream operations recovering the password input (for example, for password storage).

Unlike the modern webcrypto standard, crypto-js does not throw an error when a number of iterations is not specified, and defaults to one single iteration. In the year 1993, when PBKDF2 was originally specified, the minimum number of iterations suggested was set at 1,000. Today, OWASP recommends 1,300,000

Checklist

  • The version field in package.json is incremented following semantic versioning
  • The box that allows repo maintainers to update this PR is checked
  • I tested locally to make sure this feature/fix works

@sirtimid sirtimid changed the title security: Replace crypto-es with crypto-js [security] Replace crypto-es with crypto-js Jan 23, 2024
@sirtimid sirtimid changed the title [security] Replace crypto-es with crypto-js [security] Replace crypto-es with latest crypto-js v4.2.0 Jan 23, 2024
@sirtimid
Copy link
Author

Can you check @Adamj1232 @aaronbarnardsound?

@lnbc1QWFyb24
Copy link
Collaborator

@sirtimid we are not using PBKDF2, so I am a little confused as to why it is mentioned as a reason of why to switch libraries? Could you please point us to a reference of the "crucial security concern" with the crypto-es SHA1 function that is being used in the code?

@marcinciarka
Copy link

This is probably regarding this GH (dependabot) advisory: GHSA-mpj8-q39x-wq5h

It's nice to know that the package isn't using the vulnerable function, but it would be even better to not have this critical dependabot warning ✌️

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