net: prevent /32 ipv4 mask from matching all ips #43381
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
test/parallel/test-blocklist.js
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
See the linter issue, capitalize the comment
supriyo-biswas
force-pushed
the
ipv4-32-subnet-mask
branch
2 times, most recently
from
fbeb91a
to
539c715
Compare
src/node_sockaddr.cc
Outdated
| if (prefix == 32) | ||
| return compare_ipv4(ip, net) == SocketAddress::CompareResult::SAME; | ||
|
|
||
| uint32_t mask = ((1 << prefix) - 1) << (32 - prefix); |
There was a problem hiding this comment.
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:
| 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Doesn't matter, what you prefer. It gets squashed into a single commit before merge anyway.
supriyo-biswas
force-pushed
the
ipv4-32-subnet-mask
branch
from
539c715
to
292d933
Compare
|
Added the UB check as @bnoordhuis suggested. |
Co-authored-by: Ben Noordhuis <[email protected]>
tniessen
added
the
commit-queue-squash
github-actions
bot
removed
the
request-ci
mcollina
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in b6bc44f |
Fixes: #43360 PR-URL: #43381 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Fixes: #43360 PR-URL: #43381 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Fixes: #43360 PR-URL: #43381 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Fixes: nodejs/node#43360 PR-URL: nodejs/node#43381 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Fixes: #43360