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

inspector: allow whitelisting inspector domain #27739

Closed
wants to merge 3 commits into from
Closed

inspector: allow whitelisting inspector domain #27739

wants to merge 3 commits into from

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented May 16, 2019

This change introduces an "--inspector-domain-whitelist" that can be used to
provide a comma-separated list of enabled Inspector domains. This will allow
to limit exposure of the Node instances to applications that may connect to
the inspector socket. E.g. this allows hiding unsafe methods like
"Runtime.evaluate" that allow executing arbitrary code.

  1. Runtime.runIfWaitingForDebugger will always be available. This is essential
    for tools that rely on --inspect-brk flag to connect before the code execution
    starts.
  2. All domains are still available through the Inspector JS APIs.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 16, 2019
@eugeneo eugeneo requested review from alexkozy and ofrobots May 16, 2019 19:34
@eugeneo
Copy link
Contributor Author

eugeneo commented May 16, 2019

CC: @ofrobots, @ak239

@eugeneo
Copy link
Contributor Author

eugeneo commented May 16, 2019

This is ready for the review, I will add command line documentation before merging this in.

@eugeneo eugeneo added the inspector Issues and PRs related to the V8 inspector protocol label May 16, 2019
Copy link
Member

@alexkozy alexkozy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea to limit protocol exposure and we definitely should do it to add another security layer.
I am not sure about this approach. Would we like to have more granularity in future, e.g. per method or per event? Is protocol ready to be used in environment without some domains, e.g. protocol definition file stands that HeapProfiler domain depends on Runtime domain link?
Would we like to consider some other layer, e.g. forbidding evaluating any JavaScript in Node which is not triggered by normal Node execution, e.g. any Runtime.evaluate, Runtime.callFunctionOn, some other JavaScript triggered as side effect of other method?

@eugeneo
Copy link
Contributor Author

eugeneo commented May 16, 2019

Would we like to have more granularity in future, e.g. per method or per event?

This implementation can trivially be extended to be based on method, if there is such a requirement. Filtering out the event may also be implemented separately.

Is protocol ready to be used in environment without some domains, e.g. protocol definition file stands that HeapProfiler domain depends on Runtime domain link?

I always assumed that dependencies like that are to make types (e.g. Runtime.CallFrame) visible from the HeapProfiler domain during the codegen. It should be fine in this case.

Some frontends will definitely break if they attach to a node instance with hidden domains. In my opinion, it is up to tool vendors and users to identify proper whitelist that best suits their needs.

Would we like to consider some other layer, e.g. forbidding evaluating any JavaScript in Node which is not triggered by normal Node execution, e.g. any Runtime.evaluate, Runtime.callFunctionOn, some other JavaScript triggered as side effect of other method?

This sounds like something that requires some help on the V8 side. I am not sure if ability to execute code remotely is the only potential security issue (e.g. attacker may set a breakpoint in specific location to see the branch that code took during the execution and that may help them to understand application state)

src/inspector/main_thread_interface.cc Outdated Show resolved Hide resolved
src/inspector_agent.cc Outdated Show resolved Hide resolved
src/inspector_agent.cc Outdated Show resolved Hide resolved
src/inspector_agent.cc Show resolved Hide resolved
Eugene Ostroukhov added 3 commits May 16, 2019 15:27
This change introduces an "--inspector-domain-whitelist" that can be
used to provide a comma-separated list of enabled Inspector domains.
This will allow to limit exposure of the Node instances to applications
that may connect to the inspector socket. E.g. this allows hiding
unsafe methods like "Runtime.evaluate" that allow executing arbitrary
code.

  1. Runtime.runIfWaitingForDebugger will always be available. This is
     essential for tools that rely on --inspect-brk flag to connect
     before the code execution starts.
  2. All domains are still available through the Inspector JS APIs.
@eugeneo
Copy link
Contributor Author

eugeneo commented May 24, 2019

@ofrobots do you want to proceed with this?

@ofrobots
Copy link
Contributor

Would we like to consider some other layer, e.g. forbidding evaluating any JavaScript in Node which is not triggered by normal Node execution, e.g. any Runtime.evaluate, Runtime.callFunctionOn, some other JavaScript triggered as side effect of other method?

This made me wonder how inspector's Runtime.evaluate interplays with the V8 flag --disable-code-generation-from-strings. It turns out that Runtime.evaluate is able to bypass that flag. /cc @hashseed: is that intentional?

Regardless, I think it is useful for users to be able to whitelist specific domains, so I'm in favor of this PR.

@eugeneo eugeneo added the stalled Issues and PRs that are stalled. label Aug 2, 2019
@eugeneo
Copy link
Contributor Author

eugeneo commented Oct 21, 2019

I think it is safe to assume there is no longer an interest in this change.

@eugeneo eugeneo closed this Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants