CWE-367 has been discovered by `codeql` in `dist/main.js` file

[fabasoad]

Steps to reproduce

1. Run the command below.

cd dist
codeql database create .db-codeql-dist --language=javascript --build-mode=none
codeql database analyze .db-codeql-dist javascript-security-extended.qls --format=sarifv2.1.0 --output=dist-main.sarif

2. Open dist-main.sarif file in some SARIF viewer, e.g. in https://microsoft.github.io/sarif-web-component/
3. See 2 js/file-system-race findings in main.js.

References

• js/file-system-race
• CWE-367

[mcous]

Hi @fabasoad - that CWE has very little information and doesn't appear to link to anything specific. Are you able to identify a specific dependency or source code with an issue?

[fabasoad]

Hi @mcous! I've just scanned the code with codeql and it seems dist/main.js has this vulnerability inside. Since dist/main.js is a bundle, it might be from the own code as well as from some dependency. But I haven't done the research on where this problem originally came from. Anyway, I thought you might be interested in this finding, so I've shared it with you.

[fabasoad]

Let me look into it now 👀

[fabasoad]

In total there are 2 places:

Place 1: main.js, line 3635

if (stat.size < readSize) p.end(fs$2.readFileSync(file));

Looking at the js/file-system-race description, it seems the problem here is that race condition might occur between if condition and calling readFileSync method. All this logic exists in listFileSync function which is from tar@7.4.3 dependency. So, once it is fixed in tar then it should not be detected in the current repository.

listFileSync function is not used in the current repository, but it is used in list function in tar. And list function is used in the current repository, so problematic code is reachable.

Place 2: main.js, line 3639

fd = fs$2.openSync(file, "r");

It is basically very much the same. Same function (just in else statement). Same dependency. Same reachability.

---

IMO, there are 2 ways to solve this problem:

1. Fix it in tar library, release new version and then use new version in this repository.
2. Do not use tar library (or at least listFileSync function from it) and use something else.

[mcous]

Thanks for the analysis! Do you see a viable "attack" here? Certainly I'm down to keep the tar dependency up to date, but tar only comes into play if you provide a tarball to this action's input. In addition, we only read the tarball to find a package.json file.

If something were to maliciously mess with the tarball in a way that could exploit this race, at worst, the publish action would misunderstand the version of the package being published. I struggle to see how this could be reasonably exploited, given the npm registry itself will reject bad versions

TL;DR: I'm more than happy to keep tar up to date, but as of now, I don't see any actionable security issue here. As I've mentioned before, I think the most secure option is to avoid using this action and call npm publish directly

[fabasoad]

If something were to maliciously mess with the tarball in a way that could exploit this race, at worst, the publish action would misunderstand the version of the package being published. I struggle to see how this could be reasonably exploited, given the npm registry itself will reject bad versions

Agree with the conclusion. Feel free to close the issue if you feel that it is not relevant or it's not going to be addressed. Thank you for quick responses.

[mcous]

Thanks for your diligence! Might be worth flagging in node-tar if you haven't already, but otherwise I'm gonna close this issue in favor of regular Dependabot alerts

[fabasoad]

Just for the reference, I've opened a pull request in node-tar repository to fix this problem: isaacs/node-tar#444