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

Relax restrictions on scrypt log_n #27716

Open
CyanChanges opened this issue Jan 18, 2025 · 1 comment
Open

Relax restrictions on scrypt log_n #27716

CyanChanges opened this issue Jan 18, 2025 · 1 comment
Labels
crypto Related to node:crypto or WebCrypto node compat upstream Changes in upstream are required to solve these issues

Comments

@CyanChanges
Copy link
Contributor

Version: Deno 2.1.6

// scrypt.js
import crypto from 'node:crypto'

crypto.scrypt('password', 'salt', 128, (err, hashed) => {
  if (err) throw err
  console.log('hashed = ', hashed.toString('hex'))
})

crypto.scrypt('password', 'salt', 128, (err, hashed) => {
  if (err) throw err
  console.log('hashed = ', hashed.toString('hex'))
})
$ deno ./scrypt.js

============================================================
Deno has panicked. This is a bug in Deno. Please report this
at https://github.com/denoland/deno/issues/new.
If you can reliably reproduce this panic, include the
reproduction steps and re-run with the RUST_BACKTRACE=1 env
var set and include the backtrace in your report.

Platform: linux x86_64
Version: 2.1.6
Args: ["/home/cyan/.asdf/installs/deno/2.1.4/bin/deno", "./scrypt.js"]

thread 'tokio-runtime-worker' panicked at ext/node/ops/crypto/mod.rs:585:4:
called `Result::unwrap()` on an `Err` value: InvalidParams
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

============================================================
Deno has panicked. This is a bug in Deno. Please report this
at https://github.com/denoland/deno/issues/new.
If you can reliably reproduce this panic, include the
reproduction steps and re-run with the RUST_BACKTRACE=1 env
var set and include the backtrace in your report.

Platform: linux x86_64
Version: 2.1.6
Args: ["/home/cyan/.asdf/installs/deno/2.1.4/bin/deno", "./scrypt.js"]

thread 'tokio-runtime-worker' panicked at ext/node/ops/crypto/mod.rs:585:4:
called `Result::unwrap()` on an `Err` value: InvalidParams
$ node ./scrypt.js
hashed =  745731af4484f323968969eda289aeee005b5903ac561e64a5aca121797bf7734ef9fd58422e2e22183bcacba9ec87ba0c83b7a2e788f03ce0da06463433cda64176095fbbad7dc98c33fd75955b4b29c94f6e97617bd68d8ff17cf1ed5ad12f3fc6c8eb5b844f2d003ebaf5eaed19e8f665928472a6941f7efc6ebcdd6fd13a
hashed =  745731af4484f323968969eda289aeee005b5903ac561e64a5aca121797bf7734ef9fd58422e2e22183bcacba9ec87ba0c83b7a2e788f03ce0da06463433cda64176095fbbad7dc98c33fd75955b4b29c94f6e97617bd68d8ff17cf1ed5ad12f3fc6c8eb5b844f2d003ebaf5eaed19e8f665928472a6941f7efc6ebcdd6fd13a

Here:

let params = scrypt::Params::new(
cost as u8,
block_size,
parallelization,
keylen as usize,
)
.unwrap();

The Result is unwrapped, and it will be an Err when len > 64
According to the Document here: https://docs.rs/scrypt/latest/scrypt/struct.Params.html#conditions

But the same thing works in Node.js, maybe deno need to consider some alternatives?

@marvinhagemeister marvinhagemeister added node compat crypto Related to node:crypto or WebCrypto labels Jan 20, 2025
@littledivy littledivy changed the title Deno didn't handle the Result from scrypt::Params::new properly Relax restrictions on scrypt log_n Jan 25, 2025
@littledivy
Copy link
Member

It should error instead of panic. I updated the title as log_n > 64 support depends on RustCrypto/password-hashes#546

@littledivy littledivy added the upstream Changes in upstream are required to solve these issues label Jan 25, 2025
littledivy added a commit that referenced this issue Jan 27, 2025
Throws an error instead of panic. Ref
#27716
bartlomieju pushed a commit that referenced this issue Jan 30, 2025
Throws an error instead of panic. Ref
#27716
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Related to node:crypto or WebCrypto node compat upstream Changes in upstream are required to solve these issues
Projects
None yet
Development

No branches or pull requests

3 participants