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 Cipher/Decipher validation issues #45189

Open
targos opened this issue Oct 26, 2022 · 2 comments
Open

crypto Cipher/Decipher validation issues #45189

targos opened this issue Oct 26, 2022 · 2 comments
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.

Comments

@targos
Copy link
Member

targos commented Oct 26, 2022

This also affects Decipher methods.

Here's an example:

const { createCipheriv, randomBytes } = require('crypto');

const cipher = createCipheriv('aes-256-cbc', randomBytes(32), randomBytes(16));

cipher.update('test', 'utf-8', 'bad');

Gives an internal assertion:

Error [ERR_INTERNAL_ASSERTION]: Cannot change encoding
This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
Please open an issue with this stack trace at https://github.com/nodejs/node/issues

final has the same issue:

const { createCipheriv, randomBytes } = require('crypto');

const cipher = createCipheriv('aes-256-cbc', randomBytes(32), randomBytes(16));

cipher.update('test', 'utf-8', 'utf-8');
cipher.final('bad');

Other issue:

const { createCipheriv, randomBytes } = require('crypto');

const cipher = createCipheriv('aes-256-cbc', randomBytes(32), randomBytes(16));

let result = cipher.update('test', 'bad', 'hex');
result += cipher.final('hex');
console.log(result);

Gives no error at all for the wrong input encoding.

@nodejs/crypto

@targos targos added confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. labels Oct 26, 2022
@kivancbilen
Copy link

Would it make sense to introduce a new error like ERR_CRYPTO_UNKNOWN_ENCODING with a message Unknown encoding {wrong encoding name}?

vitpavlenko added a commit to vitpavlenko/node that referenced this issue Dec 27, 2022
Adds encoding validation to update and final cipher methods.

nodejs#45189
vitpavlenko added a commit to vitpavlenko/node that referenced this issue Dec 28, 2022
Adds encoding validation to update and final cipher methods.

nodejs#45189
vitpavlenko added a commit to vitpavlenko/node that referenced this issue Dec 28, 2022
Adds encoding validation to update and final cipher methods.

nodejs#45189
vitpavlenko added a commit to vitpavlenko/node that referenced this issue Jan 1, 2023
Adds encoding validation to update and final cipher methods.

nodejs#45189
@panva
Copy link
Member

panva commented Jan 6, 2023

I'm afraid the last mentioned "other issue" is also present in Sign , Verify, Hash, Hmac, and the like. I also suspect hard validating may be breaking for a lot of legacy applications.

I suggest to deal with the first two in #45990 and leaving the inputEncoding validation to a separate PR and CITGM to decide whether it's worth fixing or not.

Also refs: #31766

panva pushed a commit to vitpavlenko/node that referenced this issue Jan 12, 2023
panva pushed a commit to vitpavlenko/node that referenced this issue Jan 12, 2023
nodejs-github-bot pushed a commit that referenced this issue Jan 17, 2023
Refs #45189

PR-URL: #45990
Refs: #45189
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
RafaelGSS pushed a commit that referenced this issue Jan 20, 2023
Refs #45189

PR-URL: #45990
Refs: #45189
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
juanarbol pushed a commit that referenced this issue Jan 26, 2023
Refs #45189

PR-URL: #45990
Refs: #45189
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
juanarbol pushed a commit that referenced this issue Jan 31, 2023
Refs #45189

PR-URL: #45990
Refs: #45189
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants