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

SES lockdown Hermes compat tracking issue #1891

Open
20 of 28 tasks
leotm opened this issue Dec 11, 2023 · 3 comments
Open
20 of 28 tasks

SES lockdown Hermes compat tracking issue #1891

leotm opened this issue Dec 11, 2023 · 3 comments
Assignees
@erights @leotm
Labels
kriskowal-review-2024-01 Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024 metamask tracking For tracking issues that will never be closed.

Comments

@leotm
Copy link
Contributor

leotm commented Dec 11, 2023 •
edited
Loading

Briefly demo'd MetaMask/metamask-mobile#8009, a minimally transformed [email protected] wip that runs on Hermes (latest React Native default JS engine)

here's the omnibus of issues/mods below, while figuring which parts we definitely need (vs can skip/remove in a codemod) cc @erights would appreciate any thoughts

Bundling (Metro, release)

Runtime

Tested across bundled Hermes for RN

Nb: latest minor version tags above, side-by-side with Hermes tarballs downloaded during build

async functions

# ...

metamask-mobile/android/app/build/ASSETS/createBundleProdDebugJsAndAssets/index.android.bundle: error: async functions are unsupported
      const loadWithoutErrorAnnotation = async (compartmentPrivateFields, moduleAliases, compartment, moduleSpecifier, pendingJobs, moduleLoads, errors) => {

metamask-mobile/android/app/build/ASSETS/createBundleProdDebugJsAndAssets/index.android.bundle: error: async functions are unsupported
      const memoizedLoadWithErrorAnnotation = async (compartmentPrivateFields, moduleAliases, compartment, moduleSpecifier, pendingJobs, moduleLoads, errors) => {

metamask-mobile/android/app/build/ASSETS/createBundleProdDebugJsAndAssets/index.android.bundle: error: async functions are unsupported
      const load = async (compartmentPrivateFields, moduleAliases, compartment, moduleSpecifier) => {

mobile/android/app/build/ASSETS/createBundleProdDebugJsAndAssets/index.android.bundle:1233 error: async generators are unsupported
        async function* AsyncGeneratorFunctionInstance() {}

# ...

Nb: async function* a() {}; alone won't emit an error
but using/referencing it and beyond const b = a; will

encountered bundling the app with Metro (considering Re.Pack)

# via RN CLI
yarn watch:clean
yarn start:android # react-native run-android --variant=prodDebug
# or via Gradle Wrapper
./gradlew clean # after changes to SES
./gradlew :app:createBundleProdDebugJsAndAssets # bundle only
./gradlew :app:installProdDebug -PreactNativeArchitectures=arm64-v8a # bundle then build for M2

Since Hermes async and await support is In Progress (let alone async generators)

so Hermes doesn't support async arrow functions directly

but we're getting indirect support later via @babel/preset-env

# regenerator-runtime
[email protected] /Users/leo/Documents/GitHub/metamask-mobile
├─┬ @babel/[email protected]
│ └── [email protected]
# ...
# (old name: babel-plugin-transform-async-to-generator)
[email protected] /Users/leo/Documents/GitHub/metamask-mobile
├─┬ @babel/[email protected]
│ └── @babel/[email protected]
└─┬ [email protected]
  └── @babel/[email protected] deduped
# @babel/plugin-transform-arrow-functions
[email protected] /Users/leo/Documents/GitHub/metamask-mobile
├─┬ @babel/[email protected]
│ └── @babel/[email protected]
├─┬ [email protected]
│ └─┬ [email protected]
│   └── @babel/[email protected] deduped
├─┬ [email protected]
│ └── @babel/[email protected] deduped
└─┬ [email protected]
  └── @babel/[email protected] deduped
# @babel/plugin-transform-async-generator-functions
└─┬ @babel/[email protected]
  └── @babel/[email protected]
# @babel/plugin-async-generator-functions
[email protected] /Users/leo/Documents/GitHub/metamask-mobile
└── (empty)

nb: [email protected] includes these 37 babel plugins

If so, re-write to Promises, otherwise skip parts or remove and refactor, or is there a better solution?

TODO: check refactor from async arrow fns to async fns; check latest bundled Hermes versions of RN

No longer an issue on bundled Hermes for RN 0.71.17+ (i.e. not a custom build from source)

assertDirectEvalAvailable

SES cannot initialize unless 'eval' is the

Since eval is Excluded From Support

tameRegExpConstructor

no RegExpSymbol species descriptor

no RegExp[Symbol.species] descriptor, no stack

ses.cjs#4472: TypeError('no RegExp[Symbol.species] descriptor');

Screenshot 2023-11-29 at 11 05 28 am 
// node_modules/typescript/lib/lib.es2015.symbol.wellknown.d.ts
// ...
/// <reference lib="es2015.symbol" />

interface SymbolConstructor {
// ...
    /**
     * A function valued property that is the constructor function that is used to create
     * derived objects.
     */
    readonly species: unique symbol;
// ...
}

This is because it is Excluded From Support on Hermes

Symbol.species and its interactions with JS library functions

For a good reason it seems, noting both warnings

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/species

Screenshot 2024-02-28 at 6 51 52 pm

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/@@species

Screenshot 2024-03-04 at 11 49 57 am

So we likely want this part skipped or tameRegExpConstructor option to skip (plus remaining changes, then upstream)

    if( !speciesDesc) {
      throw TypeError('no RegExp[Symbol.species] descriptor'); // Thrown on Hermes since excluded from support
     }

fixed in

tameSymbolConstructor

property is not configurable Screenshot 2024-03-04 at 2 23 27 pm

property is not configurable, no stack / at [object CallSite]

ses.cjs#9481: defineProperties(SharedSymbol, descs);

Screenshot 2023-12-05 at 1 41 49 pm Screenshot 2023-12-05 at 3 41 43 pm

We can fix this simply by omitting configurable: true on Hermes

  const descs = fromEntries(
    arrayMap(originalDescsEntries, ([name, desc]) => [
      name,
      { ...desc }, // configurable: true, omitted
    ]),
  );

in order to tame the Symbol constructor (used on constructed compartments),
we attempt to temporarily make all props configurable: true
so that we can whitelist (remove non-standard) props, then harden them (back to non-configurable)

Hermes has 15 Symbol props on all versions (contrasted to e.g. Node with 18)

on metamask-mobile (Android)
when we observe Object.getOwnPropertyDescriptor(originalDescsEntries, name)?.configurable
we get 15 non-true props: 1 false prop (length), 14 undefined

nb: on Hermes v0.12.0 built from scratch

➜  bin ./hermes --version
LLVM (http://llvm.org/):
  LLVH version 8.0.0svn
  Optimized build

Hermes JavaScript compiler and Virtual Machine.
  Hermes release version: 0.12.0
  HBC bytecode version: 96

  Features:
    Debugger
    Zip file input
➜  bin ./hermes
>> Object.getOwnPropertyDescriptors(Symbol)
{ length: { value: 0, writable: false, enumerable: false, configurable: true }, name: { value: "Symbol", writable: false, enumerable: false, configurable: true }, prototype: { value: Symbol { [constructor]: [Function Symbol], [description]: [accessor], [toString]: [Function toString], [valueOf]: [Function valueOf], [Symbol(Symbol.toStringTag)]: "Symbol", [Symbol(Symbol.toPrimitive)]: [Function [Symbol.toPrimitive]] }, writable: false, enumerable: false, configurable: false }, for: { value: [Function for], writable: true, enumerable: false, configurable: true }, keyFor: { value: [Function keyFor], writable: true, enumerable: false, configurable: true }, hasInstance: { value: Symbol(Symbol.hasInstance), writable: false, enumerable: false, configurable: false }, iterator: { value: Symbol(Symbol.iterator), writable: false, enumerable: false, configurable: false }, isConcatSpreadable: { value: Symbol(Symbol.isConcatSpreadable), writable: false, enumerable: false, configurable: false }, toPrimitive: { value: Symbol(Symbol.toPrimitive), writable: false, enumerable: false, configurable: false }, toStringTag: { value: Symbol(Symbol.toStringTag), writable: false, enumerable: false, configurable: false }, match: { value: Symbol(Symbol.match), writable: false, enumerable: false, configurable: false }, matchAll: { value: Symbol(Symbol.matchAll), writable: false, enumerable: false, configurable: false }, search: { value: Symbol(Symbol.search), writable: false, enumerable: false, configurable: false }, replace: { value: Symbol(Symbol.replace), writable: false, enumerable: false, configurable: false }, split: { value: Symbol(Symbol.split), writable: false, enumerable: false, configurable: false } }

(i built/ran Hermes from scratch to observe above, since Hermes via eshost-cli on m2 is unsupported via jsvu(mac64arm) and doesn't fetch via esvu(darwin-arm64) and built Hermes via eshost complains hermes: Unknown command line argument '-fenable-tdz' and Hermes playground only prints [object Object] - but thx for demo'ing @gibson042, jealous how it worked so eloquently on your machine :p)

nb: some Hermes Language Features

  • excludes: Symbol.species and its interactions with JS library functions
  • excludes: Symbol.unscopables (Hermes does not support with)
  • in progress: Symbol.prototype.description (it's not fully spec-conformant yet. Symbol().description should be undefined but it's currently '')
  • supports: Iteration (with [Symbol.iterator])
  • supports: Symbols (including most well-known Symbols)

nb: our tameSymbolConstructor fn

// https://github.com/endojs/endo/blob/master/packages/ses/src/tame-symbol-constructor.js
/**
 * This taming provides a tamed alternative to the original `Symbol` constructor
 * that starts off identical, except that all its properties are "temporarily"
 * configurable. The original `Symbol` constructor remains unmodified on
 * the start compartment's global. The tamed alternative is used as the shared
 * `Symbol` constructor on constructed compartments.
 *
 * Starting these properties as configurable assumes two succeeding phases of
 * processing: A whitelisting phase, that
 * removes all properties not on the whitelist (which requires them to be
 * configurable) and a global hardening step that freezes all primordials,
 * returning these properties to their expected non-configurable status.
 *
 * The ses shim is constructed to eventually enable vetted shims to run between
 * repair and global hardening. However, such vetted shims would normally
 * run in the start compartment, which continues to use the original unmodified
 * `Symbol`, so they should not normally be affected by the temporary
 * configurability of these properties.
 *
 * Note that the spec refers to the global `Symbol` function as the
 * ["Symbol Constructor"](https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-symbol-constructor)
 * even though it has a call behavior (can be called as a function) and does not
 * not have a construct behavior (cannot be called with `new`). Accordingly,
 * to tame it, we must replace it with a function without a construct
 * behavior.
 */

nb: a SymbolTaming lockdown option doesn't exist yet (same with FunctionTaming on tameFunctionConstructors)

solutions

extra

  • instead of throwing a RN polyfill error-guard Fatal Error in Metro on Hermes, throw a similar SES TypeError before instead? unless tolerated
  • update The ses shim is constructed to _eventually_ enable vetted shims... 10mo ago now we have vetted shims
    • and remaining codebase refs

security considerations pending further thought

getAnonymousIntrinsics

⚠️ Babel is likely transforming SES, check the config to ensure the shim is ignored ⚠️

Conflicting definitions of InertAsyncFunction

Conflicting definitions of %InertAsyncFunction%, no stack

ses.cjs#3688: TypeError( `Conflicting definitions of ${name}`);

Screenshot 2023-12-05 at 3 14 18 pm

Not present in

Present in

  • [email protected](metamask-mobile)
    • debugger statements broken in Flipper > Hermes Debugger (RN)

commenting

throw new TypeError(`Conflicting definitions of ${name}`);

results in SES continuing to lockdown
resulting in a fully functional React Native app
but at what security cost

%InertFunction% // no conflicting definitions
%InertAsyncFunction% // ❌ conflicting definitions
%InertGeneratorFunction% // no conflicting definitions

completePrototypes

lockdown prototype property not whitelisted

lockdown.prototype property not whitelisted, no stack

ses.cjs#3730: TypeError( `${name}.prototype property not whitelisted`)

to repro, uncomment

endo/packages/ses/scripts/hermes.sh

Line 38 in cdd2dad

# $HERMES -b test/hermes-smoke-dist.hbc

whitelistIntrinsics

Unexpected intrinsic intrinsics isFinite _proto_

Unexpected intrinsic intrinsics.isFinite.__proto__ at %FunctionPrototype%, no stack

ses.cjs#3953: TypeError( `Unexpected intrinsic ${path}.__proto__ at ${protoName}`)

Screenshot 2023-12-05 at 2 51 49 pm

isFinite appears to be the first of many intrinsics

No more SES TypeErrors thrown after whitelisting intrinsics

hardenIntrinsics

No errors visible, but the React Native app hangs

__hardenTaming__: 'unsafe' (default safe) fixes the issue, but is not a viable solution

https://github.com/endojs/endo/blob/master/packages/ses/docs/lockdown.md#__hardentaming__-options

The Hermes VM throws

Error running main: (TypeError#1)
TypeError#1: Failed to load module "./index.js" in package "file:///Users/leo/Documents/GitHub/endo/packages/ses/" (1 underlying failures: Error transforming "mjs" source in "file:///Users/leo/Documents/GitHub/endo/packages/ses/index.js": Cannot assign to read only property 'toString' of function 'function (path) {
    if (validator.call(path)) {
      return fn.apply(this, arguments);
    }
  }'

  at throwAggregateError (packages/ses/src/module-load.js:546:11)
  at load (packages/ses/src/module-load.js:594:3)
  at async makeBundle (packages/compartment-mapper/src/bundle.js:292:3)
  at async writeBundle (packages/ses/scripts/bundle.js:50:18)
  at async main (packages/ses/scripts/bundle.js:107:3)
@leotm leotm mentioned this issue Dec 11, 2023
Closed
13 tasks
@leotm leotm changed the title SES on Hermes for React Native SES lockdown on Hermes for React Native Dec 19, 2023
@leotm leotm mentioned this issue Dec 20, 2023
Open
76 tasks
@kriskowal kriskowal added the kriskowal-review-2024-01 Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024 label Jan 6, 2024
@kriskowal kriskowal assigned kriskowal and leotm and unassigned kriskowal Jan 6, 2024
@kriskowal
Copy link
Member

My understanding of this issue is that we’re tracking integration problems with Hermes and some of these may motivate recommendations to change the SES shim to broaden its portability. (I’m driving similar work with XS.) You also are getting by with some transformations and manual edits of the SES shim that might upstream.

@leotm Do you plan to propose changes?

One of the things I am thinking of doing for XS is elaborate the "exports" in package.json to use a custom variant of SES for that platform. That amounts to including only certain layers, and having some XS-specific layers. I imagine something similar will be in order for Hermes.

{
  "exports": {
    "hermes": {},
    "xsnap": {},
    "default": {},
  }
}

@leotm leotm changed the title SES lockdown on Hermes for React Native SES lockdown on Hermes for tracking issue Jan 17, 2024
@leotm
Copy link
Contributor Author

leotm commented Jan 17, 2024 •
edited
Loading

My understanding of this issue is that we’re tracking integration problems with Hermes and some of these may motivate recommendations to change the SES shim to broaden its portability. (I’m driving similar work with XS.) You also are getting by with some transformations and manual edits of the SES shim that might upstream.

@leotm Do you plan to propose changes?

One of the things I am thinking of doing for XS is elaborate the "exports" in package.json to use a custom variant of SES for that platform. That amounts to including only certain layers, and having some XS

Exactly ^ renamed to tracking issue, was initially only planning on running a local codemod once functional, but a new separate export (non-async SES shim) for Hermes makes sense 👍 like you're thinking for XS

@kriskowal kriskowal added the tracking For tracking issues that will never be closed. label Jan 17, 2024
@erights erights self-assigned this Mar 4, 2024
@leotm
Copy link
Contributor Author

leotm commented Mar 4, 2024

Updated description with deeper dive on tameSymbolConstructor with couple solutions ^ security considerations pending further thought

@leotm leotm mentioned this issue Mar 26, 2024
Closed
13 tasks
@leotm leotm mentioned this issue Apr 10, 2024
Merged
2 tasks
This was referenced Apr 17, 2024
Closed
Closed
mhofman pushed a commit that referenced this issue May 7, 2024
@leotm
feat(ses): redefine SharedSymbol to bypass Hermes prototype bug on ob…
59bb9ba 
…j literal short-hand methods (#2206)

Fix SES compat with Hermes when calling
`addIntrinsics(tameSymbolConstructor());`
- #1891

Follow-up to
- #2108
@leotm leotm mentioned this issue Jun 25, 2024
Open
28 tasks
@leotm leotm changed the title SES lockdown on Hermes for tracking issue SES lockdown for Hermes tracking issue Jun 26, 2024
@leotm leotm changed the title SES lockdown for Hermes tracking issue SES lockdown Hermes compat tracking issue Jun 26, 2024
@leotm leotm mentioned this issue Jul 26, 2024
Merged
@leotm leotm mentioned this issue Sep 11, 2024
Closed
2 tasks
@leotm leotm mentioned this issue Oct 2, 2024
Closed
@leotm leotm mentioned this issue Dec 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@erights erights

@leotm leotm

Labels
kriskowal-review-2024-01 Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024 metamask tracking For tracking issues that will never be closed.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@kriskowal @erights @leotm