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

fix(NODE-6265): add Spectre Mitigation and CFG #190

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Jul 12, 2024

Description

What is changing?

This PR adds Spectre mitigation and control flow guards to the generated binaries by adding those flags to the binding.gyp file.

Is there new documentation needed for these changes?

Potentially. Enabling Spectre mitigation requires that package maintainers and contributors install Spectre-mitigated libraries for Visual Studio, assuming that Windows developers need to run node-gyp rebuild. In my case, it seems that the prebuild-install script runs and skips over the rebuild command. Non-Windows OSes should not be affected by this PR in general.

What is the motivation for this change?

BinSkim (a Microsoft binary analyzer) has identified that kerberos.node is missing Spectre mitigation and control flow guard flags. This issue affects the compliance of Visual Studio Code downstream along with the general security of the kerberos.node binary.

Release Highlight

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

Copy link

There is an existing patch(es) for this commit SHA:

Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'.

@rzhao271
Copy link
Contributor Author

evergreen retry

Copy link

There is an existing patch(es) for this commit SHA that will be aborted:

The status reported will be corresponding to this PR rather than the previous existing ones. If you would like a patch to run for another PR and to abort this one, comment 'evergreen retry' on the corresponding PR.

@nbbeeken
Copy link
Collaborator

Potentially. Enabling Spectre mitigation requires that package maintainers and contributors install Spectre-mitigated libraries for Visual Studio, assuming that Windows developers need to run node-gyp rebuild. In my case, it seems that the prebuild-install script runs and skips over the rebuild command.

Hi @rzhao271 thanks for the help here. The prebuild-install step downloads prebuilds from the github release so when our next release goes out the windows build will have been built with the flag you have added. So I believe there's no need to rebuild on the user's end after installing this library. Unless I misunderstand the bit about needing to "install Spectre-mitigated libraries". Is that an additional set of libraries needed to support this at runtime or only at build? (I assume at build only)

@rzhao271
Copy link
Contributor Author

The libraries are only required during the build step, when node-gyp calls into MSVC to build the C++ files.

Copy link
Collaborator

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

LGTM

@aditi-khare-mongoDB aditi-khare-mongoDB merged commit 54b9799 into mongodb-js:main Jul 15, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants