net: prevent /32 ipv4 mask from matching all ips by supriyo-biswas · Pull Request #43381 · nodejs/node

supriyo-biswas · 2022-06-11T14:08:22Z

Fixes: #43360

nodejs-github-bot · 2022-06-11T14:08:26Z

Review requested:

@nodejs/net

nodejs-github-bot · 2022-06-11T15:38:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44446/

mcollina

Good work!

mcollina · 2022-06-11T15:39:40Z

test/parallel/test-blocklist.js

 }
+
+{
+  // test for https://github.com/nodejs/node/issues/43360


See the linter issue, capitalize the comment

bnoordhuis · 2022-06-12T20:26:30Z

src/node_sockaddr.cc

+  if (prefix == 32)
+    return compare_ipv4(ip, net) == SocketAddress::CompareResult::SAME;
+
  uint32_t mask = ((1 << prefix) - 1) << (32 - prefix);


Not that your fix is wrong but the mask calculation has undefined behavior (signed out-of-range shift) for prefix > 30 so a more comprehensive fix is this:

Suggested change

if (prefix == 32)

return compare_ipv4(ip, net) == SocketAddress::CompareResult::SAME;

uint32_t mask = ((1 << prefix) - 1) << (32 - prefix);

uint32_t mask = ((1ull << prefix) - 1) << (32 - prefix);

There's another instance around line 296.

prefix arguably shouldn't be an int but e.g. a uint8_t because what does a less-than-zero prefix even mean? That's a bigger change though.

While I agree with your observation, currently there is a check around prefixes < 0 or > 32:

$ ~/node/bin/node > const bl = new net.BlockList() undefined > bl.addSubnet('1.1.0.0', -1, 'ipv4') Uncaught: RangeError [ERR_OUT_OF_RANGE]: The value of "prefix" is out of range. It must be >= 0 && <= 32. Received -1 at __node_internal_captureLargerStackTrace (node:internal/errors:477:5) at new NodeError (node:internal/errors:388:5) at __node_internal_ (node:internal/validators:92:13) at BlockList.addSubnet (node:internal/blocklist:107:9) { code: 'ERR_OUT_OF_RANGE' }

If you still feel that this is required, please let me know.

That's right, and there are also a couple of additional C++ checks that enforce the range but still, switching to uint8_t would help code clarity. That's just a suggestion though, not a hard requirement for this PR to land.

The UB should be fixed though. It's dangerous.

Apologies for the delay on this. What is the recommended approach to add the UB fix on top, should be a separate commit or should the previous commit be amended with the fix?

Doesn't matter, what you prefer. It gets squashed into a single commit before merge anyway.

mcollina

lgtm

Fixes: nodejs#43360

supriyo-biswas · 2022-06-15T17:42:45Z

Added the UB check as @bnoordhuis suggested.

src/node_sockaddr.cc

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot · 2022-06-19T10:59:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44721/

nodejs-github-bot · 2022-06-23T13:48:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44817/

nodejs-github-bot · 2022-06-24T18:24:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44834/

nodejs-github-bot · 2022-06-24T23:15:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44843/

nodejs-github-bot · 2022-06-25T12:47:29Z

Landed in b6bc44f

Fixes: #43360 PR-URL: #43381 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Fixes: nodejs/node#43360 PR-URL: nodejs/node#43381 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 11, 2022

mcollina reviewed Jun 11, 2022

View reviewed changes

supriyo-biswas force-pushed the ipv4-32-subnet-mask branch 2 times, most recently from fbeb91a to 539c715 Compare June 11, 2022 15:47

bnoordhuis reviewed Jun 12, 2022

View reviewed changes

github-actions bot mentioned this pull request Jun 14, 2022

CI Reliability 2022-06-14 nodejs/reliability#300

Open

16 tasks

mcollina approved these changes Jun 15, 2022

View reviewed changes

net: prevent /32 ipv4 mask from matching all ips

292d933

Fixes: nodejs#43360

supriyo-biswas force-pushed the ipv4-32-subnet-mask branch from 539c715 to 292d933 Compare June 15, 2022 17:41

bnoordhuis reviewed Jun 15, 2022

View reviewed changes

src/node_sockaddr.cc Outdated Show resolved Hide resolved

Update src/node_sockaddr.cc

d7dd5db

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

tniessen added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jun 16, 2022

This was referenced Jun 20, 2022

CI Reliability 2022-06-20 nodejs/reliability#306

Open

CI Reliability 2022-06-21 nodejs/reliability#307

Open

CI Reliability 2022-06-22 nodejs/reliability#308

Open

CI Reliability 2022-06-23 nodejs/reliability#309

Open

lpinca approved these changes Jun 23, 2022

View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 23, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 23, 2022

github-actions bot mentioned this pull request Jun 24, 2022

CI Reliability 2022-06-24 nodejs/reliability#310

Open

32 tasks

github-actions bot mentioned this pull request Jun 25, 2022

CI Reliability 2022-06-25 nodejs/reliability#311

Open

32 tasks

mcollina removed the needs-ci PRs that need a full CI run. label Jun 25, 2022

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 25, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 25, 2022

nodejs-github-bot merged commit b6bc44f into nodejs:main Jun 25, 2022

This was referenced Jun 26, 2022

CI Reliability 2022-06-26 nodejs/reliability#312

Open

CI Reliability 2022-06-27 nodejs/reliability#313

Open

CI Reliability 2022-06-28 nodejs/reliability#314

Open

targos pushed a commit that referenced this pull request Jul 12, 2022

net: prevent /32 ipv4 mask from matching all ips

f8bdc53

Fixes: #43360 PR-URL: #43381 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

targos mentioned this pull request Jul 12, 2022

v18.6.0 release proposal #43789

Merged

targos pushed a commit that referenced this pull request Jul 20, 2022

net: prevent /32 ipv4 mask from matching all ips

b8d36e5

Fixes: #43360 PR-URL: #43381 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

targos pushed a commit that referenced this pull request Jul 31, 2022

net: prevent /32 ipv4 mask from matching all ips

265810a

Fixes: #43360 PR-URL: #43381 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Uh oh!

Conversation

supriyo-biswas commented Jun 11, 2022

Uh oh!

nodejs-github-bot commented Jun 11, 2022

Uh oh!

nodejs-github-bot commented Jun 11, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Jun 11, 2022

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jun 12, 2022

Choose a reason for hiding this comment

Uh oh!

supriyo-biswas Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

supriyo-biswas Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

supriyo-biswas commented Jun 15, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 19, 2022

Uh oh!

nodejs-github-bot commented Jun 23, 2022

Uh oh!

nodejs-github-bot commented Jun 24, 2022

Uh oh!

nodejs-github-bot commented Jun 24, 2022

Uh oh!

nodejs-github-bot commented Jun 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants