Skip to content

Commit

Permalink
Merge pull request from GHSA-8cf7-32gw-wr33
Browse files Browse the repository at this point in the history
* Check if node version supports asymmetricKeyDetails

* Validate algorithms for ec key type

* Rename variable

* Rename function

* Add early return for symmetric keys

* Validate algorithm for RSA key type

* Validate algorithm for RSA-PSS key type

* Check key types for EdDSA algorithm

* Rename function

* Move validateKey function to module

* Convert arrow to function notation

* Validate key in verify function

* Simplify if

* Convert if to switch..case

* Guard against empty key in validation

* Remove empty line

* Add lib to check modulus length

* Add modulus length checks

* Validate mgf1HashAlgorithm and saltLength

* Check node version before using key details API

* Use built-in modulus length getter

* Fix Node version validations

* Remove duplicate validateKey

* Add periods to error messages

* Fix validation in verify function

* Make asymmetric key validation the latest validation step

* Change key curve validation

* Remove support for ES256K

* Fix old test that was using wrong key types to sign tokens

* Enable RSA-PSS for old Node versions

* Add specific RSA-PSS validations on Node 16 LTS+

* Improve error message

* Simplify key validation code

* Fix typo

* Improve error message

* Change var to const in test

* Change const to let to avoid reassigning problem

* Improve error message

* Test incorrect private key type

* Rename invalid to unsupported

* Test verifying of jwt token with unsupported key

* Test invalid private key type

* Change order of object parameters

* Move validation test to separate file

* Move all validation tests to separate file

* Add prime256v1 ec key

* Remove modulus length check

* WIP: Add EC key validation tests

* Fix node version checks

* Fix error message check on test

* Add successful tests for EC curve check

* Remove only from describe

* Remove `only`

* Remove duplicate block of code

* Move variable to a different scope and make it const

* Convert allowed curves to object for faster lookup

* Rename variable

* Change variable assignment order

* Remove unused object properties

* Test RSA-PSS happy path and wrong length

* Add missing tests

* Pass validation if no algorithm has been provided

* Test validation of invalid salt length

* Test error when signing token with invalid key

* Change var to const/let in verify tests

* Test verifying token with invalid key

* Improve test error messages

* Add parameter to skip private key validation

* Replace DSA key with a 4096 bit long key

* Test allowInvalidPrivateKeys in key signing

* Improve test message

* Rename variable

* Add key validation flag tests

* Fix variable name in Readme

* Change private to public dsa key in verify

* Rename flag

* Run EC validation tests conditionally

* Fix tests in old node versions

* Ignore block of code from test coverage

* Separate EC validations tests into two different ones

* Add comment

* Wrap switch in if instead of having an early return

* Remove unsupported algorithms from asymmetric key validation

* Rename option to allowInvalidAsymmetricKeyTypes and improve Readme

* 9.0.0

* adding migration notes to readme

* adding changelog for version 9.0.0

Co-authored-by: julienwoll <[email protected]>
  • Loading branch information
edgarchirivella-okta and julienwoll authored Dec 21, 2022
1 parent 5eaedbf commit e1fa9dc
Show file tree
Hide file tree
Showing 19 changed files with 557 additions and 90 deletions.
18 changes: 18 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,24 @@
All notable changes to this project will be documented in this file starting from version **v4.0.0**.
This project adheres to [Semantic Versioning](http://semver.org/).

## 9.0.0 - 2022-12-21

**Breaking changes: See [Migration from v8 to v9](https://github.com/auth0/node-jsonwebtoken/wiki/Migration-Notes:-v8-to-v9)**

### Breaking changes

- Removed support for Node versions 11 and below.
- The verify() function no longer accepts unsigned tokens by default. ([834503079514b72264fd13023a3b8d648afd6a16]https://github.com/auth0/node-jsonwebtoken/commit/834503079514b72264fd13023a3b8d648afd6a16)
- RSA key size must be 2048 bits or greater. ([ecdf6cc6073ea13a7e71df5fad043550f08d0fa6]https://github.com/auth0/node-jsonwebtoken/commit/ecdf6cc6073ea13a7e71df5fad043550f08d0fa6)
- Key types must be valid for the signing / verification algorithm

### Security fixes

- security: fixes `Arbitrary File Write via verify function` - CVE-2022-23529
- security: fixes `Insecure default algorithm in jwt.verify() could lead to signature validation bypass` - CVE-2022-23540
- security: fixes `Insecure implementation of key retrieval function could lead to Forgeable Public/Private Tokens from RSA to HMAC` - CVE-2022-23541
- security: fixes `Unrestricted key type could lead to legacy keys usage` - CVE-2022-23539

## 8.5.1 - 2019-03-18

### Bug fix
Expand Down
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ $ npm install jsonwebtoken

# Migration notes

* [From v8 to v9](https://github.com/auth0/node-jsonwebtoken/wiki/Migration-Notes:-v8-to-v9)
* [From v7 to v8](https://github.com/auth0/node-jsonwebtoken/wiki/Migration-Notes:-v7-to-v8)

# Usage
Expand Down Expand Up @@ -52,6 +53,7 @@ When signing with RSA algorithms the minimum modulus length is 2048 except when
* `keyid`
* `mutatePayload`: if true, the sign function will modify the payload object directly. This is useful if you need a raw reference to the payload after claims have been applied to it but before it has been encoded into a token.
* `allowInsecureKeySizes`: if true allows private keys with a modulus below 2048 to be used for RSA
* `allowInvalidAsymmetricKeyTypes`: if true, allows asymmetric keys which do not match the specified algorithm. This option is intended only for backwards compatability and should be avoided.



Expand Down Expand Up @@ -158,7 +160,7 @@ As mentioned in [this comment](https://github.com/auth0/node-jsonwebtoken/issues
> Eg: `1000`, `"2 days"`, `"10h"`, `"7d"`. A numeric value is interpreted as a seconds count. If you use a string be sure you provide the time units (days, hours, etc), otherwise milliseconds unit is used by default (`"120"` is equal to `"120ms"`).
* `clockTimestamp`: the time in seconds that should be used as the current time for all necessary comparisons.
* `nonce`: if you want to check `nonce` claim, provide a string value here. It is used on Open ID for the ID Tokens. ([Open ID implementation notes](https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes))

* `allowInvalidAsymmetricKeyTypes`: if true, allows asymmetric keys which do not match the specified algorithm. This option is intended only for backwards compatability and should be avoided.

```js
// verify a token symmetric - synchronous
Expand Down
3 changes: 3 additions & 0 deletions lib/asymmetricKeyDetailsSupported.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const semver = require('semver');

module.exports = semver.satisfies(process.version, '>=15.7.0');
3 changes: 3 additions & 0 deletions lib/rsaPssKeyDetailsSupported.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const semver = require('semver');

module.exports = semver.satisfies(process.version, '>=16.9.0');
66 changes: 66 additions & 0 deletions lib/validateAsymmetricKey.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
const ASYMMETRIC_KEY_DETAILS_SUPPORTED = require('./asymmetricKeyDetailsSupported');
const RSA_PSS_KEY_DETAILS_SUPPORTED = require('./rsaPssKeyDetailsSupported');

const allowedAlgorithmsForKeys = {
'ec': ['ES256', 'ES384', 'ES512'],
'rsa': ['RS256', 'PS256', 'RS384', 'PS384', 'RS512', 'PS512'],
'rsa-pss': ['PS256', 'PS384', 'PS512']
};

const allowedCurves = {
ES256: 'prime256v1',
ES384: 'secp384r1',
ES512: 'secp521r1',
};

module.exports = function(algorithm, key) {
if (!algorithm || !key) return;

const keyType = key.asymmetricKeyType;
if (!keyType) return;

const allowedAlgorithms = allowedAlgorithmsForKeys[keyType];

if (!allowedAlgorithms) {
throw new Error(`Unknown key type "${keyType}".`);
}

if (!allowedAlgorithms.includes(algorithm)) {
throw new Error(`"alg" parameter for "${keyType}" key type must be one of: ${allowedAlgorithms.join(', ')}.`)
}

/*
* Ignore the next block from test coverage because it gets executed
* conditionally depending on the Node version. Not ignoring it would
* prevent us from reaching the target % of coverage for versions of
* Node under 15.7.0.
*/
/* istanbul ignore next */
if (ASYMMETRIC_KEY_DETAILS_SUPPORTED) {
switch (keyType) {
case 'ec':
const keyCurve = key.asymmetricKeyDetails.namedCurve;
const allowedCurve = allowedCurves[algorithm];

if (keyCurve !== allowedCurve) {
throw new Error(`"alg" parameter "${algorithm}" requires curve "${allowedCurve}".`);
}
break;

case 'rsa-pss':
if (RSA_PSS_KEY_DETAILS_SUPPORTED) {
const length = parseInt(algorithm.slice(-3), 10);
const { hashAlgorithm, mgf1HashAlgorithm, saltLength } = key.asymmetricKeyDetails;

if (hashAlgorithm !== `sha${length}` || mgf1HashAlgorithm !== hashAlgorithm) {
throw new Error(`Invalid key for this operation, its RSA-PSS parameters do not meet the requirements of "alg" ${algorithm}.`);
}

if (saltLength !== undefined && saltLength > length >> 3) {
throw new Error(`Invalid key for this operation, its RSA-PSS parameter saltLength does not meet the requirements of "alg" ${algorithm}.`)
}
}
break;
}
}
}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "jsonwebtoken",
"version": "8.5.1",
"version": "9.0.0",
"description": "JSON Web Token implementation (symmetric and asymmetric)",
"main": "index.js",
"nyc": {
Expand Down
12 changes: 11 additions & 1 deletion sign.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const timespan = require('./lib/timespan');
const PS_SUPPORTED = require('./lib/psSupported');
const validateAsymmetricKey = require('./lib/validateAsymmetricKey');
const jws = require('jws');
const {includes, isBoolean, isInteger, isNumber, isPlainObject, isString, once} = require('lodash')
const { KeyObject, createSecretKey, createPrivateKey } = require('crypto')
Expand All @@ -22,7 +23,8 @@ const sign_options_schema = {
noTimestamp: { isValid: isBoolean, message: '"noTimestamp" must be a boolean' },
keyid: { isValid: isString, message: '"keyid" must be a string' },
mutatePayload: { isValid: isBoolean, message: '"mutatePayload" must be a boolean' },
allowInsecureKeySizes: { isValid: isBoolean, message: '"allowInsecureKeySizes" must be a boolean'}
allowInsecureKeySizes: { isValid: isBoolean, message: '"allowInsecureKeySizes" must be a boolean'},
allowInvalidAsymmetricKeyTypes: { isValid: isBoolean, message: '"allowInvalidAsymmetricKeyTypes" must be a boolean'}
};

const registered_claims_schema = {
Expand Down Expand Up @@ -166,6 +168,14 @@ module.exports = function (payload, secretOrPrivateKey, options, callback) {
return failure(error);
}

if (!options.allowInvalidAsymmetricKeyTypes) {
try {
validateAsymmetricKey(header.alg, secretOrPrivateKey);
} catch (error) {
return failure(error);
}
}

const timestamp = payload.iat || Math.floor(Date.now() / 1000);

if (options.noTimestamp) {
Expand Down
36 changes: 36 additions & 0 deletions test/dsa-private.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
-----BEGIN DSA PRIVATE KEY-----
MIIGWAIBAAKCAgEArzbPbt//BQpsYsnoZR4R9nXgcuvcXoH8WZjRsb4ZPfVJGchG
7CfRMlG0HR34vcUpehNj5pAavErhfNnk1CEal0TyDsOkBY/+JG239zXgRzMYjSE6
ptX5kj5pGv0uXVoozSP/JZblI8/Spd6TZkblLNAYOl3ssfcUGN4NFDXlzmiWvP+q
6ZUgE8tD7CSryicICKmXcVQIa6AG8ultYa6mBAaewzMbiIt2TUo9smglpEqGeHoL
CuLb3e7zLf0AhWDZOgTTfe1KFEiK6TXMe9HWYeP3MPuyKhS20GmT/Zcu5VN4wbr0
bP+mTWk700oLJ0OPQ6YgGkyqBmh/Bsi/TqnpJWS/mjRbJEe3E2NmNMwmP4jwJ79V
JClp5Gg9kbM6hPkmGNnhbbFzn3kwY3pi9/AiqpGyr3GUPhXvP7fYwAu/A5ISKw8r
87j/EJntyIzm51fcm8Q0mq1IDt4tNkIOwJEIc45h9r7ZC1VAKkzlCa7XT04GguFo
JMaJBYESYcOAmbKRojo8P/cN4fPuemuhQFQplkFIM6FtG9cJMo2ayp6ukH9Up8tn
8j7YgE/m9BL9SnUIbNlti9j0cNgeKVn24WC38hw9D8M0/sR5gYyclWh/OotCttoQ
I8ySZzSvB4GARZHbexagvg1EdV93ctYyAWGLkpJYAzuiXbt7FayG7e2ifYkCIQDp
IldsAFGVaiJRQdiKsWdReOSjzH6h8cw6Co3OCISiOQKCAgEAnSU29U65jK3W2BiA
fKTlTBx2yDUCDFeqnla5arZ2njGsUKiP2nocArAPLQggwk9rfqufybQltM8+zjmE
zeb4mUCVhSbTH7BvP903U0YEabZJCHLx80nTywq2RgQs0Qmn43vs2U5EidYR0xj8
CCNAH5gdzd9/CL1RYACHAf7zj4n68ZaNkAy9Jz1JjYXjP6IAxJh1W/Y0vsdFdIJ/
dnuxsyMCUCSwDvSNApSfATO/tw+DCVpGgKo4qE8b8lsfXKeihuMzyXuSe/D98YN2
UFWRTQ6gFxGrntg3LOn41RXSkXxzixgl7quacIJzm8jrFkDJSx4AZ8rgt/9JbThA
XF9PVlCVv7GL1NztUs4cDK+zsJld4O1rlI3QOz5DWq9oA+Hj1MN3L9IW3Iv2Offo
AaubXJhuv0xPWYmtCo06mPgSwkWPjDnGCbp1vuI8zPTsfyhsahuKeW0h8JttW4GB
6CTtC1AVWA1pJug5pBo36S5G24ihRsdG3Q5/aTlnke7t7H1Tkh2KuvV9hD5a5Xtw
cnuiEcKjyR0FWR81RdsAKh+7QNI3Lx75c95i22Aupon5R/Qkb05VzHdd299bb78c
x5mW8Dsg4tKLF7kpDAcWmx7JpkPHQ+5V9N766sfZ+z/PiVWfNAK8gzJRn/ceLQcK
C6uOhcZgN0o4UYrmYEy9icxJ44wCggIBAIu+yagyVMS+C5OqOprmtteh/+MyaYI+
Q3oPXFR8eHLJftsBWev1kRfje1fdxzzx/k4SQMRbxxbMtGV74KNwRUzEWOkoyAHP
AAjhMio1mxknPwAxRjWDOSE0drGJPyGpI9ZfpMUtvekQO7MCGqa45vPldY10RwZC
VN66AIpxSF0MG1OEmgD+noHMI7moclw/nw+ZUPaIFxvPstlD4EsPDkdE0I6x3k3b
UXlWAYAJFR6fNf8+Ki3xnjLjW9da3cU/p2H7+LrFDP+kPUGJpqr4bG606GUcV3Cl
dznoqlgaudWgcQCQx0NPzi7k5O7PXr7C3UU0cg+5+GkviIzogaioxidvvchnG+UU
0y5nVuji6G69j5sUhlcFXte31Nte2VUb6P8umo+mbDT0UkZZZzoOsCpw+cJ8OHOV
emFIhVphNHqQt20Tq6WVRBx+p4+YNWiThvmLtmLh0QghdnUrJZxyXx7/p8K5SE9/
+qU11t5dUvYS+53U1gJ2kgIFO4Zt6gaoOyexTt5f4Ganh9IcJ01wegl5WT58aDtf
hmw0HnOrgbWt4lRkxOra281hL74xcgtgMZQ32PTOy8wTEVTk03mmqlIq/dV4jgBc
Nh1FGQwGEeGlfbuNSB4nqgMN6zn1PmI7oCWLD9XLR6VZTebF7pGfpHtYczyivuxf
e1YOro6e0mUqAiEAx4K3cPG3dxH91uU3L+sS2vzqXEVn2BmSMmkGczSOgn4=
-----END DSA PRIVATE KEY-----
36 changes: 36 additions & 0 deletions test/dsa-public.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
-----BEGIN PUBLIC KEY-----
MIIGSDCCBDoGByqGSM44BAEwggQtAoICAQCvNs9u3/8FCmxiyehlHhH2deBy69xe
gfxZmNGxvhk99UkZyEbsJ9EyUbQdHfi9xSl6E2PmkBq8SuF82eTUIRqXRPIOw6QF
j/4kbbf3NeBHMxiNITqm1fmSPmka/S5dWijNI/8lluUjz9Kl3pNmRuUs0Bg6Xeyx
9xQY3g0UNeXOaJa8/6rplSATy0PsJKvKJwgIqZdxVAhroAby6W1hrqYEBp7DMxuI
i3ZNSj2yaCWkSoZ4egsK4tvd7vMt/QCFYNk6BNN97UoUSIrpNcx70dZh4/cw+7Iq
FLbQaZP9ly7lU3jBuvRs/6ZNaTvTSgsnQ49DpiAaTKoGaH8GyL9OqeklZL+aNFsk
R7cTY2Y0zCY/iPAnv1UkKWnkaD2RszqE+SYY2eFtsXOfeTBjemL38CKqkbKvcZQ+
Fe8/t9jAC78DkhIrDyvzuP8Qme3IjObnV9ybxDSarUgO3i02Qg7AkQhzjmH2vtkL
VUAqTOUJrtdPTgaC4WgkxokFgRJhw4CZspGiOjw/9w3h8+56a6FAVCmWQUgzoW0b
1wkyjZrKnq6Qf1Sny2fyPtiAT+b0Ev1KdQhs2W2L2PRw2B4pWfbhYLfyHD0PwzT+
xHmBjJyVaH86i0K22hAjzJJnNK8HgYBFkdt7FqC+DUR1X3dy1jIBYYuSklgDO6Jd
u3sVrIbt7aJ9iQIhAOkiV2wAUZVqIlFB2IqxZ1F45KPMfqHxzDoKjc4IhKI5AoIC
AQCdJTb1TrmMrdbYGIB8pOVMHHbINQIMV6qeVrlqtnaeMaxQqI/aehwCsA8tCCDC
T2t+q5/JtCW0zz7OOYTN5viZQJWFJtMfsG8/3TdTRgRptkkIcvHzSdPLCrZGBCzR
Cafje+zZTkSJ1hHTGPwII0AfmB3N338IvVFgAIcB/vOPifrxlo2QDL0nPUmNheM/
ogDEmHVb9jS+x0V0gn92e7GzIwJQJLAO9I0ClJ8BM7+3D4MJWkaAqjioTxvyWx9c
p6KG4zPJe5J78P3xg3ZQVZFNDqAXEaue2Dcs6fjVFdKRfHOLGCXuq5pwgnObyOsW
QMlLHgBnyuC3/0ltOEBcX09WUJW/sYvU3O1SzhwMr7OwmV3g7WuUjdA7PkNar2gD
4ePUw3cv0hbci/Y59+gBq5tcmG6/TE9Zia0KjTqY+BLCRY+MOcYJunW+4jzM9Ox/
KGxqG4p5bSHwm21bgYHoJO0LUBVYDWkm6DmkGjfpLkbbiKFGx0bdDn9pOWeR7u3s
fVOSHYq69X2EPlrle3Bye6IRwqPJHQVZHzVF2wAqH7tA0jcvHvlz3mLbYC6miflH
9CRvTlXMd13b31tvvxzHmZbwOyDi0osXuSkMBxabHsmmQ8dD7lX03vrqx9n7P8+J
VZ80AryDMlGf9x4tBwoLq46FxmA3SjhRiuZgTL2JzEnjjAOCAgYAAoICAQCLvsmo
MlTEvguTqjqa5rbXof/jMmmCPkN6D1xUfHhyyX7bAVnr9ZEX43tX3cc88f5OEkDE
W8cWzLRle+CjcEVMxFjpKMgBzwAI4TIqNZsZJz8AMUY1gzkhNHaxiT8hqSPWX6TF
Lb3pEDuzAhqmuObz5XWNdEcGQlTeugCKcUhdDBtThJoA/p6BzCO5qHJcP58PmVD2
iBcbz7LZQ+BLDw5HRNCOsd5N21F5VgGACRUenzX/Piot8Z4y41vXWt3FP6dh+/i6
xQz/pD1Biaaq+GxutOhlHFdwpXc56KpYGrnVoHEAkMdDT84u5OTuz16+wt1FNHIP
ufhpL4iM6IGoqMYnb73IZxvlFNMuZ1bo4uhuvY+bFIZXBV7Xt9TbXtlVG+j/LpqP
pmw09FJGWWc6DrAqcPnCfDhzlXphSIVaYTR6kLdtE6ullUQcfqePmDVok4b5i7Zi
4dEIIXZ1KyWccl8e/6fCuUhPf/qlNdbeXVL2Evud1NYCdpICBTuGbeoGqDsnsU7e
X+Bmp4fSHCdNcHoJeVk+fGg7X4ZsNB5zq4G1reJUZMTq2tvNYS++MXILYDGUN9j0
zsvMExFU5NN5pqpSKv3VeI4AXDYdRRkMBhHhpX27jUgeJ6oDDes59T5iO6Aliw/V
y0elWU3mxe6Rn6R7WHM8or7sX3tWDq6OntJlKg==
-----END PUBLIC KEY-----
Loading

11 comments on commit e1fa9dc

@nickhobbs94
Copy link

Choose a reason for hiding this comment

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

NIST has given the CVE for this at 9.8 severity. NIST CVE-2022-23540
Based on the comments here that seems way too high.

Is it correct that a secretOrPublicKey must not be specified for the algorithm to be set to none? Could the advisory be updated to note this? Right now it says everybody is vulnerable if they don't specify the algorithm.

@julienwoll tagging you since you published the advisory.

@julienwoll
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @nickhobbs94, that was a great feedback, we updated the advisory: GHSA-qwph-4952-7xr6

@leumasme
Copy link

@leumasme leumasme commented on e1fa9dc Jan 9, 2023

Choose a reason for hiding this comment

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

Is it just me or is CVE-2022-23529 / GHSA-27h2-hvpr-p74q complete nonsense?

The entire "vulnerability" is based on an object with a malicious toString function being passed - but the very existance of a malicious toString function should not happen in a sane situation.
If any deserializer allows creation of arbitrary functions, it's the deserializer that has a vulnerability - not a library that calls toString on the deserialized object (especially when the library explicitly states that
the argument should be of type String or Buffer, as this library does).

By this definition of a "vulnerability", even JSON.parse, which should obviously be safe for untrusted input, would be vulnerable

let x = {
    toString: () => console.error("PWNED !!11")
};
JSON.parse(x); // Logs "PWNED !!11"

Also taking the nonsensical "PoC" code into account (running code by creating a process exit event handler, then exiting the process instead of just running the code),
and the fact that this seems to have been submitted as "Arbitrary File Write via verify function" when it would just be a full RCE if it were real,
makes it seem like this was submitted by a complete beginner.

At the same time, the GHSA is marked as "GitHub Reviewed" so it somehow passed some sort of manual check?
Someone please enlighten me if this is nonsense or i'm just going insane.

@LaurenceJJones
Copy link

Choose a reason for hiding this comment

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

By this definition of a "vulnerability", even JSON.parse, which should obviously be safe for untrusted input, would be vulnerable

> let obj = { toString: ()=> {console.log("PWNED!!!");process.on('exit', () => {require('fs').writeFileSync('/tmp/owned.txt', 'Arb write on nodejs!!');});process.exit(0)}}
undefined
> JSON.parse(obj)
PWNED!!!
$ cat /tmp/owned.txt
───────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: owned.txt
───────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ Arb write on nodejs!!

existance of a malicious toString function should not happen in a sane situation.

Exactly unless there some sort of prototype pollution but in that case it would be a vulnerable application than the package but if we want to make sure then let the package take the reigns

@eoftedal
Copy link

Choose a reason for hiding this comment

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

This would even make console.log in node.js vulnerable:

let obj = { toString: ()=> {console.log("PWNED!!!");process.on('exit', () => {require('fs').writeFileSync('/tmp/owned.txt', 'Arb write on nodejs!!');});process.exit(0)}}
console.log("Look at this: " + obj)

The CVSS score of 7.6 from Github and 9.8 is NVD is completely overblown. If you can exploit this you already have RCE, because you can already supply code.

@leumasme
Copy link

Choose a reason for hiding this comment

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

This would even make console.log in node.js vulnerable:

Actually, in your example the string concatenation "x" + obj causes this, not console.log

@MichaelErmer
Copy link

Choose a reason for hiding this comment

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

@leumasme yes, its completely made up github/advisory-database#1595

@stypr
Copy link

@stypr stypr commented on e1fa9dc Jan 11, 2023

Choose a reason for hiding this comment

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

Nice bug, I wish there was Legit Impact on Impact Metrics for CVSS..

@eoftedal
Copy link

Choose a reason for hiding this comment

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

@leumasme You are right, and that actually makes it worse 😄

@ilyash-b
Copy link

Choose a reason for hiding this comment

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

Is it just me or is CVE-2022-23529 / GHSA-27h2-hvpr-p74q complete nonsense?

I suspect it was ChatGPT generated

@Uzlopak
Copy link

Choose a reason for hiding this comment

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

const malicious = {
    toString: () => console.error("PWNED !!11")
};

'' + malicious

So there is a sec. Vuln. in nodejs?
Lol.

Please sign in to comment.