Snyk Vulnerability: Command Injection through shelljs

[halhelal]

🐞 bug report

Affected Package

The issue is caused by package shelljs which is a dependency for @angular/compiler-cli

Severity

CVSS SCORE 7.0 High Severity

Description

Shelljs is vulnerable to Command Injection. It is possible to invoke commands from shell.exec() from external sources, allowing an attacker to inject arbitrary commands.

🌍 Your Environment

Angular Version:

Angular CLI: 7.3.6
Node: 10.13.0
OS: win32 x64
Angular:
...

Package                      Version
------------------------------------------------------
@angular-devkit/architect    0.13.6
@angular-devkit/core         7.3.6
@angular-devkit/schematics   7.3.6
@schematics/angular          7.3.6
@schematics/update           0.13.6
rxjs                         6.3.3
typescript                   3.2.4

Anything else relevant?

GitHub Issue 1
GitHub Issue 2
Snyk Report

[gkalpak]

Thx for reporting this. We are currently not using shelljs.exec() in compiler-cli. We are only using cp, mkdir and mv (here and there).
Closing this, since we are not affected by the vulnerability afaict.

[adamcoard-maximus]

I agree that from what I can tell Angular is not affected by this vulnerability, but nonetheless the inclusion of shelljs does set off false-positives in many scanners. We use Angular for government applications, and the presence of this false-positive has caused quite a bit of consternation. We've had people suggest downgrading our version of Angular because then the threat-level of the vulnerability goes from High to Medium, but we've decided against it because a false-positive is always better than an actual vuln.

Shelljs are not able to provide a timeline on this fix. shelljs/shelljs#495

I completely understand it's not your responsibility per se, but I ask you to consider removing shelljs as a dependency. Having a High threat vulnerability, even a false positive, for an indefinite future could hurt or hinder adoption. I don't know how much work it'd be, but considering you only use cp, mkdir, and mv, it hopefully wouldn't take a ton of time.

Thanks!

[clydin]

This really doesn’t seem like a vulnerability in shelljs either. The functionality provided by the “exec” call is intended to allow an application using the library to execute an arbitrary command. If an application doesn’t sanitize input before calling then the application itself has the security vulnerability not the library.
If this were not the case then Nodejs itself would have the same security vulnerability report since it also provides the same functionality (which this library actually uses to implement its functionality).

[xxyyzz2050]

I'm using the last version of Angular, and I'm facing the same alert.

[gkalpak]

I am re-opening the issue, so we can use it for tracking the resolution of this.

FWIW, I still believe that this is a false positive (from what I understand) and the correct resolution would be for the vulnerability reporting tools to stop reporting this.

For context:
Similar false positives in the past have been resolved in the same way.
The ShellJS maintainers seem to be working with Snyk on resolving this and they "have come to the mutual conclusion that it probably doesn't make sense to mark ShellJS as vulnerable".

[SanderElias]

I contacted Snyk about this, and got the following response:

After a chat with our security team we are already communicating with the repo maintainer, once it will be sort and will be confirmed with them we will take it off and we will send a blog post

So I'm hopeful this will be sorted out the correct way pretty soon.

[nfischer]

Hi all, I work on ShellJS.

Shelljs are not able to provide a timeline on this fix. shelljs/shelljs#495

I think you misunderstood. This is not a fix, it's my project to implement a feature request. Landing that PR will not "fix" the "vulnerability." I acknowledge I haven't provided a timeline for the feature: my time for ShellJS is very limited and must balance feature work with other work for the module. As such, I have no clue when I can finish that feature, or if a satisfactory solution is even technically feasible.

I completely understand it's not your responsibility per se, but I ask you to consider removing shelljs as a dependency. Having a High threat vulnerability, even a false positive, for an indefinite future could hurt or hinder adoption.

I agree with what the Angular team has expressed on this thread. The resolution should be for Snyk and similar services to remove this false positive and flag modules which misuse shell.exec()/child_process.exec(), rather than flagging ShellJS itself (we can't "fix" exec(), we just hope to eventually deprecate it).

I don't know how much work it'd be, but considering you only use cp, mkdir, and mv, it hopefully wouldn't take a ton of time.

We've spent years working to get the semantics right for these bash commands, and are still working hard to get proper coverage/behavior. Rewriting from scratch would probably introduce a lot of deviations from the POSIX behavior (and copy-paste might create license issues). So, while I can't stop folks from doing this, I would strongly advise against adding error-prone implementations to replace existing solutions.

I contacted Snyk about this, and got the following response

Thanks for reaching out @SanderElias! Reading the quoted response, it might be misinterpreted as Snyk is waiting on action from me. My last communication with them was prior to your comment, but they said they need no action from ShellJS or me and they're not currently ready to take down this vulnerability report. But I'm happy to help out within reason if they need further action from ShellJS.

[adamcoard-maximus]

Hey @nfischer

The resolution should be for Snyk and similar services to remove this false positive and flag modules which misuse shell.exec()/child_process.exec(), rather than flagging ShellJS itself (we can't "fix" exec(), we just hope to eventually deprecate it).

Agree completely with this. For the record this has always been my prefered resolution; I suggested Angular remove the dependency due to i) the relatively limited use of shelljs ii) my desire for a quick resolution. I've always understood and represented this issue as a false positive.

[gkalpak]

It turns out that we no longer use ShellJS in compiler-cli code (we still use it in tests and tooling), so #31468 removes it from package.json > dependencies.

But, again, I believe the correct resolution is for the vulnerability alert to be taken down ✌️

[gkalpak]

Seems like we have a resolution 🎉 shelljs/shelljs#945 (comment)

after a careful examination of this vulnerability, we've decided to remove it from our database.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}