🔒 fix: CVE-2023-26115 #33

aashutoshrathi · 2023-03-25T06:03:31Z

Fixes #32

Issue with existing approach

CVE-2023-26115
TLDR; ReDoS, the issue is due to Regex matching algo.

Approach used here

Removed the use of String.prototype.replace(Regex), instead wrote function to trim tabs and space from the end of line.

Benchmarks

Code snippet used

const wrap = require("word-wrap");

const trimTabAndSpaces = (str) => {
    const lines = str.split('\n');
    const trimmedLines = lines.map(line => {
      let i = line.length - 1;
      while (i >= 0 && (line[i] === ' ' || line[i] === '\t')) {
        i--;
      }
      return line.substring(0, i + 1);
    });
    return trimmedLines.join('\n'); 
}


for (let i = 0; i <= 10; i++) {
    const attack = "a" + "t".repeat(i * 10_00000);
    const start = performance.now();
    attack.replace(/[ \t]*$/gm, ''); // -> since this is happening inside code.
    // wrap(attack, { trim: true });
    console.log(`wrap.trim: ${attack.length} characters: ${performance.now() - start}ms`);
}

for (i = 0; i <= 10; i++) {
    const attack = "a" + "t".repeat(i * 10_00000);
    const start2 = performance.now();
    trimTabAndSpaces(attack);
    console.log(`trimTabAndSpaces: ${attack.length} characters: ${performance.now() - start2}ms`);
}

Performance Benchmarks

node test.js
wrap.trim: 1 characters: 0.015165984630584717ms
wrap.trim: 1000001 characters: 6.47087499499321ms
wrap.trim: 2000001 characters: 13.000333994626999ms
wrap.trim: 3000001 characters: 19.574499994516373ms
wrap.trim: 4000001 characters: 26.035459011793137ms
wrap.trim: 5000001 characters: 31.902458995580673ms
wrap.trim: 6000001 characters: 38.429375022649765ms
wrap.trim: 7000001 characters: 44.44049999117851ms
wrap.trim: 8000001 characters: 51.241291999816895ms
wrap.trim: 9000001 characters: 57.14437499642372ms
wrap.trim: 10000001 characters: 63.937458992004395ms
----------
trimTabAndSpaces: 1 characters: 0.3221670091152191ms
trimTabAndSpaces: 1000001 characters: 0.2049579918384552ms
trimTabAndSpaces: 2000001 characters: 0.3135409951210022ms
trimTabAndSpaces: 3000001 characters: 0.3718339800834656ms
trimTabAndSpaces: 4000001 characters: 0.7508749961853027ms
trimTabAndSpaces: 5000001 characters: 0.6123340129852295ms
trimTabAndSpaces: 6000001 characters: 0.7714170217514038ms
trimTabAndSpaces: 7000001 characters: 1.168584018945694ms
trimTabAndSpaces: 8000001 characters: 0.9755829870700836ms
trimTabAndSpaces: 9000001 characters: 1.406792014837265ms
trimTabAndSpaces: 10000001 characters: 1.5417499840259552ms

sergeyt · 2023-03-25T09:49:28Z

index.js

@@ -36,7 +42,7 @@ module.exports = function(str, options) {
  }).join(newline);

  if (options.trim === true) {
-    result = result.replace(/[ \t]*$/gm, '');


I guess /\s+$/gm works faster

Thanks, let me check on that.
But ideally, I would like to not use regex for the operation that can be easily handled by native trimEnd, right?

maybe or native better regex can be faster. need to bench to check that

Hi @aashutoshrathi, sorry for the long delay, and thanks for the PR. I'll try to get this merged in ASAP.

I guess /\s+$/gm works faster

Yes, let's use this pattern without the m flag. Using + will also be a bit safer since it won't allow that pattern to match an empty string.

/[ \t]+$/g is slightly better than /\s+$/g it would seem. Unfortunately both regexes are suspectable to redos, as opposed to trimEnd();

$ time node -e 'payloadstr=""; for ( i=0; i < 100000; i++ ) payloadstr+="\t"; payloadstr+="a"; payloadstr.replace(/[ \t]+$/gm,"");'
real 0m21.084s
$ time node -e 'payloadstr=""; for ( i=0; i < 100000; i++ ) payloadstr+="\t"; payloadstr+="a"; payloadstr.replace(/\s+$/g,"");'
real 0m8.587s
$ time node -e 'payloadstr=""; for ( i=0; i < 100000; i++ ) payloadstr+="\t"; payloadstr+="a"; payloadstr.replace(/[ \t]+$/g,"");'
real 0m6.169s
$ time node -e 'payloadstr=""; for ( i=0; i < 100000; i++ ) payloadstr+="\t"; payloadstr+="a"; payloadstr.trimEnd();'
real 0m0.082s

@jonschlinkert Any update when this PR will be merged ?

gurungkiran · 2023-03-27T08:57:46Z

index.js

 * Released under the MIT License.
 */

+function trimTabAndSpaces(str) {
+  const lines = str.split('\n');


may be using method chaining eliminates the need to create an intermediate variable ??
return str.split('\n').map(line => line.trimEnd()).join('\n');

yup! can be done. I once just need verification the approach or whether it is important or not

utilizing native String.prototype.trimEnd() would require the node-engine to be at least on version 10.0.0.

Ref: String.prototype.trimEnd() - JavaScript | MDN

This should probably considered a breaking-change which requires new major version release.
Hence I would recommend to utilize custom implementation for this patch.

oh! I can write trimEnd myself if that's the case

For the reason mentioned by @mscheid-sf, for now let's merge in the regex I suggested in my other comment. After that I'd be happy to take another PR that replaces the regex with .trimEnd() (Using .trimEnd would indeed qualify as a breaking change according to semver, and also it's generally a good practice to minimize potential for regressions when releasing patches).

lepetitpatron · 2023-03-27T15:20:07Z

Do you have an update when this PR will be merged? Thanks!

jainanuj0812 · 2023-04-12T05:57:26Z

Any update?

sergeyt · 2023-04-12T06:01:32Z

I suggest going with /\s+$/gm faster regexp first. Previous one is slower since it uses * quantifier which includes empty replacements as well which is redundant ops

aashutoshrathi · 2023-04-14T07:56:14Z

@jonschlinkert is this repo active?

MateuszKikmunter · 2023-04-14T12:01:51Z

Any update on this @jonschlinkert ? We're blocked by this issue not being fixed.

xavisegura · 2023-04-14T13:46:23Z

Any update on this @jonschlinkert ? Is this repo active?

sergeyt · 2023-04-14T14:45:04Z

Any update on this @jonschlinkert ? Is this repo active?

in meantime someone brave enough can publish a version of the package that can be a replacement in package.json. for example, like we can do for class-validator:

"class-transformer": "npm:@nestjs/class-transformer@0.4.0",
"class-validator": "npm:@nestjs/class-validator@0.13.4",

aashutoshrathi · 2023-04-14T14:49:58Z

Any update on this @jonschlinkert ? Is this repo active?

in meantime someone brave enough can publish a version of the package that can be a replacement in package.json. for example, like we can do for class-validator:
"class-transformer": "npm:@nestjs/class-transformer@0.4.0",
"class-validator": "npm:@nestjs/class-validator@0.13.4",

I think we can try, what do you think @sergeyt ?

sergeyt · 2023-04-14T15:11:05Z

Any update on this @jonschlinkert ? Is this repo active?

in meantime someone brave enough can publish a version of the package that can be a replacement in package.json. for example, like we can do for class-validator:
"class-transformer": "npm:@nestjs/class-transformer@0.4.0",
"class-validator": "npm:@nestjs/class-validator@0.13.4",
I think we can try, what do you think @sergeyt ?

Sure, why not

aashutoshrathi · 2023-04-15T09:36:31Z

Okay here's a temporary package: https://www.npmjs.com/package/@aashutoshrathi/word-wrap
In package.json, the below should do it.

"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.4"
  },

jainanuj0812 · 2023-04-16T16:49:35Z

Okay here's a temporary package: https://www.npmjs.com/package/@aashutoshrathi/word-wrap In package.json, the below should do it.
"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.4"
  },

and if you don't use yarn and using npm itself use 'overrides'

aashutoshrathi · 2023-04-17T17:47:30Z

Okay here's a temporary package: https://www.npmjs.com/package/@aashutoshrathi/word-wrap In package.json, the below should do it.
"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.4"
  },

Please use the newer version the older one causes some issues

"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.5"
  },

andreidiaconescu · 2023-04-25T11:44:34Z

If we use npm not yarn, how would we use overrides to change a package with another ? i do not find it clear from the official docs: https://docs.npmjs.com/cli/v9/configuring-npm/package-json#overrides
Thank you.

sfc-gh-dszmolka · 2023-04-25T11:56:59Z

If we use npm not yarn, how would we use overrides to change a package with another ? i do not find it clear from the official docs: https://docs.npmjs.com/cli/v9/configuring-npm/package-json#overrides Thank you.

for example, using something like this in your package.json:

"overrides": {
    "word-wrap" : "npm:@aashutoshrathi/word-wrap@1.2.5"
  },

DiegoTavelli · 2023-04-27T16:35:07Z

Okay here's a temporary package: https://www.npmjs.com/package/@aashutoshrathi/word-wrap In package.json, the below should do it.
"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.4"
  },
Please use the newer version the older one causes some issues
"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.5"
  },

I will try with this one, I have a question. Becouse that dependency is involved into eslint and jest dependecies, I have to specify from the example parent dependency. 'jest/jest-config.../word-wrap' or just specify as you say ?

aashutoshrathi · 2023-04-27T16:44:13Z

You can use the same, without specifying the path.

velsonjr · 2023-05-03T10:43:48Z

Please have this merged to the main branch

jainanuj0812 · 2023-05-03T11:04:09Z

Thanks @aashutoshrathi ,

But still, this needs to be merged..Can anyone from us reach out to @jonschlinkert through LinkedIn/Twitter please :-P

velsonjr · 2023-05-29T10:14:52Z

@aashutoshrathi , The same is still reappearing for the following path, react-scripts@5.0.1 › jest@27.5.1 › @jest/core@27.5.1 › jest-config@27.5.1 › jest-environment-jsdom@27.5.1 › jsdom@16.7.0 › escodegen@2.0.0 › optionator@0.8.3 › word-wrap@1.2.5

aashutoshrathi · 2023-05-29T10:35:57Z

@aashutoshrathi , The same is still reappearing for the following path, react-scripts@5.0.1 › jest@27.5.1 › @jest/core@27.5.1 › jest-config@27.5.1 › jest-environment-jsdom@27.5.1 › jsdom@16.7.0 › escodegen@2.0.0 › optionator@0.8.3 › word-wrap@1.2.5

Hey! if you use resolution override in the main package.json, it shouldn't happen

velsonjr · 2023-06-07T15:02:44Z

Hey @aashutoshrathi, In case do another npm i, it throws me this npm ERR! Invalid Version: npm:@aashutoshrathi/word-wrap@1.2.5 . I have the added the same inside resolutions. Also when I upgrade the react, node and npm versions, the scan still shows the vulnerability ( In my case its react 18.2, node 16.20 and npm 8.19)

xx745 · 2023-06-08T11:20:27Z

Hi all, why is this fix not merged yet?

bgswilde · 2023-06-08T15:55:13Z

@xx745, I don't think @jonschlinkert has any desire to continue managing this repo. I've reached out to him several times on twitter, where he's active, but no response. I would love to be wrong though! I would be great for him to come in and merge this or for there to be a way for somebody else to be an admin here, then he doesn't have to keep up with it.

wellwelwel · 2023-07-19T12:02:04Z

@aashutoshrathi, since this PR has been merged and this Issue was resolved with PR #41, I'm backing to the original project now.

Thank you for bringing a solution until this problem was actually solved here, I believe your fork has helped many people and keep helping (the numbers themselves prove this) 🚀

aashutoshrathi · 2023-07-19T12:36:32Z

@aashutoshrathi, since this PR has been merged and this Issue was resolved with PR #41, I'm backing to the original project now.

Thank you for bringing a solution until this problem was actually solved here, I believe your fork has helped many people and keep helping (the numbers themselves prove this) 🚀

No problemo anytime for OSS folks! 🖖🏻

