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

Command Injection #4490

Open
dev2games opened this issue Aug 13, 2018 · 11 comments
Open

Command Injection #4490

dev2games opened this issue Aug 13, 2018 · 11 comments
Labels
clutter from npm install https://twitter.com/mikermcneil/status/966895462706954240 what do you think? Community feedback requested

Comments

@dev2games
Copy link

dev2games commented Aug 13, 2018

Removed

@sailsbot
Copy link

@dev2games Thanks for posting, we'll take a look as soon as possible.


For help with questions about Sails, click here. If you’re interested in hiring @sailsbot and her minions in Austin, click here.

@oaksofmamre
Copy link
Member

oaksofmamre commented Aug 14, 2018

This is a security vulnerability in sails-generate, meaning that you'd be able to hijack your own computer when running sails new. This one doesn't expose any tangible vulnerability to production applications. We will fix in future releases. Thanks so much for bringing this up @dev2games!

@oaksofmamre oaksofmamre added generators Related to `sails new` or `sails generate` v1.x and removed v1.x labels Aug 14, 2018
@raqem raqem added the bug label Oct 24, 2018
@hakash
Copy link

hakash commented Jan 7, 2019

Hello,

This vulnerability led me here too, and it would seem like it it not only in sails-generate, but also in sails itself:

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Critical      │ Command Injection                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ open                                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ No patch available                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ sails                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ sails > machinepack-process > open                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/663                       │
└───────────────┴──────────────────────────────────────────────────────────────┘

The problem is fixed in the newest versjon of machine-pack (v4.x) where it has migrated to using opn as the replacement for open, which is deprecated.

@matdombrock
Copy link

matdombrock commented Jan 14, 2019

I'm also getting this issue after install sails-mysql.

It seems to be documented in issue #4402 as well.

Sails version: 1.1.0
Node version: 8.15.0
NPM version: 6.4.1
DB adapter name: sails-mysql
DB adapter version: 1.0.1
Operating system: Ubuntu 18.04

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Critical      │ Command Injection                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ open                                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ No patch available                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ sails                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ sails > machinepack-process > open                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/663                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Critical      │ Command Injection                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ open                                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ No patch available                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ sails                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ sails > sails-generate > machinepack-process > open          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/663                       │
└───────────────┴──────────────────────────────────────────────────────────────┘

So am I seeing that this is not actually a critical issue and can be safely ignored for now?

@hakash
Copy link

hakash commented Jan 14, 2019

@oaksofmamre @raqem I've added a PR in 'sails-generate' to bump the 'machinepack-process' version to the latest, as both my manual test, and the automatic tests show no breaking changes between v2.0.2 and v4.0.0 for sails. Aparently it is only used when generating a new sails app ('sails new') to run the 'npm install' command after the files and directories are generated.

This should fix both critical vulns reported by 'npm audit'

@PavanBahuguni
Copy link

Is there a timeline when this can be fixed? I see there is already a PR by @hakash to fix the vulnerabilities
balderdashy/sails-generate#35

@johnabrams7
Copy link
Contributor

@dev2games @oaksofmamre @hakash @matdombrock @PavanBahuguni - Sails is currently working on the patch for this 👍

@mikermcneil
Copy link
Member

@sailsbot sailsbot added clutter from npm install https://twitter.com/mikermcneil/status/966895462706954240 and removed bug generators Related to `sails new` or `sails generate` labels Apr 18, 2019
@sailsbot sailsbot reopened this Apr 18, 2019
@sailsbot
Copy link

Oh hey again, @dev2games. Now that this issue is reopened, we'll take a fresh look as soon as we can!


Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

@mikermcneil
Copy link
Member

mikermcneil commented Apr 18, 2019

@wulfsolter it's because I accidentally reopened it while logged in as sailsbot (I realized after closing it it'd be better to wait until we're through with our current purge before closing these vulnerability-related issues)

Also re the new message: sailsbot's new MO is that instead of parroting the same thing she says on initial opening, she responds to any reopening of issues and PRs with a shorter, sweeter message (mainly just to remind folks of the two points in the little footer thingie)

@johnabrams7
Copy link
Contributor

@dev2games @hakash @matdombrock @PavanBahuguni Multiple related PRs for this were merged a day ago - how is the behaviour now? I welcome the rest of the community to test this out as well 👍

@johnabrams7 johnabrams7 added the what do you think? Community feedback requested label Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clutter from npm install https://twitter.com/mikermcneil/status/966895462706954240 what do you think? Community feedback requested
Development

No branches or pull requests

9 participants