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.

Sign up for GitHub

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

Jump to bottom

Prototype pollution prevention for wallet scripts #14460

Merged
merged 7 commits into from
Aug 25, 2022
Merged

Prototype pollution prevention for wallet scripts #14460

merged 7 commits into from
Aug 25, 2022

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Aug 3, 2022 •
edited
Loading

Resolves brave/brave-browser#24415
Resolves https://github.com/brave/security/issues/940

  1. Provide extensions::SafeBuiltins like class for Brave wallet and only save what we need. (Most of the codes are copied from upstream)
  2. Move unnecessary Object creation from script to v8
  3. This measure is to protect the scripts we loaded in Brave, 3rd party scripts are out of scope
  4. SafeBuiltins are loaded in a closure when executing the script so it won't pollute the global namespace

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  1. Setup wallet and solana account
  2. Navigate to simple website like example.com
  3. Open Dev console
  4. Type this and enter
Object.defineProperties = ()=>console.log('defineProperties overwritten')
await solana.connect()
_brave_solana.createPublickey = ()=>'0x12345'
_brave_solana.createPublickey('5STiJLNtCLiNaeD6vwrEfvikNi9NbfVpEbxU7KrUv4uo').toString()
  1. Grant permission or unlock the wallet when being asked.
  2. We should expect to see '5STiJLNtCLiNaeD6vwrEfvikNi9NbfVpEbxU7KrUv4uo'
  3. Refresh the page
  4. Type this and enter
await solana.connect()
Object.prototype.__defineGetter__('publicKey', ()=>'0x00')
solana.publicKey
  1. We should be able to see real public key instead of '0x00'
  2. Refresh the page
  3. Type this and enter
Object.freeze = ()=>{}
await solana.connect()
_brave_solana.abc = 123
_brave_solana.abc
  1. We should see undefined instead of 123

@darkdh darkdh requested review from bridiver and bbondy August 3, 2022 20:24
@darkdh darkdh requested a review from a team as a code owner August 3, 2022 20:24
@darkdh darkdh self-assigned this Aug 3, 2022
@darkdh darkdh requested a review from diracdeltas August 3, 2022 20:28
@darkdh darkdh force-pushed the prototype-pollution branch 3 times, most recently from 84b6a49 to 91f2d62 Compare August 3, 2022 22:08
@diracdeltas diracdeltas requested review from thypon and removed request for diracdeltas August 3, 2022 22:11
bridiver
bridiver reviewed Aug 16, 2022
View reviewed changes
components/brave_wallet/renderer/BUILD.gn Outdated
@@ -6,6 +6,8 @@ source_set("renderer") {
"js_solana_provider.h",
"resource_helper.cc",
"resource_helper.h",
"safe_builtins.cc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like this should be a generic helper outside of wallet?

Copy link
Member Author

Choose a reason for hiding this comment

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

If it has to be generic and it needs to include everything. Right now, we only protect the one we used in wallet scripts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, it needs to include the things we are using, but those can be added for other Brave scripts that use it. What I want to avoid is duplicating this in other places

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 9cdc09281c12f97e6e37ac031c4c8572586a2169

bridiver
bridiver reviewed Aug 16, 2022
View reviewed changes
components/brave_wallet/renderer/js_solana_provider.cc Outdated
result = CreatePublicKey(context, public_key);
v8_public_key = CreatePublicKey(context, public_key);
v8::Local<v8::Object> object = v8::Object::New(isolate);
CHECK(CreateDataProperty(context, object, u"publicKey", v8_public_key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have a const for publicKey somewhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also used https://github.com/brave/brave-core/pull/14460/files#diff-7cdd1da2bab576a68d9a1469b45add340ad978aafbdff369f3e590b1aaf8febbR740

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 4d3458125b59d884a72a96565a2c43311c31eba1

@darkdh darkdh force-pushed the prototype-pollution branch from 91f2d62 to 9cdc092 Compare August 17, 2022 00:33
@darkdh darkdh requested a review from a team as a code owner August 17, 2022 00:33
@darkdh darkdh force-pushed the prototype-pollution branch from 9cdc092 to 3845582 Compare August 17, 2022 20:56
@darkdh darkdh force-pushed the prototype-pollution branch 3 times, most recently from 50f5bc0 to 980c1c1 Compare August 24, 2022 21:20
@darkdh darkdh force-pushed the prototype-pollution branch from 94f0f91 to 9b1cdb5 Compare August 24, 2022 23:02
@darkdh darkdh merged commit 1643311 into master Aug 25, 2022
@darkdh darkdh deleted the prototype-pollution branch August 25, 2022 16:03
@github-actions github-actions bot added this to the 1.45.x - Nightly milestone Aug 25, 2022
@srirambv srirambv mentioned this pull request Oct 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

@thypon thypon thypon approved these changes

@bbondy bbondy bbondy approved these changes

Assignees

@darkdh darkdh

Labels
None yet
Projects
None yet
Milestone
1.45.x - Release
Development

Successfully merging this pull request may close these issues.

Wallet prototype pollution attack
4 participants
@darkdh @bridiver @thypon @bbondy