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

Proof of Concept Insights - Avoid overwrite of Function.prototype.toString() #6

Closed
bentobox19 opened this issue Feb 26, 2022 · 2 comments
Labels
error build/run time blocker lockdown metamask-mobile v8 Chrome V8

Comments

@bentobox19
Copy link
Contributor

PoC

https://github.com/LavaMoat/docs/blob/main/react-native-and-ses-lockdown.md

Discussion

The following error

 ERROR  failed to delete intrinsics.%FunctionPrototype%.toString.prototype [TypeError: Unable to delete property.]

Is tracked to sentry assigning to Function.prototype.toString. This is a System Integration.

FunctionToString.prototype.setupOnce = function () {
    // eslint-disable-next-line @typescript-eslint/unbound-method
    originalFunctionToString = Function.prototype.toString;
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    Function.prototype.toString = function() {
        var args = [];
        for (var _i = 0; _i < arguments.length; _i++) {
            args[_i] = arguments[_i];
        }
        var context = this.__sentry_original__ || this;
        return originalFunctionToString.apply(context, args);
    };
};

From https://docs.sentry.io/platforms/javascript/configuration/integrations/default/

System integrations are enabled by default to integrate into the standard library or the interpreter itself. They are documented so you can be both aware of what they do and disable them if they cause issues.

From https://docs.sentry.io/platforms/react-native/configuration/options/

integrations
In some SDKs, the integrations are configured through this parameter on library initialization. For more information, please see our documentation for a specific integration.

defaultIntegrations
This can be used to disable integrations that are added by default. When set to false, no default integrations are added.

Disable default integrations in sentry

