feat: SES lockdown v0.18.8 (iOS JSC) #6586

leotm · 2023-06-13T08:24:17Z

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) 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, ethjs-contract (one not two), web3-core-methods, metro-react-native-babel-preset, Sentry config (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

Nb: Current behaviour (not SES)

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

Previous SES warnings when locking down at entry file (not RN InitializeCore)

https://www.diffchecker.com/fjj1iObp

# 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

# 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

Notes on patch creation

--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

Related issues

Worthy read for everyone on adding/upgrading libraries

Cannot assign to read-only property (Property override mistake) endojs/endo#1855

Fixes

☂️ React Native Compatibility Tracker LavaMoat/docs#1 various issues
- [WIP] SES lockdown() - LavaMoat Compatibility #3794
☂️ React Native Compatibility Tracker 2.0 LavaMoat/docs#12 various issues

Manual testing steps

App functions normally

Screenshots/Recordings

Before

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

Proof of Concept Insights - (Apparent) Race condition on disconnecting animated nodes LavaMoat/docs#9

And more screenshots in related issues linked above

After

App functioning normally

Pre-merge author checklist

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

With ESLint disabled since added to global

--exclude 'nothing' required to include package.json

--exclude 'nothing' required to include package.json is buggy Trim exccess fat

--exclude 'nothing' required to include package.json

--exclude 'nothing' required to include package.json is buggy Trim excess fat

Exclude polyfillGlobal

github-actions · 2023-06-13T08:24:29Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

socket-security · 2023-06-22T10:39:41Z

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

- Branch hash - COCOAPODS: 1.12.0 to 1.12.1

leotm · 2023-06-27T16:50:13Z

@SocketSecurity ignore [email protected]

Unmaintained (via devDep)

[email protected] /Users/leo/Documents/GitHub/metamask-mobile
├─┬ @storybook/[email protected]
│ └─┬ @storybook/[email protected]
│   └── [email protected]
└─┬ @storybook/[email protected]
  └─┬ @storybook/[email protected]
    └── [email protected]

Fix iOS native animation (non-nil/non-zero) assertion failure (on initial parentNode then childNode), after nav to bot 4th WebView tab (unable to load pages, likely caused by current excluded RN Promise polyfillGlobal) - Df (Foundation) *** Assertion failure in - disconnectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'parentNode' is a required parameter - Df (Foundation) *** Assertion failure in - connectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'childNode' is a required parameter Exclude iOS native animation (non-nil/non-zero) assertion macros - for now, when nodes (pointers to tags) attach/detach (to old/new parents and new views) QA: no empty animation frames observed ✅ Low probability possible risks introduced - incomplete node invalidation (outdated nodes) - race condition: prop updated before UIManager created view (outdated props) QA: no outdated animation frames observed ✅ iOS native animation assertion refs - Summary: facebook/react-native@c858420 - PR: facebook/react-native#10663 - Examples: facebook/react-native#9120 - nb: mimics ReactAndroid (i.e. NativeAnimatedNodesManager.java) nb: metro-react-native-babel-preset (0.72.3) - @babel/plugin-transform-regenerator has been removed since initial investigation - @babel/plugin-transform-runtime (removed) - 4 fewer SES warnings - intrinsics: Object.setPrototypeOf.default, Object.setPrototypeOf.__esModule, Reflect.construct.default, Reflect.construct.__esModule - @babel/plugin-transform-runtime > regenerator: false - immediate error thrown (recurring) Todo: Fix WebView page load (likely caused by current excluded RN Promise polyfillGlobal), thus fixing these assertion failures on nav, then revert this patch - Problem: Including (default) RN Promise polyfillGlobal causing app to boot empty root view Todo: Root cause of above 'regenerator: false' causing nil/zero parent/child nodes immediately to reoccur

Until Hermes runtime working on Android (currently only bundling successful)

patches/react-native+0.71.14.patch

app/store/index.ts

github-actions · 2023-11-02T15:52:06Z

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/2643987a-2d33-44ce-88f1-db28cf311e11
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

ios_e2e_test ✅
android_e2e_test ✅

ses.cjs

patches/react-native+0.71.14.patch

tommasini · 2023-11-07T17:38:20Z

patches/ethjs++ethjs-contract+0.2.2.patch

+     "lint:eslint": "*.js"
+   },
+-  "main": "lib/index.js",
+  "main": "src/index.js",


Just curious, I don't understand here why we needed to point to the src

good question ^ these couple ethjs contract/query libs we use (transitively via ethjs) to init our engine service
both optimise their bundles with few Babel runtime helpers (notably regenerator and asyncToGenerator)
both currently incompatible with SES, so using their source code instead is less problematic

so captured the details mentioned here
both in a code comment and issue

Have you considered patching babel-runtime and regenerator-runtime instead? They're used in quite a few packages (including some that we'll be introducing soon to the codebase, like our ethjs-* forks of packages like this one). It may be easier to fix the problem at the source. The extension already has patches for them to make them SES compatible.

patches/react-native+0.71.14.patch

tommasini

Thanks for the answers Leo! LGTM!

leotm · 2023-11-10T13:27:27Z

branch updated to re-run smoke tests before merge, but all CI broken on yarn audit, fixed in

fix: bump axios #7758

after fix merged ^ re-update branch, re-run smoke tests, then merge squash

edit: merged

sonarcloud · 2023-11-10T14:57:51Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

github-actions · 2023-11-10T15:07:48Z

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/156c9199-a45a-41b6-a518-6f6232b927d0
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

leotm · 2023-11-10T16:07:47Z

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/156c9199-a45a-41b6-a518-6f6232b927d0 You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

✅ ios_e2e_test
✅ android_e2e_test

index.js

leotm added 16 commits June 7, 2023 20:42

Format package.json

7ac4040

Add ses 0.18.4 via Yarn

8c1b278

Add ses 0.18.4 lockdown shim

4c205b4

Update Babel config to ignore ses.cjs

191b45e

Import ses npm module at entry file

4469470

Call ses lockdown at entry file

0f2f915

With ESLint disabled since added to global

Patch @babel/runtime

4f9a849

Patch ethjs/ethjs-contract

00dd1ac

--exclude 'nothing' required to include package.json

Fix ethjs/ethjs-contract patch (patch-package fail on fresh install)

991c926

--exclude 'nothing' required to include package.json is buggy Trim exccess fat

Patch ethjs/ethjs-query

3aba797

--exclude 'nothing' required to include package.json

Fix ethjs/ethjs-query patch (patch-package fail on fresh install)

e1e99eb

--exclude 'nothing' required to include package.json is buggy Trim excess fat

Patch react-native

a4229b0

Exclude polyfillGlobal

Document EngineService init errors w ses lockdown w/o ethjs patches

873716b

Document ses lockdown shim import TypeError

745d89a

Merge branch 'main' into feature/ses-lockdown-2

0c542c9

Merge branch 'main' into feature/ses-lockdown-2

b635b1c

leotm added 3 commits June 15, 2023 18:13

Merge branch 'main' into feature/ses-lockdown-2

15a8ab7

ESLint ignore lockdown shim

22bde3a

Merge branch 'main' into feature/ses-lockdown-2

8186199

leotm added 2 commits June 27, 2023 16:19

Merge remote-tracking branch 'upstream/main' into feature/ses-lockdown-2

401b41f

Update Podfile.lock

d2d2bf4

- Branch hash - COCOAPODS: 1.12.0 to 1.12.1

leotm changed the title ~~SES lockdown~~ feature: SES lockdown Jun 27, 2023

leotm changed the title ~~feature: SES lockdown~~ feat: SES lockdown Jun 27, 2023

leotm force-pushed the feature/ses-lockdown-2 branch 2 times, most recently from 65a7945 to 615abc6 Compare June 28, 2023 17:41

leotm force-pushed the feature/ses-lockdown-2 branch from 615abc6 to e52f13a Compare June 28, 2023 18:42

leotm changed the title ~~feat: SES lockdown (iOS JSC)~~ feat: SES lockdown v0.18.8 (iOS JSC) Nov 2, 2023

Update RN patch InitCore to only call lockdown on iOS and not Hermes

35fb6d5

Until Hermes runtime working on Android (currently only bundling successful)

leotm commented Nov 2, 2023

View reviewed changes

patches/react-native+0.71.14.patch Show resolved Hide resolved

leotm commented Nov 2, 2023

View reviewed changes

patches/react-native+0.71.14.patch Show resolved Hide resolved

leotm commented Nov 2, 2023

View reviewed changes

patches/react-native+0.71.14.patch Show resolved Hide resolved

leotm commented Nov 2, 2023

View reviewed changes

app/store/index.ts Show resolved Hide resolved

Merge branch 'main' into feature/ses-lockdown-2

9b4f859

leotm marked this pull request as ready for review November 2, 2023 15:49

github-actions bot added the Run Smoke E2E label Nov 2, 2023

tommasini reviewed Nov 7, 2023

View reviewed changes

ses.cjs Show resolved Hide resolved

tommasini reviewed Nov 7, 2023

View reviewed changes

patches/react-native+0.71.14.patch Show resolved Hide resolved

tommasini reviewed Nov 7, 2023

View reviewed changes

leotm requested a review from tommasini November 8, 2023 18:25

leotm commented Nov 9, 2023

View reviewed changes

patches/react-native+0.71.14.patch Show resolved Hide resolved

tommasini approved these changes Nov 9, 2023

View reviewed changes

Merge branch 'main' into feature/ses-lockdown-2

d88e399

Merge branch 'main' into feature/ses-lockdown-2

e8eaf91

leotm added Run Smoke E2E and removed Run Smoke E2E labels Nov 10, 2023

leotm merged commit 6edfe34 into main Nov 10, 2023
29 checks passed

leotm deleted the feature/ses-lockdown-2 branch November 10, 2023 16:30

github-actions bot locked and limited conversation to collaborators Nov 10, 2023

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Nov 10, 2023

metamaskbot added the release-7.12.0 Issue or pull request that will be included in release 7.12.0 label Nov 10, 2023

Gudahtt reviewed Nov 22, 2023

View reviewed changes

index.js Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: SES lockdown v0.18.8 (iOS JSC) #6586

feat: SES lockdown v0.18.8 (iOS JSC) #6586

leotm commented Jun 13, 2023 •

edited

Loading

github-actions bot commented Jun 13, 2023

socket-security bot commented Jun 22, 2023 •

edited

Loading

leotm commented Jun 27, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited by leotm

Loading

tommasini Nov 7, 2023

leotm Nov 8, 2023 •

edited

Loading

leotm Nov 8, 2023 •

edited

Loading

Gudahtt Nov 22, 2023

tommasini left a comment

leotm commented Nov 10, 2023 •

edited

Loading

sonarcloud bot commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

leotm commented Nov 10, 2023

feat: SES lockdown v0.18.8 (iOS JSC) #6586

feat: SES lockdown v0.18.8 (iOS JSC) #6586

Conversation

leotm commented Jun 13, 2023 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jun 13, 2023

socket-security bot commented Jun 22, 2023 • edited Loading

leotm commented Jun 27, 2023 • edited Loading

github-actions bot commented Nov 2, 2023 • edited by leotm Loading

tommasini Nov 7, 2023

Choose a reason for hiding this comment

leotm Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

leotm Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Gudahtt Nov 22, 2023

Choose a reason for hiding this comment

tommasini left a comment

Choose a reason for hiding this comment

leotm commented Nov 10, 2023 • edited Loading

sonarcloud bot commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

leotm commented Nov 10, 2023

leotm commented Jun 13, 2023 •

edited

Loading

socket-security bot commented Jun 22, 2023 •

edited

Loading

leotm commented Jun 27, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited by leotm

Loading

leotm Nov 8, 2023 •

edited

Loading

leotm Nov 8, 2023 •

edited

Loading

leotm commented Nov 10, 2023 •

edited

Loading