Skip to content

permission: add kNet check to Open() in TCPWrap/PipeWrap/UDPWrap#63848

Open
Haruna38 wants to merge 1 commit into
nodejs:mainfrom
Haruna38:permission-net-open-hardening
Open

permission: add kNet check to Open() in TCPWrap/PipeWrap/UDPWrap#63848
Haruna38 wants to merge 1 commit into
nodejs:mainfrom
Haruna38:permission-net-open-hardening

Conversation

@Haruna38

Copy link
Copy Markdown

Add THROW_IF_INSUFFICIENT_PERMISSIONS(kNet) to Open() in TCPWrap, PipeWrap, and UDPWrap.

Every other method on these classes that enables network I/O: Bind, Listen, Connect, DoSend, which already checks kNet. Open() is the only method that adopts a file descriptor into a libuv network handle without one, breaking the otherwise uniform gate across the API surface.

The permission model documentation (doc/api/permissions.md) explicitly states that "using existing file descriptors via the node:fs module bypasses the Permission Model." This carve-out is scoped to node:fs, it does not extend to node:net or node:dgram. Wrapping a fd into a net.Socket or dgram.Socket via these Open() methods is a network API operation and should be subject to kNet like every sibling method in the same classes.

This aligns Open() with the checks added in 462c741 (--allow-net initial implementation), 59c86b1 (PipeWrap Bind/Listen), and 1d2686d (PipeWrap Connect).

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

@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. needs-ci PRs that need a full CI run. labels Jun 11, 2026

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollina

Copy link
Copy Markdown
Member

Thanks for opening a PR! Can you please add a unit test?

@Haruna38

Copy link
Copy Markdown
Author

Sure thing, please give me a moment.

@Haruna38

Haruna38 commented Jun 11, 2026

Copy link
Copy Markdown
Author

@mcollina while writing the test case, the failing CI caught my attention: this patch breaks stdio under the permission model. When a process runs with --permission but without --allow-net and its stdout/stderr/stdin is a pipe or TCP socket (as in any spawned child), bootstrap crashes:

node:net:432
    err = this._handle.open(fd);
          ^
Error: Access to this API has been restricted. Use --allow-net to manage permissions.
    at new Socket (node:net:432:24)
    at createWritableStdioStream (node:internal/bootstrap/switches/is_main_thread.js)

createWritableStdioStream (and stdin setup) wrap the inherited stdio fd via new net.Socket({ fd }) --> net.js:432 open(fd), which now hits the kNet check. That's why test-compile-cache-api-permission, test-dotenv-node-options, test-config-file, test-cli-permission-* and others fail, since they crash before running. I suspect this is why Open() was left ungated originally (#58517), since it's also how Node adopts inherited stdio.

Before I update the patch, what's your opinion on the direction? My instinct is to keep the kNet check on Open() but exempt the standard stream fds (0/1/2), so stdio works while inheriting a network socket on fd >= 3 stays gated. Happy to scope it differently if you prefer. Please read the follow-up comment, there are more nuances to this problem.

@Haruna38

Copy link
Copy Markdown
Author

After digging through more of the code, the current approach likely breaks fork() and cluster too, not just stdio, since _forkChild adopts the IPC channel with new Pipe(PipeConstants.IPC); p.open(fd), so the gate would block IPC setup under --permission.

So I'd like your opinion on direction: should we keep the hardening but allow internal invocations to bypass the gate (block user-facing open(), while letting Node's own stdio/IPC adoption through), or simply drop this PR? @mcollina

@daeyeon daeyeon added the permission Issues and PRs related to the Permission Model label Jun 13, 2026
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++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants