Skip to content

{[inert function]} from toString() for Math.random etc. #910

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

Closed
2 tasks
dckc opened this issue Sep 28, 2021 · 6 comments · Fixed by #1718
Closed
2 tasks

{[inert function]} from toString() for Math.random etc. #910

dckc opened this issue Sep 28, 2021 · 6 comments · Fixed by #1718
Assignees

Comments

@dckc
Copy link
Contributor

dckc commented Sep 28, 2021

In a discussion of API evolution, @erights noted that for new features, testing for new properties by name works well, but for retroactive changes such as making Math.random() inert, it's unrealistic to expect code to test for absence of a property.

  • code changes, unit tests as usual
  • 262 test

IOU more explanation, but I have a meeting...

cc @phoddie

@erights
Copy link
Contributor

erights commented Jun 8, 2023

Therefore, retroactive removal of standard function properties should replace them with a throw-only functions that throw a reasonable diagnostic explanation. Likewise, perhaps, retroactive removal of data properties should turn them into throw-only accessors. This seems more unpleasant, but is also exceedingly rare.

@phoddie
Copy link

phoddie commented Jun 8, 2023

This guidance is consist with what XS has long done when minimizing a VM for embedded targets. For example, when the XS linker determines that a project does not use Math.cos(), instead of deleting cos, the linker replaces the implementation with a stub function that throws. The stub function should never be invoked, but is there to provide a diagnostic just in case.

@erights
Copy link
Contributor

erights commented Jun 8, 2023

Good, that's what I expected.

SES is not yet doing this, so this bug remains open for us

@phoddie
Copy link

phoddie commented Jun 8, 2023

Since we are on the topic... in secure mode, the XS implementation of SES replaces Math.random() with a function that throws a TypeError with the message "secure mode". Date.now() and appropriate paths of the Date constructor throw the same error.

@erights
Copy link
Contributor

erights commented Jun 30, 2023

See #1664

@erights
Copy link
Contributor

erights commented Aug 10, 2023

See especially thread starting at #1664 (review) . Conclusion is that the SES shim should only poison Date.now, the relevant portion of the Date constructor behavior, and the Math.random function. All the rest of the censored properties should continue to be censored by removal, as they are now.

Some odd cases worth discussing:

  • RegExp.prototype.compile predates ES5 and does not shadow anything inherited, so by the criteria of that thread it might seem we should poison it instead. However, it is already in Annex B as Normative Optional, so it is already coherent to expect programs that care to probe for it with property testing style.
  • RegExp.$1 etc. These are non-standard, and proposed only to become standard as Normative Optional. So again, we can expect property testing style.
  • Object.prototype.__proto__. We currently support it, as it does not violate any hard ocap safety rule. However, it is now (thankfully!) classified as Normative Optional, so if we ever do censor it, it is sensible for us to censor it by removal, requiring programs that use it to probe using property testing style. This is perhaps a bit more delicate because significant time passed between when it became standard mandatory and when it was demoted to Normative Optional.
  • Object.prototype.__defineGetter__, etc. In theory the same as __proto__. The difference is that we are likely to retire __proto__ for safety reasons @caridy has explained. By contrast, there is no known reason to censor the __defineGetter__ etc properties.

erights added a commit that referenced this issue Jul 16, 2024
Closes: #XXXX
Refs: #910 (comment)
#1718
#2354
#910

## Description

While investigating #2354 , I just
tried it locally by visiting

    file:///.../endojs/endo/packages/ses/demos/console/index.html

and

    file:///.../endojs/endo/packages/ses/demos/challenge/index.html

in my browser. The first is the SES demo console, which worked just
fine. The second is the SES Escape Room, which still relied on the
disabled `Date.now()` not throwing. Indeed, before
#1718 a disabled (secure mode)
`Date.now()` returned `NaN`. But
#1718 changed it to throw.

What's strange is that in #1718 I
revise endojs/endo/packages/ses/demos/challenge/index.html to adjust the
text to say that `Date.now()` is "disabled" rather than "NaN". But I
didn't fix the escape room code.

Although I found this while investigating
#2354 , this is a completely
distinct bug that is unrelated to
#2354 . This PR itself does nothing
to fix #2354 .

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

All our docs that link to an explain the Escape Room challenge need to
be revisited, especially once #2354
is fixed. For example,
https://agoric.com/blog/technology/a-taxonomy-of-security-issues , which
I just verified links to the broken page reported at
#2354

### Testing Considerations

It is frustrating that the Escape Room is broken at least since
#1718 , and also broken by
#2354 for an undetermined period of
time, without anyone noticing until now. It would be good to bring that
site under some kind of automated testing.

### Compatibility Considerations

none
### Upgrade Considerations

none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants