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

net: prevent /32 ipv4 mask from matching all ips #43381

Merged
merged 2 commits into from
Jun 25, 2022

Conversation

supriyo-biswas
Copy link
Contributor

Fixes: #43360

@nodejs-github-bot
Copy link
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++. needs-ci PRs that need a full CI run. labels Jun 11, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Good work!

@@ -272,3 +272,13 @@ const util = require('util');
const ret = util.inspect(blockList, { depth: null });
assert(ret.includes('rules: []'));
}

{
// test for https://github.com/nodejs/node/issues/43360
Copy link
Member

Choose a reason for hiding this comment

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

See the linter issue, capitalize the comment

@supriyo-biswas supriyo-biswas force-pushed the ipv4-32-subnet-mask branch 2 times, most recently from fbeb91a to 539c715 Compare June 11, 2022 15:47
Comment on lines 218 to 221
if (prefix == 32)
return compare_ipv4(ip, net) == SocketAddress::CompareResult::SAME;

uint32_t mask = ((1 << prefix) - 1) << (32 - prefix);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@supriyo-biswas
Copy link
Contributor Author

Added the UB check as @bnoordhuis suggested.

src/node_sockaddr.cc Outdated Show resolved Hide resolved
Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
@tniessen 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
@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 23, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 23, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina removed the needs-ci PRs that need a full CI run. label Jun 25, 2022
@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 25, 2022
@nodejs-github-bot 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 nodejs-github-bot merged commit b6bc44f into nodejs:main Jun 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in b6bc44f

targos pushed a commit that referenced this pull request Jul 12, 2022
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 20, 2022
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
Fixes: #43360

PR-URL: #43381
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Fixes: nodejs/node#43360

PR-URL: nodejs/node#43381
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/32 subnet mask erroneously matches all IP addresses
6 participants