bgswilde · 2023-07-19T15:54:50Z

Thanks @nicholas-quirk-mass-gov , I was just coming here to comment.

We choose to go with #41 since it's using the inline trimEnd implementation for backwards compatibility. That PR was branched off of this PR, so GitHub automatically merged in the changes here when I merged in the other PR.

I'm publishing word-wrap@1.2.4 in a few minutes with the fix from #41.

@doowb and @nicholas-quirk-mass-gov, just asking for clarity. @nicholas-quirk-mass-gov, are you saying that PR #41 (1.2.4) didn't fix the vulnerability?

aashutoshrathi · 2023-07-19T16:00:56Z

Thanks @nicholas-quirk-mass-gov , I was just coming here to comment.
We choose to go with #41 since it's using the inline trimEnd implementation for backwards compatibility. That PR was branched off of this PR, so GitHub automatically merged in the changes here when I merged in the other PR.
I'm publishing word-wrap@1.2.4 in a few minutes with the fix from #41.

@doowb and @nicholas-quirk-mass-gov, just asking for clarity. @nicholas-quirk-mass-gov, are you saying that PR #41 (1.2.4) didn't fix the vulnerability?

Yes! This PR won't fix it. until we leave Regex completely.
My first commit was better than the second after the requested changes, IMO won't fix it.

wellwelwel · 2023-07-19T16:13:48Z

@bgswilde, understanding the storyline 🧙🏻

This PR changes only the regex rule:

- result = result.replace(/[ \t]*$/gm, '');
+ result = result.replace(/\s+$/g, '');

The @aashutoshrathi/word-wrap fork uses the native trimEnd.

The PR #41 recreates the trimTabAndSpaces from @aashutoshrathi/word-wrap fork removing the regex rules and adding a custom trimEnd looking for backwards compatibility.

Both @aashutoshrathi/word-wrap fork and PR #41 fix that vulnerability, this PR (#33) don't.

For a better understand, please see the #41 changes.

🔒 fix: CVE-2023-26115

235cfd4

aashutoshrathi mentioned this pull request Mar 25, 2023

Regular Expression Denial of Service (ReDoS) - CVE-2023-26115 #32

Closed

sergeyt reviewed Mar 25, 2023

View reviewed changes

gurungkiran reviewed Mar 27, 2023

View reviewed changes

sfc-gh-dszmolka mentioned this pull request Mar 30, 2023

Snyk: snowflake-connector-nodejs word-wrap 1.2.3 | Snyk ID - SNYK-JS-WORDWRAP-3149973 snowflakedb/snowflake-connector-nodejs#454

Closed

MateuszKikmunter mentioned this pull request Apr 14, 2023

Regular Expression Denial of Service (ReDoS) vulnerability jsdom/jsdom#3527

Closed

icbat mentioned this pull request Apr 17, 2023

Regular Expression Denial of Service (ReDoS) on dependency 'word-wrap' gkz/optionator#44

Closed

Pittufo mentioned this pull request Jul 19, 2023

Bump word-wrap from 1.2.3 to 1.2.4 jbelien/MapFile-Generator#405

Open

lentz mentioned this pull request Jul 19, 2023

Bump word-wrap from 1.2.3 to 1.2.4 lentz/youtube-shuffle#237

Merged

davidshen84 mentioned this pull request Jul 20, 2023

build(deps-dev): bump word-wrap from 1.2.3 to 1.2.4 davidshen84/davidshen84.github.io#86

Merged

louiechristie mentioned this pull request Jul 20, 2023

build(deps): bump word-wrap from 1.2.3 to 1.2.4 louiechristie/weather-winton#65

Merged

andrejkurusiov mentioned this pull request Jul 20, 2023

build(deps): bump word-wrap from 1.2.3 to 1.2.4 andrejkurusiov/react-cards-users#21

Merged

rkokkelk mentioned this pull request Jul 20, 2023

Bump word-wrap from 1.2.3 to 1.2.4 rkokkelk/com.jreenen.homeyharmonyhub#116

Merged

bolt-io mentioned this pull request Jul 20, 2023

Regular Expression Denial of Service #44

Closed

neferin12 mentioned this pull request Jul 20, 2023

