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

crypto: fix Hash and Cipher abort on end #38425

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Linkgoron
Copy link
Member

@Linkgoron Linkgoron commented Apr 26, 2021

Fix Hash and Cipher aborting when using end with hex and specific lengths of chunks. The issue was caused because there was missing validation on the written content from the end method. Note that this actually affects quite a few things in Crypto. This affects decipher/cipher (both deprecated) but also decipheriv/cipheriv as well as Hash and Hmac.

I wasn't sure if this should throw in _transform or create an error and provide it to the callback, but according to the stream docs providing an error to the callback is what's expected - so I would love to get some input on my fix, and if it makes sense.

Fixes: #38015

@github-actions github-actions bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Apr 26, 2021
@Ayase-252
Copy link
Member

Does it also fix issue in #38035 (comment)? It contains invalid string (\r) in hex encoding too, however it aborts when using write.

@addaleax
Copy link
Member

@Linkgoron I think a better approach overall would be validating this on the C++ side instead, where the abort occurs in the first place.

fix Hash and Cipher aborting when using end with
hex and specific lengths of chunks

fixes: nodejs#38015
@Linkgoron Linkgoron force-pushed the crypto-cipher-hash-end-abort branch from 5e277fd to b89f9ab Compare April 27, 2021 07:45
@Linkgoron
Copy link
Member Author

Does it also fix issue in #38035 (comment)? It contains invalid string (\r) in hex encoding too, however it aborts when using write.

I checked now, and it fixes the issue in the comment - or at least it causes an error to get emitted instead of an abort.

@Linkgoron I think a better approach overall would be validating this on the C++ side instead, where the abort occurs in the first place.

Currently it appears to me that the validation in both cases (Hash and Cipher) is already done on the JS side in the update method, but maybe missed in _transform as an oversight (I'll try to go back through the PRs and see what the intent was), and I think that moving the validation to the c++ side would probably mean changing update as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"crypto.createDecipher().end" results in an abort
4 participants