diff --git a/app/util/setupSentry.js b/app/util/setupSentry.js
index 7b633ec0..a98c1b5a 100644
--- a/app/util/setupSentry.js
+++ b/app/util/setupSentry.js
@@ -18,6 +18,7 @@ export default function setupSentry() {
                dsn,
                debug: __DEV__,
                environment,
+              defaultIntegrations: false,
                integrations: [
                        new Dedupe(),
                        new ExtraErrorData(),

Items of Action

  • Understand the impact on the MetaMask Mobile App product of removing these default integrations.
  • Generate an alternative plan if the removal of such option is not viable for any reason.
bentobox19 added a commit to MetaMask/metamask-mobile that referenced this issue Jun 27, 2022
@leotm leotm added jsc JavaScriptCore error build/run time blocker lockdown metamask-mobile and removed jsc JavaScriptCore labels Mar 12, 2023
@leotm leotm added this to the MM + Lockdown milestone Mar 20, 2023
@leotm
Copy link
Collaborator

leotm commented Aug 17, 2023

Closing since error no longer present (MMM 0.66.5+ SES 0.18.1+) and patch no longer required

@leotm leotm closed this as completed Aug 17, 2023
@leotm leotm added the v8 Chrome V8 label Aug 21, 2023
@leotm
Copy link
Collaborator

leotm commented Aug 28, 2023

nb: original stack trace

Screenshot 2023-08-21 at 17 14 54

spotted previously debugging in V8 (babel runtime helpers: true) only when pausing on uncaught exceptions
otherwise unlogged to console and not preventing bootup nor affecting TTI

leotm added a commit to MetaMask/metamask-mobile that referenced this issue Nov 10, 2023
## **Description**

Problem being solved: prototype pollution/poisoning

SES lockdown (shim v0.18.8) on iOS JSC, baked early into RN core before
RN initialisation for the simplest minimal solution
as opposed to previous approach of shim'ing at the beginning of our
entry file requiring further complex lib patches

with SES lockdown on Android Hermes (introduced earlier in our [RN v0.71.6 upgrade](#6220))
being followed up separately
currently bundling successfully, but runtime not yet functional

_SES lockdown on Android JSC was also passing smoke tests after some work prior to Hermes_
_so a backup engine worth keeping on ice being followed up separately_ 

_Previous patches no longer required:
[`eth-keyring-controller`](https://github.com/MetaMask/metamask-mobile/pull/3794/files#diff-19aae36749eec9908c74557591fade5cc596e9a40422d88594c0fba456870389),
`ethjs-contract` (one not [two](https://github.com/MetaMask/metamask-mobile/pull/3794/files#diff-1970015453fca9583682c37a44695a3d26645329e586bb70ff6f05b74f936802)), [`web3-core-methods`](LavaMoat/docs#8), [`metro-react-native-babel-preset`](LavaMoat/docs#4), Sentry [config](LavaMoat/docs#6) (see previous PR: #3794

_Nb: `@babel/plugin-transform-regenerator` removed from
`metro-react-native-babel-preset` since initial investigation_
_Nb: `@babel/plugin-transform-runtime` config opt `regenerator: true`
previously caused iOS animated node assertion failures_
_Nb: default `@babel/plugin-transform-runtime` via
`metro-react-native-babel-preset` causes additional 4 SES warnings_

<details>
<summary>Nb: Current behaviour (not SES)</summary>

```
main, jsc
- import wallet via SRP
  - tap form field: disables cmd+D (app must be closed, no Metro restart)

main, v8
- (fresh) import wallet via SRP
  - if cmd+v paste ok, bot Import btn tappable when filled, but spinner hang (no error/warn), ~20s Metro dc
  - restart, enter pw, tap Unlock btn, spinner hang, still ~20s Metro dc
  - tap Reset Wallet, hang
- import (continued)
  - partial responsiveness
    - tap form field: dev menu disabled (cmd+d, app must be closed)
    - unresponsive: tap Back, tap Show, cannot tap Import, cmd+v disabled
    - responsive: cycle/input fields
    - still ~20s Metro dc
```

</details>

<details>
<summary>Previous SES warnings when locking down at entry file (not RN
InitializeCore)</summary>

https://www.diffchecker.com/fjj1iObp

```console
# JSC (26)
Removing intrinsics.Object.setPrototypeOf.default
Removing intrinsics.Object.setPrototypeOf.__esModule
Removing intrinsics.Object.assign.default
Removing intrinsics.Object.assign.__esModule
Removing intrinsics.Reflect.construct.default
Removing intrinsics.Reflect.construct.__esModule
Removing intrinsics.Reflect.decorate
Removing intrinsics.Reflect.metadata
Removing intrinsics.Reflect.defineMetadata
Removing intrinsics.Reflect.hasMetadata
Removing intrinsics.Reflect.hasOwnMetadata
Removing intrinsics.Reflect.getMetadata
Removing intrinsics.Reflect.getOwnMetadata
Removing intrinsics.Reflect.getMetadataKeys
Removing intrinsics.Reflect.getOwnMetadataKeys
Removing intrinsics.Reflect.deleteMetadata
Removing intrinsics.%ArrayPrototype%.toReversed
Removing intrinsics.%ArrayPrototype%.toSorted
Removing intrinsics.%ArrayPrototype%.toSpliced
Removing intrinsics.%ArrayPrototype%.with
Removing intrinsics.%ArrayPrototype%.@@unscopables.toReversed
Removing intrinsics.%ArrayPrototype%.@@unscopables.toSorted
Removing intrinsics.%ArrayPrototype%.@@unscopables.toSpliced
Removing intrinsics.%TypedArrayPrototype%.toReversed
Removing intrinsics.%TypedArrayPrototype%.toSorted
Removing intrinsics.%TypedArrayPrototype%.with
```

```console
# V8 (33)
Removing intrinsics.Object.assign.default
Removing intrinsics.Object.assign.__esModule
Removing intrinsics.Object.setPrototypeOf.default
Removing intrinsics.Object.setPrototypeOf.__esModule
Removing intrinsics.JSON.rawJSON
Removing intrinsics.JSON.isRawJSON
Removing intrinsics.Reflect.construct.default
Removing intrinsics.Reflect.construct.__esModule
Removing intrinsics.Reflect.decorate
Removing intrinsics.Reflect.metadata
Removing intrinsics.Reflect.defineMetadata
Removing intrinsics.Reflect.hasMetadata
Removing intrinsics.Reflect.hasOwnMetadata
Removing intrinsics.Reflect.getMetadata
Removing intrinsics.Reflect.getOwnMetadata
Removing intrinsics.Reflect.getMetadataKeys
Removing intrinsics.Reflect.getOwnMetadataKeys
Removing intrinsics.Reflect.deleteMetadata
Removing intrinsics.%ArrayPrototype%.toReversed
Removing intrinsics.%ArrayPrototype%.toSorted
Removing intrinsics.%ArrayPrototype%.toSpliced
Removing intrinsics.%ArrayPrototype%.with
Removing intrinsics.%ArrayPrototype%.@@unscopables.toReversed
Removing intrinsics.%ArrayPrototype%.@@unscopables.toSorted
Removing intrinsics.%ArrayPrototype%.@@unscopables.toSpliced
Removing intrinsics.%ArrayBufferPrototype%.transferToFixedLength
Removing intrinsics.%ArrayBufferPrototype%.detached
Removing intrinsics.%StringPrototype%.isWellFormed
Removing intrinsics.%StringPrototype%.toWellFormed
Removing intrinsics.%RegExpPrototype%.unicodeSets
Removing intrinsics.%TypedArrayPrototype%.toReversed
Removing intrinsics.%TypedArrayPrototype%.toSorted
Removing intrinsics.%TypedArrayPrototype%.with
```

</details>

<details>
<summary>Notes on patch creation</summary>

- `--exclude 'nothing'` to include `package.json` changes, then trim
patch
- `react-native` requires trimming majority of patch after initial diffs
- upon failure on symlinks, `git clean -fdx` and re-create

</details>

## **Related issues**

Fixes
- LavaMoat/docs#1 various issues
  - #3794 previous pr
- LavaMoat/docs#12 various issues

Worthy read for everyone on adding/upgrading libraries
- endojs/endo#1855

## **Manual testing steps**

App functions normally

## **Screenshots/Recordings**

### **Before**

Previously failing iOS (JSC) E2E tests have now been fixed

- LavaMoat/docs#9
  - https://github.com/MetaMask/metamask-mobile/assets/1881059/3cac630f-6ec8-4975-a273-8a115a4e8fe9
  - https://github.com/MetaMask/metamask-mobile/assets/1881059/67221672-d1a3-4346-a783-5f93b53dbbe9
  - https://github.com/MetaMask/metamask-mobile/assets/1881059/baa6e512-e307-4f0f-ae2f-77bfb4a29baf
  - https://github.com/MetaMask/metamask-mobile/assets/1881059/c21ffb0b-c9df-4223-ba73-5383dc8627f5

And more screenshots in related issues linked above

### **After**

App functions normally

---------

Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error build/run time blocker lockdown metamask-mobile v8 Chrome V8
Projects
None yet
Development

No branches or pull requests

2 participants