build(deps): bump word-wrap from 1.2.3 to 1.2.4 educorvi/timeclicker#39

Merged

This was referenced Jul 20, 2023

Bump word-wrap from 1.2.3 to 1.2.4 in /frontend HelloThisIsFlo/Professional-Beginner#17

Merged

chore(deps): bump word-wrap from 1.2.3 to 1.2.4 HelloThisIsFlo/gatsby-starter-netlify-cms#13

Merged

Uriei mentioned this pull request Jul 20, 2023

Build(deps-dev): Bump word-wrap from 1.2.3 to 1.2.4 Uriei/psi-bot#50

Merged

edgeboyo mentioned this pull request Jul 20, 2023

Bump word-wrap from 1.2.3 to 1.2.4 edgeboyo/vox_populi_actio_dei#15

Merged

instapr bot mentioned this pull request Jul 20, 2023

Bump word-wrap from 1.2.3 to 1.2.4 ammar-knowledge/node-jira-client#15

Merged

hamsbrar mentioned this pull request Jul 22, 2023

Bump word-wrap from 1.2.3 to 1.2.4 in /packages/text_editor_vscode_extension erlage/rad#30

Merged

This was referenced Jul 31, 2023

Bump word-wrap from 1.2.3 to 1.2.5 in /platform/ui ossrs/oryx#108

Closed

Bump word-wrap from 1.2.3 to 1.2.5 in /ui ossrs/oryx#115

Merged

sync-by-unito bot mentioned this pull request Jan 31, 2024

chore(deps): bump word-wrap from 1.2.3 to 1.2.4 emulsify-ds/emulsify-website#115

Closed

ericmorand-sonarsource mentioned this pull request Mar 22, 2024

Check vulnerabilities raised for SonarJS 9.13 SonarSource/SonarJS#4598

Closed

alexisdiel mentioned this pull request Oct 1, 2024

Bump word-wrap from 1.2.3 to 1.2.5 alexisdiel/winejs#10

Merged

🔒 fix: CVE-2023-26115 #33

🔒 fix: CVE-2023-26115 #33

Conversation

aashutoshrathi commented Mar 25, 2023 • edited Loading

Issue with existing approach

Approach used here

Benchmarks

Code snippet used

Performance Benchmarks

Choose a reason for hiding this comment

aashutoshrathi Mar 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sig-holec Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lepetitpatron commented Mar 27, 2023

jainanuj0812 commented Apr 12, 2023

sergeyt commented Apr 12, 2023

aashutoshrathi commented Apr 14, 2023

MateuszKikmunter commented Apr 14, 2023

xavisegura commented Apr 14, 2023

sergeyt commented Apr 14, 2023

aashutoshrathi commented Apr 14, 2023

sergeyt commented Apr 14, 2023

aashutoshrathi commented Apr 15, 2023

jainanuj0812 commented Apr 16, 2023

aashutoshrathi commented Apr 17, 2023

andreidiaconescu commented Apr 25, 2023

sfc-gh-dszmolka commented Apr 25, 2023

DiegoTavelli commented Apr 27, 2023

aashutoshrathi commented Apr 27, 2023

velsonjr commented May 3, 2023

jainanuj0812 commented May 3, 2023

velsonjr commented May 29, 2023

aashutoshrathi commented May 29, 2023

velsonjr commented Jun 7, 2023 • edited Loading

xx745 commented Jun 8, 2023

bgswilde commented Jun 8, 2023

wellwelwel commented Jul 19, 2023 • edited Loading

aashutoshrathi commented Jul 19, 2023

bgswilde commented Jul 19, 2023

aashutoshrathi commented Jul 19, 2023 • edited Loading

wellwelwel commented Jul 19, 2023 • edited Loading

aashutoshrathi commented Mar 25, 2023 •

edited

Loading

aashutoshrathi Mar 25, 2023 •

edited

Loading

sig-holec Jun 9, 2023 •

edited

Loading

velsonjr commented Jun 7, 2023 •

edited

Loading

wellwelwel commented Jul 19, 2023 •

edited

Loading

aashutoshrathi commented Jul 19, 2023 •

edited

Loading

wellwelwel commented Jul 19, 2023 •

edited

Loading