fix-vuln: patch cross-spawn to fix ReDoS vulnerability

[draialexis]

Hi! Thanks for putting open-source code out there. I appreciate you.

Context

I analyzed one of my projects on Snyk and noticed a high vulnerability from somewhere down my dependency chain.

@tsoa/[email protected] › [email protected] › [email protected] › [email protected]

It turns out cross-spawn < 7.0.5 is the problem. The fix is as simple as to update to that patch number or above. (Source: https://security.snyk.io/vuln/SNYK-JS-CROSSSPAWN-8303230)

What this PR accomplishes

This PR updates the cross-spawn dependency to its lates patch version number: 7.0.5, published two days ago.

Approach and tests

I've simply ran npm audit fix on the project, then npm test. Here's the output I saw:

> [email protected] pretest
> npm run prepare


> [email protected] prepare
> tshy


> [email protected] test
> tap

 PASS  test/normalize-fg-args.ts 112 OK 4.253s
 PASS  test/immortal-child.ts 1 OK 5.114s
 PASS  test/basic.ts 47 OK 5.820s
 PASS  test/change-exit.ts 45 OK 6.273s

                       
  🌈 TEST COMPLETE 🌈  
                       

Asserts:  205 pass  0 fail  205 of 205 complete
Suites:     4 pass  0 fail      4 of 4 complete

# { total: 205, pass: 205 }
# time=6412.587ms

Warning: Detected unsettled top-level await at file:///home/alexis/Documents/Dev/foreground-child/node_modules/tap/dist/esm/run.mjs:16
await import(String(await resolveImport('@tapjs/run', await resolveImport('tap', pathToFileURL(resolve('x'))).catch(() => import.meta.url))));
^

📝 The warning also appears on main

[draialexis]

Please let me know if there's anything I can do to help get this merged

[davidecolussi]

+1

[raveenita]

+1

[henriquecustodia]

Any news?

[draialexis]

From the commit history, it seems like only @isaacs is authoring and committing in this project these past few years.

He's also involved in other major projects (like glob and npm), so my assumption is that he's busy and will get to this when possible.

[draialexis]

I have just nudged cross-spawn up to the new patch (7.0.6) in this PR.

Running npm test I see all tests OK:

npm test               

> [email protected] pretest
> npm run prepare


> [email protected] prepare
> tshy


> [email protected] test
> tap

 PASS  test/normalize-fg-args.ts 112 OK 3.833s
 PASS  test/immortal-child.ts 1 OK 5.117s
 PASS  test/basic.ts 47 OK 5.412s
 PASS  test/change-exit.ts 45 OK 5.952s

                       
  🌈 TEST COMPLETE 🌈  
                       

Asserts:  205 pass  0 fail  205 of 205 complete
Suites:     4 pass  0 fail      4 of 4 complete

# { total: 205, pass: 205 }
# time=6115.113ms

[newtonkwan]

+1

[LunaticMuch]

+1

[kerz1488]

+1

[humam-nameer-10p]

bump

[axelcoezard]

Can you validate and merge that fix please ?

[claire-gallesio]

+1

[brandonfl]

Severity : High 7.5 / 10
EPSS score: 0.045% (17th percentile)
Weaknesses : https://github.com/advisories?query=cwe%3A1333
CVE ID : CVE-2024-21538
GHSA ID : GHSA-3xgq-45jj-v275

Versions of the package cross-spawn before 7.0.5 are vulnerable to Regular Expression Denial of Service (ReDoS) due to improper input sanitization. An attacker can increase the CPU usage and crash the program by crafting a very large and well crafted string.

[FlorentComba]

It's OK 👍

[draialexis]

Thanks for all the approvals. What kind of timeline are we looking at, for a merge?

[axelcoezard]

We hope that it will be merged soon, thanks to @pablokurskii !
Why not to merge it today ?

[draialexis]

We hope that it will be merged soon, thanks to @pablokurskii !

Why not to merge it today ?

None of the 4 reviewers who have approved this MR so far, have write-access in this repo

[axelcoezard]

Waiting final approval from @isaacs, @ljharb, or @bcoe 🙌

[ljharb]

To be clear, this PR is entirely unnecessary because it's using ^, a semver range. All you all need to do is update your lockfiles.

[valvfr]

To be clear, this PR is entirely unnecessary because it's using ^, a semver range. All you all need to do is update your lockfiles.

hi @ljharb, I know that it's using ^, but even with that prefix. I'm facing that npm is installed in my docker containers and this instead of using latest minor version 7, it's still installing v7.0.3
It may not hurt to enforce installing this subpackage since v7.0.5

[ljharb]

Updating this package won't cause npm to be updated, though, because npm bundles its deps - meaning, you can ask npm to do the update in its next version without any changes in this package.

It's not that it hurts to make this change - it's that literally anyone who thinks this PR will help them is misinformed, and is already empowered to help themselves (except if you depend on npm, but npm shouldn't be available in production anyways)

[draialexis]

Thanks for sharing your insights @ljharb

I see CVEs with high scores flying around on open source projects, and I figure I can try to give a hand, basically.

This PR is about fixing the foreground-child vuln. If and when the fix is released, I'll open a similar PR in the glob repo. Then I'll open a PR in the tsoa repo.

Even with what you say about npm, I don't know that this PR is "entirely unnecessary". If you disagree please say so -- I'm willing to listen and I'm trying to help

[ljharb]

foreground-child doesn’t have a vuln - that’s not how vulnerabilities work. only cross-spawn does, and you can update it in your own application to no longer be vulnerable. None of the dependencies in between your app and cross-spawn need to be touched, or bothered, or helped - that’s the entire point of semver ranges, so that everyone can quietly just fix their own problems whenever they come up without volunteer unpaid maintainers needing to put in additional time.

[draialexis]

foreground-child doesn’t have a vuln - only cross-spawn does

You're right, I just misspoke.

you can update it in your own application to no longer be vulnerable.

I did, but thanks for pointing it out.

None of the dependencies in between your app and cross-spawn need to be touched, or bothered, or helped - that’s the entire point of semver ranges, so that everyone can quietly just fix their own problems whenever they come up without volunteer unpaid maintainers needing to put in additional time.

This feels like you're trying to make me feel bad, which I don't get. Then again social stuff is not my forte. Assuming you want left alone, I'll unsub from this PR and be fine with whatever happens to it

[ljharb]

I’m not, really - i just want to be very clear to any participants, subscribers, or future readers how semver ranges and lockfiles and vulns work together.

[Xyloto91]

@ljharb, I updated the cross-spawn version in my packages, but when the pipeline builds, I still encounter this vulnerability. Even though the cross-spawn package is updated to version 7.0.6, the Trivy scan found this vulnerability in the base image packages. In the Dockerfile, I’m using the 23-alpine3.19 base image. Should Node.js build the image with updated packages, or is there another solution?

[ljharb]

yes, in this case it's coming from bundled npm, which means npm has to update and release, and then node has to update (or you can npm install -g npm). however since npm's not actually vulnerable here, you should configure your scanner to ignore it (since 99% of transitive vulns in the npm ecosystem are false positives, the ability to ignore warnings is pretty important)

[Moriahzimm]

@ljharb from my understanding this PR is needed, at least for the edits to the package-lock.json. As you pointed out, in the package.json the dependency on cross-spawn is unpinned and thus consumers should be able to install a higher version locally. However, when I look at the package-lock.json currently on the main branch there is a version of cross-spawn that is pinned at 7.0.3. I may be misunderstanding how this works but I believe this is what's causing consumers problems.

[ljharb]

@Moriahzimm you are; package-lock.json is dev-only, and can't ever have any effect on consumers of the package when installed.