GeezerVison

Review requested:

• @nodejs/net
• @nodejs/security-wg

I assume this also impacts the http module

I assume this also impacts the http module

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

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 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?

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.

@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.

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

@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 ❤️

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

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.

Good work!

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.

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
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'
}

5. I have updated the code to check the options in EnvironmentOptions::CheckOptions.

6. Yes. We do not support this, i will update the document.

7. 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 Thanks for your review. I have updated the code and added some replies.

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

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.

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

See #58517.