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

Security Fix upgrade required on acorn package 5.7.x/6.4.x/7.0.x #7289

Closed
bhadrim opened this issue Sep 3, 2020 · 25 comments
Closed

Security Fix upgrade required on acorn package 5.7.x/6.4.x/7.0.x #7289

bhadrim opened this issue Sep 3, 2020 · 25 comments
Assignees

Comments

@bhadrim
Copy link

bhadrim commented Sep 3, 2020

From npm manifest on appliance I see that following acorn package versions are used in manageIQ. The following advisory: https://www.npmjs.com/advisories/1488 (source https://exchange.xforce.ibmcloud.com/vulnerabilities/177309) states that some of these listed packages have a DoS seurity vunerability. Upgrade the package to 5.7.4/7.0.1.

acorn 2.6.4 MIT - OK
acorn 3.3.0 MIT - OK
acorn 4.0.13 MIT - OK
acorn 5.7.1 MIT - Affected (update to 5.7.4)
acorn 5.7.3 MIT - Affected (update to 5.7.4)
acorn 6.4.0 MIT - Affected (update to 6.4.1)
acorn 6.4.1 MIT - OK
acorn 7.0.0 MIT - Affected (update to 7.0.1, 7.1.1, 7.2.0, 7.3.0 or 7.4.0)

@Fryguy
Copy link
Member

Fryguy commented Sep 3, 2020

@himdel What does it mean when the npm list has multiple versions? Need your guidance on these.

@Fryguy
Copy link
Member

Fryguy commented Sep 3, 2020

I'm also surprised we aren't seeing this in the github alerts - https://github.com/ManageIQ/manageiq-ui-classic/network/alerts

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

What does it mean when the npm list has multiple versions?

In our case, it means this...

node_modules/acorn = 6.4.1
node_modules/acorn-globals/node_modules/acorn = 7.4.0
node_modules/jsdom/node_modules/acorn = 5.7.4
node_modules/ng-annotate/node_modules/acorn = 2.6.4
node_modules/jest-environment-jsdom/node_modules/acorn = 5.7.4
node_modules/@patternfly/react-styles/node_modules/acorn = 5.7.4
node_modules/jest-environment-jsdom/node_modules/acorn-globals/node_modules/acorn = 6.4.1
node_modules/@patternfly/react-styles/node_modules/acorn-globals/node_modules/acorn = 6.4.1

It's probably irrelevant, because we're not using acorn on the appliance, it's a build dependency. And not our dependency, only dep of a dep. (Maybe that's why github doesn't care?)

Fixing this probably means upgrading all those packages, we can't upgrade ng-annotate (no new versions) but we may be able to remove it.

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

Oh, and updating dependencies of patternfly 3 will also be problematic because there are no new versions.

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

Reading the vulnerability, I think we can close this, as this would only be exploitable by users who can rebuild assets on the appliance.

Also, versions 2.6.4, 5.7.4, 6.4.1 and 7.4.0 seem to be unaffected (@bhadrim can you update the versions to list the good ones, as well as the bad please?) so we're fine on master.


So, maybe all we need to do is yarn upgrade in jansa.

@bhadrim
Copy link
Author

bhadrim commented Sep 3, 2020

@himdel

Affected and Unaffected versions are in https://www.npmjs.com/advisories/1488/versions

In your reply, you mentioned 5.7.4 for the following, but the manifest file has 5.7.3. Does that mean when the appliance was built it used 5.7.3 but now uses 5.7.4?

node_modules/jsdom/node_modules/acorn = 5.7.4
node_modules/jest-environment-jsdom/node_modules/acorn = 5.7.4
node_modules/@patternfly/react-styles/node_modules/acorn = 5.7.4

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

Thanks, updated the list :)

So, on master, if we do...

diff --git a/package.json b/package.json
index a634ff7e2f..3132839756 100644
--- a/package.json
+++ b/package.json
@@ -145,10 +145,10 @@
     "identity-obj-proxy": "^3.0.0",
     "imports-loader": "~0.8.0",
     "jasmine-jquery": "~2.1.1",
-    "jest": "~24.9.0",
-    "jest-cli": "~24.9.0",
+    "jest": "~26.4.2",
+    "jest-cli": "~26.4.2",
     "js-yaml": "~3.13.1",
-    "ng-annotate-loader": "~0.6.1",
+    "ng-annotate-loader": "~0.7.0",
     "path-complete-extname": "~0.1.0",
     "postcss-loader": "~3.0.0",
     "postcss-smart-import": "~0.6.11",
@@ -161,9 +161,9 @@
     "style-loader": "~1.1.3",
     "stylelint": "~13.5.0",
     "stylelint-config-standard": "~20.0.0",
-    "webpack": "~4.42.0",
-    "webpack-cli": "~3.3.11",
-    "webpack-dev-server": "~3.10.3",
+    "webpack": "~4.44.1",
+    "webpack-cli": "~3.3.12",
+    "webpack-dev-server": "~3.11.0",
     "webpack-manifest-plugin": "~2.2.0",
     "webpack-merge": "~4.2.2",
     "webpack-stats-plugin": "^0.3.2"

and yarn upgrade, the list simplifies to..

$ bfs node_modules -wholename \*node_modules/acorn/package.json | while read f; do echo "$f" ; jq .version "$f" ; done
node_modules/acorn/package.json
"6.4.1"
node_modules/jsdom/node_modules/acorn/package.json
"5.7.4"
node_modules/ng-annotate/node_modules/acorn/package.json
"2.6.4"
node_modules/jest-environment-jsdom/node_modules/acorn/package.json
"7.4.0"

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

In your reply, you mentioned 5.7.4 for the following, but the manifest file has 5.7.3. Does that mean when the appliance was built it used 5.7.3 but now uses 5.7.4?

@bhadrim You're looking at a manifest from a built jansa appliance,
So far, I'm looking at how it looks on master, now.

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

For jansa, assuming my environment still has all the UI plugins, the situation looks like this:

./plugins/manageiq-providers-nuage/node_modules/acorn/package.json
"5.7.3"
./plugins/manageiq-ui-classic/node_modules/acorn/package.json
"6.4.0"
./plugins/manageiq-v2v/node_modules/acorn/package.json
"5.7.3"
./plugins/manageiq-providers-redfish/node_modules/acorn/package.json
"5.7.3"
./plugins/manageiq-ui-service/node_modules/acorn/package.json
"5.7.1"
./plugins/manageiq-providers-nuage/node_modules/acorn-globals/node_modules/acorn/package.json
"6.4.0"
./plugins/manageiq-ui-classic/node_modules/jsdom/node_modules/acorn/package.json
"5.7.3"
./plugins/manageiq-ui-classic/node_modules/ng-annotate/node_modules/acorn/package.json
"2.6.4"
./plugins/manageiq-ui-classic/node_modules/webpack/node_modules/acorn/package.json
"6.4.1"
./plugins/manageiq-v2v/node_modules/acorn-jsx/node_modules/acorn/package.json
"3.3.0"
./plugins/manageiq-v2v/node_modules/acorn-globals/node_modules/acorn/package.json
"6.4.0"
./plugins/manageiq-providers-redfish/node_modules/acorn-globals/node_modules/acorn/package.json
"6.4.0"
./plugins/manageiq-ui-service/node_modules/acorn-jsx/node_modules/acorn/package.json
"3.3.0"
./plugins/manageiq-ui-service/node_modules/ng-annotate/node_modules/acorn/package.json
"2.6.4"
./plugins/manageiq-ui-service/node_modules/espree/node_modules/acorn/package.json
"7.0.0"
./plugins/manageiq-ui-service/node_modules/acorn-dynamic-import/node_modules/acorn/package.json
"4.0.13"

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

yarn upgrade seems to help, after running it in all ui plugins (and the ui), the new situation is...

./manageiq-providers-nuage/node_modules/acorn/package.json
"5.7.4"
./manageiq-ui-classic/node_modules/acorn/package.json
"6.4.1"
./manageiq-v2v/node_modules/acorn/package.json
"5.7.4"
./manageiq-providers-redfish/node_modules/acorn/package.json
"5.7.4"
./manageiq-ui-service/node_modules/acorn/package.json
"5.7.4"
./manageiq-providers-nuage/node_modules/acorn-globals/node_modules/acorn/package.json
"6.4.1"
./manageiq-ui-classic/node_modules/jsdom/node_modules/acorn/package.json
"5.7.4"
./manageiq-ui-classic/node_modules/ng-annotate/node_modules/acorn/package.json
"2.6.4"
./manageiq-v2v/node_modules/acorn-jsx/node_modules/acorn/package.json
"3.3.0"
./manageiq-v2v/node_modules/acorn-globals/node_modules/acorn/package.json
"6.4.1"
./manageiq-providers-redfish/node_modules/acorn-globals/node_modules/acorn/package.json
"6.4.1"
./manageiq-ui-service/node_modules/acorn-jsx/node_modules/acorn/package.json
"3.3.0"
./manageiq-ui-service/node_modules/acorn-globals/node_modules/acorn/package.json
"6.4.1"
./manageiq-ui-service/node_modules/ng-annotate/node_modules/acorn/package.json
"2.6.4"
./manageiq-ui-service/node_modules/espree/node_modules/acorn/package.json
"7.4.0"
./manageiq-ui-service/node_modules/acorn-dynamic-import/node_modules/acorn/package.json
"4.0.13"

And those all seem unaffected.

@Fryguy
Copy link
Member

Fryguy commented Sep 3, 2020

@himdel So I had asked @bhadrim to open this as an example. There are a lot more of these, and each one of these may require similar upgrades. What do you think is the best way forward?

@Fryguy
Copy link
Member

Fryguy commented Sep 3, 2020

Also, you sad "master in unaffected", but do we need this change: #7289 (comment) ?

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

Also, you sad "master in unaffected", but do we need this change

We don't, I was confused about which versions were OK and which weren't, sorry :)

If anyone with master is affected, they only have to rake yarn:clobber or yarn upgrade to not be, doing a fresh yarn in the branch as is will only give us the OK versions.

There are a lot more of these, and each one of these may require similar upgrades. What do you think is the best way forward?

Well, depends..

  • we could skip development dependencies and only filter by webpack manifest
  • we could review each vulnerability to evaluate how it could affect us and whether to fix it
  • we could assume yarn upgrade fixes most of them and reevaluate with a next build
  • or we need a dedicated person/team to deal with these - I've written up the basics in https://github.com/ManageIQ/manageiq-ui-classic/wiki/Updating-npm-dependencies

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

@Fryguy Ultimately, I think the best way to get rid of JS security vulnerabilities is to get as many people as possible to go converting our old forms (and explorers).
(Apart from the obvious periodic upgrades of everything even in releases.)

Even if we can get rid of most problematic packages, we can't actually upgrade jquery to v3 without also writing and releasing compatibility fixes for @pf3/select, @pf3/timeline, bootstrap-switch, bootstrap-filestyle, eonasdan-bootstrap-datetimepicker, jquery-rjs, jquery-ui, jquery-ujs, jquery.hotkeys, jquery.observe_field, patternfly-bootstrap-treeview, spice-html5-bower and xml_display, not to mention our code (#5585).

And we probably can't update angular to latest because of similar problems with the patternfly libraries and dependencies (angular-boostrap-switch, angular-dragdrop, angular-gettext, angular-patternfly, angular-ui-bootstrap, angular-ui-codemirror, angular-ui-sortable, angular.validators, kubernetes-topology-graph). (#6452)

Not to mention the ancient ui-service build setup (webpack 3).

So we may be able to save a lot of time by not using those libraries in the first place.

@Fryguy
Copy link
Member

Fryguy commented Sep 3, 2020

  • we could skip development dependencies and only filter by webpack manifest

The npm manifest on an appliance should be production dependencies only (@simaishi right?). The webpack manifest tells us what things webpack actually used, but unfortunately, we can't tell which version webpack chose as that information is not available. Maybe there's a something we can do when we generate the manifest to adjust that? Or is there some post-thing we can run using the webpack manifest to filter the npm manifest?

  • we could assume yarn upgrade fixes most of them and reevaluate with a next build

This feels like the best course so action for the first pass as it might just drop a lot of them, and then re-run whatever scanner @bhadrim is using.

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

We could, I'm not sure if all kinds of build use the code in rpm-build to generate the manifests, but given the webpack manifest like...

[
  "(webpack)/buildind/global.js",
  "../manageiq-providers-lenovo/app/javascript/foobar.jsx",
  "./app/javascript/packs/print.js",
  "./node_modules/@babel/runtime-corejs2/node_modules/core-js/library/fn/weak-map.js",
]

we could do something like..

cat webpack_manifest.json | grep node_modules | cut -d\" -f2 | sed 's/^\(.*node_modules\/[^/]\+\).*$/\1/' | while read dir; do echo '{"dir": "'$dir'", "name": "'`basename "$dir"`'", "version": '`jq .version "$dir"/package.json`'},'; done

{"dir": "./node_modules/@babel/runtime-corejs2/node_modules/core-js", "name": "core-js", "version": "2.6.11"},

to generate the right version for each entry.

EDIT: uh, except to support ./node_modules/@babel/runtime-corejs2 we need to try both ./node_modules/@babel/package.json and ./node_modules/@babel/runtime-corejs2/package.json to find the right dir.

@himdel
Copy link
Contributor

himdel commented Sep 3, 2020

OK..

[
  "(webpack)/buildind/global.js",
  "../manageiq-providers-lenovo/app/javascript/foobar.jsx",
  "../manageiq-providers-lenovo/node_modules/@babel/code-frame/lib/index.js",
  "./app/javascript/packs/print.js",
  "./node_modules/@babel/runtime-corejs2/node_modules/core-js/library/fn/weak-map.js"
]
require 'json'
manifest = JSON.load File.read 'webpack_manifest.json'
manifest.select! { |s| s =~ /node_modules/ }

packages = manifest.map do |file|
  file =~ /(.*node_modules\/)([^\/]+)(\/[^\/]+)?/
  left, one, two = [ $1, $2, "#$2#$3" ]

  name = if File.exists? "#{left}#{one}/package.json"
           one
         elsif File.exists? "#{left}#{two}/package.json"
           two
         end

  "#{left}#{name}"
end.sort.uniq

output = packages.map do |dir|
  package = JSON.load File.read "#{dir}/package.json"

  {
    "name": package["name"],
    "license": package["license"],
    "version": package["version"],
    "location": dir,
  }
end

puts JSON.pretty_generate(output)

output

[
  {
    "name": "@babel/code-frame",
    "license": "MIT",
    "version": "7.10.4",
    "location": "../manageiq-providers-lenovo/node_modules/@babel/code-frame"
  },
  {
    "name": "core-js",
    "license": "MIT",
    "version": "2.6.11",
    "location": "./node_modules/@babel/runtime-corejs2/node_modules/core-js"
  }
]

@Fryguy
Copy link
Member

Fryguy commented Sep 4, 2020

This is awesome... We can probably bake this right into the rpm build! cc @simaishi

@simaishi
Copy link
Contributor

simaishi commented Sep 4, 2020

@Fryguy Is this something that should be added only for rpm build? I'm thinking if we want to change webpack_manifest.json generation process so the file comes out in that new format...?

If former, I'll add @himdel 's code (thanks!) to rpm build.

@simaishi
Copy link
Contributor

simaishi commented Sep 4, 2020

The npm manifest on an appliance should be production dependencies only (@simaishi right?)

That was always my assumption. But, if acorn is a development dependency but included in the output, I'll have to double check.

@Fryguy
Copy link
Member

Fryguy commented Sep 4, 2020

@Fryguy Is this something that should be added only for rpm build? I'm thinking if we want to change webpack_manifest.json generation process so the file comes out in that new format...?

That's a really good idea. Let me see if I can add to the webpack_manifest.json generator.

@Fryguy
Copy link
Member

Fryguy commented Sep 4, 2020

Added @himdel's changes directly into the webpack manifest generator here: #7291

@Fryguy
Copy link
Member

Fryguy commented Sep 8, 2020

@bhadrim FWIW I don't see acorn in the final list that I generated in #7291

@Fryguy
Copy link
Member

Fryguy commented Sep 16, 2020

@bhadrim Can we close this one?

@bhadrim bhadrim closed this as completed Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants