permission: add permission for udp #53398

theanarkh · 2024-06-09T20:48:53Z

This PR add permission check or UDP module. Usage:
node --allow-net-udp=domain1,127.0.0.1/80 --allow-net-udp-in=domain2/9999

The format can be *, */*, host, host:*, host:port, host/netmask:port,*/port ...(host can be domain or IP).

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 · 2024-06-09T20:48:59Z

Review requested:

@nodejs/net
@nodejs/security-wg

marco-ippolito · 2024-06-10T06:26:07Z

I assume this also impacts the http module

src/cares_wrap.h

theanarkh · 2024-06-10T17:36:42Z

I assume this also impacts the http module

Yes. Or we can skip the permission check when using dns module in Node.js core.

marco-ippolito · 2024-06-10T17:46:51Z

I assume this also impacts the http module

Yes. Or we can skip the permission check when using dns module in Node.js core.

Its very important to document it as I think http is unusable without dns being granted

RafaelGSS

Hey, thanks for working on that.

Would you mind providing some context on why you believe this is useful for end users? In other words, what kind of security would this provide?

Requesting changes just to make sure we are aligned this is something we'd like to see on the Permission Model

doc/api/cli.md

doc/node.1

doc/api/permissions.md

lib/internal/process/pre_execution.js

doc/api/cli.md

ShogunPanda · 2024-06-12T06:47:37Z

I assume this also impacts the http module

Yes. Or we can skip the permission check when using dns module in Node.js core.

Its very important to document it as I think http is unusable without dns being granted

I would say it's 90% unusable. AFAIK node:http does not use DNS internally but node:net does. So unless a person only uses IP based communication (that would be... interesting :D) I think you are 110% right.

Anyway it's not only http. Even the net or tls would make way less sense without DNS.
Do we want to go for --allow-net instead?

theanarkh · 2024-06-12T07:08:37Z

Thank you for reviewing the code. I tried to add permission verification to the network(TCP / UDP / DNS) module. The user can set the access to only certain domain names or IP addresses, for example, to avoid SSRF attacks.

ShogunPanda · 2024-06-12T07:14:37Z

@theanarkh I think I'm losing some context. You said you blocked access but I don't see in the code. Are you saying that you blocked at DNS level, isn't it? If that's the case, I was instead talking to prevent net.connect or net.createServer as well.

marco-ippolito · 2024-06-12T07:15:46Z

I see a lot of value in this PR, as part of adding permission model to the network.
My only concern is documenting behavior and edge cases when interacting with net module.
I probably see it more as part of --allow-net rather then dns by itself

ShogunPanda · 2024-06-12T07:18:15Z

@marco-ippolito I totally agree. If we agree this is only the first PR which will be shortly followed by others for complete coverage of net module, then I'm fine with it.

But I get to choose I'd prefer a single and comprehensive PR that covers both dns and net module in a single CLI flag.

Nonetheless, for clarity I'm not blocking this as it is an amazing piece of work ❤️

theanarkh · 2024-06-12T07:39:29Z

Thanks for all the suggestions, I convert this PR to draft and continue to work on it.

RafaelGSS · 2024-06-12T12:27:28Z

I probably see it more as part of --allow-net rather then dns by itself

I see that too, it's probably more useful to block all network communication instead of specific DNS operations.

ShogunPanda

Few nits but LGTM.

doc/api/cli.md

lib/dgram.js

RafaelGSS

I believe the checks you added to dgram are way more assertive if placed into the native side (udp_wrap). I think we must have to move it here.

RafaelGSS · 2024-06-21T14:24:31Z

doc/api/cli.md

+    at afterDns (node:dgram:337:12)
+    at node:dgram:379:9
+    at process.processTicksAndRejections (node:internal/process/task_queues:77:11) {
+  code: 'ERR_ACCESS_DENIED'


Instead of adding "Permission: bind to 127.0.0.1/9297", we should add a resource field and a permission field to the error object. See how --allow-fs-read works

RafaelGSS · 2024-06-21T14:32:01Z

src/permission/net_permission.cc

+                   &netmask,
+                   result->netmask,
+                   ip_version)) {  // 127.0.0.1/255.255.255.0
+      std::cerr << "invalid netmask: " << result->netmask << std::endl;


It must be Node.js debug log

RafaelGSS · 2024-06-21T14:33:01Z

Good work!

theanarkh · 2024-06-21T17:39:47Z

I believe the checks you added to dgram are way more assertive if placed into the native side (udp_wrap). I think we must have to move it here.

If add permission check in native side, i think we should pass a flags to tell native side if it needs to check the permission. Because the native side should not check the IP if the IP is from dns.lookup(such as --allow-net-udp=localhost).

In addition. If we use UDP in cluster worker(use --allow-net-udp=xxx flag), Node.js will call bind in main process(no permission flag) which will bypass the permission check.

So i think we also need to check the permission in JS layer.

RafaelGSS · 2024-06-23T20:33:01Z

I believe the checks you added to dgram are way more assertive if placed into the native side (udp_wrap). I think we must have to move it here.

If add permission check in native side, i think we should pass a flags to tell native side if it needs to check the permission. Because the native side should not check the IP if the IP is from dns.lookup(such as --allow-net-udp=localhost).

In addition. If we use UDP in cluster worker(use --allow-net-udp=xxx flag), Node.js will call bind in main process(no permission flag) which will bypass the permission check.

So i think we also need to check the permission in JS layer.

Hey, I'll review it carefully over the week. Sorry for the delay. Since it's a security feature, I want to make sure we have covered all caveats as it would expose security vulnerabilities if landed.

RafaelGSS

We need to adjust a few things before landing it. Assuming a regular udpv4 socket server:

const server = udp.createSocket('udp4');
server.on('error', () => {}
server.bind(() => {})

The error object should contain the resource property. In this case, resource will be the address + port.
The error object should contain the permission property. In this case, NetUDP.
How would we handle IPV6 addresses when --allow-net-udp=IPV4/22? Will Octal IP addresses be covered? We must have tests for all those assertions regardless of whether we are supporting this first iteration or not.
You need to rebase on main, since the ERR_ACCESS_DENIED error changed a bit (b9289a6#diff-670bf55805b781d9a3579f8bca9104c04d94af87cc33220149fd7d37b095ca1cR1115).
We should not use std::cerr (iostream) for error messages. Instead, create proper error objects.
What if fd is passed to _createSocketHandle? I don't think we could support it. But, we must document.
Last but not least, can't we move all those checks to the C++ side? I think it's less susceptible to changes that would bypass the UDP Check. For instance, if someone writes a new JS API to create UDP sockets, they might forget to include permission.has check leading to an exposure. If we use the udp_wrap.cc it's more likely to be reused in any new JS API.

Overall, looks really good.

theanarkh · 2024-07-11T17:41:56Z

@RafaelGSS
1,2,4: I have rebased the main branch into my branch.

3: What does the Will Octal IP addresses be covered? mean ? Binding to IPV6 address will throw an error when only allow IPV4 address. For example:

const dgram = require('dgram')
const server = dgram.createSocket('udp4');
server.on('error', console.log)
server.bind(8888, "::1",() => {})

node --experimental-permission --allow-fs-read=./app.js --allow-net-udp=127.0.0.1:8888 app.js. The output is as follows:

Error [ERR_ACCESS_DENIED]: bind to ::1/8888
    at node:dgram:379:18
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11) {
  code: 'ERR_ACCESS_DENIED',
  permission: '::1/8888',
  resource: 'NetUDP'
}

I have updated the code to check the options in EnvironmentOptions::CheckOptions.
Yes. We do not support this, i will update the document.
I can add a check in C++ layer, but i think we can't just check in the C++ layer. For example:

const dgram = require('dgram')
const server = dgram.createSocket('udp4');
server.on('error', console.log)
server.bind(8888, "localhost",() => {})

node --experimental-permission --allow-fs-read=./app.js --allow-net-udp=localhost:8888 app.js

If we only check in C++ layer, the code above will throw an error because we do not allow bind to 127.0.0.1(the result of dns.lookup).

Thanks for your review. I will continue to improve the code and add more tests.

anonrig · 2024-07-11T17:53:16Z

lib/dgram.js

+    }
+  }
+  // Resolve address first
+  state.handle.lookup(address, afterDns);


Why don't we throw the error in .lookup()? This would reduce the duplicated code.

If the address which is allowed is a domain, we should allow bind to all IPs this domain ponit to, so I think we need check before DNS lookup.

lib/dgram.js

src/node_options.cc

src/permission/net_permission.cc

src/permission/net_permission.h

theanarkh · 2024-07-13T18:51:00Z

@anonrig Thanks for your review. I have updated the code and added some replies.

RafaelGSS · 2024-07-22T18:52:00Z

doc/api/cli.md

+    at afterDns (node:dgram:337:12)
+    at node:dgram:379:9
+    at process.processTicksAndRejections (node:internal/process/task_queues:77:11) {
+  code: 'ERR_ACCESS_DENIED'


RafaelGSS · 2024-07-22T18:58:15Z

src/node_options.cc

+
+  // The flag --experimental_permission maybe set after --allow-net-udp
+  // So we just check allow_net_udp.size() here
+  if (allow_net_udp.size() > 0) {


This logic must live on NETPermission::Apply

RafaelGSS · 2024-07-22T19:00:09Z

src/permission/net_permission.cc

+
+namespace permission {
+
+bool ConvertIPToBitset(std::bitset<IPV6_BIT_LEN>* bitset,


We should have a cpp test for this

RafaelGSS · 2024-07-22T19:01:39Z

src/udp_wrap.cc

+  bool checked = true;
+  if (args.Length() > 3 && args[3]->IsBoolean()) {
+    checked = args[3].As<Boolean>()->Value();
+  }


Why is this needed?

RafaelGSS · 2024-07-22T19:02:21Z

src/util.cc

@@ -861,4 +861,59 @@ v8::Maybe<int> GetValidFileMode(Environment* env,
  return v8::Just(mode);
 }

+bool IsDigit(const std::string_view str) {


All these functions needs a cpp test

RafaelGSS · 2024-07-22T19:05:47Z

If we only check in C++ layer, the code above will throw an error because we do not allow bind to 127.0.0.1(the result of dns.lookup).

Yeah, that's expected. In the future, we can check if --allow-net-udp=localhost is passed, we add 127.0.0.1 to the allowed list too

theanarkh · 2024-07-23T15:07:13Z

If we only check in C++ layer, the code above will throw an error because we do not allow bind to 127.0.0.1(the result of dns.lookup).

Yeah, that's expected. In the future, we can check if --allow-net-udp=localhost is passed, we add 127.0.0.1 to the allowed list too

But we can not know the IP of other domains. For example, when --allow-net-udp=nodejs.org is passed(the IPs of nodejs.org is 1.1.1.1 and 2.2.2.2), C++ layer(udp_wrap.cc) will throw an error when check the 1.1.1.1 or 2.2.2.2 which is not allowed.

RafaelGSS · 2024-07-23T20:01:30Z

If we only check in C++ layer, the code above will throw an error because we do not allow bind to 127.0.0.1(the result of dns.lookup).

Yeah, that's expected. In the future, we can check if --allow-net-udp=localhost is passed, we add 127.0.0.1 to the allowed list too

But we can not know the IP of other domains. For example, when --allow-net-udp=nodejs.org is passed(the IPs of nodejs.org is 1.1.1.1 and 2.2.2.2), C++ layer(udp_wrap.cc) will throw an error when check the 1.1.1.1 or 2.2.2.2 which is not allowed.

Why we simply don't resolve the domain in the C++ layer? Something like:

nodejs startup with --allow-net-udp=nodejs.org
NetPermission::Apply resolves nodejs.org to 1.1.1.1 and 2.2.2.2
Store 1.1.1.1 & 2.2.2.2 & nodejs.org to the allowed list

Also, what happens if I pass:

--allow-net-udp=nodejs.org
nodejs.org uses 1.1.1.1 IP Address
After a while nodejs.org changes the IP address (new cloud provider/machine) to 4.4.4.4
Permission Model will compare 4.4.4.4 to 1.1.1.1 and will incorrectly deny access

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 9, 2024

theanarkh force-pushed the add-permission-for-dns branch from de7f6ac to 0fc2a52 Compare June 9, 2024 20:51

theanarkh marked this pull request as draft June 9, 2024 20:58

theanarkh force-pushed the add-permission-for-dns branch 2 times, most recently from 8e44fed to 9c645ef Compare June 9, 2024 23:07

anonrig reviewed Jun 10, 2024

View reviewed changes

src/cares_wrap.h Outdated Show resolved Hide resolved

theanarkh force-pushed the add-permission-for-dns branch from 9c645ef to 67f5b92 Compare June 10, 2024 17:37

theanarkh marked this pull request as ready for review June 10, 2024 17:41

theanarkh force-pushed the add-permission-for-dns branch 3 times, most recently from 9c54926 to 54facc9 Compare June 10, 2024 19:47

RafaelGSS requested changes Jun 11, 2024

View reviewed changes

theanarkh marked this pull request as draft June 12, 2024 07:39

theanarkh changed the title ~~permission,dns: add permission for dns~~ permission: add permission for net Jun 12, 2024

theanarkh force-pushed the add-permission-for-dns branch 3 times, most recently from ea66a82 to ed0b811 Compare June 12, 2024 18:56

theanarkh force-pushed the add-permission-for-dns branch 2 times, most recently from 5cf5136 to 29e2adc Compare June 20, 2024 17:53

theanarkh requested review from RafaelGSS, anonrig, ShogunPanda and marco-ippolito June 21, 2024 02:33

ShogunPanda approved these changes Jun 21, 2024

View reviewed changes

doc/api/cli.md Show resolved Hide resolved

lib/dgram.js Show resolved Hide resolved

lib/dgram.js Show resolved Hide resolved

lib/dgram.js Show resolved Hide resolved

lib/dgram.js Show resolved Hide resolved

lib/dgram.js Show resolved Hide resolved

RafaelGSS reviewed Jun 21, 2024

View reviewed changes

theanarkh requested a review from RafaelGSS June 23, 2024 10:50

RafaelGSS requested changes Jul 10, 2024

View reviewed changes

theanarkh force-pushed the add-permission-for-dns branch 2 times, most recently from 8b0d61d to 457760e Compare July 11, 2024 17:38

anonrig requested changes Jul 11, 2024

View reviewed changes

theanarkh force-pushed the add-permission-for-dns branch 2 times, most recently from f555bd8 to 9f3b13c Compare July 13, 2024 18:48

theanarkh force-pushed the add-permission-for-dns branch 2 times, most recently from d34f32b to d389d9a Compare July 14, 2024 04:52

permission,net,dgram: add permission for net

e4e3cc5

theanarkh force-pushed the add-permission-for-dns branch from d389d9a to e4e3cc5 Compare July 14, 2024 06:03

RafaelGSS requested changes Jul 22, 2024

View reviewed changes

RafaelGSS added the permission Issues and PRs related to the Permission Model label Sep 6, 2024

Kikobeats mentioned this pull request Sep 9, 2024

Add network limit Kikobeats/isolated-function#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

permission: add permission for udp #53398

permission: add permission for udp #53398

theanarkh commented Jun 9, 2024 •

edited

Loading

nodejs-github-bot commented Jun 9, 2024

marco-ippolito commented Jun 10, 2024

theanarkh commented Jun 10, 2024

marco-ippolito commented Jun 10, 2024

RafaelGSS left a comment

ShogunPanda commented Jun 12, 2024

theanarkh commented Jun 12, 2024

ShogunPanda commented Jun 12, 2024

marco-ippolito commented Jun 12, 2024 •

edited

Loading

ShogunPanda commented Jun 12, 2024

theanarkh commented Jun 12, 2024

RafaelGSS commented Jun 12, 2024

ShogunPanda left a comment

RafaelGSS left a comment

RafaelGSS Jun 21, 2024

RafaelGSS Jul 22, 2024

RafaelGSS Jun 21, 2024

RafaelGSS commented Jun 21, 2024

theanarkh commented Jun 21, 2024

RafaelGSS commented Jun 23, 2024

RafaelGSS left a comment

theanarkh commented Jul 11, 2024 •

edited

Loading

anonrig Jul 11, 2024

theanarkh Jul 13, 2024

theanarkh commented Jul 13, 2024

RafaelGSS Jul 22, 2024

RafaelGSS Jul 22, 2024

RafaelGSS Jul 22, 2024

RafaelGSS Jul 22, 2024

RafaelGSS Jul 22, 2024

RafaelGSS commented Jul 22, 2024

theanarkh commented Jul 23, 2024

RafaelGSS commented Jul 23, 2024


		namespace permission {

		bool ConvertIPToBitset(std::bitset<IPV6_BIT_LEN>* bitset,

permission: add permission for udp #53398

Are you sure you want to change the base?

permission: add permission for udp #53398

Conversation

theanarkh commented Jun 9, 2024 • edited Loading

nodejs-github-bot commented Jun 9, 2024

marco-ippolito commented Jun 10, 2024

theanarkh commented Jun 10, 2024

marco-ippolito commented Jun 10, 2024

RafaelGSS left a comment

Choose a reason for hiding this comment

ShogunPanda commented Jun 12, 2024

theanarkh commented Jun 12, 2024

ShogunPanda commented Jun 12, 2024

marco-ippolito commented Jun 12, 2024 • edited Loading

ShogunPanda commented Jun 12, 2024

theanarkh commented Jun 12, 2024

RafaelGSS commented Jun 12, 2024

ShogunPanda left a comment

Choose a reason for hiding this comment

RafaelGSS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafaelGSS commented Jun 21, 2024

theanarkh commented Jun 21, 2024

RafaelGSS commented Jun 23, 2024

RafaelGSS left a comment

Choose a reason for hiding this comment

theanarkh commented Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theanarkh commented Jul 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafaelGSS commented Jul 22, 2024

theanarkh commented Jul 23, 2024

RafaelGSS commented Jul 23, 2024

theanarkh commented Jun 9, 2024 •

edited

Loading

marco-ippolito commented Jun 12, 2024 •

edited

Loading

theanarkh commented Jul 11, 2024 •

edited

Loading