Skip to content

crypto: fix Hash and Cipher abort on end - #38425

Open
Linkgoron wants to merge 1 commit into
nodejs:mainfrom
Linkgoron:crypto-cipher-hash-end-abort
Open

crypto: fix Hash and Cipher abort on end#38425
Linkgoron wants to merge 1 commit into
nodejs:mainfrom
Linkgoron:crypto-cipher-hash-end-abort

Conversation

@Linkgoron

@LinkgoronLinkgoron commented Apr 26, 2021

Copy link
Copy Markdown
Contributor

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-actionsgithub-actionsBot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Apr 26, 2021
Comment threadtest/parallel/test-crypto-binary-default.js Outdated
Comment threadtest/parallel/test-crypto-binary-default.js Outdated
Comment threadlib/internal/crypto/cipher.js Outdated
@Ayase-252

Copy link
Copy Markdown
Member

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

Comment threadlib/internal/crypto/cipher.js
@addaleax

Copy link
Copy Markdown
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
Linkgoronforce-pushed the crypto-cipher-hash-end-abort branch from 5e277fd to b89f9abCompareApril 27, 2021 07:45
@Linkgoron

Copy link
Copy Markdown
ContributorAuthor

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.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cryptoIssues and PRs related to the crypto subsystem.needs-ciPRs that need a full CI run.stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants

@Linkgoron@Ayase-252@addaleax@aduh95