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

fix(crypto): validate that digest length must be an integer between 0 and isize::MAX #3799

Merged
merged 6 commits into from
Nov 15, 2023

Conversation

jeremyBanks
Copy link
Contributor

@jeremyBanks jeremyBanks commented Nov 13, 2023

Currently, if we call crypto.subtle.digest/.digestSync with a variable-length hash algorithm and length that is...

  • too large to fit in 32 bits/greater than Rust's usize::MAX for WASM: it will be truncated, usually to 0, producing an incorrect result.
  • too large to fit in in Rust's isize::MAX for WASM: it will trigger an internal capacity_overflow error from Rust's Vec.
  • negative: it will overflow and wrap around, usually resulting in a very-large value that will trigger an internal capacity_overflow error from Rust's Vec.
  • not an integer: it will be truncated to the nearest integer value.

Some of this behaviour is a result of the JS-WASM translation layer, so this PR adds a check on the JavaScript side to throw a RangeError in these cases, before calling the Rust/WASM (which already checks that the length is valid for the specific algorithm being used).

This also resolves #3798, which describes how our tests cases were already expecting errors for these cases, but the test logic had a bug that was causing the assertion errors to be suppressed. This fixes that test logic.

One might argue that this is a BREAKING change since it produces an observable change in behaviour (at least for the non-integer case). I'm not sure it's a significant enough to count, but feel free to rename this PR to indicate that it's BREAKING if you disagree.

@jeremyBanks
Copy link
Contributor Author

These canary test failures don't seem to be related to my changes..?

Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Great work! Couple of nits.

crypto/crypto.ts Show resolved Hide resolved
crypto/crypto.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you, @jeremyBanks. Thorough work, as per usual.

crypto/crypto.ts Show resolved Hide resolved
crypto/crypto_test.ts Show resolved Hide resolved
crypto/crypto.ts Outdated Show resolved Hide resolved
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@iuioiua iuioiua merged commit c93b978 into denoland:main Nov 15, 2023
12 checks passed
@jeremyBanks jeremyBanks deleted the 3798 branch November 15, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants