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

bootstrap: unfreeze console under --frozen-intrinsics #27663

Closed

Conversation

guybedford
Copy link
Contributor

This fixes the bug pointed out by @jdalton in #25685 (comment).

I haven't included an explicit test, because for all intensive purposes we could be testing EVERY global that it works under this flag. Rather the fix is by exclusion.

At the same time I've also added some documentation on how to handle the common errors, which are really only these two cases.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor Author

guybedford commented May 13, 2019

CI failure seems to be that bootstrap is not a valid subsystem for some reason?. Fixed.

@guybedford guybedford force-pushed the frozen-intrinsics-disable-console branch from 7c18dc3 to c28bbb0 Compare May 13, 2019 01:54
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

The doc fix is great, though since its unrelated to the code fix, I think it would be better as a seperate commit, if you are comfortable with splitting it.

As for the code fix, you are just excluding console from the global objects that are frozen? That seems an odd fix, it means that an attacker can do console.log = () => { /* do evil */ }, right? I'm OK with it, and the experimental option doesn't say what does or does not get frozen, or what is an "intrinsic". console is defined by WhatWG and browsers, but maybe not the JS spec, so it doesn't need freezing? I think a seperate commit with a bit more justtification would be valuable. Also, is the root cause not fixable? I.e., is it not possible for the Console constructor to use one of the patterns you documented to make console construction work even with console frozen?

@guybedford
Copy link
Contributor Author

@sam-github the fix for subclassing console if it were a frozen intrinsics would actually be to use ES6 classes and then define all the properties as undefined class properties:

class ConsoleSubclass extends Console {
  // all properties would need to be explicitly defined as class properties
  log = undefined;
}

I think turning Console into a class like the above might also provide a fix in core.

We could explore these directions further, I just didn't want to get right into the weeds on this right now. Although we could and perhaps now is the best time to do that too instead of kicking it down the road.

@joyeecheung
Copy link
Member

joyeecheung commented May 13, 2019

console.Console is non-standard. Off the top of my head an intuitive way to work around this is to skip these non-standard stuff when deep-freezing builtins. For console that means we need to create a whitelist of things to freeze for it since there are a lot of non-standard stuff there.

IIUC what is proposed in #27663 (comment), that means users can no longer do const { log } = console; log(1);? I think that probably cannot be broken at this point (and that's also part of the behavior tested by WPT)

@devsnek
Copy link
Member

devsnek commented May 13, 2019

what does standardization have to do with security? the issue is whether it exists or not right? am i missing the point of frozen intrinsics?

@joyeecheung
Copy link
Member

joyeecheung commented May 13, 2019

@devsnek Being non-standard means there is no explicit guarantee about whether it makes sense to freeze something, because the behavior of it is..well, not standardized. e.g. console._stdout and console._stderr are not supposed to be frozen or otherwise they would not function, but then users may not even know what that is supposed to do.

@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor Author

Just cancelled the new CI as this has already run at https://ci.nodejs.org/job/node-test-pull-request/23085/. Apologies for noise!

@guybedford
Copy link
Contributor Author

Was just going to land this, but git node metadata tells me I should wait another 40 hours given the single approval, so will come back around on it Monday unless anyone else wants to approve here.

doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
guybedford and others added 2 commits May 19, 2019 07:12
Co-Authored-By: Rich Trott <[email protected]>
Co-Authored-By: Rich Trott <[email protected]>
@guybedford
Copy link
Contributor Author

Landed in 1a9a577.

@guybedford guybedford closed this May 20, 2019
guybedford added a commit that referenced this pull request May 20, 2019
targos pushed a commit that referenced this pull request May 20, 2019
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants