Vulnerability in shelljs dependency: CWE-772: Missing Release of Resource after Effective Lifetime

[Gil-Littman]

ShellJS version (the most recent version/Github branch you see the bug on):

0.8.5

Description of the bug:

A transitive dependency of shelljs introduces a vulnerability. This can be solved by updating the glob version to 9.0.0 or higher.

[nfischer]

I understand that this package is a transitive dependency, but do you know if the inflight vulnerability actually be exploited in glob? Snyk has a bad habit of flagging any "vulnerability" as something which needs fixing, without consideration of whether the warning actually applies to the downstream projects.

Unfortunately, glob@9 is not compatible with node v8, which is compatibility ShellJS still supports. Fixing this is not a trivial package upgrade.

[nfischer]

#828 might be a possible path forward. I originally filed that ticket because fast-glob seemed to have nice perf wins, but switching to that would also mean we can avoid this dependency. I think it's mostly a drop-in replacement, but I see a few behavior differences around symlinks (both broken and non-broken). The behavior differences are clear since several tests are broken.

If someone wants to start a PR to move to fast-glob, let me know. I'm happy to review and provide guidance on the path forward.

[Gil-Littman]

I don't know that the vulnerability is exploitable in glob (probably not), but I also don't know that it isn't.
I was hoping this would be a straight forward fix, I'm sorry to hear it isn't.
I'll keep an eye on #828

Thanks for your quick response.

[nfischer]

I think the switch to fast-glob was more straightforward than expected. I wrote up #1153 to do this.

Unfortunately we currently expose globOptions as part of our public API. I think deprecating this is the best path forward (I don't think anyone uses this API), however if there's a need for this then we may be able to manually convert node-glob parameters into the corresponding fast-glob parameters. https://www.npmjs.com/package/fast-glob#compatible-with-node-glob has a nice conversion table.

[nfischer]

The inflight package is for async requests. It's only used by async-glob, the inflight library is never used by glob.sync(). ShellJS only calls glob.sync so it is not vulnerable to this issue. For this reason, I'm going to mark this as NOT a security issue, although I will keep it open for the sake of updating to a new glob library.

I verified this by copying the glob package, modifying shelljs to use my copy, and then editing the copy to throw new Error() right before every call to inflight. The exceptions are never triggered, thus proving that shelljs is not vulnerable to any issues in the inflight package.

cd path/to/shelljs
cp -r node_modules/glob node_modules/copyglob
# edit node_modules/copyglob/glob.js to add "throw new Error()" before each of the calls to inflight
sed -i "s/require('glob')/require('copyglob')/" src/*.js
npm test # confirm that no exceptions are triggered

[lvzhenbo]

It's proven that it won't be affected but it issues a deprecation warning when installing dependencies, I'm using the release-it package and release-it uses shelljs, annoying dependency passing!

[nfischer]

For what it's worth, I acknowledge and agree that is annoying. But let's be clear: deprecation != security vulnerability. I do not consider a dependency on a deprecated version of a package to be a high priority issue. I do intend to migrate away from glob (which in turn will also migrate away from inflight), however I'm not going to consider this to be a security issue.

I've decided that it may be a little clearer to close this issue as wontfix (because there is no known security vulnerability affecting ShellJS) and will keep issue #828 open to track the migration away from glob and inflight. Either way it's the same actual implementation work, I just want to make sure it's clear to readers that there is no known security issue.