iOS Safari fails to lockdown (with potential fix)

[ashconnell]

1. Call lockdown() on the latest version of iOS Safari
2. Lockdown fails with the error Cannot read "configurable" of undefined in the console

Digging in a bit, I found that the error is caused inside isImmuatableDataProperty():

function isImmutableDataProperty(obj, name) {
  const desc = getOwnPropertyDescriptor(obj, name)
  return (
    //
    // The getters will not have .writable, don't let the falsyness of
    // 'undefined' trick us: test with === false, not ! . However descriptors
    // inherit from the (potentially poisoned) global object, so we might see
    // extra properties which weren't really there. Accessor properties have
    // 'get/set/enumerable/configurable', while data properties have
    // 'value/writable/enumerable/configurable'.
    desc.configurable === false &&
    desc.writable === false &&
    //
    // Checks for data properties because they're the only ones we can
    // optimize (accessors are most likely non-constant). Descriptors can't
    // can't have accessors and value properties at the same time, therefore
    // this check is sufficient. Using explicit own property deal with the
    // case where Object.prototype has been poisoned.
    objectHasOwnProperty(desc, 'value')
  )
}

It is called with isImmutableDataProperty(window, 'showModalDialog'), which in iOS safari is actually undefined. So when it tries to get the property descriptor, desc is also undefined.

I added a hack to return true if desc is undefined and everything seems to work as expected. I'm not sure if that is the actual fix though.

[mhofman]

showModalDialog is not hardcoded anywhere in endo, which means it's discovered by listing the globalObject's own properties. It would seems that Safari would then claim it has an own property, yet return undefined for its description. I need to check what the invariants say about this, but at the very least, that's a fairly exotic behavior.

This would be one of those cases where if we had stronger invariants about when the host is allowed to add properties to the global, this would be illegal.

[ashconnell]

Yep, I assumed this was something weird Safari had done.

As for how to fix this in the ses package, i'm not sure if it should return true or false as immutable, but by returning true this does allow ses to run on iOS Safari.

AFAICT it doesn't have any ill side effects

[ashconnell]

I do however wonder why nobody else had this issue before, as iOS Safari is pretty well used 😅

[kriskowal]

My intuition is that adding desc === undefined || ... to the top of isImmutableDataProperty or checking before calling isImmutableDataProperty should be an acceptable solution. I would be more worried about the exotic behavior of failing to disclose that a property does in fact exist.

With a blessing from @erights, I’ll produce a PR, or accept one from you!

[erights]

I do however wonder why nobody else had this issue before, as iOS Safari is pretty well used 😅

I normally test Safari by testing only Safari on MacOS. It didn't occur to me that iOS Safari and MacOS Safari would have such differences in the JS engine.

I just tried both Safari 15.1 and Safari Technical Preview Release 136 (Safari 15.4, WebKit 17613.1.9.2) on MacOS 12.0.1 and do not see the problem there. I'll try iOS Safari. In case it is an issue, what version of iOS and iOS Safari are you using?

[erights]

For reference: https://jsfiddle.net/kryLa94b/

[erights]

Bug confirmed on Brave 1.32.3 (21.11.17.19) on iOS 15.1

Not as strange as it sounds. Apple forces all iOS browsers to use Safari under the hood.

[erights]

See https://bugs.webkit.org/show_bug.cgi?id=234282

Thanks @ashconnell for reporting this. Now that it is also reported to Safari/WebKit/Apple, let's proceed to do some kind of interim safe fix, referencing that bug and this one in the comments, with a TODO to remove once it is fixed upstream for long enough.

With a blessing from @erights, I’ll produce a PR, or accept one from you!

Please! I'm happy to review any such PR.

[ashconnell]

Yes iOS 15.1 here too. All web browsers on iOS use the same WKWebView/WebKit.

[erights]

Given the reply at https://bugs.webkit.org/show_bug.cgi?id=234282#c1 I suggest that our repair should be to remove the property, if doing so results in a non-anomalous state. Does it?

[kriskowal]

I’ve been unable to reproduce the problem on iOS using an emulator (iOS 14.4) and also with my own hardware (iOS 15.1), but I was able to verify the symptom of the undefined descriptor for the allegedly present showModalDialog property.

<meta charset="utf8">
<script>
  // Repair under consideration:
  delete window.showModalDialog;
</script>
<script src="dist/ses.umd.js"></script>
<script>
  console.log(Object.getOwnPropertyNames(window).includes("showModalDialog"));
  console.log(Object.getOwnPropertyDescriptor(window, "showModalDialog"));
  console.log(lockdown());
  console.log('got here');
</script>

Without repair, true, undefined, undefined, "got here". I was expecting lockdown to throw and not to reach "got here".

With repair, false, undefined, undefined, "got here", as expected.

@ashconnell, can you verify your SES version or more detailed steps to reproduce?

[ashconnell]

@kriskowal I think this is confirmed on iOS 15.1 and 15.2. This was likely introduced in 15.0 which is why you can't reproduce on 14.4

[ashconnell]

Apologies just noticed you mentioned you tested on 15.1 without an issue. This is interesting...

[erights]

Apparently fixed: https://bugs.webkit.org/show_bug.cgi?id=234282 !

[mhofman]

Now just have to wait 6 months until this gets release 🙃

[erights]

We got here by playing the long game ;)

[karlcow]

@mhofman @erights is it solved for you in the latest version of Safari technical preview? https://developer.apple.com/safari/technology-preview/

[mhofman]

@karlcow , this issue only happened on iOS. Is there a way to test a technical preview there?

[karlcow]

@mhofman nope 😕 sorry.

[kriskowal]

I’m labeling this as worth reviewing again, just to verify that we’re covered on iOS and close.

[ashconnell]

We ended up patching this and locking to v0.15.1 using this hack in our fork:

We also had to make a minor change to allow a crypto extension to work correctly

/**
 * Modified for Hyperfy
 * --------------------
 *
 * - if (errorTrapping !== 'none' && globalThis.process !== undefined) {
 * + if (errorTrapping !== 'none' && globalThis.process !== undefined && globalThis.process.on !== undefined) {
 *
 * Some libs (@walletconnect/ethereum-provider) will actually set window.process={...} in the browser.
 * The correct way to check is:
 *
 *   typeof process !== 'undefined' && process.versions != null && process.versions.node != null
 *
 * But this should suffice
 *
 */