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

util: protect against monkeypatched Object prototype for inspect() #25953

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Trott
Copy link
Member

Trott commented Feb 5, 2019

Prevent affects of monkeypatching (for example) Object.keys() when calling util.inspect().

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@Trott Trott added util test and removed test labels Feb 5, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Feb 5, 2019

@BridgeAR
Copy link
Member

BridgeAR left a comment

I don't think that this is actually a good test. We create an artificial error that can only happen due to Object.keys being monkey patchable.

If we prevent that (which should IMHO be done at some point) this would not be possible anymore. So instead, I would rather make sure we prevent that. Afterwards it will AFAIK not be possible to trigger that code line at all but that should be fine in this case since it's just a safe guard against spec changes.

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Feb 6, 2019

We create an artificial error that can only happen due to Object.keys being monkey patchable.

If we prevent that (which should IMHO be done at some point)

Do you mean prevent Object.keys() monkey-patching, or do you mean prevent this internal module from using a monkey-patchable Object.keys() (by, say, caching/saving the original value of Object.keys() before a user is able to get in and change it)?

Assuming the latter, I think this is a good test to have until we actually do that, and then it can be removed. What this test does in its current form is test that the guard-against-spec-changes (and also guard-against-V8-bugs) is working as expected.

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Feb 6, 2019

@BridgeAR Ooh, actually perhaps a thing we could both be happy with is changing the guard code to an assertion. So once we've cached Object.keys()so that it isn't monkey-patchable, the code might look like this (with necessary line-wrapping added):

try {
      keys = Object.keys(value);
    } catch (err) {
      assert(isNativeError(err) &&  err.name === 'ReferenceError' && isModuleNamespaceObject(value));
      keys = Object.getOwnPropertyNames(value);
    }      
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Feb 6, 2019

@Trott I am fine switching to the assertion as you suggested. We could even log the actual error (just passing through the error as message).

That could also be done without preventing Object.keys from being monkey patchable here. That way the line will be properly covered without actually changing the behavior.

util: protect against monkeypatched Object prototype for inspect()
Prevent affects of monkeypatching (for example) Object.keys() when
calling util.inspect().

@Trott Trott force-pushed the Trott:object-keys-throws branch from 3d11b79 to 7ba6848 Feb 8, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Feb 8, 2019

@BridgeAR OK, I've updated this to avoid using a monkey-patched Object in inspect.js and modified the test to confirm that nothing breaks when Object is monkey-patched. And I've converted the guard code to use the new internal micro-assert(). PTAL!

@addaleax This is a significant enough change from the one you approved that you may wish to re-review?

@Trott Trott changed the title test: add test-util-inspect-object-keys-throws util: protect against monkeypatched Object prototype for inspect() Feb 8, 2019

@Trott Trott added the semver-major label Feb 8, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Feb 8, 2019

Defensively adding semver-major in case this somehow breaks APMs or something. (Seems unlikely, but weirder stuff has happened. Happy to remove semver-major if that's the consensus.)

/ping @nodejs/tsc (for reviews, because: semver-major)

Show resolved Hide resolved lib/internal/util/inspect.js Outdated
@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Feb 8, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Feb 8, 2019

@ljharb
Copy link
Contributor

ljharb left a comment

nevermindTo be robust, there can't ever be any runtime property lookups on mutable objects.
Show resolved Hide resolved lib/internal/util/inspect.js
Show resolved Hide resolved lib/internal/util/inspect.js
Show resolved Hide resolved lib/internal/util/inspect.js
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Feb 8, 2019

@ljharb These are not the original builtins, they are frozen copies that are not accessible to userland.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Feb 8, 2019

@addaleax ahh that's the context i'm missing, thanks.

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Feb 9, 2019

@cjihrig

cjihrig approved these changes Feb 9, 2019

@Trott Trott added the author ready label Feb 9, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Feb 10, 2019

Landed in 1847696

@Trott Trott closed this Feb 10, 2019

Trott added a commit to Trott/io.js that referenced this pull request Feb 10, 2019

util: protect against monkeypatched Object prototype for inspect()
Prevent affects of monkeypatching (for example) Object.keys() when
calling util.inspect().

PR-URL: nodejs#25953
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Feb 12, 2019

Ugh, forgot a CITGM run. Sorry, everyone. Here's one after-the-fact (run against master since this landed): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1734/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment