feat(a9-insecure-components): adding popular vulnerable package: marked #83

lirantal · 2017-02-04T15:25:25Z

feat(a9-insecure-components): adding a popular vulnerable package: marked

Adding a vulnerable package called marked for parsing and compiling Markdown syntax in JavaScript.
This is a popular package that has several releases related to XSS issues one in less than a year ago, and a very recent one as well.

Adding a /memos section to add memos (notes) which allows free text that is compiled to markdown and uses the marked library
Adding documentation to A9 - Insecure Components section
Explaining how to exploit the vulnerable Marked package

…rked Adding a vulnerable package called marked for parsing and compiling Markdown syntax in JavaScript. This is a popular package that has several releases related to XSS issues one in less than a year ago, and a very recent one as well. * Adding a `/memos` section to add memos (notes) which allows free text that is compiled to markdown and uses the marked library * Adding documentation to A9 - Insecure Components section * Explaining how to exploit the vulnerable Marked package

binarymist · 2017-02-04T23:35:56Z

I haven't tested this, but sounds like a good initiative.

It's probably an idea to add a comment in the package.json around the fact that this dependency should not be updated due to the fact of A9, otherwise someone will think they're helping and update it.

Was the development.js change intentional?

"May" also want to mention that snyk although it may be good, is not free. Often this is a show stopper.

lirantal · 2017-02-05T08:43:40Z

Good comments.

development.js was a local change, I'll commit a fix that comments it out again
we can't really add comments in JSON which goes for package.json as well so I'm not sure where else to add it except to just keep an eye on future PRs :-)
snyk has some paid plans but they are free and open source as well so I actually view them as a great partner for Node.js security

binarymist · 2017-02-05T22:18:13Z

{
"//": "a comment",
"//": "another comment"
}

Maybe snyk has just added the free tier? They seem to go up very fast after that though $49 vs NSP $1 for additional repos.

…age versions

lirantal · 2017-02-07T07:23:47Z

@binarymist fixed both comments

binarymist · 2017-02-07T08:07:31Z

Hi @lirantal Could be worth specifying what exactly the purpose is with:
"do not upgrade the marked package version it is set by purpose"

Someone else may be maintaining this code in the future, even if it was you, you may not remember what the purpose was in 2 years time. Just adding a few more words to what the purpose is, could save significant time, by the time several people read the comment and try and work out what the purpose is?

lirantal · 2017-02-07T08:18:15Z

Agree, updated.

ckarande · 2017-02-09T00:08:14Z

@lirantal I loved the idea to add an example for A9. Can you please go over comments I added and let me know your thoughts.

lirantal · 2017-02-09T06:41:21Z

Great.

Did you possibly not submit your review? I only saw comments for the other PR, not on this one.

ckarande · 2017-02-08T23:42:53Z

package.json

@@ -14,11 +14,17 @@
    "express-session": "^1.13.0",
    "forever": "^0.15.1",
    "helmet": "^2.0.0",
+    "marked": "^0.3.5",


The caret (^) sign in front of the version number installs newer minor or patch level versions. It appears that the vulnerability was fixed in marked 0.3.6 and this version would get pulled from npm due to the leading ^. We will need to specify absolute version here.

Great catch, totally missed it. I updated it to match an exact 0.3.5 version.

ckarande · 2017-02-08T23:50:50Z

app/views/tutorial/a9.html

+                        </p>
+
+                        <p>
+                            This library is so popular that it has been downloaded almost <strong> 3 million </strong> times in the last month and received more than <strong> 11,000 </strong> stars on GitHub.


It is nitpicking but as the download numbers would change each month, would it be possible to rephrase it so that the statement stays true anytime users read it.

yeah, would be a bit difficult to predict the future but I tried to make it generic, take a look now.

Just say millions

ckarande · 2017-02-09T00:03:18Z

app/views/tutorial/a9.html

+                            The <a href="https://nodesecurity.io/advisories">Node Security project </a> is a great initiative and resource to know about related vulnerabilities.
+                        </li>
+                        <li>
+                            <a href="https://snyk.io/">Snyk.io </a> is another Node.js CLI tool and Platform to scan and detect vulnerable packages


Can we also add there additional popular tools here -

npm-check, david-dm,
requireSafe, and retire.js

Definitely, will add.

Also bithound, npm-outdated https://leanpub.com/holistic-infosec-for-web-developers-fascicle1-vps-network-cloud-webapplications/read#web-applications-countermeasures-consuming-free-and-open-source-tooling

ckarande · 2017-02-09T17:24:57Z

@lirantal You are right.. missed to submit the review earlier. Please check now.

…it doesn't get updated automatically by npm

lirantal · 2017-02-10T07:17:46Z

Pushed all changes.

…and npm/yarn tools

lirantal · 2017-02-13T08:50:03Z

@binarymist @ckarande updated the tutorial again with your comments.

ckarande · 2017-02-14T15:09:54Z

👍 @lirantal Thanks for the PR all the updates. @binarymist thanks for your review comments.

lirantal · 2017-02-14T20:54:27Z

Thank you both guys, much appreciate the feedback and collaboration here ;-)

binarymist · 2017-02-15T23:09:37Z

@lirantal can you please keep your commit comments shorter, aprx 80 characters, as it screws up the git log --graph

If you have long comments, please put line breaks in. Thanks.

lirantal · 2017-02-15T23:11:07Z

I'm such a naughty boy

binarymist · 2017-02-15T23:23:20Z

Na, you're probably using one of those graphical editors that handle long lines better :-)

lirantal · 2017-02-15T23:31:47Z

damn it, caught me! :-)

but seriously I actually tend to use changelog semantic commits, and thus have multilines when need to elaborate for a long commit message. I guess I just went free-style with this repo.

Rest assured however that the problem is solved:

git config --global core.editor "nano -r 70"

binarymist · 2017-02-16T02:30:10Z

There seems to be a bug moving from Memos view to Allocations. The user Id is lost.

lirantal · 2017-02-17T13:30:10Z

yep, definitely the case

lirantal mentioned this pull request Feb 4, 2017

Low severity vulnerability affecting this repo #82

Open

Liran Tal added 2 commits February 7, 2017 09:16

restore config for dev environment to its default

b8c52a8

added comments to package.json about not removing/upgrading some pack…

d8bb7cc

…age versions

updating package.json comments to be more verbose

25000d8

ckarande reviewed Feb 9, 2017

View reviewed changes

Liran Tal added 3 commits February 10, 2017 09:09

updating package.json to match exact vulnerable version of marked so …

7561846

…it doesn't get updated automatically by npm

updating tutorial with a list of other tools in this area

d2db8e6

rephrasing text about the popularity of marked lib

830c0dd

fixed millions wording for makred vulnerability and updated bithound …

541e4c3

…and npm/yarn tools

ckarande merged commit d4685de into OWASP:master Feb 14, 2017

lirantal mentioned this pull request Feb 17, 2017

userId variable isnt set when viewing the memos page #88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(a9-insecure-components): adding popular vulnerable package: marked #83

feat(a9-insecure-components): adding popular vulnerable package: marked #83

lirantal commented Feb 4, 2017

binarymist commented Feb 4, 2017

lirantal commented Feb 5, 2017

binarymist commented Feb 5, 2017

lirantal commented Feb 7, 2017

binarymist commented Feb 7, 2017

lirantal commented Feb 7, 2017

ckarande commented Feb 9, 2017

lirantal commented Feb 9, 2017

ckarande Feb 8, 2017

lirantal Feb 10, 2017

ckarande Feb 8, 2017

lirantal Feb 10, 2017

binarymist Feb 13, 2017

ckarande Feb 9, 2017

lirantal Feb 10, 2017

binarymist Feb 13, 2017

ckarande commented Feb 9, 2017

lirantal commented Feb 10, 2017

lirantal commented Feb 13, 2017

ckarande commented Feb 14, 2017

lirantal commented Feb 14, 2017

binarymist commented Feb 15, 2017 •

edited

Loading

lirantal commented Feb 15, 2017

binarymist commented Feb 15, 2017

lirantal commented Feb 15, 2017

binarymist commented Feb 16, 2017

lirantal commented Feb 17, 2017

feat(a9-insecure-components): adding popular vulnerable package: marked #83

feat(a9-insecure-components): adding popular vulnerable package: marked #83

Conversation

lirantal commented Feb 4, 2017

binarymist commented Feb 4, 2017

lirantal commented Feb 5, 2017

binarymist commented Feb 5, 2017

lirantal commented Feb 7, 2017

binarymist commented Feb 7, 2017

lirantal commented Feb 7, 2017

ckarande commented Feb 9, 2017

lirantal commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckarande commented Feb 9, 2017

lirantal commented Feb 10, 2017

lirantal commented Feb 13, 2017

ckarande commented Feb 14, 2017

lirantal commented Feb 14, 2017

binarymist commented Feb 15, 2017 • edited Loading

lirantal commented Feb 15, 2017

binarymist commented Feb 15, 2017

lirantal commented Feb 15, 2017

binarymist commented Feb 16, 2017

lirantal commented Feb 17, 2017

binarymist commented Feb 15, 2017 •

edited

Loading