scijs / cwise

Component-wise operations for ndarrays
MIT License
122 stars 12 forks source link

potential security vulnerability via an outdated version of static-module@1.5.0 > static-eval@0.2.4 #19

Open alhugot opened 6 years ago

alhugot commented 6 years ago

Hi, this is perhaps not the place to report it, please feel free to close the issue, but the version of static-module specified in the package.json is affected by this security vulnerability: https://nodesecurity.io/advisories/548 cwise@1.0.10 > static-module@1.5.0 > static-eval@0.2.4

I have tried to update static-module to version ^2.0.0 which fixes the issue: https://github.com/browserify/static-module/issues/34

...but the tests are failing. I do no know this code enough to fix it, any help is welcome.

This is part of making plotly.js pass security tests: https://github.com/plotly/plotly.js/issues/2386

Would also be good to have a security badge with: snyk: https://github.com/snyk/snyk#badge or nsp: see https://github.com/dwyl/repo-badges Thx Alex

etpinard commented 6 years ago

Unfortunately, bumping static-module to the unvulnerable 2.x series won't be trivial.

Changes from PRs https://github.com/substack/static-eval/pull/18 and https://github.com/browserify/static-module/pull/38 appear to be incompatible with cwise.

From my findings, static-module fails (i.e. does not transform cwise({args: [], body: () => {}}) expressions) whenever body() mutates one if its input arguments.

For example, with

// cmd.js
var fs = require('fs')
var sm = require('./lib/cwise-transform')()

process.stdin.pipe(sm).pipe(process.stdout)
// works.js
var cwise = require('cwise')
var f = cwise({
  args: ["array"],
  body: function() {
      return 'yo'
  },
})
// fails.js
var cwise = require('cwise')
var f = cwise({
  args: ["array"],
  body: function(a) {
      ++a
  },
})

then, node cmd.js < works.js works just fine (but doesn't do anything :smirk: ), and node cmd.js < fails.js outputs:


var f = cwise({
  args: ["array"],
  body: function(a) {
      ++a
  }
})

that is, it correctly removes the require statement, but ignores the cwise call.

alhugot commented 6 years ago

Thanks for looking at it. I tried to update the dependency but the tests are failing (I am on windows and not familiar with these libs). But I am able to run the examples which are in the example dir.

michal-rad commented 6 years ago

+1

etpinard commented 6 years ago

@alhugot I believe your patch in https://github.com/alhugot/cwise/commit/39b4ad8a1c67d94903bdc139e59bc764e59bad6f defeats the purpose of browserify suite where cwise is used as a transform.

To double-check, would you mind comparing the output of browserify -t cwise test/fill.js using old and new static-module versions?

alhugot commented 6 years ago

@etpinard I am sorry I don't know much about browserify. Do you mean changing var cwise = require("cwise") tovar cwise = require("../cwise") in the unit test?

alhugot commented 6 years ago

ok I see, I have look at https://github.com/scijs/ndarray-fill module which use cwise and indeed the transform is wrong, as in your example. Sorry a bit slow on this one.

etpinard commented 6 years ago

. Sorry a bit slow on this one.

No worries. Thanks for the help!

dutscher commented 6 years ago

+1

hakandilek commented 6 years ago

@etpinard browserify -t cwise test/fill.js outputs are the same with static-module@2.2.5

etpinard commented 6 years ago

@hakandilek thanks for the headsup, but npm test fails for me with static-module@2.2.5.

hakandilek commented 6 years ago

What's the reason? Check out my linked pull request. I've integrated Travis ci also for the actual nodejs versions.

On Wed, 30 May 2018, 17:44 Étienne Tétreault-Pinard, < notifications@github.com> wrote:

@hakandilek https://github.com/hakandilek thanks for the headsup, but npm test fails for me with static-module@2.2.5.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/scijs/cwise/issues/19#issuecomment-393211941, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBdWf3KaUh7_bwyNR-hvHwpcEnxsTS3ks5t3r5jgaJpZM4SMRVS .

etpinard commented 6 years ago

Check out my linked pull request.

Ha. Sorry I missed that. Thanks for the PR.

srinivasrk commented 6 years ago

any update on this issue ?

elf-mouse commented 5 years ago

potential security vulnerability +1

k-sai-kiranmayee commented 5 years ago

Hello! Is there any update on this one...Is this https://github.com/scijs/cwise/pull/21 successfully merged?

Thank you!