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

doc: provide info on impact of recent vulns #1547

Closed
wants to merge 6 commits into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Jan 8, 2018

No description provided.

@mhdawson
Copy link
Member Author

mhdawson commented Jan 8, 2018

@nodejs/security-wg @nodejs/website, draft based on discussion in private security repo.

Copy link
Contributor

@bnb bnb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member Author

mhdawson commented Jan 8, 2018

Pushed commit to address conflict.

@mhdawson
Copy link
Member Author

mhdawson commented Jan 8, 2018

actually looks like I got the wrong index.md. @fhemberger do we now need to update all of the index files ? for the banner ?

@mhdawson
Copy link
Member Author

mhdawson commented Jan 8, 2018

Pushed commit to update the rest of the banners, am wondering if there is a more efficient way of doing it.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


The risk from these attacks to systems running Node.js resides in the systems in which your Node.js applications run, as opposed to the Node.js runtime itself. The trust model for Node.js assumes you are running trusted code and does not provide any separation between code running within the runtime itself. Therefore, untrusted code that would be necessary to execute these attacks in Node.js could already affect the execution of your Node.js applications in ways that are more severe than possible through these new attacks.

This does not mean that you don't need to protect yourself from these new attacks when running Node.js applications. To do so, apply the security patches for your operating system. You do not need to update the Node.js runtime.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there should be more explanation.

Both Meltdown and Spectre need to run malicious code - even if permissions are restricted - to perform an attack. However, if malicious code is allowed to run on Node.js, you've lost anyways. But only to a certain degree. That malicious code still only has user permissions. Except that with Meltdown you can even read out kernel memory, if you run on an unpatched OS on an Intel CPU.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but I think the point we are trying to make is that you need to patch your OS, not Node.js. I'll pull in a few more words from what you have.

@@ -1,7 +1,7 @@
---
layout: index.hbs
labels:
banner: Important security releases, please update now!
banner: Spectre and Meltdown in the context of Node.js - no current action required.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OS needs to be updated. That's more than "no current action required".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was on the edge on that, I'll come up with something better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, "No Node.js specific action required"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually probably better to have people just read the blog so will remove the '- no current action required.'


# Summary

Project zero has recently announced some new attacks that have received a lot of attention: https://googleprojectzero.blogspot.ca/2018/01/reading-privileged-memory-with-side.html.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would you please add line breaks. Makes reviewing easier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes it more work to paste into the email list page as I then have to go and remove them all. I can do it but means more work on the other end.

@mhdawson
Copy link
Member Author

mhdawson commented Jan 8, 2018

@hashseed pushed commit to address comments.

This does not mean that you don't need to protect yourself from
these new attacks when running Node.js applications. If an attacker
manages to run malicious code on an upatched OS (whether using
javascript or something else) they may be able to access memory and or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "JavaScript" instead of "javascript" here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matteocontrini pushed commit to fix

@mhdawson
Copy link
Member Author

mhdawson commented Jan 8, 2018

@hashseed would be good to see if you are ok with my update.

@nodejs/website would be good to get confirmation that updating the banner in all of the index.md files is now the right way to go.

@mhdawson
Copy link
Member Author

mhdawson commented Jan 9, 2018

Will plan to send out first thing my morning (EST) unless there are other comments before then.

manages to run malicious code on an upatched OS (whether using
JavaScript or something else) they may be able to access memory and or
data that they should not have access to. In order to protect yourself
from these cases, apply the security patches for your operating
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make sense to link to these resources?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hashseed I'm not sure what all links/patches are available for OS's so far. Maybe that is something we can add in later on.

@fhemberger
Copy link
Contributor

@mhdawson Hmmm … not very happy to overwrite all local messages again. We didn't translate the security banner for exact this reason: Translation teams would never be able to make the adjustments so quickly in case of an urgent release.

But over time it was clear that the message used for the banners was always "Important security releases, please update now!" – so we decided to make it translatable as well.


I'm currently thinking about a "manual override" in the template instead of replacing all translations:

layouts/index.hbs#17:

- <a href="{{ project.banner.link }}">{{{ labels.banner }}}</a>
+ <a href="{{ project.banner.link }}">{{#if project.banner.text}}{{{ project.banner.text }}}{{else}}{{{ labels.banner }}}{{/if}}</a>

And just add the key back here: https://github.com/nodejs/nodejs.org/blob/master/build.js#L273

What do you think?

@lpinca
Copy link
Member

lpinca commented Jan 9, 2018

👍 for @fhemberger's suggestion.

@mhdawson
Copy link
Member Author

mhdawson commented Jan 9, 2018

@fhemberger, the banner would go through at least a couple of steps for a security release. First telling people the release was going to come in ~ 1 week, and then being updated to to tell them to update.

I think the override makes sense. Is that something we should include as part of this update ? I would like to get it out today.

@fhemberger
Copy link
Contributor

@mhdawson Yes, please feel free to add this switch with this PR so it can land.

@mhdawson
Copy link
Member Author

mhdawson commented Jan 9, 2018

Ok changes pushed to use override, should be good to go.

@mhdawson
Copy link
Member Author

mhdawson commented Jan 9, 2018

Plan to wait about 1 hour for any more comments and then will land/send out on nodejs-sec mailing list.

mhdawson added a commit that referenced this pull request Jan 9, 2018
PR-URL: #1547
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mandeep Singh <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
@mhdawson
Copy link
Member Author

mhdawson commented Jan 9, 2018

Landed as cdc4144

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants