balderdashy / sails

Realtime MVC Framework for Node.js
https://sailsjs.com
MIT License
22.81k stars 1.95k forks source link

Command Injection #4490

Open dev2games opened 6 years ago

dev2games commented 6 years ago

Removed

sailsbot commented 6 years ago

@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 commented 6 years ago

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!

hakash commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

johnabrams7 commented 5 years ago

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

mikermcneil commented 5 years ago

@dev2games @hakash @matdombrock @PavanBahuguni In addition, please see https://github.com/balderdashy/sails/issues/4699#issuecomment-483829719 and https://github.com/balderdashy/sails/issues/4402#issuecomment-483869476.

(#4699, #4402)

sailsbot commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 👍