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

feat(NODE-6225): add property ownership check before referencing mongocryptdSpawnPath and mongocryptdSpawnArgs #4151

Merged
merged 4 commits into from
Jul 1, 2024

Conversation

vuusale
Copy link
Contributor

@vuusale vuusale commented Jun 16, 2024

To prevent prototype pollution gadget, added a check to ensure that 'mongocryptSpawnPath' and 'mongocryptSpawnArgs' properties exist in 'extraOptions' object before assigning them to 'this.spawnPath' and 'this.spawnArgs', which are passed to 'spawn' function in line 53.

HackerOne report: https://hackerone.com/bugs?subject=user&report_id=2522466

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Only permit mongocryptd spawn path and arguments to be own properties

We have added some defensive programming to the options that specify spawn path and spawn arguments for mongocryptd due to the sensitivity of the system resource they control, namely, launching a process. Now, mongocryptdSpawnPath and mongocryptdSpawnArgs must be own properties of autoEncryption.extraOptions. This makes it more difficult for a global prototype pollution bug related to these options to occur.

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

@durran
Copy link
Member

durran commented Jun 25, 2024

Hi @vuusale - thanks for opening this PR. We've discussed it and would a couple of changes made here if you don't mind. 1) We'd like to change this to use Object.hasOwn() (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwn) instead and 2) could you add a test that actually verifies the change?

@durran durran self-assigned this Jun 25, 2024
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 25, 2024
@durran durran changed the title fix: adding property existence check before referencing refactor: adding property existence check before referencing Jun 25, 2024
vuusale and others added 2 commits June 27, 2024 20:26
To prevent prototype pollution gadget, added a check to ensure that 'mongocryptSpawnPath' and 'mongocryptSpawnArgs' properties exist in 'extraOptions' object before assigning them to 'this.spawnPath' and 'this.spawnArgs', which are passed to 'spawn' function in line 53. 

HackerOne report: https://hackerone.com/bugs?subject=user&report_id=2522466
@durran durran added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 27, 2024
@durran durran changed the title refactor: adding property existence check before referencing feat: adding property existence check before referencing Jun 27, 2024
@durran durran changed the title feat: adding property existence check before referencing feat(NODE-6225): adding property existence check before referencing Jul 1, 2024
@nbbeeken nbbeeken changed the title feat(NODE-6225): adding property existence check before referencing feat(NODE-6225): adding property existence check before referencing mongocryptdSpawnPath and mongocryptdSpawnArgs Jul 1, 2024
@nbbeeken nbbeeken changed the title feat(NODE-6225): adding property existence check before referencing mongocryptdSpawnPath and mongocryptdSpawnArgs feat(NODE-6225): adding property ownership check before referencing mongocryptdSpawnPath and mongocryptdSpawnArgs Jul 1, 2024
@nbbeeken nbbeeken changed the title feat(NODE-6225): adding property ownership check before referencing mongocryptdSpawnPath and mongocryptdSpawnArgs feat(NODE-6225): add property ownership check before referencing mongocryptdSpawnPath and mongocryptdSpawnArgs Jul 1, 2024
@nbbeeken nbbeeken merged commit f48f8d3 into mongodb:main Jul 1, 2024
29 checks passed
@vuusale
Copy link
Contributor Author

vuusale commented Aug 13, 2024

Dear mongodb team, thanks for approving my change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants