Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimist not updated in release. CVE found #1886

Closed
lhaussknecht opened this issue May 11, 2022 · 12 comments
Closed

Minimist not updated in release. CVE found #1886

lhaussknecht opened this issue May 11, 2022 · 12 comments
Assignees
Labels
bug Something isn't working Runner Bug Bug fix scope to the runner

Comments

@lhaussknecht
Copy link

Describe the bug
The runner contains an unfixed CVE, that I reported here: actions/actions-runner-controller#1413

It looks like Minimist was updated in 0343e76, but the current release contains an old version (1.2.5) that contains a critical CVE.

To Reproduce
Steps to reproduce the behavior:

  1. Download the release v2.291.1
  2. Extract the archive
  3. Visit /externals/node12/lib/node_modules/npm/node_modules/minimist/package.json
  4. see "_id": "[email protected]" not 1.2.6 as it was supposed to be.

Expected behavior
Minimist 1.2.6 without a CVE should be packaged in the release.

Runner Version and Platform

v2.291.1

What's not working?

The runner cannot be run because it contains a critical CVE.

Job Log Output

Runner and Worker's Diagnostic Logs

@lhaussknecht lhaussknecht added the bug Something isn't working label May 11, 2022
@ruvceskistefan
Copy link
Contributor

Hi @lhaussknecht,
Thanks for reporting this issue! This is definitely a runner issue, so we're going to investigate it and fix it as soon as posible.

@ruvceskistefan ruvceskistefan added the Runner Bug Bug fix scope to the runner label May 13, 2022
@nikola-jokic
Copy link
Contributor

nikola-jokic commented May 13, 2022

Hi @lhaussknecht,

This dependency is actually a dev dependency which is not used in the production. That means, even though an insecure package exists, it is not exploitable by any means. You can see it by running npm list --depth=9. The only production level dependency is @actions/glob which does not contain any transitive dependency to the insecure minimist version.

├─┬ @actions/[email protected]
│ ├─┬ @actions/[email protected]
│ │ └── @actions/[email protected]
│ └─┬ [email protected]
│   └─┬ [email protected]
│     ├── [email protected]
│     └── [email protected]
├── @types/[email protected]
...

This is still an issue that we want to fix for sure, and thank you for submitting it! This comment is just to explain that this is not exploitable. ☺️

@lhaussknecht
Copy link
Author

Thanks @nikola-jokic for the clarification. At the moment we use the runner inside of containers and our registry blocks pulls because of this critical CVE.

@mumoshu
Copy link

mumoshu commented May 19, 2022

@lhaussknecht Hey! Which registry are you using? Can you reconfigure it to ignore the minist dependency only for your runner images? (IMHO, vulnerability scanners can never be 100% correct in practice so I believe any serious vulnerability scanning solution should already have a way to fine-tune the scanning policy

@lhaussknecht
Copy link
Author

We are using Harbor. As far as I know it's only possible to put CVEs on a allow-list per project. So in our case that would allow a real minimist CVE in other images to be pulled.

@lhaussknecht
Copy link
Author

@nikola-jokic Just wanted to make clear, that we are talking about minimist and not minimatch.

@nikola-jokic
Copy link
Contributor

Hey @lhaussknecht,

What I posted is just the poof that we are not using minimist in the production. I showed the output of the production dependency tree just to show that we are not using minimist. Not that we are using a correct version of minimatch ☺️. Occurance of minimatch in this tree has nothing to do with the explanation. It is just a package in the tree

@thboop thboop self-assigned this May 23, 2022
@thboop
Copy link
Collaborator

thboop commented May 25, 2022

The CVE here is being found in the node12 version we bundle with the runner. Node12 is no longer actively maintained, so we don't have a version we can update to to fix this.

We are in the process of deprecating node 12, after which we will remove that binary from the runner, but that likely won't happen in the next month.

I've filed an issue in the meantime to allow runner admins to force the node16 binary to be used. As a part of your docker build process, you should be able to delete the node 12 folder and enable that env to work around your issue. I'll update this issue when that feature is ready.

@lhaussknecht
Copy link
Author

For now we build our own image and just removed everything from the base image:

RUN rm -rf /runner/externals/node12/lib/node_modules/npm/node_modules/mkdirp/node_modules/minimist/ \ && rm -rf /runner/externals/node12/lib/node_modules/npm/node_modules/minimist/ \ && rm -rf /runner/externals/node12/lib/node_modules/npm/node_modules/mkdirp/node_modules/minimist/ \ && rm -rf /runnertmp/externalstmp/node12/lib/node_modules/npm/node_modules/minimist/ \ && rm -rf /runnertmp/externalstmp/node12/lib/node_modules/npm/node_modules/mkdirp/node_modules/minimist/

@NoamGoren
Copy link

NoamGoren commented Jun 15, 2022

Hi @thboop and team
we are using the twistcli to scan our image and we found a couple more critical dependency issues:

  1. json-schema - 0.2.3 (CVE-2021-3918) - fixed at 0.4.0
  2. ansi-regex - 3.0.0 (CVE-2021-3807 fixed in 3.0.1, 4.1.1, 5.0.1, 6.0.1
  3. minimatch - 3.0.4 (isaacs/minimatch@a8763f4) - fixed in 3.0.5

the above is currently blocking our release, is there a plan to upgrade or a workaround we can use in the meantime? (for reference, both minimatch and ansi-regex are part of node-16 as well)

@nascit
Copy link

nascit commented Oct 18, 2022

Hi, there.
Any plans to deprecate node12?
AWS Inspector finds 313 vulnerabilities. Even though most seems to be dev dependencies, it's hard to identify which ones we can get rid of.

@dhirschfeld
Copy link

Any plans to deprecate node12?

https://github.blog/changelog/2021-12-10-github-actions-github-hosted-runners-now-run-node-js-16-by-default/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Runner Bug Bug fix scope to the runner
Projects
None yet
Development

No branches or pull requests

8 participants